block: Fix deadlocks in bdrv_graph_wrunlock()

bdrv_graph_wrunlock() calls aio_poll(), which may run callbacks that
have a nested event loop. Nested event loops can depend on other
iothreads making progress, so in order to allow them to make progress it
must not hold the AioContext lock of another thread while calling
aio_poll().

This introduces a @bs parameter to bdrv_graph_wrunlock() whose
AioContext is temporarily dropped (which matches bdrv_graph_wrlock()),
and a bdrv_graph_wrunlock_ctx() that can be used if the BlockDriverState
doesn't necessarily exist any more when unlocking.

This also requires a change to bdrv_schedule_unref(), which was relying
on the incorrectly taken lock. It needs to take the lock itself now.
While this is a separate bug, it can't be fixed a separate patch because
otherwise the intermediate state would either deadlock or try to release
a lock that we don't even hold.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231115172012.112727-3-kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[kwolf: Fixed up bdrv_schedule_unref()]
Signed-off-by: Kevin Wolf <kwolf@redhat.com>

13 files changed, 62 insertions, 37 deletions

diff --git a/block/backup.c b/block/backup.c
index 5bad7d1..8aae583 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -499,7 +499,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     bdrv_graph_wrlock(target);
     block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
                        &error_abort);
-    bdrv_graph_wrunlock();
+    bdrv_graph_wrunlock(target);
 
     return &job->common;
 
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index a0d7072..3678f6c 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -253,7 +253,7 @@ fail_log:
     if (ret < 0) {
         bdrv_graph_wrlock(NULL);
         bdrv_unref_child(bs, s->log_file);
-        bdrv_graph_wrunlock();
+        bdrv_graph_wrunlock(NULL);
         s->log_file = NULL;
     }
 fail:
@@ -268,7 +268,7 @@ static void blk_log_writes_close(BlockDriverState *bs)
     bdrv_graph_wrlock(NULL);
     bdrv_unref_child(bs, s->log_file);
     s->log_file = NULL;
-    bdrv_graph_wrunlock();
+    bdrv_graph_wrunlock(NULL);
 }
 
 static int64_t coroutine_fn GRAPH_RDLOCK
diff --git a/block/blkverify.c b/block/blkverify.c
index a96905d..9b17c46 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -154,7 +154,7 @@ static void blkverify_close(BlockDriverState *bs)
     bdrv_graph_wrlock(NULL);
     bdrv_unref_child(bs, s->test_file);
     s->test_file = NULL;
-    bdrv_graph_wrunlock();
+    bdrv_graph_wrunlock(NULL);
 }
 
 static int64_t coroutine_fn GRAPH_RDLOCK
diff --git a/block/block-backend.c b/block/block-backend.c
index f6f05ea..ec21148 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -889,6 +889,7 @@ void blk_remove_bs(BlockBackend *blk)
 {
     ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
     BdrvChild *root;
+    AioContext *ctx;
 
     GLOBAL_STATE_CODE();
 
@@ -918,9 +919,10 @@ void blk_remove_bs(BlockBackend *blk)
     root = blk->root;
     blk->root = NULL;
 
+    ctx = bdrv_get_aio_context(root->bs);
     bdrv_graph_wrlock(root->bs);
     bdrv_root_unref_child(root);
-    bdrv_graph_wrunlock();
+    bdrv_graph_wrunlock_ctx(ctx);
 }
 
 /*
@@ -931,6 +933,8 @@ void blk_remove_bs(BlockBackend *blk)
 int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
 {
     ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
+    AioContext *ctx = bdrv_get_aio_context(bs);
+
     GLOBAL_STATE_CODE();
     bdrv_ref(bs);
     bdrv_graph_wrlock(bs);
@@ -938,7 +942,7 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
                                        BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
                                        blk->perm, blk->shared_perm,
                                        blk, errp);
-    bdrv_graph_wrunlock();
+    bdrv_graph_wrunlock_ctx(ctx);
     if (blk->root == NULL) {
         return -EPERM;
     }
diff --git a/block/commit.c b/block/commit.c
index eb3dc01..69cc75b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -102,7 +102,7 @@ static void commit_abort(Job *job)
     bdrv_drained_begin(commit_top_backing_bs);
     bdrv_graph_wrlock(commit_top_backing_bs);
     bdrv_replace_node(s->commit_top_bs, commit_top_backing_bs, &error_abort);
-    bdrv_graph_wrunlock();
+    bdrv_graph_wrunlock(commit_top_backing_bs);
     bdrv_drained_end(commit_top_backing_bs);
 
     bdrv_unref(s->commit_top_bs);
@@ -370,19 +370,19 @@ void commit_start(const char *job_id, BlockDriverState *bs,
         ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
                                  iter_shared_perms, errp);
         if (ret < 0) {
-            bdrv_graph_wrunlock();
+            bdrv_graph_wrunlock(top);
             goto fail;
         }
     }
 
     if (bdrv_freeze_backing_chain(commit_top_bs, base, errp) < 0) {
-        bdrv_graph_wrunlock();
+        bdrv_graph_wrunlock(top);
         goto fail;
     }
     s->chain_frozen = true;
 
     ret = block_job_add_bdrv(&s->common, "base", base, 0, BLK_PERM_ALL, errp);
-    bdrv_graph_wrunlock();
+    bdrv_graph_wrunlock(top);
 
     if (ret < 0) {
         goto fail;
@@ -436,7 +436,7 @@ fail:
         bdrv_drained_begin(top);
         bdrv_graph_wrlock(top);
         bdrv_replace_node(commit_top_bs, top, &error_abort);
-        bdrv_graph_wrunlock();
+        bdrv_graph_wrunlock(top);
         bdrv_drained_end(top);
     }
 }
diff --git a/block/graph-lock.c b/block/graph-lock.c
index e5525ee..079e878 100644
--- a/block/graph-lock.c
+++ b/block/graph-lock.c
@@ -161,11 +161,21 @@ void no_coroutine_fn bdrv_graph_wrlock(BlockDriverState *bs)
     }
 }
 
-void bdrv_graph_wrunlock(void)
+void no_coroutine_fn bdrv_graph_wrunlock_ctx(AioContext *ctx)
 {
     GLOBAL_STATE_CODE();
     assert(qatomic_read(&has_writer));
 
+    /*
+     * Release only non-mainloop AioContext. The mainloop often relies on the
+     * BQL and doesn't lock the main AioContext before doing things.
+     */
+    if (ctx && ctx != qemu_get_aio_context()) {
+        aio_context_release(ctx);
+    } else {
+        ctx = NULL;
+    }
+
     WITH_QEMU_LOCK_GUARD(&aio_context_list_lock) {
         /*
          * No need for memory barriers, this works in pair with
@@ -187,6 +197,17 @@ void bdrv_graph_wrunlock(void)
      * progress.
      */
     aio_bh_poll(qemu_get_aio_context());
+
+    if (ctx) {
+        aio_context_acquire(ctx);
+    }
+}
+
+void no_coroutine_fn bdrv_graph_wrunlock(BlockDriverState *bs)
+{
+    AioContext *ctx = bs ? bdrv_get_aio_context(bs) : NULL;
+
+    bdrv_graph_wrunlock_ctx(ctx);
 }
 
 void coroutine_fn bdrv_graph_co_rdlock(void)
diff --git a/block/mirror.c b/block/mirror.c
index 2096fad..cd9d3ad 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -773,7 +773,7 @@ static int mirror_exit_common(Job *job)
                        "would not lead to an abrupt change of visible data",
                        to_replace->node_name, target_bs->node_name);
         }
-        bdrv_graph_wrunlock();
+        bdrv_graph_wrunlock(target_bs);
         bdrv_drained_end(to_replace);
         if (local_err) {
             error_report_err(local_err);
@@ -798,7 +798,7 @@ static int mirror_exit_common(Job *job)
     block_job_remove_all_bdrv(bjob);
     bdrv_graph_wrlock(mirror_top_bs);
     bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs, &error_abort);
-    bdrv_graph_wrunlock();
+    bdrv_graph_wrunlock(mirror_top_bs);
 
     bdrv_drained_end(target_bs);
     bdrv_unref(target_bs);
@@ -1920,7 +1920,7 @@ static BlockJob *mirror_start_job(
                              BLK_PERM_CONSISTENT_READ,
                              errp);
     if (ret < 0) {
-        bdrv_graph_wrunlock();
+        bdrv_graph_wrunlock(bs);
         goto fail;
     }
 
@@ -1965,17 +1965,17 @@ static BlockJob *mirror_start_job(
             ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
                                      iter_shared_perms, errp);
             if (ret < 0) {
-                bdrv_graph_wrunlock();
+                bdrv_graph_wrunlock(bs);
                 goto fail;
             }
         }
 
         if (bdrv_freeze_backing_chain(mirror_top_bs, target, errp) < 0) {
-            bdrv_graph_wrunlock();
+            bdrv_graph_wrunlock(bs);
             goto fail;
         }
     }
-    bdrv_graph_wrunlock();
+    bdrv_graph_wrunlock(bs);
 
     QTAILQ_INIT(&s->ops_in_flight);
 
@@ -2006,7 +2006,7 @@ fail:
     bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing,
                              &error_abort);
     bdrv_replace_node(mirror_top_bs, bs, &error_abort);
-    bdrv_graph_wrunlock();
+    bdrv_graph_wrunlock(bs);
     bdrv_drained_end(bs);
 
     bdrv_unref(mirror_top_bs);
diff --git a/block/qcow2.c b/block/qcow2.c
index cf24688..13e032b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2809,7 +2809,7 @@ qcow2_do_close(BlockDriverState *bs, bool close_data_file)
         bdrv_graph_rdunlock_main_loop();
         bdrv_graph_wrlock(NULL);
         bdrv_unref_child(bs, s->data_file);
-        bdrv_graph_wrunlock();
+        bdrv_graph_wrunlock(NULL);
         s->data_file = NULL;
         bdrv_graph_rdlock_main_loop();
     }
diff --git a/block/quorum.c b/block/quorum.c
index d3ffc2e..505b8b3 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1044,7 +1044,7 @@ close_exit:
         }
         bdrv_unref_child(bs, s->children[i]);
     }
-    bdrv_graph_wrunlock();
+    bdrv_graph_wrunlock(NULL);
     g_free(s->children);
     g_free(opened);
 exit:
@@ -1061,7 +1061,7 @@ static void quorum_close(BlockDriverState *bs)
     for (i = 0; i < s->num_children; i++) {
         bdrv_unref_child(bs, s->children[i]);
     }
-    bdrv_graph_wrunlock();
+    bdrv_graph_wrunlock(NULL);
 
     g_free(s->children);
 }
diff --git a/block/replication.c b/block/replication.c
index 43e2594..5ded5f1 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -568,7 +568,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
                                            &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
-            bdrv_graph_wrunlock();
+            bdrv_graph_wrunlock(bs);
             aio_context_release(aio_context);
             return;
         }
@@ -579,7 +579,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
                                               BDRV_CHILD_DATA, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
-            bdrv_graph_wrunlock();
+            bdrv_graph_wrunlock(bs);
             aio_context_release(aio_context);
             return;
         }
@@ -592,7 +592,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
         if (!top_bs || !bdrv_is_root_node(top_bs) ||
             !check_top_bs(top_bs, bs)) {
             error_setg(errp, "No top_bs or it is invalid");
-            bdrv_graph_wrunlock();
+            bdrv_graph_wrunlock(bs);
             reopen_backing_file(bs, false, NULL);
             aio_context_release(aio_context);
             return;
@@ -600,7 +600,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
         bdrv_op_block_all(top_bs, s->blocker);
         bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
 
-        bdrv_graph_wrunlock();
+        bdrv_graph_wrunlock(bs);
 
         s->backup_job = backup_job_create(
                                 NULL, s->secondary_disk->bs, s->hidden_disk->bs,
@@ -696,7 +696,7 @@ static void replication_done(void *opaque, int ret)
         s->secondary_disk = NULL;
         bdrv_unref_child(bs, s->hidden_disk);
         s->hidden_disk = NULL;
-        bdrv_graph_wrunlock();
+        bdrv_graph_wrunlock(NULL);
 
         s->error = 0;
     } else {
diff --git a/block/snapshot.c b/block/snapshot.c
index 5597427..ec8cf48 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -292,7 +292,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
         /* .bdrv_open() will re-attach it */
         bdrv_graph_wrlock(NULL);
         bdrv_unref_child(bs, fallback);
-        bdrv_graph_wrunlock();
+        bdrv_graph_wrunlock(NULL);
 
         ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp);
         open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
diff --git a/block/stream.c b/block/stream.c
index 0b92410..e3aa696 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -101,7 +101,7 @@ static int stream_prepare(Job *job)
 
         bdrv_graph_wrlock(base);
         bdrv_set_backing_hd_drained(unfiltered_bs, base, &local_err);
-        bdrv_graph_wrunlock();
+        bdrv_graph_wrunlock(base);
 
         /*
          * This call will do I/O, so the graph can change again from here on.
@@ -369,7 +369,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
     bdrv_graph_wrlock(bs);
     if (block_job_add_bdrv(&s->common, "active node", bs, 0,
                            basic_flags | BLK_PERM_WRITE, errp)) {
-        bdrv_graph_wrunlock();
+        bdrv_graph_wrunlock(bs);
         goto fail;
     }
 
@@ -389,11 +389,11 @@ void stream_start(const char *job_id, BlockDriverState *bs,
         ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
                                  basic_flags, errp);
         if (ret < 0) {
-            bdrv_graph_wrunlock();
+            bdrv_graph_wrunlock(bs);
             goto fail;
         }
     }
-    bdrv_graph_wrunlock();
+    bdrv_graph_wrunlock(bs);
 
     s->base_overlay = base_overlay;
     s->above_base = above_base;
diff --git a/block/vmdk.c b/block/vmdk.c
index dda783f..d87f6d9 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -283,7 +283,7 @@ static void vmdk_free_extents(BlockDriverState *bs)
             bdrv_unref_child(bs, e->file);
         }
     }
-    bdrv_graph_wrunlock();
+    bdrv_graph_wrunlock(NULL);
 
     g_free(s->extents);
 }
@@ -1237,7 +1237,7 @@ vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
                 bdrv_graph_rdunlock_main_loop();
                 bdrv_graph_wrlock(NULL);
                 bdrv_unref_child(bs, extent_file);
-                bdrv_graph_wrunlock();
+                bdrv_graph_wrunlock(NULL);
                 bdrv_graph_rdlock_main_loop();
                 goto out;
             }
@@ -1256,7 +1256,7 @@ vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
                 bdrv_graph_rdunlock_main_loop();
                 bdrv_graph_wrlock(NULL);
                 bdrv_unref_child(bs, extent_file);
-                bdrv_graph_wrunlock();
+                bdrv_graph_wrunlock(NULL);
                 bdrv_graph_rdlock_main_loop();
                 goto out;
             }
@@ -1267,7 +1267,7 @@ vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
                 bdrv_graph_rdunlock_main_loop();
                 bdrv_graph_wrlock(NULL);
                 bdrv_unref_child(bs, extent_file);
-                bdrv_graph_wrunlock();
+                bdrv_graph_wrunlock(NULL);
                 bdrv_graph_rdlock_main_loop();
                 goto out;
             }
@@ -1277,7 +1277,7 @@ vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
             bdrv_graph_rdunlock_main_loop();
             bdrv_graph_wrlock(NULL);
             bdrv_unref_child(bs, extent_file);
-            bdrv_graph_wrunlock();
+            bdrv_graph_wrunlock(NULL);
             bdrv_graph_rdlock_main_loop();
             ret = -ENOTSUP;
             goto out;