Live block commit

Add bdrv_find_overlay(), and bdrv_drop_intermediate().

bdrv_find_overlay(): given 'bs' and the active (topmost) BDS of an image chain,
find the image that is the immediate top of 'bs'

bdrv_drop_intermediate():
Given 3 BDS (active, top, base), drop images above base up to and including top, and set base to be the backing file of top's overlay node.

E.g., this converts:

bottom <- base <- intermediate <- top <- active

to

bottom <- base <- active

Signed-off-by Jeff Cody <[email protected]>

+/*
+ * Drops images above 'base' up to and including 'top', and sets the image
+ * above 'top' to have base as its backing file.
+ *
+ * Requires that the overlay to 'top' is opened r/w, so that the backing file
+ * information in 'bs' can be properly updated.
+ *
+ * E.g., this will convert the following chain:
+ * bottom <- base <- intermediate <- top <- active
+ *
+ * to
+ *
+ * bottom <- base <- active

+ *
+ * It is allowed for bottom==base, in which case it converts:
+ *
+ * base <- intermediate <- top <- active
+ *
+ * to
+ *
+ * base <- active

+ *
+ * base <- intermediate <- top
+ *
+ * becomes
+ *
+ * base <- top
+ *
+ * Error conditions:
+ * if active == top, that is considered an error
+ *
+ */

block.c +22 -0

+int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
+                           BlockDriverState *base)
+{
+    BlockDriverState *intermediate;
+    BlockDriverState *base_bs = NULL;
+    BlockDriverState *new_top_bs = NULL;
+    BlkIntermediateStates *intermediate_state, *next;
+    int ret = -EIO;
+
+    QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete;
+    QSIMPLEQ_INIT(&states_to_delete);
+
+    if (!top->drv || !base->drv) {
+        goto exit;
+    }
+
+    new_top_bs = bdrv_find_overlay(active, top);
+
+    if (new_top_bs == NULL) {
+        /* we could not find the image above 'top', this is an error */
+        goto exit;
+    }

This adds the live commit coroutine. This iteration focuses on the commit only below the active layer, and not the active layer itself.

The behaviour is similar to block streaming; the sectors are walked through, and anything that exists above 'base' is committed back down into base. At the end, intermediate images are deleted, and the chain stitched together. Images are restored to their original open flags upon completion.

Signed-off-by Jeff Cody <[email protected]>

block/commit.c new file +18 -0

+void commit_start(BlockDriverState *bs, BlockDriverState *base,
+                 BlockDriverState *top, int64_t speed,
+                 BlockErrorAction on_error, BlockDriverCompletionFunc *cb,
+                 void *opaque, Error **errp)
+{
+    CommitBlockJob *s;
+    BlockReopenQueue *reopen_queue = NULL;
+    int orig_overlay_flags;
+    int orig_base_flags;
+    BlockDriverState *overlay_bs;
+    Error *local_err = NULL;
+
+    if ((on_error == BLOCK_ERR_STOP_ANY ||
+         on_error == BLOCK_ERR_STOP_ENOSPC) &&
+        !bdrv_iostatus_is_enabled(bs)) {
+        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+        return;
+    }

Quoted 34 lines · Jeff Cody

This adds the live commit coroutine. This iteration focuses on the commit only below the active layer, and not the active layer itself.

The behaviour is similar to block streaming; the sectors are walked through, and anything that exists above 'base' is committed back down into base. At the end, intermediate images are deleted, and the chain stitched together. Images are restored to their original open flags upon completion.

Signed-off-by Jeff Cody <[email protected]>

block/commit.c new file +23 -0

+void commit_start(BlockDriverState *bs, BlockDriverState *base,
+                 BlockDriverState *top, int64_t speed,
+                 BlockErrorAction on_error, BlockDriverCompletionFunc *cb,
+                 void *opaque, Error **errp)
+{
+    CommitBlockJob *s;
+    BlockReopenQueue *reopen_queue = NULL;
+    int orig_overlay_flags;
+    int orig_base_flags;
+    BlockDriverState *overlay_bs;
+    Error *local_err = NULL;
+
+    if ((on_error == BLOCK_ERR_STOP_ANY ||
+         on_error == BLOCK_ERR_STOP_ENOSPC) &&
+        !bdrv_iostatus_is_enabled(bs)) {
+        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+        return;
+    }
+
+    overlay_bs = bdrv_find_overlay(bs, top);
+
+    if (overlay_bs == NULL) {
+        error_setg(errp, "Could not find overlay image for %s:", top->filename);

block/commit.c line 209

Quoted 5 lines

block/commit.c new file +5 -0

+        return;
+    }
+
+    orig_base_flags    = bdrv_get_flags(base);
+    orig_overlay_flags = bdrv_get_flags(overlay_bs);

I think you are missing a check here that base is on the backing chain of top. See also my comments to 5/7.

block/commit.c line 221

Quoted 12 lines

block/commit.c new file +12 -0

+
+    /* convert base_bs & overlay_bs to r/w, if necessary */
+    if (!(orig_base_flags & BDRV_O_RDWR)) {
+        reopen_queue = bdrv_reopen_queue(reopen_queue, base,
+                                         orig_base_flags | BDRV_O_RDWR);
+    }
+    if (!(orig_overlay_flags & BDRV_O_RDWR)) {
+        reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs,
+                                         orig_overlay_flags | BDRV_O_RDWR);
+    }
+    if (reopen_queue) {
+        bdrv_reopen_multiple(reopen_queue, &local_err);

Is it valid to make a no-op call, such as:

{ "execute":"block-commit", "arguments":{
"device":"drive0", "top":"base", "base":"base" }}

If so, should we do an early exit here, rather than temporarily changing base to R/W just to change it back to R/O?

If not, should we be rejecting it up front (again, back to the question of failing if 'base' is not a backing file of 'top', even if both 'top' and 'base' are backing files of 'device').

Quoted 8 lines · Eric Blake

Quoted 3 lines · Jeff Cody

This adds the live commit coroutine. This iteration focuses on the commit only below the active layer, and not the active layer itself.

I think you are missing a check here that base is on the backing chain of top. See also my comments to 5/7.

Did you mean your comments on 6/7 (or am I missing an email)?

This does get partially validated in patch 5/7, in the qmp_block_commit() handler - both base and top are verified to be in the chain 'bs'. What is not validated, however, is that you did not swap your 'top' and 'base' arguments. I'll add a check here for that, to make sure that base is reachable from overlay_bs.

This adds the live commit coroutine. This iteration focuses on the commit only below the active layer, and not the active layer itself.

The behaviour is similar to block streaming; the sectors are walked through, and anything that exists above 'base' is committed back down into base. At the end, intermediate images are deleted, and the chain stitched together. Images are restored to their original open flags upon completion.

Signed-off-by Jeff Cody <[email protected]>

This is a simple helper function, that will return the base image of a given image chain.

Signed-off-by Jeff Cody <[email protected]>

---
 block.c | 16 ++++++++++++++++
 block.h |  1 +
 2 files changed, 17 insertions(+)

Quoted 8 lines · Jeff Cody

This is a simple helper function, that will return the base image of a given image chain.

Signed-off-by Jeff Cody <[email protected]>

---
 block.c | 16 ++++++++++++++++
 block.h |  1 +
 2 files changed, 17 insertions(+)

Bikeshed question: Any reason patch 1/7 adds two helper functions, and patch 4/7 adds a third? Perhaps it would be worth squashing all three into one commit, or else having three commits, one per helper? But there's no point in repainting things now, so:

Reviewed-by Eric Blake <[email protected]>

The command for live block commit is added, which has the following arguments:

device: the block device to perform the commit on (mandatory)
base:   the base image to commit into; optional (if not specified,

it is the underlying original image)
top: the top image of the commit - all data from inside top down
to base will be committed into base. optional (if not specified, it is one below the active image) - see note below

speed: maximum speed, in bytes/sec

note: eventually this will support merging down the active layer,
but that code is not yet complete. If the active layer is passed in currently as top, or top is left to the default, then an error will be returned.

+++ b/qapi-schema.json

qapi-schema.json +7 -0

@@ -1468,6 +1468,41 @@
   'returns': 'str' }
 
 ##
+# @block-commit
+#
+# 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
+#

Quoted 9 lines · Jeff Cody

The command for live block commit is added, which has the following arguments:

device: the block device to perform the commit on (mandatory)
base:   the base image to commit into; optional (if not specified,

it is the underlying original image)
top: the top image of the commit - all data from inside top down
to base will be committed into base. optional (if not specified, it is one below the active image) - see note below

This says that for 'base' <- 'mid' <- 'active', omitting 'top' is the same as specifying 'top':'mid'.

Quoted 6 lines

speed: maximum speed, in bytes/sec

note: eventually this will support merging down the active layer,
but that code is not yet complete. If the active layer is passed in currently as top, or top is left to the default, then an error will be returned.

This says that for 'base' <- 'mid' <- 'active', omitting 'top' is an error (because it would be the same as an explicit 'top:'active').

Let's check the code...

blockdev.c void qmp_block_stream(const char *device, bool has_base, · line 1167

Quoted 8 lines

blockdev.c +8 -0

+    if (top && has_top) {
+        /* if we want to allow the active layer,
+         * use 'bdrv_find_image()' here */
+        top_bs = bdrv_find_backing_image(bs, top);
+    } else {
+        /* default is one below the active layer, unless that is
+         * the base */
+        top_bs = bs->backing_hd;

Aha - the former is correct as coded (you default to 'top':'mid' in my example), so the 'note' in your commit message needs editing.

On the other hand, when we ever DO get around to adding live commit, which is the saner default? That is, am I more likely to want to do live commit from 'active', or more likely to do commit of the layer below 'active'? If live commit is the more desirable case, then that argues that THIS commit should always error out if !has_top, and that the future patch will default top_bs = bs, and the rest of your commit message and documentation would need tweaking accordingly.

I don't have a preference either way (libvirt can arrange to always pass the top argument, and thus avoid the confusion when top is omitted), but if anyone else cares, now is the time to get the API right before we are locked in to it.

Quoted 12 lines

+++ b/qapi-schema.json

qapi-schema.json +7 -0

@@ -1468,6 +1468,41 @@
   'returns': 'str' }
 
 ##
+# @block-commit
+#
+# 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
+#

qapi-schema.json line 1487

Quoted 10 lines

qapi-schema.json +10 -0

+# @base:   #optional The file name of the backing image to write data into.
+#                    If not 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 one layer below the active
+#                    layer (i.e. active->backing_hd).
+#
+#                    If top == base, that is an error.
+#

This documentation of @top matches the first paragraph of your commit message.

qapi-schema.json line 1496

Quoted 9 lines

qapi-schema.json +9 -0

+#
+# @speed:  #optional the maximum speed, in bytes per second
+#
+# Returns: Nothing on success
+#          If commit or stream is already active on this device, DeviceInUse
+#          If @device does not exist, DeviceNotFound
+#          If image commit is not supported by this device, NotSupported
+#          If @base does not exist, a generic error is returned
+#          If @top does not exist, a generic error is returned

These two lines could be merged, or even made more generic (for example, based on the rest of this thread, you will also be erroring out if base and top exist, but appear as swapped arguments):

If @base or @top is invalid, a generic error is returned

The command for live block commit is added, which has the following arguments:

device: the block device to perform the commit on (mandatory)
base:   the base image to commit into; optional (if not specified,

it is the underlying original image)
top: the top image of the commit - all data from inside top down
to base will be committed into base. optional (if not specified, it is one below the active image) - see note below
speed: maximum speed, in bytes/sec

note: eventually this will support merging down the active layer,
but that code is not yet complete. If the active layer is passed in currently as top, or top is left to the default, then an error will be returned.

The is done as a block job, so upon completion a BLOCK_JOB_COMPLETED will be emitted.

Signed-off-by Jeff Cody <[email protected]>

qapi-schema.json +4 -0

index 14e4419..e614453 100644
@@ -1468,6 +1468,41 @@
   'returns': 'str' }
 
 ##
+# @block-commit
+#
+# Live commit of data from overlay image nodes into backing nodes - i.e.,
+# writes data between 'top' and 'base' into 'base'.

qapi-schema.json +10 -0

+#
+#                    If top == base, that is an error.
+#
+#
+# @speed:  #optional the maximum speed, in bytes per second
+#
+# Returns: Nothing on success
+#          If commit or stream is already active on this device, DeviceInUse
+#          If @device does not exist, DeviceNotFound
+#          If image commit is not supported by this device, NotSupported

Am 25.09.2012 18:29, schrieb Jeff Cody:

Quoted 38 lines

The command for live block commit is added, which has the following arguments:

device: the block device to perform the commit on (mandatory)
base:   the base image to commit into; optional (if not specified,

it is the underlying original image)
top: the top image of the commit - all data from inside top down
to base will be committed into base. optional (if not specified, it is one below the active image) - see note below
speed: maximum speed, in bytes/sec

note: eventually this will support merging down the active layer,
but that code is not yet complete. If the active layer is passed in currently as top, or top is left to the default, then an error will be returned.

The is done as a block job, so upon completion a BLOCK_JOB_COMPLETED will be emitted.

Signed-off-by Jeff Cody <[email protected]>

qapi-schema.json +9 -0

index 14e4419..e614453 100644
@@ -1468,6 +1468,41 @@
   'returns': 'str' }
 
 ##
+# @block-commit
+#
+# 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
+#
+# @base:   #optional The file name of the backing image to write data into.
+#                    If not specified, this is the deepest backing image

qapi-schema.json line 1484

Quoted 5 lines

qapi-schema.json +5 -0

+#
+# @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 one layer below the active
+#                    layer (i.e. active->backing_hd).

Why isn't active the default any more? I know, we don't support it yet, but long term this is what makes most sense as a default.

Quoted 10 lines

qapi-schema.json +10 -0

+#
+#                    If top == base, that is an error.
+#
+#
+# @speed:  #optional the maximum speed, in bytes per second
+#
+# Returns: Nothing on success
+#          If commit or stream is already active on this device, DeviceInUse
+#          If @device does not exist, DeviceNotFound
+#          If image commit is not supported by this device, NotSupported

qapi-schema.json line 1504

Quoted 10 lines

qapi-schema.json +10 -0

+#          If @base does not exist, a generic error is returned
+#          If @top does not exist, a generic error is returned
+#          If @speed is invalid, InvalidParameter
+#
+# Since: 1.3
+#
+##
+{ 'command': 'block-commit',
+  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
+            '*speed': 'int' } }

Kevin

Quoted 39 lines

Am 25.09.2012 18:29, schrieb Jeff Cody:

Quoted 38 lines

The command for live block commit is added, which has the following arguments:

device: the block device to perform the commit on (mandatory)
base:   the base image to commit into; optional (if not specified,

it is the underlying original image)
top: the top image of the commit - all data from inside top down
to base will be committed into base. optional (if not specified, it is one below the active image) - see note below
speed: maximum speed, in bytes/sec

note: eventually this will support merging down the active layer,
but that code is not yet complete. If the active layer is passed in currently as top, or top is left to the default, then an error will be returned.

The is done as a block job, so upon completion a BLOCK_JOB_COMPLETED will be emitted.

Signed-off-by Jeff Cody <[email protected]>

qapi-schema.json +9 -0

index 14e4419..e614453 100644
@@ -1468,6 +1468,41 @@
   'returns': 'str' }
 
 ##
+# @block-commit
+#
+# 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
+#
+# @base:   #optional The file name of the backing image to write data into.
+#                    If not specified, this is the deepest backing image

Quoted 9 lines

qapi-schema.json line 1484

Quoted 5 lines

qapi-schema.json +5 -0

+#
+# @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 one layer below the active
+#                    layer (i.e. active->backing_hd).

Why isn't active the default any more? I know, we don't support it yet, but long term this is what makes most sense as a default.

Eric had a similar question, and asked if anyone had any preference - this was my response:

---

I guess I don't have a strong preference either - I originally had it the other way, but then that meant the default in the current implementation was actually an error.

Also, I assumed (danger!) that the most common use of commit would be a snapshot, followed by a commit of active->backing_hd. With that assumption, it seemed like a sane default.

---

I can certainly revert back to having the active layer be the top, if that is the preference.

Quoted 8 lines

qapi-schema.json line 1484

Quoted 4 lines

qapi-schema.json +4 -0

+# @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 one layer below the active
+#                    layer (i.e. active->backing_hd).

Why isn't active the default any more? I know, we don't support it yet, but long term this is what makes most sense as a default.

Eric had a similar question, and asked if anyone had any preference - this was my response:

---

I guess I don't have a strong preference either - I originally had it the other way, but then that meant the default in the current implementation was actually an error.

Also, I assumed (danger!) that the most common use of commit would be a snapshot, followed by a commit of active->backing_hd. With that assumption, it seemed like a sane default.

---

I can certainly revert back to having the active layer be the top, if that is the preference.

Derived from the streaming test cases (030), this adds the following tests:

1. For the following image chain, commit [mid] into [backing], and use qemu-io to verify [backing] has its original data, as well as the data from [mid]

           [backing] <-- [mid] <-- [test]

2. Verifies that 'block-commit' with the 'speed' parameter sets the speed parameter, as reported by 'query-block-jobs'

3. Verifies that a bogus 'device' parameter to 'block-commit' results in error

Quoted 7 lines · Jeff Cody

Derived from the streaming test cases (030), this adds the following tests:

1. For the following image chain, commit [mid] into [backing], and use qemu-io to verify [backing] has its original data, as well as the data from [mid]

Quoted 7 lines

           [backing] <-- [mid] <-- [test]

2. Verifies that 'block-commit' with the 'speed' parameter sets the speed parameter, as reported by 'query-block-jobs'

3. Verifies that a bogus 'device' parameter to 'block-commit' results in error

I think you are missing a test; you should also verify that:

{ "command":"block-commit", "arguments":{
"device":"drive0", "base":"mid", "top":"backing" } }

properly fails, since 'mid' is not a backing file of 'backing'. I saw code in patch 1/7 that bdrv_drop_intermediate() should detect the situation, but I'm not confident enough in my reading of patch 2/7 to know if that detection point was early enough, or whether the coroutine stuff in 2/7 ends up corrupting 'mid' prior to failure.

Currently, after a live snapshot of a drive, the image that has been 'demoted' to be below the new active layer remains r/w. This patch reopens it read-only.

Note that we do not check for error on the reopen(), because we will not abort the snapshots if the reopen fails.

This patch depends on the bdrv_reopen() series.

Signed-off-by Jeff Cody <[email protected]>

Am 25.09.2012 18:29, schrieb Jeff Cody:

Quoted 7 lines

Currently, after a live snapshot of a drive, the image that has been 'demoted' to be below the new active layer remains r/w. This patch reopens it read-only.

Note that we do not check for error on the reopen(), because we will not abort the snapshots if the reopen fails.

Quoted 3 lines

This patch depends on the bdrv_reopen() series.

Signed-off-by Jeff Cody <[email protected]>

This should be independent from the live commit patches, so I already applied this one to the block branch.

Kevin