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

["Benoît Canet" <benoit.canet@irqsave.net>]

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.
> > 
> > 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 <benoit@irqsave.net>
> > ---
> 
> > +
> > +/* 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.

So the reset function take care of removing blocker of dead subtrees.

What would be a cleaner solution ?

Best regards

Benoît

> 
> 
> >  
> > +/* This remove unconditionally all blockers */
> 
> Unconditionally remove all blockers
> 
> 
> >  
> > +/* Used to prevent recursion loop. A case exists in block commit mirror usage */
> > +static BlockDriverState *recurse_op_bs = NULL;
> > +/* take note of the recursion depth to allow assigning recurse_op_bs once */
> > +static uint64_t recurse_op_depth = 0;
> 
> The '= 0' is redundant; the C language guarantees that all static
> variables are 0-initialized.
> 
> > +
> > +/* 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)
> > +{
> 
> Not sure I follow that comment, since 'set' is not one of the parameter
> names.
> 
> 
> > +
> > +/* 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)
> 
> and again
> 
> > +{
> > +    /* If recursion is detected simply return */
> > +    if (recurse_op_bs == bs) {
> > +        return;
> > +    }
> > +
> > +    /* if we are starting recursion remeber the bs for later comparison */
> 
> s/remeber/remember/
> 
> > +    if (!recurse_op_depth) {
> > +        recurse_op_bs = 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 */
> 
> s/childs/children/
> 
> 
> > +static void blkverify_recurse_op_block(BlockDriverState *bs, BlockOpType op,
> > +                                       BlockerAction action, Error *reason)
> > +{
> > +    BDRVBlkverifyState *s = bs->opaque;
> > +    bdrv_op_block_action(bs->file, op , action, reason);
> > +    bdrv_op_block_action(s->test_file, op , action, reason);
> 
> s/ ,/,/ twice
> 
> > +++ b/include/block/block_int.h
> > @@ -86,6 +86,11 @@ struct BlockDriver {
> >       */
> >      bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs,
> >                                               BlockDriverState *candidate);
> > +    /* In order to be able to recursively block operation on BDS trees filter
> > +     * like quorum can implement this callback
> 
> s/trees filter/trees, a filter/
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>