[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] block: Make op blockers recursive
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH] block: Make op blockers recursive |
Date: |
Thu, 18 Sep 2014 10:57:48 +0800 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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 <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.
>
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