Re: [PATCH v2 3/8] qemu-img: rebase: use backing files' BlockBackend for buffer alignment

[Eric Blake]

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