On 9/15/23 21:39, Eric Blake wrote: > 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.drobys...@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? >
Although bdrv_opt_mem_align(NULL) is safe, blk_bs(NULL) is not. And bdrv_opt_mem_align() takes BlockDriverState* not BlockBackend*, so we would have to perform the same check and there would be no simplification. >> + 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 <ebl...@redhat.com. >