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.