Re: [Qemu-devel] [PATCH 1/3] block: Ignore allocation size in underlying file

[Max Reitz <mreitz@redhat.com>]

Am 10.10.2014 um 13:50 schrieb Benoît Canet:
> The Saturday 16 Aug 2014 à 20:54:16 (+0200), Max Reitz wrote :
>> When falling through to the underlying file in
>> bdrv_co_get_block_status(), do not let the number of sectors for which
>> information could be obtained be overwritten.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 3e252a2..c922664 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3991,9 +3991,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>>       if (bs->file &&
>>           (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
>>           (ret & BDRV_BLOCK_OFFSET_VALID)) {
>> +        int backing_pnum;
>> +
>>           ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
>> -                                        *pnum, pnum);
>> -        if (ret2 >= 0) {
>> +                                        *pnum, &backing_pnum);
>> +        if (ret2 >= 0 && backing_pnum >= *pnum) {
> About backing_pnum >= *pnum.
>
> The documentation of bdrv_co_get_block_status says:
>
>   * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors goes
>   * beyond the end of the disk image it will be clamped.
>   */
> static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>                                                       int64_t sector_num,
>                                                       int nb_sectors, int *pnum)
>
> So clearly after the bdrv_co_get_block_status *pnum >= backing_pnum.
>
> This means that  backing_pnum > *pnum will never happen.
>
> I think either this test is wrong or the doc is wrong.

Thank you for confusing me, I had to think quite a while about this. *g*

The condition is not for error checking. If it was, it would be the 
wrong order (the condition should be true on success, that's why it's 
"ret2 >= 0" and not "ret2 < 0", so it should then be "backing_pnum <= 
*pnum"). So what this is testing is whether all sectors in the 
underlying file in the queried range are read as zero. But if 
"backing_pnum < *pnum" that is not the case, some clusters are not zero. 
So we may not set the zero flag if backing_pnum < *pnum; or as it reads 
in the code, we may only set it if backing_pnum >= *pnum. This is not 
about whether *pnum > backing_pnum, but more about whether backing_pnum 
== *pnum (but >= would be fine, too, if bdrv_co_get_block_status() 
supported it, so that's why I wrote it that way).

However, I'm starting to think about whether it would be better, for the 
backing_pnum < *pnum case, not to not set the zero flag, but rather 
simply set *pnum = backing_pnum. And this in turn would be pretty 
equivalent to just omitting this patch, because:

If we get to this point where we query the underlying file and it 
returns a certain number of sectors is zero; then we therefore want to 
set *pnum = backing_pnum (both if backing_pnum < *pnum and if 
backing_pnum == *pnum; backing_pnum > *pnum cannot happen, as you 
pointed out). On the other hand, if the sectors are not reported to be 
zero, but backing_pnum < *pnum, we want to shorten *pnum accordingly as 
well because this may indicate that after another backing_pnum sectors, 
we arrive at a hole in the file.

There is only one point I can imagine where it makes sense not to let 
backing_pnum overwrite *pnum: And that's if bdrv_co_get_block_status() 
reported BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID with an offset beyond 
the EOF. I think this might actually happen with qcow2, if one cluster 
simply lies beyond the EOF (which is perfectly valid). So I conclude 
that this patch has its use after all but needs to be modified so that 
backing_pnum always overwrites *pnum; except for when backing_pnum is 
zero (which should only happen at or after the EOF) in which case the 
zero flag should be set and *pnum should be left as it was.

And now in all honesty: Thanks for confusing me, I guess I can think 
better when I'm confused. :-)

Max

> Best regards
>
> Benoît
>
>
>>               /* Ignore errors.  This is just providing extra information, it
>>                * is useful but not necessary.
>>                */
>> -- 
>> 2.0.4
>>
>>