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

[Benoît Canet]

The Thursday 19 Jun 2014 à 17:17:16 (+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.
> 
> 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.

I tried to write a recursive function to block and unblock all child nodes 
today.

It turn that even if the recursion functions are easy to write there are some
hurdle with the approach.

In drive mirror when a subtree is swapped nobody will be here to unlock it
and bdrv_unref will fail.

In block-commmit the drive mirror run part form a BDS loop that is totally 
uneasy
to unblock properly in bdrv_set_backing_file.

So writing these functions is not trivial and we are late in the release cycle.

Are you sure that there is no lighter path for the quorum and node name 
patchset ?

Best regards

Benoît

> 
> Stefan