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

[Stefan Hajnoczi]

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.

Stefan

pgpg_w0XxxJgV.pgp
Description: PGP signature