Re: [Qemu-devel] [PATCH] block: Make op blocker recursive

[Fam Zheng]

On Thu, 06/19 22:20, Benoît Canet wrote:
> The Thursday 19 Jun 2014 à 14:13:20 (-0600), Eric Blake wrote :
> > On 06/19/2014 02:01 PM, Benoît Canet wrote:
> > > As the code will start to operate on arbitratry nodes we need the op 
> > > blocker
> > 
> > s/arbitratry/arbitrary/
> > 
> > > to recursively block or unblock whole BDS subtrees.

I don't get the reason, can you elaborate?

> > > 
> > > Also add a function to reset all blocker from a BDS.
> > > 
> > > This patch also take care of changing blocker user so they are not broken.
> > > 
> > > Signed-off-by: Benoit Canet <address@hidden>
> > > ---
> > 
> > > +
> > > +/* This remove unconditionally all blockers of type op of the subtree */
> > 
> > This unconditionally removes all blockers of type op of the subtree
> > 
> > Yikes - is that really what we want?  Or do we need to start doing
> > blocker reference counting?
> > 
> > Consider:
> > 
> > base <- snap1 <- active
> > 
> > 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, and
> > we start a drive-mirror on active.
> > 
> > base <- snap1 <- active
> >               |        \-- fleecing
> >               \-- copy
> > 
> > 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 detached
> from the graph and will be destroyed.
> 
> It does happen in drive mirror and bdrv_unrefing it would lead to a failed
> assertion.

Which assertion is it? Maybe it is a bug somewhere else. The caller of reset
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 last
user's.  So we don't need the unconditional reset anyway, and I _do_ think
doing this is wrong and counter-design.

> 
> So the reset function take care of removing blocker of dead subtrees.
> 
> What would be a cleaner solution ?

What is the question to solve?

Fam