| Message ID | 20231027155333.420094-10-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Graph locking part 6 (bs->file/backing) | expand |
On Fri, Oct 27, 2023 at 05:53:18PM +0200, Kevin Wolf wrote: > This adds GRAPH_RDLOCK annotations to declare that callers of > bdrv_(un)freeze_backing_chain() need to hold a reader lock for the > graph because it calls bdrv_filter_or_cow_child(), which accesses > bs->file/backing. > > Use the opportunity to make bdrv_is_backing_chain_frozen() static, it > has no external callers. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/copy-on-read.h | 3 ++- > include/block/block-global-state.h | 11 ++++++----- > block.c | 5 +++-- > block/commit.c | 6 ++++++ > block/copy-on-read.c | 19 +++++++++++++++---- > block/mirror.c | 3 +++ > block/stream.c | 16 +++++++++++----- > 7 files changed, 46 insertions(+), 17 deletions(-) > ... > +++ b/block/copy-on-read.c ... > -static void cor_close(BlockDriverState *bs) > +static void GRAPH_UNLOCKED cor_close(BlockDriverState *bs) > { > BDRVStateCOR *s = bs->opaque; > > + GLOBAL_STATE_CODE(); > + > if (s->chain_frozen) { > + bdrv_graph_rdlock_main_loop(); > s->chain_frozen = false; > bdrv_unfreeze_backing_chain(bs, s->bottom_bs); > + bdrv_graph_rdunlock_main_loop(); Why the two-line addition here... > } > > bdrv_unref(s->bottom_bs); > @@ -263,12 +271,15 @@ static BlockDriver bdrv_copy_on_read = { > }; > > > -void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs) > +void no_coroutine_fn bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs) > { > BDRVStateCOR *s = cor_filter_bs->opaque; > > + GLOBAL_STATE_CODE(); > + > /* unfreeze, as otherwise bdrv_replace_node() will fail */ > if (s->chain_frozen) { > + GRAPH_RDLOCK_GUARD_MAINLOOP(); > s->chain_frozen = false; > bdrv_unfreeze_backing_chain(cor_filter_bs, s->bottom_bs); > } ...vs. the magic one-line per-scope change here? Both work, so I don't see any problems, but it does seem odd to mix styles in the same patch. (I can see other places where you have intentionally picked the version that required the least reindenting; adding a scope just to use GRAPH_RDLOCK_GUARD_MAINLOOP() without having to carefully pair an unlock on every early exit path is fewer lines of code overall, but more lines of churn in the patch itself.) Reviewed-by: Eric Blake <eblake@redhat.com>
Am 27.10.2023 um 23:00 hat Eric Blake geschrieben: > On Fri, Oct 27, 2023 at 05:53:18PM +0200, Kevin Wolf wrote: > > This adds GRAPH_RDLOCK annotations to declare that callers of > > bdrv_(un)freeze_backing_chain() need to hold a reader lock for the > > graph because it calls bdrv_filter_or_cow_child(), which accesses > > bs->file/backing. > > > > Use the opportunity to make bdrv_is_backing_chain_frozen() static, it > > has no external callers. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block/copy-on-read.h | 3 ++- > > include/block/block-global-state.h | 11 ++++++----- > > block.c | 5 +++-- > > block/commit.c | 6 ++++++ > > block/copy-on-read.c | 19 +++++++++++++++---- > > block/mirror.c | 3 +++ > > block/stream.c | 16 +++++++++++----- > > 7 files changed, 46 insertions(+), 17 deletions(-) > > > ... > > +++ b/block/copy-on-read.c > ... > > -static void cor_close(BlockDriverState *bs) > > +static void GRAPH_UNLOCKED cor_close(BlockDriverState *bs) > > { > > BDRVStateCOR *s = bs->opaque; > > > > + GLOBAL_STATE_CODE(); > > + > > if (s->chain_frozen) { > > + bdrv_graph_rdlock_main_loop(); > > s->chain_frozen = false; > > bdrv_unfreeze_backing_chain(bs, s->bottom_bs); > > + bdrv_graph_rdunlock_main_loop(); > > Why the two-line addition here... > > > } > > > > bdrv_unref(s->bottom_bs); I don't remember if there were more reasons without having a closer look at the code, but just here from the context, bdrv_unref() is supposed to be called without the graph lock held. We don't enforce this yet because there are some cases that are not easy to fix, but I don't want to make adding the GRAPH_UNLOCKED annotation to bdrv_unref() harder than it needs to be. > > @@ -263,12 +271,15 @@ static BlockDriver bdrv_copy_on_read = { > > }; > > > > > > -void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs) > > +void no_coroutine_fn bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs) > > { > > BDRVStateCOR *s = cor_filter_bs->opaque; > > > > + GLOBAL_STATE_CODE(); > > + > > /* unfreeze, as otherwise bdrv_replace_node() will fail */ > > if (s->chain_frozen) { > > + GRAPH_RDLOCK_GUARD_MAINLOOP(); > > s->chain_frozen = false; > > bdrv_unfreeze_backing_chain(cor_filter_bs, s->bottom_bs); > > } > > ...vs. the magic one-line per-scope change here? Both work, so I > don't see any problems, but it does seem odd to mix styles in the same > patch. (I can see other places where you have intentionally picked > the version that required the least reindenting; adding a scope just > to use GRAPH_RDLOCK_GUARD_MAINLOOP() without having to carefully pair > an unlock on every early exit path is fewer lines of code overall, but > more lines of churn in the patch itself.) Generally I used the scoped locks only when I was quite sure that nothing in the function will require dropping the lock. The safe default is individually locking smaller sections. With GRAPH_RDLOCK_GUARD_MAINLOOP(), the whole situation is a bit fuzzy because it doesn't actually do anything apart from asserting that we don't need real locking. So taking it while calling functions that want to be called unlocked still works in practice, but it's a bit unclean, and it would conflict with an actual GRAPH_UNLOCKED annotation. Kevin
diff --git a/block/copy-on-read.h b/block/copy-on-read.h index 1d8ad38c74..72f9b378ea 100644 --- a/block/copy-on-read.h +++ b/block/copy-on-read.h @@ -27,6 +27,7 @@ #include "block/block_int.h" -void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs); +void no_coroutine_fn GRAPH_UNLOCKED +bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs); #endif /* BLOCK_COPY_ON_READ_H */ diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index b6860ae43b..545708c35a 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -149,11 +149,12 @@ BlockDriverState * GRAPH_RDLOCK bdrv_find_overlay(BlockDriverState *active, BlockDriverState *bs); BlockDriverState * GRAPH_RDLOCK bdrv_find_base(BlockDriverState *bs); -bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base, - Error **errp); -int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, - Error **errp); -void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base); + +int GRAPH_RDLOCK +bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, + Error **errp); +void GRAPH_RDLOCK +bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base); /* * The units of offset and total_work_size may be chosen arbitrarily by the diff --git a/block.c b/block.c index 7e8b39711b..dc1980ee42 100644 --- a/block.c +++ b/block.c @@ -5843,8 +5843,9 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs) * between @bs and @base is frozen. @errp is set if that's the case. * @base must be reachable from @bs, or NULL. */ -bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base, - Error **errp) +static bool GRAPH_RDLOCK +bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base, + Error **errp) { BlockDriverState *i; BdrvChild *child; diff --git a/block/commit.c b/block/commit.c index 05eb57d9ea..d92af02ead 100644 --- a/block/commit.c +++ b/block/commit.c @@ -48,8 +48,10 @@ static int commit_prepare(Job *job) { CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); + bdrv_graph_rdlock_main_loop(); bdrv_unfreeze_backing_chain(s->commit_top_bs, s->base_bs); s->chain_frozen = false; + bdrv_graph_rdunlock_main_loop(); /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before * the normal backing chain can be restored. */ @@ -68,7 +70,9 @@ static void commit_abort(Job *job) BlockDriverState *top_bs = blk_bs(s->top); if (s->chain_frozen) { + bdrv_graph_rdlock_main_loop(); bdrv_unfreeze_backing_chain(s->commit_top_bs, s->base_bs); + bdrv_graph_rdunlock_main_loop(); } /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */ @@ -404,7 +408,9 @@ void commit_start(const char *job_id, BlockDriverState *bs, fail: if (s->chain_frozen) { + bdrv_graph_rdlock_main_loop(); bdrv_unfreeze_backing_chain(commit_top_bs, base); + bdrv_graph_rdunlock_main_loop(); } if (s->base) { blk_unref(s->base); diff --git a/block/copy-on-read.c b/block/copy-on-read.c index 5149fcf63a..6f245b629a 100644 --- a/block/copy-on-read.c +++ b/block/copy-on-read.c @@ -35,8 +35,8 @@ typedef struct BDRVStateCOR { } BDRVStateCOR; -static int cor_open(BlockDriverState *bs, QDict *options, int flags, - Error **errp) +static int GRAPH_UNLOCKED +cor_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { BlockDriverState *bottom_bs = NULL; BDRVStateCOR *state = bs->opaque; @@ -44,6 +44,8 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags, const char *bottom_node = qdict_get_try_str(options, "bottom"); int ret; + GLOBAL_STATE_CODE(); + ret = bdrv_open_file_child(NULL, options, "file", bs, errp); if (ret < 0) { return ret; @@ -59,6 +61,8 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags, bs->file->bs->supported_zero_flags); if (bottom_node) { + GRAPH_RDLOCK_GUARD_MAINLOOP(); + bottom_bs = bdrv_find_node(bottom_node); if (!bottom_bs) { error_setg(errp, "Bottom node '%s' not found", bottom_node); @@ -227,13 +231,17 @@ cor_co_lock_medium(BlockDriverState *bs, bool locked) } -static void cor_close(BlockDriverState *bs) +static void GRAPH_UNLOCKED cor_close(BlockDriverState *bs) { BDRVStateCOR *s = bs->opaque; + GLOBAL_STATE_CODE(); + if (s->chain_frozen) { + bdrv_graph_rdlock_main_loop(); s->chain_frozen = false; bdrv_unfreeze_backing_chain(bs, s->bottom_bs); + bdrv_graph_rdunlock_main_loop(); } bdrv_unref(s->bottom_bs); @@ -263,12 +271,15 @@ static BlockDriver bdrv_copy_on_read = { }; -void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs) +void no_coroutine_fn bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs) { BDRVStateCOR *s = cor_filter_bs->opaque; + GLOBAL_STATE_CODE(); + /* unfreeze, as otherwise bdrv_replace_node() will fail */ if (s->chain_frozen) { + GRAPH_RDLOCK_GUARD_MAINLOOP(); s->chain_frozen = false; bdrv_unfreeze_backing_chain(cor_filter_bs, s->bottom_bs); } diff --git a/block/mirror.c b/block/mirror.c index 4d11a30508..304bd3208a 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -670,6 +670,7 @@ static int mirror_exit_common(Job *job) s->prepared = true; aio_context_acquire(qemu_get_aio_context()); + bdrv_graph_rdlock_main_loop(); mirror_top_bs = s->mirror_top_bs; bs_opaque = mirror_top_bs->opaque; @@ -688,6 +689,8 @@ static int mirror_exit_common(Job *job) bdrv_ref(mirror_top_bs); bdrv_ref(target_bs); + bdrv_graph_rdunlock_main_loop(); + /* * Remove target parent that still uses BLK_PERM_WRITE/RESIZE before * inserting target_bs at s->to_replace, where we might not be able to get diff --git a/block/stream.c b/block/stream.c index 5323a9976d..c32c98339a 100644 --- a/block/stream.c +++ b/block/stream.c @@ -266,6 +266,8 @@ void stream_start(const char *job_id, BlockDriverState *bs, assert(!(base && bottom)); assert(!(backing_file_str && bottom)); + bdrv_graph_rdlock_main_loop(); + if (bottom) { /* * New simple interface. The code is written in terms of old interface @@ -278,13 +280,11 @@ void stream_start(const char *job_id, BlockDriverState *bs, assert(!bottom->drv->is_filter); base_overlay = above_base = bottom; } else { - GRAPH_RDLOCK_GUARD_MAINLOOP(); - base_overlay = bdrv_find_overlay(bs, base); if (!base_overlay) { error_setg(errp, "'%s' is not in the backing chain of '%s'", base->node_name, bs->node_name); - return; + goto out_rdlock; } /* @@ -306,7 +306,7 @@ void stream_start(const char *job_id, BlockDriverState *bs, if (bs_read_only) { /* Hold the chain during reopen */ if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) { - return; + goto out_rdlock; } ret = bdrv_reopen_set_read_only(bs, false, errp); @@ -315,10 +315,12 @@ void stream_start(const char *job_id, BlockDriverState *bs, bdrv_unfreeze_backing_chain(bs, above_base); if (ret < 0) { - return; + goto out_rdlock; } } + bdrv_graph_rdunlock_main_loop(); + opts = qdict_new(); qdict_put_str(opts, "driver", "copy-on-read"); @@ -413,4 +415,8 @@ fail: if (bs_read_only) { bdrv_reopen_set_read_only(bs, true, NULL); } + return; + +out_rdlock: + bdrv_graph_rdunlock_main_loop(); }
This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_(un)freeze_backing_chain() need to hold a reader lock for the graph because it calls bdrv_filter_or_cow_child(), which accesses bs->file/backing. Use the opportunity to make bdrv_is_backing_chain_frozen() static, it has no external callers. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/copy-on-read.h | 3 ++- include/block/block-global-state.h | 11 ++++++----- block.c | 5 +++-- block/commit.c | 6 ++++++ block/copy-on-read.c | 19 +++++++++++++++---- block/mirror.c | 3 +++ block/stream.c | 16 +++++++++++----- 7 files changed, 46 insertions(+), 17 deletions(-)