From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56022) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTY1i-0003cs-5z for qemu-devel@nongnu.org; Mon, 15 Sep 2014 11:17:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XTY1a-0003vt-9K for qemu-devel@nongnu.org; Mon, 15 Sep 2014 11:17:14 -0400 Received: from dew.nodalink.com ([95.130.14.197]:55280) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTY1a-0003vX-1W for qemu-devel@nongnu.org; Mon, 15 Sep 2014 11:17:06 -0400 Date: Mon, 15 Sep 2014 15:17:05 +0000 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140915151705.GA26550@nodalink.com> References: <1408723870-7826-1-git-send-email-benoit.canet@nodalink.com> <1408723870-7826-2-git-send-email-benoit.canet@nodalink.com> <20140909115646.GI4847@noname.str.redhat.com> <20140909142822.GA7953@nodalink.com> <20140912034833.GC5478@fam-t430.nay.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140912034833.GC5478@fam-t430.nay.redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] block: Make op blockers recursive List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: Kevin Wolf , jcody@redhat.com, stefanha@redhat.com, qemu-devel@nongnu.org, =?iso-8859-1?Q?Beno=EEt?= Canet On Fri, Sep 12, 2014 at 11:48:33AM +0800, Fam Zheng wrote: > On Tue, 09/09 14:28, Beno=EEt Canet wrote: > > On Tue, Sep 09, 2014 at 01:56:46PM +0200, Kevin Wolf wrote: > > > Am 22.08.2014 um 18:11 hat Beno=EEt Canet geschrieben: > > > > Since the block layer code is starting to modify the BDS graph ri= ght in the > > > > middle of BDS chains (block-mirror's replace parameter for exampl= e) QEMU needs > > > > to properly block and unblock whole BDS subtrees; recursion is a = neat way to > > > > achieve this task. > > > >=20 > > > > This patch also takes care of modifying the op blockers users. > > > >=20 > > > > Signed-off-by: Benoit Canet > > > > --- > > > > 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(-) > > > >=20 > > > > 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 *b= s, BlockOpType op, Error **errp) > > > > return false; > > > > } > > > > =20 > > > > -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 o= p, > > > > + Error *reason) > > > > { > > > > BdrvOpBlocker *blocker; > > > > assert((int) op >=3D 0 && op < BLOCK_OP_TYPE_MAX); > > > > @@ -5456,7 +5458,9 @@ void bdrv_op_block(BlockDriverState *bs, Bl= ockOpType op, Error *reason) > > > > QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list); > > > > } > > > > =20 > > > > -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 >=3D 0 && op < BLOCK_OP_TYPE_MAX); > > > > @@ -5468,6 +5472,65 @@ void bdrv_op_unblock(BlockDriverState *bs,= BlockOpType op, Error *reason) > > > > } > > > > } > > > > =20 > > > > +static bool bdrv_op_is_blocked_by(BlockDriverState *bs, BlockOpT= ype op, > > > > + Error *reason) > > > > +{ > > > > + BdrvOpBlocker *blocker, *next; > > > > + assert((int) op >=3D 0 && op < BLOCK_OP_TYPE_MAX); > > > > + QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next= ) { > > >=20 > > > This doesn't actually need the SAFE version. > > >=20 > > > > + if (blocker->reason =3D=3D reason) { > > > > + return true; > > > > + } > > > > + } > > > > + return false; > > > > +} > > > > + > > > > +/* block recursively a BDS */ > > > > +void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *= reason) > > > > +{ > > > > + if (!bs) { > > > > + return; > > > > =20 > > > > + /* 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->b= locker); > > >=20 > > > 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 operati= on > > > that needs it. >=20 > I agree. It makes no sense if the blocker is also the checker. >=20 > >=20 > > BLOCK_OP_TYPE_MIRROR_REPLACE is checked and blocked by block-job-comp= lete > > during the time the mirror finish when an arbitrary node of the graph= must be > > replaced. >=20 > It seems to me mirror unblocks this operation from the job->blocker whe= n job > starts, and never block it (with the job->blocker) again. It's leaked. >=20 block-job-complete will block it in mirror_complete. BLOCK_OP_TYPE_MIRROR_REPLACE is blocked by driver-mirror code triggered b= y block-job complete to block the "replaces" BDS during the end of the mirr= oring. If you find silly that block-job-complete prevent itself from running twi= ce on the same BDS by checking the blocker then blocking it then the existing c= ode is wrong. Else the code in this current patch is correct. Could you have a glance at "static void mirror_complete(BlockJob *job, Er= ror **errp)" and tell me what you think about the situation. You should also look at check_to_replace_node. Best regards Beno=EEt > Fam >=20 > >=20 > > 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. > >=20 > > The comment of this hunk seems to be deficient and the code not self = explanatory. > >=20 > > On the other way maybe block-job-complete is done wrong from the star= t but > > relaxing BLOCK_OP_TYPE_MIRROR_REPLACE when the mirror start is the on= ly way > > to make block-job-complete work as intended. > >=20 > > Thanks > >=20 > > Beno=EEt