* [PATCH 01/22] test-bdrv-drain: Don't call bdrv_graph_wrlock() in coroutine context
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 02/22] block-coroutine-wrapper: Add no_co_wrapper_bdrv_rdlock functions Kevin Wolf
` (21 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
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 <kwolf@redhat.com>
---
tests/unit/test-bdrv-drain.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 0b603e7c57..bdd3757615 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);
}
}
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 02/22] block-coroutine-wrapper: Add no_co_wrapper_bdrv_rdlock functions
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
2023-09-29 14:51 ` [PATCH 01/22] test-bdrv-drain: Don't call bdrv_graph_wrlock() in coroutine context Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 03/22] block: Take graph rdlock in bdrv_inactivate_all() Kevin Wolf
` (20 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
Add a new wrapper type for GRAPH_RDLOCK functions that should be called
from coroutine context.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-common.h | 7 +++++--
scripts/block-coroutine-wrapper.py | 10 +++++++---
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/include/block/block-common.h b/include/block/block-common.h
index 2d2af7230d..d7599564db 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -66,13 +66,16 @@
* function. The coroutine yields after scheduling the BH and is reentered when
* the wrapped function returns.
*
- * A no_co_wrapper_bdrv_wrlock function is a no_co_wrapper function that
- * automatically takes the graph wrlock when calling the wrapped function.
+ * A no_co_wrapper_bdrv_rdlock function is a no_co_wrapper function that
+ * automatically takes the graph rdlock when calling the wrapped function. In
+ * the same way, no_co_wrapper_bdrv_wrlock functions automatically take the
+ * graph wrlock.
*
* If the first parameter of the function is a BlockDriverState, BdrvChild or
* BlockBackend pointer, the AioContext lock for it is taken in the wrapper.
*/
#define no_co_wrapper
+#define no_co_wrapper_bdrv_rdlock
#define no_co_wrapper_bdrv_wrlock
#include "block/blockjob.h"
diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
index 685d0b4ed4..a601c3c672 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -87,8 +87,9 @@ def __init__(self, wrapper_type: str, return_type: str, name: str,
raise ValueError(f"Invalid no_co function name: {self.name}")
if not self.create_only_co:
raise ValueError(f"no_co function can't be mixed: {self.name}")
- if self.graph_rdlock:
- raise ValueError(f"no_co function can't be rdlock: {self.name}")
+ if self.graph_rdlock and self.graph_wrlock:
+ raise ValueError("function can't be both rdlock and wrlock: "
+ f"{self.name}")
self.target_name = f'{subsystem}_{subname}'
self.ctx = self.gen_ctx()
@@ -256,7 +257,10 @@ def gen_no_co_wrapper(func: FuncDecl) -> str:
graph_lock=''
graph_unlock=''
- if func.graph_wrlock:
+ if func.graph_rdlock:
+ graph_lock=' bdrv_graph_rdlock_main_loop();'
+ graph_unlock=' bdrv_graph_rdunlock_main_loop();'
+ elif func.graph_wrlock:
graph_lock=' bdrv_graph_wrlock(NULL);'
graph_unlock=' bdrv_graph_wrunlock();'
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 03/22] block: Take graph rdlock in bdrv_inactivate_all()
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
2023-09-29 14:51 ` [PATCH 01/22] test-bdrv-drain: Don't call bdrv_graph_wrlock() in coroutine context Kevin Wolf
2023-09-29 14:51 ` [PATCH 02/22] block-coroutine-wrapper: Add no_co_wrapper_bdrv_rdlock functions Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 04/22] block: Mark bdrv_first_blk() and bdrv_is_root_node() GRAPH_RDLOCK Kevin Wolf
` (19 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
The function reads the parents list, so it needs to hold the graph lock.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index e7f349b25c..73fd386076 100644
--- a/block.c
+++ b/block.c
@@ -6975,7 +6975,8 @@ void bdrv_activate_all(Error **errp)
}
}
-static bool bdrv_has_bds_parent(BlockDriverState *bs, bool only_active)
+static bool GRAPH_RDLOCK
+bdrv_has_bds_parent(BlockDriverState *bs, bool only_active)
{
BdrvChild *parent;
GLOBAL_STATE_CODE();
@@ -6992,14 +6993,13 @@ static bool bdrv_has_bds_parent(BlockDriverState *bs, bool only_active)
return false;
}
-static int bdrv_inactivate_recurse(BlockDriverState *bs)
+static int GRAPH_RDLOCK bdrv_inactivate_recurse(BlockDriverState *bs)
{
BdrvChild *child, *parent;
int ret;
uint64_t cumulative_perms, cumulative_shared_perms;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
if (!bs->drv) {
return -ENOMEDIUM;
@@ -7065,6 +7065,7 @@ int bdrv_inactivate_all(void)
GSList *aio_ctxs = NULL, *ctx;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *aio_context = bdrv_get_aio_context(bs);
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 04/22] block: Mark bdrv_first_blk() and bdrv_is_root_node() GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (2 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 03/22] block: Take graph rdlock in bdrv_inactivate_all() Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 05/22] block: Mark drain related functions GRAPH_RDLOCK Kevin Wolf
` (18 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
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 <kwolf@redhat.com>
---
include/block/block-global-state.h | 9 +++++----
include/sysemu/block-backend-global-state.h | 4 ++--
block.c | 1 +
block/block-backend.c | 6 +++++-
block/export/export.c | 4 ++++
block/io.c | 1 +
block/monitor/block-hmp-cmds.c | 2 ++
block/qapi-sysemu.c | 2 ++
block/replication.c | 10 ++++++++--
block/snapshot.c | 15 +++++++++++----
blockdev.c | 5 +++++
migration/block.c | 2 ++
migration/migration-hmp-cmds.c | 2 ++
tests/unit/test-block-iothread.c | 3 +++
14 files changed, 53 insertions(+), 13 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 6061220a6c..505a0d0c11 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -170,9 +170,10 @@ BlockDriverState * GRAPH_RDLOCK
check_to_replace_node(BlockDriverState *parent_bs, const char *node_name,
Error **errp);
-int no_coroutine_fn bdrv_activate(BlockDriverState *bs, Error **errp);
+int no_coroutine_fn GRAPH_RDLOCK
+bdrv_activate(BlockDriverState *bs, Error **errp);
-int coroutine_fn no_co_wrapper
+int coroutine_fn no_co_wrapper_bdrv_rdlock
bdrv_co_activate(BlockDriverState *bs, Error **errp);
void bdrv_activate_all(Error **errp);
@@ -208,8 +209,8 @@ typedef struct BdrvNextIterator {
BlockDriverState *bs;
} BdrvNextIterator;
-BlockDriverState *bdrv_first(BdrvNextIterator *it);
-BlockDriverState *bdrv_next(BdrvNextIterator *it);
+BlockDriverState * GRAPH_RDLOCK bdrv_first(BdrvNextIterator *it);
+BlockDriverState * GRAPH_RDLOCK bdrv_next(BdrvNextIterator *it);
void bdrv_next_cleanup(BdrvNextIterator *it);
BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs);
diff --git a/include/sysemu/block-backend-global-state.h b/include/sysemu/block-backend-global-state.h
index d5f675493a..49c12b0fa9 100644
--- a/include/sysemu/block-backend-global-state.h
+++ b/include/sysemu/block-backend-global-state.h
@@ -59,8 +59,8 @@ BlockBackend *blk_by_public(BlockBackendPublic *public);
void blk_remove_bs(BlockBackend *blk);
int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp);
int blk_replace_bs(BlockBackend *blk, BlockDriverState *new_bs, Error **errp);
-bool bdrv_has_blk(BlockDriverState *bs);
-bool bdrv_is_root_node(BlockDriverState *bs);
+bool GRAPH_RDLOCK bdrv_has_blk(BlockDriverState *bs);
+bool GRAPH_RDLOCK bdrv_is_root_node(BlockDriverState *bs);
int GRAPH_UNLOCKED blk_set_perm(BlockBackend *blk, uint64_t perm,
uint64_t shared_perm, Error **errp);
void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm);
diff --git a/block.c b/block.c
index 73fd386076..61705980dd 100644
--- a/block.c
+++ b/block.c
@@ -6960,6 +6960,7 @@ void bdrv_activate_all(Error **errp)
BdrvNextIterator it;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *aio_context = bdrv_get_aio_context(bs);
diff --git a/block/block-backend.c b/block/block-backend.c
index 9dbdbe5545..c9193ae4b9 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -780,11 +780,12 @@ BlockDriverState *blk_bs(BlockBackend *blk)
return blk->root ? blk->root->bs : NULL;
}
-static BlockBackend *bdrv_first_blk(BlockDriverState *bs)
+static BlockBackend * GRAPH_RDLOCK bdrv_first_blk(BlockDriverState *bs)
{
BdrvChild *child;
GLOBAL_STATE_CODE();
+ assert_bdrv_graph_readable();
QLIST_FOREACH(child, &bs->parents, next_parent) {
if (child->klass == &child_root) {
@@ -812,6 +813,8 @@ bool bdrv_is_root_node(BlockDriverState *bs)
BdrvChild *c;
GLOBAL_STATE_CODE();
+ assert_bdrv_graph_readable();
+
QLIST_FOREACH(c, &bs->parents, next_parent) {
if (c->klass != &child_root) {
return false;
@@ -2259,6 +2262,7 @@ void blk_activate(BlockBackend *blk, Error **errp)
if (qemu_in_coroutine()) {
bdrv_co_activate(bs, errp);
} else {
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
bdrv_activate(bs, errp);
}
}
diff --git a/block/export/export.c b/block/export/export.c
index 10316b43c5..a8f274e526 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -83,6 +83,8 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
uint64_t perm;
int ret;
+ GLOBAL_STATE_CODE();
+
if (!id_wellformed(export->id)) {
error_setg(errp, "Invalid block export id");
return NULL;
@@ -145,7 +147,9 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
* access since the export could be available before migration handover.
* ctx was acquired in the caller.
*/
+ bdrv_graph_rdlock_main_loop();
bdrv_activate(bs, NULL);
+ bdrv_graph_rdunlock_main_loop();
perm = BLK_PERM_CONSISTENT_READ;
if (export->writable) {
diff --git a/block/io.c b/block/io.c
index 209a6da0c8..ca5f018a7e 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2329,6 +2329,7 @@ int bdrv_flush_all(void)
int result = 0;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
/*
* bdrv queue is managed by record/replay,
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index ca2599de44..6b7d7dd072 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -896,6 +896,8 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
SnapshotEntry *snapshot_entry;
Error *err = NULL;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
bs = bdrv_all_find_vmstate_bs(NULL, false, NULL, &err);
if (!bs) {
error_report_err(err);
diff --git a/block/qapi-sysemu.c b/block/qapi-sysemu.c
index ef07151892..e885a64c32 100644
--- a/block/qapi-sysemu.c
+++ b/block/qapi-sysemu.c
@@ -279,6 +279,8 @@ static void blockdev_insert_medium(const char *device, const char *id,
BlockBackend *blk;
BlockDriverState *bs;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
blk = qmp_get_blk(device, id, errp);
if (!blk) {
return;
diff --git a/block/replication.c b/block/replication.c
index dd166d2d82..7e0b615805 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -458,6 +458,8 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
Error *local_err = NULL;
BackupPerf perf = { .use_copy_range = true, .max_workers = 1 };
+ GLOBAL_STATE_CODE();
+
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
s = bs->opaque;
@@ -504,12 +506,15 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
return;
}
+ bdrv_graph_rdlock_main_loop();
secondary_disk = hidden_disk->bs->backing;
if (!secondary_disk->bs || !bdrv_has_blk(secondary_disk->bs)) {
error_setg(errp, "The secondary disk doesn't have block backend");
+ bdrv_graph_rdunlock_main_loop();
aio_context_release(aio_context);
return;
}
+ bdrv_graph_rdunlock_main_loop();
/* verify the length */
active_length = bdrv_getlength(active_disk->bs);
@@ -566,8 +571,6 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
return;
}
- bdrv_graph_wrunlock();
-
/* start backup job now */
error_setg(&s->blocker,
"Block device is in use by internal backup job");
@@ -576,6 +579,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
if (!top_bs || !bdrv_is_root_node(top_bs) ||
!check_top_bs(top_bs, bs)) {
error_setg(errp, "No top_bs or it is invalid");
+ bdrv_graph_wrunlock();
reopen_backing_file(bs, false, NULL);
aio_context_release(aio_context);
return;
@@ -583,6 +587,8 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
bdrv_op_block_all(top_bs, s->blocker);
bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
+ bdrv_graph_wrunlock();
+
s->backup_job = backup_job_create(
NULL, s->secondary_disk->bs, s->hidden_disk->bs,
0, MIRROR_SYNC_MODE_NONE, NULL, 0, false, NULL,
diff --git a/block/snapshot.c b/block/snapshot.c
index b86b5b24ad..633391e8a9 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -462,9 +462,9 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
}
-static int bdrv_all_get_snapshot_devices(bool has_devices, strList *devices,
- GList **all_bdrvs,
- Error **errp)
+static int GRAPH_RDLOCK
+bdrv_all_get_snapshot_devices(bool has_devices, strList *devices,
+ GList **all_bdrvs, Error **errp)
{
g_autoptr(GList) bdrvs = NULL;
@@ -496,7 +496,7 @@ static int bdrv_all_get_snapshot_devices(bool has_devices, strList *devices,
}
-static bool bdrv_all_snapshots_includes_bs(BlockDriverState *bs)
+static bool GRAPH_RDLOCK bdrv_all_snapshots_includes_bs(BlockDriverState *bs)
{
if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
return false;
@@ -518,6 +518,7 @@ bool bdrv_all_can_snapshot(bool has_devices, strList *devices,
GList *iterbdrvs;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
return false;
@@ -554,6 +555,7 @@ int bdrv_all_delete_snapshot(const char *name,
GList *iterbdrvs;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
return -1;
@@ -595,6 +597,7 @@ int bdrv_all_goto_snapshot(const char *name,
GList *iterbdrvs;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
return -1;
@@ -631,6 +634,7 @@ int bdrv_all_has_snapshot(const char *name,
GList *iterbdrvs;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
return -1;
@@ -673,7 +677,9 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
{
g_autoptr(GList) bdrvs = NULL;
GList *iterbdrvs;
+
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
return -1;
@@ -715,6 +721,7 @@ BlockDriverState *bdrv_all_find_vmstate_bs(const char *vmstate_bs,
GList *iterbdrvs;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
return NULL;
diff --git a/blockdev.c b/blockdev.c
index 325b7a3bef..b6b7c44288 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1041,6 +1041,8 @@ static BlockDriverState *qmp_get_root_bs(const char *name, Error **errp)
BlockDriverState *bs;
AioContext *aio_context;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
bs = bdrv_lookup_bs(name, name, errp);
if (bs == NULL) {
return NULL;
@@ -3509,6 +3511,7 @@ void qmp_blockdev_del(const char *node_name, Error **errp)
BlockDriverState *bs;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
bs = bdrv_find_node(node_name);
if (!bs) {
@@ -3636,6 +3639,8 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
AioContext *new_context;
BlockDriverState *bs;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
bs = bdrv_find_node(node_name);
if (!bs) {
error_setg(errp, "Failed to find node with node-name='%s'", node_name);
diff --git a/migration/block.c b/migration/block.c
index 86c2256a2b..17ac7a894f 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -388,6 +388,8 @@ static int init_blk_migration(QEMUFile *f)
Error *local_err = NULL;
int ret;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
block_mig_state.submitted = 0;
block_mig_state.read_done = 0;
block_mig_state.transferred = 0;
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index c115ef2d23..5b25ba24f7 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -794,6 +794,8 @@ static void vm_completion(ReadLineState *rs, const char *str)
BlockDriverState *bs;
BdrvNextIterator it;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
len = strlen(str);
readline_set_completion_index(rs, len);
diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c
index 9155547313..151049bda5 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);
}
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 05/22] block: Mark drain related functions GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (3 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 04/22] block: Mark bdrv_first_blk() and bdrv_is_root_node() GRAPH_RDLOCK Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 06/22] block: Mark bdrv_parent_cb_resize() and callers GRAPH_RDLOCK Kevin Wolf
` (17 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
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>
---
include/block/block-io.h | 23 ++++++++++++++++++-----
include/block/block_int-common.h | 6 +++---
block.c | 6 +++---
block/io.c | 32 ++++++++++++++++++++++++++++----
tests/unit/test-bdrv-drain.c | 4 ++--
5 files changed, 54 insertions(+), 17 deletions(-)
diff --git a/include/block/block-io.h b/include/block/block-io.h
index f1c796a1ce..cf72e39717 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -363,7 +363,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:
@@ -371,14 +371,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:
@@ -391,8 +391,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:
@@ -400,6 +401,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);
@@ -416,6 +423,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/block.c b/block.c
index 61705980dd..fb494f4a8b 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 ca5f018a7e..63f248d672 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);
@@ -407,12 +423,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);
}
@@ -436,6 +456,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) {
@@ -449,7 +471,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/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;
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 06/22] block: Mark bdrv_parent_cb_resize() and callers GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (4 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 05/22] block: Mark drain related functions GRAPH_RDLOCK Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 07/22] block: Mark bdrv_snapshot_fallback() " Kevin Wolf
` (16 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_parent_cb_resize() need to hold a reader lock for the graph.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/io.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/block/io.c b/block/io.c
index 63f248d672..df3000bd52 100644
--- a/block/io.c
+++ b/block/io.c
@@ -42,7 +42,9 @@
/* Maximum bounce buffer for copy-on-read and write zeroes, in bytes */
#define MAX_BOUNCE_BUFFER (32768 << BDRV_SECTOR_BITS)
-static void bdrv_parent_cb_resize(BlockDriverState *bs);
+static void coroutine_fn GRAPH_RDLOCK
+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);
@@ -2026,7 +2028,7 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, int64_t bytes,
}
}
-static inline void coroutine_fn
+static inline void coroutine_fn GRAPH_RDLOCK
bdrv_co_write_req_finish(BdrvChild *child, int64_t offset, int64_t bytes,
BdrvTrackedRequest *req, int ret)
{
@@ -3575,9 +3577,13 @@ int coroutine_fn bdrv_co_copy_range(BdrvChild *src, int64_t src_offset,
bytes, read_flags, write_flags);
}
-static void bdrv_parent_cb_resize(BlockDriverState *bs)
+static void coroutine_fn GRAPH_RDLOCK
+bdrv_parent_cb_resize(BlockDriverState *bs)
{
BdrvChild *c;
+
+ assert_bdrv_graph_readable();
+
QLIST_FOREACH(c, &bs->parents, next_parent) {
if (c->klass->resize) {
c->klass->resize(c);
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 07/22] block: Mark bdrv_snapshot_fallback() and callers GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (5 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 06/22] block: Mark bdrv_parent_cb_resize() and callers GRAPH_RDLOCK Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 08/22] block: Take graph rdlock in parts of reopen Kevin Wolf
` (15 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_snapshot_fallback() need to hold a reader lock for the graph
because it accesses the children list of a node.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block_int-common.h | 18 ++++++++--------
include/block/snapshot.h | 24 +++++++++++++---------
block/snapshot.c | 35 ++++++++++++++++++++++++++------
blockdev.c | 9 ++++++++
qemu-img.c | 5 +++++
5 files changed, 67 insertions(+), 24 deletions(-)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 8ef68176a5..29c5b8a3c5 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -313,14 +313,16 @@ struct BlockDriver {
int GRAPH_RDLOCK_PTR (*bdrv_inactivate)(BlockDriverState *bs);
- int (*bdrv_snapshot_create)(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info);
- int (*bdrv_snapshot_goto)(BlockDriverState *bs,
- const char *snapshot_id);
- int (*bdrv_snapshot_delete)(BlockDriverState *bs,
- const char *snapshot_id,
- const char *name,
- Error **errp);
+ int GRAPH_RDLOCK_PTR (*bdrv_snapshot_create)(
+ BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
+
+ int GRAPH_UNLOCKED_PTR (*bdrv_snapshot_goto)(
+ BlockDriverState *bs, const char *snapshot_id);
+
+ int GRAPH_RDLOCK_PTR (*bdrv_snapshot_delete)(
+ BlockDriverState *bs, const char *snapshot_id, const char *name,
+ Error **errp);
+
int (*bdrv_snapshot_list)(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info);
int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 50ff924710..d49c5599d9 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -25,6 +25,7 @@
#ifndef SNAPSHOT_H
#define SNAPSHOT_H
+#include "block/graph-lock.h"
#include "qapi/qapi-builtin-types.h"
#define SNAPSHOT_OPT_BASE "snapshot."
@@ -59,16 +60,19 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
const char *name,
QEMUSnapshotInfo *sn_info,
Error **errp);
-int bdrv_can_snapshot(BlockDriverState *bs);
-int bdrv_snapshot_create(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info);
-int bdrv_snapshot_goto(BlockDriverState *bs,
- const char *snapshot_id,
- Error **errp);
-int bdrv_snapshot_delete(BlockDriverState *bs,
- const char *snapshot_id,
- const char *name,
- Error **errp);
+
+int GRAPH_RDLOCK bdrv_can_snapshot(BlockDriverState *bs);
+
+int GRAPH_RDLOCK
+bdrv_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
+
+int GRAPH_UNLOCKED
+bdrv_snapshot_goto(BlockDriverState *bs, const char *snapshot_id, Error **errp);
+
+int GRAPH_RDLOCK
+bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id,
+ const char *name, Error **errp);
+
int bdrv_snapshot_list(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info);
int bdrv_snapshot_load_tmp(BlockDriverState *bs,
diff --git a/block/snapshot.c b/block/snapshot.c
index 633391e8a9..554065578b 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -155,11 +155,15 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
* back if the given BDS does not support snapshots.
* Return NULL if there is no BDS to (safely) fall back to.
*/
-static BdrvChild *bdrv_snapshot_fallback_child(BlockDriverState *bs)
+static BdrvChild * GRAPH_RDLOCK
+bdrv_snapshot_fallback_child(BlockDriverState *bs)
{
BdrvChild *fallback = bdrv_primary_child(bs);
BdrvChild *child;
+ GLOBAL_STATE_CODE();
+ assert_bdrv_graph_readable();
+
/* We allow fallback only to primary child */
if (!fallback) {
return NULL;
@@ -182,8 +186,10 @@ static BdrvChild *bdrv_snapshot_fallback_child(BlockDriverState *bs)
return fallback;
}
-static BlockDriverState *bdrv_snapshot_fallback(BlockDriverState *bs)
+static BlockDriverState * GRAPH_RDLOCK
+bdrv_snapshot_fallback(BlockDriverState *bs)
{
+ GLOBAL_STATE_CODE();
return child_bs(bdrv_snapshot_fallback_child(bs));
}
@@ -254,7 +260,10 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
return ret;
}
+ bdrv_graph_rdlock_main_loop();
fallback = bdrv_snapshot_fallback_child(bs);
+ bdrv_graph_rdunlock_main_loop();
+
if (fallback) {
QDict *options;
QDict *file_options;
@@ -374,10 +383,12 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
int bdrv_snapshot_list(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info)
{
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
BlockDriver *drv = bs->drv;
BlockDriverState *fallback_bs = bdrv_snapshot_fallback(bs);
- GLOBAL_STATE_CODE();
if (!drv) {
return -ENOMEDIUM;
}
@@ -498,6 +509,9 @@ bdrv_all_get_snapshot_devices(bool has_devices, strList *devices,
static bool GRAPH_RDLOCK bdrv_all_snapshots_includes_bs(BlockDriverState *bs)
{
+ GLOBAL_STATE_CODE();
+ assert_bdrv_graph_readable();
+
if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
return false;
}
@@ -595,11 +609,15 @@ int bdrv_all_goto_snapshot(const char *name,
{
g_autoptr(GList) bdrvs = NULL;
GList *iterbdrvs;
+ int ret;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
- if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
+ bdrv_graph_rdlock_main_loop();
+ ret = bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp);
+ bdrv_graph_rdunlock_main_loop();
+
+ if (ret < 0) {
return -1;
}
@@ -608,9 +626,14 @@ int bdrv_all_goto_snapshot(const char *name,
BlockDriverState *bs = iterbdrvs->data;
AioContext *ctx = bdrv_get_aio_context(bs);
int ret = 0;
+ bool all_snapshots_includes_bs;
aio_context_acquire(ctx);
- if (devices || bdrv_all_snapshots_includes_bs(bs)) {
+ bdrv_graph_rdlock_main_loop();
+ all_snapshots_includes_bs = bdrv_all_snapshots_includes_bs(bs);
+ bdrv_graph_rdunlock_main_loop();
+
+ if (devices || all_snapshots_includes_bs) {
ret = bdrv_snapshot_goto(bs, name, errp);
}
aio_context_release(ctx);
diff --git a/blockdev.c b/blockdev.c
index b6b7c44288..43e0c4ddf2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1138,6 +1138,9 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
SnapshotInfo *info = NULL;
int ret;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
bs = qmp_get_root_bs(device, errp);
if (!bs) {
return NULL;
@@ -1223,6 +1226,9 @@ static void internal_snapshot_action(BlockdevSnapshotInternal *internal,
AioContext *aio_context;
int ret1;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
tran_add(tran, &internal_snapshot_drv, state);
device = internal->device;
@@ -1311,6 +1317,9 @@ static void internal_snapshot_abort(void *opaque)
AioContext *aio_context;
Error *local_error = NULL;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (!state->created) {
return;
}
diff --git a/qemu-img.c b/qemu-img.c
index a48edb7101..9e293bdf78 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3470,7 +3470,10 @@ static int img_snapshot(int argc, char **argv)
sn.date_sec = rt / G_USEC_PER_SEC;
sn.date_nsec = (rt % G_USEC_PER_SEC) * 1000;
+ bdrv_graph_rdlock_main_loop();
ret = bdrv_snapshot_create(bs, &sn);
+ bdrv_graph_rdunlock_main_loop();
+
if (ret) {
error_report("Could not create snapshot '%s': %s",
snapshot_name, strerror(-ret));
@@ -3486,6 +3489,7 @@ static int img_snapshot(int argc, char **argv)
break;
case SNAPSHOT_DELETE:
+ bdrv_graph_rdlock_main_loop();
ret = bdrv_snapshot_find(bs, &sn, snapshot_name);
if (ret < 0) {
error_report("Could not delete snapshot '%s': snapshot not "
@@ -3499,6 +3503,7 @@ static int img_snapshot(int argc, char **argv)
ret = 1;
}
}
+ bdrv_graph_rdunlock_main_loop();
break;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 08/22] block: Take graph rdlock in parts of reopen
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (6 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 07/22] block: Mark bdrv_snapshot_fallback() " Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 09/22] block: Mark bdrv_get_xdbg_block_graph() and callers GRAPH_RDLOCK Kevin Wolf
` (14 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
Reopen isn't easy with respect to locking because many of its functions
need to iterate the graph, some change it, and then you get some drains
in the middle where you can't hold any locks.
Therefore just documents most of the functions to be unlocked, and take
locks internally before accessing the graph.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block_int-common.h | 13 +++++---
block.c | 57 ++++++++++++++++++++------------
2 files changed, 43 insertions(+), 27 deletions(-)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 29c5b8a3c5..0373cbed49 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -235,11 +235,14 @@ struct BlockDriver {
Error **errp);
/* For handling image reopen for split or non-split files. */
- int (*bdrv_reopen_prepare)(BDRVReopenState *reopen_state,
- BlockReopenQueue *queue, Error **errp);
- void (*bdrv_reopen_commit)(BDRVReopenState *reopen_state);
- void (*bdrv_reopen_commit_post)(BDRVReopenState *reopen_state);
- void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
+ int GRAPH_UNLOCKED_PTR (*bdrv_reopen_prepare)(
+ BDRVReopenState *reopen_state, BlockReopenQueue *queue, Error **errp);
+ void GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit)(
+ BDRVReopenState *reopen_state);
+ void GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit_post)(
+ BDRVReopenState *reopen_state);
+ void GRAPH_UNLOCKED_PTR (*bdrv_reopen_abort)(
+ BDRVReopenState *reopen_state);
void (*bdrv_join_options)(QDict *options, QDict *old_options);
int GRAPH_UNLOCKED_PTR (*bdrv_open)(
diff --git a/block.c b/block.c
index fb494f4a8b..4c4fab3579 100644
--- a/block.c
+++ b/block.c
@@ -4313,8 +4313,8 @@ static int bdrv_reset_options_allowed(BlockDriverState *bs,
/*
* Returns true if @child can be reached recursively from @bs
*/
-static bool bdrv_recurse_has_child(BlockDriverState *bs,
- BlockDriverState *child)
+static bool GRAPH_RDLOCK
+bdrv_recurse_has_child(BlockDriverState *bs, BlockDriverState *child)
{
BdrvChild *c;
@@ -4355,15 +4355,12 @@ static bool bdrv_recurse_has_child(BlockDriverState *bs,
*
* To be called with bs->aio_context locked.
*/
-static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
- BlockDriverState *bs,
- QDict *options,
- const BdrvChildClass *klass,
- BdrvChildRole role,
- bool parent_is_format,
- QDict *parent_options,
- int parent_flags,
- bool keep_old_opts)
+static BlockReopenQueue * GRAPH_RDLOCK
+bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs,
+ QDict *options, const BdrvChildClass *klass,
+ BdrvChildRole role, bool parent_is_format,
+ QDict *parent_options, int parent_flags,
+ bool keep_old_opts)
{
assert(bs != NULL);
@@ -4375,6 +4372,11 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
GLOBAL_STATE_CODE();
+ /*
+ * Strictly speaking, draining is illegal under GRAPH_RDLOCK. We know that
+ * we've been called with bdrv_graph_rdlock_main_loop(), though, so it's ok
+ * in practice.
+ */
bdrv_drained_begin(bs);
if (bs_queue == NULL) {
@@ -4516,6 +4518,7 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
QDict *options, bool keep_old_opts)
{
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
return bdrv_reopen_queue_child(bs_queue, bs, options, NULL, 0, false,
NULL, 0, keep_old_opts);
@@ -4735,9 +4738,10 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
* Callers must make sure that their AioContext locking is still correct after
* this.
*/
-static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
- bool is_backing, Transaction *tran,
- Error **errp)
+static int GRAPH_UNLOCKED
+bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
+ bool is_backing, Transaction *tran,
+ Error **errp)
{
BlockDriverState *bs = reopen_state->bs;
BlockDriverState *new_child_bs;
@@ -4747,6 +4751,7 @@ static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
QObject *value;
const char *str;
AioContext *ctx, *old_ctx;
+ bool has_child;
int ret;
GLOBAL_STATE_CODE();
@@ -4766,7 +4771,13 @@ static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
new_child_bs = bdrv_lookup_bs(NULL, str, errp);
if (new_child_bs == NULL) {
return -EINVAL;
- } else if (bdrv_recurse_has_child(new_child_bs, bs)) {
+ }
+
+ bdrv_graph_rdlock_main_loop();
+ has_child = bdrv_recurse_has_child(new_child_bs, bs);
+ bdrv_graph_rdunlock_main_loop();
+
+ if (has_child) {
error_setg(errp, "Making '%s' a %s child of '%s' would create a "
"cycle", str, child_name, bs->node_name);
return -EINVAL;
@@ -4865,9 +4876,9 @@ static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
* After calling this function, the transaction @change_child_tran may only be
* completed while holding a writer lock for the graph.
*/
-static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
- BlockReopenQueue *queue,
- Transaction *change_child_tran, Error **errp)
+static int GRAPH_UNLOCKED
+bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
+ Transaction *change_child_tran, Error **errp)
{
int ret = -1;
int old_flags;
@@ -5009,6 +5020,8 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
if (qdict_size(reopen_state->options)) {
const QDictEntry *entry = qdict_first(reopen_state->options);
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
do {
QObject *new = entry->value;
QObject *old = qdict_get(reopen_state->bs->options, entry->key);
@@ -5082,7 +5095,7 @@ error:
* makes them final by swapping the staging BlockDriverState contents into
* the active BlockDriverState contents.
*/
-static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
+static void GRAPH_UNLOCKED bdrv_reopen_commit(BDRVReopenState *reopen_state)
{
BlockDriver *drv;
BlockDriverState *bs;
@@ -5099,6 +5112,8 @@ static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
drv->bdrv_reopen_commit(reopen_state);
}
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
/* set BDS specific flags now */
qobject_unref(bs->explicit_options);
qobject_unref(bs->options);
@@ -5120,9 +5135,7 @@ static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
qdict_del(bs->explicit_options, "backing");
qdict_del(bs->options, "backing");
- bdrv_graph_rdlock_main_loop();
bdrv_refresh_limits(bs, NULL, NULL);
- bdrv_graph_rdunlock_main_loop();
bdrv_refresh_total_sectors(bs, bs->total_sectors);
}
@@ -5130,7 +5143,7 @@ static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
* Abort the reopen, and delete and free the staged changes in
* reopen_state
*/
-static void bdrv_reopen_abort(BDRVReopenState *reopen_state)
+static void GRAPH_UNLOCKED bdrv_reopen_abort(BDRVReopenState *reopen_state)
{
BlockDriver *drv;
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 09/22] block: Mark bdrv_get_xdbg_block_graph() and callers GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (7 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 08/22] block: Take graph rdlock in parts of reopen Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 10/22] block: Mark bdrv_refresh_filename() " Kevin Wolf
` (13 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_get_xdbg_block_graph() need to hold a reader lock for the graph
because it accesses the children list of a node.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-global-state.h | 2 +-
blockdev.c | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 505a0d0c11..4d80b3d554 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -192,7 +192,7 @@ int bdrv_has_zero_init_1(BlockDriverState *bs);
int bdrv_has_zero_init(BlockDriverState *bs);
BlockDriverState *bdrv_find_node(const char *node_name);
BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, Error **errp);
-XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp);
+XDbgBlockGraph * GRAPH_RDLOCK bdrv_get_xdbg_block_graph(Error **errp);
BlockDriverState *bdrv_lookup_bs(const char *device,
const char *node_name,
Error **errp);
diff --git a/blockdev.c b/blockdev.c
index 43e0c4ddf2..ff5ee9ff7b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2846,6 +2846,8 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(bool has_flat,
XDbgBlockGraph *qmp_x_debug_query_block_graph(Error **errp)
{
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
return bdrv_get_xdbg_block_graph(errp);
}
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 10/22] block: Mark bdrv_refresh_filename() and callers GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (8 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 09/22] block: Mark bdrv_get_xdbg_block_graph() and callers GRAPH_RDLOCK Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 11/22] block: Mark bdrv_primary_child() " Kevin Wolf
` (12 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_refresh_filename() need to hold a reader lock for the graph
because it accesses the children list of a node.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/vhdx.h | 5 +--
include/block/block-global-state.h | 9 ++++--
include/block/block_int-common.h | 8 ++---
include/block/qapi.h | 16 ++++------
block.c | 23 +++++++++++---
block/nfs.c | 2 +-
block/qapi.c | 11 ++++---
block/raw-format.c | 2 ++
block/vhdx.c | 4 +++
block/vmdk.c | 51 +++++++++++++++++++-----------
blockdev.c | 13 ++++++++
qemu-img.c | 4 +++
12 files changed, 101 insertions(+), 47 deletions(-)
diff --git a/block/vhdx.h b/block/vhdx.h
index 455a627a46..85594a5380 100644
--- a/block/vhdx.h
+++ b/block/vhdx.h
@@ -410,8 +410,9 @@ uint32_t vhdx_checksum_calc(uint32_t crc, uint8_t *buf, size_t size,
bool vhdx_checksum_is_valid(uint8_t *buf, size_t size, int crc_offset);
-int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
- Error **errp);
+int GRAPH_RDLOCK
+vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
+ Error **errp);
int coroutine_fn GRAPH_RDLOCK
vhdx_log_write_and_flush(BlockDriverState *bs, BDRVVHDXState *s,
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 4d80b3d554..ec623ef3c9 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -132,7 +132,7 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
Error **errp);
BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
const char *backing_file);
-void bdrv_refresh_filename(BlockDriverState *bs);
+void GRAPH_RDLOCK bdrv_refresh_filename(BlockDriverState *bs);
void GRAPH_RDLOCK
bdrv_refresh_limits(BlockDriverState *bs, Transaction *tran, Error **errp);
@@ -216,8 +216,11 @@ void bdrv_next_cleanup(BdrvNextIterator *it);
BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs);
void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
void *opaque, bool read_only);
-char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp);
-char *bdrv_dirname(BlockDriverState *bs, Error **errp);
+
+char * GRAPH_RDLOCK
+bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp);
+
+char * GRAPH_RDLOCK bdrv_dirname(BlockDriverState *bs, Error **errp);
void bdrv_img_create(const char *filename, const char *fmt,
const char *base_filename, const char *base_fmt,
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 0373cbed49..be80887898 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -272,7 +272,7 @@ struct BlockDriver {
* Refreshes the bs->exact_filename field. If that is impossible,
* bs->exact_filename has to be left empty.
*/
- void (*bdrv_refresh_filename)(BlockDriverState *bs);
+ void GRAPH_RDLOCK_PTR (*bdrv_refresh_filename)(BlockDriverState *bs);
/*
* Gathers the open options for all children into @target.
@@ -295,15 +295,15 @@ struct BlockDriver {
* block driver which implements it is probably doing something
* shady regarding its runtime option structure.
*/
- void (*bdrv_gather_child_options)(BlockDriverState *bs, QDict *target,
- bool backing_overridden);
+ void GRAPH_RDLOCK_PTR (*bdrv_gather_child_options)(
+ BlockDriverState *bs, QDict *target, bool backing_overridden);
/*
* Returns an allocated string which is the directory name of this BDS: It
* will be used to make relative filenames absolute by prepending this
* function's return value to them.
*/
- char *(*bdrv_dirname)(BlockDriverState *bs, Error **errp);
+ char * GRAPH_RDLOCK_PTR (*bdrv_dirname)(BlockDriverState *bs, Error **errp);
/*
* This informs the driver that we are no longer interested in the result
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 8663971c58..887235653a 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -29,18 +29,16 @@
#include "block/snapshot.h"
#include "qapi/qapi-types-block-core.h"
-BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
- BlockDriverState *bs,
- bool flat,
- Error **errp);
+BlockDeviceInfo * GRAPH_RDLOCK
+bdrv_block_device_info(BlockBackend *blk, BlockDriverState *bs,
+ bool flat, Error **errp);
+
int bdrv_query_snapshot_info_list(BlockDriverState *bs,
SnapshotInfoList **p_list,
Error **errp);
-void bdrv_query_image_info(BlockDriverState *bs,
- ImageInfo **p_info,
- bool flat,
- bool skip_implicit_filters,
- Error **errp);
+void GRAPH_RDLOCK
+bdrv_query_image_info(BlockDriverState *bs, ImageInfo **p_info, bool flat,
+ bool skip_implicit_filters, Error **errp);
void GRAPH_RDLOCK
bdrv_query_block_graph_info(BlockDriverState *bs, BlockGraphInfo **p_info,
Error **errp);
diff --git a/block.c b/block.c
index 4c4fab3579..7e6ba91885 100644
--- a/block.c
+++ b/block.c
@@ -371,8 +371,9 @@ char *bdrv_get_full_backing_filename_from_filename(const char *backed,
* setting @errp. In all other cases, NULL will only be returned with
* @errp set.
*/
-static char *bdrv_make_absolute_filename(BlockDriverState *relative_to,
- const char *filename, Error **errp)
+static char * GRAPH_RDLOCK
+bdrv_make_absolute_filename(BlockDriverState *relative_to,
+ const char *filename, Error **errp)
{
char *dir, *full_name;
@@ -1250,7 +1251,7 @@ static void bdrv_temp_snapshot_options(int *child_flags, QDict *child_options,
*child_flags &= ~BDRV_O_NATIVE_AIO;
}
-static void bdrv_backing_attach(BdrvChild *c)
+static void GRAPH_WRLOCK bdrv_backing_attach(BdrvChild *c)
{
BlockDriverState *parent = c->opaque;
BlockDriverState *backing_hd = c->bs;
@@ -1874,7 +1875,10 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
}
if (file != NULL) {
+ bdrv_graph_rdlock_main_loop();
bdrv_refresh_filename(blk_bs(file));
+ bdrv_graph_rdunlock_main_loop();
+
filename = blk_bs(file)->filename;
} else {
/*
@@ -3643,7 +3647,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
implicit_backing = !strcmp(bs->auto_backing_file, bs->backing_file);
}
+ bdrv_graph_rdlock_main_loop();
backing_filename = bdrv_get_full_backing_filename(bs, &local_err);
+ bdrv_graph_rdunlock_main_loop();
+
if (local_err) {
ret = -EINVAL;
error_propagate(errp, local_err);
@@ -3674,7 +3681,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
}
if (implicit_backing) {
+ bdrv_graph_rdlock_main_loop();
bdrv_refresh_filename(backing_hd);
+ bdrv_graph_rdunlock_main_loop();
pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file),
backing_hd->filename);
}
@@ -4963,7 +4972,9 @@ bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
if (local_err != NULL) {
error_propagate(errp, local_err);
} else {
+ bdrv_graph_rdlock_main_loop();
bdrv_refresh_filename(reopen_state->bs);
+ bdrv_graph_rdunlock_main_loop();
error_setg(errp, "failed while preparing to reopen image '%s'",
reopen_state->bs->filename);
}
@@ -5930,6 +5941,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
bdrv_ref(top);
bdrv_drained_begin(base);
+ bdrv_graph_rdlock_main_loop();
if (!top->drv || !base->drv) {
goto exit;
@@ -5954,11 +5966,9 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
backing_file_str = base->filename;
}
- bdrv_graph_rdlock_main_loop();
QLIST_FOREACH(c, &top->parents, next_parent) {
updated_children = g_slist_prepend(updated_children, c);
}
- bdrv_graph_rdunlock_main_loop();
/*
* It seems correct to pass detach_subchain=true here, but it triggers
@@ -6004,6 +6014,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
ret = 0;
exit:
+ bdrv_graph_rdunlock_main_loop();
bdrv_drained_end(base);
bdrv_unref(top);
return ret;
@@ -6294,6 +6305,7 @@ BlockDeviceInfoList *bdrv_named_nodes_list(bool flat,
BlockDriverState *bs;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
list = NULL;
QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
@@ -6762,6 +6774,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
BlockDriverState *bs_below;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
if (!bs || !bs->drv || !backing_file) {
return NULL;
diff --git a/block/nfs.c b/block/nfs.c
index c24df49747..f737e19cd3 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -843,7 +843,7 @@ static void nfs_refresh_filename(BlockDriverState *bs)
}
}
-static char *nfs_dirname(BlockDriverState *bs, Error **errp)
+static char * GRAPH_RDLOCK nfs_dirname(BlockDriverState *bs, Error **errp)
{
NFSClient *client = bs->opaque;
diff --git a/block/qapi.c b/block/qapi.c
index 1cbb0935ff..82a30b38fe 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -225,9 +225,8 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
* Helper function for other query info functions. Store information about @bs
* in @info, setting @errp on error.
*/
-static void bdrv_do_query_node_info(BlockDriverState *bs,
- BlockNodeInfo *info,
- Error **errp)
+static void GRAPH_RDLOCK
+bdrv_do_query_node_info(BlockDriverState *bs, BlockNodeInfo *info, Error **errp)
{
int64_t size;
const char *backing_filename;
@@ -423,8 +422,8 @@ fail:
}
/* @p_info will be set only on success. */
-static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
- Error **errp)
+static void GRAPH_RDLOCK
+bdrv_query_info(BlockBackend *blk, BlockInfo **p_info, Error **errp)
{
BlockInfo *info = g_malloc0(sizeof(*info));
BlockDriverState *bs = blk_bs(blk);
@@ -672,6 +671,8 @@ BlockInfoList *qmp_query_block(Error **errp)
BlockBackend *blk;
Error *local_err = NULL;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) {
BlockInfoList *info;
diff --git a/block/raw-format.c b/block/raw-format.c
index a8bdee5279..8ff03adfa4 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -505,7 +505,9 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
BDRV_REQ_ZERO_WRITE;
if (bs->probed && !bdrv_is_read_only(bs)) {
+ bdrv_graph_rdlock_main_loop();
bdrv_refresh_filename(bs->file->bs);
+ bdrv_graph_rdunlock_main_loop();
fprintf(stderr,
"WARNING: Image format was not specified for '%s' and probing "
"guessed raw.\n"
diff --git a/block/vhdx.c b/block/vhdx.c
index a67edcc03e..73cb214fb4 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1001,11 +1001,15 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
uint64_t signature;
Error *local_err = NULL;
+ GLOBAL_STATE_CODE();
+
ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
if (ret < 0) {
return ret;
}
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
s->bat = NULL;
s->first_visible_write = true;
diff --git a/block/vmdk.c b/block/vmdk.c
index e90649c8bf..e50965bc37 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -578,8 +578,8 @@ static int vmdk_add_extent(BlockDriverState *bs,
return 0;
}
-static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
- Error **errp)
+static int GRAPH_RDLOCK
+vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent, Error **errp)
{
int ret;
size_t l1_size;
@@ -641,9 +641,9 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
return ret;
}
-static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
- BdrvChild *file,
- int flags, Error **errp)
+static int GRAPH_RDLOCK
+vmdk_open_vmfs_sparse(BlockDriverState *bs, BdrvChild *file, int flags,
+ Error **errp)
{
int ret;
uint32_t magic;
@@ -797,9 +797,9 @@ static int check_se_sparse_volatile_header(VMDKSESparseVolatileHeader *header,
return 0;
}
-static int vmdk_open_se_sparse(BlockDriverState *bs,
- BdrvChild *file,
- int flags, Error **errp)
+static int GRAPH_RDLOCK
+vmdk_open_se_sparse(BlockDriverState *bs, BdrvChild *file, int flags,
+ Error **errp)
{
int ret;
VMDKSESparseConstHeader const_header;
@@ -913,9 +913,9 @@ static char *vmdk_read_desc(BdrvChild *file, uint64_t desc_offset, Error **errp)
return buf;
}
-static int vmdk_open_vmdk4(BlockDriverState *bs,
- BdrvChild *file,
- int flags, QDict *options, Error **errp)
+static int GRAPH_RDLOCK
+vmdk_open_vmdk4(BlockDriverState *bs, BdrvChild *file, int flags,
+ QDict *options, Error **errp)
{
int ret;
uint32_t magic;
@@ -1095,8 +1095,9 @@ static int vmdk_parse_description(const char *desc, const char *opt_name,
}
/* Open an extent file and append to bs array */
-static int vmdk_open_sparse(BlockDriverState *bs, BdrvChild *file, int flags,
- char *buf, QDict *options, Error **errp)
+static int GRAPH_RDLOCK
+vmdk_open_sparse(BlockDriverState *bs, BdrvChild *file, int flags,
+ char *buf, QDict *options, Error **errp)
{
uint32_t magic;
@@ -1123,8 +1124,9 @@ static const char *next_line(const char *s)
return s;
}
-static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
- QDict *options, Error **errp)
+static int GRAPH_RDLOCK
+vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
+ Error **errp)
{
int ret;
int matches;
@@ -1143,6 +1145,8 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
char extent_opt_prefix[32];
Error *local_err = NULL;
+ GLOBAL_STATE_CODE();
+
for (p = desc; *p; p = next_line(p)) {
/* parse extent line in one of below formats:
*
@@ -1223,9 +1227,11 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
ret = vmdk_add_extent(bs, extent_file, true, sectors,
0, 0, 0, 0, 0, &extent, errp);
if (ret < 0) {
+ bdrv_graph_rdunlock_main_loop();
bdrv_graph_wrlock(NULL);
bdrv_unref_child(bs, extent_file);
bdrv_graph_wrunlock();
+ bdrv_graph_rdlock_main_loop();
goto out;
}
extent->flat_start_offset = flat_offset << 9;
@@ -1240,26 +1246,32 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
}
g_free(buf);
if (ret) {
+ bdrv_graph_rdunlock_main_loop();
bdrv_graph_wrlock(NULL);
bdrv_unref_child(bs, extent_file);
bdrv_graph_wrunlock();
+ bdrv_graph_rdlock_main_loop();
goto out;
}
extent = &s->extents[s->num_extents - 1];
} else if (!strcmp(type, "SESPARSE")) {
ret = vmdk_open_se_sparse(bs, extent_file, bs->open_flags, errp);
if (ret) {
+ bdrv_graph_rdunlock_main_loop();
bdrv_graph_wrlock(NULL);
bdrv_unref_child(bs, extent_file);
bdrv_graph_wrunlock();
+ bdrv_graph_rdlock_main_loop();
goto out;
}
extent = &s->extents[s->num_extents - 1];
} else {
error_setg(errp, "Unsupported extent type '%s'", type);
+ bdrv_graph_rdunlock_main_loop();
bdrv_graph_wrlock(NULL);
bdrv_unref_child(bs, extent_file);
bdrv_graph_wrunlock();
+ bdrv_graph_rdlock_main_loop();
ret = -ENOTSUP;
goto out;
}
@@ -1283,8 +1295,9 @@ out:
return ret;
}
-static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
- QDict *options, Error **errp)
+static int GRAPH_RDLOCK
+vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf, QDict *options,
+ Error **errp)
{
int ret;
char ct[128];
@@ -2900,7 +2913,7 @@ static int vmdk_has_zero_init(BlockDriverState *bs)
return 1;
}
-static VmdkExtentInfo *vmdk_get_extent_info(VmdkExtent *extent)
+static VmdkExtentInfo * GRAPH_RDLOCK vmdk_get_extent_info(VmdkExtent *extent)
{
VmdkExtentInfo *info = g_new0(VmdkExtentInfo, 1);
@@ -2985,6 +2998,8 @@ static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs,
ImageInfoSpecific *spec_info = g_new0(ImageInfoSpecific, 1);
VmdkExtentInfoList **tail;
+ assume_graph_lock(); /* FIXME */
+
*spec_info = (ImageInfoSpecific){
.type = IMAGE_INFO_SPECIFIC_KIND_VMDK,
.u = {
diff --git a/blockdev.c b/blockdev.c
index ff5ee9ff7b..51c58dd432 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1665,6 +1665,8 @@ static void drive_backup_action(DriveBackup *backup,
bool set_backing_hd = false;
int ret;
+ GLOBAL_STATE_CODE();
+
tran_add(tran, &drive_backup_drv, state);
if (!backup->has_mode) {
@@ -1735,7 +1737,10 @@ static void drive_backup_action(DriveBackup *backup,
BlockDriverState *explicit_backing =
bdrv_skip_implicit_filters(source);
+ bdrv_graph_rdlock_main_loop();
bdrv_refresh_filename(explicit_backing);
+ bdrv_graph_rdunlock_main_loop();
+
bdrv_img_create(backup->target, format,
explicit_backing->filename,
explicit_backing->drv->format_name, NULL,
@@ -2398,6 +2403,8 @@ void qmp_block_stream(const char *job_id, const char *device,
Error *local_err = NULL;
int job_flags = JOB_DEFAULT;
+ GLOBAL_STATE_CODE();
+
if (base && base_node) {
error_setg(errp, "'base' and 'base-node' cannot be specified "
"at the same time");
@@ -2448,7 +2455,10 @@ void qmp_block_stream(const char *job_id, const char *device,
goto out;
}
assert(bdrv_get_aio_context(base_bs) == aio_context);
+
+ bdrv_graph_rdlock_main_loop();
bdrv_refresh_filename(base_bs);
+ bdrv_graph_rdunlock_main_loop();
}
if (bottom) {
@@ -3076,7 +3086,10 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
break;
case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
/* create new image with backing file */
+ bdrv_graph_rdlock_main_loop();
bdrv_refresh_filename(explicit_backing);
+ bdrv_graph_rdunlock_main_loop();
+
bdrv_img_create(arg->target, format,
explicit_backing->filename,
explicit_backing->drv->format_name,
diff --git a/qemu-img.c b/qemu-img.c
index 9e293bdf78..f7fa49fe65 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3165,7 +3165,9 @@ static int get_block_status(BlockDriverState *bs, int64_t offset,
has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
if (file && has_offset) {
+ bdrv_graph_rdlock_main_loop();
bdrv_refresh_filename(file);
+ bdrv_graph_rdunlock_main_loop();
filename = file->filename;
}
@@ -3688,7 +3690,9 @@ static int img_rebase(int argc, char **argv)
qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
}
+ bdrv_graph_rdlock_main_loop();
bdrv_refresh_filename(bs);
+ bdrv_graph_rdunlock_main_loop();
overlay_filename = bs->exact_filename[0] ? bs->exact_filename
: bs->filename;
out_real_path =
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 11/22] block: Mark bdrv_primary_child() and callers GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (9 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 10/22] block: Mark bdrv_refresh_filename() " Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 12/22] block: Mark bdrv_get_parent_name() " Kevin Wolf
` (11 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_primary_child() need to hold a reader lock for the graph
because it accesses the children list of a node.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block_int-io.h | 5 +++--
block.c | 11 ++++++++++-
block/snapshot.c | 3 +++
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h
index eb0da7232e..2b6004ab93 100644
--- a/include/block/block_int-io.h
+++ b/include/block/block_int-io.h
@@ -133,7 +133,7 @@ bdrv_refresh_total_sectors(BlockDriverState *bs, int64_t hint);
BdrvChild *bdrv_cow_child(BlockDriverState *bs);
BdrvChild *bdrv_filter_child(BlockDriverState *bs);
BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs);
-BdrvChild *bdrv_primary_child(BlockDriverState *bs);
+BdrvChild * GRAPH_RDLOCK bdrv_primary_child(BlockDriverState *bs);
BlockDriverState *bdrv_skip_filters(BlockDriverState *bs);
BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs);
@@ -155,7 +155,8 @@ static inline BlockDriverState *bdrv_filter_or_cow_bs(BlockDriverState *bs)
return child_bs(bdrv_filter_or_cow_child(bs));
}
-static inline BlockDriverState *bdrv_primary_bs(BlockDriverState *bs)
+static inline BlockDriverState * GRAPH_RDLOCK
+bdrv_primary_bs(BlockDriverState *bs)
{
IO_CODE();
return child_bs(bdrv_primary_child(bs));
diff --git a/block.c b/block.c
index 7e6ba91885..133efa47e2 100644
--- a/block.c
+++ b/block.c
@@ -6691,7 +6691,8 @@ void coroutine_fn bdrv_co_debug_event(BlockDriverState *bs, BlkdebugEvent event)
bs->drv->bdrv_co_debug_event(bs, event);
}
-static BlockDriverState *bdrv_find_debug_node(BlockDriverState *bs)
+static BlockDriverState * GRAPH_RDLOCK
+bdrv_find_debug_node(BlockDriverState *bs)
{
GLOBAL_STATE_CODE();
while (bs && bs->drv && !bs->drv->bdrv_debug_breakpoint) {
@@ -6710,6 +6711,8 @@ int bdrv_debug_breakpoint(BlockDriverState *bs, const char *event,
const char *tag)
{
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
bs = bdrv_find_debug_node(bs);
if (bs) {
return bs->drv->bdrv_debug_breakpoint(bs, event, tag);
@@ -6721,6 +6724,8 @@ int bdrv_debug_breakpoint(BlockDriverState *bs, const char *event,
int bdrv_debug_remove_breakpoint(BlockDriverState *bs, const char *tag)
{
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
bs = bdrv_find_debug_node(bs);
if (bs) {
return bs->drv->bdrv_debug_remove_breakpoint(bs, tag);
@@ -6732,6 +6737,8 @@ int bdrv_debug_remove_breakpoint(BlockDriverState *bs, const char *tag)
int bdrv_debug_resume(BlockDriverState *bs, const char *tag)
{
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
while (bs && (!bs->drv || !bs->drv->bdrv_debug_resume)) {
bs = bdrv_primary_bs(bs);
}
@@ -6746,6 +6753,8 @@ int bdrv_debug_resume(BlockDriverState *bs, const char *tag)
bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)
{
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
while (bs && bs->drv && !bs->drv->bdrv_debug_is_suspended) {
bs = bdrv_primary_bs(bs);
}
diff --git a/block/snapshot.c b/block/snapshot.c
index 554065578b..ad2bf6e068 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -311,7 +311,10 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
* respective option (with the qdict_put_str() call above).
* Assert that .bdrv_open() has attached the right BDS as primary child.
*/
+ bdrv_graph_rdlock_main_loop();
assert(bdrv_primary_bs(bs) == fallback_bs);
+ bdrv_graph_rdunlock_main_loop();
+
bdrv_unref(fallback_bs);
return ret;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 12/22] block: Mark bdrv_get_parent_name() and callers GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (10 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 11/22] block: Mark bdrv_primary_child() " Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 13/22] block: Mark bdrv_amend_options() " Kevin Wolf
` (10 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_get_parent_name() need to hold a reader lock for the graph
because it accesses the parents list of a node.
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: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.h | 7 +++----
include/block/block-io.h | 8 ++++++--
include/block/block_int-io.h | 2 +-
include/block/qapi.h | 7 ++++---
block.c | 14 ++++++++++++--
block/backup.c | 1 +
block/parallels.c | 3 +++
block/qcow.c | 3 +++
block/qcow2-bitmap.c | 2 ++
block/qcow2.c | 4 ++++
block/quorum.c | 4 ++--
block/rbd.c | 2 ++
block/snapshot.c | 3 +++
block/vdi.c | 3 +++
block/vpc.c | 3 +++
block/vvfat.c | 2 ++
blockjob.c | 1 +
17 files changed, 55 insertions(+), 14 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index f789ce3ae0..359bfca4aa 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -1003,10 +1003,9 @@ int coroutine_fn qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp
bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
bool release_stored, Error **errp);
int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
-bool coroutine_fn qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- uint32_t granularity,
- Error **errp);
+bool coroutine_fn GRAPH_RDLOCK
+qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+ uint32_t granularity, Error **errp);
int coroutine_fn qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
const char *name,
Error **errp);
diff --git a/include/block/block-io.h b/include/block/block-io.h
index cf72e39717..5b1a1d5998 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -176,8 +176,12 @@ const char *bdrv_get_format_name(BlockDriverState *bs);
bool bdrv_supports_compressed_writes(BlockDriverState *bs);
const char *bdrv_get_node_name(const BlockDriverState *bs);
-const char *bdrv_get_device_name(const BlockDriverState *bs);
-const char *bdrv_get_device_or_node_name(const BlockDriverState *bs);
+
+const char * GRAPH_RDLOCK
+bdrv_get_device_name(const BlockDriverState *bs);
+
+const char * GRAPH_RDLOCK
+bdrv_get_device_or_node_name(const BlockDriverState *bs);
int coroutine_fn GRAPH_RDLOCK
bdrv_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h
index 2b6004ab93..34eac72d7a 100644
--- a/include/block/block_int-io.h
+++ b/include/block/block_int-io.h
@@ -99,7 +99,7 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
*/
void bdrv_wakeup(BlockDriverState *bs);
-const char *bdrv_get_parent_name(const BlockDriverState *bs);
+const char * GRAPH_RDLOCK bdrv_get_parent_name(const BlockDriverState *bs);
bool blk_dev_has_tray(BlockBackend *blk);
bool blk_dev_is_tray_open(BlockBackend *blk);
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 887235653a..54c48de26a 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -33,9 +33,10 @@ BlockDeviceInfo * GRAPH_RDLOCK
bdrv_block_device_info(BlockBackend *blk, BlockDriverState *bs,
bool flat, Error **errp);
-int bdrv_query_snapshot_info_list(BlockDriverState *bs,
- SnapshotInfoList **p_list,
- Error **errp);
+int GRAPH_RDLOCK
+bdrv_query_snapshot_info_list(BlockDriverState *bs,
+ SnapshotInfoList **p_list,
+ Error **errp);
void GRAPH_RDLOCK
bdrv_query_image_info(BlockDriverState *bs, ImageInfo **p_info, bool flat,
bool skip_implicit_filters, Error **errp);
diff --git a/block.c b/block.c
index 133efa47e2..9f6cd5bb8e 100644
--- a/block.c
+++ b/block.c
@@ -279,8 +279,9 @@ bool bdrv_is_read_only(BlockDriverState *bs)
return !(bs->open_flags & BDRV_O_RDWR);
}
-static int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
- bool ignore_allow_rdw, Error **errp)
+static int GRAPH_RDLOCK
+bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
+ bool ignore_allow_rdw, Error **errp)
{
IO_CODE();
@@ -319,6 +320,8 @@ int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
int ret = 0;
IO_CODE();
+ assume_graph_lock(); /* FIXME */
+
if (!(bs->open_flags & BDRV_O_RDWR)) {
return 0;
}
@@ -4949,7 +4952,10 @@ bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
* to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
* not set, or if the BDS still has copy_on_read enabled */
read_only = !(reopen_state->flags & BDRV_O_RDWR);
+
+ bdrv_graph_rdlock_main_loop();
ret = bdrv_can_set_read_only(reopen_state->bs, read_only, true, &local_err);
+ bdrv_graph_rdunlock_main_loop();
if (local_err) {
error_propagate(errp, local_err);
goto error;
@@ -4983,9 +4989,11 @@ bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
} else {
/* It is currently mandatory to have a bdrv_reopen_prepare()
* handler for each supported drv. */
+ bdrv_graph_rdlock_main_loop();
error_setg(errp, "Block format '%s' used by node '%s' "
"does not support reopening files", drv->format_name,
bdrv_get_device_or_node_name(reopen_state->bs));
+ bdrv_graph_rdunlock_main_loop();
ret = -1;
goto error;
}
@@ -7241,6 +7249,8 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
{
BdrvOpBlocker *blocker;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
if (!QLIST_EMPTY(&bs->op_blockers[op])) {
blocker = QLIST_FIRST(&bs->op_blockers[op]);
diff --git a/block/backup.c b/block/backup.c
index db3791f4d1..9a3c4bdc82 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -374,6 +374,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
assert(bs);
assert(target);
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
/* QMP interface protects us from these cases */
assert(sync_mode != MIRROR_SYNC_MODE_INCREMENTAL);
diff --git a/block/parallels.c b/block/parallels.c
index 48c32d6821..39d332ed49 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1170,9 +1170,12 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
bitmap_new(DIV_ROUND_UP(s->header_size, s->bat_dirty_block));
/* Disable migration until bdrv_activate method is added */
+ bdrv_graph_rdlock_main_loop();
error_setg(&s->migration_blocker, "The Parallels format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
+ bdrv_graph_rdunlock_main_loop();
+
ret = migrate_add_blocker(s->migration_blocker, errp);
if (ret < 0) {
error_setg(errp, "Migration blocker error");
diff --git a/block/qcow.c b/block/qcow.c
index d56d24ab6d..38a16253b8 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -301,9 +301,12 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
}
/* Disable migration when qcow images are used */
+ bdrv_graph_rdlock_main_loop();
error_setg(&s->migration_blocker, "The qcow format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
+ bdrv_graph_rdunlock_main_loop();
+
ret = migrate_add_blocker(s->migration_blocker, errp);
if (ret < 0) {
error_free(s->migration_blocker);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 037fa2d435..4e20a28cc7 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -166,6 +166,8 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
int64_t len = bdrv_getlength(bs);
int64_t bitmap_bytes;
+ assume_graph_lock(); /* FIXME */
+
assert(granularity > 0);
assert((granularity & (granularity - 1)) == 0);
diff --git a/block/qcow2.c b/block/qcow2.c
index af43d59d76..db5fe86c5a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2737,6 +2737,8 @@ static int qcow2_inactivate(BlockDriverState *bs)
int ret, result = 0;
Error *local_err = NULL;
+ assume_graph_lock(); /* FIXME */
+
qcow2_store_persistent_dirty_bitmaps(bs, true, &local_err);
if (local_err != NULL) {
result = -EINVAL;
@@ -5944,6 +5946,8 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
char *message;
va_list ap;
+ assume_graph_lock(); /* FIXME */
+
fatal = fatal && bdrv_is_writable(bs);
if (s->signaled_corruption &&
diff --git a/block/quorum.c b/block/quorum.c
index 05220cab7f..d3ffc2ee33 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -206,7 +206,7 @@ static void quorum_report_bad(QuorumOpType type, uint64_t offset,
end_sector - start_sector);
}
-static void quorum_report_failure(QuorumAIOCB *acb)
+static void GRAPH_RDLOCK quorum_report_failure(QuorumAIOCB *acb)
{
const char *reference = bdrv_get_device_or_node_name(acb->bs);
int64_t start_sector = acb->offset / BDRV_SECTOR_SIZE;
@@ -219,7 +219,7 @@ static void quorum_report_failure(QuorumAIOCB *acb)
static int quorum_vote_error(QuorumAIOCB *acb);
-static bool quorum_has_too_much_io_failed(QuorumAIOCB *acb)
+static bool GRAPH_RDLOCK quorum_has_too_much_io_failed(QuorumAIOCB *acb)
{
BDRVQuorumState *s = acb->bs->opaque;
diff --git a/block/rbd.c b/block/rbd.c
index 978671411e..f2defad9da 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1208,6 +1208,8 @@ static int qemu_rbd_reopen_prepare(BDRVReopenState *state,
BDRVRBDState *s = state->bs->opaque;
int ret = 0;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (s->snap && state->flags & BDRV_O_RDWR) {
error_setg(errp,
"Cannot change node '%s' to r/w when using RBD snapshot",
diff --git a/block/snapshot.c b/block/snapshot.c
index ad2bf6e068..6e16eb803a 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -432,6 +432,7 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
BlockDriver *drv = bs->drv;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
if (!drv) {
error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
@@ -641,8 +642,10 @@ int bdrv_all_goto_snapshot(const char *name,
}
aio_context_release(ctx);
if (ret < 0) {
+ bdrv_graph_rdlock_main_loop();
error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
name, bdrv_get_device_or_node_name(bs));
+ bdrv_graph_rdunlock_main_loop();
return -1;
}
diff --git a/block/vdi.c b/block/vdi.c
index 6c35309e04..85b4382612 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -492,9 +492,12 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
}
/* Disable migration when vdi images are used */
+ bdrv_graph_rdlock_main_loop();
error_setg(&s->migration_blocker, "The vdi format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
+ bdrv_graph_rdunlock_main_loop();
+
ret = migrate_add_blocker(s->migration_blocker, errp);
if (ret < 0) {
error_free(s->migration_blocker);
diff --git a/block/vpc.c b/block/vpc.c
index ceb87dd3d8..945847fe4a 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -446,9 +446,12 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
}
/* Disable migration when VHD images are used */
+ bdrv_graph_rdlock_main_loop();
error_setg(&s->migration_blocker, "The vpc format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
+ bdrv_graph_rdunlock_main_loop();
+
ret = migrate_add_blocker(s->migration_blocker, errp);
if (ret < 0) {
error_free(s->migration_blocker);
diff --git a/block/vvfat.c b/block/vvfat.c
index 0ddc91fc09..7d770c5672 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1145,6 +1145,8 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
QemuOpts *opts;
int ret;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
#ifdef DEBUG
vvv = s;
#endif
diff --git a/blockjob.c b/blockjob.c
index 58c5d64539..807f992b59 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -485,6 +485,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
BlockJob *job;
int ret;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
if (job_id == NULL && !(flags & JOB_INTERNAL)) {
job_id = bdrv_get_device_name(bs);
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 13/22] block: Mark bdrv_amend_options() and callers GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (11 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 12/22] block: Mark bdrv_get_parent_name() " Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 14/22] qcow2: Mark qcow2_signal_corruption() " Kevin Wolf
` (9 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_amend_options() need to hold a reader lock for the graph. This
removes an assume_graph_lock() call in crypto's implementation.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-global-state.h | 8 ++++----
include/block/block_int-common.h | 10 ++++------
block/crypto.c | 4 +---
qemu-img.c | 7 +++++++
4 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index ec623ef3c9..9dc35133a8 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -160,10 +160,10 @@ void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
*/
typedef void BlockDriverAmendStatusCB(BlockDriverState *bs, int64_t offset,
int64_t total_work_size, void *opaque);
-int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
- BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
- bool force,
- Error **errp);
+int GRAPH_RDLOCK
+bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
+ BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
+ bool force, Error **errp);
/* check if a named node can be replaced when doing drive-mirror */
BlockDriverState * GRAPH_RDLOCK
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index be80887898..c8661775f4 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -259,12 +259,10 @@ struct BlockDriver {
int coroutine_fn GRAPH_UNLOCKED_PTR (*bdrv_co_create_opts)(
BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp);
- int (*bdrv_amend_options)(BlockDriverState *bs,
- QemuOpts *opts,
- BlockDriverAmendStatusCB *status_cb,
- void *cb_opaque,
- bool force,
- Error **errp);
+ int GRAPH_RDLOCK_PTR (*bdrv_amend_options)(
+ BlockDriverState *bs, QemuOpts *opts,
+ BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
+ bool force, Error **errp);
int (*bdrv_make_empty)(BlockDriverState *bs);
diff --git a/block/crypto.c b/block/crypto.c
index c9c9a39fa3..b3f0233d53 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -828,7 +828,7 @@ block_crypto_amend_options_generic_luks(BlockDriverState *bs,
errp);
}
-static int
+static int GRAPH_RDLOCK
block_crypto_amend_options_luks(BlockDriverState *bs,
QemuOpts *opts,
BlockDriverAmendStatusCB *status_cb,
@@ -841,8 +841,6 @@ block_crypto_amend_options_luks(BlockDriverState *bs,
QCryptoBlockAmendOptions *amend_options = NULL;
int ret = -EINVAL;
- assume_graph_lock(); /* FIXME */
-
assert(crypto);
assert(crypto->block);
diff --git a/qemu-img.c b/qemu-img.c
index f7fa49fe65..6d8cab69a5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4129,6 +4129,8 @@ static int print_amend_option_help(const char *format)
{
BlockDriver *drv;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
/* Find driver and parse its options */
drv = bdrv_find_format(format);
if (!drv) {
@@ -4267,9 +4269,11 @@ static int img_amend(int argc, char **argv)
goto out;
}
+ bdrv_graph_rdlock_main_loop();
if (!bs->drv->bdrv_amend_options) {
error_report("Format driver '%s' does not support option amendment",
fmt);
+ bdrv_graph_rdunlock_main_loop();
ret = -1;
goto out;
}
@@ -4289,6 +4293,7 @@ static int img_amend(int argc, char **argv)
"This option is only supported for image creation\n");
}
+ bdrv_graph_rdunlock_main_loop();
error_report_err(err);
ret = -1;
goto out;
@@ -4298,6 +4303,8 @@ static int img_amend(int argc, char **argv)
qemu_progress_print(0.f, 0);
ret = bdrv_amend_options(bs, opts, &amend_status_cb, NULL, force, &err);
qemu_progress_print(100.f, 0);
+ bdrv_graph_rdunlock_main_loop();
+
if (ret < 0) {
error_report_err(err);
goto out;
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 14/22] qcow2: Mark qcow2_signal_corruption() and callers GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (12 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 13/22] block: Mark bdrv_amend_options() " Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 15/22] qcow2: Mark qcow2_inactivate() " Kevin Wolf
` (8 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
This adds GRAPH_RDLOCK annotations to declare that callers of
qcow2_signal_corruption() need to hold a reader lock for the graph
because it calls bdrv_get_node_name(), which accesses the parents list
of a node.
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: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.h | 180 +++++++++++++++++------------
include/block/block-global-state.h | 2 +-
include/block/block_int-common.h | 2 +-
block.c | 3 +-
block/block-backend.c | 2 +
block/qcow2-bitmap.c | 31 ++---
block/qcow2-cache.c | 11 +-
block/qcow2-cluster.c | 62 +++++-----
block/qcow2-refcount.c | 80 ++++++-------
block/qcow2.c | 61 +++++-----
block/replication.c | 8 +-
11 files changed, 250 insertions(+), 192 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 359bfca4aa..29958c512b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -838,9 +838,10 @@ int qcow2_mark_dirty(BlockDriverState *bs);
int qcow2_mark_corrupt(BlockDriverState *bs);
int qcow2_update_header(BlockDriverState *bs);
-void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
- int64_t size, const char *message_format, ...)
- G_GNUC_PRINTF(5, 6);
+void GRAPH_RDLOCK
+qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
+ int64_t size, const char *message_format, ...)
+ G_GNUC_PRINTF(5, 6);
int qcow2_validate_table(BlockDriverState *bs, uint64_t offset,
uint64_t entries, size_t entry_len,
@@ -851,33 +852,41 @@ int qcow2_validate_table(BlockDriverState *bs, uint64_t offset,
int coroutine_fn GRAPH_RDLOCK qcow2_refcount_init(BlockDriverState *bs);
void qcow2_refcount_close(BlockDriverState *bs);
-int qcow2_get_refcount(BlockDriverState *bs, int64_t cluster_index,
- uint64_t *refcount);
+int GRAPH_RDLOCK qcow2_get_refcount(BlockDriverState *bs, int64_t cluster_index,
+ uint64_t *refcount);
-int qcow2_update_cluster_refcount(BlockDriverState *bs, int64_t cluster_index,
- uint64_t addend, bool decrease,
- enum qcow2_discard_type type);
+int GRAPH_RDLOCK
+qcow2_update_cluster_refcount(BlockDriverState *bs, int64_t cluster_index,
+ uint64_t addend, bool decrease,
+ enum qcow2_discard_type type);
-int64_t qcow2_refcount_area(BlockDriverState *bs, uint64_t offset,
- uint64_t additional_clusters, bool exact_size,
- int new_refblock_index,
- uint64_t new_refblock_offset);
+int64_t GRAPH_RDLOCK
+qcow2_refcount_area(BlockDriverState *bs, uint64_t offset,
+ uint64_t additional_clusters, bool exact_size,
+ int new_refblock_index,
+ uint64_t new_refblock_offset);
-int64_t qcow2_alloc_clusters(BlockDriverState *bs, uint64_t size);
-int64_t coroutine_fn qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
- int64_t nb_clusters);
-int64_t coroutine_fn GRAPH_RDLOCK qcow2_alloc_bytes(BlockDriverState *bs, int size);
-void qcow2_free_clusters(BlockDriverState *bs,
- int64_t offset, int64_t size,
- enum qcow2_discard_type type);
-void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
- enum qcow2_discard_type type);
+int64_t GRAPH_RDLOCK
+qcow2_alloc_clusters(BlockDriverState *bs, uint64_t size);
-int qcow2_update_snapshot_refcount(BlockDriverState *bs,
- int64_t l1_table_offset, int l1_size, int addend);
+int64_t GRAPH_RDLOCK coroutine_fn
+qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
+ int64_t nb_clusters);
-int qcow2_flush_caches(BlockDriverState *bs);
-int qcow2_write_caches(BlockDriverState *bs);
+int64_t coroutine_fn GRAPH_RDLOCK qcow2_alloc_bytes(BlockDriverState *bs, int size);
+void GRAPH_RDLOCK qcow2_free_clusters(BlockDriverState *bs,
+ int64_t offset, int64_t size,
+ enum qcow2_discard_type type);
+void GRAPH_RDLOCK
+qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
+ enum qcow2_discard_type type);
+
+int GRAPH_RDLOCK
+qcow2_update_snapshot_refcount(BlockDriverState *bs, int64_t l1_table_offset,
+ int l1_size, int addend);
+
+int GRAPH_RDLOCK qcow2_flush_caches(BlockDriverState *bs);
+int GRAPH_RDLOCK qcow2_write_caches(BlockDriverState *bs);
int coroutine_fn qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
BdrvCheckMode fix);
@@ -885,39 +894,48 @@ void qcow2_process_discards(BlockDriverState *bs, int ret);
int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
int64_t size);
-int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
- int64_t size, bool data_file);
+int GRAPH_RDLOCK
+qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
+ int64_t size, bool data_file);
+
int coroutine_fn qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size,
int64_t offset, int64_t size);
-int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
- BlockDriverAmendStatusCB *status_cb,
- void *cb_opaque, Error **errp);
+int GRAPH_RDLOCK
+qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
+ BlockDriverAmendStatusCB *status_cb,
+ void *cb_opaque, Error **errp);
int coroutine_fn GRAPH_RDLOCK qcow2_shrink_reftable(BlockDriverState *bs);
-int64_t coroutine_fn qcow2_get_last_cluster(BlockDriverState *bs, int64_t size);
+
+int64_t coroutine_fn GRAPH_RDLOCK
+qcow2_get_last_cluster(BlockDriverState *bs, int64_t size);
int coroutine_fn GRAPH_RDLOCK
qcow2_detect_metadata_preallocation(BlockDriverState *bs);
/* qcow2-cluster.c functions */
-int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
- bool exact_size);
+int GRAPH_RDLOCK
+qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, bool exact_size);
int coroutine_fn GRAPH_RDLOCK
qcow2_shrink_l1_table(BlockDriverState *bs, uint64_t max_size);
-int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index);
+int GRAPH_RDLOCK qcow2_write_l1_entry(BlockDriverState *bs, int l1_index);
int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num,
uint8_t *buf, int nb_sectors, bool enc, Error **errp);
-int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
- unsigned int *bytes, uint64_t *host_offset,
- QCow2SubclusterType *subcluster_type);
-int coroutine_fn qcow2_alloc_host_offset(BlockDriverState *bs, uint64_t offset,
- unsigned int *bytes,
- uint64_t *host_offset, QCowL2Meta **m);
+int GRAPH_RDLOCK
+qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
+ unsigned int *bytes, uint64_t *host_offset,
+ QCow2SubclusterType *subcluster_type);
+
+int coroutine_fn GRAPH_RDLOCK
+qcow2_alloc_host_offset(BlockDriverState *bs, uint64_t offset,
+ unsigned int *bytes, uint64_t *host_offset,
+ QCowL2Meta **m);
+
int coroutine_fn GRAPH_RDLOCK
qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs, uint64_t offset,
int compressed_size, uint64_t *host_offset);
@@ -927,26 +945,33 @@ void qcow2_parse_compressed_l2_entry(BlockDriverState *bs, uint64_t l2_entry,
int coroutine_fn GRAPH_RDLOCK
qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
-void coroutine_fn qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m);
-int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, enum qcow2_discard_type type,
- bool full_discard);
+void coroutine_fn GRAPH_RDLOCK
+qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m);
+
+int GRAPH_RDLOCK
+qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
+ enum qcow2_discard_type type, bool full_discard);
int coroutine_fn GRAPH_RDLOCK
qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
int flags);
-int qcow2_expand_zero_clusters(BlockDriverState *bs,
- BlockDriverAmendStatusCB *status_cb,
- void *cb_opaque);
+int GRAPH_RDLOCK
+qcow2_expand_zero_clusters(BlockDriverState *bs,
+ BlockDriverAmendStatusCB *status_cb,
+ void *cb_opaque);
/* qcow2-snapshot.c functions */
-int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
-int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
-int qcow2_snapshot_delete(BlockDriverState *bs,
- const char *snapshot_id,
- const char *name,
- Error **errp);
+int GRAPH_RDLOCK
+qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
+
+int GRAPH_RDLOCK
+qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
+
+int GRAPH_RDLOCK
+qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id,
+ const char *name, Error **errp);
+
int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
int qcow2_snapshot_load_tmp(BlockDriverState *bs,
const char *snapshot_id,
@@ -956,15 +981,15 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
void qcow2_free_snapshots(BlockDriverState *bs);
int coroutine_fn GRAPH_RDLOCK
qcow2_read_snapshots(BlockDriverState *bs, Error **errp);
-int qcow2_write_snapshots(BlockDriverState *bs);
+int GRAPH_RDLOCK qcow2_write_snapshots(BlockDriverState *bs);
int coroutine_fn GRAPH_RDLOCK
qcow2_check_read_snapshot_table(BlockDriverState *bs, BdrvCheckResult *result,
BdrvCheckMode fix);
-int coroutine_fn qcow2_check_fix_snapshot_table(BlockDriverState *bs,
- BdrvCheckResult *result,
- BdrvCheckMode fix);
+int coroutine_fn GRAPH_RDLOCK
+qcow2_check_fix_snapshot_table(BlockDriverState *bs, BdrvCheckResult *result,
+ BdrvCheckMode fix);
/* qcow2-cache.c functions */
Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
@@ -972,19 +997,23 @@ Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
int qcow2_cache_destroy(Qcow2Cache *c);
void qcow2_cache_entry_mark_dirty(Qcow2Cache *c, void *table);
-int qcow2_cache_flush(BlockDriverState *bs, Qcow2Cache *c);
-int qcow2_cache_write(BlockDriverState *bs, Qcow2Cache *c);
-int qcow2_cache_set_dependency(BlockDriverState *bs, Qcow2Cache *c,
- Qcow2Cache *dependency);
+int GRAPH_RDLOCK qcow2_cache_flush(BlockDriverState *bs, Qcow2Cache *c);
+int GRAPH_RDLOCK qcow2_cache_write(BlockDriverState *bs, Qcow2Cache *c);
+int GRAPH_RDLOCK qcow2_cache_set_dependency(BlockDriverState *bs, Qcow2Cache *c,
+ Qcow2Cache *dependency);
void qcow2_cache_depends_on_flush(Qcow2Cache *c);
void qcow2_cache_clean_unused(Qcow2Cache *c);
-int qcow2_cache_empty(BlockDriverState *bs, Qcow2Cache *c);
+int GRAPH_RDLOCK qcow2_cache_empty(BlockDriverState *bs, Qcow2Cache *c);
+
+int GRAPH_RDLOCK
+qcow2_cache_get(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
+ void **table);
+
+int GRAPH_RDLOCK
+qcow2_cache_get_empty(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
+ void **table);
-int qcow2_cache_get(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
- void **table);
-int qcow2_cache_get_empty(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
- void **table);
void qcow2_cache_put(Qcow2Cache *c, void **table);
void *qcow2_cache_is_table_offset(Qcow2Cache *c, uint64_t offset);
void qcow2_cache_discard(Qcow2Cache *c, void *table);
@@ -998,17 +1027,22 @@ bool coroutine_fn GRAPH_RDLOCK
qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated, Error **errp);
bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
Qcow2BitmapInfoList **info_list, Error **errp);
-int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
+int GRAPH_RDLOCK qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
+int GRAPH_RDLOCK qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
int coroutine_fn qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
-bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
- bool release_stored, Error **errp);
-int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
+
+bool GRAPH_RDLOCK
+qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, bool release_stored,
+ Error **errp);
+
bool coroutine_fn GRAPH_RDLOCK
qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
uint32_t granularity, Error **errp);
-int coroutine_fn qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- Error **errp);
+
+int coroutine_fn GRAPH_RDLOCK
+qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+ Error **errp);
+
bool qcow2_supports_persistent_dirty_bitmap(BlockDriverState *bs);
uint64_t qcow2_get_persistent_dirty_bitmap_size(BlockDriverState *bs,
uint32_t cluster_size);
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 9dc35133a8..794ef34ded 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -138,7 +138,7 @@ void GRAPH_RDLOCK
bdrv_refresh_limits(BlockDriverState *bs, Transaction *tran, Error **errp);
int bdrv_commit(BlockDriverState *bs);
-int bdrv_make_empty(BdrvChild *c, Error **errp);
+int GRAPH_RDLOCK bdrv_make_empty(BdrvChild *c, Error **errp);
int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file,
const char *backing_fmt, bool warn);
void bdrv_register(BlockDriver *bdrv);
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index c8661775f4..d971d73f8f 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -264,7 +264,7 @@ struct BlockDriver {
BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
bool force, Error **errp);
- int (*bdrv_make_empty)(BlockDriverState *bs);
+ int GRAPH_RDLOCK_PTR (*bdrv_make_empty)(BlockDriverState *bs);
/*
* Refreshes the bs->exact_filename field. If that is impossible,
diff --git a/block.c b/block.c
index 9f6cd5bb8e..39c28a8dde 100644
--- a/block.c
+++ b/block.c
@@ -7249,7 +7249,8 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
{
BdrvOpBlocker *blocker;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+
+ assume_graph_lock(); /* FIXME */
assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
if (!QLIST_EMPTY(&bs->op_blockers[op])) {
diff --git a/block/block-backend.c b/block/block-backend.c
index c9193ae4b9..0fd1b4a437 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2905,6 +2905,8 @@ const BdrvChild *blk_root(BlockBackend *blk)
int blk_make_empty(BlockBackend *blk, Error **errp)
{
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (!blk_is_available(blk)) {
error_setg(errp, "No medium inserted");
return -ENOMEDIUM;
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 4e20a28cc7..80f3ecea62 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -206,8 +206,9 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
return 0;
}
-static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
- uint32_t bitmap_table_size)
+static void GRAPH_RDLOCK
+clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
+ uint32_t bitmap_table_size)
{
BDRVQcow2State *s = bs->opaque;
int i;
@@ -261,7 +262,8 @@ fail:
return ret;
}
-static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb)
+static int GRAPH_RDLOCK
+free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb)
{
int ret;
uint64_t *bitmap_table;
@@ -732,8 +734,9 @@ out:
* Store bitmap list to qcow2 image as a bitmap directory.
* Everything is checked.
*/
-static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
- uint64_t *offset, uint64_t *size, bool in_place)
+static int GRAPH_RDLOCK
+bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
+ uint64_t *offset, uint64_t *size, bool in_place)
{
int ret;
uint8_t *dir;
@@ -831,8 +834,9 @@ fail:
* Bitmap List end
*/
-static int update_ext_header_and_dir_in_place(BlockDriverState *bs,
- Qcow2BitmapList *bm_list)
+static int GRAPH_RDLOCK
+update_ext_header_and_dir_in_place(BlockDriverState *bs,
+ Qcow2BitmapList *bm_list)
{
BDRVQcow2State *s = bs->opaque;
int ret;
@@ -879,8 +883,8 @@ static int update_ext_header_and_dir_in_place(BlockDriverState *bs,
*/
}
-static int update_ext_header_and_dir(BlockDriverState *bs,
- Qcow2BitmapList *bm_list)
+static int GRAPH_RDLOCK
+update_ext_header_and_dir(BlockDriverState *bs, Qcow2BitmapList *bm_list)
{
BDRVQcow2State *s = bs->opaque;
int ret;
@@ -1273,9 +1277,9 @@ out:
/* store_bitmap_data()
* Store bitmap to image, filling bitmap table accordingly.
*/
-static uint64_t *store_bitmap_data(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap,
- uint32_t *bitmap_table_size, Error **errp)
+static uint64_t * GRAPH_RDLOCK
+store_bitmap_data(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+ uint32_t *bitmap_table_size, Error **errp)
{
int ret;
BDRVQcow2State *s = bs->opaque;
@@ -1372,7 +1376,8 @@ fail:
* Store bm->dirty_bitmap to qcow2.
* Set bm->table_offset and bm->table_size accordingly.
*/
-static int store_bitmap(BlockDriverState *bs, Qcow2Bitmap *bm, Error **errp)
+static int GRAPH_RDLOCK
+store_bitmap(BlockDriverState *bs, Qcow2Bitmap *bm, Error **errp)
{
int ret;
uint64_t *tb;
diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 01c67bdddc..23d9588b08 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -163,7 +163,8 @@ int qcow2_cache_destroy(Qcow2Cache *c)
return 0;
}
-static int qcow2_cache_flush_dependency(BlockDriverState *bs, Qcow2Cache *c)
+static int GRAPH_RDLOCK
+qcow2_cache_flush_dependency(BlockDriverState *bs, Qcow2Cache *c)
{
int ret;
@@ -178,7 +179,8 @@ static int qcow2_cache_flush_dependency(BlockDriverState *bs, Qcow2Cache *c)
return 0;
}
-static int qcow2_cache_entry_flush(BlockDriverState *bs, Qcow2Cache *c, int i)
+static int GRAPH_RDLOCK
+qcow2_cache_entry_flush(BlockDriverState *bs, Qcow2Cache *c, int i)
{
BDRVQcow2State *s = bs->opaque;
int ret = 0;
@@ -318,8 +320,9 @@ int qcow2_cache_empty(BlockDriverState *bs, Qcow2Cache *c)
return 0;
}
-static int qcow2_cache_do_get(BlockDriverState *bs, Qcow2Cache *c,
- uint64_t offset, void **table, bool read_from_disk)
+static int GRAPH_RDLOCK
+qcow2_cache_do_get(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
+ void **table, bool read_from_disk)
{
BDRVQcow2State *s = bs->opaque;
int i;
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f4f6cd6ad0..904f00d1b3 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -207,8 +207,9 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
* the cache is used; otherwise the L2 slice is loaded from the image
* file.
*/
-static int l2_load(BlockDriverState *bs, uint64_t offset,
- uint64_t l2_offset, uint64_t **l2_slice)
+static int GRAPH_RDLOCK
+l2_load(BlockDriverState *bs, uint64_t offset,
+ uint64_t l2_offset, uint64_t **l2_slice)
{
BDRVQcow2State *s = bs->opaque;
int start_of_slice = l2_entry_size(s) *
@@ -269,7 +270,7 @@ int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
*
*/
-static int l2_allocate(BlockDriverState *bs, int l1_index)
+static int GRAPH_RDLOCK l2_allocate(BlockDriverState *bs, int l1_index)
{
BDRVQcow2State *s = bs->opaque;
uint64_t old_l2_offset;
@@ -751,9 +752,9 @@ fail:
*
* Returns 0 on success, -errno in failure case
*/
-static int get_cluster_table(BlockDriverState *bs, uint64_t offset,
- uint64_t **new_l2_slice,
- int *new_l2_index)
+static int GRAPH_RDLOCK
+get_cluster_table(BlockDriverState *bs, uint64_t offset,
+ uint64_t **new_l2_slice, int *new_l2_index)
{
BDRVQcow2State *s = bs->opaque;
unsigned int l2_index;
@@ -1155,11 +1156,10 @@ void coroutine_fn qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m)
*
* Returns 0 on success, -errno on failure.
*/
-static int coroutine_fn calculate_l2_meta(BlockDriverState *bs,
- uint64_t host_cluster_offset,
- uint64_t guest_offset, unsigned bytes,
- uint64_t *l2_slice, QCowL2Meta **m,
- bool keep_old)
+static int coroutine_fn GRAPH_RDLOCK
+calculate_l2_meta(BlockDriverState *bs, uint64_t host_cluster_offset,
+ uint64_t guest_offset, unsigned bytes, uint64_t *l2_slice,
+ QCowL2Meta **m, bool keep_old)
{
BDRVQcow2State *s = bs->opaque;
int sc_index, l2_index = offset_to_l2_slice_index(s, guest_offset);
@@ -1490,9 +1490,9 @@ static int coroutine_fn handle_dependencies(BlockDriverState *bs,
*
* -errno: in error cases
*/
-static int coroutine_fn handle_copied(BlockDriverState *bs,
- uint64_t guest_offset, uint64_t *host_offset, uint64_t *bytes,
- QCowL2Meta **m)
+static int coroutine_fn GRAPH_RDLOCK
+handle_copied(BlockDriverState *bs, uint64_t guest_offset,
+ uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m)
{
BDRVQcow2State *s = bs->opaque;
int l2_index;
@@ -1600,10 +1600,9 @@ out:
* function has been waiting for another request and the allocation must be
* restarted, but the whole request should not be failed.
*/
-static int coroutine_fn do_alloc_cluster_offset(BlockDriverState *bs,
- uint64_t guest_offset,
- uint64_t *host_offset,
- uint64_t *nb_clusters)
+static int coroutine_fn GRAPH_RDLOCK
+do_alloc_cluster_offset(BlockDriverState *bs, uint64_t guest_offset,
+ uint64_t *host_offset, uint64_t *nb_clusters)
{
BDRVQcow2State *s = bs->opaque;
@@ -1658,9 +1657,9 @@ static int coroutine_fn do_alloc_cluster_offset(BlockDriverState *bs,
*
* -errno: in error cases
*/
-static int coroutine_fn handle_alloc(BlockDriverState *bs,
- uint64_t guest_offset, uint64_t *host_offset, uint64_t *bytes,
- QCowL2Meta **m)
+static int coroutine_fn GRAPH_RDLOCK
+handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
+ uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m)
{
BDRVQcow2State *s = bs->opaque;
int l2_index;
@@ -1898,9 +1897,9 @@ again:
* all clusters in the same L2 slice) and returns the number of discarded
* clusters.
*/
-static int discard_in_l2_slice(BlockDriverState *bs, uint64_t offset,
- uint64_t nb_clusters,
- enum qcow2_discard_type type, bool full_discard)
+static int GRAPH_RDLOCK
+discard_in_l2_slice(BlockDriverState *bs, uint64_t offset, uint64_t nb_clusters,
+ enum qcow2_discard_type type, bool full_discard)
{
BDRVQcow2State *s = bs->opaque;
uint64_t *l2_slice;
@@ -2037,7 +2036,7 @@ fail:
* all clusters in the same L2 slice) and returns the number of zeroed
* clusters.
*/
-static int coroutine_fn
+static int coroutine_fn GRAPH_RDLOCK
zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
uint64_t nb_clusters, int flags)
{
@@ -2093,7 +2092,7 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
return nb_clusters;
}
-static int coroutine_fn
+static int coroutine_fn GRAPH_RDLOCK
zero_l2_subclusters(BlockDriverState *bs, uint64_t offset,
unsigned nb_subclusters)
{
@@ -2231,11 +2230,12 @@ fail:
* status_cb(). l1_entries contains the total number of L1 entries and
* *visited_l1_entries counts all visited L1 entries.
*/
-static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
- int l1_size, int64_t *visited_l1_entries,
- int64_t l1_entries,
- BlockDriverAmendStatusCB *status_cb,
- void *cb_opaque)
+static int GRAPH_RDLOCK
+expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
+ int l1_size, int64_t *visited_l1_entries,
+ int64_t l1_entries,
+ BlockDriverAmendStatusCB *status_cb,
+ void *cb_opaque)
{
BDRVQcow2State *s = bs->opaque;
bool is_active_l1 = (l1_table == s->l1_table);
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 996d1217d0..0266542cee 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -229,9 +229,9 @@ static void set_refcount_ro6(void *refcount_array, uint64_t index,
}
-static int load_refcount_block(BlockDriverState *bs,
- int64_t refcount_block_offset,
- void **refcount_block)
+static int GRAPH_RDLOCK
+load_refcount_block(BlockDriverState *bs, int64_t refcount_block_offset,
+ void **refcount_block)
{
BDRVQcow2State *s = bs->opaque;
@@ -302,8 +302,9 @@ static int in_same_refcount_block(BDRVQcow2State *s, uint64_t offset_a,
*
* Returns 0 on success or -errno in error case
*/
-static int alloc_refcount_block(BlockDriverState *bs,
- int64_t cluster_index, void **refcount_block)
+static int GRAPH_RDLOCK
+alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index,
+ void **refcount_block)
{
BDRVQcow2State *s = bs->opaque;
unsigned int refcount_table_index;
@@ -806,12 +807,9 @@ found:
/* XXX: cache several refcount block clusters ? */
/* @addend is the absolute value of the addend; if @decrease is set, @addend
* will be subtracted from the current refcount, otherwise it will be added */
-static int update_refcount(BlockDriverState *bs,
- int64_t offset,
- int64_t length,
- uint64_t addend,
- bool decrease,
- enum qcow2_discard_type type)
+static int GRAPH_RDLOCK
+update_refcount(BlockDriverState *bs, int64_t offset, int64_t length,
+ uint64_t addend, bool decrease, enum qcow2_discard_type type)
{
BDRVQcow2State *s = bs->opaque;
int64_t start, last, cluster_offset;
@@ -967,8 +965,8 @@ int qcow2_update_cluster_refcount(BlockDriverState *bs,
/* return < 0 if error */
-static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size,
- uint64_t max)
+static int64_t GRAPH_RDLOCK
+alloc_clusters_noref(BlockDriverState *bs, uint64_t size, uint64_t max)
{
BDRVQcow2State *s = bs->opaque;
uint64_t i, nb_clusters, refcount;
@@ -2302,7 +2300,7 @@ calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
* Compares the actual reference count for each cluster in the image against the
* refcount as reported by the refcount structures on-disk.
*/
-static void coroutine_fn
+static void coroutine_fn GRAPH_RDLOCK
compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
BdrvCheckMode fix, bool *rebuild,
int64_t *highest_cluster,
@@ -3103,20 +3101,22 @@ int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
*
* @allocated should be set to true if a new cluster has been allocated.
*/
-typedef int (RefblockFinishOp)(BlockDriverState *bs, uint64_t **reftable,
- uint64_t reftable_index, uint64_t *reftable_size,
- void *refblock, bool refblock_empty,
- bool *allocated, Error **errp);
+typedef int /* GRAPH_RDLOCK_PTR */
+ (RefblockFinishOp)(BlockDriverState *bs, uint64_t **reftable,
+ uint64_t reftable_index, uint64_t *reftable_size,
+ void *refblock, bool refblock_empty,
+ bool *allocated, Error **errp);
/**
* This "operation" for walk_over_reftable() allocates the refblock on disk (if
* it is not empty) and inserts its offset into the new reftable. The size of
* this new reftable is increased as required.
*/
-static int alloc_refblock(BlockDriverState *bs, uint64_t **reftable,
- uint64_t reftable_index, uint64_t *reftable_size,
- void *refblock, bool refblock_empty, bool *allocated,
- Error **errp)
+static int GRAPH_RDLOCK
+alloc_refblock(BlockDriverState *bs, uint64_t **reftable,
+ uint64_t reftable_index, uint64_t *reftable_size,
+ void *refblock, bool refblock_empty, bool *allocated,
+ Error **errp)
{
BDRVQcow2State *s = bs->opaque;
int64_t offset;
@@ -3166,10 +3166,11 @@ static int alloc_refblock(BlockDriverState *bs, uint64_t **reftable,
* offset specified by the new reftable's entry. It does not modify the new
* reftable or change any refcounts.
*/
-static int flush_refblock(BlockDriverState *bs, uint64_t **reftable,
- uint64_t reftable_index, uint64_t *reftable_size,
- void *refblock, bool refblock_empty, bool *allocated,
- Error **errp)
+static int GRAPH_RDLOCK
+flush_refblock(BlockDriverState *bs, uint64_t **reftable,
+ uint64_t reftable_index, uint64_t *reftable_size,
+ void *refblock, bool refblock_empty, bool *allocated,
+ Error **errp)
{
BDRVQcow2State *s = bs->opaque;
int64_t offset;
@@ -3210,16 +3211,17 @@ static int flush_refblock(BlockDriverState *bs, uint64_t **reftable,
*
* @allocated is set to true if a new cluster has been allocated.
*/
-static int walk_over_reftable(BlockDriverState *bs, uint64_t **new_reftable,
- uint64_t *new_reftable_index,
- uint64_t *new_reftable_size,
- void *new_refblock, int new_refblock_size,
- int new_refcount_bits,
- RefblockFinishOp *operation, bool *allocated,
- Qcow2SetRefcountFunc *new_set_refcount,
- BlockDriverAmendStatusCB *status_cb,
- void *cb_opaque, int index, int total,
- Error **errp)
+static int GRAPH_RDLOCK
+walk_over_reftable(BlockDriverState *bs, uint64_t **new_reftable,
+ uint64_t *new_reftable_index,
+ uint64_t *new_reftable_size,
+ void *new_refblock, int new_refblock_size,
+ int new_refcount_bits,
+ RefblockFinishOp *operation, bool *allocated,
+ Qcow2SetRefcountFunc *new_set_refcount,
+ BlockDriverAmendStatusCB *status_cb,
+ void *cb_opaque, int index, int total,
+ Error **errp)
{
BDRVQcow2State *s = bs->opaque;
uint64_t reftable_index;
@@ -3545,8 +3547,8 @@ done:
return ret;
}
-static int64_t coroutine_fn get_refblock_offset(BlockDriverState *bs,
- uint64_t offset)
+static int64_t coroutine_fn GRAPH_RDLOCK
+get_refblock_offset(BlockDriverState *bs, uint64_t offset)
{
BDRVQcow2State *s = bs->opaque;
uint32_t index = offset_to_reftable_index(s, offset);
@@ -3565,7 +3567,7 @@ static int64_t coroutine_fn get_refblock_offset(BlockDriverState *bs,
return covering_refblock_offset;
}
-static int coroutine_fn
+static int coroutine_fn GRAPH_RDLOCK
qcow2_discard_refcount_block(BlockDriverState *bs, uint64_t discard_block_offs)
{
BDRVQcow2State *s = bs->opaque;
diff --git a/block/qcow2.c b/block/qcow2.c
index db5fe86c5a..1c01de95af 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -536,7 +536,7 @@ int qcow2_mark_dirty(BlockDriverState *bs)
* function when there are no pending requests, it does not guard against
* concurrent requests dirtying the image.
*/
-static int qcow2_mark_clean(BlockDriverState *bs)
+static int GRAPH_RDLOCK qcow2_mark_clean(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
@@ -570,7 +570,8 @@ int qcow2_mark_corrupt(BlockDriverState *bs)
* Marks the image as consistent, i.e., unsets the corrupt bit, and flushes
* before if necessary.
*/
-static int coroutine_fn qcow2_mark_consistent(BlockDriverState *bs)
+static int coroutine_fn GRAPH_RDLOCK
+qcow2_mark_consistent(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
@@ -980,10 +981,9 @@ typedef struct Qcow2ReopenState {
QCryptoBlockOpenOptions *crypto_opts; /* Disk encryption runtime options */
} Qcow2ReopenState;
-static int qcow2_update_options_prepare(BlockDriverState *bs,
- Qcow2ReopenState *r,
- QDict *options, int flags,
- Error **errp)
+static int GRAPH_RDLOCK
+qcow2_update_options_prepare(BlockDriverState *bs, Qcow2ReopenState *r,
+ QDict *options, int flags, Error **errp)
{
BDRVQcow2State *s = bs->opaque;
QemuOpts *opts = NULL;
@@ -1260,7 +1260,7 @@ static void qcow2_update_options_abort(BlockDriverState *bs,
qapi_free_QCryptoBlockOpenOptions(r->crypto_opts);
}
-static int coroutine_fn
+static int coroutine_fn GRAPH_RDLOCK
qcow2_update_options(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -1969,13 +1969,17 @@ static void qcow2_refresh_limits(BlockDriverState *bs, Error **errp)
bs->bl.pdiscard_alignment = s->cluster_size;
}
-static int qcow2_reopen_prepare(BDRVReopenState *state,
- BlockReopenQueue *queue, Error **errp)
+static int GRAPH_UNLOCKED
+qcow2_reopen_prepare(BDRVReopenState *state,BlockReopenQueue *queue,
+ Error **errp)
{
BDRVQcow2State *s = state->bs->opaque;
Qcow2ReopenState *r;
int ret;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
r = g_new0(Qcow2ReopenState, 1);
state->opaque = r;
@@ -2038,6 +2042,8 @@ static void qcow2_reopen_commit(BDRVReopenState *state)
static void qcow2_reopen_commit_post(BDRVReopenState *state)
{
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (state->flags & BDRV_O_RDWR) {
Error *local_err = NULL;
@@ -4078,8 +4084,8 @@ qcow2_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes,
return ret;
}
-static coroutine_fn int qcow2_co_pdiscard(BlockDriverState *bs,
- int64_t offset, int64_t bytes)
+static int coroutine_fn GRAPH_RDLOCK
+qcow2_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
{
int ret;
BDRVQcow2State *s = bs->opaque;
@@ -4824,7 +4830,7 @@ fail:
return ret;
}
-static int make_completely_empty(BlockDriverState *bs)
+static int GRAPH_RDLOCK make_completely_empty(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
Error *local_err = NULL;
@@ -4975,7 +4981,7 @@ fail:
return ret;
}
-static int qcow2_make_empty(BlockDriverState *bs)
+static int GRAPH_RDLOCK qcow2_make_empty(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
uint64_t offset, end_offset;
@@ -5019,7 +5025,7 @@ static int qcow2_make_empty(BlockDriverState *bs)
return ret;
}
-static coroutine_fn int qcow2_co_flush_to_os(BlockDriverState *bs)
+static coroutine_fn GRAPH_RDLOCK int qcow2_co_flush_to_os(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
int ret;
@@ -5368,7 +5374,7 @@ qcow2_co_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
return bs->drv->bdrv_co_preadv_part(bs, offset, qiov->size, qiov, 0, 0);
}
-static int qcow2_has_compressed_clusters(BlockDriverState *bs)
+static int GRAPH_RDLOCK qcow2_has_compressed_clusters(BlockDriverState *bs)
{
int64_t offset = 0;
int64_t bytes = bdrv_getlength(bs);
@@ -5404,9 +5410,10 @@ static int qcow2_has_compressed_clusters(BlockDriverState *bs)
* Downgrades an image's version. To achieve this, any incompatible features
* have to be removed.
*/
-static int qcow2_downgrade(BlockDriverState *bs, int target_version,
- BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
- Error **errp)
+static int GRAPH_RDLOCK
+qcow2_downgrade(BlockDriverState *bs, int target_version,
+ BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
+ Error **errp)
{
BDRVQcow2State *s = bs->opaque;
int current_version = s->qcow_version;
@@ -5514,9 +5521,10 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
* features of older versions, some things may have to be presented
* differently.
*/
-static int qcow2_upgrade(BlockDriverState *bs, int target_version,
- BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
- Error **errp)
+static int GRAPH_RDLOCK
+qcow2_upgrade(BlockDriverState *bs, int target_version,
+ BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
+ Error **errp)
{
BDRVQcow2State *s = bs->opaque;
bool need_snapshot_update;
@@ -5642,11 +5650,10 @@ static void qcow2_amend_helper_cb(BlockDriverState *bs,
info->original_cb_opaque);
}
-static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
- BlockDriverAmendStatusCB *status_cb,
- void *cb_opaque,
- bool force,
- Error **errp)
+static int GRAPH_RDLOCK
+qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
+ BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
+ bool force, Error **errp)
{
BDRVQcow2State *s = bs->opaque;
int old_version = s->qcow_version, new_version = old_version;
@@ -5946,8 +5953,6 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
char *message;
va_list ap;
- assume_graph_lock(); /* FIXME */
-
fatal = fatal && bdrv_is_writable(bs);
if (s->signaled_corruption &&
diff --git a/block/replication.c b/block/replication.c
index 7e0b615805..7d2cec7786 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -307,13 +307,16 @@ out:
return ret;
}
-static void secondary_do_checkpoint(BlockDriverState *bs, Error **errp)
+static void GRAPH_UNLOCKED
+secondary_do_checkpoint(BlockDriverState *bs, Error **errp)
{
BDRVReplicationState *s = bs->opaque;
BdrvChild *active_disk = bs->file;
Error *local_err = NULL;
int ret;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (!s->backup_job) {
error_setg(errp, "Backup job was cancelled unexpectedly");
return;
@@ -531,13 +534,16 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
/* Must be true, or the bdrv_getlength() calls would have failed */
assert(active_disk->bs->drv && hidden_disk->bs->drv);
+ bdrv_graph_rdlock_main_loop();
if (!active_disk->bs->drv->bdrv_make_empty ||
!hidden_disk->bs->drv->bdrv_make_empty) {
error_setg(errp,
"Active disk or hidden disk doesn't support make_empty");
aio_context_release(aio_context);
+ bdrv_graph_rdunlock_main_loop();
return;
}
+ bdrv_graph_rdunlock_main_loop();
/* reopen the backing file in r/w mode */
reopen_backing_file(bs, true, &local_err);
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 15/22] qcow2: Mark qcow2_inactivate() and callers GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (13 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 14/22] qcow2: Mark qcow2_signal_corruption() " Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 16/22] qcow2: Mark check_constraints_on_bitmap() GRAPH_RDLOCK Kevin Wolf
` (7 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
This adds GRAPH_RDLOCK annotations to declare that callers of
qcow2_inactivate() need to hold a reader lock for the graph because it
calls bdrv_get_device_or_node_name(), which accesses the parents list of
a node.
qcow2_do_close() is a bit strange because it is called from different
contexts. If close_data_file = true, we know that we were called from
non-coroutine main loop context (more specifically, we're coming from
qcow2_close()) and can safely drop the reader lock temporarily with
bdrv_graph_rdunlock_main_loop() and acquire the writer lock.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 1c01de95af..490b763f57 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2737,14 +2737,12 @@ fail_nometa:
return ret;
}
-static int qcow2_inactivate(BlockDriverState *bs)
+static int GRAPH_RDLOCK qcow2_inactivate(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
int ret, result = 0;
Error *local_err = NULL;
- assume_graph_lock(); /* FIXME */
-
qcow2_store_persistent_dirty_bitmaps(bs, true, &local_err);
if (local_err != NULL) {
result = -EINVAL;
@@ -2774,7 +2772,8 @@ static int qcow2_inactivate(BlockDriverState *bs)
return result;
}
-static void qcow2_do_close(BlockDriverState *bs, bool close_data_file)
+static void coroutine_mixed_fn GRAPH_RDLOCK
+qcow2_do_close(BlockDriverState *bs, bool close_data_file)
{
BDRVQcow2State *s = bs->opaque;
qemu_vfree(s->l1_table);
@@ -2801,18 +2800,24 @@ static void qcow2_do_close(BlockDriverState *bs, bool close_data_file)
g_free(s->image_backing_format);
if (close_data_file && has_data_file(bs)) {
+ GLOBAL_STATE_CODE();
+ bdrv_graph_rdunlock_main_loop();
bdrv_graph_wrlock(NULL);
bdrv_unref_child(bs, s->data_file);
bdrv_graph_wrunlock();
s->data_file = NULL;
+ bdrv_graph_rdlock_main_loop();
}
qcow2_refcount_close(bs);
qcow2_free_snapshots(bs);
}
-static void qcow2_close(BlockDriverState *bs)
+static void GRAPH_UNLOCKED qcow2_close(BlockDriverState *bs)
{
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
qcow2_do_close(bs, true);
}
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 16/22] qcow2: Mark check_constraints_on_bitmap() GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (14 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 15/22] qcow2: Mark qcow2_inactivate() " Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 17/22] block: Mark bdrv_op_is_blocked() and callers GRAPH_RDLOCK Kevin Wolf
` (6 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
It still has an assume_graph_lock() call, but all of its callers are now
properly annotated to hold the graph lock. Update the function to be
GRAPH_RDLOCK as well and remove the assume_graph_lock().
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2-bitmap.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 80f3ecea62..9c1b5e3d00 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -156,18 +156,15 @@ static int64_t get_bitmap_bytes_needed(int64_t len, uint32_t granularity)
return DIV_ROUND_UP(num_bits, 8);
}
-static int check_constraints_on_bitmap(BlockDriverState *bs,
- const char *name,
- uint32_t granularity,
- Error **errp)
+static int GRAPH_RDLOCK
+check_constraints_on_bitmap(BlockDriverState *bs, const char *name,
+ uint32_t granularity, Error **errp)
{
BDRVQcow2State *s = bs->opaque;
int granularity_bits = ctz32(granularity);
int64_t len = bdrv_getlength(bs);
int64_t bitmap_bytes;
- assume_graph_lock(); /* FIXME */
-
assert(granularity > 0);
assert((granularity & (granularity - 1)) == 0);
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 17/22] block: Mark bdrv_op_is_blocked() and callers GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (15 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 16/22] qcow2: Mark check_constraints_on_bitmap() GRAPH_RDLOCK Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 18/22] block: Mark bdrv_apply_auto_read_only() " Kevin Wolf
` (5 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_op_is_blocked() need to hold a reader lock for the graph
because it calls bdrv_get_device_or_node_name(), which accesses the
parents list of a node.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-global-state.h | 4 +++-
block.c | 2 --
block/block-backend.c | 1 +
block/commit.c | 1 +
block/monitor/block-hmp-cmds.c | 3 +++
block/qapi-sysemu.c | 9 +++++++--
blockdev.c | 15 +++++++++++++++
7 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 794ef34ded..6bfafe781d 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -246,7 +246,9 @@ bdrv_attach_child(BlockDriverState *parent_bs,
BdrvChildRole child_role,
Error **errp);
-bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
+bool GRAPH_RDLOCK
+bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
+
void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason);
void bdrv_op_block_all(BlockDriverState *bs, Error *reason);
diff --git a/block.c b/block.c
index 39c28a8dde..0814b0704e 100644
--- a/block.c
+++ b/block.c
@@ -7250,8 +7250,6 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
BdrvOpBlocker *blocker;
GLOBAL_STATE_CODE();
- assume_graph_lock(); /* FIXME */
-
assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
if (!QLIST_EMPTY(&bs->op_blockers[op])) {
blocker = QLIST_FIRST(&bs->op_blockers[op]);
diff --git a/block/block-backend.c b/block/block-backend.c
index 0fd1b4a437..a7057400d7 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2388,6 +2388,7 @@ bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp)
{
BlockDriverState *bs = blk_bs(blk);
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
if (!bs) {
return false;
diff --git a/block/commit.c b/block/commit.c
index aa45beb0f0..43d1de7577 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -434,6 +434,7 @@ int bdrv_commit(BlockDriverState *bs)
Error *local_err = NULL;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
if (!drv)
return -ENOMEDIUM;
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 6b7d7dd072..7645c7e5fb 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -144,6 +144,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
AioContext *aio_context;
Error *local_err = NULL;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
bs = bdrv_find_node(id);
if (bs) {
qmp_blockdev_del(id, &local_err);
diff --git a/block/qapi-sysemu.c b/block/qapi-sysemu.c
index e885a64c32..3f614cbc04 100644
--- a/block/qapi-sysemu.c
+++ b/block/qapi-sysemu.c
@@ -169,14 +169,16 @@ void qmp_blockdev_close_tray(const char *device,
}
}
-static void blockdev_remove_medium(const char *device, const char *id,
- Error **errp)
+static void GRAPH_UNLOCKED
+blockdev_remove_medium(const char *device, const char *id, Error **errp)
{
BlockBackend *blk;
BlockDriverState *bs;
AioContext *aio_context;
bool has_attached_device;
+ GLOBAL_STATE_CODE();
+
blk = qmp_get_blk(device, id, errp);
if (!blk) {
return;
@@ -205,9 +207,12 @@ static void blockdev_remove_medium(const char *device, const char *id,
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
+ bdrv_graph_rdlock_main_loop();
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
+ bdrv_graph_rdunlock_main_loop();
goto out;
}
+ bdrv_graph_rdunlock_main_loop();
blk_remove_bs(blk);
diff --git a/blockdev.c b/blockdev.c
index 51c58dd432..a01c62596b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1696,9 +1696,12 @@ static void drive_backup_action(DriveBackup *backup,
}
/* Early check to avoid creating target */
+ bdrv_graph_rdlock_main_loop();
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
+ bdrv_graph_rdunlock_main_loop();
goto out;
}
+ bdrv_graph_rdunlock_main_loop();
flags = bs->open_flags | BDRV_O_RDWR;
@@ -2360,10 +2363,13 @@ void coroutine_fn qmp_block_resize(const char *device, const char *node_name,
return;
}
+ bdrv_graph_co_rdlock();
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, NULL)) {
error_setg(errp, QERR_DEVICE_IN_USE, device);
+ bdrv_graph_co_rdunlock();
return;
}
+ bdrv_graph_co_rdunlock();
blk = blk_co_new_with_bs(bs, BLK_PERM_RESIZE, BLK_PERM_ALL, errp);
if (!blk) {
@@ -2487,13 +2493,16 @@ void qmp_block_stream(const char *job_id, const char *device,
* Check for op blockers in the whole chain between bs and base (or bottom)
*/
iter_end = bottom ? bdrv_filter_or_cow_bs(bottom_bs) : base_bs;
+ bdrv_graph_rdlock_main_loop();
for (iter = bs; iter && iter != iter_end;
iter = bdrv_filter_or_cow_bs(iter))
{
if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
+ bdrv_graph_rdunlock_main_loop();
goto out;
}
}
+ bdrv_graph_rdunlock_main_loop();
/* if we are streaming the entire chain, the result will have no backing
* file, and specifying one is therefore an error */
@@ -3021,9 +3030,12 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
}
/* Early check to avoid creating target */
+ bdrv_graph_rdlock_main_loop();
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
+ bdrv_graph_rdunlock_main_loop();
return;
}
+ bdrv_graph_rdunlock_main_loop();
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
@@ -3409,9 +3421,12 @@ void qmp_change_backing_file(const char *device,
/* even though we are not necessarily operating on bs, we need it to
* determine if block ops are currently prohibited on the chain */
+ bdrv_graph_rdlock_main_loop();
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_CHANGE, errp)) {
+ bdrv_graph_rdunlock_main_loop();
goto out;
}
+ bdrv_graph_rdunlock_main_loop();
/* final sanity check */
if (!bdrv_chain_contains(bs, image_bs)) {
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 18/22] block: Mark bdrv_apply_auto_read_only() and callers GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (16 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 17/22] block: Mark bdrv_op_is_blocked() and callers GRAPH_RDLOCK Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 19/22] block: Mark bdrv_get_specific_info() " Kevin Wolf
` (4 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_apply_auto_read_only() need to hold a reader lock for the graph
because it calls bdrv_can_set_read_only(), which indirectly accesses the
parents list of a node.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-io.h | 6 ++++--
block.c | 4 ++--
block/bochs.c | 2 ++
block/cloop.c | 2 ++
block/curl.c | 2 ++
block/dmg.c | 2 ++
block/gluster.c | 2 ++
block/iscsi.c | 2 ++
block/nbd.c | 3 ++-
block/rbd.c | 2 ++
10 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 5b1a1d5998..13cc756ed8 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -156,8 +156,10 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
int coroutine_fn GRAPH_RDLOCK
bdrv_co_is_zero_fast(BlockDriverState *bs, int64_t offset, int64_t bytes);
-int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
- Error **errp);
+int GRAPH_RDLOCK
+bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
+ Error **errp);
+
bool bdrv_is_read_only(BlockDriverState *bs);
bool bdrv_is_writable(BlockDriverState *bs);
bool bdrv_is_sg(BlockDriverState *bs);
diff --git a/block.c b/block.c
index 0814b0704e..3178c1d819 100644
--- a/block.c
+++ b/block.c
@@ -320,8 +320,6 @@ int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
int ret = 0;
IO_CODE();
- assume_graph_lock(); /* FIXME */
-
if (!(bs->open_flags & BDRV_O_RDWR)) {
return 0;
}
@@ -1908,7 +1906,9 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, ro)) {
if (!ro && bdrv_is_whitelisted(drv, true)) {
+ bdrv_graph_rdlock_main_loop();
ret = bdrv_apply_auto_read_only(bs, NULL, NULL);
+ bdrv_graph_rdunlock_main_loop();
} else {
ret = -ENOTSUP;
}
diff --git a/block/bochs.c b/block/bochs.c
index 66e7a58e5e..8c659fa9b9 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -106,7 +106,9 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
int ret;
/* No write support yet */
+ bdrv_graph_rdlock_main_loop();
ret = bdrv_apply_auto_read_only(bs, NULL, errp);
+ bdrv_graph_rdunlock_main_loop();
if (ret < 0) {
return ret;
}
diff --git a/block/cloop.c b/block/cloop.c
index 835a0fe3da..773d7918be 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -67,7 +67,9 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
uint32_t offsets_size, max_compressed_block_size = 1, i;
int ret;
+ bdrv_graph_rdlock_main_loop();
ret = bdrv_apply_auto_read_only(bs, NULL, errp);
+ bdrv_graph_rdunlock_main_loop();
if (ret < 0) {
return ret;
}
diff --git a/block/curl.c b/block/curl.c
index 0fc42d03d7..419f7c89ef 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -696,8 +696,10 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
const char *protocol_delimiter;
int ret;
+ bdrv_graph_rdlock_main_loop();
ret = bdrv_apply_auto_read_only(bs, "curl driver does not support writes",
errp);
+ bdrv_graph_rdunlock_main_loop();
if (ret < 0) {
return ret;
}
diff --git a/block/dmg.c b/block/dmg.c
index 06a0244a9c..38ee72bbe5 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -452,7 +452,9 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
int64_t offset;
int ret;
+ bdrv_graph_rdlock_main_loop();
ret = bdrv_apply_auto_read_only(bs, NULL, errp);
+ bdrv_graph_rdunlock_main_loop();
if (ret < 0) {
return ret;
}
diff --git a/block/gluster.c b/block/gluster.c
index ad5fadbe79..cc74af06dc 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -863,11 +863,13 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options,
if (ret == -EACCES || ret == -EROFS) {
/* Try to degrade to read-only, but if it doesn't work, still use the
* normal error message. */
+ bdrv_graph_rdlock_main_loop();
if (bdrv_apply_auto_read_only(bs, NULL, NULL) == 0) {
open_flags = (open_flags & ~O_RDWR) | O_RDONLY;
s->fd = glfs_open(s->glfs, gconf->path, open_flags);
ret = s->fd ? 0 : -errno;
}
+ bdrv_graph_rdunlock_main_loop();
}
s->supports_seek_data = qemu_gluster_test_seek(s->fd);
diff --git a/block/iscsi.c b/block/iscsi.c
index 5640c8b565..2ff14b7472 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1925,7 +1925,9 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
/* Check the write protect flag of the LUN if we want to write */
if (iscsilun->type == TYPE_DISK && (flags & BDRV_O_RDWR) &&
iscsilun->write_protected) {
+ bdrv_graph_rdlock_main_loop();
ret = bdrv_apply_auto_read_only(bs, "LUN is write protected", errp);
+ bdrv_graph_rdunlock_main_loop();
if (ret < 0) {
goto out;
}
diff --git a/block/nbd.c b/block/nbd.c
index cc48580df7..2236a5a3a9 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -275,7 +275,8 @@ static bool nbd_client_will_reconnect(BDRVNBDState *s)
* Return failure if the server's advertised options are incompatible with the
* client's needs.
*/
-static int nbd_handle_updated_info(BlockDriverState *bs, Error **errp)
+static int coroutine_fn GRAPH_RDLOCK
+nbd_handle_updated_info(BlockDriverState *bs, Error **errp)
{
BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
int ret;
diff --git a/block/rbd.c b/block/rbd.c
index f2defad9da..d23e6d6b80 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1168,7 +1168,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
/* If we are using an rbd snapshot, we must be r/o, otherwise
* leave as-is */
if (s->snap != NULL) {
+ bdrv_graph_rdlock_main_loop();
r = bdrv_apply_auto_read_only(bs, "rbd snapshots are read-only", errp);
+ bdrv_graph_rdunlock_main_loop();
if (r < 0) {
goto failed_post_open;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 19/22] block: Mark bdrv_get_specific_info() and callers GRAPH_RDLOCK
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (17 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 18/22] block: Mark bdrv_apply_auto_read_only() " Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 20/22] block: Protect bs->parents with graph_lock Kevin Wolf
` (3 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_get_specific_info() need to hold a reader lock for the graph.
This removes an assume_graph_lock() call in vmdk's implementation.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-io.h | 5 +++--
include/block/block_int-common.h | 4 ++--
block/vmdk.c | 6 ++----
qemu-io-cmds.c | 3 +++
4 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 13cc756ed8..0c09693b0f 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -191,8 +191,9 @@ bdrv_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
int co_wrapper_mixed_bdrv_rdlock
bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
-ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs,
- Error **errp);
+ImageInfoSpecific * GRAPH_RDLOCK
+bdrv_get_specific_info(BlockDriverState *bs, Error **errp);
+
BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs);
void bdrv_round_to_subclusters(BlockDriverState *bs,
int64_t offset, int64_t bytes,
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index d971d73f8f..024262b5a2 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -728,8 +728,8 @@ struct BlockDriver {
int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_get_info)(
BlockDriverState *bs, BlockDriverInfo *bdi);
- ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs,
- Error **errp);
+ ImageInfoSpecific * GRAPH_RDLOCK_PTR (*bdrv_get_specific_info)(
+ BlockDriverState *bs, Error **errp);
BlockStatsSpecific *(*bdrv_get_specific_stats)(BlockDriverState *bs);
int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_save_vmstate)(
diff --git a/block/vmdk.c b/block/vmdk.c
index e50965bc37..8a3b152798 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2990,16 +2990,14 @@ vmdk_co_check(BlockDriverState *bs, BdrvCheckResult *result, BdrvCheckMode fix)
return ret;
}
-static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs,
- Error **errp)
+static ImageInfoSpecific * GRAPH_RDLOCK
+vmdk_get_specific_info(BlockDriverState *bs, Error **errp)
{
int i;
BDRVVmdkState *s = bs->opaque;
ImageInfoSpecific *spec_info = g_new0(ImageInfoSpecific, 1);
VmdkExtentInfoList **tail;
- assume_graph_lock(); /* FIXME */
-
*spec_info = (ImageInfoSpecific){
.type = IMAGE_INFO_SPECIFIC_KIND_VMDK,
.u = {
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 3f75d2f5a6..f5d7202a13 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2037,6 +2037,9 @@ static int info_f(BlockBackend *blk, int argc, char **argv)
char s1[64], s2[64];
int ret;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (bs->drv && bs->drv->format_name) {
printf("format name: %s\n", bs->drv->format_name);
}
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 20/22] block: Protect bs->parents with graph_lock
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (18 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 19/22] block: Mark bdrv_get_specific_info() " Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 21/22] block: Protect bs->children " Kevin Wolf
` (2 subsequent siblings)
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
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 <kwolf@redhat.com>
---
include/block/block_int-common.h | 4 ++--
tests/unit/test-block-iothread.c | 5 +++++
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 024262b5a2..0e37acd976 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1043,7 +1043,7 @@ struct BdrvChild {
bool quiesced_parent;
QLIST_ENTRY(BdrvChild) next;
- QLIST_ENTRY(BdrvChild) next_parent;
+ QLIST_ENTRY(BdrvChild GRAPH_RDLOCK_PTR) next_parent;
};
/*
@@ -1180,7 +1180,7 @@ struct BlockDriverState {
BdrvChild *backing;
BdrvChild *file;
- QLIST_HEAD(, BdrvChild) parents;
+ QLIST_HEAD(, BdrvChild GRAPH_RDLOCK_PTR) parents;
QDict *options;
QDict *explicit_options;
diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c
index 151049bda5..9b15d2768c 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);
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 21/22] block: Protect bs->children with graph_lock
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (19 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 20/22] block: Protect bs->parents with graph_lock Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-09-29 14:51 ` [PATCH 22/22] block: Add assertion for bdrv_graph_wrlock() Kevin Wolf
2023-10-10 20:48 ` [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Stefan Hajnoczi
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
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 <kwolf@redhat.com>
---
include/block/block_int-common.h | 4 ++--
block.c | 2 ++
block/replication.c | 3 ++-
tests/unit/test-bdrv-drain.c | 4 ++++
4 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 0e37acd976..b8d9d24f39 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1042,7 +1042,7 @@ struct BdrvChild {
*/
bool quiesced_parent;
- QLIST_ENTRY(BdrvChild) next;
+ QLIST_ENTRY(BdrvChild GRAPH_RDLOCK_PTR) next;
QLIST_ENTRY(BdrvChild GRAPH_RDLOCK_PTR) next_parent;
};
@@ -1176,7 +1176,7 @@ struct BlockDriverState {
* See also comment in include/block/block.h, to learn how backing and file
* are connected with BdrvChildRole.
*/
- QLIST_HEAD(, BdrvChild) children;
+ QLIST_HEAD(, BdrvChild GRAPH_RDLOCK_PTR) children;
BdrvChild *backing;
BdrvChild *file;
diff --git a/block.c b/block.c
index 3178c1d819..0b50d66747 100644
--- a/block.c
+++ b/block.c
@@ -2973,6 +2973,8 @@ static void bdrv_child_free(BdrvChild *child)
{
assert(!child->bs);
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
assert(!child->next.le_prev); /* not in children list */
g_free(child->name);
diff --git a/block/replication.c b/block/replication.c
index 7d2cec7786..dc880bee4d 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -430,7 +430,8 @@ static void backup_job_completed(void *opaque, int ret)
backup_job_cleanup(bs);
}
-static bool check_top_bs(BlockDriverState *top_bs, BlockDriverState *bs)
+static bool GRAPH_RDLOCK
+check_top_bs(BlockDriverState *top_bs, BlockDriverState *bs)
{
BdrvChild *child;
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index d734829778..f67e9df01c 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;
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 22/22] block: Add assertion for bdrv_graph_wrlock()
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (20 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 21/22] block: Protect bs->children " Kevin Wolf
@ 2023-09-29 14:51 ` Kevin Wolf
2023-10-10 20:48 ` [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Stefan Hajnoczi
22 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-09-29 14:51 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, stefanha, eesposit, eblake, pbonzini, vsementsov,
qemu-devel
bdrv_graph_wrlock() can't run in a coroutine (because it polls) and
requires holding the BQL. We already have GLOBAL_STATE_CODE() to assert
the latter. Assert the former as well and add a no_coroutine_fn marker.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/graph-lock.h | 3 ++-
block/graph-lock.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h
index 7e04f98ff0..6f1cd12745 100644
--- a/include/block/graph-lock.h
+++ b/include/block/graph-lock.h
@@ -116,7 +116,8 @@ void unregister_aiocontext(AioContext *ctx);
* This function polls. Callers must not hold the lock of any AioContext other
* than the current one and the one of @bs.
*/
-void bdrv_graph_wrlock(BlockDriverState *bs) TSA_ACQUIRE(graph_lock) TSA_NO_TSA;
+void no_coroutine_fn TSA_ACQUIRE(graph_lock) TSA_NO_TSA
+bdrv_graph_wrlock(BlockDriverState *bs);
/*
* bdrv_graph_wrunlock:
diff --git a/block/graph-lock.c b/block/graph-lock.c
index 58a799065f..e5525ee2db 100644
--- a/block/graph-lock.c
+++ b/block/graph-lock.c
@@ -106,12 +106,13 @@ static uint32_t reader_count(void)
return rd;
}
-void bdrv_graph_wrlock(BlockDriverState *bs)
+void no_coroutine_fn bdrv_graph_wrlock(BlockDriverState *bs)
{
AioContext *ctx = NULL;
GLOBAL_STATE_CODE();
assert(!qatomic_read(&has_writer));
+ assert(!qemu_in_coroutine());
/*
* Release only non-mainloop AioContext. The mainloop often relies on the
--
2.41.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 00/22] block: Graph locking part 5 (protect children/parent links)
2023-09-29 14:51 [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Kevin Wolf
` (21 preceding siblings ...)
2023-09-29 14:51 ` [PATCH 22/22] block: Add assertion for bdrv_graph_wrlock() Kevin Wolf
@ 2023-10-10 20:48 ` Stefan Hajnoczi
2023-10-11 11:05 ` Kevin Wolf
22 siblings, 1 reply; 25+ messages in thread
From: Stefan Hajnoczi @ 2023-10-10 20:48 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, eesposit, eblake, pbonzini, vsementsov, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6089 bytes --]
On Fri, Sep 29, 2023 at 04:51:35PM +0200, Kevin Wolf wrote:
> After all the preparation in previous series, this series reaches an
> important milestone for the graph locking work: TSA can now verify that
> all accesses to the children and parent lists of nodes happen under the
> graph lock.
>
> However, this is not the end of the graph locking work yet. On the one
> hand, graph locking annotations aren't consistently present on all
> functions and having an unannotated function in the middle of the call
> chain means that TSA doesn't check if the locking is consistent (in
> fact, we know that functions like bdrv_unref() are still called in
> places where they strictly speaking must not be called).
>
> On the other hand, the graph consists of more than just the children and
> parent lists. While it might be possible to convince me that the global
> node lists (graph_bdrv_states/all_bdrv_states) are safe because
> iothreads shouldn't access them, at least BlockDriverState.file/backing
> still need proper locking.
>
> There may be other fields in BlockDriverState that need to be covered
> by the lock, too. For example, Stefan said that he would look into
> annotating BlockLimits accesses to be protected by the graph lock, too.
>
> Emanuele Giuseppe Esposito (1):
> block: Mark drain related functions GRAPH_RDLOCK
>
> Kevin Wolf (21):
> test-bdrv-drain: Don't call bdrv_graph_wrlock() in coroutine context
> block-coroutine-wrapper: Add no_co_wrapper_bdrv_rdlock functions
> block: Take graph rdlock in bdrv_inactivate_all()
> block: Mark bdrv_first_blk() and bdrv_is_root_node() GRAPH_RDLOCK
> block: Mark bdrv_parent_cb_resize() and callers GRAPH_RDLOCK
> block: Mark bdrv_snapshot_fallback() and callers GRAPH_RDLOCK
> block: Take graph rdlock in parts of reopen
> block: Mark bdrv_get_xdbg_block_graph() and callers GRAPH_RDLOCK
> block: Mark bdrv_refresh_filename() and callers GRAPH_RDLOCK
> block: Mark bdrv_primary_child() and callers GRAPH_RDLOCK
> block: Mark bdrv_get_parent_name() and callers GRAPH_RDLOCK
> block: Mark bdrv_amend_options() and callers GRAPH_RDLOCK
> qcow2: Mark qcow2_signal_corruption() and callers GRAPH_RDLOCK
> qcow2: Mark qcow2_inactivate() and callers GRAPH_RDLOCK
> qcow2: Mark check_constraints_on_bitmap() GRAPH_RDLOCK
> block: Mark bdrv_op_is_blocked() and callers GRAPH_RDLOCK
> block: Mark bdrv_apply_auto_read_only() and callers GRAPH_RDLOCK
> block: Mark bdrv_get_specific_info() and callers GRAPH_RDLOCK
> block: Protect bs->parents with graph_lock
> block: Protect bs->children with graph_lock
> block: Add assertion for bdrv_graph_wrlock()
>
> block/qcow2.h | 187 ++++++++++++--------
> block/vhdx.h | 5 +-
> include/block/block-common.h | 7 +-
> include/block/block-global-state.h | 34 ++--
> include/block/block-io.h | 42 +++--
> include/block/block_int-common.h | 69 ++++----
> include/block/block_int-io.h | 7 +-
> include/block/graph-lock.h | 3 +-
> include/block/qapi.h | 23 ++-
> include/block/snapshot.h | 24 +--
> include/sysemu/block-backend-global-state.h | 4 +-
> block.c | 120 +++++++++----
> block/backup.c | 1 +
> block/block-backend.c | 9 +-
> block/bochs.c | 2 +
> block/cloop.c | 2 +
> block/commit.c | 1 +
> block/crypto.c | 4 +-
> block/curl.c | 2 +
> block/dmg.c | 2 +
> block/export/export.c | 4 +
> block/gluster.c | 2 +
> block/graph-lock.c | 3 +-
> block/io.c | 45 ++++-
> block/iscsi.c | 2 +
> block/monitor/block-hmp-cmds.c | 5 +
> block/nbd.c | 3 +-
> block/nfs.c | 2 +-
> block/parallels.c | 3 +
> block/qapi-sysemu.c | 11 +-
> block/qapi.c | 11 +-
> block/qcow.c | 3 +
> block/qcow2-bitmap.c | 38 ++--
> block/qcow2-cache.c | 11 +-
> block/qcow2-cluster.c | 62 +++----
> block/qcow2-refcount.c | 80 +++++----
> block/qcow2.c | 72 +++++---
> block/quorum.c | 4 +-
> block/raw-format.c | 2 +
> block/rbd.c | 4 +
> block/replication.c | 21 ++-
> block/snapshot.c | 54 +++++-
> block/vdi.c | 3 +
> block/vhdx.c | 4 +
> block/vmdk.c | 53 +++---
> block/vpc.c | 3 +
> block/vvfat.c | 2 +
> blockdev.c | 44 +++++
> blockjob.c | 1 +
> migration/block.c | 2 +
> migration/migration-hmp-cmds.c | 2 +
> qemu-img.c | 16 ++
> qemu-io-cmds.c | 3 +
> tests/unit/test-bdrv-drain.c | 15 +-
> tests/unit/test-block-iothread.c | 8 +
> scripts/block-coroutine-wrapper.py | 10 +-
> 56 files changed, 769 insertions(+), 387 deletions(-)
>
> --
> 2.41.0
>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 00/22] block: Graph locking part 5 (protect children/parent links)
2023-10-10 20:48 ` [PATCH 00/22] block: Graph locking part 5 (protect children/parent links) Stefan Hajnoczi
@ 2023-10-11 11:05 ` Kevin Wolf
0 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2023-10-11 11:05 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-block, eesposit, eblake, pbonzini, vsementsov, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6419 bytes --]
Am 10.10.2023 um 22:48 hat Stefan Hajnoczi geschrieben:
> On Fri, Sep 29, 2023 at 04:51:35PM +0200, Kevin Wolf wrote:
> > After all the preparation in previous series, this series reaches an
> > important milestone for the graph locking work: TSA can now verify that
> > all accesses to the children and parent lists of nodes happen under the
> > graph lock.
> >
> > However, this is not the end of the graph locking work yet. On the one
> > hand, graph locking annotations aren't consistently present on all
> > functions and having an unannotated function in the middle of the call
> > chain means that TSA doesn't check if the locking is consistent (in
> > fact, we know that functions like bdrv_unref() are still called in
> > places where they strictly speaking must not be called).
> >
> > On the other hand, the graph consists of more than just the children and
> > parent lists. While it might be possible to convince me that the global
> > node lists (graph_bdrv_states/all_bdrv_states) are safe because
> > iothreads shouldn't access them, at least BlockDriverState.file/backing
> > still need proper locking.
> >
> > There may be other fields in BlockDriverState that need to be covered
> > by the lock, too. For example, Stefan said that he would look into
> > annotating BlockLimits accesses to be protected by the graph lock, too.
> >
> > Emanuele Giuseppe Esposito (1):
> > block: Mark drain related functions GRAPH_RDLOCK
> >
> > Kevin Wolf (21):
> > test-bdrv-drain: Don't call bdrv_graph_wrlock() in coroutine context
> > block-coroutine-wrapper: Add no_co_wrapper_bdrv_rdlock functions
> > block: Take graph rdlock in bdrv_inactivate_all()
> > block: Mark bdrv_first_blk() and bdrv_is_root_node() GRAPH_RDLOCK
> > block: Mark bdrv_parent_cb_resize() and callers GRAPH_RDLOCK
> > block: Mark bdrv_snapshot_fallback() and callers GRAPH_RDLOCK
> > block: Take graph rdlock in parts of reopen
> > block: Mark bdrv_get_xdbg_block_graph() and callers GRAPH_RDLOCK
> > block: Mark bdrv_refresh_filename() and callers GRAPH_RDLOCK
> > block: Mark bdrv_primary_child() and callers GRAPH_RDLOCK
> > block: Mark bdrv_get_parent_name() and callers GRAPH_RDLOCK
> > block: Mark bdrv_amend_options() and callers GRAPH_RDLOCK
> > qcow2: Mark qcow2_signal_corruption() and callers GRAPH_RDLOCK
> > qcow2: Mark qcow2_inactivate() and callers GRAPH_RDLOCK
> > qcow2: Mark check_constraints_on_bitmap() GRAPH_RDLOCK
> > block: Mark bdrv_op_is_blocked() and callers GRAPH_RDLOCK
> > block: Mark bdrv_apply_auto_read_only() and callers GRAPH_RDLOCK
> > block: Mark bdrv_get_specific_info() and callers GRAPH_RDLOCK
> > block: Protect bs->parents with graph_lock
> > block: Protect bs->children with graph_lock
> > block: Add assertion for bdrv_graph_wrlock()
> >
> > block/qcow2.h | 187 ++++++++++++--------
> > block/vhdx.h | 5 +-
> > include/block/block-common.h | 7 +-
> > include/block/block-global-state.h | 34 ++--
> > include/block/block-io.h | 42 +++--
> > include/block/block_int-common.h | 69 ++++----
> > include/block/block_int-io.h | 7 +-
> > include/block/graph-lock.h | 3 +-
> > include/block/qapi.h | 23 ++-
> > include/block/snapshot.h | 24 +--
> > include/sysemu/block-backend-global-state.h | 4 +-
> > block.c | 120 +++++++++----
> > block/backup.c | 1 +
> > block/block-backend.c | 9 +-
> > block/bochs.c | 2 +
> > block/cloop.c | 2 +
> > block/commit.c | 1 +
> > block/crypto.c | 4 +-
> > block/curl.c | 2 +
> > block/dmg.c | 2 +
> > block/export/export.c | 4 +
> > block/gluster.c | 2 +
> > block/graph-lock.c | 3 +-
> > block/io.c | 45 ++++-
> > block/iscsi.c | 2 +
> > block/monitor/block-hmp-cmds.c | 5 +
> > block/nbd.c | 3 +-
> > block/nfs.c | 2 +-
> > block/parallels.c | 3 +
> > block/qapi-sysemu.c | 11 +-
> > block/qapi.c | 11 +-
> > block/qcow.c | 3 +
> > block/qcow2-bitmap.c | 38 ++--
> > block/qcow2-cache.c | 11 +-
> > block/qcow2-cluster.c | 62 +++----
> > block/qcow2-refcount.c | 80 +++++----
> > block/qcow2.c | 72 +++++---
> > block/quorum.c | 4 +-
> > block/raw-format.c | 2 +
> > block/rbd.c | 4 +
> > block/replication.c | 21 ++-
> > block/snapshot.c | 54 +++++-
> > block/vdi.c | 3 +
> > block/vhdx.c | 4 +
> > block/vmdk.c | 53 +++---
> > block/vpc.c | 3 +
> > block/vvfat.c | 2 +
> > blockdev.c | 44 +++++
> > blockjob.c | 1 +
> > migration/block.c | 2 +
> > migration/migration-hmp-cmds.c | 2 +
> > qemu-img.c | 16 ++
> > qemu-io-cmds.c | 3 +
> > tests/unit/test-bdrv-drain.c | 15 +-
> > tests/unit/test-block-iothread.c | 8 +
> > scripts/block-coroutine-wrapper.py | 10 +-
> > 56 files changed, 769 insertions(+), 387 deletions(-)
> >
> > --
> > 2.41.0
> >
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Thanks, applied to the block branch.
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread