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 <ben...@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
>
