From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55809) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WxipG-0000en-LI for qemu-devel@nongnu.org; Thu, 19 Jun 2014 16:20:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WxipC-0005Wx-3t for qemu-devel@nongnu.org; Thu, 19 Jun 2014 16:20:50 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:51667 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WxipB-0005Wp-IH for qemu-devel@nongnu.org; Thu, 19 Jun 2014 16:20:46 -0400 Date: Thu, 19 Jun 2014 22:20:43 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140619202043.GA18306@irqsave.net> References: <1403208081-18247-1-git-send-email-benoit.canet@irqsave.net> <1403208081-18247-2-git-send-email-benoit.canet@irqsave.net> <53A34460.8010302@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <53A34460.8010302@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] block: Make op blocker recursive List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , kwolf@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com The Thursday 19 Jun 2014 =E0 14:13:20 (-0600), Eric Blake wrote : > On 06/19/2014 02:01 PM, Beno=EEt Canet wrote: > > As the code will start to operate on arbitratry nodes we need the op = blocker >=20 > s/arbitratry/arbitrary/ >=20 > > to recursively block or unblock whole BDS subtrees. > >=20 > > Also add a function to reset all blocker from a BDS. > >=20 > > This patch also take care of changing blocker user so they are not br= oken. > >=20 > > Signed-off-by: Benoit Canet > > --- >=20 > > + > > +/* This remove unconditionally all blockers of type op of the subtre= e */ >=20 > This unconditionally removes all blockers of type op of the subtree >=20 > Yikes - is that really what we want? Or do we need to start doing > blocker reference counting? >=20 > Consider: >=20 > base <- snap1 <- active >=20 > Looking at Jeff's proposal of making blockers based on access patterns > rather than operations, we want the mere act of being a backing file to > automatically put a guest_write block on base and snap1 (we must not > modify the backing chain out of underneath active). But now suppose we > do two operations in parallel - we take a fleecing export of active, an= d > we start a drive-mirror on active. >=20 > base <- snap1 <- active > | \-- fleecing > \-- copy >=20 > Both of those actions should be doable in parallel, and both of them > probably put additional blocker restrictions on the chain. But if we > unconditionally clear those additional restrictions on the first of the > two jobs ending, that would inappropriately stop blocking that action > from the still on-going second action. The only way I see around that > is via reference-counted blocking. Definitely not 2.1 material (but > good to be thinking about it now, so we can get it in early in the 2.2 > cycle). I added this reset function for the case where a whole BDS subtree is det= ached from the graph and will be destroyed. It does happen in drive mirror and bdrv_unrefing it would lead to a faile= d assertion. So the reset function take care of removing blocker of dead subtrees. What would be a cleaner solution ? Best regards Beno=EEt >=20 >=20 > > =20 > > +/* This remove unconditionally all blockers */ >=20 > Unconditionally remove all blockers >=20 >=20 > > =20 > > +/* Used to prevent recursion loop. A case exists in block commit mir= ror usage */ > > +static BlockDriverState *recurse_op_bs =3D NULL; > > +/* take note of the recursion depth to allow assigning recurse_op_bs= once */ > > +static uint64_t recurse_op_depth =3D 0; >=20 > The '=3D 0' is redundant; the C language guarantees that all static > variables are 0-initialized. >=20 > > + > > +/* set or unset an op blocker to a BDS whether set is true or false = */ > > +void bdrv_op_block_action(BlockDriverState *bs, BlockOpType op, > > + BlockerAction action, Error *reason) > > +{ >=20 > Not sure I follow that comment, since 'set' is not one of the parameter > names. >=20 >=20 > > + > > +/* Recursively set or unset an op block to a BDS tree whether set is= true or > > + * false > > + */ > > +void bdrv_recurse_op_block(BlockDriverState *bs, BlockOpType op, > > + BlockerAction action, Error *reason) >=20 > and again >=20 > > +{ > > + /* If recursion is detected simply return */ > > + if (recurse_op_bs =3D=3D bs) { > > + return; > > + } > > + > > + /* if we are starting recursion remeber the bs for later compari= son */ >=20 > s/remeber/remember/ >=20 > > + if (!recurse_op_depth) { > > + recurse_op_bs =3D bs; > > + } > > + > > + /* try to set or unset on bs->file and bs->backing_hd first */ > > + bdrv_op_block_action(bs->file, op, action, reason); > > + bdrv_op_block_action(bs->backing_hd, op, action, reason); > > + > > + /* if the BDS is a filter with multiple childs ask the driver to= recurse */ >=20 > s/childs/children/ >=20 >=20 > > +static void blkverify_recurse_op_block(BlockDriverState *bs, BlockOp= Type op, > > + BlockerAction action, Error *= reason) > > +{ > > + BDRVBlkverifyState *s =3D bs->opaque; > > + bdrv_op_block_action(bs->file, op , action, reason); > > + bdrv_op_block_action(s->test_file, op , action, reason); >=20 > s/ ,/,/ twice >=20 > > +++ b/include/block/block_int.h > > @@ -86,6 +86,11 @@ struct BlockDriver { > > */ > > bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs, > > BlockDriverState *candi= date); > > + /* In order to be able to recursively block operation on BDS tre= es filter > > + * like quorum can implement this callback >=20 > s/trees filter/trees, a filter/ >=20 > --=20 > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >=20