mirrors/qemu
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

block: Mark drain related functions GRAPH_RDLOCK

Browse Source 
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 <eesposit@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20230929145157.45443-6-kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
This commit is contained in:
Emanuele Giuseppe Esposito 2023-09-29 16:51:40 +02:00 committed by Kevin Wolf
parent 2b3912f135
commit d05ab380db
5 changed files with 54 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
block.c
View File

@ -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);

30
block/io.c
View File

@ -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,
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. */

21
include/block/block-io.h
View File

@ -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,7 +398,8 @@ 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 GRAPH_RDLOCK
bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent,
bool ignore_bds_parents);
/**
@ -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);

6
include/block/block_int-common.h
View File

@ -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.

4
tests/unit/test-bdrv-drain.c
View File

@ -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;