Re: [Qemu-devel] [PATCH v6 for 2.1 00/10] Modify block jobs to use node-names

[Jeff Cody <jcody@redhat.com>]

On Thu, Jun 19, 2014 at 05:17:16PM +0800, Stefan Hajnoczi wrote:
> On Tue, Jun 17, 2014 at 05:53:48PM -0400, Jeff Cody wrote:
> > Changes from v5->v6:
> > 
> > * Check for attempt to commit an image to itself (Eric)
> > * Add a comment to the bdrv_find for block-commit, indicating
> >   that libvirt uses the error case for probing (Eric)
> > * Added Benoit's R-b's
> > 
> > Changes from v4->v5:
> > 
> > * Rebased on master
> > * Fixed commit log typos / stale paragraphs (Eric)
> > * Fixed comment typo (Eric)
> > * Added Eric's remaining R-b's
> > 
> > 
> > Changes from v3->v4:
> > 
> > * Rebased on master
> > * Dropped overlay pointers, Eric's concerns are correct
> > * Require "device" for all arguments, in light of the above,
> >   so we can find the active layer in all cases.
> > * Simplify more operations!
> > * Dropped Eric's Reviewed-by: on patches 3,5,6
> >   Added Eric's Reviewed-by: on patches 8,9
> > 
> > 
> > Changes from v2->v3:
> > 
> > * Add Eric's reviewed-by
> > * Addressed Eric's review comments
> > * Dropped HMP changes
> > * Added helper function for setting the overlay, and
> >   set the overlay in bdrv_append()
> > * Use bs->backing_file instead of bs->backing_hd->filename in block_stream 
> > 
> > Using node-names instead of filenames for block job operations
> > over QMP is a superior method of identifying the block driver
> > images to operate on, as it removes all pathname ambiguity.
> > 
> > This series modifies block-commit and block-stream to use node-names,
> > and creates a new QAPI command to allow stand-alone backing file
> > changes on an image file.
> > 
> > So that node-names can be used as desired for all block job
> > operations, this series also auto-generates node-names for every
> > BDS.  User-specified node-names will override any autogenerated
> > 
> > Jeff Cody (10):
> >   block: Auto-generate node_names for each BDS entry
> >   block: add helper function to determine if a BDS is in a chain
> >   block: simplify bdrv_find_base() and bdrv_find_overlay()
> >   block: make 'top' argument to block-commit optional
> >   block: Accept node-name arguments for block-commit
> >   block: extend block-commit to accept a string for the backing file
> >   block: add ability for block-stream to use node-name
> >   block: add backing-file option to block-stream
> >   block: Add QMP documentation for block-stream
> >   block: add QAPI command to allow live backing file change
> > 
> >  block.c                   |  80 ++++++++--------
> >  block/commit.c            |   9 +-
> >  block/stream.c            |  11 +--
> >  blockdev.c                | 238 ++++++++++++++++++++++++++++++++++++++++++----
> >  hmp.c                     |   1 +
> >  include/block/block.h     |   4 +-
> >  include/block/block_int.h |   3 +-
> >  qapi/block-core.json      | 145 +++++++++++++++++++++++++---
> >  qmp-commands.hx           | 181 +++++++++++++++++++++++++++++++++--
> >  tests/qemu-iotests/040    |  28 ++++--
> >  10 files changed, 602 insertions(+), 98 deletions(-)
> 
> This series side-steps lack of child op blockers by checking only the
> root node/drive.
> 
> Existing node-name commands like resize and snapshot-sync check for op
> blockers on the actual node.  They do not take the same approach as this
> patch series.
> 
> We have a mess and I don't want to commit this series before we've
> figured out what to do about child op blockers.
> 
> Let's discuss this topic in a sub-thread and figure out what to do for
> QEMU 2.1.  This is an important issue to solve before the release
> because we can't change QMP command semantics easily later.
> 
> My questions are:
> a. How do we fix resize, snapshot-sync, etc?  It seems like we need to
>    propagate child op blockers.
> 
> b. Is it a good idea to perform op blocker checks on the root node?
>    It's inconsistent with resize, snapshot-sync, etc.  Permissions in
>    BDS graphs with multiple root nodes (e.g. guest device and NBD
>    run-time server) will be different depending on which root you
>    specify.

Looking at the code, backing_blocker on all backing files currently
breaks block-commit.

>From commit_run(), in block/commit.c (~line 89):

    if (base_len < s->common.len) {
        ret = bdrv_truncate(base, s->common.len);
        if (ret) {
            goto exit_restore_reopen;
        }
    }

We grow the base, if the top layer is larger, otherwise we get -EIO.
Now, with the backing blockers, from bdrv_truncate() in block.c:

    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, NULL)) {
        return -EBUSY;
    }

So this will be a regression for 2.1, unless we fix it. 

Per IRC conversation, I think we can safely move the
bdrv_op_is_blocked() check to qmp_block_resize().

I'll put a patch out to do this, and add a test to qemu-iotests.

> 
> c. We're painting ourselves into a corner by using the root node for op
>    blocker checks.  We'll have to apply the same op blockers to all
>    nodes in a graph.  There's no opportunity to apply different op
>    blockers to a subset of the child nodes.  I *think* this can be
>    changed later without affecting the QMP API, so it's not a critical
>    issue.
> 
> The answer seems to be that op blockers should be propagated to all
> child nodes and commands should test the node, not the drive/root node.
> That gives us the flexibility for per-node op blockers in the future and
> maintains compatibility with existing node-name users.
> 
> Stefan