| Message ID | 20201023183652.478921-11-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Exposing backing-chain allocation over NBD | expand |
23.10.2020 21:36, Eric Blake wrote: > When checking for allocation across a chain, it's already easy to > count the depth within the chain at which the allocation is found. > Instead of throwing that information away, return it to the caller. > Existing callers only cared about allocated/non-allocated, but having > a depth available will be used by NBD in the next patch. > > Note that the previous code (since commit 188a7bbf94 in 2012) was > lazy: for each layer deeper in the backing chain, it was issuing a > bdrv_is_allocated request on the original 'bytes' amount, rather than > on any smaller amount 'n' learned from an upper layer. These > semantics happened to work, since if you have: > > Base <- Active > XX-- -XX- > > the consecutive results are offset 0: '11' with *pnum 2, followed by > offset 2: '1' with *pnum 1, followed by offset 3: '0' with *pnum 1; > the resulting sequence 1110 matches reality (the first three clusters > are indeed allocated somewhere in the given range). But post-patch, > we correctly give the results offset 0: '2' with *pnum 1, followed by > offset 1: '11' with *pnum 2, followed by offset 3: '0' with *pnum 1 > (2110), without over-reporting the length of contributions from the > backing layers. > > Signed-off-by: Eric Blake <eblake@redhat.com> This conflicts with block-status/is-allocated fix/merge patches in Stefan's "[PULL v3 00/28] Block patches" -- Best regards, Vladimir
24.10.2020 12:49, Vladimir Sementsov-Ogievskiy wrote: > 23.10.2020 21:36, Eric Blake wrote: >> When checking for allocation across a chain, it's already easy to >> count the depth within the chain at which the allocation is found. >> Instead of throwing that information away, return it to the caller. >> Existing callers only cared about allocated/non-allocated, but having >> a depth available will be used by NBD in the next patch. >> >> Note that the previous code (since commit 188a7bbf94 in 2012) was >> lazy: for each layer deeper in the backing chain, it was issuing a >> bdrv_is_allocated request on the original 'bytes' amount, rather than >> on any smaller amount 'n' learned from an upper layer. These >> semantics happened to work, since if you have: >> >> Base <- Active >> XX-- -XX- >> >> the consecutive results are offset 0: '11' with *pnum 2, followed by >> offset 2: '1' with *pnum 1, followed by offset 3: '0' with *pnum 1; >> the resulting sequence 1110 matches reality (the first three clusters >> are indeed allocated somewhere in the given range). But post-patch, >> we correctly give the results offset 0: '2' with *pnum 1, followed by >> offset 1: '11' with *pnum 2, followed by offset 3: '0' with *pnum 1 >> (2110), without over-reporting the length of contributions from the >> backing layers. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > This conflicts with block-status/is-allocated fix/merge patches in Stefan's "[PULL v3 00/28] Block patches" > the patches applied to master branch -- Best regards, Vladimir
diff --git a/block/io.c b/block/io.c index 54f0968aee27..4a4fa1c9ab1b 100644 --- a/block/io.c +++ b/block/io.c @@ -2414,8 +2414,9 @@ int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset, /* * Given an image chain: ... -> [BASE] -> [INTER1] -> [INTER2] -> [TOP] * - * Return 1 if (a prefix of) the given range is allocated in any image - * between BASE and TOP (BASE is only included if include_base is set). + * Return a positive depth if (a prefix of) the given range is allocated + * in any image between BASE and TOP (BASE is only included if include_base + * is set). Depth 1 is TOP, 2 is the first backing layer, and so forth. * BASE can be NULL to check if the given offset is allocated in any * image of the chain. Return 0 otherwise, or negative errno on * failure. @@ -2435,6 +2436,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, { BlockDriverState *intermediate; int ret; + int depth = 0; int64_t n = bytes; assert(base || !include_base); @@ -2444,14 +2446,15 @@ int bdrv_is_allocated_above(BlockDriverState *top, int64_t pnum_inter; int64_t size_inter; + depth++; assert(intermediate); - ret = bdrv_is_allocated(intermediate, offset, bytes, &pnum_inter); + ret = bdrv_is_allocated(intermediate, offset, n, &pnum_inter); if (ret < 0) { return ret; } if (ret) { *pnum = pnum_inter; - return 1; + return depth; } size_inter = bdrv_getlength(intermediate); diff --git a/block/commit.c b/block/commit.c index 1e85c306cc41..71db7ba7472e 100644 --- a/block/commit.c +++ b/block/commit.c @@ -156,7 +156,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp) /* Copy if allocated above the base */ ret = bdrv_is_allocated_above(blk_bs(s->top), s->base_overlay, true, offset, COMMIT_BUFFER_SIZE, &n); - copy = (ret == 1); + copy = (ret > 0); trace_commit_one_iteration(s, offset, n, ret); if (copy) { assert(n < SIZE_MAX); diff --git a/block/mirror.c b/block/mirror.c index 26acf4af6fb7..8e1ad6eceb57 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -846,7 +846,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) } assert(count); - if (ret == 1) { + if (ret > 0) { bdrv_set_dirty_bitmap(s->dirty_bitmap, offset, count); } offset += count; diff --git a/block/stream.c b/block/stream.c index 8ce6729a33da..236384f2f739 100644 --- a/block/stream.c +++ b/block/stream.c @@ -167,7 +167,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp) n = len - offset; } - copy = (ret == 1); + copy = (ret > 0); } trace_stream_one_iteration(s, offset, n, ret); if (copy) {
When checking for allocation across a chain, it's already easy to count the depth within the chain at which the allocation is found. Instead of throwing that information away, return it to the caller. Existing callers only cared about allocated/non-allocated, but having a depth available will be used by NBD in the next patch. Note that the previous code (since commit 188a7bbf94 in 2012) was lazy: for each layer deeper in the backing chain, it was issuing a bdrv_is_allocated request on the original 'bytes' amount, rather than on any smaller amount 'n' learned from an upper layer. These semantics happened to work, since if you have: Base <- Active XX-- -XX- the consecutive results are offset 0: '11' with *pnum 2, followed by offset 2: '1' with *pnum 1, followed by offset 3: '0' with *pnum 1; the resulting sequence 1110 matches reality (the first three clusters are indeed allocated somewhere in the given range). But post-patch, we correctly give the results offset 0: '2' with *pnum 1, followed by offset 1: '11' with *pnum 2, followed by offset 3: '0' with *pnum 1 (2110), without over-reporting the length of contributions from the backing layers. Signed-off-by: Eric Blake <eblake@redhat.com> --- block/io.c | 11 +++++++---- block/commit.c | 2 +- block/mirror.c | 2 +- block/stream.c | 2 +- 4 files changed, 10 insertions(+), 7 deletions(-)