| Message ID | 20200916122008.20303-3-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix & merge block_status_above and is_allocated_above | expand |
On Wed 16 Sep 2020 02:20:05 PM CEST, Vladimir Sementsov-Ogievskiy wrote: > - for (p = backing_bs(bs); p != base; p = backing_bs(p)) { > + for (p = backing_bs(bs); include_base || p != base; p = backing_bs(p)) { > ret = bdrv_co_block_status(p, want_zero, offset, bytes, pnum, map, > file); > if (ret < 0) { > @@ -2420,6 +2422,11 @@ bdrv_co_common_block_status_above(BlockDriverState *bs, > break; > } > > + if (p == base) { > + assert(include_base); > + break; > + } > + Another option is something like: BlockDriverState *last_bs = include_base ? base : backing_bs(base); and you get a simpler 'for' loop. But why do we need include_base at all? Can't the caller just pass backing_bs(base) instead? I'm talking also about the existing case of bdrv_is_allocated_above(). Berto
23.09.2020 19:18, Alberto Garcia wrote: > On Wed 16 Sep 2020 02:20:05 PM CEST, Vladimir Sementsov-Ogievskiy wrote: >> - for (p = backing_bs(bs); p != base; p = backing_bs(p)) { >> + for (p = backing_bs(bs); include_base || p != base; p = backing_bs(p)) { >> ret = bdrv_co_block_status(p, want_zero, offset, bytes, pnum, map, >> file); >> if (ret < 0) { >> @@ -2420,6 +2422,11 @@ bdrv_co_common_block_status_above(BlockDriverState *bs, >> break; >> } >> >> + if (p == base) { >> + assert(include_base); >> + break; >> + } >> + > > Another option is something like: > > BlockDriverState *last_bs = include_base ? base : backing_bs(base); hmm, in case when include_base is false, last bs is not backing_bs(base) but the parent of base. > > and you get a simpler 'for' loop. > > But why do we need include_base at all? Can't the caller just pass > backing_bs(base) instead? I'm talking also about the existing case of > bdrv_is_allocated_above(). > include_base was introduced for the case when caller doesn't own backing_bs(base), and therefore shouldn't do operations that may yield (block_status can) dependent on backing_bs(base). In particular, in block stream, where link to base is not frozen.
On Wed 23 Sep 2020 07:11:57 PM CEST, Vladimir Sementsov-Ogievskiy wrote: >> BlockDriverState *last_bs = include_base ? base : backing_bs(base); > > hmm, in case when include_base is false, last bs is not > backing_bs(base) but the parent of base. Oops, yes, it should be the other way around %-) >> But why do we need include_base at all? Can't the caller just pass >> backing_bs(base) instead? I'm talking also about the existing case of >> bdrv_is_allocated_above(). > > include_base was introduced for the case when caller doesn't own > backing_bs(base), and therefore shouldn't do operations that may yield > (block_status can) dependent on backing_bs(base). In particular, in > block stream, where link to base is not frozen. You're right, thanks! Berto
On Wed 16 Sep 2020 02:20:05 PM CEST, Vladimir Sementsov-Ogievskiy wrote: > In order to reuse bdrv_common_block_status_above in > bdrv_is_allocated_above, let's support include_base parameter. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
diff --git a/block/coroutines.h b/block/coroutines.h index f69179f5ef..1cb3128b94 100644 --- a/block/coroutines.h +++ b/block/coroutines.h @@ -41,6 +41,7 @@ bdrv_pwritev(BdrvChild *child, int64_t offset, unsigned int bytes, int coroutine_fn bdrv_co_common_block_status_above(BlockDriverState *bs, BlockDriverState *base, + bool include_base, bool want_zero, int64_t offset, int64_t bytes, @@ -50,6 +51,7 @@ bdrv_co_common_block_status_above(BlockDriverState *bs, int generated_co_wrapper bdrv_common_block_status_above(BlockDriverState *bs, BlockDriverState *base, + bool include_base, bool want_zero, int64_t offset, int64_t bytes, diff --git a/block/io.c b/block/io.c index e381d2da35..0cc2dd7a3e 100644 --- a/block/io.c +++ b/block/io.c @@ -2359,6 +2359,7 @@ early_out: int coroutine_fn bdrv_co_common_block_status_above(BlockDriverState *bs, BlockDriverState *base, + bool include_base, bool want_zero, int64_t offset, int64_t bytes, @@ -2370,10 +2371,11 @@ bdrv_co_common_block_status_above(BlockDriverState *bs, BlockDriverState *p; int64_t eof = 0; - assert(bs != base); + assert(include_base || bs != base); + assert(!include_base || base); /* Can't include NULL base */ ret = bdrv_co_block_status(bs, want_zero, offset, bytes, pnum, map, file); - if (ret < 0 || *pnum == 0 || ret & BDRV_BLOCK_ALLOCATED) { + if (ret < 0 || *pnum == 0 || ret & BDRV_BLOCK_ALLOCATED || bs == base) { return ret; } @@ -2384,7 +2386,7 @@ bdrv_co_common_block_status_above(BlockDriverState *bs, assert(*pnum <= bytes); bytes = *pnum; - for (p = backing_bs(bs); p != base; p = backing_bs(p)) { + for (p = backing_bs(bs); include_base || p != base; p = backing_bs(p)) { ret = bdrv_co_block_status(p, want_zero, offset, bytes, pnum, map, file); if (ret < 0) { @@ -2420,6 +2422,11 @@ bdrv_co_common_block_status_above(BlockDriverState *bs, break; } + if (p == base) { + assert(include_base); + break; + } + /* * OK, [offset, offset + *pnum) region is unallocated on this layer, * let's continue the diving. @@ -2439,7 +2446,7 @@ int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base, int64_t offset, int64_t bytes, int64_t *pnum, int64_t *map, BlockDriverState **file) { - return bdrv_common_block_status_above(bs, base, true, offset, bytes, + return bdrv_common_block_status_above(bs, base, false, true, offset, bytes, pnum, map, file); } @@ -2456,7 +2463,7 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int ret; int64_t dummy; - ret = bdrv_common_block_status_above(bs, backing_bs(bs), false, offset, + ret = bdrv_common_block_status_above(bs, bs, true, false, offset, bytes, pnum ? pnum : &dummy, NULL, NULL); if (ret < 0) {
In order to reuse bdrv_common_block_status_above in bdrv_is_allocated_above, let's support include_base parameter. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/coroutines.h | 2 ++ block/io.c | 17 ++++++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-)