Live block commit

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 stiched together. Images are restored to their original open

block_int.h +13 -0

+ * @cb: Completion function for the job.
+ * @opaque: Opaque pointer value passed to @cb.
+ * @orig_base_flags: The original open flags for the base image
+ * @orig_top_flags: The original open flags for the top image
+ * @errp: Error object.
+ *
+ */
+void commit_start(BlockDriverState *bs, BlockDriverState *base,
+                 BlockDriverState *top, int64_t speed,
+                 BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
+                 void *opaque, int orig_base_flags, int orig_top_flags,
+                 Error **errp);
+
 #endif /* BLOCK_INT_H */

Add bdrv_find_image(), bdrv_find_base(), and bdrv_delete_intermediate().

bdrv_find_image(): given a filename and a BDS, find the image in the chain
that matches the passed filename.

bdrv_find_base(): given a BDS, find the base image (parent-most image)

bdrv_delete_intermediate():
Given 3 BDS (active, top, base), delete images above base up to and including top, and set base to be the parent of top's child node.

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

---
 block.c |  136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 block.h |    4 ++
 2 files changed, 139 insertions(+), 1 deletion(-)

block.c +0 -0

index 522acd1..a3c8d33 100644

block.c +1 -0

@@ -1661,6 +1661,110 @@ int bdrv_change_backing_file(BlockDriverState *bs,
     return ret;
 }
 
+typedef struct BlkIntermediateStates {

block.c +1 -0

@@ -3128,6 +3232,36 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
     return NULL;
 }
 
+BlockDriverState *bdrv_find_image(BlockDriverState *bs,

block.c +6 -0

+    }
+}
+
+BlockDriverState *bdrv_find_base(BlockDriverState *bs)
+{
+    BlockDriverState *curr_bs = NULL;

Quoted 7 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 stiched together. Images are restored to their original open

s/stiched/stitched/

block/commit.c line 27

Quoted 9 lines

block/commit.c new file +9 -0

+
+enum {
+    /*
+     * Size of data buffer for populating the image file.  This should be large
+     * enough to process multiple clusters in a single call, so that populating
+     * contiguous regions of the image is efficient.
+     */
+    COMMIT_BUFFER_SIZE = 512 * 1024, /* in bytes */
+};

Paolo's latest round of patches got to the point of making this configurable for drive-mirror; is that something you should be copying here?

Quoted 6 lines

+++ b/block_int.h

block_int.h +0 -0

@@ -342,4 +342,23 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
                   BlockDriverCompletionFunc *cb,
                   void *opaque, Error **errp);
 

block_int.h void mirror_start(BlockDriverState *bs, BlockDriverState *target, · line 350

Quoted 6 lines

block_int.h +6 -0

+/**
+ * commit_start:
+ * @bs: Top Block device
+ * @base: Block device that will be written into, and become the new top
+ * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
+ * @on_error: The action to take upon error.

Do we need to distinguish between rerror and werror?

Quoted 15 lines

block_int.h +13 -0

+ * @cb: Completion function for the job.
+ * @opaque: Opaque pointer value passed to @cb.
+ * @orig_base_flags: The original open flags for the base image
+ * @orig_top_flags: The original open flags for the top image
+ * @errp: Error object.
+ *
+ */
+void commit_start(BlockDriverState *bs, BlockDriverState *base,
+                 BlockDriverState *top, int64_t speed,
+                 BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
+                 void *opaque, int orig_base_flags, int orig_top_flags,
+                 Error **errp);
+
 #endif /* BLOCK_INT_H */

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

---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+)

qerror.c +0 -0

index 25c2733..69a59ab 100644

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 the active image) - see note below
speed: maximum speed, in bytes/sec
on_error: action to take on error (optional - default is report)

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 the error QERR_TOP_NOT_FOUND will be returned.

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

+++ b/qapi-schema.json

qapi-schema.json +1 -0

@@ -1365,6 +1365,39 @@
   'returns': 'str' } 
 
 ##
+# @block-commit

qapi-schema.json +6 -0

+# @on_error: #optional the action to take on an error (default report)
+#
+# 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

Quoted 7 lines · Jeff Cody

Add bdrv_find_image(), bdrv_find_base(), and bdrv_delete_intermediate().

bdrv_find_image(): given a filename and a BDS, find the image in the chain
that matches the passed filename.

bdrv_find_base(): given a BDS, find the base image (parent-most image)

Quoted 4 lines

bdrv_delete_intermediate():
Given 3 BDS (active, top, base), delete images above base up to and including top, and set base to be the parent of top's child node.

A diagram, as was in your cover letter, would help (either here, or better yet in the comments describing this function in place)...

Quoted 13 lines

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

---
 block.c |  136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 block.h |    4 ++
 2 files changed, 139 insertions(+), 1 deletion(-)

block.c +0 -0

index 522acd1..a3c8d33 100644
@@ -1408,7 +1408,7 @@ int bdrv_commit(BlockDriverState *bs)
 

block.c int bdrv_commit(BlockDriverState *bs) · line 1414

Quoted 7 lines

block.c +1 -1

     if (!drv)
         return -ENOMEDIUM;
-    
+
     if (!bs->backing_hd) {
         return -ENOTSUP;
     }

Spurious whitespace cleanup, since nothing else in this hunk belongs to you. Is that trailing whitespace present upstream, or was it introduced in one of the patches you based off of?

Quoted 6 lines

block.c +1 -0

@@ -1661,6 +1661,110 @@ int bdrv_change_backing_file(BlockDriverState *bs,
     return ret;
 }
 
+typedef struct BlkIntermediateStates {

block.c int bdrv_change_backing_file(BlockDriverState *bs, · line 1674

Quoted 10 lines

block.c +10 -0

+    BlockDriverState *bs;
+    QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
+} BlkIntermediateStates;
+
+
+/* deletes images above 'base' up to and including 'top', and sets the image
+ * above 'top' to have base as its backing file.
+ */
+int bdrv_delete_intermediate(BlockDriverState *active, BlockDriverState *top,
+                             BlockDriverState *base)

...that is, I think this would aid the reader:

Converts:

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

to

bottom <- base <- active

where top==active is permitted

Quoted 5 lines

block.c +0 -0

@@ -3128,6 +3232,36 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
     return NULL;
 }
 

block.c BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, · line 3245

Quoted 11 lines

block.c +11 -0

+BlockDriverState *bdrv_find_image(BlockDriverState *bs,
+                                  const char *filename)
+{
+    if (!bs || !bs->drv) {
+        return NULL;
+    }
+
+    if (strcmp(bs->filename, filename) == 0) {
+        return bs;
+    } else {
+        return bdrv_find_image(bs->backing_hd, filename);

Tail-recursive; are we worried enough about ever hitting stack overflow due to a really deep change to convert this into a while loop recursion instead? [Probably not]

Quoted 12 lines

block.c +11 -0

+    }
+}
+
+BlockDriverState *bdrv_find_base(BlockDriverState *bs)
+{
+    BlockDriverState *curr_bs = NULL;
+
+    if (!bs) {
+        return NULL;
+    }
+

block.c BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, · line 3261

Quoted 5 lines

block.c +5 -0

+    curr_bs = bs;
+
+    while (curr_bs->backing_hd) {
+        curr_bs = curr_bs->backing_hd;
+    }

Then again, here you did while-loop recursion, so using the same style in both functions might be worthwhile.

Quoted 11 lines · Jeff Cody

Quoted 7 lines

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 stiched together. Images are restored to their original open

s/stiched/stitched/

Quoted 12 lines

block/commit.c line 27

Quoted 9 lines

block/commit.c new file +9 -0

+
+enum {
+    /*
+     * Size of data buffer for populating the image file.  This should be large
+     * enough to process multiple clusters in a single call, so that populating
+     * contiguous regions of the image is efficient.
+     */
+    COMMIT_BUFFER_SIZE = 512 * 1024, /* in bytes */
+};

Paolo's latest round of patches got to the point of making this configurable for drive-mirror; is that something you should be copying here?

Yes

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Quoted 11 lines

Quoted 8 lines

block/commit.c line 27

Quoted 8 lines

block/commit.c new file +8 -0

+enum {
+    /*
+     * Size of data buffer for populating the image file.  This should be large
+     * enough to process multiple clusters in a single call, so that populating
+     * contiguous regions of the image is efficient.
+     */
+    COMMIT_BUFFER_SIZE = 512 * 1024, /* in bytes */
+};

Paolo's latest round of patches got to the point of making this configurable for drive-mirror; is that something you should be copying here?

Quoted 1 line

Quoted 1 line

Yes

Though its use is very limited for live commit. For the mirror it's important because a larger number can mean that more data is unnecessarily written, and the target can become larger than the source.

For live commit, I think using a larger buffer is always better.

Hm, part of the difference is that I assume that commit uses bdrv_is_allocated() to check whether some data must really be copied. But then, there's no reason why mirroring couldn't do that as well. Paolo?

Il 01/08/2012 08:32, Kevin Wolf ha scritto:

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Quoted 11 lines

Quoted 8 lines

block/commit.c line 27

Quoted 8 lines

block/commit.c new file +8 -0

+enum {
+    /*
+     * Size of data buffer for populating the image file.  This should be large
+     * enough to process multiple clusters in a single call, so that populating
+     * contiguous regions of the image is efficient.
+     */
+    COMMIT_BUFFER_SIZE = 512 * 1024, /* in bytes */
+};

Paolo's latest round of patches got to the point of making this configurable for drive-mirror; is that something you should be copying here?

Quoted 4 lines

Quoted 1 line

Quoted 1 line

Yes

Though its use is very limited for live commit. For the mirror it's important because a larger number can mean that more data is unnecessarily written, and the target can become larger than the source.

Note that the latest version of mirroring has _two_ knobs:

1) granularity is what decides how much data could be written unnecessarily, because of the dirty bitmap.

2) buffer size is what decides how much I/O is in flight at one time.

The default values are resp. the cluster size (or 64K for raw) and 10M.
The two together give mirroring some self-tuning capability. For
example in the first part of the mirroring you will likely proceed in 10M chunks with no concurrency; once you're synchronized, you'll probably send several chunks, perhaps all 64K if the guest does random writes.

Live commit as it is done now doesn't need any of this complication; it is just a background operation that does not need to compete with the guest. So using a larger buffer is indeed always better, and 512K is a nice intermediate value between mirroring's 64K and 10M extremes.

Quoted 5 lines

For live commit, I think using a larger buffer is always better.

Hm, part of the difference is that I assume that commit uses bdrv_is_allocated() to check whether some data must really be copied. But then, there's no reason why mirroring couldn't do that as well. Paolo?

We copy a cluster at a time, and that's also the resolution of bdrv_is_allocated so we wouldn't gain anything. Nice idea though, I had to mull about it to find the flaw. :)

Paolo

These are proposed changes, to add live block commit functionality.

I originally had intended for this RFC series to include the more complicated case of a live commit of the active layer, but removed it for this commit in the hopes of making it into the soft feature freeze for 1.2, so this series is the simpler case.

This series adds the basic case, of a live commit between two images below the active layer, e.g.:

[base] <--- [snp-1] <--- [snp-2] <--- [snp-3] <--- [active]

can be collapsed down via commit, into:

[base] <--- [active]

or,

[base] <--- [snp-1] <--- [active],

[base] <--- [snp-3] <--- [active],

etc..

qemu-io tests for the block commit will be adding onto the v1 patch set.

These patches are on top of Supriya's reopen() series, and Paolo's block mirror series (the RFC series). I have not rebased yet to the newer patches put out by Supriya and Paolo - this was tested w/o the reopen, by manually making sure the images stayed in a r/w state.

Jeff Cody (4):
block: add support functions for live commit, to find and delete
images.
block: add live block commit functionality
qerror: new errors for live block commit, QERR_TOP_NOT_FOUND
QAPI: add command for live block commit, 'block-commit'

 block.c             |  136 ++++++++++++++++++++++++++++++++++-
 block.h             |    4 ++
 block/Makefile.objs |    2 +-
 block/commit.c      |  200 +++++++++++++++++++++++++++++++++++++++++++++++++++
 block_int.h         |   19 +++++
 blockdev.c          |   78 ++++++++++++++++++++
 qapi-schema.json    |   33 +++++++++
 qerror.c            |    4 ++
 qerror.h            |    3 +
 qmp-commands.hx     |    6 ++
 trace-events        |    2 +
 11 files changed, 485 insertions(+), 2 deletions(-)
 create mode 100644 block/commit.c

Hi Jeff

This is an awesome feature and absolutely necessary to get a working live-snapshot/backup solution.

What is the status on this? Will it make it into 1.2.0?

Thanks in advance,
best regards,
Tiziano

Am Dienstag, den 31.07.2012, 01:16 -0400 schrieb Jeff Cody:

Quoted 55 lines

These are proposed changes, to add live block commit functionality.

I originally had intended for this RFC series to include the more complicated case of a live commit of the active layer, but removed it for this commit in the hopes of making it into the soft feature freeze for 1.2, so this series is the simpler case.

This series adds the basic case, of a live commit between two images below the active layer, e.g.:

[base] <--- [snp-1] <--- [snp-2] <--- [snp-3] <--- [active]

can be collapsed down via commit, into:

[base] <--- [active]

or,

[base] <--- [snp-1] <--- [active],

[base] <--- [snp-3] <--- [active],

etc..

qemu-io tests for the block commit will be adding onto the v1 patch set.

These patches are on top of Supriya's reopen() series, and Paolo's block mirror series (the RFC series). I have not rebased yet to the newer patches put out by Supriya and Paolo - this was tested w/o the reopen, by manually making sure the images stayed in a r/w state.

Jeff Cody (4):
block: add support functions for live commit, to find and delete
images.
block: add live block commit functionality
qerror: new errors for live block commit, QERR_TOP_NOT_FOUND
QAPI: add command for live block commit, 'block-commit'

 block.c             |  136 ++++++++++++++++++++++++++++++++++-
 block.h             |    4 ++
 block/Makefile.objs |    2 +-
 block/commit.c      |  200 +++++++++++++++++++++++++++++++++++++++++++++++++++
 block_int.h         |   19 +++++
 blockdev.c          |   78 ++++++++++++++++++++
 qapi-schema.json    |   33 +++++++++
 qerror.c            |    4 ++
 qerror.h            |    3 +
 qmp-commands.hx     |    6 ++
 trace-events        |    2 +
 11 files changed, 485 insertions(+), 2 deletions(-)
 create mode 100644 block/commit.c