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.
