Re: [Qemu-devel] [PATCH v4 05/10] block: Accept node-name arguments for block-commit

[Jeff Cody <jcody@redhat.com>]

On Wed, Jun 04, 2014 at 03:38:55PM -0600, Eric Blake wrote:
> On 06/04/2014 07:51 AM, Jeff Cody wrote:
> > This modifies the block operation block-commit so that it will
> > accept node-name arguments for either 'top' or 'base' BDS.
> > 
> > The filename and node-name are mutually exclusive to each other;
> > i.e.:
> >     "top" and "top-node-name" are mutually exclusive (enforced)
> >     "base" and "base-node-name" are mutually exclusive (enforced)
> > 
> > The "device" argument now becomes optional as well, because with
> > a node-name we can identify the block device chain.  It is only
> > optional if a node-name is not specified.
> 
> Stale paragraph; just delete it.  [We may later want to add patches to
> make device optional, if we can figure out a solution for what to do
> when a BDS has more than one overlay; but that doesn't need to stall
> this series]

Argh, yes.  Forgot to update the commit message here, thanks.  If a v5
is needed, I'll delete it, otherwise perhaps a kind maintainer could
fix it up for me before applying :)

> 
> > 
> > The preferred and recommended method now of using block-commit
> > is to specify the BDS to operate on via their node-name arguments.
> > 
> > This also adds an explicit check that base_bs is in the chain of
> > top_bs, because if they are referenced by their unique node-name
> > arguments, the discovery process does not intrinsically verify
> > they are in the same chain.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  blockdev.c       | 55 +++++++++++++++++++++++++++++++++++++++++++++++--------
> >  qapi-schema.json | 37 +++++++++++++++++++++++++++----------
> >  qmp-commands.hx  | 31 +++++++++++++++++++++++--------
> >  3 files changed, 97 insertions(+), 26 deletions(-)
> > 
> 
> > -    bs = bdrv_find(device);
> > -    if (!bs) {
> > -        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> > +    /* argument combination validation */
> > +    if (has_base && has_base_node_name) {
> > +        error_setg(errp, "'base' and 'base-node-name' are mutually exclusive");
> > +        return;
> > +    }
> 
> An alternative would have been to validate that both 'base' and
> 'base-node-name' resolve to the same BDS, but I like the simplicity of
> erroring out here.
> 
> > +
> > +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
> > +        return;
> > +    }
> 
> [unrelated to this commit - are we sure that the semantics of the commit
> blocker are correct? If we implement BDS reuse among multiple chains,
> consider what happens if I have:
>        / sn1 <- sn2
> base <
>        \ file3
> 
> Even if there is no other block job operating on sn2 or its chain, we
> really want to block an attempt to merge file3 into base, as modifying
> base would corrupt both sn1 and sn2.  That is, a BDS being a common
> backing between more than one chain should automatically behave as an
> op-blocker for commits - where the block is what you are committing
> into, not where you are committing from.]


No, the commit blocker likely won't catch everything.  But that is
because there are additional enhancements the blocker code needs (it
is ultimately intended to block on an individual BDS, but in reality
currently operates on the active layer BDS for the whole chain).

For every block job, the appropriate blockers need to be set on each
BDS affected by the job (intermediate images, overlay images in some
cases, the target, etc..).

It currently functions like the older in_use flag (blocks a chain as
referenced by the active layer), but the groundwork is in place for
truly independent BDS blockers to come later.

(For more discussion, see the qemu-devel list replies to:
[PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller)

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
>

Thanks!