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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4242 bytes --]

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).


>  
> +/* 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]