From 6b89e851fabf78d7fb090bcdc71789ea1ef55c9b Mon Sep 17 00:00:00 2001 From: Fiona Ebner Date: Fri, 30 May 2025 17:11:01 +0200 Subject: block: add bdrv_graph_wrlock_drained() convenience wrapper Many write-locked sections are also drained sections. A new bdrv_graph_wrunlock_drained() wrapper around bdrv_graph_wrunlock() is introduced, which will begin a drained section first. A global variable is used so bdrv_graph_wrunlock() knows if it also needs to end such a drained section. Both the aio_poll call in bdrv_graph_wrlock() and the aio_bh_poll() in bdrv_graph_wrunlock() can re-enter a write-locked section. While for the latter, ending the drain could be moved to before the call, the former requires that the variable is a counter and not just a boolean. Since the wrapper calls bdrv_drain_all_begin(), which must be called with the graph unlocked, mark the wrapper as GRAPH_UNLOCKED too. The switch to the new helpers was generated with the following commands and then manually checked: find . -name '*.c' -exec sed -i -z 's/bdrv_drain_all_begin();\n\s*bdrv_graph_wrlock();/bdrv_graph_wrlock_drained();/g' {} ';' find . -name '*.c' -exec sed -i -z 's/bdrv_graph_wrunlock();\n\s*bdrv_drain_all_end();/bdrv_graph_wrunlock();/g' {} ';' Suggested-by: Kevin Wolf Signed-off-by: Fiona Ebner Message-ID: <20250530151125.955508-25-f.ebner@proxmox.com> [kwolf: Removed redundant GRAPH_UNLOCKED] Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/commit.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'block/commit.c') diff --git a/block/commit.c b/block/commit.c index 6c4b736ff8..dc1942483b 100644 --- a/block/commit.c +++ b/block/commit.c @@ -392,8 +392,7 @@ void commit_start(const char *job_id, BlockDriverState *bs, * this is the responsibility of the interface (i.e. whoever calls * commit_start()). */ - bdrv_drain_all_begin(); - bdrv_graph_wrlock(); + bdrv_graph_wrlock_drained(); s->base_overlay = bdrv_find_overlay(top, base); assert(s->base_overlay); @@ -425,21 +424,18 @@ void commit_start(const char *job_id, BlockDriverState *bs, iter_shared_perms, errp); if (ret < 0) { bdrv_graph_wrunlock(); - bdrv_drain_all_end(); goto fail; } } if (bdrv_freeze_backing_chain(commit_top_bs, base, errp) < 0) { bdrv_graph_wrunlock(); - bdrv_drain_all_end(); goto fail; } s->chain_frozen = true; ret = block_job_add_bdrv(&s->common, "base", base, 0, BLK_PERM_ALL, errp); bdrv_graph_wrunlock(); - bdrv_drain_all_end(); if (ret < 0) { goto fail; -- cgit 1.4.1 From 47bc2ed6f6a8eb637ede90c0545bca082b68379f Mon Sep 17 00:00:00 2001 From: Fiona Ebner Date: Fri, 30 May 2025 17:11:03 +0200 Subject: block/commit: switch to bdrv_set_backing_hd_drained() variant This is in preparation to mark bdrv_set_backing_hd() as GRAPH_UNLOCKED. Switch to using the bdrv_set_backing_hd_drained() variant. For the first pair of calls to avoid draining and locking twice in a row within the individual calls. For the third call, so that the drained and locked section can also cover bdrv_cow_bs(). Signed-off-by: Fiona Ebner Message-ID: <20250530151125.955508-27-f.ebner@proxmox.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/commit.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) (limited to 'block/commit.c') diff --git a/block/commit.c b/block/commit.c index dc1942483b..c9690a5da0 100644 --- a/block/commit.c +++ b/block/commit.c @@ -514,28 +514,32 @@ int bdrv_commit(BlockDriverState *bs) Error *local_err = NULL; GLOBAL_STATE_CODE(); - GRAPH_RDLOCK_GUARD_MAINLOOP(); if (!drv) return -ENOMEDIUM; + bdrv_graph_rdlock_main_loop(); + backing_file_bs = bdrv_cow_bs(bs); if (!backing_file_bs) { - return -ENOTSUP; + ret = -ENOTSUP; + goto out; } if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT_SOURCE, NULL) || bdrv_op_is_blocked(backing_file_bs, BLOCK_OP_TYPE_COMMIT_TARGET, NULL)) { - return -EBUSY; + ret = -EBUSY; + goto out; } ro = bdrv_is_read_only(backing_file_bs); if (ro) { if (bdrv_reopen_set_read_only(backing_file_bs, false, NULL)) { - return -EACCES; + ret = -EACCES; + goto out; } } @@ -559,8 +563,14 @@ int bdrv_commit(BlockDriverState *bs) goto ro_cleanup; } - bdrv_set_backing_hd(commit_top_bs, backing_file_bs, &error_abort); - bdrv_set_backing_hd(bs, commit_top_bs, &error_abort); + bdrv_graph_rdunlock_main_loop(); + + bdrv_graph_wrlock_drained(); + bdrv_set_backing_hd_drained(commit_top_bs, backing_file_bs, &error_abort); + bdrv_set_backing_hd_drained(bs, commit_top_bs, &error_abort); + bdrv_graph_wrunlock(); + + bdrv_graph_rdlock_main_loop(); ret = blk_insert_bs(backing, backing_file_bs, &local_err); if (ret < 0) { @@ -635,9 +645,14 @@ int bdrv_commit(BlockDriverState *bs) ret = 0; ro_cleanup: blk_unref(backing); + + bdrv_graph_rdunlock_main_loop(); + bdrv_graph_wrlock_drained(); if (bdrv_cow_bs(bs) != backing_file_bs) { - bdrv_set_backing_hd(bs, backing_file_bs, &error_abort); + bdrv_set_backing_hd_drained(bs, backing_file_bs, &error_abort); } + bdrv_graph_wrunlock(); + bdrv_graph_rdlock_main_loop(); bdrv_unref(commit_top_bs); blk_unref(src); @@ -646,5 +661,8 @@ ro_cleanup: bdrv_reopen_set_read_only(backing_file_bs, true, NULL); } +out: + bdrv_graph_rdunlock_main_loop(); + return ret; } -- cgit 1.4.1 From 54eb59d668d6e4e7584188628ca44f3e9bd39d17 Mon Sep 17 00:00:00 2001 From: Fiona Ebner Date: Fri, 30 May 2025 17:11:07 +0200 Subject: block: drop wrapper for bdrv_set_backing_hd_drained() Nearly all callers (outside of the tests) are already using the _drained() variant of the function. It doesn't seem worth keeping. Simply adapt the remaining callers of bdrv_set_backing_hd() and rename bdrv_set_backing_hd_drained() to bdrv_set_backing_hd(). Signed-off-by: Fiona Ebner Message-ID: <20250530151125.955508-31-f.ebner@proxmox.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block.c | 20 ++++---------------- block/commit.c | 6 +++--- block/mirror.c | 2 +- block/stream.c | 13 ++++++------- blockdev.c | 13 ++++++++----- include/block/block-global-state.h | 5 +---- tests/unit/test-bdrv-drain.c | 12 +++++++++++- tests/unit/test-bdrv-graph-mod.c | 2 +- 8 files changed, 35 insertions(+), 38 deletions(-) (limited to 'block/commit.c') diff --git a/block.c b/block.c index 9ef3b0262c..4754705bfd 100644 --- a/block.c +++ b/block.c @@ -3570,9 +3570,8 @@ out: * * All block nodes must be drained. */ -int bdrv_set_backing_hd_drained(BlockDriverState *bs, - BlockDriverState *backing_hd, - Error **errp) +int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd, + Error **errp) { int ret; Transaction *tran = tran_new(); @@ -3594,19 +3593,6 @@ out: return ret; } -int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd, - Error **errp) -{ - int ret; - GLOBAL_STATE_CODE(); - - bdrv_graph_wrlock_drained(); - ret = bdrv_set_backing_hd_drained(bs, backing_hd, errp); - bdrv_graph_wrunlock(); - - return ret; -} - /* * Opens the backing file for a BlockDriverState if not yet open * @@ -3715,7 +3701,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, /* Hook up the backing file link; drop our reference, bs owns the * backing_hd reference now */ bdrv_graph_rdunlock_main_loop(); + bdrv_graph_wrlock_drained(); ret = bdrv_set_backing_hd(bs, backing_hd, errp); + bdrv_graph_wrunlock(); bdrv_graph_rdlock_main_loop(); bdrv_unref(backing_hd); diff --git a/block/commit.c b/block/commit.c index c9690a5da0..7496cf732e 100644 --- a/block/commit.c +++ b/block/commit.c @@ -566,8 +566,8 @@ int bdrv_commit(BlockDriverState *bs) bdrv_graph_rdunlock_main_loop(); bdrv_graph_wrlock_drained(); - bdrv_set_backing_hd_drained(commit_top_bs, backing_file_bs, &error_abort); - bdrv_set_backing_hd_drained(bs, commit_top_bs, &error_abort); + bdrv_set_backing_hd(commit_top_bs, backing_file_bs, &error_abort); + bdrv_set_backing_hd(bs, commit_top_bs, &error_abort); bdrv_graph_wrunlock(); bdrv_graph_rdlock_main_loop(); @@ -649,7 +649,7 @@ ro_cleanup: bdrv_graph_rdunlock_main_loop(); bdrv_graph_wrlock_drained(); if (bdrv_cow_bs(bs) != backing_file_bs) { - bdrv_set_backing_hd_drained(bs, backing_file_bs, &error_abort); + bdrv_set_backing_hd(bs, backing_file_bs, &error_abort); } bdrv_graph_wrunlock(); bdrv_graph_rdlock_main_loop(); diff --git a/block/mirror.c b/block/mirror.c index 873e95d029..b344182c74 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -772,7 +772,7 @@ static int mirror_exit_common(Job *job) backing = s->sync_mode == MIRROR_SYNC_MODE_NONE ? src : s->base; if (bdrv_cow_bs(unfiltered_target) != backing) { - bdrv_set_backing_hd_drained(unfiltered_target, backing, &local_err); + bdrv_set_backing_hd(unfiltered_target, backing, &local_err); if (local_err) { error_report_err(local_err); local_err = NULL; diff --git a/block/stream.c b/block/stream.c index a6ef840e29..17e240460c 100644 --- a/block/stream.c +++ b/block/stream.c @@ -73,12 +73,11 @@ static int stream_prepare(Job *job) s->cor_filter_bs = NULL; /* - * bdrv_set_backing_hd() requires that the unfiltered_bs and the COW child - * of unfiltered_bs is drained. Drain already here and use - * bdrv_set_backing_hd_drained() instead because the polling during - * drained_begin() might change the graph, and if we do this only later, we - * may end up working with the wrong base node (or it might even have gone - * away by the time we want to use it). + * bdrv_set_backing_hd() requires that all block nodes are drained. Drain + * already here, because the polling during drained_begin() might change the + * graph, and if we do this only later, we may end up working with the wrong + * base node (or it might even have gone away by the time we want to use + * it). */ if (unfiltered_bs_cow) { bdrv_ref(unfiltered_bs_cow); @@ -105,7 +104,7 @@ static int stream_prepare(Job *job) } bdrv_graph_wrlock(); - bdrv_set_backing_hd_drained(unfiltered_bs, base, &local_err); + bdrv_set_backing_hd(unfiltered_bs, base, &local_err); bdrv_graph_wrunlock(); /* diff --git a/blockdev.c b/blockdev.c index 3c53472a23..9f3f42d896 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1587,12 +1587,12 @@ static void external_snapshot_abort(void *opaque) /* * Note that state->old_bs would not disappear during the * write-locked section, because the unref from - * bdrv_set_backing_hd_drained() only happens at the end of the - * write-locked section. However, just be explicit about keeping a - * reference and don't rely on that implicit detail. + * bdrv_set_backing_hd() only happens at the end of the write-locked + * section. However, just be explicit about keeping a reference and + * don't rely on that implicit detail. */ bdrv_ref(state->old_bs); - bdrv_set_backing_hd_drained(state->new_bs, NULL, &error_abort); + bdrv_set_backing_hd(state->new_bs, NULL, &error_abort); /* * The call to bdrv_set_backing_hd() above returns state->old_bs to @@ -1776,7 +1776,10 @@ static void drive_backup_action(DriveBackup *backup, } if (set_backing_hd) { - if (bdrv_set_backing_hd(target_bs, source, errp) < 0) { + bdrv_graph_wrlock_drained(); + ret = bdrv_set_backing_hd(target_bs, source, errp); + bdrv_graph_wrunlock(); + if (ret < 0) { goto unref; } } diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index 009b9ac946..bcbb624a7b 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -100,12 +100,9 @@ bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp); BlockDriverState * coroutine_fn no_co_wrapper bdrv_co_open_blockdev_ref(BlockdevRef *ref, Error **errp); -int GRAPH_UNLOCKED +int GRAPH_WRLOCK bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd, Error **errp); -int GRAPH_WRLOCK -bdrv_set_backing_hd_drained(BlockDriverState *bs, BlockDriverState *backing_hd, - Error **errp); int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, const char *bdref_key, Error **errp); diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index 3369c2c2aa..43b0ba8648 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -193,7 +193,9 @@ static BlockBackend * no_coroutine_fn test_setup(void) blk_insert_bs(blk, bs, &error_abort); backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort); + bdrv_graph_wrlock_drained(); bdrv_set_backing_hd(bs, backing, &error_abort); + bdrv_graph_wrunlock(); bdrv_unref(backing); bdrv_unref(bs); @@ -386,7 +388,9 @@ static void test_nested(void) backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort); backing_s = backing->opaque; + bdrv_graph_wrlock_drained(); bdrv_set_backing_hd(bs, backing, &error_abort); + bdrv_graph_wrunlock(); for (outer = 0; outer < DRAIN_TYPE_MAX; outer++) { for (inner = 0; inner < DRAIN_TYPE_MAX; inner++) { @@ -733,10 +737,12 @@ static void test_blockjob_common_drain_node(enum drain_type drain_type, src_overlay = bdrv_new_open_driver(&bdrv_test, "source-overlay", BDRV_O_RDWR, &error_abort); + bdrv_graph_wrlock_drained(); bdrv_set_backing_hd(src_overlay, src, &error_abort); bdrv_unref(src); bdrv_set_backing_hd(src, src_backing, &error_abort); bdrv_unref(src_backing); + bdrv_graph_wrunlock(); blk_src = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL); blk_insert_bs(blk_src, src_overlay, &error_abort); @@ -1436,8 +1442,10 @@ static void test_drop_backing_job_commit(Job *job) TestDropBackingBlockJob *s = container_of(job, TestDropBackingBlockJob, common.job); + bdrv_graph_wrlock_drained(); bdrv_set_backing_hd(s->bs, NULL, &error_abort); bdrv_set_backing_hd(s->detach_also, NULL, &error_abort); + bdrv_graph_wrunlock(); *s->did_complete = true; } @@ -1530,7 +1538,9 @@ static void test_blockjob_commit_by_drained_end(void) snprintf(name, sizeof(name), "parent-node-%i", i); bs_parents[i] = bdrv_new_open_driver(&bdrv_test, name, BDRV_O_RDWR, &error_abort); + bdrv_graph_wrlock_drained(); bdrv_set_backing_hd(bs_parents[i], bs_child, &error_abort); + bdrv_graph_wrunlock(); } job = block_job_create("job", &test_drop_backing_job_driver, NULL, @@ -1679,13 +1689,13 @@ static void test_drop_intermediate_poll(void) job_node = bdrv_new_open_driver(&bdrv_test, "job-node", BDRV_O_RDWR, &error_abort); + bdrv_graph_wrlock_drained(); bdrv_set_backing_hd(job_node, chain[1], &error_abort); /* * Establish the chain last, so the chain links are the first * elements in the BDS.parents lists */ - bdrv_graph_wrlock_drained(); for (i = 0; i < 3; i++) { if (i) { /* Takes the reference to chain[i - 1] */ diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c index b077f0e3e3..567db99e4f 100644 --- a/tests/unit/test-bdrv-graph-mod.c +++ b/tests/unit/test-bdrv-graph-mod.c @@ -202,9 +202,9 @@ static void test_should_update_child(void) blk_insert_bs(root, bs, &error_abort); + bdrv_graph_wrlock_drained(); bdrv_set_backing_hd(target, bs, &error_abort); - bdrv_graph_wrlock_drained(); g_assert(target->backing->bs == bs); bdrv_attach_child(filter, target, "target", &child_of_bds, BDRV_CHILD_DATA, &error_abort); -- cgit 1.4.1 From 60f609c1526102df35d8de8f513a80e6d3528bd8 Mon Sep 17 00:00:00 2001 From: Fiona Ebner Date: Fri, 30 May 2025 17:11:17 +0200 Subject: block/commit: mark commit_abort() as GRAPH_UNLOCKED The function commit_abort() calls bdrv_drained_begin(), which must be called with the graph unlocked. Also mark the JobDriver's abort() callback as GRAPH_UNLOCKED_PTR, because that is the callback via which commit_abort() is reached. Signed-off-by: Fiona Ebner Message-ID: <20250530151125.955508-41-f.ebner@proxmox.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/commit.c | 2 +- include/qemu/job.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'block/commit.c') diff --git a/block/commit.c b/block/commit.c index 7496cf732e..0d9e1a16d7 100644 --- a/block/commit.c +++ b/block/commit.c @@ -68,7 +68,7 @@ static int commit_prepare(Job *job) s->backing_mask_protocol); } -static void commit_abort(Job *job) +static void GRAPH_UNLOCKED commit_abort(Job *job) { CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); BlockDriverState *top_bs = blk_bs(s->top); diff --git a/include/qemu/job.h b/include/qemu/job.h index bb8ee766ef..ead31578d3 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -283,7 +283,7 @@ struct JobDriver { * All jobs will complete with a call to either .commit() or .abort() but * never both. */ - void (*abort)(Job *job); + void GRAPH_UNLOCKED_PTR (*abort)(Job *job); /** * If the callback is not NULL, it will be invoked after a call to either -- cgit 1.4.1