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.ca...@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