From 903df115aaa5b9455ffde2894002f3f4820868bd Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:36 +0200 Subject: test-bdrv-drain: Don't call bdrv_graph_wrlock() in coroutine context AIO callbacks are effectively coroutine_mixed_fn. If AIO requests don't return immediately, their callback is called from the request coroutine. This means that in AIO callbacks, we can't call no_coroutine_fns such as bdrv_graph_wrlock(). Unfortunately test-bdrv-drain does so. Change the test to use a BH to drop out of coroutine context, and add coroutine_mixed_fn and no_coroutine_fn markers to clarify the context each function runs in. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-2-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- tests/unit/test-bdrv-drain.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'tests') diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index 0b603e7..bdd3757 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -1168,7 +1168,7 @@ struct detach_by_parent_data { }; static struct detach_by_parent_data detach_by_parent_data; -static void detach_indirect_bh(void *opaque) +static void no_coroutine_fn detach_indirect_bh(void *opaque) { struct detach_by_parent_data *data = opaque; @@ -1184,14 +1184,15 @@ static void detach_indirect_bh(void *opaque) bdrv_graph_wrunlock(); } -static void detach_by_parent_aio_cb(void *opaque, int ret) +static void coroutine_mixed_fn detach_by_parent_aio_cb(void *opaque, int ret) { struct detach_by_parent_data *data = &detach_by_parent_data; g_assert_cmpint(ret, ==, 0); if (data->by_parent_cb) { bdrv_inc_in_flight(data->child_b->bs); - detach_indirect_bh(data); + aio_bh_schedule_oneshot(qemu_get_current_aio_context(), + detach_indirect_bh, &detach_by_parent_data); } } -- cgit v1.1 From 2b3912f1350971fbc2c04d986a1d0c60ae757c78 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:39 +0200 Subject: block: Mark bdrv_first_blk() and bdrv_is_root_node() GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_first_blk() and bdrv_is_root_node() need to hold a reader lock for the graph. These functions are the only functions in block-backend.c that access the parent list of a node. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-5-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- tests/unit/test-block-iothread.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'tests') diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c index 9155547..151049b 100644 --- a/tests/unit/test-block-iothread.c +++ b/tests/unit/test-block-iothread.c @@ -383,6 +383,9 @@ static void test_sync_op_check(BdrvChild *c) static void test_sync_op_activate(BdrvChild *c) { + GLOBAL_STATE_CODE(); + GRAPH_RDLOCK_GUARD_MAINLOOP(); + /* Early success: Image is not inactive */ bdrv_activate(c->bs, NULL); } -- cgit v1.1 From d05ab380db649d882396653f9830b67d84bffbe1 Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Fri, 29 Sep 2023 16:51:40 +0200 Subject: block: Mark drain related functions GRAPH_RDLOCK Draining recursively traverses the graph, therefore we need to make sure that also such accesses to the graph are protected by the graph rdlock. There are 3 different drain callers to consider: 1. drain in the main loop: no issue at all, rdlock is nop. 2. drain in an iothread: rdlock only works in main loop or coroutines, so disallow it. 3. drain in a coroutine (regardless of AioContext): the drain mechanism takes care of scheduling a BH in the bs->aio_context that will then take care of perform the actual draining. This is wrong, because as pointed in (2) if bs->aio_context is an iothread then rdlock won't work. Therefore change bdrv_co_yield_to_drain to schedule the BH in the main loop. Caller (2) also implies that we need to modify test-bdrv-drain.c to disallow draining in the iothreads. For some places, we know that they will hold the lock, but we don't have the GRAPH_RDLOCK annotations yet. In this case, add assume_graph_lock() with a FIXME comment. These places will be removed once everything is properly annotated. Signed-off-by: Emanuele Giuseppe Esposito Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-6-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- tests/unit/test-bdrv-drain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index bdd3757..d734829 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -1196,7 +1196,7 @@ static void coroutine_mixed_fn detach_by_parent_aio_cb(void *opaque, int ret) } } -static void detach_by_driver_cb_drained_begin(BdrvChild *child) +static void GRAPH_RDLOCK detach_by_driver_cb_drained_begin(BdrvChild *child) { struct detach_by_parent_data *data = &detach_by_parent_data; @@ -1233,7 +1233,7 @@ static BdrvChildClass detach_by_driver_cb_class; * state is messed up, but if it is only polled in the single * BDRV_POLL_WHILE() at the end of the drain, this should work fine. */ -static void test_detach_indirect(bool by_parent_cb) +static void TSA_NO_TSA test_detach_indirect(bool by_parent_cb) { BlockBackend *blk; BlockDriverState *parent_a, *parent_b, *a, *b, *c; -- cgit v1.1 From b59b466071391cb76b39584e1558be2d0797c054 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:55 +0200 Subject: block: Protect bs->parents with graph_lock Almost all functions that access the parent link already take the graph lock now. Add locking to the remaining user in a test case and finally annotate the struct field itself as protected by the graph lock. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-21-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- tests/unit/test-block-iothread.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'tests') diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c index 151049b..9b15d27 100644 --- a/tests/unit/test-block-iothread.c +++ b/tests/unit/test-block-iothread.c @@ -471,11 +471,16 @@ static void test_sync_op(const void *opaque) BlockDriverState *bs; BdrvChild *c; + GLOBAL_STATE_CODE(); + blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL); bs = bdrv_new_open_driver(&bdrv_test, "base", BDRV_O_RDWR, &error_abort); bs->total_sectors = 65536 / BDRV_SECTOR_SIZE; blk_insert_bs(blk, bs, &error_abort); + + bdrv_graph_rdlock_main_loop(); c = QLIST_FIRST(&bs->parents); + bdrv_graph_rdunlock_main_loop(); blk_set_aio_context(blk, ctx, &error_abort); aio_context_acquire(ctx); -- cgit v1.1 From 680e0cc40c5830ebcbfa0bce99bf932e1a4cf6c6 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:56 +0200 Subject: block: Protect bs->children with graph_lock Almost all functions that access the child links already take the graph lock now. Add locking to the remaining users and finally annotate the struct field itself as protected by the graph lock. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-22-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- tests/unit/test-bdrv-drain.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'tests') diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index d734829..f67e9df 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -1034,9 +1034,13 @@ static void coroutine_fn test_co_delete_by_drain(void *opaque) blk_co_unref(blk); } else { BdrvChild *c, *next_c; + bdrv_graph_co_rdlock(); QLIST_FOREACH_SAFE(c, &bs->children, next, next_c) { + bdrv_graph_co_rdunlock(); bdrv_co_unref_child(bs, c); + bdrv_graph_co_rdlock(); } + bdrv_graph_co_rdunlock(); } dbdd->done = true; -- cgit v1.1