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