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

[Fam Zheng <famz@redhat.com>]

On Tue, 09/09 14:28, Benoît Canet wrote:
> On Tue, Sep 09, 2014 at 01:56:46PM +0200, Kevin Wolf wrote:
> > 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 <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) {
> > 
> > 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.
> 
> Good idea.
> 
> > 
> > >          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.

I agree. It makes no sense if the blocker is also the checker.

> 
> BLOCK_OP_TYPE_MIRROR_REPLACE is checked and blocked by block-job-complete
> during the time the mirror finish when an arbitrary node of the graph must be
> replaced.

It seems to me mirror unblocks this operation from the job->blocker when job
starts, and never block it (with the job->blocker) again. It's leaked.

Fam

> 
> The start of the mirroring block everything including
> BLOCK_OP_TYPE_MIRROR_REPLACE so this hunk relax the blocking so block-job-complete
> can have a chance of being able to block it.
> 
> The comment of this hunk seems to be deficient and the code not self explanatory.
> 
> On the other way maybe block-job-complete is done wrong from the start but
> relaxing BLOCK_OP_TYPE_MIRROR_REPLACE when the mirror start is the only way
> to make block-job-complete work as intended.
> 
> Thanks
> 
> Benoît