* [PATCH v4 01/48] block: remove outdated comments about AioContext locking
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 02/48] block: move drain outside of read-locked bdrv_reopen_queue_child() Fiona Ebner
` (48 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
AioContext locking was removed in commit b49f4755c7 ("block: remove
AioContext locking").
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/block.c b/block.c
index f222e1a50a..a5399888ba 100644
--- a/block.c
+++ b/block.c
@@ -4359,8 +4359,6 @@ bdrv_recurse_has_child(BlockDriverState *bs, BlockDriverState *child)
* bs_queue, or the existing bs_queue being used.
*
* bs is drained here and undrained by bdrv_reopen_queue_free().
- *
- * To be called with bs->aio_context locked.
*/
static BlockReopenQueue * GRAPH_RDLOCK
bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs,
@@ -4519,7 +4517,6 @@ bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs,
return bs_queue;
}
-/* To be called with bs->aio_context locked */
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
BlockDriverState *bs,
QDict *options, bool keep_old_opts)
@@ -7278,10 +7275,6 @@ bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
return true;
}
-/*
- * Must not be called while holding the lock of an AioContext other than the
- * current one.
- */
void bdrv_img_create(const char *filename, const char *fmt,
const char *base_filename, const char *base_fmt,
char *options, uint64_t img_size, int flags, bool quiet,
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 02/48] block: move drain outside of read-locked bdrv_reopen_queue_child()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 01/48] block: remove outdated comments about AioContext locking Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 03/48] block/snapshot: move drain outside of read-locked bdrv_snapshot_delete() Fiona Ebner
` (47 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is in preparation to mark bdrv_drained_begin() as GRAPH_UNLOCKED.
More granular draining is not trivially possible, because
bdrv_reopen_queue_child() can recursively call itself.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/block.c b/block.c
index a5399888ba..3065d5c91e 100644
--- a/block.c
+++ b/block.c
@@ -4358,7 +4358,7 @@ bdrv_recurse_has_child(BlockDriverState *bs, BlockDriverState *child)
* returns a pointer to bs_queue, which is either the newly allocated
* bs_queue, or the existing bs_queue being used.
*
- * bs is drained here and undrained by bdrv_reopen_queue_free().
+ * bs must be drained.
*/
static BlockReopenQueue * GRAPH_RDLOCK
bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs,
@@ -4377,12 +4377,7 @@ bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs,
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);
+ assert(bs->quiesce_counter > 0);
if (bs_queue == NULL) {
bs_queue = g_new0(BlockReopenQueue, 1);
@@ -4522,6 +4517,12 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
QDict *options, bool keep_old_opts)
{
GLOBAL_STATE_CODE();
+
+ if (bs_queue == NULL) {
+ /* Paired with bdrv_drain_all_end() in bdrv_reopen_queue_free(). */
+ bdrv_drain_all_begin();
+ }
+
GRAPH_RDLOCK_GUARD_MAINLOOP();
return bdrv_reopen_queue_child(bs_queue, bs, options, NULL, 0, false,
@@ -4534,12 +4535,14 @@ void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue)
if (bs_queue) {
BlockReopenQueueEntry *bs_entry, *next;
QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
- bdrv_drained_end(bs_entry->state.bs);
qobject_unref(bs_entry->state.explicit_options);
qobject_unref(bs_entry->state.options);
g_free(bs_entry);
}
g_free(bs_queue);
+
+ /* Paired with bdrv_drain_all_begin() in bdrv_reopen_queue(). */
+ bdrv_drain_all_end();
}
}
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 03/48] block/snapshot: move drain outside of read-locked bdrv_snapshot_delete()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 01/48] block: remove outdated comments about AioContext locking Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 02/48] block: move drain outside of read-locked bdrv_reopen_queue_child() Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 04/48] block: move drain outside of read-locked bdrv_inactivate_recurse() Fiona Ebner
` (46 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is in preparation to mark bdrv_drained_begin() as GRAPH_UNLOCKED.
More granular draining is not trivially possible, because
bdrv_snapshot_delete() can recursively call itself.
The return value of bdrv_all_delete_snapshot() changes from -1 to
-errno propagated from failed sub-calls. This is fine for the existing
callers of bdrv_all_delete_snapshot().
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/snapshot.c | 26 +++++++++++++++-----------
blockdev.c | 25 +++++++++++++++++--------
qemu-img.c | 2 ++
3 files changed, 34 insertions(+), 19 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 22567f1fb9..9f300a78bd 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -327,7 +327,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
/**
* Delete an internal snapshot by @snapshot_id and @name.
- * @bs: block device used in the operation
+ * @bs: block device used in the operation, must be drained
* @snapshot_id: unique snapshot ID, or NULL
* @name: snapshot name, or NULL
* @errp: location to store error
@@ -358,6 +358,8 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
GLOBAL_STATE_CODE();
+ assert(bs->quiesce_counter > 0);
+
if (!drv) {
error_setg(errp, "Device '%s' has no medium",
bdrv_get_device_name(bs));
@@ -368,9 +370,6 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
return -EINVAL;
}
- /* drain all pending i/o before deleting snapshot */
- bdrv_drained_begin(bs);
-
if (drv->bdrv_snapshot_delete) {
ret = drv->bdrv_snapshot_delete(bs, snapshot_id, name, errp);
} else if (fallback_bs) {
@@ -382,7 +381,6 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
ret = -ENOTSUP;
}
- bdrv_drained_end(bs);
return ret;
}
@@ -571,19 +569,22 @@ int bdrv_all_delete_snapshot(const char *name,
ERRP_GUARD();
g_autoptr(GList) bdrvs = NULL;
GList *iterbdrvs;
+ int ret = 0;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
- if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
- return -1;
+ bdrv_drain_all_begin();
+ bdrv_graph_rdlock_main_loop();
+
+ ret = bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp);
+ if (ret < 0) {
+ goto out;
}
iterbdrvs = bdrvs;
while (iterbdrvs) {
BlockDriverState *bs = iterbdrvs->data;
QEMUSnapshotInfo sn1, *snapshot = &sn1;
- int ret = 0;
if ((devices || bdrv_all_snapshots_includes_bs(bs)) &&
bdrv_snapshot_find(bs, snapshot, name) >= 0)
@@ -594,13 +595,16 @@ int bdrv_all_delete_snapshot(const char *name,
if (ret < 0) {
error_prepend(errp, "Could not delete snapshot '%s' on '%s': ",
name, bdrv_get_device_or_node_name(bs));
- return -1;
+ goto out;
}
iterbdrvs = iterbdrvs->next;
}
- return 0;
+out:
+ bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_end();
+ return ret;
}
diff --git a/blockdev.c b/blockdev.c
index 21443b4514..3982f9776b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1132,39 +1132,41 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
int ret;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+
+ bdrv_drain_all_begin();
+ bdrv_graph_rdlock_main_loop();
bs = qmp_get_root_bs(device, errp);
if (!bs) {
- return NULL;
+ goto error;
}
if (!id && !name) {
error_setg(errp, "Name or id must be provided");
- return NULL;
+ goto error;
}
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, errp)) {
- return NULL;
+ goto error;
}
ret = bdrv_snapshot_find_by_id_and_name(bs, id, name, &sn, &local_err);
if (local_err) {
error_propagate(errp, local_err);
- return NULL;
+ goto error;
}
if (!ret) {
error_setg(errp,
"Snapshot with id '%s' and name '%s' does not exist on "
"device '%s'",
STR_OR_NULL(id), STR_OR_NULL(name), device);
- return NULL;
+ goto error;
}
bdrv_snapshot_delete(bs, id, name, &local_err);
if (local_err) {
error_propagate(errp, local_err);
- return NULL;
+ goto error;
}
info = g_new0(SnapshotInfo, 1);
@@ -1180,6 +1182,9 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
info->has_icount = true;
}
+error:
+ bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_end();
return info;
}
@@ -1295,12 +1300,14 @@ static void internal_snapshot_abort(void *opaque)
Error *local_error = NULL;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
if (!state->created) {
return;
}
+ bdrv_drain_all_begin();
+ bdrv_graph_rdlock_main_loop();
+
if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) {
error_reportf_err(local_error,
"Failed to delete snapshot with id '%s' and "
@@ -1308,6 +1315,8 @@ static void internal_snapshot_abort(void *opaque)
sn->id_str, sn->name,
bdrv_get_device_name(bs));
}
+ bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_end();
}
static void internal_snapshot_clean(void *opaque)
diff --git a/qemu-img.c b/qemu-img.c
index 139eeb5039..e75707180d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3505,6 +3505,7 @@ static int img_snapshot(int argc, char **argv)
break;
case SNAPSHOT_DELETE:
+ bdrv_drain_all_begin();
bdrv_graph_rdlock_main_loop();
ret = bdrv_snapshot_find(bs, &sn, snapshot_name);
if (ret < 0) {
@@ -3520,6 +3521,7 @@ static int img_snapshot(int argc, char **argv)
}
}
bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_end();
break;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 04/48] block: move drain outside of read-locked bdrv_inactivate_recurse()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (2 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 03/48] block/snapshot: move drain outside of read-locked bdrv_snapshot_delete() Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 05/48] block: mark bdrv_parent_change_aio_context() GRAPH_RDLOCK Fiona Ebner
` (45 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is in preparation to mark bdrv_drained_begin() as GRAPH_UNLOCKED.
More granular draining is not trivially possible, because
bdrv_inactivate_recurse() can recursively call itself.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index 3065d5c91e..fa55dfba68 100644
--- a/block.c
+++ b/block.c
@@ -6989,6 +6989,8 @@ bdrv_inactivate_recurse(BlockDriverState *bs, bool top_level)
GLOBAL_STATE_CODE();
+ assert(bs->quiesce_counter > 0);
+
if (!bs->drv) {
return -ENOMEDIUM;
}
@@ -7032,9 +7034,7 @@ bdrv_inactivate_recurse(BlockDriverState *bs, bool top_level)
return -EPERM;
}
- bdrv_drained_begin(bs);
bs->open_flags |= BDRV_O_INACTIVE;
- bdrv_drained_end(bs);
/*
* Update permissions, they may differ for inactive nodes.
@@ -7059,20 +7059,26 @@ int bdrv_inactivate(BlockDriverState *bs, Error **errp)
int ret;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+
+ bdrv_drain_all_begin();
+ bdrv_graph_rdlock_main_loop();
if (bdrv_has_bds_parent(bs, true)) {
error_setg(errp, "Node has active parent node");
- return -EPERM;
+ ret = -EPERM;
+ goto out;
}
ret = bdrv_inactivate_recurse(bs, true);
if (ret < 0) {
error_setg_errno(errp, -ret, "Failed to inactivate node");
- return ret;
+ goto out;
}
- return 0;
+out:
+ bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_end();
+ return ret;
}
int bdrv_inactivate_all(void)
@@ -7082,7 +7088,9 @@ int bdrv_inactivate_all(void)
int ret = 0;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+
+ bdrv_drain_all_begin();
+ bdrv_graph_rdlock_main_loop();
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
/* Nodes with BDS parents are covered by recursion from the last
@@ -7098,6 +7106,9 @@ int bdrv_inactivate_all(void)
}
}
+ bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_end();
+
return ret;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 05/48] block: mark bdrv_parent_change_aio_context() GRAPH_RDLOCK
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (3 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 04/48] block: move drain outside of read-locked bdrv_inactivate_recurse() Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 06/48] block: mark change_aio_ctx() callback and instances as GRAPH_RDLOCK(_PTR) Fiona Ebner
` (44 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is a small step in preparation to mark bdrv_drained_begin() as
GRAPH_UNLOCKED. More concretely, it allows marking the
change_aio_ctx() callback GRAPH_RDLOCK_PTR, which is the next step.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index fa55dfba68..7207978e53 100644
--- a/block.c
+++ b/block.c
@@ -7575,10 +7575,10 @@ typedef struct BdrvStateSetAioContext {
BlockDriverState *bs;
} BdrvStateSetAioContext;
-static bool bdrv_parent_change_aio_context(BdrvChild *c, AioContext *ctx,
- GHashTable *visited,
- Transaction *tran,
- Error **errp)
+static bool GRAPH_RDLOCK
+bdrv_parent_change_aio_context(BdrvChild *c, AioContext *ctx,
+ GHashTable *visited, Transaction *tran,
+ Error **errp)
{
GLOBAL_STATE_CODE();
if (g_hash_table_contains(visited, c)) {
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 06/48] block: mark change_aio_ctx() callback and instances as GRAPH_RDLOCK(_PTR)
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (4 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 05/48] block: mark bdrv_parent_change_aio_context() GRAPH_RDLOCK Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 07/48] block: mark bdrv_child_change_aio_context() GRAPH_RDLOCK Fiona Ebner
` (43 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is a small step in preparation to mark bdrv_drained_begin() as
GRAPH_UNLOCKED. More concretely, it is in preparation to move the
drain out of bdrv_change_aio_context() and marking that function as
GRAPH_RDLOCK.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 7 ++++---
block/block-backend.c | 6 +++---
blockjob.c | 6 +++---
include/block/block_int-common.h | 6 +++---
4 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/block.c b/block.c
index 7207978e53..01144c895e 100644
--- a/block.c
+++ b/block.c
@@ -1226,9 +1226,10 @@ static int bdrv_child_cb_inactivate(BdrvChild *child)
return 0;
}
-static bool bdrv_child_cb_change_aio_ctx(BdrvChild *child, AioContext *ctx,
- GHashTable *visited, Transaction *tran,
- Error **errp)
+static bool GRAPH_RDLOCK
+bdrv_child_cb_change_aio_ctx(BdrvChild *child, AioContext *ctx,
+ GHashTable *visited, Transaction *tran,
+ Error **errp)
{
BlockDriverState *bs = child->opaque;
return bdrv_change_aio_context(bs, ctx, visited, tran, errp);
diff --git a/block/block-backend.c b/block/block-backend.c
index a402db13f2..6a6949edeb 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -136,9 +136,9 @@ static void blk_root_drained_end(BdrvChild *child);
static void blk_root_change_media(BdrvChild *child, bool load);
static void blk_root_resize(BdrvChild *child);
-static bool blk_root_change_aio_ctx(BdrvChild *child, AioContext *ctx,
- GHashTable *visited, Transaction *tran,
- Error **errp);
+static bool GRAPH_RDLOCK
+blk_root_change_aio_ctx(BdrvChild *child, AioContext *ctx, GHashTable *visited,
+ Transaction *tran, Error **errp);
static char *blk_root_get_parent_desc(BdrvChild *child)
{
diff --git a/blockjob.c b/blockjob.c
index 32007f31a9..34185d7715 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -144,9 +144,9 @@ static TransactionActionDrv change_child_job_context = {
.clean = g_free,
};
-static bool child_job_change_aio_ctx(BdrvChild *c, AioContext *ctx,
- GHashTable *visited, Transaction *tran,
- Error **errp)
+static bool GRAPH_RDLOCK
+child_job_change_aio_ctx(BdrvChild *c, AioContext *ctx, GHashTable *visited,
+ Transaction *tran, Error **errp)
{
BlockJob *job = c->opaque;
BdrvStateChildJobContext *s;
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 2982dd3118..37466c7841 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -983,9 +983,9 @@ struct BdrvChildClass {
bool backing_mask_protocol,
Error **errp);
- bool (*change_aio_ctx)(BdrvChild *child, AioContext *ctx,
- GHashTable *visited, Transaction *tran,
- Error **errp);
+ bool GRAPH_RDLOCK_PTR (*change_aio_ctx)(BdrvChild *child, AioContext *ctx,
+ GHashTable *visited,
+ Transaction *tran, Error **errp);
/*
* I/O API functions. These functions are thread-safe.
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 07/48] block: mark bdrv_child_change_aio_context() GRAPH_RDLOCK
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (5 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 06/48] block: mark change_aio_ctx() callback and instances as GRAPH_RDLOCK(_PTR) Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 08/48] block: move drain outside of bdrv_change_aio_context() and mark GRAPH_RDLOCK Fiona Ebner
` (42 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is a small step in preparation to mark bdrv_drained_begin() as
GRAPH_UNLOCKED. More concretely, it is in preparation to move the
drain out of bdrv_change_aio_context() and marking that function as
GRAPH_RDLOCK.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-global-state.h | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 9be34b3c99..aad160956a 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -274,9 +274,10 @@ int bdrv_debug_remove_breakpoint(BlockDriverState *bs, const char *tag);
int bdrv_debug_resume(BlockDriverState *bs, const char *tag);
bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag);
-bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
- GHashTable *visited, Transaction *tran,
- Error **errp);
+bool GRAPH_RDLOCK
+bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
+ GHashTable *visited, Transaction *tran,
+ Error **errp);
int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
BdrvChild *ignore_child, Error **errp);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 08/48] block: move drain outside of bdrv_change_aio_context() and mark GRAPH_RDLOCK
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (6 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 07/48] block: mark bdrv_child_change_aio_context() GRAPH_RDLOCK Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 09/48] block: move drain outside of bdrv_try_change_aio_context() Fiona Ebner
` (41 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is in preparation to mark bdrv_drained_begin() as GRAPH_UNLOCKED.
Note that even if bdrv_drained_begin() were already marked as
GRAPH_UNLOCKED, TSA would not complain about the instance in
bdrv_change_aio_context() before this change, because it is preceded
by a bdrv_graph_rdunlock_main_loop() call. It is not correct to
release the lock here, and in case the caller holds a write lock, it
wouldn't actually release the lock.
In combination with block-stream, there is a deadlock that can happen
because of this [0]. In particular, it can happen that
main thread IO thread
1. acquires write lock
in blk_co_do_preadv_part():
2. have non-zero blk->in_flight
3. try to acquire read lock
4. begin drain
Steps 3 and 4 might be switched. Draining will poll and get stuck,
because it will see the non-zero in_flight counter. But the IO thread
will not make any progress either, because it cannot acquire the read
lock.
After this change, all paths to bdrv_change_aio_context() drain:
bdrv_change_aio_context() is called by:
1. bdrv_child_cb_change_aio_ctx() which is only called via the
change_aio_ctx() callback, see below.
2. bdrv_child_change_aio_context(), see below.
3. bdrv_try_change_aio_context(), where a drained section is
introduced.
The change_aio_ctx() callback is called by:
1. bdrv_attach_child_common_abort(), where a drained section is
introduced.
2. bdrv_attach_child_common(), where a drained section is introduced.
3. bdrv_parent_change_aio_context(), see below.
bdrv_child_change_aio_context() is called by:
1. bdrv_change_aio_context(), i.e. recursive, so being in a drained
section is invariant.
2. child_job_change_aio_ctx(), which is only called via the
change_aio_ctx() callback, see above.
bdrv_parent_change_aio_context() is called by:
1. bdrv_change_aio_context(), i.e. recursive, so being in a drained
section is invariant.
This resolves all code paths. Note that bdrv_attach_child_common()
and bdrv_attach_child_common_abort() hold the graph write lock and
callers of bdrv_try_change_aio_context() might too, so they are not
actually allowed to drain either. This will be addressed in the
following commits.
More granular draining is not trivially possible, because
bdrv_change_aio_context() can recursively call itself e.g. via
bdrv_child_change_aio_context().
[0]: https://lore.kernel.org/qemu-devel/73839c04-7616-407e-b057-80ca69e63f51@virtuozzo.com/
Reported-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 57 +++++++++++++++++++++++---------
include/block/block_int-common.h | 12 +++++++
2 files changed, 53 insertions(+), 16 deletions(-)
diff --git a/block.c b/block.c
index 01144c895e..6f42c0f1ab 100644
--- a/block.c
+++ b/block.c
@@ -106,9 +106,9 @@ static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
static bool bdrv_backing_overridden(BlockDriverState *bs);
-static bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
- GHashTable *visited, Transaction *tran,
- Error **errp);
+static bool GRAPH_RDLOCK
+bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
+ GHashTable *visited, Transaction *tran, Error **errp);
/* If non-zero, use only whitelisted block drivers */
static int use_bdrv_whitelist;
@@ -3040,8 +3040,10 @@ static void GRAPH_WRLOCK bdrv_attach_child_common_abort(void *opaque)
/* No need to visit `child`, because it has been detached already */
visited = g_hash_table_new(NULL, NULL);
+ bdrv_drain_all_begin();
ret = s->child->klass->change_aio_ctx(s->child, s->old_parent_ctx,
visited, tran, &error_abort);
+ bdrv_drain_all_end();
g_hash_table_destroy(visited);
/* transaction is supposed to always succeed */
@@ -3122,9 +3124,11 @@ bdrv_attach_child_common(BlockDriverState *child_bs,
bool ret_child;
g_hash_table_add(visited, new_child);
+ bdrv_drain_all_begin();
ret_child = child_class->change_aio_ctx(new_child, child_ctx,
visited, aio_ctx_tran,
NULL);
+ bdrv_drain_all_end();
if (ret_child == true) {
error_free(local_err);
ret = 0;
@@ -7576,6 +7580,17 @@ typedef struct BdrvStateSetAioContext {
BlockDriverState *bs;
} BdrvStateSetAioContext;
+/*
+ * Changes the AioContext of @child to @ctx and recursively for the associated
+ * block nodes and all their children and parents. Returns true if the change is
+ * possible and the transaction @tran can be continued. Returns false and sets
+ * @errp if not and the transaction must be aborted.
+ *
+ * @visited will accumulate all visited BdrvChild objects. The caller is
+ * responsible for freeing the list afterwards.
+ *
+ * Must be called with the affected block nodes drained.
+ */
static bool GRAPH_RDLOCK
bdrv_parent_change_aio_context(BdrvChild *c, AioContext *ctx,
GHashTable *visited, Transaction *tran,
@@ -7604,6 +7619,17 @@ bdrv_parent_change_aio_context(BdrvChild *c, AioContext *ctx,
return true;
}
+/*
+ * Changes the AioContext of @c->bs to @ctx and recursively for all its children
+ * and parents. Returns true if the change is possible and the transaction @tran
+ * can be continued. Returns false and sets @errp if not and the transaction
+ * must be aborted.
+ *
+ * @visited will accumulate all visited BdrvChild objects. The caller is
+ * responsible for freeing the list afterwards.
+ *
+ * Must be called with the affected block nodes drained.
+ */
bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
GHashTable *visited, Transaction *tran,
Error **errp)
@@ -7619,10 +7645,6 @@ bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
static void bdrv_set_aio_context_clean(void *opaque)
{
BdrvStateSetAioContext *state = (BdrvStateSetAioContext *) opaque;
- BlockDriverState *bs = (BlockDriverState *) state->bs;
-
- /* Paired with bdrv_drained_begin in bdrv_change_aio_context() */
- bdrv_drained_end(bs);
g_free(state);
}
@@ -7650,10 +7672,12 @@ static TransactionActionDrv set_aio_context = {
*
* @visited will accumulate all visited BdrvChild objects. The caller is
* responsible for freeing the list afterwards.
+ *
+ * @bs must be drained.
*/
-static bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
- GHashTable *visited, Transaction *tran,
- Error **errp)
+static bool GRAPH_RDLOCK
+bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
+ GHashTable *visited, Transaction *tran, Error **errp)
{
BdrvChild *c;
BdrvStateSetAioContext *state;
@@ -7664,21 +7688,17 @@ static bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
return true;
}
- bdrv_graph_rdlock_main_loop();
QLIST_FOREACH(c, &bs->parents, next_parent) {
if (!bdrv_parent_change_aio_context(c, ctx, visited, tran, errp)) {
- bdrv_graph_rdunlock_main_loop();
return false;
}
}
QLIST_FOREACH(c, &bs->children, next) {
if (!bdrv_child_change_aio_context(c, ctx, visited, tran, errp)) {
- bdrv_graph_rdunlock_main_loop();
return false;
}
}
- bdrv_graph_rdunlock_main_loop();
state = g_new(BdrvStateSetAioContext, 1);
*state = (BdrvStateSetAioContext) {
@@ -7686,8 +7706,7 @@ static bool bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
.bs = bs,
};
- /* Paired with bdrv_drained_end in bdrv_set_aio_context_clean() */
- bdrv_drained_begin(bs);
+ assert(bs->quiesce_counter > 0);
tran_add(tran, &set_aio_context, state);
@@ -7720,6 +7739,8 @@ int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
if (ignore_child) {
g_hash_table_add(visited, ignore_child);
}
+ bdrv_drain_all_begin();
+ bdrv_graph_rdlock_main_loop();
ret = bdrv_change_aio_context(bs, ctx, visited, tran, errp);
g_hash_table_destroy(visited);
@@ -7733,10 +7754,14 @@ int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
if (!ret) {
/* Just run clean() callbacks. No AioContext changed. */
tran_abort(tran);
+ bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_end();
return -EPERM;
}
tran_commit(tran);
+ bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_end();
return 0;
}
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 37466c7841..168f703fa1 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -983,6 +983,18 @@ struct BdrvChildClass {
bool backing_mask_protocol,
Error **errp);
+ /*
+ * Notifies the parent that the child is trying to change its AioContext.
+ * The parent may in turn change the AioContext of other nodes in the same
+ * transaction. Returns true if the change is possible and the transaction
+ * can be continued. Returns false and sets @errp if not and the transaction
+ * must be aborted.
+ *
+ * @visited will accumulate all visited BdrvChild objects. The caller is
+ * responsible for freeing the list afterwards.
+ *
+ * Must be called with the affected block nodes drained.
+ */
bool GRAPH_RDLOCK_PTR (*change_aio_ctx)(BdrvChild *child, AioContext *ctx,
GHashTable *visited,
Transaction *tran, Error **errp);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 09/48] block: move drain outside of bdrv_try_change_aio_context()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (7 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 08/48] block: move drain outside of bdrv_change_aio_context() and mark GRAPH_RDLOCK Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 10/48] block: move drain outside of bdrv_attach_child_common(_abort)() Fiona Ebner
` (40 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is part of resolving the deadlock mentioned in commit "block:
move draining out of bdrv_change_aio_context() and mark GRAPH_RDLOCK".
Convert the function to a _locked() version that has to be called with
the graph lock held and add a convenience wrapper that has to be
called with the graph unlocked, which drains and takes the lock
itself. Since bdrv_try_change_aio_context() is global state code, the
wrapper is too.
Callers are adapted to use the appropriate variant, depending on
whether the caller already holds the lock. In the
test_set_aio_context() unit test, prior drains can be removed, because
draining already happens inside the new wrapper.
Note that bdrv_attach_child_common_abort(), bdrv_attach_child_common()
and bdrv_root_unref_child() hold the graph lock and are not actually
allowed to drain either. This will be addressed in the following
commits.
Functions like qmp_blockdev_mirror() query the nodes to act on before
draining and locking. In theory, draining could invalidate those nodes.
This kind of issue is not addressed by these commits.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 58 ++++++++++++++++++++++--------
blockdev.c | 15 +++++---
include/block/block-global-state.h | 8 +++--
tests/unit/test-bdrv-drain.c | 4 ---
4 files changed, 59 insertions(+), 26 deletions(-)
diff --git a/block.c b/block.c
index 6f42c0f1ab..3aaacabf7f 100644
--- a/block.c
+++ b/block.c
@@ -3028,7 +3028,10 @@ static void GRAPH_WRLOCK bdrv_attach_child_common_abort(void *opaque)
bdrv_replace_child_noperm(s->child, NULL);
if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
- bdrv_try_change_aio_context(bs, s->old_child_ctx, NULL, &error_abort);
+ bdrv_drain_all_begin();
+ bdrv_try_change_aio_context_locked(bs, s->old_child_ctx, NULL,
+ &error_abort);
+ bdrv_drain_all_end();
}
if (bdrv_child_get_parent_aio_context(s->child) != s->old_parent_ctx) {
@@ -3115,8 +3118,10 @@ bdrv_attach_child_common(BlockDriverState *child_bs,
parent_ctx = bdrv_child_get_parent_aio_context(new_child);
if (child_ctx != parent_ctx) {
Error *local_err = NULL;
- int ret = bdrv_try_change_aio_context(child_bs, parent_ctx, NULL,
- &local_err);
+ bdrv_drain_all_begin();
+ int ret = bdrv_try_change_aio_context_locked(child_bs, parent_ctx, NULL,
+ &local_err);
+ bdrv_drain_all_end();
if (ret < 0 && child_class->change_aio_ctx) {
Transaction *aio_ctx_tran = tran_new();
@@ -3319,8 +3324,10 @@ void bdrv_root_unref_child(BdrvChild *child)
* When the parent requiring a non-default AioContext is removed, the
* node moves back to the main AioContext
*/
- bdrv_try_change_aio_context(child_bs, qemu_get_aio_context(), NULL,
- NULL);
+ bdrv_drain_all_begin();
+ bdrv_try_change_aio_context_locked(child_bs, qemu_get_aio_context(),
+ NULL, NULL);
+ bdrv_drain_all_end();
}
bdrv_schedule_unref(child_bs);
@@ -7719,9 +7726,13 @@ bdrv_change_aio_context(BlockDriverState *bs, AioContext *ctx,
*
* If ignore_child is not NULL, that child (and its subgraph) will not
* be touched.
+ *
+ * Called with the graph lock held.
+ *
+ * Called while all bs are drained.
*/
-int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
- BdrvChild *ignore_child, Error **errp)
+int bdrv_try_change_aio_context_locked(BlockDriverState *bs, AioContext *ctx,
+ BdrvChild *ignore_child, Error **errp)
{
Transaction *tran;
GHashTable *visited;
@@ -7730,17 +7741,15 @@ int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
/*
* Recursion phase: go through all nodes of the graph.
- * Take care of checking that all nodes support changing AioContext
- * and drain them, building a linear list of callbacks to run if everything
- * is successful (the transaction itself).
+ * Take care of checking that all nodes support changing AioContext,
+ * building a linear list of callbacks to run if everything is successful
+ * (the transaction itself).
*/
tran = tran_new();
visited = g_hash_table_new(NULL, NULL);
if (ignore_child) {
g_hash_table_add(visited, ignore_child);
}
- bdrv_drain_all_begin();
- bdrv_graph_rdlock_main_loop();
ret = bdrv_change_aio_context(bs, ctx, visited, tran, errp);
g_hash_table_destroy(visited);
@@ -7754,15 +7763,34 @@ int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
if (!ret) {
/* Just run clean() callbacks. No AioContext changed. */
tran_abort(tran);
- bdrv_graph_rdunlock_main_loop();
- bdrv_drain_all_end();
return -EPERM;
}
tran_commit(tran);
+ return 0;
+}
+
+/*
+ * Change bs's and recursively all of its parents' and children's AioContext
+ * to the given new context, returning an error if that isn't possible.
+ *
+ * If ignore_child is not NULL, that child (and its subgraph) will not
+ * be touched.
+ */
+int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
+ BdrvChild *ignore_child, Error **errp)
+{
+ int ret;
+
+ GLOBAL_STATE_CODE();
+
+ bdrv_drain_all_begin();
+ bdrv_graph_rdlock_main_loop();
+ ret = bdrv_try_change_aio_context_locked(bs, ctx, ignore_child, errp);
bdrv_graph_rdunlock_main_loop();
bdrv_drain_all_end();
- return 0;
+
+ return ret;
}
void bdrv_add_aio_context_notifier(BlockDriverState *bs,
diff --git a/blockdev.c b/blockdev.c
index 3982f9776b..750beba41f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3601,12 +3601,13 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
AioContext *new_context;
BlockDriverState *bs;
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+ bdrv_drain_all_begin();
+ bdrv_graph_rdlock_main_loop();
bs = bdrv_find_node(node_name);
if (!bs) {
error_setg(errp, "Failed to find node with node-name='%s'", node_name);
- return;
+ goto out;
}
/* Protects against accidents. */
@@ -3614,14 +3615,14 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
error_setg(errp, "Node %s is associated with a BlockBackend and could "
"be in use (use force=true to override this check)",
node_name);
- return;
+ goto out;
}
if (iothread->type == QTYPE_QSTRING) {
IOThread *obj = iothread_by_id(iothread->u.s);
if (!obj) {
error_setg(errp, "Cannot find iothread %s", iothread->u.s);
- return;
+ goto out;
}
new_context = iothread_get_aio_context(obj);
@@ -3629,7 +3630,11 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
new_context = qemu_get_aio_context();
}
- bdrv_try_change_aio_context(bs, new_context, NULL, errp);
+ bdrv_try_change_aio_context_locked(bs, new_context, NULL, errp);
+
+out:
+ bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_end();
}
QemuOptsList qemu_common_drive_opts = {
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index aad160956a..91f249b5ad 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -278,8 +278,12 @@ bool GRAPH_RDLOCK
bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
GHashTable *visited, Transaction *tran,
Error **errp);
-int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
- BdrvChild *ignore_child, Error **errp);
+int GRAPH_UNLOCKED
+bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
+ BdrvChild *ignore_child, Error **errp);
+int GRAPH_RDLOCK
+bdrv_try_change_aio_context_locked(BlockDriverState *bs, AioContext *ctx,
+ BdrvChild *ignore_child, Error **errp);
int GRAPH_RDLOCK bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz);
int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo);
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 290cd2a70e..3185f3f429 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -1396,14 +1396,10 @@ static void test_set_aio_context(void)
bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR,
&error_abort);
- bdrv_drained_begin(bs);
bdrv_try_change_aio_context(bs, ctx_a, NULL, &error_abort);
- bdrv_drained_end(bs);
- bdrv_drained_begin(bs);
bdrv_try_change_aio_context(bs, ctx_b, NULL, &error_abort);
bdrv_try_change_aio_context(bs, qemu_get_aio_context(), NULL, &error_abort);
- bdrv_drained_end(bs);
bdrv_unref(bs);
iothread_join(a);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 10/48] block: move drain outside of bdrv_attach_child_common(_abort)()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (8 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 09/48] block: move drain outside of bdrv_try_change_aio_context() Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 11/48] block: move drain outside of bdrv_set_backing_hd_drained() Fiona Ebner
` (39 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is part of resolving the deadlock mentioned in commit "block:
move draining out of bdrv_change_aio_context() and mark GRAPH_RDLOCK".
The function bdrv_attach_child_common_abort() is used only as the
abort callback in bdrv_attach_child_common_drv transactions, so the
tran_finalize() calls of such transactions need to be in drained
sections too.
All code paths are covered:
The bdrv_attach_child_common_drv transactions are only used in
bdrv_attach_child_common(), so it is enough to check callers of
bdrv_attach_child_common() following the transactions.
bdrv_attach_child_common() is called by:
1. bdrv_attach_child_noperm(), which does not finalize the
transaction yet.
2. bdrv_root_attach_child(), where a drained section is introduced.
bdrv_attach_child_noperm() is called by:
1. bdrv_attach_child(), where a drained section is introduced.
2. bdrv_set_file_or_backing_noperm(), which does not finalize the
transaction yet.
3. bdrv_append(), where a drained section is introduced.
bdrv_set_file_or_backing_noperm() is called by:
1. bdrv_set_backing_hd_drained(), where a drained section is
introduced.
2. bdrv_reopen_parse_file_or_backing(), which does not finalize the
transaction yet. Draining the old child bs currently happens under
the graph lock there. This is replaced with an assertion, because
the drain will be moved further up to the caller.
bdrv_reopen_parse_file_or_backing() is called by:
1. bdrv_reopen_prepare(), which does not finalize the transaction yet.
bdrv_reopen_prepare() is called by:
1. bdrv_reopen_multiple(), which does finalize the transaction. It is
called after bdrv_reopen_queue(), which starts a drained section.
The drained section ends, when bdrv_reopen_queue_free() is called
at the end of bdrv_reopen_multiple().
This resolves all code paths.
The functions bdrv_set_backing_hd_drained(), bdrv_attach_child() and
bdrv_root_attach_child() run under the graph lock, so they are not
actually allowed to drain. This will be addressed in the following
commits.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
Changes in v4:
* Where applicable, document requirement to drain all nodes from
before calling the function until finalizing the transaction.
block.c | 40 ++++++++++++++++++++++++----------------
1 file changed, 24 insertions(+), 16 deletions(-)
diff --git a/block.c b/block.c
index 3aaacabf7f..46eb2fe449 100644
--- a/block.c
+++ b/block.c
@@ -3028,10 +3028,8 @@ static void GRAPH_WRLOCK bdrv_attach_child_common_abort(void *opaque)
bdrv_replace_child_noperm(s->child, NULL);
if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
- bdrv_drain_all_begin();
bdrv_try_change_aio_context_locked(bs, s->old_child_ctx, NULL,
&error_abort);
- bdrv_drain_all_end();
}
if (bdrv_child_get_parent_aio_context(s->child) != s->old_parent_ctx) {
@@ -3043,10 +3041,8 @@ static void GRAPH_WRLOCK bdrv_attach_child_common_abort(void *opaque)
/* No need to visit `child`, because it has been detached already */
visited = g_hash_table_new(NULL, NULL);
- bdrv_drain_all_begin();
ret = s->child->klass->change_aio_ctx(s->child, s->old_parent_ctx,
visited, tran, &error_abort);
- bdrv_drain_all_end();
g_hash_table_destroy(visited);
/* transaction is supposed to always succeed */
@@ -3075,6 +3071,9 @@ static TransactionActionDrv bdrv_attach_child_common_drv = {
*
* Both @parent_bs and @child_bs can move to a different AioContext in this
* function.
+ *
+ * All block nodes must be drained before this function is called until after
+ * the transaction is finalized.
*/
static BdrvChild * GRAPH_WRLOCK
bdrv_attach_child_common(BlockDriverState *child_bs,
@@ -3118,10 +3117,8 @@ bdrv_attach_child_common(BlockDriverState *child_bs,
parent_ctx = bdrv_child_get_parent_aio_context(new_child);
if (child_ctx != parent_ctx) {
Error *local_err = NULL;
- bdrv_drain_all_begin();
int ret = bdrv_try_change_aio_context_locked(child_bs, parent_ctx, NULL,
&local_err);
- bdrv_drain_all_end();
if (ret < 0 && child_class->change_aio_ctx) {
Transaction *aio_ctx_tran = tran_new();
@@ -3129,11 +3126,9 @@ bdrv_attach_child_common(BlockDriverState *child_bs,
bool ret_child;
g_hash_table_add(visited, new_child);
- bdrv_drain_all_begin();
ret_child = child_class->change_aio_ctx(new_child, child_ctx,
visited, aio_ctx_tran,
NULL);
- bdrv_drain_all_end();
if (ret_child == true) {
error_free(local_err);
ret = 0;
@@ -3189,6 +3184,9 @@ bdrv_attach_child_common(BlockDriverState *child_bs,
*
* After calling this function, the transaction @tran may only be completed
* while holding a writer lock for the graph.
+ *
+ * All block nodes must be drained before this function is called until after
+ * the transaction is finalized.
*/
static BdrvChild * GRAPH_WRLOCK
bdrv_attach_child_noperm(BlockDriverState *parent_bs,
@@ -3244,6 +3242,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
GLOBAL_STATE_CODE();
+ bdrv_drain_all_begin();
child = bdrv_attach_child_common(child_bs, child_name, child_class,
child_role, perm, shared_perm, opaque,
tran, errp);
@@ -3256,6 +3255,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
out:
tran_finalize(tran, ret);
+ bdrv_drain_all_end();
bdrv_schedule_unref(child_bs);
@@ -3283,6 +3283,7 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
GLOBAL_STATE_CODE();
+ bdrv_drain_all_begin();
child = bdrv_attach_child_noperm(parent_bs, child_bs, child_name,
child_class, child_role, tran, errp);
if (!child) {
@@ -3297,6 +3298,7 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
out:
tran_finalize(tran, ret);
+ bdrv_drain_all_end();
bdrv_schedule_unref(child_bs);
@@ -3465,6 +3467,9 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)
*
* After calling this function, the transaction @tran may only be completed
* while holding a writer lock for the graph.
+ *
+ * All block nodes must be drained before this function is called until after
+ * the transaction is finalized.
*/
static int GRAPH_WRLOCK
bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs,
@@ -3573,6 +3578,7 @@ int bdrv_set_backing_hd_drained(BlockDriverState *bs,
assert(bs->backing->bs->quiesce_counter > 0);
}
+ bdrv_drain_all_begin();
ret = bdrv_set_file_or_backing_noperm(bs, backing_hd, true, tran, errp);
if (ret < 0) {
goto out;
@@ -3581,6 +3587,7 @@ int bdrv_set_backing_hd_drained(BlockDriverState *bs,
ret = bdrv_refresh_perms(bs, tran, errp);
out:
tran_finalize(tran, ret);
+ bdrv_drain_all_end();
return ret;
}
@@ -4721,6 +4728,9 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
* Return 0 on success, otherwise return < 0 and set @errp.
*
* @reopen_state->bs can move to a different AioContext in this function.
+ *
+ * All block nodes must be drained before this function is called until after
+ * the transaction is finalized.
*/
static int GRAPH_UNLOCKED
bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
@@ -4814,7 +4824,7 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
if (old_child_bs) {
bdrv_ref(old_child_bs);
- bdrv_drained_begin(old_child_bs);
+ assert(old_child_bs->quiesce_counter > 0);
}
bdrv_graph_rdunlock_main_loop();
@@ -4826,7 +4836,6 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
bdrv_graph_wrunlock();
if (old_child_bs) {
- bdrv_drained_end(old_child_bs);
bdrv_unref(old_child_bs);
}
@@ -4855,6 +4864,9 @@ out_rdlock:
*
* After calling this function, the transaction @change_child_tran may only be
* completed while holding a writer lock for the graph.
+ *
+ * All block nodes must be drained before this function is called until after
+ * the transaction is finalized.
*/
static int GRAPH_UNLOCKED
bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
@@ -5501,9 +5513,7 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
assert(!bs_new->backing);
bdrv_graph_rdunlock_main_loop();
- bdrv_drained_begin(bs_top);
- bdrv_drained_begin(bs_new);
-
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
child = bdrv_attach_child_noperm(bs_new, bs_top, "backing",
@@ -5525,9 +5535,7 @@ out:
bdrv_refresh_limits(bs_top, NULL, NULL);
bdrv_graph_wrunlock();
-
- bdrv_drained_end(bs_top);
- bdrv_drained_end(bs_new);
+ bdrv_drain_all_end();
return ret;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 11/48] block: move drain outside of bdrv_set_backing_hd_drained()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (9 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 10/48] block: move drain outside of bdrv_attach_child_common(_abort)() Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 12/48] block: move drain outside of bdrv_root_attach_child() Fiona Ebner
` (38 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is part of resolving the deadlock mentioned in commit "block:
move draining out of bdrv_change_aio_context() and mark GRAPH_RDLOCK".
The function bdrv_set_backing_hd_drained() holds the graph lock, so it
is not allowed to drain. It is called by:
1. bdrv_set_backing_hd(), where a drained section is introduced,
replacing the previously present bs-specific drains.
2. stream_prepare(), where a drained section is introduced replacing
the previously present bs-specific drains.
The drain_bs variable in bdrv_set_backing_hd_drained() is now
superfluous and thus dropped.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
Changes in v4:
* Drop now superfluous drain_bs variable.
* Document requirement that all nodes need to be drained for
bdrv_set_backing_hd_drained().
block.c | 16 +++-------------
block/stream.c | 6 ++----
2 files changed, 5 insertions(+), 17 deletions(-)
diff --git a/block.c b/block.c
index 46eb2fe449..e53a88e1b6 100644
--- a/block.c
+++ b/block.c
@@ -3562,8 +3562,7 @@ out:
* Both @bs and @backing_hd can move to a different AioContext in this
* function.
*
- * If a backing child is already present (i.e. we're detaching a node), that
- * child node must be drained.
+ * All block nodes must be drained.
*/
int bdrv_set_backing_hd_drained(BlockDriverState *bs,
BlockDriverState *backing_hd,
@@ -3578,7 +3577,6 @@ int bdrv_set_backing_hd_drained(BlockDriverState *bs,
assert(bs->backing->bs->quiesce_counter > 0);
}
- bdrv_drain_all_begin();
ret = bdrv_set_file_or_backing_noperm(bs, backing_hd, true, tran, errp);
if (ret < 0) {
goto out;
@@ -3587,28 +3585,20 @@ int bdrv_set_backing_hd_drained(BlockDriverState *bs,
ret = bdrv_refresh_perms(bs, tran, errp);
out:
tran_finalize(tran, ret);
- bdrv_drain_all_end();
return ret;
}
int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
Error **errp)
{
- BlockDriverState *drain_bs;
int ret;
GLOBAL_STATE_CODE();
- bdrv_graph_rdlock_main_loop();
- drain_bs = bs->backing ? bs->backing->bs : bs;
- bdrv_graph_rdunlock_main_loop();
-
- bdrv_ref(drain_bs);
- bdrv_drained_begin(drain_bs);
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
ret = bdrv_set_backing_hd_drained(bs, backing_hd, errp);
bdrv_graph_wrunlock();
- bdrv_drained_end(drain_bs);
- bdrv_unref(drain_bs);
+ bdrv_drain_all_end();
return ret;
}
diff --git a/block/stream.c b/block/stream.c
index 999d9e56d4..6ba49cffd3 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -80,11 +80,10 @@ static int stream_prepare(Job *job)
* may end up working with the wrong base node (or it might even have gone
* away by the time we want to use it).
*/
- bdrv_drained_begin(unfiltered_bs);
if (unfiltered_bs_cow) {
bdrv_ref(unfiltered_bs_cow);
- bdrv_drained_begin(unfiltered_bs_cow);
}
+ bdrv_drain_all_begin();
bdrv_graph_rdlock_main_loop();
base = bdrv_filter_or_cow_bs(s->above_base);
@@ -123,11 +122,10 @@ static int stream_prepare(Job *job)
}
out:
+ bdrv_drain_all_end();
if (unfiltered_bs_cow) {
- bdrv_drained_end(unfiltered_bs_cow);
bdrv_unref(unfiltered_bs_cow);
}
- bdrv_drained_end(unfiltered_bs);
return ret;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 12/48] block: move drain outside of bdrv_root_attach_child()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (10 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 11/48] block: move drain outside of bdrv_set_backing_hd_drained() Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 13/48] block: move drain outside of bdrv_attach_child() Fiona Ebner
` (37 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is part of resolving the deadlock mentioned in commit "block:
move draining out of bdrv_change_aio_context() and mark GRAPH_RDLOCK".
The function bdrv_root_attach_child() runs under the graph lock, so it
is not allowed to drain. It is called by:
1. blk_insert_bs(), where a drained section is introduced.
2. block_job_add_bdrv(), which holds the graph lock itself.
block_job_add_bdrv() is called by:
1. mirror_start_job()
2. stream_start()
3. commit_start()
4. backup_job_create()
5. block_job_create()
6. In the test_blockjob_common_drain_node() unit test
In all callers, a drained section is introduced.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
Changes in v4:
* Document requirement that all nodes need to be drained for
bdrv_root_attach_child() and block_job_add_bdrv().
block.c | 4 ++--
block/backup.c | 2 ++
block/block-backend.c | 2 ++
block/commit.c | 4 ++++
block/mirror.c | 5 +++++
block/stream.c | 4 ++++
blockjob.c | 4 ++++
include/block/blockjob.h | 2 ++
tests/unit/test-bdrv-drain.c | 2 ++
9 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index e53a88e1b6..17c34dc240 100644
--- a/block.c
+++ b/block.c
@@ -3228,6 +3228,8 @@ bdrv_attach_child_noperm(BlockDriverState *parent_bs,
*
* On failure NULL is returned, errp is set and the reference to
* child_bs is also dropped.
+ *
+ * All block nodes must be drained.
*/
BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
const char *child_name,
@@ -3242,7 +3244,6 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
GLOBAL_STATE_CODE();
- bdrv_drain_all_begin();
child = bdrv_attach_child_common(child_bs, child_name, child_class,
child_role, perm, shared_perm, opaque,
tran, errp);
@@ -3255,7 +3256,6 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
out:
tran_finalize(tran, ret);
- bdrv_drain_all_end();
bdrv_schedule_unref(child_bs);
diff --git a/block/backup.c b/block/backup.c
index 0151e84395..909027c17a 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -498,10 +498,12 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
block_copy_set_speed(bcs, speed);
/* Required permissions are taken by copy-before-write filter target */
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
&error_abort);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
return &job->common;
diff --git a/block/block-backend.c b/block/block-backend.c
index 6a6949edeb..24cae3cb55 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -904,6 +904,7 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
GLOBAL_STATE_CODE();
bdrv_ref(bs);
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
if ((bs->open_flags & BDRV_O_INACTIVE) && blk_can_inactivate(blk)) {
@@ -919,6 +920,7 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
perm, shared_perm, blk, errp);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
if (blk->root == NULL) {
return -EPERM;
}
diff --git a/block/commit.c b/block/commit.c
index 7cc8c0f0df..6c4b736ff8 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -392,6 +392,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
* this is the responsibility of the interface (i.e. whoever calls
* commit_start()).
*/
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
s->base_overlay = bdrv_find_overlay(top, base);
assert(s->base_overlay);
@@ -424,18 +425,21 @@ void commit_start(const char *job_id, BlockDriverState *bs,
iter_shared_perms, errp);
if (ret < 0) {
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
goto fail;
}
}
if (bdrv_freeze_backing_chain(commit_top_bs, base, errp) < 0) {
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
goto fail;
}
s->chain_frozen = true;
ret = block_job_add_bdrv(&s->common, "base", base, 0, BLK_PERM_ALL, errp);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
if (ret < 0) {
goto fail;
diff --git a/block/mirror.c b/block/mirror.c
index c2c5099c95..6e8caf4b49 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -2014,6 +2014,7 @@ static BlockJob *mirror_start_job(
*/
bdrv_disable_dirty_bitmap(s->dirty_bitmap);
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
ret = block_job_add_bdrv(&s->common, "source", bs, 0,
BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE |
@@ -2021,6 +2022,7 @@ static BlockJob *mirror_start_job(
errp);
if (ret < 0) {
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
goto fail;
}
@@ -2066,16 +2068,19 @@ static BlockJob *mirror_start_job(
iter_shared_perms, errp);
if (ret < 0) {
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
goto fail;
}
}
if (bdrv_freeze_backing_chain(mirror_top_bs, target, errp) < 0) {
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
goto fail;
}
}
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
QTAILQ_INIT(&s->ops_in_flight);
diff --git a/block/stream.c b/block/stream.c
index 6ba49cffd3..f5441f27f4 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -371,10 +371,12 @@ void stream_start(const char *job_id, BlockDriverState *bs,
* already have our own plans. Also don't allow resize as the image size is
* queried only at the job start and then cached.
*/
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
if (block_job_add_bdrv(&s->common, "active node", bs, 0,
basic_flags | BLK_PERM_WRITE, errp)) {
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
goto fail;
}
@@ -395,10 +397,12 @@ void stream_start(const char *job_id, BlockDriverState *bs,
basic_flags, errp);
if (ret < 0) {
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
goto fail;
}
}
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
s->base_overlay = base_overlay;
s->above_base = above_base;
diff --git a/blockjob.c b/blockjob.c
index 34185d7715..44991e3ff7 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -496,6 +496,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
int ret;
GLOBAL_STATE_CODE();
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
if (job_id == NULL && !(flags & JOB_INTERNAL)) {
@@ -506,6 +507,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
flags, cb, opaque, errp);
if (job == NULL) {
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
return NULL;
}
@@ -544,10 +546,12 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
}
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
return job;
fail:
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
job_early_fail(&job->job);
return NULL;
}
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 7061ab7201..990f3e179a 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -137,6 +137,8 @@ BlockJob *block_job_get_locked(const char *id);
* Add @bs to the list of BlockDriverState that are involved in
* @job. This means that all operations will be blocked on @bs while
* @job exists.
+ *
+ * All block nodes must be drained.
*/
int GRAPH_WRLOCK
block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 3185f3f429..4f3057844b 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -772,9 +772,11 @@ static void test_blockjob_common_drain_node(enum drain_type drain_type,
tjob->bs = src;
job = &tjob->common;
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
block_job_add_bdrv(job, "target", target, 0, BLK_PERM_ALL, &error_abort);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
switch (result) {
case TEST_JOB_SUCCESS:
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 13/48] block: move drain outside of bdrv_attach_child()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (11 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 12/48] block: move drain outside of bdrv_root_attach_child() Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 14/48] block: move drain outside of quorum_add_child() Fiona Ebner
` (36 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is part of resolving the deadlock mentioned in commit "block:
move draining out of bdrv_change_aio_context() and mark GRAPH_RDLOCK".
The function bdrv_attach_child() runs under the graph lock, so it is
not allowed to drain. It is called by:
1. replication_start()
2. quorum_add_child()
3. bdrv_open_child_common()
4. Throughout test-bdrv-graph-mod.c and test-bdrv-drain.c unit tests.
In all callers, a drained section is introduced.
The function quorum_add_child() runs under the graph lock, so it is
not actually allowed to drain. This will be addressed by the following
commit.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
Changes in v4:
* Document requirement that all nodes need to be drained for
bdrv_attach_child().
block.c | 6 ++++--
block/quorum.c | 2 ++
block/replication.c | 5 +++++
tests/unit/test-bdrv-drain.c | 14 ++++++++++++++
tests/unit/test-bdrv-graph-mod.c | 10 ++++++++++
5 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 17c34dc240..6fc87aa318 100644
--- a/block.c
+++ b/block.c
@@ -3269,6 +3269,8 @@ out:
*
* On failure NULL is returned, errp is set and the reference to
* child_bs is also dropped.
+ *
+ * All block nodes must be drained.
*/
BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
BlockDriverState *child_bs,
@@ -3283,7 +3285,6 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
GLOBAL_STATE_CODE();
- bdrv_drain_all_begin();
child = bdrv_attach_child_noperm(parent_bs, child_bs, child_name,
child_class, child_role, tran, errp);
if (!child) {
@@ -3298,7 +3299,6 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
out:
tran_finalize(tran, ret);
- bdrv_drain_all_end();
bdrv_schedule_unref(child_bs);
@@ -3789,10 +3789,12 @@ static BdrvChild *bdrv_open_child_common(const char *filename,
return NULL;
}
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
child = bdrv_attach_child(parent, bs, bdref_key, child_class, child_role,
errp);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
return child;
}
diff --git a/block/quorum.c b/block/quorum.c
index ed8ce801ee..ea17b0ec13 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1096,8 +1096,10 @@ quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs, Error **errp)
/* We can safely add the child now */
bdrv_ref(child_bs);
+ bdrv_drain_all_begin();
child = bdrv_attach_child(bs, child_bs, indexstr, &child_of_bds,
BDRV_CHILD_DATA, errp);
+ bdrv_drain_all_end();
if (child == NULL) {
s->next_child_index--;
return;
diff --git a/block/replication.c b/block/replication.c
index 07f274de9e..54cbd03e00 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -540,6 +540,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
return;
}
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_ref(hidden_disk->bs);
@@ -549,6 +550,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
if (local_err) {
error_propagate(errp, local_err);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
return;
}
@@ -559,6 +561,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
if (local_err) {
error_propagate(errp, local_err);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
return;
}
@@ -571,12 +574,14 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
!check_top_bs(top_bs, bs)) {
error_setg(errp, "No top_bs or it is invalid");
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
reopen_backing_file(bs, false, NULL);
return;
}
bdrv_op_block_all(top_bs, s->blocker);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
s->backup_job = backup_job_create(
NULL, s->secondary_disk->bs, s->hidden_disk->bs,
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 4f3057844b..ac76525e5a 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -1049,10 +1049,12 @@ static void do_test_delete_by_drain(bool detach_instead_of_delete,
null_bs = bdrv_open("null-co://", NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
&error_abort);
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_attach_child(bs, null_bs, "null-child", &child_of_bds,
BDRV_CHILD_DATA, &error_abort);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
/* This child will be the one to pass to requests through to, and
* it will stall until a drain occurs */
@@ -1060,21 +1062,25 @@ static void do_test_delete_by_drain(bool detach_instead_of_delete,
&error_abort);
child_bs->total_sectors = 65536 >> BDRV_SECTOR_BITS;
/* Takes our reference to child_bs */
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
tts->wait_child = bdrv_attach_child(bs, child_bs, "wait-child",
&child_of_bds,
BDRV_CHILD_DATA | BDRV_CHILD_PRIMARY,
&error_abort);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
/* This child is just there to be deleted
* (for detach_instead_of_delete == true) */
null_bs = bdrv_open("null-co://", NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
&error_abort);
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_attach_child(bs, null_bs, "null-child", &child_of_bds, BDRV_CHILD_DATA,
&error_abort);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
blk_insert_bs(blk, bs, &error_abort);
@@ -1157,6 +1163,7 @@ static void no_coroutine_fn detach_indirect_bh(void *opaque)
bdrv_dec_in_flight(data->child_b->bs);
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_unref_child(data->parent_b, data->child_b);
@@ -1165,6 +1172,7 @@ static void no_coroutine_fn detach_indirect_bh(void *opaque)
&child_of_bds, BDRV_CHILD_DATA,
&error_abort);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
}
static void coroutine_mixed_fn detach_by_parent_aio_cb(void *opaque, int ret)
@@ -1262,6 +1270,7 @@ static void TSA_NO_TSA test_detach_indirect(bool by_parent_cb)
/* Set child relationships */
bdrv_ref(b);
bdrv_ref(a);
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
child_b = bdrv_attach_child(parent_b, b, "PB-B", &child_of_bds,
BDRV_CHILD_DATA, &error_abort);
@@ -1273,6 +1282,7 @@ static void TSA_NO_TSA test_detach_indirect(bool by_parent_cb)
by_parent_cb ? &child_of_bds : &detach_by_driver_cb_class,
BDRV_CHILD_DATA, &error_abort);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
g_assert_cmpint(parent_a->refcnt, ==, 1);
g_assert_cmpint(parent_b->refcnt, ==, 1);
@@ -1685,6 +1695,7 @@ static void test_drop_intermediate_poll(void)
* Establish the chain last, so the chain links are the first
* elements in the BDS.parents lists
*/
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
for (i = 0; i < 3; i++) {
if (i) {
@@ -1694,6 +1705,7 @@ static void test_drop_intermediate_poll(void)
}
}
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
job = block_job_create("job", &test_simple_job_driver, NULL, job_node,
0, BLK_PERM_ALL, 0, 0, NULL, NULL, &error_abort);
@@ -1940,10 +1952,12 @@ static void do_test_replace_child_mid_drain(int old_drain_count,
new_child_bs->total_sectors = 1;
bdrv_ref(old_child_bs);
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_attach_child(parent_bs, old_child_bs, "child", &child_of_bds,
BDRV_CHILD_COW, &error_abort);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
parent_s->setup_completed = true;
for (i = 0; i < old_drain_count; i++) {
diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c
index d743abb4bb..7b03ebe4b0 100644
--- a/tests/unit/test-bdrv-graph-mod.c
+++ b/tests/unit/test-bdrv-graph-mod.c
@@ -137,10 +137,12 @@ static void test_update_perm_tree(void)
blk_insert_bs(root, bs, &error_abort);
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_attach_child(filter, bs, "child", &child_of_bds,
BDRV_CHILD_DATA, &error_abort);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
ret = bdrv_append(filter, bs, NULL);
g_assert_cmpint(ret, <, 0);
@@ -204,11 +206,13 @@ static void test_should_update_child(void)
bdrv_set_backing_hd(target, bs, &error_abort);
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
g_assert(target->backing->bs == bs);
bdrv_attach_child(filter, target, "target", &child_of_bds,
BDRV_CHILD_DATA, &error_abort);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
bdrv_append(filter, bs, &error_abort);
bdrv_graph_rdlock_main_loop();
@@ -244,6 +248,7 @@ static void test_parallel_exclusive_write(void)
bdrv_ref(base);
bdrv_ref(fl1);
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_attach_child(top, fl1, "backing", &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -257,6 +262,7 @@ static void test_parallel_exclusive_write(void)
bdrv_replace_node(fl1, fl2, &error_abort);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
bdrv_drained_end(fl2);
bdrv_drained_end(fl1);
@@ -363,6 +369,7 @@ static void test_parallel_perm_update(void)
*/
bdrv_ref(base);
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_attach_child(top, ws, "file", &child_of_bds, BDRV_CHILD_DATA,
&error_abort);
@@ -377,6 +384,7 @@ static void test_parallel_perm_update(void)
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
&error_abort);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
/* Select fl1 as first child to be active */
s->selected = c_fl1;
@@ -430,11 +438,13 @@ static void test_append_greedy_filter(void)
BlockDriverState *base = no_perm_node("base");
BlockDriverState *fl = exclusive_writer_node("fl1");
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_attach_child(top, base, "backing", &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
&error_abort);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
bdrv_append(fl, base, &error_abort);
bdrv_unref(fl);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 14/48] block: move drain outside of quorum_add_child()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (12 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 13/48] block: move drain outside of bdrv_attach_child() Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 15/48] block: move drain outside of bdrv_root_unref_child() Fiona Ebner
` (35 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is part of resolving the deadlock mentioned in commit "block:
move draining out of bdrv_change_aio_context() and mark GRAPH_RDLOCK".
The quorum_add_child() callback runs under the graph lock, so it is
not allowed to drain. It is only called as the .bdrv_add_child()
callback, which is only called in the bdrv_add_child() function, which
also runs under the graph lock.
The bdrv_add_child() function is called by qmp_x_blockdev_change(),
where a drained section is introduced.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
Changes in v4:
* Document requirement that all nodes need to be drained for
bdrv_add_child() wrapper and callback.
* Split common comment for bdrv_add_child()/bdrv_del_child() to become
one comment for each function.
* Add comment for bdrv_add_child() callback.
block.c | 10 ++++++++--
block/quorum.c | 2 --
blockdev.c | 2 ++
include/block/block_int-common.h | 7 +++++++
4 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index 6fc87aa318..f6c2f7e208 100644
--- a/block.c
+++ b/block.c
@@ -8220,8 +8220,10 @@ char *bdrv_dirname(BlockDriverState *bs, Error **errp)
}
/*
- * Hot add/remove a BDS's child. So the user can take a child offline when
- * it is broken and take a new child online
+ * Hot add a BDS's child. Used in combination with bdrv_del_child, so the user
+ * can take a child offline when it is broken and take a new child online.
+ *
+ * All block nodes must be drained.
*/
void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs,
Error **errp)
@@ -8261,6 +8263,10 @@ void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs,
parent_bs->drv->bdrv_add_child(parent_bs, child_bs, errp);
}
+/*
+ * Hot remove a BDS's child. Used in combination with bdrv_add_child, so the
+ * user can take a child offline when it is broken and take a new child online.
+ */
void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
{
BdrvChild *tmp;
diff --git a/block/quorum.c b/block/quorum.c
index ea17b0ec13..ed8ce801ee 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1096,10 +1096,8 @@ quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs, Error **errp)
/* We can safely add the child now */
bdrv_ref(child_bs);
- bdrv_drain_all_begin();
child = bdrv_attach_child(bs, child_bs, indexstr, &child_of_bds,
BDRV_CHILD_DATA, errp);
- bdrv_drain_all_end();
if (child == NULL) {
s->next_child_index--;
return;
diff --git a/blockdev.c b/blockdev.c
index 750beba41f..bd5ca77619 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3531,6 +3531,7 @@ void qmp_x_blockdev_change(const char *parent, const char *child,
BlockDriverState *parent_bs, *new_bs = NULL;
BdrvChild *p_child;
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
parent_bs = bdrv_lookup_bs(parent, parent, errp);
@@ -3568,6 +3569,7 @@ void qmp_x_blockdev_change(const char *parent, const char *child,
out:
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
}
BlockJobInfoList *qmp_query_block_jobs(Error **errp)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 168f703fa1..f9e742f812 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -396,6 +396,13 @@ struct BlockDriver {
int GRAPH_RDLOCK_PTR (*bdrv_probe_geometry)(
BlockDriverState *bs, HDGeometry *geo);
+ /**
+ * Hot add a BDS's child. Used in combination with bdrv_del_child, so the
+ * user can take a child offline when it is broken and take a new child
+ * online.
+ *
+ * All block nodes must be drained.
+ */
void GRAPH_WRLOCK_PTR (*bdrv_add_child)(
BlockDriverState *parent, BlockDriverState *child, Error **errp);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 15/48] block: move drain outside of bdrv_root_unref_child()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (13 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 14/48] block: move drain outside of quorum_add_child() Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 16/48] block: move drain outside of quorum_del_child() Fiona Ebner
` (34 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is part of resolving the deadlock mentioned in commit "block:
move draining out of bdrv_change_aio_context() and mark GRAPH_RDLOCK".
bdrv_root_unref_child() is called by:
1. blk_remove_bs(), where a drained section is introduced.
2. bdrv_unref_child(), which runs under the graph lock, so the drain
will be moved further up to its callers.
3. block_job_remove_all_bdrv(), where a drained section is introduced.
For all callers of bdrv_unref_child() and its generated
bdrv_co_unref_child() coroutine variant, a drained section is
introduced, they are not explicilty listed here. The caller
quorum_del_child() holds the graph lock, so it is not actually allowed
to drain. This will be addressed in the next commit.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
Changes in v4:
* Document requirement that all nodes need to be drained for
bdrv_root_unref_child() and bdrv_unref_child().
* Also cover bdrv_co_unref_child().
block.c | 18 ++++++++++++++----
block/blklogwrites.c | 4 ++++
block/blkverify.c | 2 ++
block/block-backend.c | 2 ++
block/qcow2.c | 4 ++++
block/quorum.c | 6 ++++++
block/replication.c | 2 ++
block/snapshot.c | 2 ++
block/vmdk.c | 10 ++++++++++
blockjob.c | 2 ++
tests/unit/test-bdrv-drain.c | 4 ++++
11 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index f6c2f7e208..15a8ccb822 100644
--- a/block.c
+++ b/block.c
@@ -1721,12 +1721,14 @@ bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, const char *node_name,
open_failed:
bs->drv = NULL;
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
if (bs->file != NULL) {
bdrv_unref_child(bs, bs->file);
assert(!bs->file);
}
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
g_free(bs->opaque);
bs->opaque = NULL;
@@ -3305,7 +3307,11 @@ out:
return ret < 0 ? NULL : child;
}
-/* Callers must ensure that child->frozen is false. */
+/*
+ * Callers must ensure that child->frozen is false.
+ *
+ * All block nodes must be drained.
+ */
void bdrv_root_unref_child(BdrvChild *child)
{
BlockDriverState *child_bs = child->bs;
@@ -3326,10 +3332,8 @@ void bdrv_root_unref_child(BdrvChild *child)
* When the parent requiring a non-default AioContext is removed, the
* node moves back to the main AioContext
*/
- bdrv_drain_all_begin();
bdrv_try_change_aio_context_locked(child_bs, qemu_get_aio_context(),
NULL, NULL);
- bdrv_drain_all_end();
}
bdrv_schedule_unref(child_bs);
@@ -3402,7 +3406,11 @@ bdrv_unset_inherits_from(BlockDriverState *root, BdrvChild *child,
}
}
-/* Callers must ensure that child->frozen is false. */
+/*
+ * Callers must ensure that child->frozen is false.
+ *
+ * All block nodes must be drained.
+ */
void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
{
GLOBAL_STATE_CODE();
@@ -5172,6 +5180,7 @@ static void bdrv_close(BlockDriverState *bs)
bs->drv = NULL;
}
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
bdrv_unref_child(bs, child);
@@ -5180,6 +5189,7 @@ static void bdrv_close(BlockDriverState *bs)
assert(!bs->backing);
assert(!bs->file);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
g_free(bs->opaque);
bs->opaque = NULL;
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index b0f78c4bc7..70ac76f401 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -281,9 +281,11 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
ret = 0;
fail_log:
if (ret < 0) {
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_unref_child(bs, s->log_file);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
s->log_file = NULL;
qemu_mutex_destroy(&s->mutex);
}
@@ -296,10 +298,12 @@ static void blk_log_writes_close(BlockDriverState *bs)
{
BDRVBlkLogWritesState *s = bs->opaque;
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_unref_child(bs, s->log_file);
s->log_file = NULL;
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
qemu_mutex_destroy(&s->mutex);
}
diff --git a/block/blkverify.c b/block/blkverify.c
index db79a36681..3a71f7498c 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -151,10 +151,12 @@ static void blkverify_close(BlockDriverState *bs)
{
BDRVBlkverifyState *s = bs->opaque;
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_unref_child(bs, s->test_file);
s->test_file = NULL;
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
}
static int64_t coroutine_fn GRAPH_RDLOCK
diff --git a/block/block-backend.c b/block/block-backend.c
index 24cae3cb55..68209bb2f7 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -889,9 +889,11 @@ void blk_remove_bs(BlockBackend *blk)
root = blk->root;
blk->root = NULL;
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_root_unref_child(root);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
}
/*
diff --git a/block/qcow2.c b/block/qcow2.c
index 66fba89b41..45451a7ee8 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1895,7 +1895,9 @@ qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
g_free(s->image_data_file);
if (open_data_file && has_data_file(bs)) {
bdrv_graph_co_rdunlock();
+ bdrv_drain_all_begin();
bdrv_co_unref_child(bs, s->data_file);
+ bdrv_drain_all_end();
bdrv_graph_co_rdlock();
s->data_file = NULL;
}
@@ -2821,9 +2823,11 @@ qcow2_do_close(BlockDriverState *bs, bool close_data_file)
if (close_data_file && has_data_file(bs)) {
GLOBAL_STATE_CODE();
bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_unref_child(bs, s->data_file);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
s->data_file = NULL;
bdrv_graph_rdlock_main_loop();
}
diff --git a/block/quorum.c b/block/quorum.c
index ed8ce801ee..81407a38ee 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1037,6 +1037,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
close_exit:
/* cleanup on error */
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
for (i = 0; i < s->num_children; i++) {
if (!opened[i]) {
@@ -1045,6 +1046,7 @@ close_exit:
bdrv_unref_child(bs, s->children[i]);
}
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
g_free(s->children);
g_free(opened);
exit:
@@ -1057,11 +1059,13 @@ static void quorum_close(BlockDriverState *bs)
BDRVQuorumState *s = bs->opaque;
int i;
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
for (i = 0; i < s->num_children; i++) {
bdrv_unref_child(bs, s->children[i]);
}
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
g_free(s->children);
}
@@ -1143,7 +1147,9 @@ quorum_del_child(BlockDriverState *bs, BdrvChild *child, Error **errp)
(s->num_children - i - 1) * sizeof(BdrvChild *));
s->children = g_renew(BdrvChild *, s->children, --s->num_children);
+ bdrv_drain_all_begin();
bdrv_unref_child(bs, child);
+ bdrv_drain_all_end();
quorum_refresh_flags(bs);
}
diff --git a/block/replication.c b/block/replication.c
index 54cbd03e00..0879718854 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -656,12 +656,14 @@ static void replication_done(void *opaque, int ret)
if (ret == 0) {
s->stage = BLOCK_REPLICATION_DONE;
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_unref_child(bs, s->secondary_disk);
s->secondary_disk = NULL;
bdrv_unref_child(bs, s->hidden_disk);
s->hidden_disk = NULL;
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
s->error = 0;
} else {
diff --git a/block/snapshot.c b/block/snapshot.c
index 9f300a78bd..28c9c43621 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -291,9 +291,11 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
}
/* .bdrv_open() will re-attach it */
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_unref_child(bs, fallback);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp);
memset(bs->opaque, 0, drv->instance_size);
diff --git a/block/vmdk.c b/block/vmdk.c
index 9c7ab037e1..89a7250120 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -271,6 +271,7 @@ static void vmdk_free_extents(BlockDriverState *bs)
BDRVVmdkState *s = bs->opaque;
VmdkExtent *e;
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
for (i = 0; i < s->num_extents; i++) {
e = &s->extents[i];
@@ -283,6 +284,7 @@ static void vmdk_free_extents(BlockDriverState *bs)
}
}
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
g_free(s->extents);
}
@@ -1247,9 +1249,11 @@ vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
0, 0, 0, 0, 0, &extent, errp);
if (ret < 0) {
bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_unref_child(bs, extent_file);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
bdrv_graph_rdlock_main_loop();
goto out;
}
@@ -1266,9 +1270,11 @@ vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
g_free(buf);
if (ret) {
bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_unref_child(bs, extent_file);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
bdrv_graph_rdlock_main_loop();
goto out;
}
@@ -1277,9 +1283,11 @@ vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
ret = vmdk_open_se_sparse(bs, extent_file, bs->open_flags, errp);
if (ret) {
bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_unref_child(bs, extent_file);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
bdrv_graph_rdlock_main_loop();
goto out;
}
@@ -1287,9 +1295,11 @@ vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
} else {
error_setg(errp, "Unsupported extent type '%s'", type);
bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
bdrv_unref_child(bs, extent_file);
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
bdrv_graph_rdlock_main_loop();
ret = -ENOTSUP;
goto out;
diff --git a/blockjob.c b/blockjob.c
index 44991e3ff7..e68181a35b 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -198,6 +198,7 @@ void block_job_remove_all_bdrv(BlockJob *job)
* one to make sure that such a concurrent access does not attempt
* to process an already freed BdrvChild.
*/
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
while (job->nodes) {
GSList *l = job->nodes;
@@ -211,6 +212,7 @@ void block_job_remove_all_bdrv(BlockJob *job)
g_slist_free_1(l);
}
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
}
bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs)
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index ac76525e5a..59c2793725 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -955,11 +955,13 @@ static void bdrv_test_top_close(BlockDriverState *bs)
{
BdrvChild *c, *next_c;
+ bdrv_drain_all_begin();
bdrv_graph_wrlock();
QLIST_FOREACH_SAFE(c, &bs->children, next, next_c) {
bdrv_unref_child(bs, c);
}
bdrv_graph_wrunlock();
+ bdrv_drain_all_end();
}
static int coroutine_fn GRAPH_RDLOCK
@@ -1016,7 +1018,9 @@ static void coroutine_fn test_co_delete_by_drain(void *opaque)
bdrv_graph_co_rdlock();
QLIST_FOREACH_SAFE(c, &bs->children, next, next_c) {
bdrv_graph_co_rdunlock();
+ bdrv_drain_all_begin();
bdrv_co_unref_child(bs, c);
+ bdrv_drain_all_end();
bdrv_graph_co_rdlock();
}
bdrv_graph_co_rdunlock();
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 16/48] block: move drain outside of quorum_del_child()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (14 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 15/48] block: move drain outside of bdrv_root_unref_child() Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 17/48] blockdev: drain while unlocked in internal_snapshot_action() Fiona Ebner
` (33 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The quorum_del_child() callback runs under the graph lock, so it is
not allowed to drain. It is only called as the .bdrv_del_child()
callback, which is only called in the bdrv_del_child() function, which
also runs under the graph lock.
The bdrv_del_child() function is called by qmp_x_blockdev_change().
A drained section was already introduced there by commit "block: move
drain out of quorum_add_child()".
This finally finishes moving out the drain to places that are not
under the graph lock started in "block: move draining out of
bdrv_change_aio_context() and mark GRAPH_RDLOCK".
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
Changes in v4:
* Document requirement that all nodes need to be drained for
bdrv_del_child() wrapper and callback.
* Add function comment for bdrv_del_child() callback.
block.c | 2 ++
block/quorum.c | 2 --
include/block/block_int-common.h | 7 +++++++
3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 15a8ccb822..bfd4340b24 100644
--- a/block.c
+++ b/block.c
@@ -8276,6 +8276,8 @@ void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs,
/*
* Hot remove a BDS's child. Used in combination with bdrv_add_child, so the
* user can take a child offline when it is broken and take a new child online.
+ *
+ * All block nodes must be drained.
*/
void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
{
diff --git a/block/quorum.c b/block/quorum.c
index 81407a38ee..cc3bc5f4e7 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1147,9 +1147,7 @@ quorum_del_child(BlockDriverState *bs, BdrvChild *child, Error **errp)
(s->num_children - i - 1) * sizeof(BdrvChild *));
s->children = g_renew(BdrvChild *, s->children, --s->num_children);
- bdrv_drain_all_begin();
bdrv_unref_child(bs, child);
- bdrv_drain_all_end();
quorum_refresh_flags(bs);
}
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index f9e742f812..925a3e7353 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -406,6 +406,13 @@ struct BlockDriver {
void GRAPH_WRLOCK_PTR (*bdrv_add_child)(
BlockDriverState *parent, BlockDriverState *child, Error **errp);
+ /**
+ * Hot remove a BDS's child. Used in combination with bdrv_add_child, so the
+ * user can take a child offline when it is broken and take a new child
+ * online.
+ *
+ * All block nodes must be drained.
+ */
void GRAPH_WRLOCK_PTR (*bdrv_del_child)(
BlockDriverState *parent, BdrvChild *child, Error **errp);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 17/48] blockdev: drain while unlocked in internal_snapshot_action()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (15 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 16/48] block: move drain outside of quorum_del_child() Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 18/48] blockdev: drain while unlocked in external_snapshot_action() Fiona Ebner
` (32 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is in preparation to mark bdrv_drained_begin() as GRAPH_UNLOCKED.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
blockdev.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index bd5ca77619..506755bef1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1208,7 +1208,7 @@ static void internal_snapshot_action(BlockdevSnapshotInternal *internal,
Error *local_err = NULL;
const char *device;
const char *name;
- BlockDriverState *bs;
+ BlockDriverState *bs, *check_bs;
QEMUSnapshotInfo old_sn, *sn;
bool ret;
int64_t rt;
@@ -1216,7 +1216,7 @@ static void internal_snapshot_action(BlockdevSnapshotInternal *internal,
int ret1;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+ bdrv_graph_rdlock_main_loop();
tran_add(tran, &internal_snapshot_drv, state);
@@ -1225,14 +1225,29 @@ static void internal_snapshot_action(BlockdevSnapshotInternal *internal,
bs = qmp_get_root_bs(device, errp);
if (!bs) {
+ bdrv_graph_rdunlock_main_loop();
return;
}
state->bs = bs;
+ /* Need to drain while unlocked. */
+ bdrv_graph_rdunlock_main_loop();
/* Paired with .clean() */
bdrv_drained_begin(bs);
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
+ /* Make sure the root bs did not change with the drain. */
+ check_bs = qmp_get_root_bs(device, errp);
+ if (bs != check_bs) {
+ if (check_bs) {
+ error_setg(errp, "Block node of device '%s' unexpectedly changed",
+ device);
+ } /* else errp is already set */
+ return;
+ }
+
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, errp)) {
return;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 18/48] blockdev: drain while unlocked in external_snapshot_action()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (16 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 17/48] blockdev: drain while unlocked in internal_snapshot_action() Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 19/48] block: mark bdrv_drained_begin() and friends as GRAPH_UNLOCKED Fiona Ebner
` (31 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is in preparation to mark bdrv_drained_begin() as GRAPH_UNLOCKED.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
blockdev.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index 506755bef1..2e7fda6780 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1377,9 +1377,10 @@ static void external_snapshot_action(TransactionAction *action,
const char *new_image_file;
ExternalSnapshotState *state = g_new0(ExternalSnapshotState, 1);
uint64_t perm, shared;
+ BlockDriverState *check_bs;
/* TODO We'll eventually have to take a writer lock in this function */
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+ bdrv_graph_rdlock_main_loop();
tran_add(tran, &external_snapshot_drv, state);
@@ -1412,11 +1413,25 @@ static void external_snapshot_action(TransactionAction *action,
state->old_bs = bdrv_lookup_bs(device, node_name, errp);
if (!state->old_bs) {
+ bdrv_graph_rdunlock_main_loop();
return;
}
+ /* Need to drain while unlocked. */
+ bdrv_graph_rdunlock_main_loop();
/* Paired with .clean() */
bdrv_drained_begin(state->old_bs);
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
+ /* Make sure the associated bs did not change with the drain. */
+ check_bs = bdrv_lookup_bs(device, node_name, errp);
+ if (state->old_bs != check_bs) {
+ if (check_bs) {
+ error_setg(errp, "Block node of device '%s' unexpectedly changed",
+ device);
+ } /* else errp is already set */
+ return;
+ }
if (!bdrv_is_inserted(state->old_bs)) {
error_setg(errp, "Device '%s' has no medium",
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 19/48] block: mark bdrv_drained_begin() and friends as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (17 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 18/48] blockdev: drain while unlocked in external_snapshot_action() Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 20/48] iotests/graph-changes-while-io: remove image file after test Fiona Ebner
` (30 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
All of bdrv_drain_all_begin(), bdrv_drain_all() and
bdrv_drained_begin() poll and are not allowed to be called with the
block graph lock held. Mark the function as such.
Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
include/block/block-global-state.h | 4 ++--
include/block/block-io.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 91f249b5ad..84a2a4ecd5 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -192,10 +192,10 @@ int bdrv_inactivate_all(void);
int bdrv_flush_all(void);
void bdrv_close_all(void);
-void bdrv_drain_all_begin(void);
+void GRAPH_UNLOCKED bdrv_drain_all_begin(void);
void bdrv_drain_all_begin_nopoll(void);
void bdrv_drain_all_end(void);
-void bdrv_drain_all(void);
+void GRAPH_UNLOCKED bdrv_drain_all(void);
void bdrv_aio_cancel(BlockAIOCB *acb);
diff --git a/include/block/block-io.h b/include/block/block-io.h
index b99cc98d26..4cf83fb367 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -431,7 +431,7 @@ bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent,
*
* This function can be recursive.
*/
-void bdrv_drained_begin(BlockDriverState *bs);
+void GRAPH_UNLOCKED bdrv_drained_begin(BlockDriverState *bs);
/**
* bdrv_do_drained_begin_quiesce:
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 20/48] iotests/graph-changes-while-io: remove image file after test
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (18 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 19/48] block: mark bdrv_drained_begin() and friends as GRAPH_UNLOCKED Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 21/48] iotests/graph-changes-while-io: add test case with removal of lower snapshot Fiona Ebner
` (29 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
tests/qemu-iotests/tests/graph-changes-while-io | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/qemu-iotests/tests/graph-changes-while-io b/tests/qemu-iotests/tests/graph-changes-while-io
index 194fda500e..35489e3b5e 100755
--- a/tests/qemu-iotests/tests/graph-changes-while-io
+++ b/tests/qemu-iotests/tests/graph-changes-while-io
@@ -57,6 +57,7 @@ class TestGraphChangesWhileIO(QMPTestCase):
def tearDown(self) -> None:
self.qsd.stop()
+ os.remove(top)
def test_blockdev_add_while_io(self) -> None:
# Run qemu-img bench in the background
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 21/48] iotests/graph-changes-while-io: add test case with removal of lower snapshot
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (19 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 20/48] iotests/graph-changes-while-io: remove image file after test Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:10 ` [PATCH v4 22/48] block/io: remove duplicate GLOBAL_STATE_CODE() in bdrv_do_drained_end() Fiona Ebner
` (28 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
From: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
This case is catching potential deadlock which takes place when job-dismiss
is issued when I/O requests are processed in a separate iothread.
See https://mail.gnu.org/archive/html/qemu-devel/2025-04/msg04421.html
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
[FE: re-use top image and rename snap1->mid as suggested by Kevin Wolf
remove image file after test as suggested by Kevin Wolf
add type annotation for function argument to make mypy happy]
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
.../qemu-iotests/tests/graph-changes-while-io | 101 ++++++++++++++++--
.../tests/graph-changes-while-io.out | 4 +-
2 files changed, 96 insertions(+), 9 deletions(-)
diff --git a/tests/qemu-iotests/tests/graph-changes-while-io b/tests/qemu-iotests/tests/graph-changes-while-io
index 35489e3b5e..dca1167b6d 100755
--- a/tests/qemu-iotests/tests/graph-changes-while-io
+++ b/tests/qemu-iotests/tests/graph-changes-while-io
@@ -27,6 +27,7 @@ from iotests import imgfmt, qemu_img, qemu_img_create, qemu_io, \
top = os.path.join(iotests.test_dir, 'top.img')
+mid = os.path.join(iotests.test_dir, 'mid.img')
nbd_sock = os.path.join(iotests.sock_dir, 'nbd.sock')
@@ -59,6 +60,15 @@ class TestGraphChangesWhileIO(QMPTestCase):
self.qsd.stop()
os.remove(top)
+ def _wait_for_blockjob(self, status: str) -> None:
+ done = False
+ while not done:
+ for event in self.qsd.get_qmp().get_events(wait=10.0):
+ if event['event'] != 'JOB_STATUS_CHANGE':
+ continue
+ if event['data']['status'] == status:
+ done = True
+
def test_blockdev_add_while_io(self) -> None:
# Run qemu-img bench in the background
bench_thr = Thread(target=do_qemu_img_bench)
@@ -117,16 +127,93 @@ class TestGraphChangesWhileIO(QMPTestCase):
'device': 'job0',
})
- cancelled = False
- while not cancelled:
- for event in self.qsd.get_qmp().get_events(wait=10.0):
- if event['event'] != 'JOB_STATUS_CHANGE':
- continue
- if event['data']['status'] == 'null':
- cancelled = True
+ self._wait_for_blockjob('null')
bench_thr.join()
+ def test_remove_lower_snapshot_while_io(self) -> None:
+ # Run qemu-img bench in the background
+ bench_thr = Thread(target=do_qemu_img_bench, args=(100000, ))
+ bench_thr.start()
+
+ # While I/O is performed on 'node0' node, consequently add 2 snapshots
+ # on top of it, then remove (commit) them starting from lower one.
+ while bench_thr.is_alive():
+ # Recreate snapshot images on every iteration
+ qemu_img_create('-f', imgfmt, mid, '1G')
+ qemu_img_create('-f', imgfmt, top, '1G')
+
+ self.qsd.cmd('blockdev-add', {
+ 'driver': imgfmt,
+ 'node-name': 'mid',
+ 'file': {
+ 'driver': 'file',
+ 'filename': mid
+ }
+ })
+
+ self.qsd.cmd('blockdev-snapshot', {
+ 'node': 'node0',
+ 'overlay': 'mid',
+ })
+
+ self.qsd.cmd('blockdev-add', {
+ 'driver': imgfmt,
+ 'node-name': 'top',
+ 'file': {
+ 'driver': 'file',
+ 'filename': top
+ }
+ })
+
+ self.qsd.cmd('blockdev-snapshot', {
+ 'node': 'mid',
+ 'overlay': 'top',
+ })
+
+ self.qsd.cmd('block-commit', {
+ 'job-id': 'commit-mid',
+ 'device': 'top',
+ 'top-node': 'mid',
+ 'base-node': 'node0',
+ 'auto-finalize': True,
+ 'auto-dismiss': False,
+ })
+
+ self._wait_for_blockjob('concluded')
+ self.qsd.cmd('job-dismiss', {
+ 'id': 'commit-mid',
+ })
+
+ self.qsd.cmd('block-commit', {
+ 'job-id': 'commit-top',
+ 'device': 'top',
+ 'top-node': 'top',
+ 'base-node': 'node0',
+ 'auto-finalize': True,
+ 'auto-dismiss': False,
+ })
+
+ self._wait_for_blockjob('ready')
+ self.qsd.cmd('job-complete', {
+ 'id': 'commit-top',
+ })
+
+ self._wait_for_blockjob('concluded')
+ self.qsd.cmd('job-dismiss', {
+ 'id': 'commit-top',
+ })
+
+ self.qsd.cmd('blockdev-del', {
+ 'node-name': 'mid'
+ })
+ self.qsd.cmd('blockdev-del', {
+ 'node-name': 'top'
+ })
+
+ bench_thr.join()
+ os.remove(mid)
+
if __name__ == '__main__':
# Format must support raw backing files
iotests.main(supported_fmts=['qcow', 'qcow2', 'qed'],
diff --git a/tests/qemu-iotests/tests/graph-changes-while-io.out b/tests/qemu-iotests/tests/graph-changes-while-io.out
index fbc63e62f8..8d7e996700 100644
--- a/tests/qemu-iotests/tests/graph-changes-while-io.out
+++ b/tests/qemu-iotests/tests/graph-changes-while-io.out
@@ -1,5 +1,5 @@
-..
+...
----------------------------------------------------------------------
-Ran 2 tests
+Ran 3 tests
OK
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 22/48] block/io: remove duplicate GLOBAL_STATE_CODE() in bdrv_do_drained_end()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (20 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 21/48] iotests/graph-changes-while-io: add test case with removal of lower snapshot Fiona Ebner
@ 2025-05-30 15:10 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 23/48] block: never use atomics to access bs->quiesce_counter Fiona Ebner
` (27 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Both commit ab61335025 ("block: drain from main loop thread in
bdrv_co_yield_to_drain()") and commit d05ab380db ("block: Mark drain
related functions GRAPH_RDLOCK") introduced a GLOBAL_STATE_CODE()
macro in bdrv_do_drained_end(). The assertion of being in the main
thread cannot change here, so keep only the earlier instance.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block/io.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/block/io.c b/block/io.c
index 4fd7768f9c..ac5c7174f6 100644
--- a/block/io.c
+++ b/block/io.c
@@ -413,7 +413,6 @@ static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent)
/* 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);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 23/48] block: never use atomics to access bs->quiesce_counter
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (21 preceding siblings ...)
2025-05-30 15:10 ` [PATCH v4 22/48] block/io: remove duplicate GLOBAL_STATE_CODE() in bdrv_do_drained_end() Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-06-02 14:45 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 24/48] block: add bdrv_graph_wrlock_drained() convenience wrapper Fiona Ebner
` (26 subsequent siblings)
49 siblings, 1 reply; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
All accesses of bs->quiesce_counter are in the main thread, either
after a GLOBAL_STATE_CODE() macro or in a function with GRAPH_WRLOCK
annotation.
This is essentially a revert of 414c2ec358 ("block: access
quiesce_counter with atomic ops"). At that time, neither the
GLOBAL_STATE_CODE() macro nor the GRAPH_WRLOCK annotation existed.
Even if the field was only accessed in the main thread back then (did
not check if that is actually the case), it wouldn't have been easy to
verify.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block/io.c | 7 ++-----
include/block/block_int-common.h | 2 +-
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/block/io.c b/block/io.c
index ac5c7174f6..9bd8ba8431 100644
--- a/block/io.c
+++ b/block/io.c
@@ -361,7 +361,7 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent,
GLOBAL_STATE_CODE();
/* Stop things in parent-to-child order */
- if (qatomic_fetch_inc(&bs->quiesce_counter) == 0) {
+ if (bs->quiesce_counter++ == 0) {
GRAPH_RDLOCK_GUARD_MAINLOOP();
bdrv_parent_drained_begin(bs, parent);
if (bs->drv && bs->drv->bdrv_drain_begin) {
@@ -401,8 +401,6 @@ bdrv_drained_begin(BlockDriverState *bs)
*/
static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent)
{
- int old_quiesce_counter;
-
IO_OR_GS_CODE();
if (qemu_in_coroutine()) {
@@ -415,8 +413,7 @@ static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent)
assert(bs->quiesce_counter > 0);
/* Re-enable things in child-to-parent order */
- old_quiesce_counter = qatomic_fetch_dec(&bs->quiesce_counter);
- if (old_quiesce_counter == 1) {
+ if (--bs->quiesce_counter == 0) {
GRAPH_RDLOCK_GUARD_MAINLOOP();
if (bs->drv && bs->drv->bdrv_drain_end) {
bs->drv->bdrv_drain_end(bs);
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 925a3e7353..e96c6a6a03 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1253,7 +1253,7 @@ struct BlockDriverState {
/* do we need to tell the quest if we have a volatile write cache? */
int enable_write_cache;
- /* Accessed with atomic ops. */
+ /* Accessed only in the main thread. */
int quiesce_counter;
unsigned int write_gen; /* Current data generation */
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* Re: [PATCH v4 23/48] block: never use atomics to access bs->quiesce_counter
2025-05-30 15:11 ` [PATCH v4 23/48] block: never use atomics to access bs->quiesce_counter Fiona Ebner
@ 2025-06-02 14:45 ` Fiona Ebner
2025-07-01 11:24 ` Kevin Wolf
0 siblings, 1 reply; 60+ messages in thread
From: Fiona Ebner @ 2025-06-02 14:45 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Am 30.05.25 um 17:11 schrieb Fiona Ebner:
> All accesses of bs->quiesce_counter are in the main thread, either
> after a GLOBAL_STATE_CODE() macro or in a function with GRAPH_WRLOCK
> annotation.
Now I wonder if that is actually good enough? Because vCPUs threads can
also satisfy the qemu_in_main_thread() condition.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 23/48] block: never use atomics to access bs->quiesce_counter
2025-06-02 14:45 ` Fiona Ebner
@ 2025-07-01 11:24 ` Kevin Wolf
0 siblings, 0 replies; 60+ messages in thread
From: Kevin Wolf @ 2025-07-01 11:24 UTC (permalink / raw)
To: Fiona Ebner
Cc: qemu-block, qemu-devel, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Am 02.06.2025 um 16:45 hat Fiona Ebner geschrieben:
> Am 30.05.25 um 17:11 schrieb Fiona Ebner:
> > All accesses of bs->quiesce_counter are in the main thread, either
> > after a GLOBAL_STATE_CODE() macro or in a function with GRAPH_WRLOCK
> > annotation.
>
> Now I wonder if that is actually good enough? Because vCPUs threads can
> also satisfy the qemu_in_main_thread() condition.
It is. vcpu threads can only act as the main thread while holding the
BQL, so in this case you already have the necessary synchronisation
without atomics. Basically all the otherwise non-thread-safe code
running in the main loop depends on this.
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 24/48] block: add bdrv_graph_wrlock_drained() convenience wrapper
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (22 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 23/48] block: never use atomics to access bs->quiesce_counter Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-07-01 11:37 ` Kevin Wolf
2025-05-30 15:11 ` [PATCH v4 25/48] block/mirror: switch to bdrv_set_backing_hd_drained() variant Fiona Ebner
` (25 subsequent siblings)
49 siblings, 1 reply; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Many write-locked sections are also drained sections. A new
bdrv_graph_wrunlock_drained() wrapper around bdrv_graph_wrunlock() is
introduced, which will begin a drained section first. A global
variable is used so bdrv_graph_wrunlock() knows if it also needs
to end such a drained section. Both the aio_poll call in
bdrv_graph_wrlock() and the aio_bh_poll() in bdrv_graph_wrunlock()
can re-enter a write-locked section. While for the latter, ending the
drain could be moved to before the call, the former requires that the
variable is a counter and not just a boolean.
Since the wrapper calls bdrv_drain_all_begin(), which must be called
with the graph unlocked, mark the wrapper as GRAPH_UNLOCKED too.
The switch to the new helpers was generated with the following
commands and then manually checked:
find . -name '*.c' -exec sed -i -z 's/bdrv_drain_all_begin();\n\s*bdrv_graph_wrlock();/bdrv_graph_wrlock_drained();/g' {} ';'
find . -name '*.c' -exec sed -i -z 's/bdrv_graph_wrunlock();\n\s*bdrv_drain_all_end();/bdrv_graph_wrunlock();/g' {} ';'
Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
Changes in v4:
* Adapt to context changes from earlier patch.
* Mark the wrapper as GRAPH_UNLOCKED itself
block.c | 20 ++++------------
block/backup.c | 4 +---
block/blklogwrites.c | 8 ++-----
block/blkverify.c | 4 +---
block/block-backend.c | 8 ++-----
block/commit.c | 6 +----
block/graph-lock.c | 40 +++++++++++++++++++++++++++++---
block/mirror.c | 7 +-----
block/qcow2.c | 4 +---
block/quorum.c | 8 ++-----
block/replication.c | 11 ++-------
block/snapshot.c | 4 +---
block/stream.c | 6 +----
block/vmdk.c | 20 ++++------------
blockdev.c | 4 +---
blockjob.c | 10 ++------
include/block/graph-lock.h | 11 +++++++++
tests/unit/test-bdrv-drain.c | 36 +++++++---------------------
tests/unit/test-bdrv-graph-mod.c | 20 ++++------------
19 files changed, 90 insertions(+), 141 deletions(-)
diff --git a/block.c b/block.c
index bfd4340b24..1da10d55f0 100644
--- a/block.c
+++ b/block.c
@@ -1721,14 +1721,12 @@ bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, const char *node_name,
open_failed:
bs->drv = NULL;
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
if (bs->file != NULL) {
bdrv_unref_child(bs, bs->file);
assert(!bs->file);
}
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
g_free(bs->opaque);
bs->opaque = NULL;
@@ -3602,11 +3600,9 @@ int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
int ret;
GLOBAL_STATE_CODE();
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
ret = bdrv_set_backing_hd_drained(bs, backing_hd, errp);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
return ret;
}
@@ -3797,12 +3793,10 @@ static BdrvChild *bdrv_open_child_common(const char *filename,
return NULL;
}
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
child = bdrv_attach_child(parent, bs, bdref_key, child_class, child_role,
errp);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
return child;
}
@@ -5180,8 +5174,7 @@ static void bdrv_close(BlockDriverState *bs)
bs->drv = NULL;
}
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
bdrv_unref_child(bs, child);
}
@@ -5189,7 +5182,6 @@ static void bdrv_close(BlockDriverState *bs)
assert(!bs->backing);
assert(!bs->file);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
g_free(bs->opaque);
bs->opaque = NULL;
@@ -5515,8 +5507,7 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
assert(!bs_new->backing);
bdrv_graph_rdunlock_main_loop();
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
child = bdrv_attach_child_noperm(bs_new, bs_top, "backing",
&child_of_bds, bdrv_backing_role(bs_new),
@@ -5537,7 +5528,6 @@ out:
bdrv_refresh_limits(bs_top, NULL, NULL);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
return ret;
}
diff --git a/block/backup.c b/block/backup.c
index 909027c17a..d4713fa1cd 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -498,12 +498,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
block_copy_set_speed(bcs, speed);
/* Required permissions are taken by copy-before-write filter target */
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
&error_abort);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
return &job->common;
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 70ac76f401..aa1f888869 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -281,11 +281,9 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
ret = 0;
fail_log:
if (ret < 0) {
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_unref_child(bs, s->log_file);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
s->log_file = NULL;
qemu_mutex_destroy(&s->mutex);
}
@@ -298,12 +296,10 @@ static void blk_log_writes_close(BlockDriverState *bs)
{
BDRVBlkLogWritesState *s = bs->opaque;
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_unref_child(bs, s->log_file);
s->log_file = NULL;
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
qemu_mutex_destroy(&s->mutex);
}
diff --git a/block/blkverify.c b/block/blkverify.c
index 3a71f7498c..72efcbe7ef 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -151,12 +151,10 @@ static void blkverify_close(BlockDriverState *bs)
{
BDRVBlkverifyState *s = bs->opaque;
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_unref_child(bs, s->test_file);
s->test_file = NULL;
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
}
static int64_t coroutine_fn GRAPH_RDLOCK
diff --git a/block/block-backend.c b/block/block-backend.c
index 68209bb2f7..f8d6ba65c1 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -889,11 +889,9 @@ void blk_remove_bs(BlockBackend *blk)
root = blk->root;
blk->root = NULL;
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_root_unref_child(root);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
}
/*
@@ -906,8 +904,7 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
GLOBAL_STATE_CODE();
bdrv_ref(bs);
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
if ((bs->open_flags & BDRV_O_INACTIVE) && blk_can_inactivate(blk)) {
blk->disable_perm = true;
@@ -922,7 +919,6 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
perm, shared_perm, blk, errp);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
if (blk->root == NULL) {
return -EPERM;
}
diff --git a/block/commit.c b/block/commit.c
index 6c4b736ff8..dc1942483b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -392,8 +392,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
* this is the responsibility of the interface (i.e. whoever calls
* commit_start()).
*/
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
s->base_overlay = bdrv_find_overlay(top, base);
assert(s->base_overlay);
@@ -425,21 +424,18 @@ void commit_start(const char *job_id, BlockDriverState *bs,
iter_shared_perms, errp);
if (ret < 0) {
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
goto fail;
}
}
if (bdrv_freeze_backing_chain(commit_top_bs, base, errp) < 0) {
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
goto fail;
}
s->chain_frozen = true;
ret = block_job_add_bdrv(&s->common, "base", base, 0, BLK_PERM_ALL, errp);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
if (ret < 0) {
goto fail;
diff --git a/block/graph-lock.c b/block/graph-lock.c
index c81162b147..b7319473a1 100644
--- a/block/graph-lock.c
+++ b/block/graph-lock.c
@@ -33,6 +33,17 @@ static QemuMutex aio_context_list_lock;
/* Written and read with atomic operations. */
static int has_writer;
+/*
+ * Many write-locked sections are also drained sections. There is a convenience
+ * wrapper bdrv_graph_wrlock_drained() which begins a drained section before
+ * acquiring the lock. This variable here is used so bdrv_graph_wrunlock() knows
+ * if it also needs to end such a drained section. It needs to be a counter,
+ * because the aio_poll() call in bdrv_graph_wrlock() might re-enter
+ * bdrv_graph_wrlock_drained(). And note that aio_bh_poll() in
+ * bdrv_graph_wrunlock() might also re-enter a write-locked section.
+ */
+static int wrlock_quiesced_counter;
+
/*
* A reader coroutine could move from an AioContext to another.
* If this happens, there is no problem from the point of view of
@@ -112,8 +123,14 @@ void no_coroutine_fn bdrv_graph_wrlock(void)
assert(!qatomic_read(&has_writer));
assert(!qemu_in_coroutine());
- /* Make sure that constantly arriving new I/O doesn't cause starvation */
- bdrv_drain_all_begin_nopoll();
+ bool need_drain = wrlock_quiesced_counter == 0;
+
+ if (need_drain) {
+ /*
+ * Make sure that constantly arriving new I/O doesn't cause starvation
+ */
+ bdrv_drain_all_begin_nopoll();
+ }
/*
* reader_count == 0: this means writer will read has_reader as 1
@@ -139,7 +156,18 @@ void no_coroutine_fn bdrv_graph_wrlock(void)
smp_mb();
} while (reader_count() >= 1);
- bdrv_drain_all_end();
+ if (need_drain) {
+ bdrv_drain_all_end();
+ }
+}
+
+void no_coroutine_fn bdrv_graph_wrlock_drained(void)
+{
+ GLOBAL_STATE_CODE();
+
+ bdrv_drain_all_begin();
+ wrlock_quiesced_counter++;
+ bdrv_graph_wrlock();
}
void no_coroutine_fn bdrv_graph_wrunlock(void)
@@ -168,6 +196,12 @@ void no_coroutine_fn bdrv_graph_wrunlock(void)
* progress.
*/
aio_bh_poll(qemu_get_aio_context());
+
+ if (wrlock_quiesced_counter > 0) {
+ bdrv_drain_all_end();
+ wrlock_quiesced_counter--;
+ }
+
}
void coroutine_fn bdrv_graph_co_rdlock(void)
diff --git a/block/mirror.c b/block/mirror.c
index 6e8caf4b49..82a6e50cf8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -2014,15 +2014,13 @@ static BlockJob *mirror_start_job(
*/
bdrv_disable_dirty_bitmap(s->dirty_bitmap);
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
ret = block_job_add_bdrv(&s->common, "source", bs, 0,
BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE |
BLK_PERM_CONSISTENT_READ,
errp);
if (ret < 0) {
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
goto fail;
}
@@ -2068,19 +2066,16 @@ static BlockJob *mirror_start_job(
iter_shared_perms, errp);
if (ret < 0) {
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
goto fail;
}
}
if (bdrv_freeze_backing_chain(mirror_top_bs, target, errp) < 0) {
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
goto fail;
}
}
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
QTAILQ_INIT(&s->ops_in_flight);
diff --git a/block/qcow2.c b/block/qcow2.c
index 45451a7ee8..4aa9f9e068 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2823,11 +2823,9 @@ qcow2_do_close(BlockDriverState *bs, bool close_data_file)
if (close_data_file && has_data_file(bs)) {
GLOBAL_STATE_CODE();
bdrv_graph_rdunlock_main_loop();
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_unref_child(bs, s->data_file);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
s->data_file = NULL;
bdrv_graph_rdlock_main_loop();
}
diff --git a/block/quorum.c b/block/quorum.c
index cc3bc5f4e7..76a4feb2d9 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1037,8 +1037,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
close_exit:
/* cleanup on error */
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
for (i = 0; i < s->num_children; i++) {
if (!opened[i]) {
continue;
@@ -1046,7 +1045,6 @@ close_exit:
bdrv_unref_child(bs, s->children[i]);
}
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
g_free(s->children);
g_free(opened);
exit:
@@ -1059,13 +1057,11 @@ static void quorum_close(BlockDriverState *bs)
BDRVQuorumState *s = bs->opaque;
int i;
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
for (i = 0; i < s->num_children; i++) {
bdrv_unref_child(bs, s->children[i]);
}
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
g_free(s->children);
}
diff --git a/block/replication.c b/block/replication.c
index 0879718854..83978b61f5 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -540,8 +540,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
return;
}
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_ref(hidden_disk->bs);
s->hidden_disk = bdrv_attach_child(bs, hidden_disk->bs, "hidden disk",
@@ -550,7 +549,6 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
if (local_err) {
error_propagate(errp, local_err);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
return;
}
@@ -561,7 +559,6 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
if (local_err) {
error_propagate(errp, local_err);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
return;
}
@@ -574,14 +571,12 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
!check_top_bs(top_bs, bs)) {
error_setg(errp, "No top_bs or it is invalid");
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
reopen_backing_file(bs, false, NULL);
return;
}
bdrv_op_block_all(top_bs, s->blocker);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
s->backup_job = backup_job_create(
NULL, s->secondary_disk->bs, s->hidden_disk->bs,
@@ -656,14 +651,12 @@ static void replication_done(void *opaque, int ret)
if (ret == 0) {
s->stage = BLOCK_REPLICATION_DONE;
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_unref_child(bs, s->secondary_disk);
s->secondary_disk = NULL;
bdrv_unref_child(bs, s->hidden_disk);
s->hidden_disk = NULL;
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
s->error = 0;
} else {
diff --git a/block/snapshot.c b/block/snapshot.c
index 28c9c43621..bd9d759b32 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -291,11 +291,9 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
}
/* .bdrv_open() will re-attach it */
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_unref_child(bs, fallback);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp);
memset(bs->opaque, 0, drv->instance_size);
diff --git a/block/stream.c b/block/stream.c
index f5441f27f4..a6ef840e29 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -371,12 +371,10 @@ void stream_start(const char *job_id, BlockDriverState *bs,
* already have our own plans. Also don't allow resize as the image size is
* queried only at the job start and then cached.
*/
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
if (block_job_add_bdrv(&s->common, "active node", bs, 0,
basic_flags | BLK_PERM_WRITE, errp)) {
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
goto fail;
}
@@ -397,12 +395,10 @@ void stream_start(const char *job_id, BlockDriverState *bs,
basic_flags, errp);
if (ret < 0) {
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
goto fail;
}
}
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
s->base_overlay = base_overlay;
s->above_base = above_base;
diff --git a/block/vmdk.c b/block/vmdk.c
index 89a7250120..04986c8d55 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -271,8 +271,7 @@ static void vmdk_free_extents(BlockDriverState *bs)
BDRVVmdkState *s = bs->opaque;
VmdkExtent *e;
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
for (i = 0; i < s->num_extents; i++) {
e = &s->extents[i];
g_free(e->l1_table);
@@ -284,7 +283,6 @@ static void vmdk_free_extents(BlockDriverState *bs)
}
}
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
g_free(s->extents);
}
@@ -1249,11 +1247,9 @@ vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
0, 0, 0, 0, 0, &extent, errp);
if (ret < 0) {
bdrv_graph_rdunlock_main_loop();
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_unref_child(bs, extent_file);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
bdrv_graph_rdlock_main_loop();
goto out;
}
@@ -1270,11 +1266,9 @@ vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
g_free(buf);
if (ret) {
bdrv_graph_rdunlock_main_loop();
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_unref_child(bs, extent_file);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
bdrv_graph_rdlock_main_loop();
goto out;
}
@@ -1283,11 +1277,9 @@ vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
ret = vmdk_open_se_sparse(bs, extent_file, bs->open_flags, errp);
if (ret) {
bdrv_graph_rdunlock_main_loop();
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_unref_child(bs, extent_file);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
bdrv_graph_rdlock_main_loop();
goto out;
}
@@ -1295,11 +1287,9 @@ vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
} else {
error_setg(errp, "Unsupported extent type '%s'", type);
bdrv_graph_rdunlock_main_loop();
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_unref_child(bs, extent_file);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
bdrv_graph_rdlock_main_loop();
ret = -ENOTSUP;
goto out;
diff --git a/blockdev.c b/blockdev.c
index 2e7fda6780..e625534925 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3561,8 +3561,7 @@ void qmp_x_blockdev_change(const char *parent, const char *child,
BlockDriverState *parent_bs, *new_bs = NULL;
BdrvChild *p_child;
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
parent_bs = bdrv_lookup_bs(parent, parent, errp);
if (!parent_bs) {
@@ -3599,7 +3598,6 @@ void qmp_x_blockdev_change(const char *parent, const char *child,
out:
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
}
BlockJobInfoList *qmp_query_block_jobs(Error **errp)
diff --git a/blockjob.c b/blockjob.c
index e68181a35b..db7c3a69a0 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -198,8 +198,7 @@ void block_job_remove_all_bdrv(BlockJob *job)
* one to make sure that such a concurrent access does not attempt
* to process an already freed BdrvChild.
*/
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
while (job->nodes) {
GSList *l = job->nodes;
BdrvChild *c = l->data;
@@ -212,7 +211,6 @@ void block_job_remove_all_bdrv(BlockJob *job)
g_slist_free_1(l);
}
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
}
bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs)
@@ -498,8 +496,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
int ret;
GLOBAL_STATE_CODE();
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
if (job_id == NULL && !(flags & JOB_INTERNAL)) {
job_id = bdrv_get_device_name(bs);
@@ -509,7 +506,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
flags, cb, opaque, errp);
if (job == NULL) {
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
return NULL;
}
@@ -548,12 +544,10 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
}
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
return job;
fail:
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
job_early_fail(&job->job);
return NULL;
}
diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h
index 2c26c72108..b564cba2c0 100644
--- a/include/block/graph-lock.h
+++ b/include/block/graph-lock.h
@@ -112,10 +112,21 @@ void unregister_aiocontext(AioContext *ctx);
void no_coroutine_fn TSA_ACQUIRE(graph_lock) TSA_NO_TSA
bdrv_graph_wrlock(void);
+/*
+ * bdrv_graph_wrlock_drained:
+ * Similar to bdrv_graph_wrlock, but will begin a drained section before
+ * locking.
+ */
+void no_coroutine_fn TSA_ACQUIRE(graph_lock) TSA_NO_TSA GRAPH_UNLOCKED
+bdrv_graph_wrlock_drained(void);
+
/*
* bdrv_graph_wrunlock:
* Write finished, reset global has_writer to 0 and restart
* all readers that are waiting.
+ *
+ * Also ends the drained section if bdrv_graph_wrlock_drained() was used to lock
+ * the graph.
*/
void no_coroutine_fn TSA_RELEASE(graph_lock) TSA_NO_TSA
bdrv_graph_wrunlock(void);
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 59c2793725..3369c2c2aa 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -772,11 +772,9 @@ static void test_blockjob_common_drain_node(enum drain_type drain_type,
tjob->bs = src;
job = &tjob->common;
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
block_job_add_bdrv(job, "target", target, 0, BLK_PERM_ALL, &error_abort);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
switch (result) {
case TEST_JOB_SUCCESS:
@@ -955,13 +953,11 @@ static void bdrv_test_top_close(BlockDriverState *bs)
{
BdrvChild *c, *next_c;
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
QLIST_FOREACH_SAFE(c, &bs->children, next, next_c) {
bdrv_unref_child(bs, c);
}
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
}
static int coroutine_fn GRAPH_RDLOCK
@@ -1053,12 +1049,10 @@ static void do_test_delete_by_drain(bool detach_instead_of_delete,
null_bs = bdrv_open("null-co://", NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
&error_abort);
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_attach_child(bs, null_bs, "null-child", &child_of_bds,
BDRV_CHILD_DATA, &error_abort);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
/* This child will be the one to pass to requests through to, and
* it will stall until a drain occurs */
@@ -1066,25 +1060,21 @@ static void do_test_delete_by_drain(bool detach_instead_of_delete,
&error_abort);
child_bs->total_sectors = 65536 >> BDRV_SECTOR_BITS;
/* Takes our reference to child_bs */
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
tts->wait_child = bdrv_attach_child(bs, child_bs, "wait-child",
&child_of_bds,
BDRV_CHILD_DATA | BDRV_CHILD_PRIMARY,
&error_abort);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
/* This child is just there to be deleted
* (for detach_instead_of_delete == true) */
null_bs = bdrv_open("null-co://", NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
&error_abort);
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_attach_child(bs, null_bs, "null-child", &child_of_bds, BDRV_CHILD_DATA,
&error_abort);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
blk_insert_bs(blk, bs, &error_abort);
@@ -1167,8 +1157,7 @@ static void no_coroutine_fn detach_indirect_bh(void *opaque)
bdrv_dec_in_flight(data->child_b->bs);
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_unref_child(data->parent_b, data->child_b);
bdrv_ref(data->c);
@@ -1176,7 +1165,6 @@ static void no_coroutine_fn detach_indirect_bh(void *opaque)
&child_of_bds, BDRV_CHILD_DATA,
&error_abort);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
}
static void coroutine_mixed_fn detach_by_parent_aio_cb(void *opaque, int ret)
@@ -1274,8 +1262,7 @@ static void TSA_NO_TSA test_detach_indirect(bool by_parent_cb)
/* Set child relationships */
bdrv_ref(b);
bdrv_ref(a);
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
child_b = bdrv_attach_child(parent_b, b, "PB-B", &child_of_bds,
BDRV_CHILD_DATA, &error_abort);
child_a = bdrv_attach_child(parent_b, a, "PB-A", &child_of_bds,
@@ -1286,7 +1273,6 @@ static void TSA_NO_TSA test_detach_indirect(bool by_parent_cb)
by_parent_cb ? &child_of_bds : &detach_by_driver_cb_class,
BDRV_CHILD_DATA, &error_abort);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
g_assert_cmpint(parent_a->refcnt, ==, 1);
g_assert_cmpint(parent_b->refcnt, ==, 1);
@@ -1699,8 +1685,7 @@ static void test_drop_intermediate_poll(void)
* Establish the chain last, so the chain links are the first
* elements in the BDS.parents lists
*/
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
for (i = 0; i < 3; i++) {
if (i) {
/* Takes the reference to chain[i - 1] */
@@ -1709,7 +1694,6 @@ static void test_drop_intermediate_poll(void)
}
}
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
job = block_job_create("job", &test_simple_job_driver, NULL, job_node,
0, BLK_PERM_ALL, 0, 0, NULL, NULL, &error_abort);
@@ -1956,12 +1940,10 @@ static void do_test_replace_child_mid_drain(int old_drain_count,
new_child_bs->total_sectors = 1;
bdrv_ref(old_child_bs);
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_attach_child(parent_bs, old_child_bs, "child", &child_of_bds,
BDRV_CHILD_COW, &error_abort);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
parent_s->setup_completed = true;
for (i = 0; i < old_drain_count; i++) {
diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c
index 7b03ebe4b0..b077f0e3e3 100644
--- a/tests/unit/test-bdrv-graph-mod.c
+++ b/tests/unit/test-bdrv-graph-mod.c
@@ -137,12 +137,10 @@ static void test_update_perm_tree(void)
blk_insert_bs(root, bs, &error_abort);
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_attach_child(filter, bs, "child", &child_of_bds,
BDRV_CHILD_DATA, &error_abort);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
ret = bdrv_append(filter, bs, NULL);
g_assert_cmpint(ret, <, 0);
@@ -206,13 +204,11 @@ static void test_should_update_child(void)
bdrv_set_backing_hd(target, bs, &error_abort);
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
g_assert(target->backing->bs == bs);
bdrv_attach_child(filter, target, "target", &child_of_bds,
BDRV_CHILD_DATA, &error_abort);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
bdrv_append(filter, bs, &error_abort);
bdrv_graph_rdlock_main_loop();
@@ -248,8 +244,7 @@ static void test_parallel_exclusive_write(void)
bdrv_ref(base);
bdrv_ref(fl1);
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_attach_child(top, fl1, "backing", &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
&error_abort);
@@ -262,7 +257,6 @@ static void test_parallel_exclusive_write(void)
bdrv_replace_node(fl1, fl2, &error_abort);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
bdrv_drained_end(fl2);
bdrv_drained_end(fl1);
@@ -369,8 +363,7 @@ static void test_parallel_perm_update(void)
*/
bdrv_ref(base);
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_attach_child(top, ws, "file", &child_of_bds, BDRV_CHILD_DATA,
&error_abort);
c_fl1 = bdrv_attach_child(ws, fl1, "first", &child_of_bds,
@@ -384,7 +377,6 @@ static void test_parallel_perm_update(void)
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
&error_abort);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
/* Select fl1 as first child to be active */
s->selected = c_fl1;
@@ -438,13 +430,11 @@ static void test_append_greedy_filter(void)
BlockDriverState *base = no_perm_node("base");
BlockDriverState *fl = exclusive_writer_node("fl1");
- bdrv_drain_all_begin();
- bdrv_graph_wrlock();
+ bdrv_graph_wrlock_drained();
bdrv_attach_child(top, base, "backing", &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
&error_abort);
bdrv_graph_wrunlock();
- bdrv_drain_all_end();
bdrv_append(fl, base, &error_abort);
bdrv_unref(fl);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* Re: [PATCH v4 24/48] block: add bdrv_graph_wrlock_drained() convenience wrapper
2025-05-30 15:11 ` [PATCH v4 24/48] block: add bdrv_graph_wrlock_drained() convenience wrapper Fiona Ebner
@ 2025-07-01 11:37 ` Kevin Wolf
0 siblings, 0 replies; 60+ messages in thread
From: Kevin Wolf @ 2025-07-01 11:37 UTC (permalink / raw)
To: Fiona Ebner
Cc: qemu-block, qemu-devel, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Am 30.05.2025 um 17:11 hat Fiona Ebner geschrieben:
> Many write-locked sections are also drained sections. A new
> bdrv_graph_wrunlock_drained() wrapper around bdrv_graph_wrunlock() is
> introduced, which will begin a drained section first. A global
> variable is used so bdrv_graph_wrunlock() knows if it also needs
> to end such a drained section. Both the aio_poll call in
> bdrv_graph_wrlock() and the aio_bh_poll() in bdrv_graph_wrunlock()
> can re-enter a write-locked section. While for the latter, ending the
> drain could be moved to before the call, the former requires that the
> variable is a counter and not just a boolean.
>
> Since the wrapper calls bdrv_drain_all_begin(), which must be called
> with the graph unlocked, mark the wrapper as GRAPH_UNLOCKED too.
>
> The switch to the new helpers was generated with the following
> commands and then manually checked:
> find . -name '*.c' -exec sed -i -z 's/bdrv_drain_all_begin();\n\s*bdrv_graph_wrlock();/bdrv_graph_wrlock_drained();/g' {} ';'
> find . -name '*.c' -exec sed -i -z 's/bdrv_graph_wrunlock();\n\s*bdrv_drain_all_end();/bdrv_graph_wrunlock();/g' {} ';'
>
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> Changes in v4:
> * Adapt to context changes from earlier patch.
> * Mark the wrapper as GRAPH_UNLOCKED itself
> diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h
> index 2c26c72108..b564cba2c0 100644
> --- a/include/block/graph-lock.h
> +++ b/include/block/graph-lock.h
> @@ -112,10 +112,21 @@ void unregister_aiocontext(AioContext *ctx);
> void no_coroutine_fn TSA_ACQUIRE(graph_lock) TSA_NO_TSA
> bdrv_graph_wrlock(void);
>
> +/*
> + * bdrv_graph_wrlock_drained:
> + * Similar to bdrv_graph_wrlock, but will begin a drained section before
> + * locking.
> + */
> +void no_coroutine_fn TSA_ACQUIRE(graph_lock) TSA_NO_TSA GRAPH_UNLOCKED
> +bdrv_graph_wrlock_drained(void);
GRAPH_UNLOCKED is redundant. TSA_ACQUIRE(graph_lock) already means that
you can't call the function while holding the lock.
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 25/48] block/mirror: switch to bdrv_set_backing_hd_drained() variant
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (23 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 24/48] block: add bdrv_graph_wrlock_drained() convenience wrapper Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 26/48] block/commit: " Fiona Ebner
` (24 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is in preparation to mark bdrv_set_backing_hd() as
GRAPH_UNLOCKED.
Switch to using the bdrv_set_backing_hd_drained() variant, so that the
drained and locked section can also cover the calls to
bdrv_skip_filters() and bdrv_cow_bs().
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block/mirror.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 82a6e50cf8..873e95d029 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -761,30 +761,36 @@ static int mirror_exit_common(Job *job)
bdrv_graph_rdlock_main_loop();
bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing,
&error_abort);
+ bdrv_graph_rdunlock_main_loop();
if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
BlockDriverState *backing;
- BlockDriverState *unfiltered_target = bdrv_skip_filters(target_bs);
+ BlockDriverState *unfiltered_target;
+
+ bdrv_graph_wrlock_drained();
+ unfiltered_target = bdrv_skip_filters(target_bs);
backing = s->sync_mode == MIRROR_SYNC_MODE_NONE ? src : s->base;
if (bdrv_cow_bs(unfiltered_target) != backing) {
- bdrv_set_backing_hd(unfiltered_target, backing, &local_err);
+ bdrv_set_backing_hd_drained(unfiltered_target, backing, &local_err);
if (local_err) {
error_report_err(local_err);
local_err = NULL;
ret = -EPERM;
}
}
+ bdrv_graph_wrunlock();
} else if (!abort && s->backing_mode == MIRROR_OPEN_BACKING_CHAIN) {
+ bdrv_graph_rdlock_main_loop();
assert(!bdrv_backing_chain_next(target_bs));
ret = bdrv_open_backing_file(bdrv_skip_filters(target_bs), NULL,
"backing", &local_err);
+ bdrv_graph_rdunlock_main_loop();
if (ret < 0) {
error_report_err(local_err);
local_err = NULL;
}
}
- bdrv_graph_rdunlock_main_loop();
if (s->should_complete && !abort) {
BlockDriverState *to_replace = s->to_replace ?: src;
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 26/48] block/commit: switch to bdrv_set_backing_hd_drained() variant
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (24 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 25/48] block/mirror: switch to bdrv_set_backing_hd_drained() variant Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 27/48] block: call bdrv_set_backing_hd() while unlocked in bdrv_open_backing_file() Fiona Ebner
` (23 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is in preparation to mark bdrv_set_backing_hd() as
GRAPH_UNLOCKED.
Switch to using the bdrv_set_backing_hd_drained() variant. For the
first pair of calls to avoid draining and locking twice in a row
within the individual calls. For the third call, so that the drained
and locked section can also cover bdrv_cow_bs().
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block/commit.c | 32 +++++++++++++++++++++++++-------
1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index dc1942483b..c9690a5da0 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -514,28 +514,32 @@ int bdrv_commit(BlockDriverState *bs)
Error *local_err = NULL;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
if (!drv)
return -ENOMEDIUM;
+ bdrv_graph_rdlock_main_loop();
+
backing_file_bs = bdrv_cow_bs(bs);
if (!backing_file_bs) {
- return -ENOTSUP;
+ ret = -ENOTSUP;
+ goto out;
}
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT_SOURCE, NULL) ||
bdrv_op_is_blocked(backing_file_bs, BLOCK_OP_TYPE_COMMIT_TARGET, NULL))
{
- return -EBUSY;
+ ret = -EBUSY;
+ goto out;
}
ro = bdrv_is_read_only(backing_file_bs);
if (ro) {
if (bdrv_reopen_set_read_only(backing_file_bs, false, NULL)) {
- return -EACCES;
+ ret = -EACCES;
+ goto out;
}
}
@@ -559,8 +563,14 @@ int bdrv_commit(BlockDriverState *bs)
goto ro_cleanup;
}
- bdrv_set_backing_hd(commit_top_bs, backing_file_bs, &error_abort);
- bdrv_set_backing_hd(bs, commit_top_bs, &error_abort);
+ bdrv_graph_rdunlock_main_loop();
+
+ bdrv_graph_wrlock_drained();
+ bdrv_set_backing_hd_drained(commit_top_bs, backing_file_bs, &error_abort);
+ bdrv_set_backing_hd_drained(bs, commit_top_bs, &error_abort);
+ bdrv_graph_wrunlock();
+
+ bdrv_graph_rdlock_main_loop();
ret = blk_insert_bs(backing, backing_file_bs, &local_err);
if (ret < 0) {
@@ -635,9 +645,14 @@ int bdrv_commit(BlockDriverState *bs)
ret = 0;
ro_cleanup:
blk_unref(backing);
+
+ bdrv_graph_rdunlock_main_loop();
+ bdrv_graph_wrlock_drained();
if (bdrv_cow_bs(bs) != backing_file_bs) {
- bdrv_set_backing_hd(bs, backing_file_bs, &error_abort);
+ bdrv_set_backing_hd_drained(bs, backing_file_bs, &error_abort);
}
+ bdrv_graph_wrunlock();
+ bdrv_graph_rdlock_main_loop();
bdrv_unref(commit_top_bs);
blk_unref(src);
@@ -646,5 +661,8 @@ ro_cleanup:
bdrv_reopen_set_read_only(backing_file_bs, true, NULL);
}
+out:
+ bdrv_graph_rdunlock_main_loop();
+
return ret;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 27/48] block: call bdrv_set_backing_hd() while unlocked in bdrv_open_backing_file()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (25 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 26/48] block/commit: " Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-07-01 13:13 ` Kevin Wolf
2025-05-30 15:11 ` [PATCH v4 28/48] block: mark bdrv_set_backing_hd() as GRAPH_UNLOCKED Fiona Ebner
` (22 subsequent siblings)
49 siblings, 1 reply; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
This is in preparation to mark bdrv_set_backing_hd() as
GRAPH_UNLOCKED.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 1da10d55f0..ca3b67b233 100644
--- a/block.c
+++ b/block.c
@@ -3632,7 +3632,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
Error *local_err = NULL;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+
+ bdrv_graph_rdlock_main_loop();
if (bs->backing != NULL) {
goto free_exit;
@@ -3711,9 +3712,12 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
backing_hd->filename);
}
+
/* Hook up the backing file link; drop our reference, bs owns the
* backing_hd reference now */
+ bdrv_graph_rdunlock_main_loop();
ret = bdrv_set_backing_hd(bs, backing_hd, errp);
+ bdrv_graph_rdlock_main_loop();
bdrv_unref(backing_hd);
if (ret < 0) {
@@ -3725,6 +3729,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
free_exit:
g_free(backing_filename);
qobject_unref(tmp_parent_options);
+ bdrv_graph_rdunlock_main_loop();
return ret;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* Re: [PATCH v4 27/48] block: call bdrv_set_backing_hd() while unlocked in bdrv_open_backing_file()
2025-05-30 15:11 ` [PATCH v4 27/48] block: call bdrv_set_backing_hd() while unlocked in bdrv_open_backing_file() Fiona Ebner
@ 2025-07-01 13:13 ` Kevin Wolf
0 siblings, 0 replies; 60+ messages in thread
From: Kevin Wolf @ 2025-07-01 13:13 UTC (permalink / raw)
To: Fiona Ebner
Cc: qemu-block, qemu-devel, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Am 30.05.2025 um 17:11 hat Fiona Ebner geschrieben:
> This is in preparation to mark bdrv_set_backing_hd() as
> GRAPH_UNLOCKED.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> block.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index 1da10d55f0..ca3b67b233 100644
> --- a/block.c
> +++ b/block.c
> @@ -3632,7 +3632,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
> Error *local_err = NULL;
>
> GLOBAL_STATE_CODE();
> - GRAPH_RDLOCK_GUARD_MAINLOOP();
> +
> + bdrv_graph_rdlock_main_loop();
>
> if (bs->backing != NULL) {
> goto free_exit;
> @@ -3711,9 +3712,12 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
> backing_hd->filename);
> }
>
> +
Extra blank line.
> /* Hook up the backing file link; drop our reference, bs owns the
> * backing_hd reference now */
> + bdrv_graph_rdunlock_main_loop();
> ret = bdrv_set_backing_hd(bs, backing_hd, errp);
> + bdrv_graph_rdlock_main_loop();
> bdrv_unref(backing_hd);
>
> if (ret < 0) {
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 28/48] block: mark bdrv_set_backing_hd() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (26 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 27/48] block: call bdrv_set_backing_hd() while unlocked in bdrv_open_backing_file() Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 29/48] blockdev: avoid locking and draining multiple times in external_snapshot_abort() Fiona Ebner
` (21 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function bdrv_set_backing_hd() calls bdrv_drain_all_begin(), which
must be called with the graph unlocked.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
include/block/block-global-state.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 84a2a4ecd5..009b9ac946 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -100,8 +100,9 @@ bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
BlockDriverState * coroutine_fn no_co_wrapper
bdrv_co_open_blockdev_ref(BlockdevRef *ref, Error **errp);
-int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
- Error **errp);
+int GRAPH_UNLOCKED
+bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+ Error **errp);
int GRAPH_WRLOCK
bdrv_set_backing_hd_drained(BlockDriverState *bs, BlockDriverState *backing_hd,
Error **errp);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 29/48] blockdev: avoid locking and draining multiple times in external_snapshot_abort()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (27 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 28/48] block: mark bdrv_set_backing_hd() as GRAPH_UNLOCKED Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-06-02 8:56 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 30/48] block: drop wrapper for bdrv_set_backing_hd_drained() Fiona Ebner
` (20 subsequent siblings)
49 siblings, 1 reply; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
By using the appropriate variants bdrv_set_backing_hd_drained() and
bdrv_try_change_aio_context_locked(), there only needs to be a single
drained and write-locked section in external_snapshot_abort().
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
The assumption in the added code comment about the reference is AFAIU
it. Is this correct?
And unrelated, but I'm wondering, isn't this dead code? It's only
executed if state->overlay_appended is set, which happens at the very
end of external_snapshot_action(). How can the transaction still be
aborted after that?
blockdev.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index e625534925..3c53472a23 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1580,11 +1580,19 @@ static void external_snapshot_abort(void *opaque)
AioContext *tmp_context;
int ret;
+ bdrv_graph_wrlock_drained();
+
aio_context = bdrv_get_aio_context(state->old_bs);
- bdrv_ref(state->old_bs); /* we can't let bdrv_set_backind_hd()
- close state->old_bs; we need it */
- bdrv_set_backing_hd(state->new_bs, NULL, &error_abort);
+ /*
+ * Note that state->old_bs would not disappear during the
+ * write-locked section, because the unref from
+ * bdrv_set_backing_hd_drained() only happens at the end of the
+ * write-locked section. However, just be explicit about keeping a
+ * reference and don't rely on that implicit detail.
+ */
+ bdrv_ref(state->old_bs);
+ bdrv_set_backing_hd_drained(state->new_bs, NULL, &error_abort);
/*
* The call to bdrv_set_backing_hd() above returns state->old_bs to
@@ -1593,16 +1601,14 @@ static void external_snapshot_abort(void *opaque)
*/
tmp_context = bdrv_get_aio_context(state->old_bs);
if (aio_context != tmp_context) {
- ret = bdrv_try_change_aio_context(state->old_bs,
- aio_context, NULL, NULL);
+ ret = bdrv_try_change_aio_context_locked(state->old_bs,
+ aio_context, NULL,
+ NULL);
assert(ret == 0);
}
- bdrv_drained_begin(state->new_bs);
- bdrv_graph_wrlock();
bdrv_replace_node(state->new_bs, state->old_bs, &error_abort);
bdrv_graph_wrunlock();
- bdrv_drained_end(state->new_bs);
bdrv_unref(state->old_bs); /* bdrv_replace_node() ref'ed old_bs */
}
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* Re: [PATCH v4 29/48] blockdev: avoid locking and draining multiple times in external_snapshot_abort()
2025-05-30 15:11 ` [PATCH v4 29/48] blockdev: avoid locking and draining multiple times in external_snapshot_abort() Fiona Ebner
@ 2025-06-02 8:56 ` Fiona Ebner
0 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-06-02 8:56 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Am 30.05.25 um 17:11 schrieb Fiona Ebner:
> By using the appropriate variants bdrv_set_backing_hd_drained() and
> bdrv_try_change_aio_context_locked(), there only needs to be a single
> drained and write-locked section in external_snapshot_abort().
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> The assumption in the added code comment about the reference is AFAIU
> it. Is this correct?
>
> And unrelated, but I'm wondering, isn't this dead code? It's only
> executed if state->overlay_appended is set, which happens at the very
> end of external_snapshot_action(). How can the transaction still be
> aborted after that?
Of course, the point is that there can be multiple actions in a
transaction :) I had tunnel vision on Friday.
Best Regards,
Fiona
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 30/48] block: drop wrapper for bdrv_set_backing_hd_drained()
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (28 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 29/48] blockdev: avoid locking and draining multiple times in external_snapshot_abort() Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 31/48] block-backend: mark blk_drain_all() as GRAPH_UNLOCKED Fiona Ebner
` (19 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Nearly all callers (outside of the tests) are already using the
_drained() variant of the function. It doesn't seem worth keeping.
Simply adapt the remaining callers of bdrv_set_backing_hd() and rename
bdrv_set_backing_hd_drained() to bdrv_set_backing_hd().
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block.c | 20 ++++----------------
block/commit.c | 6 +++---
block/mirror.c | 2 +-
block/stream.c | 13 ++++++-------
blockdev.c | 13 ++++++++-----
include/block/block-global-state.h | 5 +----
tests/unit/test-bdrv-drain.c | 12 +++++++++++-
tests/unit/test-bdrv-graph-mod.c | 2 +-
8 files changed, 35 insertions(+), 38 deletions(-)
diff --git a/block.c b/block.c
index ca3b67b233..ae1b122fa8 100644
--- a/block.c
+++ b/block.c
@@ -3570,9 +3570,8 @@ out:
*
* All block nodes must be drained.
*/
-int bdrv_set_backing_hd_drained(BlockDriverState *bs,
- BlockDriverState *backing_hd,
- Error **errp)
+int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+ Error **errp)
{
int ret;
Transaction *tran = tran_new();
@@ -3594,19 +3593,6 @@ out:
return ret;
}
-int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
- Error **errp)
-{
- int ret;
- GLOBAL_STATE_CODE();
-
- bdrv_graph_wrlock_drained();
- ret = bdrv_set_backing_hd_drained(bs, backing_hd, errp);
- bdrv_graph_wrunlock();
-
- return ret;
-}
-
/*
* Opens the backing file for a BlockDriverState if not yet open
*
@@ -3716,7 +3702,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
/* Hook up the backing file link; drop our reference, bs owns the
* backing_hd reference now */
bdrv_graph_rdunlock_main_loop();
+ bdrv_graph_wrlock_drained();
ret = bdrv_set_backing_hd(bs, backing_hd, errp);
+ bdrv_graph_wrunlock();
bdrv_graph_rdlock_main_loop();
bdrv_unref(backing_hd);
diff --git a/block/commit.c b/block/commit.c
index c9690a5da0..7496cf732e 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -566,8 +566,8 @@ int bdrv_commit(BlockDriverState *bs)
bdrv_graph_rdunlock_main_loop();
bdrv_graph_wrlock_drained();
- bdrv_set_backing_hd_drained(commit_top_bs, backing_file_bs, &error_abort);
- bdrv_set_backing_hd_drained(bs, commit_top_bs, &error_abort);
+ bdrv_set_backing_hd(commit_top_bs, backing_file_bs, &error_abort);
+ bdrv_set_backing_hd(bs, commit_top_bs, &error_abort);
bdrv_graph_wrunlock();
bdrv_graph_rdlock_main_loop();
@@ -649,7 +649,7 @@ ro_cleanup:
bdrv_graph_rdunlock_main_loop();
bdrv_graph_wrlock_drained();
if (bdrv_cow_bs(bs) != backing_file_bs) {
- bdrv_set_backing_hd_drained(bs, backing_file_bs, &error_abort);
+ bdrv_set_backing_hd(bs, backing_file_bs, &error_abort);
}
bdrv_graph_wrunlock();
bdrv_graph_rdlock_main_loop();
diff --git a/block/mirror.c b/block/mirror.c
index 873e95d029..b344182c74 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -772,7 +772,7 @@ static int mirror_exit_common(Job *job)
backing = s->sync_mode == MIRROR_SYNC_MODE_NONE ? src : s->base;
if (bdrv_cow_bs(unfiltered_target) != backing) {
- bdrv_set_backing_hd_drained(unfiltered_target, backing, &local_err);
+ bdrv_set_backing_hd(unfiltered_target, backing, &local_err);
if (local_err) {
error_report_err(local_err);
local_err = NULL;
diff --git a/block/stream.c b/block/stream.c
index a6ef840e29..17e240460c 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -73,12 +73,11 @@ static int stream_prepare(Job *job)
s->cor_filter_bs = NULL;
/*
- * bdrv_set_backing_hd() requires that the unfiltered_bs and the COW child
- * of unfiltered_bs is drained. Drain already here and use
- * bdrv_set_backing_hd_drained() instead because the polling during
- * drained_begin() might change the graph, and if we do this only later, we
- * may end up working with the wrong base node (or it might even have gone
- * away by the time we want to use it).
+ * bdrv_set_backing_hd() requires that all block nodes are drained. Drain
+ * already here, because the polling during drained_begin() might change the
+ * graph, and if we do this only later, we may end up working with the wrong
+ * base node (or it might even have gone away by the time we want to use
+ * it).
*/
if (unfiltered_bs_cow) {
bdrv_ref(unfiltered_bs_cow);
@@ -105,7 +104,7 @@ static int stream_prepare(Job *job)
}
bdrv_graph_wrlock();
- bdrv_set_backing_hd_drained(unfiltered_bs, base, &local_err);
+ bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
bdrv_graph_wrunlock();
/*
diff --git a/blockdev.c b/blockdev.c
index 3c53472a23..9f3f42d896 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1587,12 +1587,12 @@ static void external_snapshot_abort(void *opaque)
/*
* Note that state->old_bs would not disappear during the
* write-locked section, because the unref from
- * bdrv_set_backing_hd_drained() only happens at the end of the
- * write-locked section. However, just be explicit about keeping a
- * reference and don't rely on that implicit detail.
+ * bdrv_set_backing_hd() only happens at the end of the write-locked
+ * section. However, just be explicit about keeping a reference and
+ * don't rely on that implicit detail.
*/
bdrv_ref(state->old_bs);
- bdrv_set_backing_hd_drained(state->new_bs, NULL, &error_abort);
+ bdrv_set_backing_hd(state->new_bs, NULL, &error_abort);
/*
* The call to bdrv_set_backing_hd() above returns state->old_bs to
@@ -1776,7 +1776,10 @@ static void drive_backup_action(DriveBackup *backup,
}
if (set_backing_hd) {
- if (bdrv_set_backing_hd(target_bs, source, errp) < 0) {
+ bdrv_graph_wrlock_drained();
+ ret = bdrv_set_backing_hd(target_bs, source, errp);
+ bdrv_graph_wrunlock();
+ if (ret < 0) {
goto unref;
}
}
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 009b9ac946..bcbb624a7b 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -100,12 +100,9 @@ bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
BlockDriverState * coroutine_fn no_co_wrapper
bdrv_co_open_blockdev_ref(BlockdevRef *ref, Error **errp);
-int GRAPH_UNLOCKED
+int GRAPH_WRLOCK
bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
Error **errp);
-int GRAPH_WRLOCK
-bdrv_set_backing_hd_drained(BlockDriverState *bs, BlockDriverState *backing_hd,
- Error **errp);
int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
const char *bdref_key, Error **errp);
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 3369c2c2aa..43b0ba8648 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -193,7 +193,9 @@ static BlockBackend * no_coroutine_fn test_setup(void)
blk_insert_bs(blk, bs, &error_abort);
backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort);
+ bdrv_graph_wrlock_drained();
bdrv_set_backing_hd(bs, backing, &error_abort);
+ bdrv_graph_wrunlock();
bdrv_unref(backing);
bdrv_unref(bs);
@@ -386,7 +388,9 @@ static void test_nested(void)
backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort);
backing_s = backing->opaque;
+ bdrv_graph_wrlock_drained();
bdrv_set_backing_hd(bs, backing, &error_abort);
+ bdrv_graph_wrunlock();
for (outer = 0; outer < DRAIN_TYPE_MAX; outer++) {
for (inner = 0; inner < DRAIN_TYPE_MAX; inner++) {
@@ -733,10 +737,12 @@ static void test_blockjob_common_drain_node(enum drain_type drain_type,
src_overlay = bdrv_new_open_driver(&bdrv_test, "source-overlay",
BDRV_O_RDWR, &error_abort);
+ bdrv_graph_wrlock_drained();
bdrv_set_backing_hd(src_overlay, src, &error_abort);
bdrv_unref(src);
bdrv_set_backing_hd(src, src_backing, &error_abort);
bdrv_unref(src_backing);
+ bdrv_graph_wrunlock();
blk_src = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
blk_insert_bs(blk_src, src_overlay, &error_abort);
@@ -1436,8 +1442,10 @@ static void test_drop_backing_job_commit(Job *job)
TestDropBackingBlockJob *s =
container_of(job, TestDropBackingBlockJob, common.job);
+ bdrv_graph_wrlock_drained();
bdrv_set_backing_hd(s->bs, NULL, &error_abort);
bdrv_set_backing_hd(s->detach_also, NULL, &error_abort);
+ bdrv_graph_wrunlock();
*s->did_complete = true;
}
@@ -1530,7 +1538,9 @@ static void test_blockjob_commit_by_drained_end(void)
snprintf(name, sizeof(name), "parent-node-%i", i);
bs_parents[i] = bdrv_new_open_driver(&bdrv_test, name, BDRV_O_RDWR,
&error_abort);
+ bdrv_graph_wrlock_drained();
bdrv_set_backing_hd(bs_parents[i], bs_child, &error_abort);
+ bdrv_graph_wrunlock();
}
job = block_job_create("job", &test_drop_backing_job_driver, NULL,
@@ -1679,13 +1689,13 @@ static void test_drop_intermediate_poll(void)
job_node = bdrv_new_open_driver(&bdrv_test, "job-node", BDRV_O_RDWR,
&error_abort);
+ bdrv_graph_wrlock_drained();
bdrv_set_backing_hd(job_node, chain[1], &error_abort);
/*
* Establish the chain last, so the chain links are the first
* elements in the BDS.parents lists
*/
- bdrv_graph_wrlock_drained();
for (i = 0; i < 3; i++) {
if (i) {
/* Takes the reference to chain[i - 1] */
diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c
index b077f0e3e3..567db99e4f 100644
--- a/tests/unit/test-bdrv-graph-mod.c
+++ b/tests/unit/test-bdrv-graph-mod.c
@@ -202,9 +202,9 @@ static void test_should_update_child(void)
blk_insert_bs(root, bs, &error_abort);
+ bdrv_graph_wrlock_drained();
bdrv_set_backing_hd(target, bs, &error_abort);
- bdrv_graph_wrlock_drained();
g_assert(target->backing->bs == bs);
bdrv_attach_child(filter, target, "target", &child_of_bds,
BDRV_CHILD_DATA, &error_abort);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 31/48] block-backend: mark blk_drain_all() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (29 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 30/48] block: drop wrapper for bdrv_set_backing_hd_drained() Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 32/48] block/snapshot: mark bdrv_all_delete_snapshot() " Fiona Ebner
` (18 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function blk_drain_all() calls bdrv_drain_all_begin(), which must
be called with the graph unlocked.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
include/system/block-backend-global-state.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/system/block-backend-global-state.h b/include/system/block-backend-global-state.h
index 35b5e8380b..a62dbdf0dc 100644
--- a/include/system/block-backend-global-state.h
+++ b/include/system/block-backend-global-state.h
@@ -79,7 +79,7 @@ void blk_aio_cancel(BlockAIOCB *acb);
int blk_commit_all(void);
bool blk_in_drain(BlockBackend *blk);
void blk_drain(BlockBackend *blk);
-void blk_drain_all(void);
+void GRAPH_UNLOCKED blk_drain_all(void);
void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
BlockdevOnError on_write_error);
bool blk_supports_write_perm(BlockBackend *blk);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 32/48] block/snapshot: mark bdrv_all_delete_snapshot() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (30 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 31/48] block-backend: mark blk_drain_all() as GRAPH_UNLOCKED Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 33/48] block/stream: mark stream_prepare() " Fiona Ebner
` (17 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function bdrv_all_delete_snapshot() calls bdrv_drain_all_begin(),
which must be called with the graph unlocked.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
include/block/snapshot.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 304cc6ea61..2316a43e26 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -90,9 +90,9 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
bool bdrv_all_can_snapshot(bool has_devices, strList *devices,
Error **errp);
-int bdrv_all_delete_snapshot(const char *name,
- bool has_devices, strList *devices,
- Error **errp);
+int GRAPH_UNLOCKED
+bdrv_all_delete_snapshot(const char *name, bool has_devices, strList *devices,
+ Error **errp);
int bdrv_all_goto_snapshot(const char *name,
bool has_devices, strList *devices,
Error **errp);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 33/48] block/stream: mark stream_prepare() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (31 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 32/48] block/snapshot: mark bdrv_all_delete_snapshot() " Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 34/48] block: mark bdrv_reopen_queue() and bdrv_reopen_multiple() " Fiona Ebner
` (16 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function stream_prepare() calls bdrv_drain_all_begin(), which
must be called with the graph unlocked.
Also mark the JobDriver's prepare() callback as GRAPH_UNLOCKED_PTR,
because that is the callback via which stream_prepare() is reached.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block/stream.c | 2 +-
include/qemu/job.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 17e240460c..c0616b69e2 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -51,7 +51,7 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
return blk_co_preadv(blk, offset, bytes, NULL, BDRV_REQ_PREFETCH);
}
-static int stream_prepare(Job *job)
+static int GRAPH_UNLOCKED stream_prepare(Job *job)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockDriverState *unfiltered_bs;
diff --git a/include/qemu/job.h b/include/qemu/job.h
index a5a04155ea..bb8ee766ef 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -263,7 +263,7 @@ struct JobDriver {
* This callback will not be invoked if the job has already failed.
* If it fails, abort and then clean will be called.
*/
- int (*prepare)(Job *job);
+ int GRAPH_UNLOCKED_PTR (*prepare)(Job *job);
/**
* If the callback is not NULL, it will be invoked when all the jobs
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 34/48] block: mark bdrv_reopen_queue() and bdrv_reopen_multiple() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (32 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 33/48] block/stream: mark stream_prepare() " Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 35/48] block: mark bdrv_inactivate() as GRAPH_RDLOCK and move drain to callers Fiona Ebner
` (15 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function bdrv_reopen_queue() can call bdrv_drain_all_begin(),
which must be called with the graph unlocked.
The function bdrv_reopen_multiple() calls bdrv_reopen_prepare() which
must be called with the graph unlocked.
To mark bdrv_reopen_queue() as GRAPH_UNLOCKED, it is necessary to make
the locked section in reopen_backing_file() shorter.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block/replication.c | 3 ++-
include/block/block-global-state.h | 9 +++++----
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/block/replication.c b/block/replication.c
index 83978b61f5..3a431e908c 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -364,14 +364,15 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable,
BlockReopenQueue *reopen_queue = NULL;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+ bdrv_graph_rdlock_main_loop();
/*
* s->hidden_disk and s->secondary_disk may not be set yet, as they will
* only be set after the children are writable.
*/
hidden_disk = bs->file->bs->backing;
secondary_disk = hidden_disk->bs->backing;
+ bdrv_graph_rdunlock_main_loop();
if (writable) {
s->orig_hidden_read_only = bdrv_is_read_only(hidden_disk->bs);
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index bcbb624a7b..f25c65c1b4 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -121,11 +121,12 @@ BlockDriverState *bdrv_new_open_driver_opts(BlockDriver *drv,
Error **errp);
BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
int flags, Error **errp);
-BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
- BlockDriverState *bs, QDict *options,
- bool keep_old_opts);
+BlockReopenQueue * GRAPH_UNLOCKED
+bdrv_reopen_queue(BlockReopenQueue *bs_queue, BlockDriverState *bs,
+ QDict *options, bool keep_old_opts);
void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue);
-int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
+int GRAPH_UNLOCKED
+bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
int bdrv_reopen(BlockDriverState *bs, QDict *opts, bool keep_old_opts,
Error **errp);
int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 35/48] block: mark bdrv_inactivate() as GRAPH_RDLOCK and move drain to callers
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (33 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 34/48] block: mark bdrv_reopen_queue() and bdrv_reopen_multiple() " Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 36/48] block: mark bdrv_inactivate_all() as GRAPH_UNLOCKED Fiona Ebner
` (14 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function bdrv_inactivate() calls bdrv_drain_all_begin(), which
needs to be called with the graph unlocked, so either
bdrv_inactivate() should be marked as GRAPH_UNLOCKED or the drain
needs to be moved to the callers. The caller in
qmp_blockdev_set_active() requires that the locked section covers
bdrv_find_node() too, so the latter alternative is chosen.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block.c | 14 ++----
blockdev.c | 75 ++++++++++++++++++------------
include/block/block-global-state.h | 2 +-
3 files changed, 51 insertions(+), 40 deletions(-)
diff --git a/block.c b/block.c
index ae1b122fa8..6f470aac2f 100644
--- a/block.c
+++ b/block.c
@@ -7059,31 +7059,25 @@ bdrv_inactivate_recurse(BlockDriverState *bs, bool top_level)
return 0;
}
+/* All block nodes must be drained. */
int bdrv_inactivate(BlockDriverState *bs, Error **errp)
{
int ret;
GLOBAL_STATE_CODE();
- bdrv_drain_all_begin();
- bdrv_graph_rdlock_main_loop();
-
if (bdrv_has_bds_parent(bs, true)) {
error_setg(errp, "Node has active parent node");
- ret = -EPERM;
- goto out;
+ return -EPERM;
}
ret = bdrv_inactivate_recurse(bs, true);
if (ret < 0) {
error_setg_errno(errp, -ret, "Failed to inactivate node");
- goto out;
+ return ret;
}
-out:
- bdrv_graph_rdunlock_main_loop();
- bdrv_drain_all_end();
- return ret;
+ return 0;
}
int bdrv_inactivate_all(void)
diff --git a/blockdev.c b/blockdev.c
index 9f3f42d896..b451fee6e1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1421,7 +1421,7 @@ static void external_snapshot_action(TransactionAction *action,
bdrv_graph_rdunlock_main_loop();
/* Paired with .clean() */
bdrv_drained_begin(state->old_bs);
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+ bdrv_graph_rdlock_main_loop();
/* Make sure the associated bs did not change with the drain. */
check_bs = bdrv_lookup_bs(device, node_name, errp);
@@ -1430,18 +1430,18 @@ static void external_snapshot_action(TransactionAction *action,
error_setg(errp, "Block node of device '%s' unexpectedly changed",
device);
} /* else errp is already set */
- return;
+ goto unlock;
}
if (!bdrv_is_inserted(state->old_bs)) {
error_setg(errp, "Device '%s' has no medium",
bdrv_get_device_or_node_name(state->old_bs));
- return;
+ goto unlock;
}
if (bdrv_op_is_blocked(state->old_bs,
BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) {
- return;
+ goto unlock;
}
if (!bdrv_is_read_only(state->old_bs)) {
@@ -1449,7 +1449,7 @@ static void external_snapshot_action(TransactionAction *action,
if (ret < 0) {
error_setg_errno(errp, -ret, "Write to node '%s' failed",
bdrv_get_device_or_node_name(state->old_bs));
- return;
+ goto unlock;
}
}
@@ -1461,13 +1461,13 @@ static void external_snapshot_action(TransactionAction *action,
if (node_name && !snapshot_node_name) {
error_setg(errp, "New overlay node-name missing");
- return;
+ goto unlock;
}
if (snapshot_node_name &&
bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
error_setg(errp, "New overlay node-name already in use");
- return;
+ goto unlock;
}
flags = state->old_bs->open_flags;
@@ -1480,7 +1480,7 @@ static void external_snapshot_action(TransactionAction *action,
int64_t size = bdrv_getlength(state->old_bs);
if (size < 0) {
error_setg_errno(errp, -size, "bdrv_getlength failed");
- return;
+ goto unlock;
}
bdrv_refresh_filename(state->old_bs);
@@ -1491,7 +1491,7 @@ static void external_snapshot_action(TransactionAction *action,
if (local_err) {
error_propagate(errp, local_err);
- return;
+ goto unlock;
}
}
@@ -1507,7 +1507,7 @@ static void external_snapshot_action(TransactionAction *action,
/* We will manually add the backing_hd field to the bs later */
if (!state->new_bs) {
- return;
+ goto unlock;
}
/*
@@ -1518,22 +1518,22 @@ static void external_snapshot_action(TransactionAction *action,
bdrv_get_cumulative_perm(state->new_bs, &perm, &shared);
if (perm & BLK_PERM_CONSISTENT_READ) {
error_setg(errp, "The overlay is already in use");
- return;
+ goto unlock;
}
if (state->new_bs->drv->is_filter) {
error_setg(errp, "Filters cannot be used as overlays");
- return;
+ goto unlock;
}
if (bdrv_cow_child(state->new_bs)) {
error_setg(errp, "The overlay already has a backing image");
- return;
+ goto unlock;
}
if (!state->new_bs->drv->supports_backing) {
error_setg(errp, "The overlay does not support backing images");
- return;
+ goto unlock;
}
/*
@@ -1546,17 +1546,23 @@ static void external_snapshot_action(TransactionAction *action,
* to keep this working.
*/
if (bdrv_is_inactive(state->old_bs) && !bdrv_is_inactive(state->new_bs)) {
+ bdrv_graph_rdunlock_main_loop();
+ bdrv_drain_all_begin();
+ bdrv_graph_rdlock_main_loop();
ret = bdrv_inactivate(state->new_bs, errp);
+ bdrv_drain_all_end();
if (ret < 0) {
- return;
+ goto unlock;
}
}
ret = bdrv_append(state->new_bs, state->old_bs, errp);
if (ret < 0) {
- return;
+ goto unlock;
}
state->overlay_appended = true;
+unlock:
+ bdrv_graph_rdunlock_main_loop();
}
static void external_snapshot_commit(void *opaque)
@@ -3520,10 +3526,10 @@ void qmp_blockdev_del(const char *node_name, Error **errp)
void qmp_blockdev_set_active(const char *node_name, bool active, Error **errp)
{
+ BlockDriverState *bs;
int ret;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
if (!node_name) {
if (active) {
@@ -3534,19 +3540,30 @@ void qmp_blockdev_set_active(const char *node_name, bool active, Error **errp)
error_setg_errno(errp, -ret, "Failed to inactivate all nodes");
}
}
- } else {
- BlockDriverState *bs = bdrv_find_node(node_name);
- if (!bs) {
- error_setg(errp, "Failed to find node with node-name='%s'",
- node_name);
- return;
- }
+ return;
+ }
- if (active) {
- bdrv_activate(bs, errp);
- } else {
- bdrv_inactivate(bs, errp);
- }
+ if (!active) {
+ bdrv_drain_all_begin();
+ }
+ bdrv_graph_rdlock_main_loop();
+
+ bs = bdrv_find_node(node_name);
+ if (!bs) {
+ error_setg(errp, "Failed to find node with node-name='%s'",
+ node_name);
+ goto unlock;
+ }
+ if (active) {
+ bdrv_activate(bs, errp);
+ } else {
+ bdrv_inactivate(bs, errp);
+ }
+
+unlock:
+ bdrv_graph_rdunlock_main_loop();
+ if (!active) {
+ bdrv_drain_all_end();
}
}
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index f25c65c1b4..a641beb270 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -183,7 +183,7 @@ bdrv_activate(BlockDriverState *bs, Error **errp);
int coroutine_fn no_co_wrapper_bdrv_rdlock
bdrv_co_activate(BlockDriverState *bs, Error **errp);
-int no_coroutine_fn
+int no_coroutine_fn GRAPH_RDLOCK
bdrv_inactivate(BlockDriverState *bs, Error **errp);
void bdrv_activate_all(Error **errp);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 36/48] block: mark bdrv_inactivate_all() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (34 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 35/48] block: mark bdrv_inactivate() as GRAPH_RDLOCK and move drain to callers Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 37/48] block: mark blk_remove_bs() " Fiona Ebner
` (13 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function bdrv_inactivate_all() calls bdrv_drain_all_begin(), which
must be called with the graph unlocked.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
include/block/block-global-state.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index a641beb270..eec92a98da 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -187,7 +187,7 @@ int no_coroutine_fn GRAPH_RDLOCK
bdrv_inactivate(BlockDriverState *bs, Error **errp);
void bdrv_activate_all(Error **errp);
-int bdrv_inactivate_all(void);
+int GRAPH_UNLOCKED bdrv_inactivate_all(void);
int bdrv_flush_all(void);
void bdrv_close_all(void);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 37/48] block: mark blk_remove_bs() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (35 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 36/48] block: mark bdrv_inactivate_all() as GRAPH_UNLOCKED Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 38/48] block: mark blk_drain() " Fiona Ebner
` (12 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function blk_remove_bs() calls bdrv_graph_wrlock_drained() and can
also call bdrv_drained_begin(), both of which which must be called with
the graph unlocked.
Marking blk_remove_bs() as GRAPH_UNLOCKED requires temporarily
unlocking in hmp_drive_del().
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block/monitor/block-hmp-cmds.c | 15 ++++++++++-----
include/system/block-backend-global-state.h | 2 +-
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 6919a49bf5..282d1c386e 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -144,7 +144,7 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
Error *local_err = NULL;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+ bdrv_graph_rdlock_main_loop();
bs = bdrv_find_node(id);
if (bs) {
@@ -152,29 +152,31 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
if (local_err) {
error_report_err(local_err);
}
- return;
+ goto unlock;
}
blk = blk_by_name(id);
if (!blk) {
error_report("Device '%s' not found", id);
- return;
+ goto unlock;
}
if (!blk_legacy_dinfo(blk)) {
error_report("Deleting device added with blockdev-add"
" is not supported");
- return;
+ goto unlock;
}
bs = blk_bs(blk);
if (bs) {
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
error_report_err(local_err);
- return;
+ goto unlock;
}
+ bdrv_graph_rdunlock_main_loop();
blk_remove_bs(blk);
+ bdrv_graph_rdlock_main_loop();
}
/* Make the BlockBackend and the attached BlockDriverState anonymous */
@@ -191,6 +193,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
} else {
blk_unref(blk);
}
+
+unlock:
+ bdrv_graph_rdunlock_main_loop();
}
void hmp_commit(Monitor *mon, const QDict *qdict)
diff --git a/include/system/block-backend-global-state.h b/include/system/block-backend-global-state.h
index a62dbdf0dc..1a134083b7 100644
--- a/include/system/block-backend-global-state.h
+++ b/include/system/block-backend-global-state.h
@@ -55,7 +55,7 @@ void monitor_remove_blk(BlockBackend *blk);
BlockBackendPublic *blk_get_public(BlockBackend *blk);
-void blk_remove_bs(BlockBackend *blk);
+void GRAPH_UNLOCKED 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 GRAPH_RDLOCK bdrv_has_blk(BlockDriverState *bs);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 38/48] block: mark blk_drain() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (36 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 37/48] block: mark blk_remove_bs() " Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 39/48] block-backend: mark blk_io_limits_disable() " Fiona Ebner
` (11 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function blk_drain() calls bdrv_drained_begin(), which must be
called with the graph unlocked.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
include/system/block-backend-global-state.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/system/block-backend-global-state.h b/include/system/block-backend-global-state.h
index 1a134083b7..f6ec1174e6 100644
--- a/include/system/block-backend-global-state.h
+++ b/include/system/block-backend-global-state.h
@@ -78,7 +78,7 @@ int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags);
void blk_aio_cancel(BlockAIOCB *acb);
int blk_commit_all(void);
bool blk_in_drain(BlockBackend *blk);
-void blk_drain(BlockBackend *blk);
+void GRAPH_UNLOCKED blk_drain(BlockBackend *blk);
void GRAPH_UNLOCKED blk_drain_all(void);
void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
BlockdevOnError on_write_error);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 39/48] block-backend: mark blk_io_limits_disable() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (37 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 38/48] block: mark blk_drain() " Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 40/48] block/commit: mark commit_abort() " Fiona Ebner
` (10 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function blk_io_limits_disable() calls bdrv_drained_begin(), which
must be called with the graph unlocked.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
include/system/block-backend-global-state.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/system/block-backend-global-state.h b/include/system/block-backend-global-state.h
index f6ec1174e6..c3849640df 100644
--- a/include/system/block-backend-global-state.h
+++ b/include/system/block-backend-global-state.h
@@ -109,7 +109,7 @@ int blk_probe_blocksizes(BlockBackend *blk, BlockSizes *bsz);
int blk_probe_geometry(BlockBackend *blk, HDGeometry *geo);
void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg);
-void blk_io_limits_disable(BlockBackend *blk);
+void GRAPH_UNLOCKED blk_io_limits_disable(BlockBackend *blk);
void blk_io_limits_enable(BlockBackend *blk, const char *group);
void blk_io_limits_update_group(BlockBackend *blk, const char *group);
void blk_set_force_allow_inactivate(BlockBackend *blk);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 40/48] block/commit: mark commit_abort() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (38 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 39/48] block-backend: mark blk_io_limits_disable() " Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 41/48] block: mark bdrv_new() " Fiona Ebner
` (9 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function commit_abort() calls bdrv_drained_begin(), which must be
called with the graph unlocked.
Also mark the JobDriver's abort() callback as GRAPH_UNLOCKED_PTR,
because that is the callback via which commit_abort() is reached.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block/commit.c | 2 +-
include/qemu/job.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index 7496cf732e..0d9e1a16d7 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -68,7 +68,7 @@ static int commit_prepare(Job *job)
s->backing_mask_protocol);
}
-static void commit_abort(Job *job)
+static void GRAPH_UNLOCKED commit_abort(Job *job)
{
CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
BlockDriverState *top_bs = blk_bs(s->top);
diff --git a/include/qemu/job.h b/include/qemu/job.h
index bb8ee766ef..ead31578d3 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -283,7 +283,7 @@ struct JobDriver {
* All jobs will complete with a call to either .commit() or .abort() but
* never both.
*/
- void (*abort)(Job *job);
+ void GRAPH_UNLOCKED_PTR (*abort)(Job *job);
/**
* If the callback is not NULL, it will be invoked after a call to either
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 41/48] block: mark bdrv_new() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (39 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 40/48] block/commit: mark commit_abort() " Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-07-01 16:55 ` Kevin Wolf
2025-05-30 15:11 ` [PATCH v4 42/48] block: mark bdrv_replace_child_bs() " Fiona Ebner
` (8 subsequent siblings)
49 siblings, 1 reply; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function bdrv_new() calls bdrv_drained_begin(), which must be
called with the graph unlocked.
Marking bdrv_new() as GRAPH_UNLOCKED requires making the locked
section in bdrv_open_inherit() shorter.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
I'm not sure if the TODO comment is only intended for the
lower half of the function, i.e. is moving it like this okay?
block.c | 7 ++++---
include/block/block-global-state.h | 2 +-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index 6f470aac2f..1b9c99dda9 100644
--- a/block.c
+++ b/block.c
@@ -3995,10 +3995,8 @@ bdrv_open_inherit(const char *filename, const char *reference, QDict *options,
GLOBAL_STATE_CODE();
assert(!qemu_in_coroutine());
- /* TODO We'll eventually have to take a writer lock in this function */
- GRAPH_RDLOCK_GUARD_MAINLOOP();
-
if (reference) {
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
bool options_non_empty = options ? qdict_size(options) : false;
qobject_unref(options);
@@ -4019,6 +4017,9 @@ bdrv_open_inherit(const char *filename, const char *reference, QDict *options,
bs = bdrv_new();
+ /* TODO We'll eventually have to take a writer lock in this function */
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
/* NULL means an empty set of options */
if (options == NULL) {
options = qdict_new();
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index eec92a98da..b1f826dca6 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -67,7 +67,7 @@ int co_wrapper bdrv_create(BlockDriver *drv, const char *filename,
int coroutine_fn GRAPH_UNLOCKED
bdrv_co_create_file(const char *filename, QemuOpts *opts, Error **errp);
-BlockDriverState *bdrv_new(void);
+BlockDriverState * GRAPH_UNLOCKED bdrv_new(void);
int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
Error **errp);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* Re: [PATCH v4 41/48] block: mark bdrv_new() as GRAPH_UNLOCKED
2025-05-30 15:11 ` [PATCH v4 41/48] block: mark bdrv_new() " Fiona Ebner
@ 2025-07-01 16:55 ` Kevin Wolf
0 siblings, 0 replies; 60+ messages in thread
From: Kevin Wolf @ 2025-07-01 16:55 UTC (permalink / raw)
To: Fiona Ebner
Cc: qemu-block, qemu-devel, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Am 30.05.2025 um 17:11 hat Fiona Ebner geschrieben:
> The function bdrv_new() calls bdrv_drained_begin(), which must be
> called with the graph unlocked.
>
> Marking bdrv_new() as GRAPH_UNLOCKED requires making the locked
> section in bdrv_open_inherit() shorter.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> I'm not sure if the TODO comment is only intended for the
> lower half of the function, i.e. is moving it like this okay?
The thing that should require locking is when you attach the new node to
something, which is after the place where you moved it to. Currently,
these functions take the lock internally, and I'm not sure if that can
possibly be changed because opening an image usually involves a mix of
I/O to read image metadata (which is incompatible with having a writer
lock) and graph changing operations. It's not clear if this TODO can
ever be resolved...
But I'm not sure if bdrv_new() really should be GRAPH_UNLOCKED. We know
that we don't have any active I/O for a node that we only just created
and that isn't even linked in the global list yet. So maybe the other
option is using bdrv_do_drained_begin_quiesce(bs, NULL) in bdrv_new()
instead? Then callers can hold the lock if they want to.
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 42/48] block: mark bdrv_replace_child_bs() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (40 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 41/48] block: mark bdrv_new() " Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 43/48] block: mark bdrv_insert_node() " Fiona Ebner
` (7 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function bdrv_replace_child_bs() calls bdrv_drained_begin() which
must be called with the graph unlocked.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
include/block/block-global-state.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index b1f826dca6..dfdf5e187f 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -74,8 +74,8 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
int GRAPH_WRLOCK
bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, Error **errp);
-int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
- Error **errp);
+int GRAPH_UNLOCKED
+bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs, Error **errp);
BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
int flags, Error **errp);
int bdrv_drop_filter(BlockDriverState *bs, Error **errp);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 43/48] block: mark bdrv_insert_node() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (41 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 42/48] block: mark bdrv_replace_child_bs() " Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 44/48] block: mark bdrv_drop_intermediate() " Fiona Ebner
` (6 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function bdrv_insert_node() calls bdrv_drained_begin() which must
be called with the graph unlocked.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
include/block/block-global-state.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index dfdf5e187f..55f84c1f8f 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -76,8 +76,9 @@ bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, Error **errp);
int GRAPH_UNLOCKED
bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs, Error **errp);
-BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
- int flags, Error **errp);
+BlockDriverState * GRAPH_UNLOCKED
+bdrv_insert_node(BlockDriverState *bs, QDict *node_options, int flags,
+ Error **errp);
int bdrv_drop_filter(BlockDriverState *bs, Error **errp);
BdrvChild * no_coroutine_fn
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 44/48] block: mark bdrv_drop_intermediate() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (42 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 43/48] block: mark bdrv_insert_node() " Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 45/48] block: mark bdrv_close_all() " Fiona Ebner
` (5 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function bdrv_drop_intermediate() calls bdrv_drained_begin(),
which must be called with the graph unlocked.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
include/block/block-global-state.h | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 55f84c1f8f..ad78fa6498 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -143,9 +143,10 @@ int bdrv_commit(BlockDriverState *bs);
int GRAPH_RDLOCK bdrv_make_empty(BdrvChild *c, Error **errp);
void bdrv_register(BlockDriver *bdrv);
-int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
- const char *backing_file_str,
- bool backing_mask_protocol);
+int GRAPH_UNLOCKED
+bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
+ const char *backing_file_str,
+ bool backing_mask_protocol);
BlockDriverState * GRAPH_RDLOCK
bdrv_find_overlay(BlockDriverState *active, BlockDriverState *bs);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 45/48] block: mark bdrv_close_all() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (43 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 44/48] block: mark bdrv_drop_intermediate() " Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 46/48] block: mark bdrv_close() " Fiona Ebner
` (4 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function bdrv_close_all() calls bdrv_drain_all(), which must be
called with the graph unlocked.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
include/block/block-global-state.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index ad78fa6498..fb3bb6f707 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -192,7 +192,7 @@ void bdrv_activate_all(Error **errp);
int GRAPH_UNLOCKED bdrv_inactivate_all(void);
int bdrv_flush_all(void);
-void bdrv_close_all(void);
+void GRAPH_UNLOCKED bdrv_close_all(void);
void GRAPH_UNLOCKED bdrv_drain_all_begin(void);
void bdrv_drain_all_begin_nopoll(void);
void bdrv_drain_all_end(void);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 46/48] block: mark bdrv_close() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (44 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 45/48] block: mark bdrv_close_all() " Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 47/48] block: mark bdrv_open_child_common() and its callers GRAPH_UNLOCKED Fiona Ebner
` (3 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The functions blk_log_writes_close(), blkverify_close(),
quorum_close(), vmdk_close() via vmdk_free_extents(), and other
bdrv_close() implementations call bdrv_graph_wrlock_drained(), which
must be called with the graph unlocked. They are reached via the
BlockDriver's bdrv_close() callback and the bdrv_close() wrapper,
which are also marked as GRAPH_UNLOCKED_PTR and GRAPH_UNLOCKED.
Furthermore, the function bdrv_close() also calls bdrv_drained_begin()
and bdrv_graph_wrlock_drained(), so there are additional reasons for
marking it GRAPH_UNLOCKED.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block.c | 2 +-
include/block/block_int-common.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 1b9c99dda9..a4b79d70db 100644
--- a/block.c
+++ b/block.c
@@ -5148,7 +5148,7 @@ static void GRAPH_UNLOCKED bdrv_reopen_abort(BDRVReopenState *reopen_state)
}
-static void bdrv_close(BlockDriverState *bs)
+static void GRAPH_UNLOCKED bdrv_close(BlockDriverState *bs)
{
BdrvAioNotifier *ban, *ban_next;
BdrvChild *child, *next;
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index e96c6a6a03..034c0634c8 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -248,7 +248,7 @@ struct BlockDriver {
int GRAPH_UNLOCKED_PTR (*bdrv_open)(
BlockDriverState *bs, QDict *options, int flags, Error **errp);
- void (*bdrv_close)(BlockDriverState *bs);
+ void GRAPH_UNLOCKED_PTR (*bdrv_close)(BlockDriverState *bs);
int coroutine_fn GRAPH_UNLOCKED_PTR (*bdrv_co_create)(
BlockdevCreateOptions *opts, Error **errp);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 47/48] block: mark bdrv_open_child_common() and its callers GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (45 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 46/48] block: mark bdrv_close() " Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-05-30 15:11 ` [PATCH v4 48/48] blockjob: mark block_job_remove_all_bdrv() as GRAPH_UNLOCKED Fiona Ebner
` (2 subsequent siblings)
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function bdrv_open_child_common() calls
bdrv_graph_wrlock_drained(), which must be called with the graph
unlocked. Mark it and its two callers bdrv_open_file_child() and
bdrv_open_child() as GRAPH_UNLOCKED. This requires temporarily
unlocking in vmdk_parse_extents() and making the locked section
shorter in vmdk_open().
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block.c | 13 ++++++-------
block/vmdk.c | 6 ++++--
include/block/block-global-state.h | 9 +++++----
3 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/block.c b/block.c
index a4b79d70db..132faf10f0 100644
--- a/block.c
+++ b/block.c
@@ -3767,13 +3767,12 @@ done:
return bs;
}
-static BdrvChild *bdrv_open_child_common(const char *filename,
- QDict *options, const char *bdref_key,
- BlockDriverState *parent,
- const BdrvChildClass *child_class,
- BdrvChildRole child_role,
- bool allow_none, bool parse_filename,
- Error **errp)
+static BdrvChild * GRAPH_UNLOCKED
+bdrv_open_child_common(const char *filename, QDict *options,
+ const char *bdref_key, BlockDriverState *parent,
+ const BdrvChildClass *child_class,
+ BdrvChildRole child_role, bool allow_none,
+ bool parse_filename, Error **errp)
{
BlockDriverState *bs;
BdrvChild *child;
diff --git a/block/vmdk.c b/block/vmdk.c
index 04986c8d55..7b98debc2b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1229,9 +1229,11 @@ vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
extent_role |= BDRV_CHILD_METADATA;
}
+ bdrv_graph_rdunlock_main_loop();
extent_file = bdrv_open_child(extent_path, options, extent_opt_prefix,
bs, &child_of_bds, extent_role, false,
&local_err);
+ bdrv_graph_rdlock_main_loop();
g_free(extent_path);
if (!extent_file) {
error_propagate(errp, local_err);
@@ -1352,13 +1354,13 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
BDRVVmdkState *s = bs->opaque;
uint32_t magic;
- GRAPH_RDLOCK_GUARD_MAINLOOP();
-
ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
if (ret < 0) {
return ret;
}
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
buf = vmdk_read_desc(bs->file, 0, errp);
if (!buf) {
return -EINVAL;
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index fb3bb6f707..49e54fb4d3 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -81,7 +81,7 @@ bdrv_insert_node(BlockDriverState *bs, QDict *node_options, int flags,
Error **errp);
int bdrv_drop_filter(BlockDriverState *bs, Error **errp);
-BdrvChild * no_coroutine_fn
+BdrvChild * no_coroutine_fn GRAPH_UNLOCKED
bdrv_open_child(const char *filename, QDict *options, const char *bdref_key,
BlockDriverState *parent, const BdrvChildClass *child_class,
BdrvChildRole child_role, bool allow_none, Error **errp);
@@ -91,9 +91,10 @@ bdrv_co_open_child(const char *filename, QDict *options, const char *bdref_key,
BlockDriverState *parent, const BdrvChildClass *child_class,
BdrvChildRole child_role, bool allow_none, Error **errp);
-int bdrv_open_file_child(const char *filename,
- QDict *options, const char *bdref_key,
- BlockDriverState *parent, Error **errp);
+int GRAPH_UNLOCKED
+bdrv_open_file_child(const char *filename, QDict *options,
+ const char *bdref_key, BlockDriverState *parent,
+ Error **errp);
BlockDriverState * no_coroutine_fn
bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v4 48/48] blockjob: mark block_job_remove_all_bdrv() as GRAPH_UNLOCKED
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (46 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 47/48] block: mark bdrv_open_child_common() and its callers GRAPH_UNLOCKED Fiona Ebner
@ 2025-05-30 15:11 ` Fiona Ebner
2025-06-03 14:54 ` [PATCH v4 00/48] block: do not drain while holding the graph lock Kevin Wolf
2025-07-01 17:16 ` Kevin Wolf
49 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-05-30 15:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, kwolf, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
The function block_job_remove_all_bdrv() calls
bdrv_graph_wrlock_drained(), which must be called with the graph
unlocked.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
include/block/blockjob.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 990f3e179a..85284cb25e 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -151,7 +151,7 @@ block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
* Remove all BlockDriverStates from the list of nodes that are involved in the
* job. This removes the blockers added with block_job_add_bdrv().
*/
-void block_job_remove_all_bdrv(BlockJob *job);
+void GRAPH_UNLOCKED block_job_remove_all_bdrv(BlockJob *job);
/**
* block_job_has_bdrv:
--
2.39.5
^ permalink raw reply related [flat|nested] 60+ messages in thread* Re: [PATCH v4 00/48] block: do not drain while holding the graph lock
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (47 preceding siblings ...)
2025-05-30 15:11 ` [PATCH v4 48/48] blockjob: mark block_job_remove_all_bdrv() as GRAPH_UNLOCKED Fiona Ebner
@ 2025-06-03 14:54 ` Kevin Wolf
2025-06-04 7:38 ` Fiona Ebner
2025-07-01 17:16 ` Kevin Wolf
49 siblings, 1 reply; 60+ messages in thread
From: Kevin Wolf @ 2025-06-03 14:54 UTC (permalink / raw)
To: Fiona Ebner
Cc: qemu-block, qemu-devel, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Am 30.05.2025 um 17:10 hat Fiona Ebner geschrieben:
> Previous discussions:
> v3: [0]
> v2: [1]
> v1: [2]
>
> Changes in v4:
> * Document requirement to drain all block nodes for affected
> functions.
> * Also cover the generated bdrv_co_unref_child().
> * Remove now superfluous drain_bs variable in bdrv_set_backing_hd().
> * Mark bdrv_graph_wrlock_drained() wrapper as GRAPH_UNLOCKED.
> * Unify bdrv_set_backing_hd() with its drained_variant.
> * Mark more functions up the call-stack as GRAPH_UNLOCKED. This is
> almost all of the new patches in the latter half of the series, most
> do not require substantial changes, but there are a few where
> something needed to be done. I did not mark functions outside the
> block layer like qemu_cleanup(), save_snapshot(), qmp_xyz(), etc.
> and also not functions that explicitly do a rdunlock_main_loop()
> before calling a function that is GRAPH_UNLOCKED.
>
> There were no changes for patches 01/48-09/48 and 17/48-23/48, endpoints
> inclusive. All patches starting from 25/48 are new in v4.
This is starting to become a little unmanageable. :-)
I'm sure we could keep adding more and more cleanup patches with each
version of the series, but we don't really have to let the first fixes
wait for all cleanups and fix the whole world in a single series.
So I decided to start with a prefix of this series and applied
patches 1-22, which all had received review before and were easy to
compare against the previous version. If you don't object, I'd send a
pull request for those without waiting for the rest.
If the other patches still need some changes, you can start a new patch
series at v1 containing the remaining patches. (No need to resend them
now, though, I can review them in this series.)
Does this make sense to you?
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH v4 00/48] block: do not drain while holding the graph lock
2025-06-03 14:54 ` [PATCH v4 00/48] block: do not drain while holding the graph lock Kevin Wolf
@ 2025-06-04 7:38 ` Fiona Ebner
0 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-06-04 7:38 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, qemu-devel, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Am 03.06.25 um 16:54 schrieb Kevin Wolf:
> Am 30.05.2025 um 17:10 hat Fiona Ebner geschrieben:
>> There were no changes for patches 01/48-09/48 and 17/48-23/48, endpoints
>> inclusive. All patches starting from 25/48 are new in v4.
>
> This is starting to become a little unmanageable. :-)
>
> I'm sure we could keep adding more and more cleanup patches with each
> version of the series, but we don't really have to let the first fixes
> wait for all cleanups and fix the whole world in a single series.
>
> So I decided to start with a prefix of this series and applied
> patches 1-22, which all had received review before and were easy to
> compare against the previous version. If you don't object, I'd send a
> pull request for those without waiting for the rest.
>
> If the other patches still need some changes, you can start a new patch
> series at v1 containing the remaining patches. (No need to resend them
> now, though, I can review them in this series.)
>
> Does this make sense to you?
Yes, that sounds very good! I'll try to keep this in mind in the future
and send follow-up series instead in such cases :)
Best Regards,
Fiona
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 00/48] block: do not drain while holding the graph lock
2025-05-30 15:10 [PATCH v4 00/48] block: do not drain while holding the graph lock Fiona Ebner
` (48 preceding siblings ...)
2025-06-03 14:54 ` [PATCH v4 00/48] block: do not drain while holding the graph lock Kevin Wolf
@ 2025-07-01 17:16 ` Kevin Wolf
2025-07-14 13:43 ` Kevin Wolf
49 siblings, 1 reply; 60+ messages in thread
From: Kevin Wolf @ 2025-07-01 17:16 UTC (permalink / raw)
To: Fiona Ebner
Cc: qemu-block, qemu-devel, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Am 30.05.2025 um 17:10 hat Fiona Ebner geschrieben:
> This series is an attempt to fix a deadlock issue reported by Andrey
> here [3].
>
> bdrv_drained_begin() polls and is not allowed to be called with the
> block graph lock held. Mark the function as GRAPH_UNLOCKED.
>
> This alone does not catch the issue reported by Andrey, because there
> is a bdrv_graph_rdunlock_main_loop() before bdrv_drained_begin() in
> the function bdrv_change_aio_context(). That unlock is of course
> ineffective if the exclusive lock is held, but it prevents TSA from
> finding the issue.
>
> Thus the bdrv_drained_begin() call from inside
> bdrv_change_aio_context() needs to be moved up the call stack before
> acquiring the locks. This is the bulk of the series.
>
> Granular draining is not trivially possible, because many of the
> affected functions can recursively call themselves.
>
> In place where bdrv_drained_begin() calls were removed, assertions
> are added, checking the quiesced_counter to ensure that the nodes
> already got drained further up in the call stack.
I finished review for this series. I had some minor comments on patches
24, 27 and 41. Once we agree what to do there, I can probably just make
any changes myself while applying.
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH v4 00/48] block: do not drain while holding the graph lock
2025-07-01 17:16 ` Kevin Wolf
@ 2025-07-14 13:43 ` Kevin Wolf
2025-07-15 13:24 ` Fiona Ebner
0 siblings, 1 reply; 60+ messages in thread
From: Kevin Wolf @ 2025-07-14 13:43 UTC (permalink / raw)
To: Fiona Ebner
Cc: qemu-block, qemu-devel, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Am 01.07.2025 um 19:16 hat Kevin Wolf geschrieben:
> Am 30.05.2025 um 17:10 hat Fiona Ebner geschrieben:
> > This series is an attempt to fix a deadlock issue reported by Andrey
> > here [3].
> >
> > bdrv_drained_begin() polls and is not allowed to be called with the
> > block graph lock held. Mark the function as GRAPH_UNLOCKED.
> >
> > This alone does not catch the issue reported by Andrey, because there
> > is a bdrv_graph_rdunlock_main_loop() before bdrv_drained_begin() in
> > the function bdrv_change_aio_context(). That unlock is of course
> > ineffective if the exclusive lock is held, but it prevents TSA from
> > finding the issue.
> >
> > Thus the bdrv_drained_begin() call from inside
> > bdrv_change_aio_context() needs to be moved up the call stack before
> > acquiring the locks. This is the bulk of the series.
> >
> > Granular draining is not trivially possible, because many of the
> > affected functions can recursively call themselves.
> >
> > In place where bdrv_drained_begin() calls were removed, assertions
> > are added, checking the quiesced_counter to ensure that the nodes
> > already got drained further up in the call stack.
>
> I finished review for this series. I had some minor comments on patches
> 24, 27 and 41. Once we agree what to do there, I can probably just make
> any changes myself while applying.
I don't see any objections, so I just applied this and made all the
changes I had suggested.
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 00/48] block: do not drain while holding the graph lock
2025-07-14 13:43 ` Kevin Wolf
@ 2025-07-15 13:24 ` Fiona Ebner
0 siblings, 0 replies; 60+ messages in thread
From: Fiona Ebner @ 2025-07-15 13:24 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, qemu-devel, den, andrey.drobyshev, hreitz, stefanha,
eblake, jsnow, vsementsov, xiechanglong.d, wencongyang2, berto,
fam, ari
Am 14.07.25 um 15:43 schrieb Kevin Wolf:
> Am 01.07.2025 um 19:16 hat Kevin Wolf geschrieben:
>> Am 30.05.2025 um 17:10 hat Fiona Ebner geschrieben:
>>> This series is an attempt to fix a deadlock issue reported by Andrey
>>> here [3].
>>>
>>> bdrv_drained_begin() polls and is not allowed to be called with the
>>> block graph lock held. Mark the function as GRAPH_UNLOCKED.
>>>
>>> This alone does not catch the issue reported by Andrey, because there
>>> is a bdrv_graph_rdunlock_main_loop() before bdrv_drained_begin() in
>>> the function bdrv_change_aio_context(). That unlock is of course
>>> ineffective if the exclusive lock is held, but it prevents TSA from
>>> finding the issue.
>>>
>>> Thus the bdrv_drained_begin() call from inside
>>> bdrv_change_aio_context() needs to be moved up the call stack before
>>> acquiring the locks. This is the bulk of the series.
>>>
>>> Granular draining is not trivially possible, because many of the
>>> affected functions can recursively call themselves.
>>>
>>> In place where bdrv_drained_begin() calls were removed, assertions
>>> are added, checking the quiesced_counter to ensure that the nodes
>>> already got drained further up in the call stack.
>>
>> I finished review for this series. I had some minor comments on patches
>> 24, 27 and 41. Once we agree what to do there, I can probably just make
>> any changes myself while applying.
>
> I don't see any objections, so I just applied this and made all the
> changes I had suggested.
Sorry, for not responding anymore. I was on vacation for a while and
will still be busy with other stuff in the coming weeks. The changes you
suggested sound good to me, thanks!
Best Regards,
Fiona
^ permalink raw reply [flat|nested] 60+ messages in thread