[RFC PATCH v2 5/8] block.c: wrlock in bdrv_replace_child_noperm

[Emanuele Giuseppe Esposito]

The only problem here is ->attach and ->detach callbacks
could call bdrv_{un}apply_subtree_drain(), which itself
will use a rdlock to navigate through all nodes.
To avoid deadlocks, take the lock only outside the drains,
and if we need to both attach and detach, do it in a single
critical section.

Therefore change ->attach and ->detach to return true if they
are modifying the lock, so that we don't take it twice or release
temporarly.

Signed-off-by: Emanuele Giuseppe Esposito <eespo...@redhat.com>
---
 block.c                          | 31 +++++++++++++++++++++++++++----
 block/block-backend.c            |  6 ++++--
 include/block/block_int-common.h |  8 ++++++--
 3 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index b2eb679abb..6cd87e8dd3 100644
--- a/block.c
+++ b/block.c
@@ -1434,21 +1434,26 @@ static void bdrv_inherited_options(BdrvChildRole role, 
bool parent_is_format,
     *child_flags = flags;
 }
 
-static void bdrv_child_cb_attach(BdrvChild *child)
+static bool bdrv_child_cb_attach(BdrvChild *child)
 {
     BlockDriverState *bs = child->opaque;
 
     assert_bdrv_graph_writable(bs);
     QLIST_INSERT_HEAD(&bs->children, child, next);
 
+    /* Paired with bdrv_graph_wrlock() in bdrv_replace_child_noperm */
+    bdrv_graph_wrunlock();
+
     if (child->role & BDRV_CHILD_COW) {
         bdrv_backing_attach(child);
     }
 
     bdrv_apply_subtree_drain(child, bs);
+
+    return true;
 }
 
-static void bdrv_child_cb_detach(BdrvChild *child)
+static bool bdrv_child_cb_detach(BdrvChild *child)
 {
     BlockDriverState *bs = child->opaque;
 
@@ -1458,8 +1463,13 @@ static void bdrv_child_cb_detach(BdrvChild *child)
 
     bdrv_unapply_subtree_drain(child, bs);
 
+    /* Paired with bdrv_graph_wrunlock() in bdrv_replace_child_noperm */
+    bdrv_graph_wrlock();
+
     assert_bdrv_graph_writable(bs);
     QLIST_REMOVE(child, next);
+
+    return true;
 }
 
 static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,
@@ -2842,6 +2852,7 @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
     BlockDriverState *old_bs = child->bs;
     int new_bs_quiesce_counter;
     int drain_saldo;
+    bool locked = false;
 
     assert(!child->frozen);
     assert(old_bs != new_bs);
@@ -2868,8 +2879,12 @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
          * are already gone and we only end the drain sections that came from
          * elsewhere. */
         if (child->klass->detach) {
-            child->klass->detach(child);
+            locked = child->klass->detach(child);
+        }
+        if (!locked) {
+            bdrv_graph_wrlock();
         }
+        locked = true;
         assert_bdrv_graph_writable(old_bs);
         QLIST_REMOVE(child, next_parent);
     }
@@ -2880,6 +2895,10 @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
     }
 
     if (new_bs) {
+        if (!locked) {
+            bdrv_graph_wrlock();
+            locked = true;
+        }
         assert_bdrv_graph_writable(new_bs);
         QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
 
@@ -2896,10 +2915,14 @@ static void bdrv_replace_child_noperm(BdrvChild 
**childp,
          * drain sections coming from @child don't get an extra .drained_begin
          * callback. */
         if (child->klass->attach) {
-            child->klass->attach(child);
+            locked = !(child->klass->attach(child));
         }
     }
 
+    if (locked) {
+        bdrv_graph_wrunlock();
+    }
+
     /*
      * If the old child node was drained but the new one is not, allow
      * requests to come in only after the new node has been attached.
diff --git a/block/block-backend.c b/block/block-backend.c
index e0e1aff4b1..5dbd9fceae 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -282,7 +282,7 @@ static int blk_root_inactivate(BdrvChild *child)
     return 0;
 }
 
-static void blk_root_attach(BdrvChild *child)
+static bool blk_root_attach(BdrvChild *child)
 {
     BlockBackend *blk = child->opaque;
     BlockBackendAioNotifier *notifier;
@@ -295,9 +295,10 @@ static void blk_root_attach(BdrvChild *child)
                 notifier->detach_aio_context,
                 notifier->opaque);
     }
+    return false;
 }
 
-static void blk_root_detach(BdrvChild *child)
+static bool blk_root_detach(BdrvChild *child)
 {
     BlockBackend *blk = child->opaque;
     BlockBackendAioNotifier *notifier;
@@ -310,6 +311,7 @@ static void blk_root_detach(BdrvChild *child)
                 notifier->detach_aio_context,
                 notifier->opaque);
     }
+    return false;
 }
 
 static AioContext *blk_root_get_parent_aio_context(BdrvChild *c)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 5a04c778e4..dd058c1fd8 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -857,8 +857,12 @@ struct BdrvChildClass {
     void (*activate)(BdrvChild *child, Error **errp);
     int (*inactivate)(BdrvChild *child);
 
-    void (*attach)(BdrvChild *child);
-    void (*detach)(BdrvChild *child);
+    /*
+     * Return true if the graph wrlock is taken/released,
+     * false if the wrlock state is not changed.
+     */
+    bool (*attach)(BdrvChild *child);
+    bool (*detach)(BdrvChild *child);
 
     /*
      * Notifies the parent that the filename of its child has changed (e.g.
-- 
2.31.1