Re: [PATCH 0/4] fix & merge block_status_above and is_allocated_above

[Vladimir Sementsov-Ogievskiy]

19.11.2019 19:58, Stefan Hajnoczi wrote:
> On Sat, Nov 16, 2019 at 07:34:06PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> I wanted to understand, what is the real difference between 
>> bdrv_block_status_above
>> and bdrv_is_allocated_above, IMHO bdrv_is_allocated_above should work through
>> bdrv_block_status_above..
>>
>> And I found the problem: bdrv_is_allocated_above considers space after EOF as
>> UNALLOCATED for intermediate nodes..
>>
>> UNALLOCATED is not about allocation at fs level, but about should we go to 
>> backing or
>> not.. And it seems incorrect for me, as in case of short backing file, we'll 
>> read
>> zeroes after EOF, instead of going further by backing chain.
>>
>> This leads to the following effect:
>>
>> ./qemu-img create -f qcow2 base.qcow2 2M
>> ./qemu-io -c "write -P 0x1 0 2M" base.qcow2
>>
>> ./qemu-img create -f qcow2 -b base.qcow2 mid.qcow2 1M
>> ./qemu-img create -f qcow2 -b mid.qcow2 top.qcow2 2M
>>
>> Region 1M..2M is shadowed by short middle image, so guest sees zeroes:
>> ./qemu-io -c "read -P 0 1M 1M" top.qcow2
>> read 1048576/1048576 bytes at offset 1048576
>> 1 MiB, 1 ops; 00.00 sec (22.795 GiB/sec and 23341.5807 ops/sec)
>>
>> But after commit guest visible state is changed, which seems wrong for me:
>> ./qemu-img commit top.qcow2 -b mid.qcow2
>>
>> ./qemu-io -c "read -P 0 1M 1M" mid.qcow2
>> Pattern verification failed at offset 1048576, 1048576 bytes
>> read 1048576/1048576 bytes at offset 1048576
>> 1 MiB, 1 ops; 00.00 sec (4.981 GiB/sec and 5100.4794 ops/sec)
>>
>> ./qemu-io -c "read -P 1 1M 1M" mid.qcow2
>> read 1048576/1048576 bytes at offset 1048576
>> 1 MiB, 1 ops; 00.00 sec (3.365 GiB/sec and 3446.1606 ops/sec)
>>
>>
>> I don't know, is it a real bug, as I don't know, do we support backing file 
>> larger than
>> its parent. Still, I'm not sure that this behavior of 
>> bdrv_is_allocated_above don't lead
>> to other problems.
> 
> It seems correct that a backing file limits the virtual disk size of its
> backing chain.
> 
> The "qemu-img commit" behavior seems counter-intuitive at first, but the
> problem there is that the top-most image file is larger than its backing
> file - not that the backing chain has differing sizes.  Committing
> should not lose data, mid.qcow2 will be grown and then you get the
> result you've observed.
> 
> I consider this a little weird but not a bug.  Does anyone else have
> opinions?
> 

I thing, that the fact that read and block_status treats space after EOF
in different ways is a bug anyway.

Also, block_status behavior don't correspond to our definition of
BDRV_BLOCK_ALLOCATED (see Kevin's response)

Also, as I explained in parallel branch, such situation is possible for example
when we interleave incremental backup creation with vm disk resize.

So, then, we have incremental backup chain with different sizes like in
example. Assume we want to merge some sequential incremental backups into
one (aka, remove incremental backup), it may be done with help of commit.
And we get final image, where we see data from base backup instead of
zeros. This is illegal, vm should not get access to this data.

Possibly, similar thing is possible with snapshots.

-- 
Best regards,
Vladimir