| Message ID | 5da6a0b4d7cf0177bb5bf8e2e74a49769d90120c.1401886089.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
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] > > 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.] Reviewed-by: Eric Blake <eblake@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!
diff --git a/blockdev.c b/blockdev.c index 58b526a..50d4890 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1915,12 +1915,15 @@ void qmp_block_stream(const char *device, bool has_base, void qmp_block_commit(const char *device, bool has_base, const char *base, + bool has_base_node_name, const char *base_node_name, bool has_top, const char *top, + bool has_top_node_name, const char *top_node_name, bool has_speed, int64_t speed, Error **errp) { - BlockDriverState *bs; - BlockDriverState *base_bs, *top_bs; + BlockDriverState *bs = NULL; + BlockDriverState *base_bs = NULL; + BlockDriverState *top_bs = NULL; Error *local_err = NULL; /* This will be part of the QMP command, if/when the * BlockdevOnError change for blkmirror makes it in @@ -1934,20 +1937,33 @@ void qmp_block_commit(const char *device, /* drain all i/o before commits */ bdrv_drain_all(); - 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; + } + if (has_top && has_top_node_name) { + error_setg(errp, "'top' and 'top-node-name' are mutually exclusive"); return; } - if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) { + /* device lookups */ + bs = bdrv_find(device); + if (!bs) { + error_set(errp, QERR_DEVICE_NOT_FOUND, device); return; } /* default top_bs is the active layer */ top_bs = bs; - if (has_top && top) { + if (has_top_node_name) { + top_bs = bdrv_lookup_bs(NULL, top_node_name, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + } else if (has_top && top) { if (strcmp(bs->filename, top) != 0) { top_bs = bdrv_find_backing_image(bs, top); } @@ -1958,7 +1974,13 @@ void qmp_block_commit(const char *device, return; } - if (has_base && base) { + if (has_base_node_name) { + base_bs = bdrv_lookup_bs(NULL, base_node_name, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + } else if (has_base && base) { base_bs = bdrv_find_backing_image(top_bs, base); } else { base_bs = bdrv_find_base(top_bs); @@ -1969,6 +1991,23 @@ void qmp_block_commit(const char *device, return; } + /* Verify that 'base' is in the same chain as 'top' */ + if (!bdrv_chain_contains(top_bs, base_bs)) { + error_setg(errp, "'base' and 'top' are not in the same chain"); + return; + } + + /* This should technically be caught in commit_start, but + * check here for validation completeness */ + if (!bdrv_chain_contains(bs, top_bs)) { + error_setg(errp, "'%s' and 'top' are not in the same chain", device); + return; + } + + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) { + return; + } + if (top_bs == bs) { commit_active_start(bs, base_bs, speed, on_error, block_job_cb, bs, &local_err); diff --git a/qapi-schema.json b/qapi-schema.json index 97cf053..67a627f 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2097,14 +2097,29 @@ # Live commit of data from overlay image nodes into backing nodes - i.e., # writes data between 'top' and 'base' into 'base'. # -# @device: the name of the device +# @device: The name of the device. # -# @base: #optional The file name of the backing image to write data into. -# If not specified, this is the deepest backing image +# For 'base', either @base or @base-node-name may be set but not both. If +# neither is specified, this is the deepest backing image. # -# @top: #optional The file name of the backing image within the image chain, -# which contains the topmost data to be committed down. If -# not specified, this is the active layer. +# @base: #optional The file name of the backing image to write data +# into. +# +# @base-node-name #optional The name of the block driver state node of the +# backing image to write data into. +# (Since 2.1) +# +# For 'top', either @top or @top-node-name must be set but not both. If +# neither is specified, this is the active layer. +# +# @top: #optional The file name of the backing image within the image +# chain, which contains the topmost data to be +# committed down. +# +# @top-node-name: #optional The block driver state node name of the backing +# image within the image chain, which contains the +# topmost data to be committed down. +# (Since 2.1) # # If top == base, that is an error. # If top == active, the job will not be completed by itself, @@ -2123,17 +2138,19 @@ # # Returns: Nothing on success # If commit or stream is already active on this device, DeviceInUse -# If @device does not exist, DeviceNotFound +# If @device does not exist or cannot be determined, DeviceNotFound # If image commit is not supported by this device, NotSupported -# If @base or @top is invalid, a generic error is returned +# If @base, @top, @base-node-name, @top-node-name invalid, GenericError # If @speed is invalid, InvalidParameter +# If both @base and @base-node-name are specified, GenericError +# If both @top and @top-node-name are specified, GenericError # # Since: 1.3 # ## { 'command': 'block-commit', - 'data': { 'device': 'str', '*base': 'str', '*top': 'str', - '*speed': 'int' } } + 'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str', + '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } } ## # @drive-backup diff --git a/qmp-commands.hx b/qmp-commands.hx index 6b67d2f..c61f4cb 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -985,7 +985,7 @@ EQMP { .name = "block-commit", - .args_type = "device:B,base:s?,top:s?,speed:o?", + .args_type = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,speed:o?", .mhandler.cmd_new = qmp_marshal_input_block_commit, }, @@ -998,13 +998,28 @@ data between 'top' and 'base' into 'base'. Arguments: -- "device": The device's ID, must be unique (json-string) -- "base": The file name of the backing image to write data into. - If not specified, this is the deepest backing image - (json-string, optional) -- "top": The file name of the backing image within the image chain, - which contains the topmost data to be committed down. If - not specified, this is the active layer. (json-string, optional) +- "device": The device's ID, must be unique (json-string) + +For 'base', either base or base-node-name may be set but not both. If +neither is specified, this is the deepest backing image + +- "base": The file name of the backing image to write data into. + (json-string, optional) +- "base-node-name": The name of the block driver state node of the + backing image to write data into. + (json-string, optional) (Since 2.1) + +For 'top', either top or top-node-name must be set but not both. If +neither is specified, this is the active layer + +- "top": The file name of the backing image within the image chain, + which contains the topmost data to be committed down. + (json-string, optional) + +- "top-node-name": The block driver state node name of the backing + image within the image chain, which contains the + topmost data to be committed down. + (json-string, optional) (Since 2.1) If top == base, that is an error. If top == active, the job will not be completed by itself,
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. 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(-)