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

[Hanna Czenczek <hreitz@redhat.com>]

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!