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 <[email protected]>
>> ---
>> 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 <[email protected].
>
