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

[Kevin Wolf]

Am 22.08.2014 um 18:11 hat Benoît Canet geschrieben:
> 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) {

This doesn't actually need the SAFE version.

> +        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);
> +    }

Here you block bs->file/bs->backing_hd automatically, no matter whether
the block driver implements the callback or not. I'm not sure if we can
do it like this for all times, but for now that should be okay.

> +}
> +
> +/* 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)) {

That rename could have been a separate patch.

>          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);
> +}

Here you block bs->file again, even though the block.c function has
already done it. Nothing bad happens, because bdrv_op_block() simply
stops when the blocker is already set, but the code would be nicer if we
did block bs->file only in one place. (I'm leaning towards doing it in
the drivers, but that's your decision.)

>  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);

This suggests that bdrv_op_(un)block_all() doesn't provide the right
interface, but we rather want an optional base argument in it, so that
you can (un)block a specific part of the backing file chain.

>          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);

You need to unblock all of the BDSes that are going to be removed.
Aren't you unblocking more than that here?

It's not a real problem because block_job_completed() would unblock the
rest anyway and its bdrv_op_unblock_all() call is simply ignored now,
but it would be nicer to just unblock here what's actually needed.

>          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);

What purpose does a blocker serve when it's disabled before it is
checked? I would only ever expect a bdrv_op_unblock() after some
operation on the BDS has finished, but not when starting an operation
that needs it.

>      s->replaces = g_strdup(replaces);
>      s->on_source_error = on_source_error;
>      s->on_target_error = on_target_error;
> 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);

Same thing as for commit.

>          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);

Already blocked in block.c, remove one of the two

> +    for (i = 0; i < s->num_extents; i++) {
> +        if (s->extents[i].file) {
> +            bdrv_op_block(s->extents[i].file, op, reason);
> +        }
> +    }
> +}

Kevin