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

[Vladimir Sementsov-Ogievskiy]

19.08.2020 14:34, Alberto Garcia wrote:
> 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?.

That's the point of course, I'll fix.

So, if we get EOF on top layer, we should add it to final ret anyway, 
regardless of backing chain statuses.

> > > > +        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


--
Best regards,
Vladimir