From d4c5f8c980f1073356d2f18d51dc68d42bebb59d Mon Sep 17 00:00:00 2001 From: Fiona Ebner Date: Fri, 30 May 2025 17:10:40 +0200 Subject: block/snapshot: move drain outside of read-locked bdrv_snapshot_delete() This is in preparation to mark bdrv_drained_begin() as GRAPH_UNLOCKED. More granular draining is not trivially possible, because bdrv_snapshot_delete() can recursively call itself. The return value of bdrv_all_delete_snapshot() changes from -1 to -errno propagated from failed sub-calls. This is fine for the existing callers of bdrv_all_delete_snapshot(). Signed-off-by: Fiona Ebner Reviewed-by: Kevin Wolf Message-ID: <20250530151125.955508-4-f.ebner@proxmox.com> Signed-off-by: Kevin Wolf --- blockdev.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) (limited to 'blockdev.c') diff --git a/blockdev.c b/blockdev.c index 21443b4514..3982f9776b 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1132,39 +1132,41 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device, int ret; GLOBAL_STATE_CODE(); - GRAPH_RDLOCK_GUARD_MAINLOOP(); + + bdrv_drain_all_begin(); + bdrv_graph_rdlock_main_loop(); bs = qmp_get_root_bs(device, errp); if (!bs) { - return NULL; + goto error; } if (!id && !name) { error_setg(errp, "Name or id must be provided"); - return NULL; + goto error; } if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, errp)) { - return NULL; + goto error; } ret = bdrv_snapshot_find_by_id_and_name(bs, id, name, &sn, &local_err); if (local_err) { error_propagate(errp, local_err); - return NULL; + goto error; } if (!ret) { error_setg(errp, "Snapshot with id '%s' and name '%s' does not exist on " "device '%s'", STR_OR_NULL(id), STR_OR_NULL(name), device); - return NULL; + goto error; } bdrv_snapshot_delete(bs, id, name, &local_err); if (local_err) { error_propagate(errp, local_err); - return NULL; + goto error; } info = g_new0(SnapshotInfo, 1); @@ -1180,6 +1182,9 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device, info->has_icount = true; } +error: + bdrv_graph_rdunlock_main_loop(); + bdrv_drain_all_end(); return info; } @@ -1295,12 +1300,14 @@ static void internal_snapshot_abort(void *opaque) Error *local_error = NULL; GLOBAL_STATE_CODE(); - GRAPH_RDLOCK_GUARD_MAINLOOP(); if (!state->created) { return; } + bdrv_drain_all_begin(); + bdrv_graph_rdlock_main_loop(); + if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) { error_reportf_err(local_error, "Failed to delete snapshot with id '%s' and " @@ -1308,6 +1315,8 @@ static void internal_snapshot_abort(void *opaque) sn->id_str, sn->name, bdrv_get_device_name(bs)); } + bdrv_graph_rdunlock_main_loop(); + bdrv_drain_all_end(); } static void internal_snapshot_clean(void *opaque) -- cgit 1.4.1 From a1ea8eb5912256c0b2be16fae5d3786aebc80cb1 Mon Sep 17 00:00:00 2001 From: Fiona Ebner Date: Fri, 30 May 2025 17:10:46 +0200 Subject: block: move drain outside of bdrv_try_change_aio_context() This is part of resolving the deadlock mentioned in commit "block: move draining out of bdrv_change_aio_context() and mark GRAPH_RDLOCK". Convert the function to a _locked() version that has to be called with the graph lock held and add a convenience wrapper that has to be called with the graph unlocked, which drains and takes the lock itself. Since bdrv_try_change_aio_context() is global state code, the wrapper is too. Callers are adapted to use the appropriate variant, depending on whether the caller already holds the lock. In the test_set_aio_context() unit test, prior drains can be removed, because draining already happens inside the new wrapper. Note that bdrv_attach_child_common_abort(), bdrv_attach_child_common() and bdrv_root_unref_child() hold the graph lock and are not actually allowed to drain either. This will be addressed in the following commits. Functions like qmp_blockdev_mirror() query the nodes to act on before draining and locking. In theory, draining could invalidate those nodes. This kind of issue is not addressed by these commits. Signed-off-by: Fiona Ebner Reviewed-by: Kevin Wolf Message-ID: <20250530151125.955508-10-f.ebner@proxmox.com> Signed-off-by: Kevin Wolf --- block.c | 58 ++++++++++++++++++++++++++++---------- blockdev.c | 15 ++++++---- include/block/block-global-state.h | 8 ++++-- tests/unit/test-bdrv-drain.c | 4 --- 4 files changed, 59 insertions(+), 26 deletions(-) (limited to 'blockdev.c') diff --git a/block.c b/block.c index 6f42c0f1ab..3aaacabf7f 100644 --- a/block.c +++ b/block.c @@ -3028,7 +3028,10 @@ static void GRAPH_WRLOCK bdrv_attach_child_common_abort(void *opaque) bdrv_replace_child_noperm(s->child, NULL); if (bdrv_get_aio_context(bs) != s->old_child_ctx) { - bdrv_try_change_aio_context(bs, s->old_child_ctx, NULL, &error_abort); + bdrv_drain_all_begin(); + bdrv_try_change_aio_context_locked(bs, s->old_child_ctx, NULL, + &error_abort); + bdrv_drain_all_end(); } if (bdrv_child_get_parent_aio_context(s->child) != s->old_parent_ctx) { @@ -3115,8 +3118,10 @@ bdrv_attach_child_common(BlockDriverState *child_bs, parent_ctx = bdrv_child_get_parent_aio_context(new_child); if (child_ctx != parent_ctx) { Error *local_err = NULL; - int ret = bdrv_try_change_aio_context(child_bs, parent_ctx, NULL, - &local_err); + bdrv_drain_all_begin(); + int ret = bdrv_try_change_aio_context_locked(child_bs, parent_ctx, NULL, + &local_err); + bdrv_drain_all_end(); if (ret < 0 && child_class->change_aio_ctx) { Transaction *aio_ctx_tran = tran_new(); @@ -3319,8 +3324,10 @@ void bdrv_root_unref_child(BdrvChild *child) * When the parent requiring a non-default AioContext is removed, the * node moves back to the main AioContext */ - bdrv_try_change_aio_context(child_bs, qemu_get_aio_context(), NULL, - NULL); + bdrv_drain_all_begin(); + bdrv_try_change_aio_context_locked(child_bs, qemu_get_aio_context(), + NULL, NULL); + bdrv_drain_all_end(); } bdrv_schedule_unref(child_bs); @@ -7719,9 +7726,13 @@ bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx, * * If ignore_child is not NULL, that child (and its subgraph) will not * be touched. + * + * Called with the graph lock held. + * + * Called while all bs are drained. */ -int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx, - BdrvChild *ignore_child, Error **errp) +int bdrv_try_change_aio_context_locked(BlockDriverState *bs, AioContext *ctx, + BdrvChild *ignore_child, Error **errp) { Transaction *tran; GHashTable *visited; @@ -7730,17 +7741,15 @@ int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx, /* * Recursion phase: go through all nodes of the graph. - * Take care of checking that all nodes support changing AioContext - * and drain them, building a linear list of callbacks to run if everything - * is successful (the transaction itself). + * Take care of checking that all nodes support changing AioContext, + * building a linear list of callbacks to run if everything is successful + * (the transaction itself). */ tran = tran_new(); visited = g_hash_table_new(NULL, NULL); if (ignore_child) { g_hash_table_add(visited, ignore_child); } - bdrv_drain_all_begin(); - bdrv_graph_rdlock_main_loop(); ret = bdrv_change_aio_context(bs, ctx, visited, tran, errp); g_hash_table_destroy(visited); @@ -7754,15 +7763,34 @@ int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx, if (!ret) { /* Just run clean() callbacks. No AioContext changed. */ tran_abort(tran); - bdrv_graph_rdunlock_main_loop(); - bdrv_drain_all_end(); return -EPERM; } tran_commit(tran); + return 0; +} + +/* + * Change bs's and recursively all of its parents' and children's AioContext + * to the given new context, returning an error if that isn't possible. + * + * If ignore_child is not NULL, that child (and its subgraph) will not + * be touched. + */ +int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx, + BdrvChild *ignore_child, Error **errp) +{ + int ret; + + GLOBAL_STATE_CODE(); + + bdrv_drain_all_begin(); + bdrv_graph_rdlock_main_loop(); + ret = bdrv_try_change_aio_context_locked(bs, ctx, ignore_child, errp); bdrv_graph_rdunlock_main_loop(); bdrv_drain_all_end(); - return 0; + + return ret; } void bdrv_add_aio_context_notifier(BlockDriverState *bs, diff --git a/blockdev.c b/blockdev.c index 3982f9776b..750beba41f 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3601,12 +3601,13 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread, AioContext *new_context; BlockDriverState *bs; - GRAPH_RDLOCK_GUARD_MAINLOOP(); + bdrv_drain_all_begin(); + bdrv_graph_rdlock_main_loop(); bs = bdrv_find_node(node_name); if (!bs) { error_setg(errp, "Failed to find node with node-name='%s'", node_name); - return; + goto out; } /* Protects against accidents. */ @@ -3614,14 +3615,14 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread, error_setg(errp, "Node %s is associated with a BlockBackend and could " "be in use (use force=true to override this check)", node_name); - return; + goto out; } if (iothread->type == QTYPE_QSTRING) { IOThread *obj = iothread_by_id(iothread->u.s); if (!obj) { error_setg(errp, "Cannot find iothread %s", iothread->u.s); - return; + goto out; } new_context = iothread_get_aio_context(obj); @@ -3629,7 +3630,11 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread, new_context = qemu_get_aio_context(); } - bdrv_try_change_aio_context(bs, new_context, NULL, errp); + bdrv_try_change_aio_context_locked(bs, new_context, NULL, errp); + +out: + bdrv_graph_rdunlock_main_loop(); + bdrv_drain_all_end(); } QemuOptsList qemu_common_drive_opts = { diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index aad160956a..91f249b5ad 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -278,8 +278,12 @@ bool GRAPH_RDLOCK bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx, GHashTable *visited, Transaction *tran, Error **errp); -int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx, - BdrvChild *ignore_child, Error **errp); +int GRAPH_UNLOCKED +bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx, + BdrvChild *ignore_child, Error **errp); +int GRAPH_RDLOCK +bdrv_try_change_aio_context_locked(BlockDriverState *bs, AioContext *ctx, + BdrvChild *ignore_child, Error **errp); int GRAPH_RDLOCK bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz); int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo); diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index 290cd2a70e..3185f3f429 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -1396,14 +1396,10 @@ static void test_set_aio_context(void) bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR, &error_abort); - bdrv_drained_begin(bs); bdrv_try_change_aio_context(bs, ctx_a, NULL, &error_abort); - bdrv_drained_end(bs); - bdrv_drained_begin(bs); bdrv_try_change_aio_context(bs, ctx_b, NULL, &error_abort); bdrv_try_change_aio_context(bs, qemu_get_aio_context(), NULL, &error_abort); - bdrv_drained_end(bs); bdrv_unref(bs); iothread_join(a); -- cgit 1.4.1 From 0414930d3adfa89299eaea5ce92accab15d9fba5 Mon Sep 17 00:00:00 2001 From: Fiona Ebner Date: Fri, 30 May 2025 17:10:51 +0200 Subject: block: move drain outside of quorum_add_child() This is part of resolving the deadlock mentioned in commit "block: move draining out of bdrv_change_aio_context() and mark GRAPH_RDLOCK". The quorum_add_child() callback runs under the graph lock, so it is not allowed to drain. It is only called as the .bdrv_add_child() callback, which is only called in the bdrv_add_child() function, which also runs under the graph lock. The bdrv_add_child() function is called by qmp_x_blockdev_change(), where a drained section is introduced. Signed-off-by: Fiona Ebner Message-ID: <20250530151125.955508-15-f.ebner@proxmox.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block.c | 10 ++++++++-- block/quorum.c | 2 -- blockdev.c | 2 ++ include/block/block_int-common.h | 7 +++++++ 4 files changed, 17 insertions(+), 4 deletions(-) (limited to 'blockdev.c') diff --git a/block.c b/block.c index 6fc87aa318..f6c2f7e208 100644 --- a/block.c +++ b/block.c @@ -8220,8 +8220,10 @@ char *bdrv_dirname(BlockDriverState *bs, Error **errp) } /* - * Hot add/remove a BDS's child. So the user can take a child offline when - * it is broken and take a new child online + * Hot add a BDS's child. Used in combination with bdrv_del_child, so the user + * can take a child offline when it is broken and take a new child online. + * + * All block nodes must be drained. */ void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs, Error **errp) @@ -8261,6 +8263,10 @@ void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs, parent_bs->drv->bdrv_add_child(parent_bs, child_bs, errp); } +/* + * Hot remove a BDS's child. Used in combination with bdrv_add_child, so the + * user can take a child offline when it is broken and take a new child online. + */ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp) { BdrvChild *tmp; diff --git a/block/quorum.c b/block/quorum.c index ea17b0ec13..ed8ce801ee 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -1096,10 +1096,8 @@ quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs, Error **errp) /* We can safely add the child now */ bdrv_ref(child_bs); - bdrv_drain_all_begin(); child = bdrv_attach_child(bs, child_bs, indexstr, &child_of_bds, BDRV_CHILD_DATA, errp); - bdrv_drain_all_end(); if (child == NULL) { s->next_child_index--; return; diff --git a/blockdev.c b/blockdev.c index 750beba41f..bd5ca77619 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3531,6 +3531,7 @@ void qmp_x_blockdev_change(const char *parent, const char *child, BlockDriverState *parent_bs, *new_bs = NULL; BdrvChild *p_child; + bdrv_drain_all_begin(); bdrv_graph_wrlock(); parent_bs = bdrv_lookup_bs(parent, parent, errp); @@ -3568,6 +3569,7 @@ void qmp_x_blockdev_change(const char *parent, const char *child, out: bdrv_graph_wrunlock(); + bdrv_drain_all_end(); } BlockJobInfoList *qmp_query_block_jobs(Error **errp) diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 168f703fa1..f9e742f812 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -396,6 +396,13 @@ struct BlockDriver { int GRAPH_RDLOCK_PTR (*bdrv_probe_geometry)( BlockDriverState *bs, HDGeometry *geo); + /** + * Hot add a BDS's child. Used in combination with bdrv_del_child, so the + * user can take a child offline when it is broken and take a new child + * online. + * + * All block nodes must be drained. + */ void GRAPH_WRLOCK_PTR (*bdrv_add_child)( BlockDriverState *parent, BlockDriverState *child, Error **errp); -- cgit 1.4.1 From 6f101614f95c889399352b8301917c0ac7919ae7 Mon Sep 17 00:00:00 2001 From: Fiona Ebner Date: Fri, 30 May 2025 17:10:54 +0200 Subject: blockdev: drain while unlocked in internal_snapshot_action() This is in preparation to mark bdrv_drained_begin() as GRAPH_UNLOCKED. Signed-off-by: Fiona Ebner Message-ID: <20250530151125.955508-18-f.ebner@proxmox.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- blockdev.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) (limited to 'blockdev.c') diff --git a/blockdev.c b/blockdev.c index bd5ca77619..506755bef1 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1208,7 +1208,7 @@ static void internal_snapshot_action(BlockdevSnapshotInternal *internal, Error *local_err = NULL; const char *device; const char *name; - BlockDriverState *bs; + BlockDriverState *bs, *check_bs; QEMUSnapshotInfo old_sn, *sn; bool ret; int64_t rt; @@ -1216,7 +1216,7 @@ static void internal_snapshot_action(BlockdevSnapshotInternal *internal, int ret1; GLOBAL_STATE_CODE(); - GRAPH_RDLOCK_GUARD_MAINLOOP(); + bdrv_graph_rdlock_main_loop(); tran_add(tran, &internal_snapshot_drv, state); @@ -1225,14 +1225,29 @@ static void internal_snapshot_action(BlockdevSnapshotInternal *internal, bs = qmp_get_root_bs(device, errp); if (!bs) { + bdrv_graph_rdunlock_main_loop(); return; } state->bs = bs; + /* Need to drain while unlocked. */ + bdrv_graph_rdunlock_main_loop(); /* Paired with .clean() */ bdrv_drained_begin(bs); + GRAPH_RDLOCK_GUARD_MAINLOOP(); + + /* Make sure the root bs did not change with the drain. */ + check_bs = qmp_get_root_bs(device, errp); + if (bs != check_bs) { + if (check_bs) { + error_setg(errp, "Block node of device '%s' unexpectedly changed", + device); + } /* else errp is already set */ + return; + } + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, errp)) { return; } -- cgit 1.4.1 From 195a8a946a8681dfe7e8aa8d49db415693db5311 Mon Sep 17 00:00:00 2001 From: Fiona Ebner Date: Fri, 30 May 2025 17:10:55 +0200 Subject: blockdev: drain while unlocked in external_snapshot_action() This is in preparation to mark bdrv_drained_begin() as GRAPH_UNLOCKED. Signed-off-by: Fiona Ebner Message-ID: <20250530151125.955508-19-f.ebner@proxmox.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- blockdev.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) (limited to 'blockdev.c') diff --git a/blockdev.c b/blockdev.c index 506755bef1..2e7fda6780 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1377,9 +1377,10 @@ static void external_snapshot_action(TransactionAction *action, const char *new_image_file; ExternalSnapshotState *state = g_new0(ExternalSnapshotState, 1); uint64_t perm, shared; + BlockDriverState *check_bs; /* TODO We'll eventually have to take a writer lock in this function */ - GRAPH_RDLOCK_GUARD_MAINLOOP(); + bdrv_graph_rdlock_main_loop(); tran_add(tran, &external_snapshot_drv, state); @@ -1412,11 +1413,25 @@ static void external_snapshot_action(TransactionAction *action, state->old_bs = bdrv_lookup_bs(device, node_name, errp); if (!state->old_bs) { + bdrv_graph_rdunlock_main_loop(); return; } + /* Need to drain while unlocked. */ + bdrv_graph_rdunlock_main_loop(); /* Paired with .clean() */ bdrv_drained_begin(state->old_bs); + GRAPH_RDLOCK_GUARD_MAINLOOP(); + + /* Make sure the associated bs did not change with the drain. */ + check_bs = bdrv_lookup_bs(device, node_name, errp); + if (state->old_bs != check_bs) { + if (check_bs) { + error_setg(errp, "Block node of device '%s' unexpectedly changed", + device); + } /* else errp is already set */ + return; + } if (!bdrv_is_inserted(state->old_bs)) { error_setg(errp, "Device '%s' has no medium", -- cgit 1.4.1