[Qemu-devel] [PATCH] block: Make op blockers recursive

[Benoît Canet]

Since the block layer code is starting to modify the BDS graph right in the
middle of BDS chains (block-mirror's replace parameter for example) QEMU needs
to properly block and unblock whole BDS subtrees; recursion is a neat way to
achieve this task.

This patch also takes care of modifying the op blockers users.

Signed-off-by: Benoit Canet <address@hidden>
---
 block.c                   | 69 ++++++++++++++++++++++++++++++++++++++++++++---
 block/blkverify.c         | 21 +++++++++++++++
 block/commit.c            |  3 +++
 block/mirror.c            | 17 ++++++++----
 block/quorum.c            | 25 +++++++++++++++++
 block/stream.c            |  3 +++
 block/vmdk.c              | 34 +++++++++++++++++++++++
 include/block/block.h     |  2 +-
 include/block/block_int.h |  6 +++++
 9 files changed, 171 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 6fa0201..d964b6c 100644
--- a/block.c
+++ b/block.c
@@ -5446,7 +5446,9 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType 
op, Error **errp)
     return false;
 }
 
-void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason)
+/* do the real work of blocking a BDS */
+static void bdrv_do_op_block(BlockDriverState *bs, BlockOpType op,
+                             Error *reason)
 {
     BdrvOpBlocker *blocker;
     assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
@@ -5456,7 +5458,9 @@ void bdrv_op_block(BlockDriverState *bs, BlockOpType op, 
Error *reason)
     QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list);
 }
 
-void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)
+/* do the real work of unblocking a BDS */
+static void bdrv_do_op_unblock(BlockDriverState *bs, BlockOpType op,
+                               Error *reason)
 {
     BdrvOpBlocker *blocker, *next;
     assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
@@ -5468,6 +5472,65 @@ void bdrv_op_unblock(BlockDriverState *bs, BlockOpType 
op, Error *reason)
     }
 }
 
+static bool bdrv_op_is_blocked_by(BlockDriverState *bs, BlockOpType op,
+                                  Error *reason)
+{
+    BdrvOpBlocker *blocker, *next;
+    assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
+    QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) {
+        if (blocker->reason == reason) {
+            return true;
+        }
+    }
+    return false;
+}
+
+/* block recursively a BDS */
+void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason)
+{
+    if (!bs) {
+        return;
+    }
+
+    /* prevent recursion loop */
+    if (bdrv_op_is_blocked_by(bs, op, reason)) {
+        return;
+    }
+
+    /* block first for recursion loop protection to work */
+    bdrv_do_op_block(bs, op, reason);
+
+    bdrv_op_block(bs->file, op, reason);
+    bdrv_op_block(bs->backing_hd, op, reason);
+
+    if (bs->drv && bs->drv->bdrv_op_recursive_block) {
+        bs->drv->bdrv_op_recursive_block(bs, op, reason);
+    }
+}
+
+/* unblock recursively a BDS */
+void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason)
+{
+    if (!bs) {
+        return;
+    }
+
+    /* prevent recursion loop */
+    if (!bdrv_op_is_blocked_by(bs, op, reason)) {
+        return;
+    }
+
+    /* unblock first for recursion loop protection to work */
+    bdrv_do_op_unblock(bs, op, reason);
+
+    bdrv_op_unblock(bs->file, op, reason);
+    bdrv_op_unblock(bs->backing_hd, op, reason);
+
+    if (bs->drv && bs->drv->bdrv_op_recursive_unblock) {
+        bs->drv->bdrv_op_recursive_unblock(bs, op, reason);
+    }
+}
+
 void bdrv_op_block_all(BlockDriverState *bs, Error *reason)
 {
     int i;
@@ -5848,7 +5911,7 @@ BlockDriverState *check_to_replace_node(const char 
*node_name, Error **errp)
         return NULL;
     }
 
-    if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)) {
+    if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_MIRROR_REPLACE, errp)) 
{
         return NULL;
     }
 
diff --git a/block/blkverify.c b/block/blkverify.c
index 621b785..75ec3df 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -320,6 +320,24 @@ static void blkverify_attach_aio_context(BlockDriverState 
*bs,
     bdrv_attach_aio_context(s->test_file, new_context);
 }
 
+static void blkverify_op_recursive_block(BlockDriverState *bs, BlockOpType op,
+                                         Error *reason)
+{
+    BDRVBlkverifyState *s = bs->opaque;
+
+    bdrv_op_block(bs->file, op, reason);
+    bdrv_op_block(s->test_file, op, reason);
+}
+
+static void blkverify_op_recursive_unblock(BlockDriverState *bs, BlockOpType 
op,
+                                           Error *reason)
+{
+    BDRVBlkverifyState *s = bs->opaque;
+
+    bdrv_op_unblock(bs->file, op, reason);
+    bdrv_op_unblock(s->test_file, op, reason);
+}
+
 static BlockDriver bdrv_blkverify = {
     .format_name                      = "blkverify",
     .protocol_name                    = "blkverify",
@@ -337,6 +355,9 @@ static BlockDriver bdrv_blkverify = {
     .bdrv_attach_aio_context          = blkverify_attach_aio_context,
     .bdrv_detach_aio_context          = blkverify_detach_aio_context,
 
+    .bdrv_op_recursive_block          = blkverify_op_recursive_block,
+    .bdrv_op_recursive_unblock        = blkverify_op_recursive_unblock,
+
     .is_filter                        = true,
     .bdrv_recurse_is_first_non_filter = blkverify_recurse_is_first_non_filter,
 };
diff --git a/block/commit.c b/block/commit.c
index 91517d3..8a122b7 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -142,6 +142,9 @@ wait:
 
     if (!block_job_is_cancelled(&s->common) && sector_num == end) {
         /* success */
+        /* unblock only BDS to be dropped */
+        bdrv_op_unblock_all(top, s->common.blocker);
+        bdrv_op_block_all(base, s->common.blocker);
         ret = bdrv_drop_intermediate(active, top, base, s->backing_file_str);
     }
 
diff --git a/block/mirror.c b/block/mirror.c
index 5e7a166..28ed47d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -324,6 +324,7 @@ static void coroutine_fn mirror_run(void *opaque)
     uint64_t last_pause_ns;
     BlockDriverInfo bdi;
     char backing_filename[1024];
+    BlockDriverState *to_replace;
     int ret = 0;
     int n;
 
@@ -512,14 +513,16 @@ immediate_exit:
     g_free(s->in_flight_bitmap);
     bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
     bdrv_iostatus_disable(s->target);
+    to_replace = s->common.bs;
+    if (s->to_replace) {
+        bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
+        to_replace = s->to_replace;
+    }
     if (s->should_complete && ret == 0) {
-        BlockDriverState *to_replace = s->common.bs;
-        if (s->to_replace) {
-            to_replace = s->to_replace;
-        }
         if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
             bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
         }
+        bdrv_op_unblock_all(to_replace, bs->job->blocker);
         bdrv_swap(s->target, to_replace);
         if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
             /* drop the bs loop chain formed by the swap: break the loop then
@@ -530,7 +533,6 @@ immediate_exit:
         }
     }
     if (s->to_replace) {
-        bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
         error_free(s->replace_blocker);
         bdrv_unref(s->to_replace);
     }
@@ -648,6 +650,11 @@ static void mirror_start_job(BlockDriverState *bs, 
BlockDriverState *target,
         return;
     }
 
+    /* remove BLOCK_OP_TYPE_MIRROR_REPLACE from the list of
+     * blocked operations so the replaces parameter can work
+     */
+    bdrv_op_unblock(bs, BLOCK_OP_TYPE_MIRROR_REPLACE, bs->job->blocker);
+
     s->replaces = g_strdup(replaces);
     s->on_source_error = on_source_error;
     s->on_target_error = on_target_error;
diff --git a/block/quorum.c b/block/quorum.c
index d5ee9c0..c260171 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -945,6 +945,28 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
     }
 }
 
+static void quorum_op_recursive_block(BlockDriverState *bs, BlockOpType op,
+                                      Error *reason)
+{
+    BDRVQuorumState *s = bs->opaque;
+    int i;
+
+    for (i = 0; i < s->num_children; i++) {
+        bdrv_op_block(s->bs[i], op, reason);
+    }
+}
+
+static void quorum_op_recursive_unblock(BlockDriverState *bs, BlockOpType op,
+                                        Error *reason)
+{
+    BDRVQuorumState *s = bs->opaque;
+    int i;
+
+    for (i = 0; i < s->num_children; i++) {
+        bdrv_op_unblock(s->bs[i], op, reason);
+    }
+}
+
 static BlockDriver bdrv_quorum = {
     .format_name                        = "quorum",
     .protocol_name                      = "quorum",
@@ -965,6 +987,9 @@ static BlockDriver bdrv_quorum = {
     .bdrv_detach_aio_context            = quorum_detach_aio_context,
     .bdrv_attach_aio_context            = quorum_attach_aio_context,
 
+    .bdrv_op_recursive_block            = quorum_op_recursive_block,
+    .bdrv_op_recursive_unblock          = quorum_op_recursive_unblock,
+
     .is_filter                          = true,
     .bdrv_recurse_is_first_non_filter   = quorum_recurse_is_first_non_filter,
 };
diff --git a/block/stream.c b/block/stream.c
index cdea3e8..2c917b7 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -192,6 +192,9 @@ wait:
             }
         }
         ret = bdrv_change_backing_file(bs, base_id, base_fmt);
+        /* unblock only BDS to be dropped */
+        bdrv_op_unblock_all(bs->backing_hd, s->common.blocker);
+        bdrv_op_block_all(base, s->common.blocker);
         close_unused_images(bs, base, base_id);
     }
 
diff --git a/block/vmdk.c b/block/vmdk.c
index 01412a8..bc5b17f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2220,6 +2220,38 @@ static QemuOptsList vmdk_create_opts = {
     }
 };
 
+static void vmdk_op_recursive_block(BlockDriverState *bs, BlockOpType op,
+                                      Error *reason)
+{
+    BDRVVmdkState *s = bs->opaque;
+    int i;
+
+    bdrv_op_block(bs->file, op, reason);
+    bdrv_op_block(bs->backing_hd, op, reason);
+
+    for (i = 0; i < s->num_extents; i++) {
+        if (s->extents[i].file) {
+            bdrv_op_block(s->extents[i].file, op, reason);
+        }
+    }
+}
+
+static void vmdk_op_recursive_unblock(BlockDriverState *bs, BlockOpType op,
+                                        Error *reason)
+{
+    BDRVVmdkState *s = bs->opaque;
+    int i;
+
+    bdrv_op_unblock(bs->file, op, reason);
+    bdrv_op_unblock(bs->backing_hd, op, reason);
+
+    for (i = 0; i < s->num_extents; i++) {
+        if (s->extents[i].file) {
+            bdrv_op_unblock(s->extents[i].file, op, reason);
+        }
+    }
+}
+
 static BlockDriver bdrv_vmdk = {
     .format_name                  = "vmdk",
     .instance_size                = sizeof(BDRVVmdkState),
@@ -2242,6 +2274,8 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_get_info                = vmdk_get_info,
     .bdrv_detach_aio_context      = vmdk_detach_aio_context,
     .bdrv_attach_aio_context      = vmdk_attach_aio_context,
+    .bdrv_op_recursive_block      = vmdk_op_recursive_block,
+    .bdrv_op_recursive_unblock    = vmdk_op_recursive_unblock,
 
     .supports_backing             = true,
     .create_opts                  = &vmdk_create_opts,
diff --git a/include/block/block.h b/include/block/block.h
index e94b701..4729a0a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -175,7 +175,7 @@ typedef enum BlockOpType {
     BLOCK_OP_TYPE_MIRROR,
     BLOCK_OP_TYPE_RESIZE,
     BLOCK_OP_TYPE_STREAM,
-    BLOCK_OP_TYPE_REPLACE,
+    BLOCK_OP_TYPE_MIRROR_REPLACE,
     BLOCK_OP_TYPE_MAX,
 } BlockOpType;
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7b541a0..a802abc 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -266,6 +266,12 @@ struct BlockDriver {
     void (*bdrv_io_unplug)(BlockDriverState *bs);
     void (*bdrv_flush_io_queue)(BlockDriverState *bs);
 
+    /* helps blockers to propagate recursively */
+    void (*bdrv_op_recursive_block)(BlockDriverState *bs, BlockOpType op,
+                                    Error *reason);
+    void (*bdrv_op_recursive_unblock)(BlockDriverState *bs, BlockOpType op,
+                                      Error *reason);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
2.1.0