From nobody Sat Nov 15 19:43:27 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 1748266081717669.8060753691663; Mon, 26 May 2025 06:28:01 -0700 (PDT) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1uJXn6-0000u8-HG; Mon, 26 May 2025 09:22:56 -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 1uJXmf-0000KF-5V; Mon, 26 May 2025 09:22:30 -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 1uJXmX-0000L9-My; Mon, 26 May 2025 09:22:27 -0400 Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D2DE0445E8; 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 13/24] block: move drain outside of bdrv_attach_child() Date: Mon, 26 May 2025 15:21:29 +0200 Message-Id: <20250526132140.1641377-14-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: 1748266082915116600 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". The function bdrv_attach_child() runs under the graph lock, so it is not allowed to drain. It is called by: 1. replication_start() 2. quorum_add_child() 3. bdrv_open_child_common() 4. Throughout test-bdrv-graph-mod.c and test-bdrv-drain.c unit tests. In all callers, a drained section is introduced. The function quorum_add_child() runs under the graph lock, so it is not actually allowed to drain. This will be addressed by the following commit. Signed-off-by: Fiona Ebner Reviewed-by: Kevin Wolf --- No changes in v3. block.c | 4 ++-- block/quorum.c | 2 ++ block/replication.c | 5 +++++ tests/unit/test-bdrv-drain.c | 14 ++++++++++++++ tests/unit/test-bdrv-graph-mod.c | 10 ++++++++++ 5 files changed, 33 insertions(+), 2 deletions(-) diff --git a/block.c b/block.c index c0d2670ac7..66fbc707f3 100644 --- a/block.c +++ b/block.c @@ -3275,7 +3275,6 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent= _bs, =20 GLOBAL_STATE_CODE(); =20 - bdrv_drain_all_begin(); child =3D bdrv_attach_child_noperm(parent_bs, child_bs, child_name, child_class, child_role, tran, errp); if (!child) { @@ -3290,7 +3289,6 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent= _bs, =20 out: tran_finalize(tran, ret); - bdrv_drain_all_end(); =20 bdrv_schedule_unref(child_bs); =20 @@ -3786,10 +3784,12 @@ static BdrvChild *bdrv_open_child_common(const char= *filename, return NULL; } =20 + bdrv_drain_all_begin(); bdrv_graph_wrlock(); child =3D bdrv_attach_child(parent, bs, bdref_key, child_class, child_= role, errp); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); =20 return child; } diff --git a/block/quorum.c b/block/quorum.c index ed8ce801ee..ea17b0ec13 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -1096,8 +1096,10 @@ quorum_add_child(BlockDriverState *bs, BlockDriverSt= ate *child_bs, Error **errp) /* We can safely add the child now */ bdrv_ref(child_bs); =20 + bdrv_drain_all_begin(); child =3D bdrv_attach_child(bs, child_bs, indexstr, &child_of_bds, BDRV_CHILD_DATA, errp); + bdrv_drain_all_end(); if (child =3D=3D NULL) { s->next_child_index--; return; diff --git a/block/replication.c b/block/replication.c index 07f274de9e..54cbd03e00 100644 --- a/block/replication.c +++ b/block/replication.c @@ -540,6 +540,7 @@ static void replication_start(ReplicationState *rs, Rep= licationMode mode, return; } =20 + bdrv_drain_all_begin(); bdrv_graph_wrlock(); =20 bdrv_ref(hidden_disk->bs); @@ -549,6 +550,7 @@ static void replication_start(ReplicationState *rs, Rep= licationMode mode, if (local_err) { error_propagate(errp, local_err); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); return; } =20 @@ -559,6 +561,7 @@ static void replication_start(ReplicationState *rs, Rep= licationMode mode, if (local_err) { error_propagate(errp, local_err); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); return; } =20 @@ -571,12 +574,14 @@ static void replication_start(ReplicationState *rs, R= eplicationMode mode, !check_top_bs(top_bs, bs)) { error_setg(errp, "No top_bs or it is invalid"); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); reopen_backing_file(bs, false, NULL); return; } bdrv_op_block_all(top_bs, s->blocker); =20 bdrv_graph_wrunlock(); + bdrv_drain_all_end(); =20 s->backup_job =3D backup_job_create( NULL, s->secondary_disk->bs, s->hidden_dis= k->bs, diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index 4f3057844b..ac76525e5a 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -1049,10 +1049,12 @@ static void do_test_delete_by_drain(bool detach_ins= tead_of_delete, =20 null_bs =3D bdrv_open("null-co://", NULL, NULL, BDRV_O_RDWR | BDRV_O_P= ROTOCOL, &error_abort); + bdrv_drain_all_begin(); bdrv_graph_wrlock(); bdrv_attach_child(bs, null_bs, "null-child", &child_of_bds, BDRV_CHILD_DATA, &error_abort); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); =20 /* This child will be the one to pass to requests through to, and * it will stall until a drain occurs */ @@ -1060,21 +1062,25 @@ static void do_test_delete_by_drain(bool detach_ins= tead_of_delete, &error_abort); child_bs->total_sectors =3D 65536 >> BDRV_SECTOR_BITS; /* Takes our reference to child_bs */ + bdrv_drain_all_begin(); bdrv_graph_wrlock(); tts->wait_child =3D bdrv_attach_child(bs, child_bs, "wait-child", &child_of_bds, BDRV_CHILD_DATA | BDRV_CHILD_PRIMA= RY, &error_abort); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); =20 /* This child is just there to be deleted * (for detach_instead_of_delete =3D=3D true) */ null_bs =3D bdrv_open("null-co://", NULL, NULL, BDRV_O_RDWR | BDRV_O_P= ROTOCOL, &error_abort); + bdrv_drain_all_begin(); bdrv_graph_wrlock(); bdrv_attach_child(bs, null_bs, "null-child", &child_of_bds, BDRV_CHILD= _DATA, &error_abort); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); =20 blk =3D blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL); blk_insert_bs(blk, bs, &error_abort); @@ -1157,6 +1163,7 @@ static void no_coroutine_fn detach_indirect_bh(void *= opaque) =20 bdrv_dec_in_flight(data->child_b->bs); =20 + bdrv_drain_all_begin(); bdrv_graph_wrlock(); bdrv_unref_child(data->parent_b, data->child_b); =20 @@ -1165,6 +1172,7 @@ static void no_coroutine_fn detach_indirect_bh(void *= opaque) &child_of_bds, BDRV_CHILD_DATA, &error_abort); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); } =20 static void coroutine_mixed_fn detach_by_parent_aio_cb(void *opaque, int r= et) @@ -1262,6 +1270,7 @@ static void TSA_NO_TSA test_detach_indirect(bool by_p= arent_cb) /* Set child relationships */ bdrv_ref(b); bdrv_ref(a); + bdrv_drain_all_begin(); bdrv_graph_wrlock(); child_b =3D bdrv_attach_child(parent_b, b, "PB-B", &child_of_bds, BDRV_CHILD_DATA, &error_abort); @@ -1273,6 +1282,7 @@ static void TSA_NO_TSA test_detach_indirect(bool by_p= arent_cb) by_parent_cb ? &child_of_bds : &detach_by_driver_cb_= class, BDRV_CHILD_DATA, &error_abort); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); =20 g_assert_cmpint(parent_a->refcnt, =3D=3D, 1); g_assert_cmpint(parent_b->refcnt, =3D=3D, 1); @@ -1685,6 +1695,7 @@ static void test_drop_intermediate_poll(void) * Establish the chain last, so the chain links are the first * elements in the BDS.parents lists */ + bdrv_drain_all_begin(); bdrv_graph_wrlock(); for (i =3D 0; i < 3; i++) { if (i) { @@ -1694,6 +1705,7 @@ static void test_drop_intermediate_poll(void) } } bdrv_graph_wrunlock(); + bdrv_drain_all_end(); =20 job =3D block_job_create("job", &test_simple_job_driver, NULL, job_nod= e, 0, BLK_PERM_ALL, 0, 0, NULL, NULL, &error_abort= ); @@ -1940,10 +1952,12 @@ static void do_test_replace_child_mid_drain(int old= _drain_count, new_child_bs->total_sectors =3D 1; =20 bdrv_ref(old_child_bs); + bdrv_drain_all_begin(); bdrv_graph_wrlock(); bdrv_attach_child(parent_bs, old_child_bs, "child", &child_of_bds, BDRV_CHILD_COW, &error_abort); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); parent_s->setup_completed =3D true; =20 for (i =3D 0; i < old_drain_count; i++) { diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-= mod.c index d743abb4bb..7b03ebe4b0 100644 --- a/tests/unit/test-bdrv-graph-mod.c +++ b/tests/unit/test-bdrv-graph-mod.c @@ -137,10 +137,12 @@ static void test_update_perm_tree(void) =20 blk_insert_bs(root, bs, &error_abort); =20 + bdrv_drain_all_begin(); bdrv_graph_wrlock(); bdrv_attach_child(filter, bs, "child", &child_of_bds, BDRV_CHILD_DATA, &error_abort); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); =20 ret =3D bdrv_append(filter, bs, NULL); g_assert_cmpint(ret, <, 0); @@ -204,11 +206,13 @@ static void test_should_update_child(void) =20 bdrv_set_backing_hd(target, bs, &error_abort); =20 + bdrv_drain_all_begin(); bdrv_graph_wrlock(); g_assert(target->backing->bs =3D=3D bs); bdrv_attach_child(filter, target, "target", &child_of_bds, BDRV_CHILD_DATA, &error_abort); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); bdrv_append(filter, bs, &error_abort); =20 bdrv_graph_rdlock_main_loop(); @@ -244,6 +248,7 @@ static void test_parallel_exclusive_write(void) bdrv_ref(base); bdrv_ref(fl1); =20 + bdrv_drain_all_begin(); bdrv_graph_wrlock(); bdrv_attach_child(top, fl1, "backing", &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, @@ -257,6 +262,7 @@ static void test_parallel_exclusive_write(void) =20 bdrv_replace_node(fl1, fl2, &error_abort); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); =20 bdrv_drained_end(fl2); bdrv_drained_end(fl1); @@ -363,6 +369,7 @@ static void test_parallel_perm_update(void) */ bdrv_ref(base); =20 + bdrv_drain_all_begin(); bdrv_graph_wrlock(); bdrv_attach_child(top, ws, "file", &child_of_bds, BDRV_CHILD_DATA, &error_abort); @@ -377,6 +384,7 @@ static void test_parallel_perm_update(void) BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, &error_abort); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); =20 /* Select fl1 as first child to be active */ s->selected =3D c_fl1; @@ -430,11 +438,13 @@ static void test_append_greedy_filter(void) BlockDriverState *base =3D no_perm_node("base"); BlockDriverState *fl =3D exclusive_writer_node("fl1"); =20 + bdrv_drain_all_begin(); bdrv_graph_wrlock(); bdrv_attach_child(top, base, "backing", &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, &error_abort); bdrv_graph_wrunlock(); + bdrv_drain_all_end(); =20 bdrv_append(fl, base, &error_abort); bdrv_unref(fl); --=20 2.39.5