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 <vsement...@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