Re: [PATCH v2 5/5] iotests: add commit top->base cases to 274

[Eric Blake]

On 5/19/20 2:55 PM, Vladimir Sementsov-Ogievskiy wrote:
> These cases are fixed by previous patches around block_status and
> is_allocated.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>   tests/qemu-iotests/274     | 20 ++++++++++++
>   tests/qemu-iotests/274.out | 65 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 85 insertions(+)

Okay, so this test fails when applied in isolation without the rest of 
your series.

> diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274
> index 5d1bf34dff..e910455f13 100755
> --- a/tests/qemu-iotests/274
> +++ b/tests/qemu-iotests/274
> @@ -115,6 +115,26 @@ with iotests.FilePath('base') as base, \
>       iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
>       iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), 
> mid)
>   
> +    iotests.log('=== Testing qemu-img commit (top -> base) ===')
> +
> +    create_chain()
> +    iotests.qemu_img_log('commit', '-b', base, top)
> +    iotests.img_info_log(base)
> +    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, base)
> +    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), 
> base)

So if I understand it, we are going from:

base    11111111
mid     ----
top     --------
guest   11110000

and we want to go to:

base    11110000

except that we are not properly writing the zeroes into base, because we 
grabbed the wrong status, ending up with:

base    11111111

The status of top from 1M onwards is unallocated, and if we were to 
commit to just mid, Kevin's truncate fixes solve that (we now zero out 
the tail of mid as part of resizing it to be large enough).  But you are 
instead skipping mid, and committing all the way to base.  So we need 
_something_ that can tell qemu-img commit that even though the region 
1m-2m is unallocated in top, we must behave as though the status of mid 
reports it as allocated (because when reading beyond EOF in mid, we DO 
read zero).  Since the data is allocated not in top, but acts as though 
it was allocated in mid, which is above base, then the commit operation 
has to do something to preserve that allocation.

Okay, you've convinced me we have a bug.  However, I'm still not sold 
that patches 1 and 4 are quite the right fix.  Going back to the 
original setup, unpatched qemu.git head reports:

$ ./qemu-img map --output=json top.qcow2
[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data": 
true, "offset": 327680},
{ "start": 1048576, "length": 1048576, "depth": 0, "zero": true, "data": 
false}]

I think what we really want is:

[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data": 
true, "offset": 327680},
{ "start": 1048576, "length": 1048576, "depth": 1, "zero": true, "data": 
false}]

because then we would be _accurately_ reporting that the zeroes that we 
read from 1m-2m come _because_ we read from mid (beyond EOF), which is 
different from our current answer that the zeroes come from top (they 
don't, because top deferred to mid).  If we fix up qemu-img map output 
to correctly report zeroes beyond EOF from the correct layer, will that 
also fix up the bug we are seeing in qemu-img commit?

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org