Re: [Qemu-devel] [PATCH v2 2/2] block: Make op blockers recursive

[Benoît Canet]

Thanks a lot for reviewing this patch.

Since the code is not trivial I will give my arguments for writing it
this way.


> > +/* block recursively a BDS
> > + *
> > + * If base != NULL the caller must make sure that there is no multiple 
> > child
> > + * BDS in the subtree pointed to by bs
> 
> In the case when base != NULL, should that really matter?  In a
> driver's .bdrv_op_recursive_block/unblock, if that driver has private
> children (multiple or not), shouldn't the private children be treated
> as one black box, and blocked / unblocked just like the parent
> BDS?

This is a stale comment. My bad.

> 
> For instance, what if we had a tree such as this:
> 
>                        [quorum1] <---- [active]
>                            |
>                            | (private)
>                            |
>                            v
> [node2]<-- [node1] <--- [imag> 
> if 'quorum1' was to be op blocked, and 'image1' and its children all
> comprise 'quorum1', wouldn't we always want to lock 'image1' all the
> way down to 'node2'?

That's what the patch does.

> 
> Or do we expect that someone will intentionally pass a 'base' pointer
> along that matches somewhere in the private child tree?

This is not expected by the caller but I wrote the patch so it will also work.

> 
> > + *
> > + * @bs:     the BDS to start to recurse from
> > + * @base:   the BDS where the recursion should end
> > + *          could be NULL if the recursion should go down everywhere
> > + * @op:     the type of op blocker to block
> > + * @reason: the error message associated with the blocking
> > + */
> > +void bdrv_op_block(BlockDriverState *bs, BlockDriverState *base,
> > +                   BlockOpType op, Error *reason)
> > +{
> > +    if (!bs) {
> > +        return;
> > +    }
> > +
> > +    /* did we recurse down to base ? */
> > +    if (bs == base) {
> > +        return;
> > +    }
> > +
> > +    /* prevent recursion loop */
> > +    if (bdrv_op_is_blocked_by(bs, op, reason)) {
> > +        return;
> > +    }
> 
> Should we handle this somehow (isn't this effectively an error, that
> will prematurely end the blocking of this particular line)? 

The main purpose of this is mirror.c and commit.c would form BDS loops on 
completion.
These callers could break the look manually but the code would fail
if a loop is not breaked and the blocker function are called on it.
So the blocker code have to handle recursion loops.

The bdrv_op_is_blocked_by match a reason being the criteria to test the blocker.

So this test will trigger only if a BDS is already blocked by the same reason
pointer.

This make the bdrv_op_block function idempotent.

note that it is the only sane way I found to make the blockers function handle
loops.

> 
> If we hit this, we are going to end up in a scenario where we haven't
> blocked the chain as requested, and we don't know the state of the
> blocking below this failure point.  Seems like the caller should know,
> and we should have a way of cleaning up.

If we hit this the chain was already blocked with the same reason pointer.

> 
> Actually, now I am wondering if we perhaps shouldn't be building a
> list to block, and then blocking everything atomically once we know
> there are no failure points.
>

I don't think this particular test is a failure point.

> > +
> > +    /* block first for recursion loop protection to work */
> > +    bdrv_do_op_block(bs, op, reason);
> > +
> > +    bdrv_op_block(bs->file, base, op, reason);
> > +
> > +    if (bs->drv && bs->drv->supports_backing) {
> > +        bdrv_op_block(bs->backing_hd, base, op, reason);
> > +    }
> > +
> > +    if (bs->drv && bs->drv->bdrv_op_recursive_block) {
> > +        bs->drv->bdrv_op_recursive_block(bs, base, op, reason);
> 
> Do we need to allow .bdrv_op_recursive_block() to fail and return
> error (and handle it, of course)?

I don't know yet: but lets let this question float a little more in the mail 
thread.

> 
> 
> 
> s/block/unblock
Thanks

> 
> 
> > + * @reason: the error message associated with the blocking
> > + */
> > +void bdrv_op_unblock(BlockDriverState *bs, BlockDriverState *base,
> > +                     BlockOpType op, Error *reason)
> > +{
> > +    if (!bs) {
> > +        return;
> > +    }
> > +
> > +    /* did we recurse down to base ? */
> > +    if (bs == base) {
> > +        return;
> > +    }
> > +
> > +    /* prevent recursion loop */
> > +    if (!bdrv_op_is_blocked_by(bs, op, reason)) {
> > +        return;
> > +    }
> 
> Do we want to do this negative check here?  Even if a given node is
> not blocked, its children may be, and I would assume (since this is
> recursive) that if I called bdrv_op_unblock() all of the chain down to
> 'base' would be unblocked for the given reason.
> 
> E.g. 
> 
>    [image1] <-- [image2] <-- [image3]
>    blocked      unblocked     blocked

To reach this state the caller code would have to do the following twisted 
sequence.

block(image3, with_reason1)
unblock(image2, with_reason1)
block(image1, with_reason1)

There is no such sequence in the code thanks to the base argument and we could
enforce that no such sequence ever get written.

> 
> I would assume that bdrv_op_unblock(image2, NULL, reason) would still
> unblock image1, even if image2 was unblocked.

Should we also assume that bdrv_op_unblock(image4, NULL, reason) with image4 
being
image3 parent unblock everything underneath ?

> > +static void quorum_op_recursive_block(BlockDriverState *bs,
> > +                                      BlockDriverState *base,
> > +                                      BlockOpType op,
> > +                                      Error *reason)
> > +{
> > +    BDRVQuorumState *s = bs->opaque;
> > +    int i;
> > +
> > +    for (i = 0; i < s->num_children; i++) {
> > +        bdrv_op_block(s->bs[i], base, op, reason);
> > +    }
> 
> I am wondering if the 'base' argument above should always be NULL in
> the case of private children.  I.e., the state of private children
> trees in their entirety should always reflect the state of the
> multi-children BDS parent.

Yes we could simply discard it.

> > +    /* helps blockers to propagate recursively */
> > +    void (*bdrv_op_recursive_block)(BlockDriverState *bs,
> > +                                    BlockDriverState *base,
> > +                                    BlockOpType op,
> > +                                    Error *reason);
> > +    void (*bdrv_op_recursive_unblock)(BlockDriverState *bs,
> > +                                      BlockDriverState *base,
> > +                                      BlockOpType op,
> > +                                      Error *reason);
> > +
> 
> Seems like these new functions would be better named '.bdrv_op_block'
> and '.bdrv_op_unblock'?  That way, recursive or not, it is clear block
> drivers can implement whatever blocking is appropriate for themselves.

I agree.

Best regards

Benoît