Re: [PATCH 1/4] block/io: fix bdrv_co_block_status_above

[Kevin Wolf <kwolf@redhat.com>]

Am 26.11.2019 um 08:26 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 25.11.2019 19:00, Kevin Wolf wrote:
> > Am 16.11.2019 um 17:34 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> bdrv_co_block_status_above has several problems with handling short
> >> backing files:
> >>
> >> 1. With want_zeros=true, it may return ret with BDRV_BLOCK_ZERO but
> >> without BDRV_BLOCK_ALLOCATED flag, when actually short backing file
> >> which produces these after-EOF zeros is inside requested backing
> >> sequesnce.
> > 
> > s/sequesnce/sequence/
> > 
> >>
> >> 2. With want_zeros=false, it will just stop inside requested region, if
> >> we have unallocated region in top node when underlying backing is
> >> short.
> > 
> > I honestly don't understand this one. Can you rephrase/explain in more
> > detail what you mean by "stop inside [the] requested region"?
> 
> Hmm, yes, bad description. I mean, it may return pnum=0 prior to actual EOF,
> because of EOF of short backing file.

Ah, yes, that's true. Definitely mention pnum=0 in the comment, this
explanation is much clearer.

> >> Fix these things, making logic about short backing files clearer.
> >>
> >> Note that 154 output changed, because now bdrv_block_status_above don't
> >> merge unallocated zeros with zeros after EOF (which are actually
> >> "allocated" in POV of read from backing-chain top) and is_zero() just
> >> don't understand that the whole head or tail is zero. We may update
> >> is_zero to call bdrv_block_status_above several times, or add flag to
> >> bdrv_block_status_above that we are not interested in ALLOCATED flag,
> >> so ranges with different ALLOCATED status may be merged, but actually,
> >> it seems that we'd better don't care about this corner case.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >> ---
> >>   block/io.c                 | 41 ++++++++++++++++++++++++++++----------
> >>   tests/qemu-iotests/154.out |  4 ++--
> >>   2 files changed, 32 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/block/io.c b/block/io.c
> >> index f75777f5ea..4d7fa99bd2 100644
> >> --- a/block/io.c
> >> +++ b/block/io.c
> >> @@ -2434,25 +2434,44 @@ static int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
> >>           ret = bdrv_co_block_status(p, want_zero, offset, bytes, pnum, map,
> >>                                      file);
> >>           if (ret < 0) {
> >> -            break;
> >> +            return ret;
> >>           }
> >> -        if (ret & BDRV_BLOCK_ZERO && ret & BDRV_BLOCK_EOF && !first) {
> >> +        if (*pnum == 0) {
> >> +            if (first) {
> >> +                return ret;
> >> +            }
> >> +
> >>               /*
> >> -             * Reading beyond the end of the file continues to read
> >> -             * zeroes, but we can only widen the result to the
> >> -             * unallocated length we learned from an earlier
> >> -             * iteration.
> >> +             * Reads from bs for selected region will return zeroes, produced
> >> +             * because current level is short. We should consider it as
> >> +             * allocated.
> > 
> > "the selected region"
> > "the current level"
> > 
> >> +             * TODO: Should we report p as file here?
> > 
> > I think that would make sense.
> > 
> >>                */
> >> +            assert(ret & BDRV_BLOCK_EOF);
> > 
> > Can this assertion be moved above the if (first)?
> 
> it may correspond to requested bytes==0.. But we can check it separately
> before for loop and move this assertion.

Ah, right. Don't bother then, it's fine either way.

Kevin