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

[Fam Zheng <famz@redhat.com>]

On Mon, 09/15 15:17, Benoît Canet wrote:
> 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 <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;
> > > > >  
> > > > > +    /* 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.
> 

I'd prefer early error from user's point of view, so maybe checking and
blocking can be done during mirror_start instead, then we don't need the
relaxation. What's the advantage to delay the check to block-job-complete?

Fam