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
