From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59930) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wxqwf-0002xC-Dw for qemu-devel@nongnu.org; Fri, 20 Jun 2014 01:01:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wxqwa-0004im-BQ for qemu-devel@nongnu.org; Fri, 20 Jun 2014 01:01:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:12159) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wxqwa-0004ic-33 for qemu-devel@nongnu.org; Fri, 20 Jun 2014 01:00:56 -0400 Date: Fri, 20 Jun 2014 13:01:06 +0800 From: Fam Zheng Message-ID: <20140620050106.GB15938@T430.redhat.com> 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> <20140619202043.GA18306@irqsave.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140619202043.GA18306@irqsave.net> 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: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: kwolf@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On Thu, 06/19 22:20, Beno=EEt Canet wrote: > 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 o= p blocker > >=20 > > s/arbitratry/arbitrary/ > >=20 > > > to recursively block or unblock whole BDS subtrees. I don't get the reason, can you elaborate? > > >=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 = broken. > > >=20 > > > Signed-off-by: Benoit Canet > > > --- > >=20 > > > + > > > +/* This remove unconditionally all blockers of type op of the subt= ree */ > >=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 pattern= s > > 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, = and > > 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 t= he > > two jobs ending, that would inappropriately stop blocking that action > > from the still on-going second action. The only way I see around tha= t > > 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). >=20 > I added this reset function for the case where a whole BDS subtree is d= etached > from the graph and will be destroyed. >=20 > It does happen in drive mirror and bdrv_unrefing it would lead to a fai= led > assertion. Which assertion is it? Maybe it is a bug somewhere else. The caller of re= set wouldn't know about other blockers, why is ignoring their blocking reason= and forcing the reset safe here? A BDS and its subtree will only be destroyed if their refcnts go to 0. In= this case, there should be no other blockers to be released other than the las= t user's. So we don't need the unconditional reset anyway, and I _do_ thin= k doing this is wrong and counter-design. >=20 > So the reset function take care of removing blocker of dead subtrees. >=20 > What would be a cleaner solution ? What is the question to solve? Fam