From d05ab380db649d882396653f9830b67d84bffbe1 Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Fri, 29 Sep 2023 16:51:40 +0200 Subject: [PATCH] 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 --- block.c | 6 +++--- block/io.c | 32 ++++++++++++++++++++++++++++---- include/block/block-io.h | 23 ++++++++++++++++++----- include/block/block_int-common.h | 6 +++--- tests/unit/test-bdrv-drain.c | 4 ++-- 5 files changed, 54 insertions(+), 17 deletions(-) diff --git a/block.c b/block.c index 701f77fe2f..6cc4115510 100644 --- a/block.c +++ b/block.c @@ -1192,19 +1192,19 @@ static char *bdrv_child_get_parent_desc(BdrvChild *c) return g_strdup_printf("node '%s'", bdrv_get_node_name(parent)); } -static void bdrv_child_cb_drained_begin(BdrvChild *child) +static void GRAPH_RDLOCK bdrv_child_cb_drained_begin(BdrvChild *child) { BlockDriverState *bs = child->opaque; bdrv_do_drained_begin_quiesce(bs, NULL); } -static bool bdrv_child_cb_drained_poll(BdrvChild *child) +static bool GRAPH_RDLOCK bdrv_child_cb_drained_poll(BdrvChild *child) { BlockDriverState *bs = child->opaque; return bdrv_drain_poll(bs, NULL, false); } -static void bdrv_child_cb_drained_end(BdrvChild *child) +static void GRAPH_RDLOCK bdrv_child_cb_drained_end(BdrvChild *child) { BlockDriverState *bs = child->opaque; bdrv_drained_end(bs); diff --git a/block/io.c b/block/io.c index 5821a4d1f5..e2e846d37d 100644 --- a/block/io.c +++ b/block/io.c @@ -46,9 +46,12 @@ static void bdrv_parent_cb_resize(BlockDriverState *bs); static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes, BdrvRequestFlags flags); -static void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore) +static void GRAPH_RDLOCK +bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore) { BdrvChild *c, *next; + IO_OR_GS_CODE(); + assert_bdrv_graph_readable(); QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) { if (c == ignore) { @@ -70,9 +73,12 @@ void bdrv_parent_drained_end_single(BdrvChild *c) } } -static void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore) +static void GRAPH_RDLOCK +bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore) { BdrvChild *c; + IO_OR_GS_CODE(); + assert_bdrv_graph_readable(); QLIST_FOREACH(c, &bs->parents, next_parent) { if (c == ignore) { @@ -84,17 +90,22 @@ static void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore) bool bdrv_parent_drained_poll_single(BdrvChild *c) { + IO_OR_GS_CODE(); + if (c->klass->drained_poll) { return c->klass->drained_poll(c); } return false; } -static bool bdrv_parent_drained_poll(BlockDriverState *bs, BdrvChild *ignore, - bool ignore_bds_parents) +static bool GRAPH_RDLOCK +bdrv_parent_drained_poll(BlockDriverState *bs, BdrvChild *ignore, + bool ignore_bds_parents) { BdrvChild *c, *next; bool busy = false; + IO_OR_GS_CODE(); + assert_bdrv_graph_readable(); QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) { if (c == ignore || (ignore_bds_parents && c->klass->parent_is_bds)) { @@ -114,6 +125,7 @@ void bdrv_parent_drained_begin_single(BdrvChild *c) c->quiesced_parent = true; if (c->klass->drained_begin) { + /* called with rdlock taken, but it doesn't really need it. */ c->klass->drained_begin(c); } } @@ -263,6 +275,9 @@ bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent, static bool bdrv_drain_poll_top_level(BlockDriverState *bs, BdrvChild *ignore_parent) { + GLOBAL_STATE_CODE(); + GRAPH_RDLOCK_GUARD_MAINLOOP(); + return bdrv_drain_poll(bs, ignore_parent, false); } @@ -362,6 +377,7 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent, /* Stop things in parent-to-child order */ if (qatomic_fetch_inc(&bs->quiesce_counter) == 0) { + GRAPH_RDLOCK_GUARD_MAINLOOP(); bdrv_parent_drained_begin(bs, parent); if (bs->drv && bs->drv->bdrv_drain_begin) { bs->drv->bdrv_drain_begin(bs); @@ -408,12 +424,16 @@ static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent) bdrv_co_yield_to_drain(bs, false, parent, false); return; } + + /* At this point, we should be always running in the main loop. */ + GLOBAL_STATE_CODE(); assert(bs->quiesce_counter > 0); GLOBAL_STATE_CODE(); /* Re-enable things in child-to-parent order */ old_quiesce_counter = qatomic_fetch_dec(&bs->quiesce_counter); if (old_quiesce_counter == 1) { + GRAPH_RDLOCK_GUARD_MAINLOOP(); if (bs->drv && bs->drv->bdrv_drain_end) { bs->drv->bdrv_drain_end(bs); } @@ -437,6 +457,8 @@ void bdrv_drain(BlockDriverState *bs) static void bdrv_drain_assert_idle(BlockDriverState *bs) { BdrvChild *child, *next; + GLOBAL_STATE_CODE(); + GRAPH_RDLOCK_GUARD_MAINLOOP(); assert(qatomic_read(&bs->in_flight) == 0); QLIST_FOREACH_SAFE(child, &bs->children, next, next) { @@ -450,7 +472,9 @@ static bool bdrv_drain_all_poll(void) { BlockDriverState *bs = NULL; bool result = false; + GLOBAL_STATE_CODE(); + GRAPH_RDLOCK_GUARD_MAINLOOP(); /* bdrv_drain_poll() can't make changes to the graph and we are holding the * main AioContext lock, so iterating bdrv_next_all_states() is safe. */ diff --git a/include/block/block-io.h b/include/block/block-io.h index 6485b194a4..9707eb3eff 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -370,7 +370,7 @@ bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos); * * Begin a quiesced section for the parent of @c. */ -void bdrv_parent_drained_begin_single(BdrvChild *c); +void GRAPH_RDLOCK bdrv_parent_drained_begin_single(BdrvChild *c); /** * bdrv_parent_drained_poll_single: @@ -378,14 +378,14 @@ void bdrv_parent_drained_begin_single(BdrvChild *c); * Returns true if there is any pending activity to cease before @c can be * called quiesced, false otherwise. */ -bool bdrv_parent_drained_poll_single(BdrvChild *c); +bool GRAPH_RDLOCK bdrv_parent_drained_poll_single(BdrvChild *c); /** * bdrv_parent_drained_end_single: * * End a quiesced section for the parent of @c. */ -void bdrv_parent_drained_end_single(BdrvChild *c); +void GRAPH_RDLOCK bdrv_parent_drained_end_single(BdrvChild *c); /** * bdrv_drain_poll: @@ -398,8 +398,9 @@ void bdrv_parent_drained_end_single(BdrvChild *c); * * This is part of bdrv_drained_begin. */ -bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent, - bool ignore_bds_parents); +bool GRAPH_RDLOCK +bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent, + bool ignore_bds_parents); /** * bdrv_drained_begin: @@ -407,6 +408,12 @@ bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent, * Begin a quiesced section for exclusive access to the BDS, by disabling * external request sources including NBD server, block jobs, and device model. * + * This function can only be invoked by the main loop or a coroutine + * (regardless of the AioContext where it is running). + * If the coroutine is running in an Iothread AioContext, this function will + * just schedule a BH to run in the main loop. + * However, it cannot be directly called by an Iothread. + * * This function can be recursive. */ void bdrv_drained_begin(BlockDriverState *bs); @@ -423,6 +430,12 @@ void bdrv_do_drained_begin_quiesce(BlockDriverState *bs, BdrvChild *parent); * bdrv_drained_end: * * End a quiescent section started by bdrv_drained_begin(). + * + * This function can only be invoked by the main loop or a coroutine + * (regardless of the AioContext where it is running). + * If the coroutine is running in an Iothread AioContext, this function will + * just schedule a BH to run in the main loop. + * However, it cannot be directly called by an Iothread. */ void bdrv_drained_end(BlockDriverState *bs); diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 2ca3758cb8..8ef68176a5 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -963,15 +963,15 @@ struct BdrvChildClass { * Note that this can be nested. If drained_begin() was called twice, new * I/O is allowed only after drained_end() was called twice, too. */ - void (*drained_begin)(BdrvChild *child); - void (*drained_end)(BdrvChild *child); + void GRAPH_RDLOCK_PTR (*drained_begin)(BdrvChild *child); + void GRAPH_RDLOCK_PTR (*drained_end)(BdrvChild *child); /* * Returns whether the parent has pending requests for the child. This * callback is polled after .drained_begin() has been called until all * activity on the child has stopped. */ - bool (*drained_poll)(BdrvChild *child); + bool GRAPH_RDLOCK_PTR (*drained_poll)(BdrvChild *child); /* * Notifies the parent that the filename of its child has changed (e.g. diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index bdd3757615..d734829778 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;