[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 3/8] qemu-img: rebase: use backing files' BlockBackend for
From: |
Eric Blake |
Subject: |
Re: [PATCH v2 3/8] qemu-img: rebase: use backing files' BlockBackend for buffer alignment |
Date: |
Fri, 15 Sep 2023 13:39:35 -0500 |
User-agent: |
NeoMutt/20230517 |
On Fri, Sep 15, 2023 at 07:20:11PM +0300, Andrey Drobyshev wrote:
> Since commit bb1c05973cf ("qemu-img: Use qemu_blockalign"), buffers for
> the data read from the old and new backing files are aligned using
> BlockDriverState (or BlockBackend later on) referring to the target image.
> However, this isn't quite right, because buf_new is only being used for
> reading from the new backing, while buf_old is being used for both reading
> from the old backing and writing to the target. Let's take that into account
> and use more appropriate values as alignments.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
> qemu-img.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 50660ba920..d12e4a4753 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3750,8 +3750,13 @@ static int img_rebase(int argc, char **argv)
> int64_t n;
> float local_progress = 0;
>
> - buf_old = blk_blockalign(blk, IO_BUF_SIZE);
> - buf_new = blk_blockalign(blk, IO_BUF_SIZE);
> + if (blk_old_backing && bdrv_opt_mem_align(blk_bs(blk)) >
> + bdrv_opt_mem_align(blk_bs(blk_old_backing))) {
> + buf_old = blk_blockalign(blk, IO_BUF_SIZE);
> + } else {
> + buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE);
> + }
Since bdrv_opt_mem_align(NULL) is safe, could we just simplify this to:
buf_old = qemu_memalign(MAX(bdrv_opt_mem_align(blk_old_backing),
bdrv_opt_mem_align(blk)), IO_BUF_SIZE);
instead of going through an if statement? Or is the problem that
bdrv_opt_mem_align(NULL) can return the host page size (perhaps 64k),
which may be larger than technically needed in some scenarios?
> + buf_new = blk_blockalign(blk_new_backing, IO_BUF_SIZE);
>
> size = blk_getlength(blk);
> if (size < 0) {
> --
> 2.39.3
At any rate, aligning the buffers by how they will be used makes sense
(if the destination blk has looser requirements than the source
blk_old_backing, then accesses into blk_old are suspect).
Reviewed-by: Eric Blake <eblake@redhat.com.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
- [PATCH v2 1/8] qemu-img: rebase: stop when reaching EOF of old backing file, (continued)
- [PATCH v2 1/8] qemu-img: rebase: stop when reaching EOF of old backing file, Andrey Drobyshev, 2023/09/15
- [PATCH v2 8/8] iotests: add tests for "qemu-img rebase" with compression, Andrey Drobyshev, 2023/09/15
- [PATCH v2 4/8] qemu-img: add chunk size parameter to compare_buffers(), Andrey Drobyshev, 2023/09/15
- [PATCH v2 5/8] qemu-img: rebase: avoid unnecessary COW operations, Andrey Drobyshev, 2023/09/15
- [PATCH v2 6/8] iotests/{024, 271}: add testcases for qemu-img rebase, Andrey Drobyshev, 2023/09/15
- [PATCH v2 3/8] qemu-img: rebase: use backing files' BlockBackend for buffer alignment, Andrey Drobyshev, 2023/09/15