Re: [PATCH v6 2/5] block/io: bdrv_common_block_status_above: support include_base
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 Reviewed-by: Alberto Garcia Berto
Re: [PATCH v6 2/5] block/io: bdrv_common_block_status_above: support include_base
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
Re: [PATCH v6 2/5] block/io: bdrv_common_block_status_above: support include_base
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.
--
Best regards,
Vladimir
Re: [PATCH v6 2/5] block/io: bdrv_common_block_status_above: support include_base
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
[PATCH v6 2/5] block/io: bdrv_common_block_status_above: support include_base
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
---
block/coroutines.h | 2 ++
block/io.c | 17 -
2 files changed, 14 insertions(+), 5 deletions(-)
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) {
--
2.21.3
