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

[Fam Zheng <famz@redhat.com>]

On Fri, 08/22 18:11, Benoît Canet wrote:
> 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.

Is this going to replace backing_blocker?

I think it is too general an approach to control the operation properly,
because the op blocker may not work in the same way for all types of BDS
connections.  In other words, the choosing of op blockers are likely
conditional on graph edge types, that's why backing_blocker was added here. For
example, A VMDK extent connection will probably need a different set of
blockers than bs->file connection.

So could you explain in which cases is the recursive blocking/unblocking
useful?

Fam

> 
> Signed-off-by: Benoit Canet <benoit.canet@nodalink.com>
> ---
>  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
>