Patchwork [v2,2/7] block: add live block commit functionality

login
register
mail settings
Submitter Jeff Cody
Date Sept. 25, 2012, 4:29 p.m.
Message ID <1efc2c8c9b7c215b787507afa5a34c0175f7f4f0.1348589526.git.jcody@redhat.com>
Download mbox | patch
Permalink /patch/186850/
State New
Headers show

Comments

Jeff Cody - Sept. 25, 2012, 4:29 p.m.
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 <jcody@redhat.com>
---
 block/Makefile.objs |   1 +
 block/commit.c      | 246 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 block_int.h         |  16 ++++
 trace-events        |   2 +
 4 files changed, 265 insertions(+)
 create mode 100644 block/commit.c
Eric Blake - Sept. 25, 2012, 6:12 p.m.
On 09/25/2012 10:29 AM, Jeff Cody wrote:
> 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 <jcody@redhat.com>

> +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);
> +        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.

> +
> +    /* 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').
Jeff Cody - Sept. 25, 2012, 6:58 p.m.
On 09/25/2012 02:12 PM, Eric Blake wrote:
> On 09/25/2012 10:29 AM, Jeff Cody wrote:
>> 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 <jcody@redhat.com>
> 
>> +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);
>> +        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.
> 

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.


>> +
>> +    /* 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').
> 

I'll add a quick check for that as well.  Patch 5 checks for it in one
scenario (default 'top'), and returns a generic error of:
"Invalid files for merge: top and base are the same"

I'll make sure to check for that in all scenarios (or just make the test
mentioned earlier incorporate top == base as well)

Thanks,
Jeff
Eric Blake - Sept. 25, 2012, 7:05 p.m.
On 09/25/2012 12:58 PM, Jeff Cody wrote:
> On 09/25/2012 02:12 PM, Eric Blake wrote:
>> On 09/25/2012 10:29 AM, Jeff Cody wrote:
>>> 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)?

Shoot.  You're right, and I messed myself up by reviewing out of order :)

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

At any rate, it sounds like I got my point across, in spite of myself.
Kevin Wolf - Sept. 26, 2012, 2:03 p.m.
Am 25.09.2012 18:29, schrieb 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 <jcody@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Patch

diff --git a/block/Makefile.objs b/block/Makefile.objs
index b5754d3..4a136b8 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -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
diff --git a/block/commit.c b/block/commit.c
new file mode 100644
index 0000000..67fd9df
--- /dev/null
+++ b/block/commit.c
@@ -0,0 +1,246 @@ 
+/*
+ * Live block commit
+ *
+ * Copyright Red Hat, Inc. 2012
+ *
+ * Authors:
+ *  Jeff Cody   <jcody@redhat.com>
+ *  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 orig_overlay_flags;
+} CommitBlockJob;
+
+static int coroutine_fn commit_populate(BlockDriverState *bs,
+                                        BlockDriverState *base,
+                                        int64_t sector_num, int nb_sectors,
+                                        void *buf)
+{
+    int ret = 0;
+
+    ret = bdrv_read(bs, sector_num, buf, nb_sectors);
+    if (ret) {
+        return ret;
+    }
+
+    ret = bdrv_write(base, sector_num, buf, nb_sectors);
+    if (ret) {
+        return ret;
+    }
+
+    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 *overlay_bs = NULL;
+    int64_t sector_num, end;
+    int ret = 0;
+    int n = 0;
+    void *buf;
+    int bytes_written = 0;
+    int64_t base_len;
+
+    ret = s->common.len = bdrv_getlength(top);
+
+
+    if (s->common.len < 0) {
+        goto exit_restore_reopen;
+    }
+
+    ret = base_len = bdrv_getlength(base);
+    if (base_len < 0) {
+        goto exit_restore_reopen;
+    }
+
+    if (base_len < s->common.len) {
+        ret = bdrv_truncate(base, s->common.len);
+        if (ret) {
+            goto exit_restore_reopen;
+        }
+    }
+
+    overlay_bs = bdrv_find_overlay(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);
+        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 (copy) {
+            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_REPORT      ||
+                (s->on_error == BLOCK_ERR_STOP_ENOSPC && ret == -ENOSPC)) {
+                goto exit_free_buf;
+            } else {
+                n = 0;
+                continue;
+            }
+        }
+        /* Publish progress */
+        s->common.offset += n * BDRV_SECTOR_SIZE;
+    }
+
+    ret = 0;
+
+    if (!block_job_is_cancelled(&s->common) && sector_num == end) {
+        /* success */
+        ret = bdrv_drop_intermediate(active, top, base);
+    }
+
+exit_free_buf:
+    qemu_vfree(buf);
+
+exit_restore_reopen:
+    /* 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->orig_overlay_flags != bdrv_get_flags(overlay_bs)) {
+        bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL);
+    }
+
+    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, 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);
+        return;
+    }
+
+    orig_base_flags    = bdrv_get_flags(base);
+    orig_overlay_flags = bdrv_get_flags(overlay_bs);
+
+    /* 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);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
+
+    s = block_job_create(&commit_job_type, bs, speed, cb, opaque, errp);
+    if (!s) {
+        return;
+    }
+
+    s->base   = base;
+    s->top    = top;
+    s->active = bs;
+
+    s->base_flags          = orig_base_flags;
+    s->orig_overlay_flags  = orig_overlay_flags;
+
+    s->on_error = on_error;
+    s->common.co = qemu_coroutine_create(commit_run);
+
+    trace_commit_start(bs, base, top, s, s->common.co, opaque);
+    qemu_coroutine_enter(s->common.co, s);
+}
diff --git a/block_int.h b/block_int.h
index ac4245c..56164a7 100644
--- a/block_int.h
+++ b/block_int.h
@@ -463,4 +463,20 @@  void stream_start(BlockDriverState *bs, BlockDriverState *base,
                   BlockDriverCompletionFunc *cb,
                   void *opaque, Error **errp);
 
+/**
+ * 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.
+ * @cb: Completion function for the job.
+ * @opaque: Opaque pointer value passed to @cb.
+ * @errp: Error object.
+ *
+ */
+void commit_start(BlockDriverState *bs, BlockDriverState *base,
+                 BlockDriverState *top, int64_t speed,
+                 BlockErrorAction on_error, BlockDriverCompletionFunc *cb,
+                 void *opaque, Error **errp);
+
 #endif /* BLOCK_INT_H */
diff --git a/trace-events b/trace-events
index f5b5097..dbc3007 100644
--- a/trace-events
+++ b/trace-events
@@ -74,6 +74,8 @@  bdrv_co_do_copy_on_readv(void *bs, int64_t sector_num, int nb_sectors, int64_t c
 # block/stream.c
 stream_one_iteration(void *s, int64_t sector_num, int nb_sectors, int is_allocated) "s %p sector_num %"PRId64" nb_sectors %d is_allocated %d"
 stream_start(void *bs, void *base, void *s, void *co, void *opaque) "bs %p base %p s %p co %p opaque %p"
+commit_one_iteration(void *s, int64_t sector_num, int nb_sectors, int is_allocated) "s %p sector_num %"PRId64" nb_sectors %d is_allocated %d"
+commit_start(void *bs, void *base, void *top, void *s, void *co, void *opaque) "bs %p base %p top %p s %p co %p opaque %p"
 
 # blockdev.c
 qmp_block_job_cancel(void *job) "job %p"