Re: [PATCH v5 1/5] block/io: fix bdrv_co_block_status_above

[Alberto Garcia]

On Wed 19 Aug 2020 11:48:25 AM CEST, Vladimir Sementsov-Ogievskiy wrote:
>>> +             * The top layer deferred to this layer, and because this 
>>> layer is
>>> +             * short, any zeroes that we synthesize beyond EOF behave as 
>>> if they
>>> +             * were allocated at this layer
>>>                */
>>> +            assert(ret & BDRV_BLOCK_EOF);
>>>               *pnum = bytes;
>>> +            if (file) {
>>> +                *file = p;
>>> +            }
>>> +            return BDRV_BLOCK_ZERO | BDRV_BLOCK_ALLOCATED;
>> 
>> You don't add BDRV_BLOCK_EOF to the return code here ?
>
> No we shouldn't, as this is the end of backing file when the top layer
> is larger.

Ok, but maybe the original request also reaches EOF on the top layer.

An example that does not actually involve short backing files: let's say
that the size of the active image is 1MB and the backing file is 2MB.

We do 'write -z 960k 63k', that zeroes the last cluster minus a 1KB
tail, so qcow2_co_pwrite_zeroes() calls is_zero() to check if that last
1KB already contains zeroes.

bdrv_co_block_status_above() on the top layer returns BDRV_BLOCK_EOF: no
allocation, no zeros, we simply reached EOF. So we go to the backing
image to see what's there. This is also unallocated, there's no backing
file this time and want_zero is set, so this returns BDRV_BLOCK_ZERO.
However the backing file is longer so bdrv_co_block_status() does not
return BDRV_BLOCK_EOF this time.

If the backing file would have been the same size (1MB) we would have
BDRV_BLOCK_ZERO | BDRV_BLOCK_EOF, but if it's longer or (with your
patch) shorter then BDRV_BLOCK_EOF disappears.

Now, I don't see anyone else using BDRV_BLOCK_EOF for anything so this
does not have any practical effect at the moment, but is this worth
fixing?.

>>> +        res = bdrv_block_status_above(bs, NULL, offset, bytes, &nr, NULL, 
>>> NULL);
>>> +        offset += nr;
>>> +        bytes -= nr;
>>> +    } while (res >= 0 && (res & BDRV_BLOCK_ZERO) && nr && bytes);
>> 
>> About this last "... && nr && bytes", I think 'nr' already implies
>> 'bytes', maybe you want to use an assertion instead?
>
> No, on the last iteration, bytes would be 0 and nr is a last
> chunk. So, if we check only nr, we'll do one extra call of
> bdrv_block_status_above with bytes=0, I don't want do it.

You're right !

Berto