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"? > 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)? > *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. > - bytes = MIN(bytes, *pnum); > + > + /* Proceed to backing */ > + assert(*pnum <= bytes); > + bytes = *pnum; > first = false; > } > + > return ret; > } Kevin