From nobody Sat Nov 15 19:39:07 2025 Delivered-To: importer@patchew.org Authentication-Results: mx.zohomail.com; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org Return-Path: Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mx.zohomail.com with SMTPS id 1748265978283488.61407536253137; Mon, 26 May 2025 06:26:18 -0700 (PDT) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1uJXnL-0001bc-SX; Mon, 26 May 2025 09:23:13 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1uJXmc-0000G3-27; Mon, 26 May 2025 09:22:26 -0400 Received: from proxmox-new.maurer-it.com ([94.136.29.106]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1uJXmV-0000K0-V7; Mon, 26 May 2025 09:22:24 -0400 Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 200DB44552; Mon, 26 May 2025 15:21:50 +0200 (CEST) From: Fiona Ebner To: qemu-block@nongnu.org Cc: qemu-devel@nongnu.org, kwolf@redhat.com, den@virtuozzo.com, andrey.drobyshev@virtuozzo.com, hreitz@redhat.com, stefanha@redhat.com, eblake@redhat.com, jsnow@redhat.com, vsementsov@yandex-team.ru, xiechanglong.d@gmail.com, wencongyang2@huawei.com, berto@igalia.com, fam@euphon.net, ari@tuxera.com Subject: [PATCH v3 09/24] block: move drain outside of bdrv_try_change_aio_context() Date: Mon, 26 May 2025 15:21:25 +0200 Message-Id: <20250526132140.1641377-10-f.ebner@proxmox.com> X-Mailer: git-send-email 2.39.5 In-Reply-To: <20250526132140.1641377-1-f.ebner@proxmox.com> References: <20250526132140.1641377-1-f.ebner@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Received-SPF: pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Received-SPF: pass client-ip=94.136.29.106; envelope-from=f.ebner@proxmox.com; helo=proxmox-new.maurer-it.com X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, RCVD_IN_VALIDITY_RPBL_BLOCKED=0.001, RCVD_IN_VALIDITY_SAFE_BLOCKED=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: qemu-devel-bounces+importer=patchew.org@nongnu.org X-ZM-MESSAGEID: 1748265979692116600 Content-Type: text/plain; charset="utf-8" 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 --- Changes in v3: * Clarify that _locked() version was used iff caller already holds the graph lock. * Mention that certain kinds of issues are not solved by these changes alone. * Rebase on changes to previous patch. 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(-) 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_ab= ort(void *opaque) bdrv_replace_child_noperm(s->child, NULL); =20 if (bdrv_get_aio_context(bs) !=3D s->old_child_ctx) { - bdrv_try_change_aio_context(bs, s->old_child_ctx, NULL, &error_abo= rt); + bdrv_drain_all_begin(); + bdrv_try_change_aio_context_locked(bs, s->old_child_ctx, NULL, + &error_abort); + bdrv_drain_all_end(); } =20 if (bdrv_child_get_parent_aio_context(s->child) !=3D s->old_parent_ctx= ) { @@ -3115,8 +3118,10 @@ bdrv_attach_child_common(BlockDriverState *child_bs, parent_ctx =3D bdrv_child_get_parent_aio_context(new_child); if (child_ctx !=3D parent_ctx) { Error *local_err =3D NULL; - int ret =3D bdrv_try_change_aio_context(child_bs, parent_ctx, NULL, - &local_err); + bdrv_drain_all_begin(); + int ret =3D bdrv_try_change_aio_context_locked(child_bs, parent_ct= x, NULL, + &local_err); + bdrv_drain_all_end(); =20 if (ret < 0 && child_class->change_aio_ctx) { Transaction *aio_ctx_tran =3D 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(); } =20 bdrv_schedule_unref(child_bs); @@ -7719,9 +7726,13 @@ bdrv_change_aio_context(BlockDriverState *bs, AioCon= text *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 *c= tx, + BdrvChild *ignore_child, Error **er= rp) { Transaction *tran; GHashTable *visited; @@ -7730,17 +7741,15 @@ int bdrv_try_change_aio_context(BlockDriverState *b= s, AioContext *ctx, =20 /* * 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 every= thing - * 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 success= ful + * (the transaction itself). */ tran =3D tran_new(); visited =3D 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 =3D bdrv_change_aio_context(bs, ctx, visited, tran, errp); g_hash_table_destroy(visited); =20 @@ -7754,15 +7763,34 @@ int bdrv_try_change_aio_context(BlockDriverState *b= s, 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; } =20 tran_commit(tran); + return 0; +} + +/* + * Change bs's and recursively all of its parents' and children's AioConte= xt + * 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 =3D bdrv_try_change_aio_context_locked(bs, ctx, ignore_child, errp= ); bdrv_graph_rdunlock_main_loop(); bdrv_drain_all_end(); - return 0; + + return ret; } =20 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_n= ame, StrOrNull *iothread, AioContext *new_context; BlockDriverState *bs; =20 - GRAPH_RDLOCK_GUARD_MAINLOOP(); + bdrv_drain_all_begin(); + bdrv_graph_rdlock_main_loop(); =20 bs =3D bdrv_find_node(node_name); if (!bs) { error_setg(errp, "Failed to find node with node-name=3D'%s'", node= _name); - return; + goto out; } =20 /* Protects against accidents. */ @@ -3614,14 +3615,14 @@ void qmp_x_blockdev_set_iothread(const char *node_n= ame, StrOrNull *iothread, error_setg(errp, "Node %s is associated with a BlockBackend and co= uld " "be in use (use force=3Dtrue to override this che= ck)", node_name); - return; + goto out; } =20 if (iothread->type =3D=3D QTYPE_QSTRING) { IOThread *obj =3D iothread_by_id(iothread->u.s); if (!obj) { error_setg(errp, "Cannot find iothread %s", iothread->u.s); - return; + goto out; } =20 new_context =3D iothread_get_aio_context(obj); @@ -3629,7 +3630,11 @@ void qmp_x_blockdev_set_iothread(const char *node_na= me, StrOrNull *iothread, new_context =3D qemu_get_aio_context(); } =20 - 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(); } =20 QemuOptsList qemu_common_drive_opts =3D { diff --git a/include/block/block-global-state.h b/include/block/block-globa= l-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); =20 int GRAPH_RDLOCK bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *b= sz); 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 =3D bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR, &error_abort); =20 - bdrv_drained_begin(bs); bdrv_try_change_aio_context(bs, ctx_a, NULL, &error_abort); - bdrv_drained_end(bs); =20 - 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_a= bort); - bdrv_drained_end(bs); =20 bdrv_unref(bs); iothread_join(a); --=20 2.39.5