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 target image is only being
written to and has nothing to do with those buffers.  Let's fix that.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qemu-img.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 78433f3746..60f4c06487 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3746,8 +3746,8 @@ 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);
+        buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE);
+        buf_new = blk_blockalign(blk_new_backing, IO_BUF_SIZE);
 
         size = blk_getlength(blk);
         if (size < 0) {
-- 
2.31.1

On 01.06.23 21:28, Andrey Drobyshev via 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 target image is only being
> written to and has nothing to do with those buffers.  Let's fix that.

I don’t understand.  The write to the target image does use one of those 
buffers (buf_old, specifically).

This change is correct for buf_new/blk_new_backing, but for buf_old, in 
theory, we need a buffer that fulfills both the alignment requirements 
of blk and blk_old_backing.  (Not that this patch really makes the 
situation worse for buf_old.)

Hanna

On 8/25/23 17:29, Hanna Czenczek wrote:
> On 01.06.23 21:28, Andrey Drobyshev via 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 target image is only being
>> written to and has nothing to do with those buffers.  Let's fix that.
> 
> I don’t understand.  The write to the target image does use one of those
> buffers (buf_old, specifically).
> 
> This change is correct for buf_new/blk_new_backing, but for buf_old, in
> theory, we need a buffer that fulfills both the alignment requirements
> of blk and blk_old_backing.  (Not that this patch really makes the
> situation worse for buf_old.)
> 
> Hanna
> 

Hmm, you're right.  In which case the right thing to do would probably
be smth like:

>          float local_progress = 0;
>  
> -        buf_old = blk_blockalign(blk, IO_BUF_SIZE);
> -        buf_new = blk_blockalign(blk, IO_BUF_SIZE);
> +        if (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);
> +        }
> +        buf_new = blk_blockalign(blk_new_backing, IO_BUF_SIZE);
>  
>          size = blk_getlength(blk);

I'll include this in v2 if you don't have any objections.

On 29.08.23 09:06, Andrey Drobyshev wrote:
> On 8/25/23 17:29, Hanna Czenczek wrote:
>> On 01.06.23 21:28, Andrey Drobyshev via 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 target image is only being
>>> written to and has nothing to do with those buffers.  Let's fix that.
>> I don’t understand.  The write to the target image does use one of those
>> buffers (buf_old, specifically).
>>
>> This change is correct for buf_new/blk_new_backing, but for buf_old, in
>> theory, we need a buffer that fulfills both the alignment requirements
>> of blk and blk_old_backing.  (Not that this patch really makes the
>> situation worse for buf_old.)
>>
>> Hanna
>>
> Hmm, you're right.  In which case the right thing to do would probably
> be smth like:
>
>>           float local_progress = 0;
>>   
>> -        buf_old = blk_blockalign(blk, IO_BUF_SIZE);
>> -        buf_new = blk_blockalign(blk, IO_BUF_SIZE);
>> +        if (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);
>> +        }
>> +        buf_new = blk_blockalign(blk_new_backing, IO_BUF_SIZE);
>>   
>>           size = blk_getlength(blk);
> I'll include this in v2 if you don't have any objections.

Looks good to me, thanks!

On 6/1/23 21:28, 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 target image is only being
> written to and has nothing to do with those buffers.  Let's fix that.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   qemu-img.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 78433f3746..60f4c06487 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3746,8 +3746,8 @@ 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);
> +        buf_old = blk_blockalign(blk_old_backing, IO_BUF_SIZE);
> +        buf_new = blk_blockalign(blk_new_backing, IO_BUF_SIZE);
>   
>           size = blk_getlength(blk);
>           if (size < 0) {
Reviewed-by: Denis V. Lunev <den@openvz.org>