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. > >> 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. > >> *pnum = bytes; >> + return BDRV_BLOCK_ZERO | BDRV_BLOCK_ALLOCATED; >> } >> - if (ret & (BDRV_BLOCK_ZERO | BDRV_BLOCK_DATA)) { >> - break; >> + if (ret & BDRV_BLOCK_ALLOCATED) { >> + /* We've found the node and the status, we must return. */ >> + >> + if (ret & BDRV_BLOCK_ZERO && ret & BDRV_BLOCK_EOF && !first) { >> + /* >> + * This level also responsible for reads after EOF inside >> + * unallocated region in previous level. > > "is also responsible" > "the unallocated region in the previous level" > >> + */ >> + *pnum = bytes; >> + } >> + >> + return ret; >> } >> - /* [offset, pnum] unallocated on this layer, which could be only >> - * the first part of [offset, bytes]. */ > > Any reason for deleting this comment? I think it's still valid. Hmm, I decided that it's obvious and shorter comment is enough. I can leave it, of course. > >> - bytes = MIN(bytes, *pnum); >> + >> + /* Proceed to backing */ >> + assert(*pnum <= bytes); >> + bytes = *pnum; >> first = false; >> } >> + >> return ret; >> } > > Kevin > -- Best regards, Vladimir