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

[Benoît Canet]

On Fri, Sep 12, 2014 at 11:48:33AM +0800, Fam Zheng wrote:
> 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 <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;
> > > >  
> > > > +    /* 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.
> 

block-job-complete will block it in mirror_complete.

BLOCK_OP_TYPE_MIRROR_REPLACE is blocked by driver-mirror code triggered by
block-job complete to block the "replaces" BDS during the end of the mirroring.

If you find silly that block-job-complete prevent itself from running twice on
the same BDS by checking the blocker then blocking it then the existing code is
wrong.

Else the code in this current patch is correct.

Could you have a glance at "static void mirror_complete(BlockJob *job, Error 
**errp)"
and tell me what you think about the situation. You should also look at
check_to_replace_node.

Best regards

Benoît

> 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