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
