Live block commit

Live block commit.

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..

TODO: * qemu-io tests (in progress)
* 'stage-2' of live commit functionality, to be able to push down the active layer. This structured something like mirroring, to allow for convergence.

Changes from the RFC v1 series:

* This patch series is not on top of Paolo's blk mirror series yet, to make it easier to apply independently if desired. This means some of what was in the previous RFC series is not in this one (BlockdevOnError, for instance), but that can be easily added in once Paolo's series are in.

* This patches series is dependent on the reopen() series with transactional reopen.

* The target release for this series is 1.3

* Found some mistakes in the reopen calls

* Dropped the BlockdevOnError argument (for now), will add in if rebasing on top of Paolo's series.

* Used the new qerror system

Jeff Cody (6):
1/6 block: add support functions for live commit, to find and delete
images.

  2/6   block: add live block commit functionality
  3/6   blockdev: rename block_stream_cb to a generic block_job_cb
  4/6   qerror: new error for live block commit, QERR_TOP_NOT_FOUND
  5/6   block: helper function, to find the base image of a chain
  6/6   QAPI: add command for live block commit, 'block-commit'

 block.c             | 158 ++++++++++++++++++++++++++++++++++++++++
 block.h             |   6 +-
 block/Makefile.objs |   1 +
 block/commit.c      | 202 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 block_int.h         |  19 +++++
 blockdev.c          |  91 ++++++++++++++++++++++-
 qapi-schema.json    |  30 ++++++++
 qerror.h            |   3 +
 qmp-commands.hx     |   6 ++
 trace-events        |   4 +-
 10 files changed, 515 insertions(+), 5 deletions(-)
 create mode 100644 block/commit.c

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

---
 qerror.h | 3 +++
 1 file changed, 3 insertions(+)

qerror.h +0 -0

index d0a76a4..7396184 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

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 done as a block job, so upon completion a BLOCK_JOB_COMPLETED will be emitted.

##

qapi-schema.json +14 -0

+# @block-commit
+#
+# Live commit of data from child image nodes into parent nodes - i.e.,
+# writes data between 'top' and 'base' into 'base'.
+#
+# @device:  the name of the device
+#
+# @base:   #optional The parent image of the device to write data into.
+#                    If not specified, this is the original parent image.
+#
+# @top:    #optional The child image, above which data will not be committed
+#                    down.  If not specified, this is the active layer.
+#
+# @speed:  #optional the maximum speed, in bytes per second

Quoted 13 lines · Jeff Cody

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

---
 qerror.h | 3 +++
 1 file changed, 3 insertions(+)

qerror.h +0 -0

index d0a76a4..7396184 100644
@@ -219,6 +219,9 @@ void assert_no_error(Error *err);
 #define QERR_TOO_MANY_FILES \
     ERROR_CLASS_GENERIC_ERROR, "Too many open files"
 

qerror.h void assert_no_error(Error *err); · line 224

Quoted 3 lines

qerror.h +3 -0

+#define QERR_TOP_NOT_FOUND \
+    ERROR_CLASS_GENERIC_ERROR, "Top image file %s not found"
+

Are you sure this patch is needed, or should you be using error_setg()?

http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg04980.html

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

Quoted 2 lines

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

Likewise, the job can be canceled, and it is possible to track progress of the job or change the speed on the fly, using existing block job commands.

Will the BLOCK_JOB_COMPLETED event have a new category listing that it was a commit job instead of a stream job that completed? That is, I think this patch is incomplete, and that you also need to modify QMP/qmp-events.txt to modify the 'type' field of BLOCK_JOB_{CANCELLED,COMPLETED} to distinguish this new sub-type of event.

Quoted 16 lines

##

qapi-schema.json +14 -0

+# @block-commit
+#
+# Live commit of data from child image nodes into parent nodes - i.e.,
+# writes data between 'top' and 'base' into 'base'.
+#
+# @device:  the name of the device
+#
+# @base:   #optional The parent image of the device to write data into.
+#                    If not specified, this is the original parent image.
+#
+# @top:    #optional The child image, above which data will not be committed
+#                    down.  If not specified, this is the active layer.
+#
+# @speed:  #optional the maximum speed, in bytes per second

qapi-schema.json line 1427

Quoted 10 lines

qapi-schema.json +10 -0

+#
+# 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, BaseNotFound
+#          If @top does not exist, TopNotFound
+#          If @speed is invalid, InvalidParameter
+#
+# Since: 1.2

1.3

Add bdrv_find_child(), and bdrv_delete_intermediate().

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

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.

E.g., this converts:

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

to

bottom <- base <- active

where top == active is permitted, although active will not be deleted.

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

---
 block.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block.h |   5 ++-
 2 files changed, 146 insertions(+), 1 deletion(-)

block.c +0 -0

index 9470319..11e275c 100644

block.c +5 -0

+    if (active == bs) {
+        child = active;
+    } else {
+        intermediate = active;
+        while (intermediate->backing_hd) {

block.c +17 -0

+
+typedef struct BlkIntermediateStates {
+    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.
+ *
+ * E.g., this will convert the following chain:
+ * bottom <- base <- intermediate <- top <- active
+ *
+ * to
+ *
+ * bottom <- base <- active
+ *

block.c +14 -0

+ *
+ * base <- intermediate <- top
+ *
+ * becomes
+ *
+ * base <- top
+ */
+int bdrv_delete_intermediate(BlockDriverState *active, BlockDriverState *top,
+                             BlockDriverState *base)
+{
+    BlockDriverState *intermediate;
+    BlockDriverState *base_bs = NULL;
+    BlockDriverState *new_top_bs = NULL;
+    BlkIntermediateStates *intermediate_state, *next;

block.c +14 -0

+
+    /* now we will go down through the list, and add each BDS we find
+     * into our deletion queue, until we hit the 'base'
+     */
+    while (intermediate) {
+        intermediate_state = g_malloc0(sizeof(BlkIntermediateStates));
+        intermediate_state->bs = intermediate;
+        QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry);
+
+        if (intermediate->backing_hd == base) {
+            base_bs = intermediate->backing_hd;
+            break;
+        }
+        intermediate = intermediate->backing_hd;

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/Makefile.objs |   1 +
 block/commit.c      | 202 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 block_int.h         |  19 +++++
 trace-events        |   2 +
 4 files changed, 224 insertions(+)
 create mode 100644 block/commit.c

block/Makefile.objs +1 -0

index b5754d3..4a136b8 100644
@@ -4,6 +4,7 @@ block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
 block-obj-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
 block-obj-y += stream.o
+block-obj-y += commit.o
 block-obj-$(CONFIG_WIN32) += raw-win32.o
 block-obj-$(CONFIG_POSIX) += raw-posix.o
 block-obj-$(CONFIG_LIBISCSI) += iscsi.o

block/commit.c new file +41 -0

new file mode 100644
index 0000000..bd3d882
@@ -0,0 +1,202 @@
+/*
+ * Live block commit
+ *
+ * Copyright Red Hat, Inc. 2012
+ *
+ * Authors:
+ *  Jeff Cody   <[email protected]>
+ *  Based on stream.c by Stefan Hajnoczi
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "trace.h"
+#include "block_int.h"
+#include "qemu/ratelimit.h"
+
+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 */
+};
+
+#define SLICE_TIME 100000000ULL /* ns */
+
+typedef struct CommitBlockJob {
+    BlockJob common;
+    RateLimit limit;
+    BlockDriverState *active;
+    BlockDriverState *top;
+    BlockDriverState *base;
+    BlockErrorAction on_error;
+    int base_flags;
+    int top_flags;
+} CommitBlockJob;
+
+static int coroutine_fn commit_populate(BlockDriverState *bs,

block/commit.c new file +37 -0

+    return 0;
+}
+
+static void coroutine_fn commit_run(void *opaque)
+{
+    CommitBlockJob *s = opaque;
+    BlockDriverState *active = s->active;
+    BlockDriverState *top = s->top;
+    BlockDriverState *base = s->base;
+    BlockDriverState *top_child = NULL;
+    int64_t sector_num, end;
+    int error = 0;
+    int ret = 0;
+    int n = 0;
+    void *buf;
+    int bytes_written = 0;
+
+    s->common.len = bdrv_getlength(top);
+    if (s->common.len < 0) {
+        block_job_complete(&s->common, s->common.len);
+        return;
+    }
+
+    top_child = bdrv_find_child(active, top);
+
+    end = s->common.len >> BDRV_SECTOR_BITS;
+    buf = qemu_blockalign(top, COMMIT_BUFFER_SIZE);
+
+    for (sector_num = 0; sector_num < end; sector_num += n) {
+        uint64_t delay_ns = 0;
+        bool copy;
+
+wait:
+        /* Note that even when no rate limit is applied we need to yield
+         * with no pending I/O here so that qemu_aio_flush() returns.
+         */
+        block_job_sleep_ns(&s->common, rt_clock, delay_ns);

block/commit.c new file +11 -0

+                n = 0;
+                continue;
+            }
+            if (error == 0) {
+                error = ret;
+            }
+            if (s->on_error == BLOCK_ERR_REPORT) {
+                break;
+            }
+        }
+        ret = 0;

block/commit.c new file +51 -0

+        }
+    }
+
+    /* restore base open flags here if appropriate (e.g., change the base back
+     * to r/o). These reopens do not need to be atomic, since we won't abort
+     * even on failure here */
+
+    if (s->base_flags != bdrv_get_flags(base)) {
+        bdrv_reopen(base, s->base_flags, NULL);
+    }
+    if (s->top_flags != bdrv_get_flags(top_child)) {
+        bdrv_reopen(top_child, s->top_flags, NULL);
+    }
+
+    qemu_vfree(buf);
+    block_job_complete(&s->common, ret);
+}
+
+static void commit_set_speed(BlockJob *job, int64_t speed, Error **errp)
+{
+    CommitBlockJob *s = container_of(job, CommitBlockJob, common);
+
+    if (speed < 0) {
+        error_set(errp, QERR_INVALID_PARAMETER, "speed");
+        return;
+    }
+    ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE, SLICE_TIME);
+}
+
+static BlockJobType commit_job_type = {
+    .instance_size = sizeof(CommitBlockJob),
+    .job_type      = "commit",
+    .set_speed     = commit_set_speed,
+};
+
+void commit_start(BlockDriverState *bs, BlockDriverState *base,
+                 BlockDriverState *top, int64_t speed,
+                 BlockErrorAction on_error, BlockDriverCompletionFunc *cb,
+                 void *opaque, int orig_base_flags, int orig_top_flags,
+                 Error **errp)
+{
+    CommitBlockJob *s;
+
+    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;
+    }
+
+    s = block_job_create(&commit_job_type, bs, speed, cb, opaque, errp);

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

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 done as a block job, so upon completion a BLOCK_JOB_COMPLETED will be emitted.

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

---
 blockdev.c       | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 30 ++++++++++++++++++++
 qmp-commands.hx  |  6 ++++
 3 files changed, 119 insertions(+)

blockdev.c +22 -0

index 68d65fb..e0d6ca0 100644
@@ -827,6 +827,89 @@ exit:
     return;
 }
 
+void qmp_block_commit(const char *device,
+                      bool has_base, const char *base,
+                      bool has_top, const char *top,
+                      bool has_speed, int64_t speed,
+                      Error **errp)
+{
+    BlockDriverState *bs;
+    BlockDriverState *base_bs, *top_bs, *child_bs;
+    Error *local_err = NULL;
+    int orig_base_flags, orig_top_flags;
+    BlockReopenQueue *reopen_queue = NULL;
+    /* This will be part of the QMP command, if/when the
+     * BlockdevOnError change for blkmirror makes it in
+     */
+    BlockErrorAction on_error = BLOCK_ERR_REPORT;
+
+    /* drain all i/o before commits */
+    bdrv_drain_all();
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);

blockdev.c +20 -0

+        return;
+    }
+
+    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);
+        if (top_bs == NULL) {
+            error_set(errp, QERR_TOP_NOT_FOUND, top);
+            return;
+        }
+    } else {
+        /* we will eventually default to the top layer,i.e. top_bs = bs */
+        error_set(errp, QERR_TOP_NOT_FOUND, top);
+        return;
+    }
+
+    child_bs = bdrv_find_child(bs, top_bs);
+
+    orig_base_flags = bdrv_get_flags(base_bs);  /* what we are writing into   */

blockdev.c +21 -0

+    }
+    if (reopen_queue) {
+        bdrv_reopen_multiple(reopen_queue, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
+    commit_start(bs, base_bs, top_bs, speed, on_error,
+                 block_job_cb, bs, orig_base_flags, orig_top_flags,
+                 &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    /* Grab a reference so hotplug does not delete the BlockDriverState from
+     * underneath us.
+     */
+    drive_get_ref(drive_get_by_blockdev(bs));
+}
 
 static void eject_device(BlockDriverState *bs, int force, Error **errp)
 {

qapi-schema.json +0 -0

index bd8ad74..45feda6 100644
@@ -1401,6 +1401,36 @@
   'returns': 'str' }
 

Am 30.08.2012 20:47, schrieb Jeff Cody:

Quoted 21 lines

Add bdrv_find_child(), and bdrv_delete_intermediate().

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

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.

E.g., this converts:

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

to

bottom <- base <- active

where top == active is permitted, although active will not be deleted.

Quoted 1 line

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

At first, when just reading the function name, I thought this would actually delete the image file. Of course, it only removes it from the backing file chain, but leaves the image file around. I don't have a good suggestion, but if someone has a better name, I think we should change it.

Quoted 11 lines

---
 block.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block.h |   5 ++-
 2 files changed, 146 insertions(+), 1 deletion(-)

block.c +0 -0

index 9470319..11e275c 100644

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

Quoted 7 lines

block.c +4 -0

@@ -1752,6 +1752,148 @@ int bdrv_change_backing_file(BlockDriverState *bs,
     return ret;
 }
 
+/* Finds the image layer immediately to the 'top' of bs.
+ *
+ * active is the current topmost image.
+ */

Most other function header comments in block.c have the /* on its own line, so it would be nice to stay consistent.

Without looking at the code yet, I'm not quite sure if I understand this correctly. Could use an example:

base <- a <- b <- c <- d

bdrv_find_child(d, a) would return b? Why is it called bdrv_find_child then, b is neither a (direct) child of d nor a.

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

Quoted 9 lines

block.c +9 -0

+BlockDriverState *bdrv_find_child(BlockDriverState *active,
+                                  BlockDriverState *bs)
+{
+    BlockDriverState *child = NULL;
+    BlockDriverState *intermediate;
+
+    /* if the active bs layer is the same as the new top, then there
+     * is no image above the top, so it will be returned as the child
+     */

"new top"?

And should this be mentioned in the header comment? Not sure how generally useful this behaviour is. If it's only the right thing for the only caller, maybe returning NULL and handling the case in the caller would be better.

Quoted 10 lines

block.c +10 -0

+    if (active == bs) {
+        child = active;
+    } else {
+        intermediate = active;
+        while (intermediate->backing_hd) {
+            if (intermediate->backing_hd == bs) {
+                child = intermediate;
+                break;
+            }
+            intermediate = intermediate->backing_hd;

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

Quoted 5 lines

block.c +5 -0

+        }
+    }
+
+    return child;
+}

So it returns NULL when bs isn't in the backing file chain of active. Probably worth mentioning in the comment as well.

Quoted 23 lines

block.c +22 -0

+
+typedef struct BlkIntermediateStates {
+    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.
+ *
+ * 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

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

Quoted 5 lines

block.c +5 -0

+ *
+ * base <- active
+ *
+ * It is also allowed for top==active, except in that case active is not
+ * deleted:

Hm, makes the interface inconsistent. Shouldn't you be using top == intermediate and it would work without any special casing?

Quoted 14 lines

block.c +14 -0

+ *
+ * base <- intermediate <- top
+ *
+ * becomes
+ *
+ * base <- top
+ */
+int bdrv_delete_intermediate(BlockDriverState *active, BlockDriverState *top,
+                             BlockDriverState *base)
+{
+    BlockDriverState *intermediate;
+    BlockDriverState *base_bs = NULL;
+    BlockDriverState *new_top_bs = NULL;
+    BlkIntermediateStates *intermediate_state, *next;

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

Quoted 10 lines

block.c +10 -0

+    int ret = -1;
+
+    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_child(active, top);

new_top_bs is the first BDS that should not be removed from the chain.

Why do we pass top and then search for new_top instead of passing new_top directly?

Quoted 5 lines

block.c +5 -0

+
+    /* special case of new_top_bs->backing_hd already pointing to base - nothing
+     * to do, no intermediate images
+     */
+    if (new_top_bs->backing_hd == base) {

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

Quoted 5 lines

block.c +5 -0

+        ret = 0;
+        goto exit;
+    }
+
+    if (new_top_bs == NULL) {

Never reached, we segfault before. (How about unit tests for this function, in the style of tests/check-q*.c?)

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

Quoted 8 lines

block.c +8 -0

+        /* we could not find the image above 'top', this is an error */
+        goto exit;
+    }
+
+    /* if the active and top image passed in are the same, then we
+     * can't delete the active, so we start one below
+     */
+    intermediate = (active == top) ? active->backing_hd : top;

Aha. So intermediate is used to undo the special case. Now we're always on the last image to be deleted.

This is equivalent to an unconditional new_top_bs->backing_hd.

Quoted 18 lines

block.c +18 -0

+
+    /* now we will go down through the list, and add each BDS we find
+     * into our deletion queue, until we hit the 'base'
+     */
+    while (intermediate) {
+        intermediate_state = g_malloc0(sizeof(BlkIntermediateStates));
+        intermediate_state->bs = intermediate;
+        QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry);
+
+        if (intermediate->backing_hd == base) {
+            base_bs = intermediate->backing_hd;
+            break;
+        }
+        intermediate = intermediate->backing_hd;
+    }
+    if (base_bs == NULL) {
+        /* something went wrong, we did not end at the base. safely
+         * unravel everything, and exit with error */

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

Quoted 6 lines

block.c +6 -0

+        goto exit;
+    }
+
+    /* success - we can delete the intermediate states, and link top->base */
+    ret = bdrv_change_backing_file(new_top_bs, base_bs->filename,
+                                   base_bs->drv ? base_bs->drv->format_name : "");

Requires that new_top_bs is opened r/w. Definitely worth documenting.

Am 30.08.2012 20:47, schrieb Jeff Cody:

Quoted 5 lines

Add bdrv_find_child(), and bdrv_delete_intermediate().

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

Quoted 1 line

bdrv_delete_intermediate():

At first, when just reading the function name, I thought this would actually delete the image file. Of course, it only removes it from the backing file chain, but leaves the image file around. I don't have a good suggestion, but if someone has a better name, I think we should change it.

Am 30.08.2012 20:47, schrieb Jeff Cody:

Quoted 81 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 stitched together. Images are restored to their original open flags upon completion.

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

---
 block/Makefile.objs |   1 +
 block/commit.c      | 202 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 block_int.h         |  19 +++++
 trace-events        |   2 +
 4 files changed, 224 insertions(+)
 create mode 100644 block/commit.c

block/Makefile.objs +1 -0

index b5754d3..4a136b8 100644
@@ -4,6 +4,7 @@ block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
 block-obj-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
 block-obj-y += stream.o
+block-obj-y += commit.o
 block-obj-$(CONFIG_WIN32) += raw-win32.o
 block-obj-$(CONFIG_POSIX) += raw-posix.o
 block-obj-$(CONFIG_LIBISCSI) += iscsi.o

block/commit.c new file +45 -0

new file mode 100644
index 0000000..bd3d882
@@ -0,0 +1,202 @@
+/*
+ * Live block commit
+ *
+ * Copyright Red Hat, Inc. 2012
+ *
+ * Authors:
+ *  Jeff Cody   <[email protected]>
+ *  Based on stream.c by Stefan Hajnoczi
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "trace.h"
+#include "block_int.h"
+#include "qemu/ratelimit.h"
+
+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 */
+};
+
+#define SLICE_TIME 100000000ULL /* ns */
+
+typedef struct CommitBlockJob {
+    BlockJob common;
+    RateLimit limit;
+    BlockDriverState *active;
+    BlockDriverState *top;
+    BlockDriverState *base;
+    BlockErrorAction on_error;
+    int base_flags;
+    int top_flags;
+} CommitBlockJob;
+
+static int coroutine_fn commit_populate(BlockDriverState *bs,
+                                        BlockDriverState *base,
+                                        int64_t sector_num, int nb_sectors,
+                                        void *buf)
+{

block/commit.c line 51

Quoted 6 lines

block/commit.c new file +6 -0

+    if (bdrv_read(bs, sector_num, buf, nb_sectors)) {
+        return -EIO;
+    }
+    if (bdrv_write(base, sector_num, buf, nb_sectors)) {
+        return -EIO;
+    }

Don't throw error codes away.

What should we do with backing files that are smaller than the image to commit? In this version, data is copied up to the size of the backing file, and then we get -EIO from bdrv_co_do_writev().

Quoted 37 lines

block/commit.c new file +37 -0

+    return 0;
+}
+
+static void coroutine_fn commit_run(void *opaque)
+{
+    CommitBlockJob *s = opaque;
+    BlockDriverState *active = s->active;
+    BlockDriverState *top = s->top;
+    BlockDriverState *base = s->base;
+    BlockDriverState *top_child = NULL;
+    int64_t sector_num, end;
+    int error = 0;
+    int ret = 0;
+    int n = 0;
+    void *buf;
+    int bytes_written = 0;
+
+    s->common.len = bdrv_getlength(top);
+    if (s->common.len < 0) {
+        block_job_complete(&s->common, s->common.len);
+        return;
+    }
+
+    top_child = bdrv_find_child(active, top);
+
+    end = s->common.len >> BDRV_SECTOR_BITS;
+    buf = qemu_blockalign(top, COMMIT_BUFFER_SIZE);
+
+    for (sector_num = 0; sector_num < end; sector_num += n) {
+        uint64_t delay_ns = 0;
+        bool copy;
+
+wait:
+        /* Note that even when no rate limit is applied we need to yield
+         * with no pending I/O here so that qemu_aio_flush() returns.
+         */
+        block_job_sleep_ns(&s->common, rt_clock, delay_ns);

block/commit.c line 98

Quoted 10 lines

block/commit.c new file +10 -0

+        if (block_job_is_cancelled(&s->common)) {
+            break;
+        }
+        /* Copy if allocated above the base */
+        ret = bdrv_co_is_allocated_above(top, base, sector_num,
+                                         COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE,
+                                         &n);
+        copy = (ret == 1);
+        trace_commit_one_iteration(s, sector_num, n, ret);
+        if (ret >= 0 && copy) {

If ret == 1, it's automatically >= 0...

By the way, I'm not sure if the interface is 0/1/-errno or 0/positive/-errno.

block/commit.c line 110

Quoted 12 lines

block/commit.c new file +12 -0

+            if (s->common.speed) {
+                delay_ns = ratelimit_calculate_delay(&s->limit, n);
+                if (delay_ns > 0) {
+                    goto wait;
+                }
+            }
+            ret = commit_populate(top, base, sector_num, n, buf);
+            bytes_written += n * BDRV_SECTOR_SIZE;
+        }
+        if (ret < 0) {
+            if (s->on_error == BLOCK_ERR_STOP_ANY ||
+                s->on_error == BLOCK_ERR_STOP_ENOSPC) {

Shouldn't there be a check for ret == -ENOSPC then...? And does this error handling do anything useful if you can't pause the job? Wouldn't it retry all the time?

Quoted 11 lines

block/commit.c new file +11 -0

+                n = 0;
+                continue;
+            }
+            if (error == 0) {
+                error = ret;
+            }
+            if (s->on_error == BLOCK_ERR_REPORT) {
+                break;
+            }
+        }
+        ret = 0;

block/commit.c line 131

Quoted 10 lines

block/commit.c new file +10 -0

+
+        /* Publish progress */
+        s->common.offset += n * BDRV_SECTOR_SIZE;
+    }
+
+    if (!block_job_is_cancelled(&s->common) && sector_num == end && ret == 0) {
+        /* success */
+        if (bdrv_delete_intermediate(active, top, base)) {
+            /* something went wrong! */
+            /* TODO:add error reporting here */

Indeed :-)

Quoted 56 lines

block/commit.c new file +56 -0

+        }
+    }
+
+    /* restore base open flags here if appropriate (e.g., change the base back
+     * to r/o). These reopens do not need to be atomic, since we won't abort
+     * even on failure here */
+
+    if (s->base_flags != bdrv_get_flags(base)) {
+        bdrv_reopen(base, s->base_flags, NULL);
+    }
+    if (s->top_flags != bdrv_get_flags(top_child)) {
+        bdrv_reopen(top_child, s->top_flags, NULL);
+    }
+
+    qemu_vfree(buf);
+    block_job_complete(&s->common, ret);
+}
+
+static void commit_set_speed(BlockJob *job, int64_t speed, Error **errp)
+{
+    CommitBlockJob *s = container_of(job, CommitBlockJob, common);
+
+    if (speed < 0) {
+        error_set(errp, QERR_INVALID_PARAMETER, "speed");
+        return;
+    }
+    ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE, SLICE_TIME);
+}
+
+static BlockJobType commit_job_type = {
+    .instance_size = sizeof(CommitBlockJob),
+    .job_type      = "commit",
+    .set_speed     = commit_set_speed,
+};
+
+void commit_start(BlockDriverState *bs, BlockDriverState *base,
+                 BlockDriverState *top, int64_t speed,
+                 BlockErrorAction on_error, BlockDriverCompletionFunc *cb,
+                 void *opaque, int orig_base_flags, int orig_top_flags,
+                 Error **errp)
+{
+    CommitBlockJob *s;
+
+    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;
+    }
+
+    s = block_job_create(&commit_job_type, bs, speed, cb, opaque, errp);
+    if (!s) {
+        return;
+    }
+
+    s->base   = base;

block/commit.c line 192

Quoted 5 lines

block/commit.c new file +5 -0

+    s->top    = top;
+    s->active = bs;
+
+    s->base_flags = orig_base_flags;
+    s->top_flags  = orig_top_flags;

So it's the caller who is expected to reopen r/w and then pass the original flags in? Can't we do both of it here?

block/commit.c line 201

Quoted 9 lines

block/commit.c new file +9 -0

+
+    s->on_error = on_error;
+    s->common.co = qemu_coroutine_create(commit_run);
+
+    trace_commit_start(bs, base, top, s, s->common.co, opaque,
+                       orig_base_flags, orig_top_flags);
+    qemu_coroutine_enter(s->common.co, s);
+
+    return;

Unnecessary return.

Kevin

Am 30.08.2012 20:47, schrieb Jeff Cody:

Quoted 60 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 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 the error QERR_TOP_NOT_FOUND 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]>

---
 blockdev.c       | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 30 ++++++++++++++++++++
 qmp-commands.hx  |  6 ++++
 3 files changed, 119 insertions(+)

blockdev.c +26 -0

index 68d65fb..e0d6ca0 100644
@@ -827,6 +827,89 @@ exit:
     return;
 }
 
+void qmp_block_commit(const char *device,
+                      bool has_base, const char *base,
+                      bool has_top, const char *top,
+                      bool has_speed, int64_t speed,
+                      Error **errp)
+{
+    BlockDriverState *bs;
+    BlockDriverState *base_bs, *top_bs, *child_bs;
+    Error *local_err = NULL;
+    int orig_base_flags, orig_top_flags;
+    BlockReopenQueue *reopen_queue = NULL;
+    /* This will be part of the QMP command, if/when the
+     * BlockdevOnError change for blkmirror makes it in
+     */
+    BlockErrorAction on_error = BLOCK_ERR_REPORT;
+
+    /* drain all i/o before commits */
+    bdrv_drain_all();
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+    if (base && has_base) {
+        base_bs = bdrv_find_backing_image(bs, base);

blockdev.c exit: · line 861

Quoted 6 lines

blockdev.c +6 -0

+    } else {
+        base_bs = bdrv_find_base(bs);
+    }
+
+    if (base_bs == NULL) {
+        error_set(errp, QERR_BASE_NOT_FOUND, "NULL");

Shouldn't it be base rather than "NULL"?

Quoted 25 lines

blockdev.c +25 -0

+        return;
+    }
+
+    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);
+        if (top_bs == NULL) {
+            error_set(errp, QERR_TOP_NOT_FOUND, top);
+            return;
+        }
+    } else {
+        /* we will eventually default to the top layer,i.e. top_bs = bs */
+        error_set(errp, QERR_TOP_NOT_FOUND, top);
+        return;
+    }
+
+    child_bs = bdrv_find_child(bs, top_bs);
+
+    orig_base_flags = bdrv_get_flags(base_bs);  /* what we are writing into   */
+    orig_top_flags = bdrv_get_flags(child_bs);  /* to change the backing file */
+
+    /* convert base_bs to r/w, if necessary */
+    if (!(orig_base_flags & BDRV_O_RDWR)) {
+        reopen_queue = bdrv_reopen_queue(reopen_queue, base_bs,

blockdev.c exit: · line 891

Quoted 5 lines

blockdev.c +5 -0

+                                         orig_base_flags | BDRV_O_RDWR);
+    }
+    if (!(orig_top_flags & BDRV_O_RDWR)) {
+        reopen_queue = bdrv_reopen_queue(reopen_queue, base_bs,
+                                         orig_base_flags | BDRV_O_RDWR);

I think you mean child_bs and orig_top_flags. (Why isn't it called orig_child_flags?)

Quoted 31 lines

blockdev.c +21 -0

+    }
+    if (reopen_queue) {
+        bdrv_reopen_multiple(reopen_queue, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
+    commit_start(bs, base_bs, top_bs, speed, on_error,
+                 block_job_cb, bs, orig_base_flags, orig_top_flags,
+                 &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    /* Grab a reference so hotplug does not delete the BlockDriverState from
+     * underneath us.
+     */
+    drive_get_ref(drive_get_by_blockdev(bs));
+}
 
 static void eject_device(BlockDriverState *bs, int force, Error **errp)
 {

qapi-schema.json +0 -0

index bd8ad74..45feda6 100644
@@ -1401,6 +1401,36 @@
   'returns': 'str' }
 

qapi-schema.json line 1412

Quoted 10 lines

qapi-schema.json +9 -0

 ##
+# @block-commit
+#
+# Live commit of data from child image nodes into parent nodes - i.e.,
+# writes data between 'top' and 'base' into 'base'.
+#
+# @device:  the name of the device
+#
+# @base:   #optional The parent image of the device to write data into.
+#                    If not specified, this is the original parent image.

"The file name of the parent image", actually.

Which I'm afraid means that this isn't an API for the long term, but there's little we can do about it now...

qapi-schema.json line 1415

Quoted 3 lines

qapi-schema.json +3 -0

+#
+# @top:    #optional The child image, above which data will not be committed
+#                    down.  If not specified, this is the active layer.

Again, the file name.

Kevin

Quoted 10 lines · Kevin Wolf

Am 30.08.2012 20:47, schrieb Jeff Cody:

Quoted 9 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 stitched together. Images are restored to their original open flags upon completion.

Quoted 4 lines

What should we do with backing files that are smaller than the image to commit? In this version, data is copied up to the size of the backing file, and then we get -EIO from bdrv_co_do_writev().

We could leave it like that, and let it receive -EIO in that case. Alternatively, we could try and determine before the commit if the data will fit in the base, and return -ENOSPC if not.

Quoted 22 lines

Am 30.08.2012 20:47, schrieb Jeff Cody:

Quoted 21 lines

Add bdrv_find_child(), and bdrv_delete_intermediate().

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

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.

E.g., this converts:

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

to

bottom <- base <- active

where top == active is permitted, although active will not be deleted.

Quoted 7 lines

Quoted 1 line

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

At first, when just reading the function name, I thought this would actually delete the image file. Of course, it only removes it from the backing file chain, but leaves the image file around. I don't have a good suggestion, but if someone has a better name, I think we should change it.

Hmm, the naming seems consistent with bdrv_delete(), which does not actually delete the image files either (and, that is essentially what this does... calls bdrv_delete(), on the intermediate images).

However, here are some other name proposals:

   *  bdrv_disconnect_intermediate()
   *  bdrv_drop_intermediate()
   *  bdrv_shorten_chain()

Quoted 23 lines

Quoted 23 lines

block.c +22 -0

+
+typedef struct BlkIntermediateStates {
+    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.
+ *
+ * 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

Quoted 9 lines

Quoted 5 lines

+ *
+ * base <- active
+ *
+ * It is also allowed for top==active, except in that case active is not
+ * deleted:

Hm, makes the interface inconsistent. Shouldn't you be using top == intermediate and it would work without any special casing?

To remain consistent, maybe we should define it as an error if top==active, and return error in that case? The caller can be responsible for checking for that - if the caller wants to merge down the active layer, there are additional steps to be taken anyway.

Quoted 9 lines

Quoted 8 lines

block.c +8 -0

+        /* we could not find the image above 'top', this is an error */
+        goto exit;
+    }
+
+    /* if the active and top image passed in are the same, then we
+     * can't delete the active, so we start one below
+     */
+    intermediate = (active == top) ? active->backing_hd : top;

Quoted 4 lines

Aha. So intermediate is used to undo the special case. Now we're always on the last image to be deleted.

This is equivalent to an unconditional new_top_bs->backing_hd.

Am 06.09.2012 16:59, schrieb Jeff Cody:

Quoted 35 lines · Kevin Wolf

Quoted 22 lines

Am 30.08.2012 20:47, schrieb Jeff Cody:

Quoted 21 lines

Add bdrv_find_child(), and bdrv_delete_intermediate().

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

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.

E.g., this converts:

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

to

bottom <- base <- active

where top == active is permitted, although active will not be deleted.

Quoted 7 lines

Quoted 1 line

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

At first, when just reading the function name, I thought this would actually delete the image file. Of course, it only removes it from the backing file chain, but leaves the image file around. I don't have a good suggestion, but if someone has a better name, I think we should change it.

Hmm, the naming seems consistent with bdrv_delete(), which does not actually delete the image files either (and, that is essentially what this does... calls bdrv_delete(), on the intermediate images).

Quoted 5 lines

However, here are some other name proposals:

   *  bdrv_disconnect_intermediate()
   *  bdrv_drop_intermediate()
   *  bdrv_shorten_chain()

bdrv_drop_intermediate() sounds good to me.

Quoted 33 lines

Quoted 23 lines

Quoted 23 lines

block.c +22 -0

+
+typedef struct BlkIntermediateStates {
+    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.
+ *
+ * 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

Quoted 9 lines

Quoted 5 lines

+ *
+ * base <- active
+ *
+ * It is also allowed for top==active, except in that case active is not
+ * deleted:

Hm, makes the interface inconsistent. Shouldn't you be using top == intermediate and it would work without any special casing?

Quoted 4 lines

To remain consistent, maybe we should define it as an error if top==active, and return error in that case? The caller can be responsible for checking for that - if the caller wants to merge down the active layer, there are additional steps to be taken anyway.

Yes, why not.

And we can always revisit when implementing the additional functionality.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

block.c +8 -0

+        /* we could not find the image above 'top', this is an error */
+        goto exit;
+    }
+
+    /* if the active and top image passed in are the same, then we
+     * can't delete the active, so we start one below
+     */
+    intermediate = (active == top) ? active->backing_hd : top;

Quoted 4 lines

Quoted 4 lines

Aha. So intermediate is used to undo the special case. Now we're always on the last image to be deleted.

This is equivalent to an unconditional new_top_bs->backing_hd.

How about changing this to use the simpler unconditional version?

Kevin

Quoted 18 lines · Jeff Cody

Quoted 16 lines · Kevin Wolf

Am 30.08.2012 20:47, schrieb Jeff Cody:

Quoted 9 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 stitched together. Images are restored to their original open flags upon completion.

What should we do with backing files that are smaller than the image to commit? In this version, data is copied up to the size of the backing file, and then we get -EIO from bdrv_co_do_writev().

Quoted 3 lines

We could leave it like that, and let it receive -EIO in that case. Alternatively, we could try and determine before the commit if the data will fit in the base, and return -ENOSPC if not.

Neither sounds appealing. Why can't we first try to resize the base to the new data size being committed, and only fall back to -ENOSPC or -EIO if the resize fails?

Il 30/08/2012 20:47, Jeff Cody ha scritto:

blockdev.c static const int if_max_devs[IF_COUNT] = { · line 60

Quoted 9 lines

blockdev.c +2 -0

@@ -53,6 +53,8 @@ static const int if_max_devs[IF_COUNT] = {
     [IF_SCSI] = 7,
 };
 
+static void block_job_cb(void *opaque, int ret);
+
 /*
  * We automatically delete the drive when a device using it gets
  * unplugged.  Questionable feature, but we can't just drop it.

Can you avoid the forward declaration?

Paolo

Quoted 13 lines

Il 30/08/2012 20:47, Jeff Cody ha scritto:

blockdev.c static const int if_max_devs[IF_COUNT] = { · line 60

Quoted 9 lines

blockdev.c +2 -0

@@ -53,6 +53,8 @@ static const int if_max_devs[IF_COUNT] = {
     [IF_SCSI] = 7,
 };
 
+static void block_job_cb(void *opaque, int ret);
+
 /*
  * We automatically delete the drive when a device using it gets
  * unplugged.  Questionable feature, but we can't just drop it.

Can you avoid the forward declaration?

Quoted 2 lines

Paolo

Yes, sure - honestly, I added this patch in, but I assumed that the similar patch of yours to support mirroring would go in first, making this patch moot.