[Qemu-devel] [PATCH v4 0/8] block: drive-backup live backup command

[Qemu-devel] [PATCH v4 0/8] block: drive-backup live backup command

[Stefan Hajnoczi]

Note: These patches apply to my block-next tree.  You can also grab the code
from git here:
git://github.com/stefanha/qemu.git block-backup-core

This series adds a new QMP command, drive-backup, which takes a point-in-time
snapshot of a block device.  The snapshot is copied out to a target block
device.  A simple example is:

  drive-backup device=virtio0 format=qcow2 target=backup-20130401.qcow2

The original drive-backup blockjob was written by Dietmar Maurer
<dietmar@proxmox.com>.  He is currently busy but I feel the feature is worth
pushing into QEMU since there has been interest.  This is my version of his
patch, plus the QMP command and qemu-iotests test case.

QMP 'transaction' support is included since v3.  It adds support for atomic
snapshots of multiple block devices.  I also added an 'abort' transaction to
allow testing of the .abort()/.cleanup() code path.  Thanks to Wenchao for
making qmp_transaction() extensible.

How is this different from block-stream and drive-mirror?
---------------------------------------------------------
Both block-stream and drive-mirror do not provide immediate point-in-time
snapshots.  Instead they copy data into a new file and then switch to it.  In
other words, the point at which the "snapshot" is taken cannot be controlled
directly.

drive-backup intercepts guest writes and saves data into the target block
device before it is overwritten.  The target block device can be a raw image
file, backing files are not used to implement this feature.

How can drive-backup be used?
-----------------------------
The simplest use-case is to copy a point-in-time snapshot to a local file.

More advanced users may wish to make the target an NBD URL.  The NBD server
listening on the other side can process the backup writes any way it wishes.  I
previously posted an RFC series with a backup server that streamed Dietmar's
VMA backup archive format.

What's next for drive-backup?
-----------------------------
1. Sync modes like drive-mirror (top, full, none).  This makes it possible to
   preserve the backing file chain.

v4:
 * Use notifier lists and BdrvTrackedRequest instead of custom callbacks [bonzini]
 * Add drive-backup QMP example JSON [eblake]
 * Add "Since: 1.6" to QMP schema changes [eblake]

v3:
 * Rename to drive-backup for consistency with drive-mirror [kwolf]
 * Add QMP transaction support [kwolf]
 * Introduce bdrv_add_before_write_cb() to hook writes
 * Mention 'query-block-jobs' lists job of type 'backup' [eblake]
 * Rename rwlock to flush_rwlock [kwolf]
 * Fix space in block/backup.c comment [kwolf]

v2:
 * s/block_backup/block-backup/ in commit message [eblake]
 * Avoid funny spacing in QMP docs [eblake]
 * Document query-block-jobs and block-job-cancel usage [eblake]

Dietmar Maurer (1):
  block: add basic backup support to block driver

Stefan Hajnoczi (7):
  block: add bdrv_add_before_write_notifier()
  block: add drive-backup QMP command
  qemu-iotests: add 055 drive-backup test case
  blockdev: rename BlkTransactionStates to singular
  blockdev: add DriveBackup transaction
  blockdev: add Abort transaction
  qemu-iotests: test 'drive-backup' transaction in 055

 block.c                    |  18 ++-
 block/Makefile.objs        |   1 +
 block/backup.c             | 283 ++++++++++++++++++++++++++++++++++++
 blockdev.c                 | 264 +++++++++++++++++++++++++++-------
 include/block/block_int.h  |  38 ++++-
 qapi-schema.json           |  69 ++++++++-
 qmp-commands.hx            |  37 +++++
 tests/qemu-iotests/055     | 348 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/055.out |   5 +
 tests/qemu-iotests/group   |   1 +
 10 files changed, 1000 insertions(+), 64 deletions(-)
 create mode 100644 block/backup.c
 create mode 100755 tests/qemu-iotests/055
 create mode 100644 tests/qemu-iotests/055.out

-- 
1.8.1.4

[Qemu-devel] [PATCH v4 1/8] block: add bdrv_add_before_write_notifier()

[Stefan Hajnoczi]

The bdrv_add_before_write_notifier() function installs a callback that
is invoked before a write request is processed.  This will be used to
implement copy-on-write point-in-time snapshots where we need to copy
out old data before overwriting it.

Note that BdrvTrackedRequest is moved to block_int.h since it is passed
to .notify() functions.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block.c                   | 18 ++++++++----------
 include/block/block_int.h | 22 +++++++++++++++++++++-
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 3f87489..591e312 100644
--- a/block.c
+++ b/block.c
@@ -308,6 +308,7 @@ BlockDriverState *bdrv_new(const char *device_name)
     }
     bdrv_iostatus_disable(bs);
     notifier_list_init(&bs->close_notifiers);
+    notifier_list_init(&bs->before_write_notifiers);
 
     return bs;
 }
@@ -1839,16 +1840,6 @@ int bdrv_commit_all(void)
     return 0;
 }
 
-struct BdrvTrackedRequest {
-    BlockDriverState *bs;
-    int64_t sector_num;
-    int nb_sectors;
-    bool is_write;
-    QLIST_ENTRY(BdrvTrackedRequest) list;
-    Coroutine *co; /* owner, used for deadlock detection */
-    CoQueue wait_queue; /* coroutines blocked on this request */
-};
-
 /**
  * Remove an active request from the tracked requests list
  *
@@ -2619,6 +2610,8 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
 
     tracked_request_begin(&req, bs, sector_num, nb_sectors, true);
 
+    notifier_list_notify(&bs->before_write_notifiers, &req);
+
     if (flags & BDRV_REQ_ZERO_WRITE) {
         ret = bdrv_co_do_write_zeroes(bs, sector_num, nb_sectors);
     } else {
@@ -4883,3 +4876,8 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs)
     /* Currently BlockDriverState always uses the main loop AioContext */
     return qemu_get_aio_context();
 }
+
+void bdrv_add_before_write_notifier(BlockDriverState *bs, Notifier *notifier)
+{
+    notifier_list_add(&bs->before_write_notifiers, notifier);
+}
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6078dd3..a498fb0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -58,7 +58,16 @@
 #define BLOCK_OPT_LAZY_REFCOUNTS    "lazy_refcounts"
 #define BLOCK_OPT_ADAPTER_TYPE      "adapter_type"
 
-typedef struct BdrvTrackedRequest BdrvTrackedRequest;
+typedef struct BdrvTrackedRequest {
+    BlockDriverState *bs;
+    int64_t sector_num;
+    int nb_sectors;
+    bool is_write;
+    QLIST_ENTRY(BdrvTrackedRequest) list;
+    Coroutine *co; /* owner, used for deadlock detection */
+    CoQueue wait_queue; /* coroutines blocked on this request */
+} BdrvTrackedRequest;
+
 
 typedef struct BlockIOLimit {
     int64_t bps[3];
@@ -247,6 +256,9 @@ struct BlockDriverState {
 
     NotifierList close_notifiers;
 
+    /* Callback before write request is processed */
+    NotifierList before_write_notifiers;
+
     /* number of in-flight copy-on-read requests */
     unsigned int copy_on_read_in_flight;
 
@@ -298,6 +310,14 @@ void bdrv_set_io_limits(BlockDriverState *bs,
                         BlockIOLimit *io_limits);
 
 /**
+ * bdrv_add_before_write_notifier:
+ *
+ * Register a callback that is invoked before write requests are processed but
+ * after any throttling or waiting for overlapping requests.
+ */
+void bdrv_add_before_write_notifier(BlockDriverState *bs, Notifier *notifier);
+
+/**
  * bdrv_get_aio_context:
  *
  * Returns: the currently bound #AioContext
-- 
1.8.1.4

[Qemu-devel] [PATCH v4 2/8] block: add basic backup support to block driver

[Stefan Hajnoczi]

From: Dietmar Maurer <dietmar@proxmox.com>

backup_start() creates a block job that copies a point-in-time snapshot
of a block device to a target block device.

We call backup_do_cow() for each write during backup. That function
reads the original data from the block device before it gets
overwritten.  The data is then written to the target device.

Currently backup cluster size is hardcoded to 65536 bytes.

[I made a number of changes to Dietmar's original patch and folded them
in to make code review easy.  Here is the full list:

 * Drop BackupDumpFunc interface in favor of a target block device
 * Detect zero clusters with buffer_is_zero()
 * Don't write zero clusters to the target
 * Use 0 delay instead of 1us, like other block jobs
 * Unify creation/start functions into backup_start()
 * Simplify cleanup, free bitmap in backup_run() instead of cb
 * function
 * Use HBitmap to avoid duplicating bitmap code
 * Use bdrv_getlength() instead of accessing ->total_sectors
 * directly
 * Delete the backup.h header file, it is no longer necessary
 * Move ./backup.c to block/backup.c
 * Remove #ifdefed out code
 * Coding style and whitespace cleanups
 * Use bdrv_add_before_write_notifier() instead of blockjob-specific hooks
 * Keep our own in-flight CowRequest list instead of using block.c
   tracked requests.  This means a little code duplication but is much
   simpler than trying to share the tracked requests list and use the
   backup block size.

-- stefanha]

Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/Makefile.objs       |   1 +
 block/backup.c            | 283 ++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block_int.h |  16 +++
 3 files changed, 300 insertions(+)
 create mode 100644 block/backup.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 5f0358a..88bd101 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -20,5 +20,6 @@ endif
 common-obj-y += stream.o
 common-obj-y += commit.o
 common-obj-y += mirror.o
+common-obj-y += backup.o
 
 $(obj)/curl.o: QEMU_CFLAGS+=$(CURL_CFLAGS)
diff --git a/block/backup.c b/block/backup.c
new file mode 100644
index 0000000..5438e26
--- /dev/null
+++ b/block/backup.c
@@ -0,0 +1,283 @@
+/*
+ * QEMU backup
+ *
+ * Copyright (C) 2013 Proxmox Server Solutions
+ *
+ * Authors:
+ *  Dietmar Maurer (dietmar@proxmox.com)
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include <stdio.h>
+#include <errno.h>
+#include <unistd.h>
+
+#include "block/block.h"
+#include "block/block_int.h"
+#include "block/blockjob.h"
+#include "qemu/ratelimit.h"
+
+#define DEBUG_BACKUP 0
+
+#define DPRINTF(fmt, ...) \
+    do { \
+        if (DEBUG_BACKUP) { \
+            fprintf(stderr, "backup: " fmt, ## __VA_ARGS__); \
+        } \
+    } while (0)
+
+#define BACKUP_CLUSTER_BITS 16
+#define BACKUP_CLUSTER_SIZE (1 << BACKUP_CLUSTER_BITS)
+#define BACKUP_BLOCKS_PER_CLUSTER (BACKUP_CLUSTER_SIZE / BDRV_SECTOR_SIZE)
+
+#define SLICE_TIME 100000000ULL /* ns */
+
+typedef struct CowRequest {
+    int64_t start;
+    int64_t end;
+    QLIST_ENTRY(CowRequest) list;
+    CoQueue wait_queue; /* coroutines blocked on this request */
+} CowRequest;
+
+typedef struct BackupBlockJob {
+    BlockJob common;
+    BlockDriverState *target;
+    RateLimit limit;
+    CoRwlock flush_rwlock;
+    uint64_t sectors_read;
+    HBitmap *bitmap;
+    QLIST_HEAD(, CowRequest) inflight_reqs;
+} BackupBlockJob;
+
+/* See if in-flight requests overlap and wait for them to complete */
+static void coroutine_fn wait_for_overlapping_requests(BackupBlockJob *job,
+                                                       int64_t start,
+                                                       int64_t end)
+{
+    CowRequest *req;
+    bool retry;
+
+    do {
+        retry = false;
+        QLIST_FOREACH(req, &job->inflight_reqs, list) {
+            if (end > req->start && start < req->end) {
+                qemu_co_queue_wait(&req->wait_queue);
+                retry = true;
+                break;
+            }
+        }
+    } while (retry);
+}
+
+/* Keep track of an in-flight request */
+static void cow_request_begin(CowRequest *req, BackupBlockJob *job,
+                                     int64_t start, int64_t end)
+{
+    req->start = start;
+    req->end = end;
+    qemu_co_queue_init(&req->wait_queue);
+    QLIST_INSERT_HEAD(&job->inflight_reqs, req, list);
+}
+
+/* Forget about a completed request */
+static void cow_request_end(CowRequest *req)
+{
+    QLIST_REMOVE(req, list);
+    qemu_co_queue_restart_all(&req->wait_queue);
+}
+
+static int coroutine_fn backup_do_cow(BlockDriverState *bs,
+                                      int64_t sector_num, int nb_sectors)
+{
+    BackupBlockJob *job = (BackupBlockJob *)bs->job;
+    CowRequest cow_request;
+    struct iovec iov;
+    QEMUIOVector bounce_qiov;
+    void *bounce_buffer = NULL;
+    int ret = 0;
+    int64_t start, end;
+
+    qemu_co_rwlock_rdlock(&job->flush_rwlock);
+
+    start = sector_num / BACKUP_BLOCKS_PER_CLUSTER;
+    end = DIV_ROUND_UP(sector_num + nb_sectors, BACKUP_BLOCKS_PER_CLUSTER);
+
+    DPRINTF("brdv_co_backup_cow enter %s C%" PRId64 " %" PRId64 " %d\n",
+            bdrv_get_device_name(bs), start, sector_num, nb_sectors);
+
+    wait_for_overlapping_requests(job, start, end);
+    cow_request_begin(&cow_request, job, start, end);
+
+    for (; start < end; start++) {
+        if (hbitmap_get(job->bitmap, start)) {
+            DPRINTF("brdv_co_backup_cow skip C%" PRId64 "\n", start);
+            continue; /* already copied */
+        }
+
+        /* immediately set bitmap (avoid coroutine race) */
+        hbitmap_set(job->bitmap, start, 1);
+
+        DPRINTF("brdv_co_backup_cow C%" PRId64 "\n", start);
+
+        if (!bounce_buffer) {
+            iov.iov_len = BACKUP_CLUSTER_SIZE;
+            iov.iov_base = bounce_buffer = qemu_blockalign(bs, iov.iov_len);
+            qemu_iovec_init_external(&bounce_qiov, &iov, 1);
+        }
+
+        ret = bdrv_co_readv(bs, start * BACKUP_BLOCKS_PER_CLUSTER,
+                            BACKUP_BLOCKS_PER_CLUSTER,
+                            &bounce_qiov);
+        if (ret < 0) {
+            DPRINTF("brdv_co_backup_cow bdrv_read C%" PRId64 " failed\n",
+                    start);
+            goto out;
+        }
+
+        job->sectors_read += BACKUP_BLOCKS_PER_CLUSTER;
+
+        if (!buffer_is_zero(bounce_buffer, BACKUP_CLUSTER_SIZE)) {
+            ret = bdrv_co_writev(job->target, start * BACKUP_BLOCKS_PER_CLUSTER,
+                                 BACKUP_BLOCKS_PER_CLUSTER,
+                                 &bounce_qiov);
+            if (ret < 0) {
+                DPRINTF("brdv_co_backup_cow dump_cluster_cb C%" PRId64
+                        " failed\n", start);
+                goto out;
+            }
+        }
+
+        DPRINTF("brdv_co_backup_cow done C%" PRId64 "\n", start);
+    }
+
+out:
+    if (bounce_buffer) {
+        qemu_vfree(bounce_buffer);
+    }
+
+    cow_request_end(&cow_request);
+
+    qemu_co_rwlock_unlock(&job->flush_rwlock);
+
+    return ret;
+}
+
+static void coroutine_fn backup_before_write_notify(Notifier *notifier,
+                                                    void *opaque)
+{
+    BdrvTrackedRequest *req = opaque;
+    backup_do_cow(req->bs, req->sector_num, req->nb_sectors);
+}
+
+static void backup_set_speed(BlockJob *job, int64_t speed, Error **errp)
+{
+    BackupBlockJob *s = container_of(job, BackupBlockJob, 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 backup_job_type = {
+    .instance_size = sizeof(BackupBlockJob),
+    .job_type = "backup",
+    .set_speed = backup_set_speed,
+};
+
+static void coroutine_fn backup_run(void *opaque)
+{
+    BackupBlockJob *job = opaque;
+    BlockDriverState *bs = job->common.bs;
+    Notifier before_write = {
+        .notify = backup_before_write_notify,
+    };
+    int64_t start, end;
+    int ret = 0;
+
+    QLIST_INIT(&job->inflight_reqs);
+    qemu_co_rwlock_init(&job->flush_rwlock);
+
+    start = 0;
+    end = DIV_ROUND_UP(bdrv_getlength(bs) / BDRV_SECTOR_SIZE,
+                       BACKUP_BLOCKS_PER_CLUSTER);
+
+    job->bitmap = hbitmap_alloc(end, 0);
+
+    bdrv_add_before_write_notifier(bs, &before_write);
+
+    DPRINTF("backup_run start %s %" PRId64 " %" PRId64 "\n",
+            bdrv_get_device_name(bs), start, end);
+
+    for (; start < end; start++) {
+        if (block_job_is_cancelled(&job->common)) {
+            break;
+        }
+
+        /* we need to yield so that qemu_aio_flush() returns.
+         * (without, VM does not reboot)
+         */
+        if (job->common.speed) {
+            uint64_t delay_ns = ratelimit_calculate_delay(
+                &job->limit, job->sectors_read);
+            job->sectors_read = 0;
+            block_job_sleep_ns(&job->common, rt_clock, delay_ns);
+        } else {
+            block_job_sleep_ns(&job->common, rt_clock, 0);
+        }
+
+        if (block_job_is_cancelled(&job->common)) {
+            break;
+        }
+
+        DPRINTF("backup_run loop C%" PRId64 "\n", start);
+
+        ret = backup_do_cow(bs, start * BACKUP_BLOCKS_PER_CLUSTER, 1);
+        if (ret < 0) {
+            break;
+        }
+
+        /* Publish progress */
+        job->common.offset += BACKUP_CLUSTER_SIZE;
+    }
+
+    notifier_remove(&before_write);
+
+    /* wait until pending backup_do_cow() calls have completed */
+    qemu_co_rwlock_wrlock(&job->flush_rwlock);
+    qemu_co_rwlock_unlock(&job->flush_rwlock);
+
+    hbitmap_free(job->bitmap);
+
+    bdrv_delete(job->target);
+
+    DPRINTF("backup_run complete %d\n", ret);
+    block_job_completed(&job->common, ret);
+}
+
+void backup_start(BlockDriverState *bs, BlockDriverState *target,
+                  int64_t speed,
+                  BlockDriverCompletionFunc *cb, void *opaque,
+                  Error **errp)
+{
+    assert(bs);
+    assert(target);
+    assert(cb);
+
+    DPRINTF("backup_start %s\n", bdrv_get_device_name(bs));
+
+    BackupBlockJob *job = block_job_create(&backup_job_type, bs, speed,
+                                           cb, opaque, errp);
+    if (!job) {
+        return;
+    }
+
+    job->target = target;
+    job->common.len = bdrv_getlength(bs);
+    job->common.co = qemu_coroutine_create(backup_run);
+    qemu_coroutine_enter(job->common.co, job);
+}
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a498fb0..625ebcf 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -397,4 +397,20 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
                   BlockDriverCompletionFunc *cb,
                   void *opaque, Error **errp);
 
+/*
+ * backup_start:
+ * @bs: Block device to operate on.
+ * @target: Block device to write to.
+ * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
+ * @cb: Completion function for the job.
+ * @opaque: Opaque pointer value passed to @cb.
+ *
+ * Start a backup operation on @bs.  Clusters in @bs are written to @target
+ * until the job is cancelled or manually completed.
+ */
+void backup_start(BlockDriverState *bs, BlockDriverState *target,
+                  int64_t speed,
+                  BlockDriverCompletionFunc *cb, void *opaque,
+                  Error **errp);
+
 #endif /* BLOCK_INT_H */
-- 
1.8.1.4

[Qemu-devel] [PATCH v4 3/8] block: add drive-backup QMP command

[Stefan Hajnoczi]

@drive-backup

Start a point-in-time copy of a block device to a new destination.  The
status of ongoing drive-backup operations can be checked with
query-block-jobs where the BlockJobInfo.type field has the value 'backup'.
The operation can be stopped before it has completed using the
block-job-cancel command.

@device: the name of the device whose writes should be mirrored.

@target: the target of the new image. If the file exists, or if it
         is a device, the existing file/device will be used as the new
         destination.  If it does not exist, a new file will be created.

@format: #optional the format of the new destination, default is to
         probe if @mode is 'existing', else the format of the source

@mode: #optional whether and how QEMU should create a new image, default is
       'absolute-paths'.

@speed: #optional the maximum speed, in bytes per second

Returns: nothing on success
         If @device is not a valid block device, DeviceNotFound

Since 1.6

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c       | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 32 ++++++++++++++++++++
 qmp-commands.hx  | 37 +++++++++++++++++++++++
 3 files changed, 161 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index d1ec99a..c7a5e1e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1353,6 +1353,98 @@ void qmp_block_commit(const char *device,
     drive_get_ref(drive_get_by_blockdev(bs));
 }
 
+void qmp_drive_backup(const char *device, const char *target,
+                      bool has_format, const char *format,
+                      bool has_mode, enum NewImageMode mode,
+                      bool has_speed, int64_t speed,
+                      Error **errp)
+{
+    BlockDriverState *bs;
+    BlockDriverState *target_bs;
+    BlockDriver *proto_drv;
+    BlockDriver *drv = NULL;
+    Error *local_err = NULL;
+    int flags;
+    uint64_t size;
+    int ret;
+
+    if (!has_speed) {
+        speed = 0;
+    }
+    if (!has_mode) {
+        mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+    }
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (!bdrv_is_inserted(bs)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        return;
+    }
+
+    if (!has_format) {
+        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
+    }
+    if (format) {
+        drv = bdrv_find_format(format);
+        if (!drv) {
+            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+            return;
+        }
+    }
+
+    if (bdrv_in_use(bs)) {
+        error_set(errp, QERR_DEVICE_IN_USE, device);
+        return;
+    }
+
+    flags = bs->open_flags | BDRV_O_RDWR;
+
+    proto_drv = bdrv_find_protocol(target);
+    if (!proto_drv) {
+        error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+        return;
+    }
+
+    bdrv_get_geometry(bs, &size);
+    size *= 512;
+    if (mode != NEW_IMAGE_MODE_EXISTING) {
+        assert(format && drv);
+        bdrv_img_create(target, format,
+                        NULL, NULL, NULL, size, flags, &local_err, false);
+    }
+
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    target_bs = bdrv_new("");
+    ret = bdrv_open(target_bs, target, NULL, flags, drv);
+
+    if (ret < 0) {
+        bdrv_delete(target_bs);
+        error_set(errp, QERR_OPEN_FILE_FAILED, target);
+        return;
+    }
+
+    backup_start(bs, target_bs, speed, block_job_cb, bs, &local_err);
+    if (local_err != NULL) {
+        bdrv_delete(target_bs);
+        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));
+}
+
 #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
 
 void qmp_drive_mirror(const char *device, const char *target,
diff --git a/qapi-schema.json b/qapi-schema.json
index 98cd81c..e716522 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1730,6 +1730,38 @@
             '*speed': 'int' } }
 
 ##
+# @drive-backup
+#
+# Start a point-in-time copy of a block device to a new destination.  The
+# status of ongoing drive-backup operations can be checked with
+# query-block-jobs where the BlockJobInfo.type field has the value 'backup'.
+# The operation can be stopped before it has completed using the
+# block-job-cancel command.
+#
+# @device: the name of the device whose writes should be mirrored.
+#
+# @target: the target of the new image. If the file exists, or if it
+#          is a device, the existing file/device will be used as the new
+#          destination.  If it does not exist, a new file will be created.
+#
+# @format: #optional the format of the new destination, default is to
+#          probe if @mode is 'existing', else the format of the source
+#
+# @mode: #optional whether and how QEMU should create a new image, default is
+#        'absolute-paths'.
+#
+# @speed: #optional the maximum speed, in bytes per second
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#
+# Since 1.6
+##
+{ 'command': 'drive-backup',
+  'data': { 'device': 'str', 'target': 'str', '*format': 'str',
+            '*mode': 'NewImageMode', '*speed': 'int' } }
+
+##
 # @drive-mirror
 #
 # Start mirroring a block device's writes to a new destination.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ffd130e..99545b3 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -912,6 +912,43 @@ EQMP
     },
 
     {
+        .name       = "drive-backup",
+        .args_type  = "device:B,target:s,speed:i?,mode:s?,format:s?",
+        .mhandler.cmd_new = qmp_marshal_input_drive_backup,
+    },
+
+SQMP
+drive-backup
+------------
+
+Start a point-in-time copy of a block device to a new destination.  The
+status of ongoing drive-backup operations can be checked with
+query-block-jobs where the BlockJobInfo.type field has the value 'backup'.
+The operation can be stopped before it has completed using the
+block-job-cancel command.
+
+Arguments:
+
+- "device": the name of the device whose writes should be mirrored
+            (json-string)
+- "target": the target of the new image. If the file exists, or if it is a
+            device, the existing file/device will be used as the new
+            destination.  If it does not exist, a new file will be created.
+            (json-string)
+- "format": the format of the new destination, default is to probe if 'mode' is
+            'existing', else the format of the source
+            (json-string, optional)
+- "mode": whether and how QEMU should create a new image
+          (NewImageMode, optional, default 'absolute-paths')
+- "speed": the maximum speed, in bytes per second (json-int, optional)
+
+Example:
+-> { "execute": "drive-backup", "arguments": { "device": "drive0",
+                                               "target": "backup.img" } }
+<- { "return": {} }
+EQMP
+
+    {
         .name       = "block-job-set-speed",
         .args_type  = "device:B,speed:o",
         .mhandler.cmd_new = qmp_marshal_input_block_job_set_speed,
-- 
1.8.1.4

[Qemu-devel] [PATCH v4 4/8] qemu-iotests: add 055 drive-backup test case

[Stefan Hajnoczi]

Testing drive-backup is similar to image streaming and drive mirroring.
This test case is based on 041.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/055     | 230 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/055.out |   5 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 236 insertions(+)
 create mode 100755 tests/qemu-iotests/055
 create mode 100644 tests/qemu-iotests/055.out

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
new file mode 100755
index 0000000..bc2eebf
--- /dev/null
+++ b/tests/qemu-iotests/055
@@ -0,0 +1,230 @@
+#!/usr/bin/env python
+#
+# Tests for drive-backup
+#
+# Copyright (C) 2013 Red Hat, Inc.
+#
+# Based on 041.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import time
+import os
+import iotests
+from iotests import qemu_img, qemu_io
+
+test_img = os.path.join(iotests.test_dir, 'test.img')
+target_img = os.path.join(iotests.test_dir, 'target.img')
+
+class DriveBackupTestCase(iotests.QMPTestCase):
+    '''Abstract base class for drive-backup test cases'''
+
+    def assert_no_active_backups(self):
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return', [])
+
+    def cancel_and_wait(self, drive='drive0'):
+        '''Cancel a block job and wait for it to finish'''
+        result = self.vm.qmp('block-job-cancel', device=drive)
+        self.assert_qmp(result, 'return', {})
+
+        cancelled = False
+        while not cancelled:
+            for event in self.vm.get_qmp_events(wait=True):
+                if event['event'] == 'BLOCK_JOB_COMPLETED' or \
+                   event['event'] == 'BLOCK_JOB_CANCELLED':
+                    self.assert_qmp(event, 'data/type', 'backup')
+                    self.assert_qmp(event, 'data/device', drive)
+                    cancelled = True
+
+        self.assert_no_active_backups()
+
+    def complete_and_wait(self):
+        completed = False
+        while not completed:
+            for event in self.vm.get_qmp_events(wait=True):
+                if event['event'] == 'BLOCK_JOB_COMPLETED':
+                    self.assert_qmp(event, 'data/type', 'backup')
+                    self.assert_qmp(event, 'data/device', 'drive0')
+                    self.assert_qmp(event, 'data/offset', self.image_len)
+                    self.assert_qmp(event, 'data/len', self.image_len)
+                    completed = True
+        self.assert_no_active_backups()
+
+    def compare_images(self, img1, img2):
+        try:
+            qemu_img('convert', '-f', iotests.imgfmt, '-O', 'raw', img1, img1 + '.raw')
+            qemu_img('convert', '-f', iotests.imgfmt, '-O', 'raw', img2, img2 + '.raw')
+            file1 = open(img1 + '.raw', 'r')
+            file2 = open(img2 + '.raw', 'r')
+            return file1.read() == file2.read()
+        finally:
+            if file1 is not None:
+                file1.close()
+            if file2 is not None:
+                file2.close()
+            try:
+                os.remove(img1 + '.raw')
+            except OSError:
+                pass
+            try:
+                os.remove(img2 + '.raw')
+            except OSError:
+                pass
+
+class TestSingleDrive(DriveBackupTestCase):
+    image_len = 120 * 1024 * 1024 # MB
+
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestSingleDrive.image_len))
+        self.vm = iotests.VM().add_drive(test_img)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(test_img)
+        try:
+            os.remove(target_img)
+        except OSError:
+            pass
+
+    def test_complete(self):
+        self.assert_no_active_backups()
+
+        result = self.vm.qmp('drive-backup', device='drive0',
+                             target=target_img)
+        self.assert_qmp(result, 'return', {})
+
+        self.complete_and_wait()
+        self.vm.shutdown()
+        self.assertTrue(self.compare_images(test_img, target_img),
+                        'target image does not match source after backup')
+
+    def test_cancel(self):
+        self.assert_no_active_backups()
+
+        result = self.vm.qmp('drive-backup', device='drive0',
+                             target=target_img)
+        self.assert_qmp(result, 'return', {})
+
+        self.cancel_and_wait()
+
+    def test_pause(self):
+        self.assert_no_active_backups()
+
+        result = self.vm.qmp('drive-backup', device='drive0',
+                             target=target_img)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block-job-pause', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        time.sleep(1)
+        result = self.vm.qmp('query-block-jobs')
+        offset = self.dictpath(result, 'return[0]/offset')
+
+        time.sleep(1)
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return[0]/offset', offset)
+
+        result = self.vm.qmp('block-job-resume', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        self.complete_and_wait()
+        self.vm.shutdown()
+        self.assertTrue(self.compare_images(test_img, target_img),
+                        'target image does not match source after backup')
+
+    def test_medium_not_found(self):
+        result = self.vm.qmp('drive-backup', device='ide1-cd0',
+                             target=target_img)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_image_not_found(self):
+        result = self.vm.qmp('drive-backup', device='drive0',
+                             mode='existing', target=target_img)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_device_not_found(self):
+        result = self.vm.qmp('drive-backup', device='nonexistent',
+                             target=target_img)
+        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+class TestSetSpeed(DriveBackupTestCase):
+    image_len = 80 * 1024 * 1024 # MB
+
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestSetSpeed.image_len))
+        self.vm = iotests.VM().add_drive(test_img)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(test_img)
+        os.remove(target_img)
+
+    def test_set_speed(self):
+        self.assert_no_active_backups()
+
+        result = self.vm.qmp('drive-backup', device='drive0',
+                             target=target_img)
+        self.assert_qmp(result, 'return', {})
+
+        # Default speed is 0
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return[0]/device', 'drive0')
+        self.assert_qmp(result, 'return[0]/speed', 0)
+
+        result = self.vm.qmp('block-job-set-speed', device='drive0', speed=8 * 1024 * 1024)
+        self.assert_qmp(result, 'return', {})
+
+        # Ensure the speed we set was accepted
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return[0]/device', 'drive0')
+        self.assert_qmp(result, 'return[0]/speed', 8 * 1024 * 1024)
+
+        self.cancel_and_wait()
+
+        # Check setting speed in drive-backup works
+        result = self.vm.qmp('drive-backup', device='drive0',
+                             target=target_img, speed=4*1024*1024)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return[0]/device', 'drive0')
+        self.assert_qmp(result, 'return[0]/speed', 4 * 1024 * 1024)
+
+        self.cancel_and_wait()
+
+    def test_set_speed_invalid(self):
+        self.assert_no_active_backups()
+
+        result = self.vm.qmp('drive-backup', device='drive0',
+                             target=target_img, speed=-1)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        self.assert_no_active_backups()
+
+        result = self.vm.qmp('drive-backup', device='drive0',
+                             target=target_img)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        self.cancel_and_wait()
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=['raw', 'qcow2'])
diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out
new file mode 100644
index 0000000..594c16f
--- /dev/null
+++ b/tests/qemu-iotests/055.out
@@ -0,0 +1,5 @@
+........
+----------------------------------------------------------------------
+Ran 8 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 387b050..e5762f9 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -61,3 +61,4 @@
 052 rw auto backing
 053 rw auto
 054 rw auto
+055 rw auto
-- 
1.8.1.4

[Qemu-devel] [PATCH v4 5/8] blockdev: rename BlkTransactionStates to singular

[Stefan Hajnoczi]

The QMP 'transaction' command keeps a list of in-flight transactions.
The transaction state structure is called BlkTransactionStates even
though it only deals with a single transaction.  The only plural thing
is the linked list of transaction states.

I find it confusing to call the single structure "States".  This patch
renames it to "State", just like BlockDriverState is singular.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c | 104 ++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 52 insertions(+), 52 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index c7a5e1e..b6109da 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -780,7 +780,7 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
 
 /* New and old BlockDriverState structs for group snapshots */
 
-typedef struct BlkTransactionStates BlkTransactionStates;
+typedef struct BlkTransactionState BlkTransactionState;
 
 /* Only prepare() may fail. In a single transaction, only one of commit() or
    abort() will be called, clean() will always be called if it present. */
@@ -788,13 +788,13 @@ typedef struct BdrvActionOps {
     /* Size of state struct, in bytes. */
     size_t instance_size;
     /* Prepare the work, must NOT be NULL. */
-    void (*prepare)(BlkTransactionStates *common, Error **errp);
+    void (*prepare)(BlkTransactionState *common, Error **errp);
     /* Commit the changes, must NOT be NULL. */
-    void (*commit)(BlkTransactionStates *common);
+    void (*commit)(BlkTransactionState *common);
     /* Abort the changes on fail, can be NULL. */
-    void (*abort)(BlkTransactionStates *common);
+    void (*abort)(BlkTransactionState *common);
     /* Clean up resource in the end, can be NULL. */
-    void (*clean)(BlkTransactionStates *common);
+    void (*clean)(BlkTransactionState *common);
 } BdrvActionOps;
 
 /*
@@ -802,20 +802,20 @@ typedef struct BdrvActionOps {
  * that compiler will also arrange it to the same address with parent instance.
  * Later it will be used in free().
  */
-struct BlkTransactionStates {
+struct BlkTransactionState {
     TransactionAction *action;
     const BdrvActionOps *ops;
-    QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
+    QSIMPLEQ_ENTRY(BlkTransactionState) entry;
 };
 
 /* external snapshot private data */
-typedef struct ExternalSnapshotStates {
-    BlkTransactionStates common;
+typedef struct ExternalSnapshotState {
+    BlkTransactionState common;
     BlockDriverState *old_bs;
     BlockDriverState *new_bs;
-} ExternalSnapshotStates;
+} ExternalSnapshotState;
 
-static void external_snapshot_prepare(BlkTransactionStates *common,
+static void external_snapshot_prepare(BlkTransactionState *common,
                                       Error **errp)
 {
     BlockDriver *proto_drv;
@@ -826,8 +826,8 @@ static void external_snapshot_prepare(BlkTransactionStates *common,
     const char *new_image_file;
     const char *format = "qcow2";
     enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-    ExternalSnapshotStates *states =
-                             DO_UPCAST(ExternalSnapshotStates, common, common);
+    ExternalSnapshotState *state =
+                             DO_UPCAST(ExternalSnapshotState, common, common);
     TransactionAction *action = common->action;
 
     /* get parameters */
@@ -849,30 +849,30 @@ static void external_snapshot_prepare(BlkTransactionStates *common,
         return;
     }
 
-    states->old_bs = bdrv_find(device);
-    if (!states->old_bs) {
+    state->old_bs = bdrv_find(device);
+    if (!state->old_bs) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
         return;
     }
 
-    if (!bdrv_is_inserted(states->old_bs)) {
+    if (!bdrv_is_inserted(state->old_bs)) {
         error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
         return;
     }
 
-    if (bdrv_in_use(states->old_bs)) {
+    if (bdrv_in_use(state->old_bs)) {
         error_set(errp, QERR_DEVICE_IN_USE, device);
         return;
     }
 
-    if (!bdrv_is_read_only(states->old_bs)) {
-        if (bdrv_flush(states->old_bs)) {
+    if (!bdrv_is_read_only(state->old_bs)) {
+        if (bdrv_flush(state->old_bs)) {
             error_set(errp, QERR_IO_ERROR);
             return;
         }
     }
 
-    flags = states->old_bs->open_flags;
+    flags = state->old_bs->open_flags;
 
     proto_drv = bdrv_find_protocol(new_image_file);
     if (!proto_drv) {
@@ -883,8 +883,8 @@ static void external_snapshot_prepare(BlkTransactionStates *common,
     /* create new image w/backing file */
     if (mode != NEW_IMAGE_MODE_EXISTING) {
         bdrv_img_create(new_image_file, format,
-                        states->old_bs->filename,
-                        states->old_bs->drv->format_name,
+                        state->old_bs->filename,
+                        state->old_bs->drv->format_name,
                         NULL, -1, flags, &local_err, false);
         if (error_is_set(&local_err)) {
             error_propagate(errp, local_err);
@@ -893,42 +893,42 @@ static void external_snapshot_prepare(BlkTransactionStates *common,
     }
 
     /* We will manually add the backing_hd field to the bs later */
-    states->new_bs = bdrv_new("");
+    state->new_bs = bdrv_new("");
     /* TODO Inherit bs->options or only take explicit options with an
      * extended QMP command? */
-    ret = bdrv_open(states->new_bs, new_image_file, NULL,
+    ret = bdrv_open(state->new_bs, new_image_file, NULL,
                     flags | BDRV_O_NO_BACKING, drv);
     if (ret != 0) {
         error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
     }
 }
 
-static void external_snapshot_commit(BlkTransactionStates *common)
+static void external_snapshot_commit(BlkTransactionState *common)
 {
-    ExternalSnapshotStates *states =
-                             DO_UPCAST(ExternalSnapshotStates, common, common);
+    ExternalSnapshotState *state =
+                             DO_UPCAST(ExternalSnapshotState, common, common);
 
-    /* This removes our old bs from the bdrv_states, and adds the new bs */
-    bdrv_append(states->new_bs, states->old_bs);
+    /* This removes our old bs and adds the new bs */
+    bdrv_append(state->new_bs, state->old_bs);
     /* We don't need (or want) to use the transactional
      * bdrv_reopen_multiple() across all the entries at once, because we
      * don't want to abort all of them if one of them fails the reopen */
-    bdrv_reopen(states->new_bs, states->new_bs->open_flags & ~BDRV_O_RDWR,
+    bdrv_reopen(state->new_bs, state->new_bs->open_flags & ~BDRV_O_RDWR,
                 NULL);
 }
 
-static void external_snapshot_abort(BlkTransactionStates *common)
+static void external_snapshot_abort(BlkTransactionState *common)
 {
-    ExternalSnapshotStates *states =
-                             DO_UPCAST(ExternalSnapshotStates, common, common);
-    if (states->new_bs) {
-        bdrv_delete(states->new_bs);
+    ExternalSnapshotState *state =
+                             DO_UPCAST(ExternalSnapshotState, common, common);
+    if (state->new_bs) {
+        bdrv_delete(state->new_bs);
     }
 }
 
 static const BdrvActionOps actions[] = {
     [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
-        .instance_size = sizeof(ExternalSnapshotStates),
+        .instance_size = sizeof(ExternalSnapshotState),
         .prepare  = external_snapshot_prepare,
         .commit   = external_snapshot_commit,
         .abort = external_snapshot_abort,
@@ -943,10 +943,10 @@ static const BdrvActionOps actions[] = {
 void qmp_transaction(TransactionActionList *dev_list, Error **errp)
 {
     TransactionActionList *dev_entry = dev_list;
-    BlkTransactionStates *states, *next;
+    BlkTransactionState *state, *next;
     Error *local_err = NULL;
 
-    QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) snap_bdrv_states;
+    QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionState) snap_bdrv_states;
     QSIMPLEQ_INIT(&snap_bdrv_states);
 
     /* drain all i/o before any snapshots */
@@ -963,20 +963,20 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
         assert(dev_info->kind < ARRAY_SIZE(actions));
 
         ops = &actions[dev_info->kind];
-        states = g_malloc0(ops->instance_size);
-        states->ops = ops;
-        states->action = dev_info;
-        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
+        state = g_malloc0(ops->instance_size);
+        state->ops = ops;
+        state->action = dev_info;
+        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
 
-        states->ops->prepare(states, &local_err);
+        state->ops->prepare(state, &local_err);
         if (error_is_set(&local_err)) {
             error_propagate(errp, local_err);
             goto delete_and_fail;
         }
     }
 
-    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        states->ops->commit(states);
+    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+        state->ops->commit(state);
     }
 
     /* success */
@@ -987,17 +987,17 @@ delete_and_fail:
     * failure, and it is all-or-none; abandon each new bs, and keep using
     * the original bs for all images
     */
-    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        if (states->ops->abort) {
-            states->ops->abort(states);
+    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+        if (state->ops->abort) {
+            state->ops->abort(state);
         }
     }
 exit:
-    QSIMPLEQ_FOREACH_SAFE(states, &snap_bdrv_states, entry, next) {
-        if (states->ops->clean) {
-            states->ops->clean(states);
+    QSIMPLEQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) {
+        if (state->ops->clean) {
+            state->ops->clean(state);
         }
-        g_free(states);
+        g_free(state);
     }
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH v4 6/8] blockdev: add DriveBackup transaction

[Stefan Hajnoczi]

This patch adds a transactional version of the drive-backup QMP command.
It allows atomic snapshots of multiple drives along with automatic
cleanup if there is a failure to start one of the backup jobs.

Note that QMP events are emitted for block job completion/cancellation
and the block job will be listed by query-block-jobs.

@DriveBackup

@device: the name of the device whose writes should be mirrored.

@target: the target of the new image. If the file exists, or if it
         is a device, the existing file/device will be used as the new
         destination.  If it does not exist, a new file will be created.

@format: #optional the format of the new destination, default is to
         probe if @mode is 'existing', else the format of the source

@mode: #optional whether and how QEMU should create a new image, default is
       'absolute-paths'.

@speed: #optional the maximum speed, in bytes per second

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c       | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 26 +++++++++++++++++++++++++-
 2 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index b6109da..c386bb6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -926,6 +926,53 @@ static void external_snapshot_abort(BlkTransactionState *common)
     }
 }
 
+typedef struct DriveBackupState {
+    BlkTransactionState common;
+    BlockDriverState *bs;
+    BlockJob *job;
+} DriveBackupState;
+
+static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
+{
+    DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+    DriveBackup *backup;
+    Error *local_err = NULL;
+
+    assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
+    backup = common->action->drive_backup;
+
+    qmp_drive_backup(backup->device, backup->target,
+                     backup->has_format, backup->format,
+                     backup->has_mode, backup->mode,
+                     backup->has_speed, backup->speed,
+                     &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+        state->bs = NULL;
+        state->job = NULL;
+        return;
+    }
+
+    state->bs = bdrv_find(backup->device);
+    state->job = state->bs->job;
+}
+
+static void drive_backup_commit(BlkTransactionState *common)
+{
+    /* Block job has started, nothing to do here */
+}
+
+static void drive_backup_abort(BlkTransactionState *common)
+{
+    DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+    BlockDriverState *bs = state->bs;
+
+    /* Only cancel if it's the job we started */
+    if (bs && bs->job && bs->job == state->job) {
+        block_job_cancel_sync(bs->job);
+    }
+}
+
 static const BdrvActionOps actions[] = {
     [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
         .instance_size = sizeof(ExternalSnapshotState),
@@ -933,6 +980,12 @@ static const BdrvActionOps actions[] = {
         .commit   = external_snapshot_commit,
         .abort = external_snapshot_abort,
     },
+    [TRANSACTION_ACTION_KIND_DRIVE_BACKUP] = {
+        .instance_size = sizeof(DriveBackupState),
+        .prepare = drive_backup_prepare,
+        .commit = drive_backup_commit,
+        .abort = drive_backup_abort,
+    },
 };
 
 /*
diff --git a/qapi-schema.json b/qapi-schema.json
index e716522..114ae50 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1609,6 +1609,29 @@
             '*mode': 'NewImageMode' } }
 
 ##
+# @DriveBackup
+#
+# @device: the name of the device whose writes should be mirrored.
+#
+# @target: the target of the new image. If the file exists, or if it
+#          is a device, the existing file/device will be used as the new
+#          destination.  If it does not exist, a new file will be created.
+#
+# @format: #optional the format of the new destination, default is to
+#          probe if @mode is 'existing', else the format of the source
+#
+# @mode: #optional whether and how QEMU should create a new image, default is
+#        'absolute-paths'.
+#
+# @speed: #optional the maximum speed, in bytes per second
+#
+# Since: 1.6
+##
+{ 'type': 'DriveBackup',
+  'data': { 'device': 'str', 'target': 'str', '*format': 'str',
+            '*mode': 'NewImageMode', '*speed': 'int' } }
+
+##
 # @TransactionAction
 #
 # A discriminated record of operations that can be performed with
@@ -1616,7 +1639,8 @@
 ##
 { 'union': 'TransactionAction',
   'data': {
-       'blockdev-snapshot-sync': 'BlockdevSnapshot'
+       'blockdev-snapshot-sync': 'BlockdevSnapshot',
+       'drive-backup': 'DriveBackup'
    } }
 
 ##
-- 
1.8.1.4

[Qemu-devel] [PATCH v4 7/8] blockdev: add Abort transaction

[Stefan Hajnoczi]

The Abort action can be used to test QMP 'transaction' failure.  Add it
as the last action to exercise the .abort() and .cleanup() code paths
for all previous actions.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c       | 15 +++++++++++++++
 qapi-schema.json | 13 ++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index c386bb6..fcce219 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -973,6 +973,16 @@ static void drive_backup_abort(BlkTransactionState *common)
     }
 }
 
+static void abort_prepare(BlkTransactionState *common, Error **errp)
+{
+    error_setg(errp, "Transaction aborted using Abort action");
+}
+
+static void abort_commit(BlkTransactionState *common)
+{
+    assert(false); /* this action never succeeds */
+}
+
 static const BdrvActionOps actions[] = {
     [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
         .instance_size = sizeof(ExternalSnapshotState),
@@ -986,6 +996,11 @@ static const BdrvActionOps actions[] = {
         .commit = drive_backup_commit,
         .abort = drive_backup_abort,
     },
+    [TRANSACTION_ACTION_KIND_ABORT] = {
+        .instance_size = sizeof(BlkTransactionState),
+        .prepare = abort_prepare,
+        .commit = abort_commit,
+    },
 };
 
 /*
diff --git a/qapi-schema.json b/qapi-schema.json
index 114ae50..ac7bb0b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1632,6 +1632,16 @@
             '*mode': 'NewImageMode', '*speed': 'int' } }
 
 ##
+# @Abort
+#
+# This action can be used to test transaction failure.
+#
+# Since: 1.6
+###
+{ 'type': 'Abort',
+  'data': { } }
+
+##
 # @TransactionAction
 #
 # A discriminated record of operations that can be performed with
@@ -1640,7 +1650,8 @@
 { 'union': 'TransactionAction',
   'data': {
        'blockdev-snapshot-sync': 'BlockdevSnapshot',
-       'drive-backup': 'DriveBackup'
+       'drive-backup': 'DriveBackup',
+       'abort': 'Abort'
    } }
 
 ##
-- 
1.8.1.4

[Qemu-devel] [PATCH v4 8/8] qemu-iotests: test 'drive-backup' transaction in 055

[Stefan Hajnoczi]

Ensure that the 'drive-backup' transaction works and check that a
transaction abort works properly.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/055     | 118 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/055.out |   4 +-
 2 files changed, 120 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index bc2eebf..d61993f 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -226,5 +226,123 @@ class TestSetSpeed(DriveBackupTestCase):
 
         self.cancel_and_wait()
 
+class TestSingleTransaction(DriveBackupTestCase):
+    image_len = 120 * 1024 * 1024 # MB
+
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestSingleDrive.image_len))
+        self.vm = iotests.VM().add_drive(test_img)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(test_img)
+        try:
+            os.remove(target_img)
+        except OSError:
+            pass
+
+    def test_complete(self):
+        self.assert_no_active_backups()
+
+        result = self.vm.qmp('transaction', actions=[{
+                'type': 'drive-backup',
+                'data': { 'device': 'drive0',
+                          'target': target_img },
+            }
+        ])
+        self.assert_qmp(result, 'return', {})
+
+        self.complete_and_wait()
+        self.vm.shutdown()
+        self.assertTrue(self.compare_images(test_img, target_img),
+                        'target image does not match source after backup')
+
+    def test_cancel(self):
+        self.assert_no_active_backups()
+
+        result = self.vm.qmp('transaction', actions=[{
+                'type': 'drive-backup',
+                'data': { 'device': 'drive0',
+                          'target': target_img },
+            }
+        ])
+        self.assert_qmp(result, 'return', {})
+
+        self.cancel_and_wait()
+
+    def test_pause(self):
+        self.assert_no_active_backups()
+
+        result = self.vm.qmp('transaction', actions=[{
+                'type': 'drive-backup',
+                'data': { 'device': 'drive0',
+                          'target': target_img },
+            }
+        ])
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block-job-pause', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        time.sleep(1)
+        result = self.vm.qmp('query-block-jobs')
+        offset = self.dictpath(result, 'return[0]/offset')
+
+        time.sleep(1)
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return[0]/offset', offset)
+
+        result = self.vm.qmp('block-job-resume', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        self.complete_and_wait()
+        self.vm.shutdown()
+        self.assertTrue(self.compare_images(test_img, target_img),
+                        'target image does not match source after backup')
+
+    def test_medium_not_found(self):
+        result = self.vm.qmp('transaction', actions=[{
+                'type': 'drive-backup',
+                'data': { 'device': 'ide1-cd0',
+                          'target': target_img },
+            }
+        ])
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_image_not_found(self):
+        result = self.vm.qmp('transaction', actions=[{
+                'type': 'drive-backup',
+                'data': { 'device': 'drive0',
+                          'mode': 'existing',
+                          'target': target_img },
+            }
+        ])
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_device_not_found(self):
+        result = self.vm.qmp('transaction', actions=[{
+                'type': 'drive-backup',
+                'data': { 'device': 'nonexistent',
+                          'mode': 'existing',
+                          'target': target_img },
+            }
+        ])
+        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+    def test_abort(self):
+        result = self.vm.qmp('transaction', actions=[{
+                'type': 'drive-backup',
+                'data': { 'device': 'nonexistent',
+                          'mode': 'existing',
+                          'target': target_img },
+            }, {
+                'type': 'Abort',
+                'data': {},
+            }
+        ])
+        self.assert_qmp(result, 'error/class', 'GenericError')
+        self.assert_no_active_backups()
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['raw', 'qcow2'])
diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out
index 594c16f..96961ed 100644
--- a/tests/qemu-iotests/055.out
+++ b/tests/qemu-iotests/055.out
@@ -1,5 +1,5 @@
-........
+...............
 ----------------------------------------------------------------------
-Ran 8 tests
+Ran 15 tests
 
 OK
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v4 3/8] block: add drive-backup QMP command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

On 05/16/2013 02:36 AM, Stefan Hajnoczi wrote:
> @drive-backup
> 
> Start a point-in-time copy of a block device to a new destination.  The
> status of ongoing drive-backup operations can be checked with
> query-block-jobs where the BlockJobInfo.type field has the value 'backup'.
> The operation can be stopped before it has completed using the
> block-job-cancel command.
> 
> @device: the name of the device whose writes should be mirrored.
> 
> @target: the target of the new image. If the file exists, or if it
>          is a device, the existing file/device will be used as the new
>          destination.  If it does not exist, a new file will be created.
> 
> @format: #optional the format of the new destination, default is to
>          probe if @mode is 'existing', else the format of the source
> 
> @mode: #optional whether and how QEMU should create a new image, default is
>        'absolute-paths'.
> 
> @speed: #optional the maximum speed, in bytes per second
> 
> Returns: nothing on success
>          If @device is not a valid block device, DeviceNotFound
> 
> Since 1.6
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  blockdev.c       | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json | 32 ++++++++++++++++++++
>  qmp-commands.hx  | 37 +++++++++++++++++++++++
>  3 files changed, 161 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v4 6/8] blockdev: add DriveBackup transaction

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2448 bytes --]

On 05/16/2013 02:36 AM, Stefan Hajnoczi wrote:
> This patch adds a transactional version of the drive-backup QMP command.
> It allows atomic snapshots of multiple drives along with automatic
> cleanup if there is a failure to start one of the backup jobs.
> 
> Note that QMP events are emitted for block job completion/cancellation
> and the block job will be listed by query-block-jobs.
> 
> @DriveBackup
> 
> @device: the name of the device whose writes should be mirrored.
> 
> @target: the target of the new image. If the file exists, or if it
>          is a device, the existing file/device will be used as the new
>          destination.  If it does not exist, a new file will be created.
> 
> @format: #optional the format of the new destination, default is to
>          probe if @mode is 'existing', else the format of the source
> 
> @mode: #optional whether and how QEMU should create a new image, default is
>        'absolute-paths'.
> 
> @speed: #optional the maximum speed, in bytes per second
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  blockdev.c       | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json | 26 +++++++++++++++++++++++++-
>  2 files changed, 78 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

Hmm, I wonder if we can simplify patch 3/8, by hoisting the DriveBackup
definition into that patch, and writing:

{ 'command': 'drive-backup',
  'data': 'DriveBackup' }

instead of the current open-coding repetition of the arguments for the
standalone command in comparison to the transaction action.  So far, all
uses of { 'command':'str', 'data':{...} } in the qapi-schema.json use a
direct object instead of a named type, but if we could fix the qapi code
generation to honor dictionary types in place of an open-coded type, it
might make our interface file more compact.

> +
> +static void drive_backup_commit(BlkTransactionState *common)
> +{
> +    /* Block job has started, nothing to do here */
> +}

Given this implementation, should we modify the extensible transaction
series to allow for a NULL commit callback, and merely insist only that
at least one of commit/abort is non-NULL (rather than the current
insistence that commit is mandatory and abort is optional)?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v4 7/8] blockdev: add Abort transaction

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 664 bytes --]

On 05/16/2013 02:36 AM, Stefan Hajnoczi wrote:
> The Abort action can be used to test QMP 'transaction' failure.  Add it
> as the last action to exercise the .abort() and .cleanup() code paths
> for all previous actions.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  blockdev.c       | 15 +++++++++++++++
>  qapi-schema.json | 13 ++++++++++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)

This looks unchanged from v3, which means you can carry forward my earlier:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v4 6/8] blockdev: add DriveBackup transaction

[Stefan Hajnoczi]

On Thu, May 16, 2013 at 01:21:19PM -0600, Eric Blake wrote:
> On 05/16/2013 02:36 AM, Stefan Hajnoczi wrote:
> > This patch adds a transactional version of the drive-backup QMP command.
> > It allows atomic snapshots of multiple drives along with automatic
> > cleanup if there is a failure to start one of the backup jobs.
> > 
> > Note that QMP events are emitted for block job completion/cancellation
> > and the block job will be listed by query-block-jobs.
> > 
> > @DriveBackup
> > 
> > @device: the name of the device whose writes should be mirrored.
> > 
> > @target: the target of the new image. If the file exists, or if it
> >          is a device, the existing file/device will be used as the new
> >          destination.  If it does not exist, a new file will be created.
> > 
> > @format: #optional the format of the new destination, default is to
> >          probe if @mode is 'existing', else the format of the source
> > 
> > @mode: #optional whether and how QEMU should create a new image, default is
> >        'absolute-paths'.
> > 
> > @speed: #optional the maximum speed, in bytes per second
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  blockdev.c       | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qapi-schema.json | 26 +++++++++++++++++++++++++-
> >  2 files changed, 78 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Hmm, I wonder if we can simplify patch 3/8, by hoisting the DriveBackup
> definition into that patch, and writing:
> 
> { 'command': 'drive-backup',
>   'data': 'DriveBackup' }
> 
> instead of the current open-coding repetition of the arguments for the
> standalone command in comparison to the transaction action.  So far, all
> uses of { 'command':'str', 'data':{...} } in the qapi-schema.json use a
> direct object instead of a named type, but if we could fix the qapi code
> generation to honor dictionary types in place of an open-coded type, it
> might make our interface file more compact.

That would be nice.  The only thing to watch out for is a situation
where the transaction takes additional arguments that the regular
command does not.  But in most cases that would not be necessary.

> > +
> > +static void drive_backup_commit(BlkTransactionState *common)
> > +{
> > +    /* Block job has started, nothing to do here */
> > +}
> 
> Given this implementation, should we modify the extensible transaction
> series to allow for a NULL commit callback, and merely insist only that
> at least one of commit/abort is non-NULL (rather than the current
> insistence that commit is mandatory and abort is optional)?

If I need to respin I'll do that.

Re: [Qemu-devel] [PATCH v4 0/8] block: drive-backup live backup command

[Richard W.M. Jones]

On Thu, May 16, 2013 at 10:36:11AM +0200, Stefan Hajnoczi wrote:
[...]
> How can drive-backup be used?
> -----------------------------
> The simplest use-case is to copy a point-in-time snapshot to a local file.
> 
> More advanced users may wish to make the target an NBD URL.  The NBD server
> listening on the other side can process the backup writes any way it wishes.  I
> previously posted an RFC series with a backup server that streamed Dietmar's
> VMA backup archive format.

I'm guessing the answer is no, but thought I would ask:  Is there
any way to use this to do point-in-time inspection of guests?
AFAICT the only way to do it would be to make a complete copy of a
guest disk in another file.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Re: [Qemu-devel] [PATCH v4 0/8] block: drive-backup live backup command

[Paolo Bonzini]

Il 19/05/2013 18:47, Richard W.M. Jones ha scritto:
>> > 
>> > More advanced users may wish to make the target an NBD URL.  The NBD server
>> > listening on the other side can process the backup writes any way it wishes.  I
>> > previously posted an RFC series with a backup server that streamed Dietmar's
>> > VMA backup archive format.
> I'm guessing the answer is no, but thought I would ask:  Is there
> any way to use this to do point-in-time inspection of guests?

Almost, and even for an atomic copy of multiple disks.  All that is left
is to create the copy with the disk as a backing file, and switch the
backing file at the end of the copy.  It is a very simple change on top
of these patches.

Paolo

> AFAICT the only way to do it would be to make a complete copy of a
> guest disk in another file.

Re: [Qemu-devel] [PATCH v4 0/8] block: drive-backup live backup command

[Richard W.M. Jones]

On Sun, May 19, 2013 at 09:30:46PM +0200, Paolo Bonzini wrote:
> Il 19/05/2013 18:47, Richard W.M. Jones ha scritto:
> >> > 
> >> > More advanced users may wish to make the target an NBD URL.  The NBD server
> >> > listening on the other side can process the backup writes any way it wishes.  I
> >> > previously posted an RFC series with a backup server that streamed Dietmar's
> >> > VMA backup archive format.
> > I'm guessing the answer is no, but thought I would ask:  Is there
> > any way to use this to do point-in-time inspection of guests?
> 
> Almost, and even for an atomic copy of multiple disks.  All that is left
> is to create the copy with the disk as a backing file, and switch the
> backing file at the end of the copy.  It is a very simple change on top
> of these patches.

Cool news.  I look forward to this :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Re: [Qemu-devel] [PATCH v4 2/8] block: add basic backup support to block driver

[Kevin Wolf]

Am 16.05.2013 um 10:36 hat Stefan Hajnoczi geschrieben:
> From: Dietmar Maurer <dietmar@proxmox.com>
> 
> backup_start() creates a block job that copies a point-in-time snapshot
> of a block device to a target block device.
> 
> We call backup_do_cow() for each write during backup. That function
> reads the original data from the block device before it gets
> overwritten.  The data is then written to the target device.
> 
> Currently backup cluster size is hardcoded to 65536 bytes.
> 
> [I made a number of changes to Dietmar's original patch and folded them
> in to make code review easy.  Here is the full list:
> 
>  * Drop BackupDumpFunc interface in favor of a target block device
>  * Detect zero clusters with buffer_is_zero()
>  * Don't write zero clusters to the target
>  * Use 0 delay instead of 1us, like other block jobs
>  * Unify creation/start functions into backup_start()
>  * Simplify cleanup, free bitmap in backup_run() instead of cb
>  * function
>  * Use HBitmap to avoid duplicating bitmap code
>  * Use bdrv_getlength() instead of accessing ->total_sectors
>  * directly
>  * Delete the backup.h header file, it is no longer necessary
>  * Move ./backup.c to block/backup.c
>  * Remove #ifdefed out code
>  * Coding style and whitespace cleanups
>  * Use bdrv_add_before_write_notifier() instead of blockjob-specific hooks
>  * Keep our own in-flight CowRequest list instead of using block.c
>    tracked requests.  This means a little code duplication but is much
>    simpler than trying to share the tracked requests list and use the
>    backup block size.
> 
> -- stefanha]
> 
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/Makefile.objs       |   1 +
>  block/backup.c            | 283 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block_int.h |  16 +++
>  3 files changed, 300 insertions(+)
>  create mode 100644 block/backup.c
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 5f0358a..88bd101 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -20,5 +20,6 @@ endif
>  common-obj-y += stream.o
>  common-obj-y += commit.o
>  common-obj-y += mirror.o
> +common-obj-y += backup.o
>  
>  $(obj)/curl.o: QEMU_CFLAGS+=$(CURL_CFLAGS)
> diff --git a/block/backup.c b/block/backup.c
> new file mode 100644
> index 0000000..5438e26
> --- /dev/null
> +++ b/block/backup.c
> @@ -0,0 +1,283 @@
> +/*
> + * QEMU backup
> + *
> + * Copyright (C) 2013 Proxmox Server Solutions
> + *
> + * Authors:
> + *  Dietmar Maurer (dietmar@proxmox.com)
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <stdio.h>
> +#include <errno.h>
> +#include <unistd.h>
> +
> +#include "block/block.h"
> +#include "block/block_int.h"
> +#include "block/blockjob.h"
> +#include "qemu/ratelimit.h"
> +
> +#define DEBUG_BACKUP 0
> +
> +#define DPRINTF(fmt, ...) \
> +    do { \
> +        if (DEBUG_BACKUP) { \
> +            fprintf(stderr, "backup: " fmt, ## __VA_ARGS__); \
> +        } \
> +    } while (0)
> +
> +#define BACKUP_CLUSTER_BITS 16
> +#define BACKUP_CLUSTER_SIZE (1 << BACKUP_CLUSTER_BITS)
> +#define BACKUP_BLOCKS_PER_CLUSTER (BACKUP_CLUSTER_SIZE / BDRV_SECTOR_SIZE)

BACKUP_SECTORS_PER_CLUSTER for more consistent naming?

> +
> +#define SLICE_TIME 100000000ULL /* ns */
> +
> +typedef struct CowRequest {
> +    int64_t start;
> +    int64_t end;
> +    QLIST_ENTRY(CowRequest) list;
> +    CoQueue wait_queue; /* coroutines blocked on this request */
> +} CowRequest;
> +
> +typedef struct BackupBlockJob {
> +    BlockJob common;
> +    BlockDriverState *target;
> +    RateLimit limit;
> +    CoRwlock flush_rwlock;
> +    uint64_t sectors_read;
> +    HBitmap *bitmap;
> +    QLIST_HEAD(, CowRequest) inflight_reqs;
> +} BackupBlockJob;
> +
> +/* See if in-flight requests overlap and wait for them to complete */
> +static void coroutine_fn wait_for_overlapping_requests(BackupBlockJob *job,
> +                                                       int64_t start,
> +                                                       int64_t end)
> +{
> +    CowRequest *req;
> +    bool retry;
> +
> +    do {
> +        retry = false;
> +        QLIST_FOREACH(req, &job->inflight_reqs, list) {
> +            if (end > req->start && start < req->end) {
> +                qemu_co_queue_wait(&req->wait_queue);
> +                retry = true;
> +                break;
> +            }
> +        }
> +    } while (retry);
> +}
> +
> +/* Keep track of an in-flight request */
> +static void cow_request_begin(CowRequest *req, BackupBlockJob *job,
> +                                     int64_t start, int64_t end)
> +{
> +    req->start = start;
> +    req->end = end;
> +    qemu_co_queue_init(&req->wait_queue);
> +    QLIST_INSERT_HEAD(&job->inflight_reqs, req, list);
> +}
> +
> +/* Forget about a completed request */
> +static void cow_request_end(CowRequest *req)
> +{
> +    QLIST_REMOVE(req, list);
> +    qemu_co_queue_restart_all(&req->wait_queue);
> +}
> +
> +static int coroutine_fn backup_do_cow(BlockDriverState *bs,
> +                                      int64_t sector_num, int nb_sectors)
> +{
> +    BackupBlockJob *job = (BackupBlockJob *)bs->job;
> +    CowRequest cow_request;
> +    struct iovec iov;
> +    QEMUIOVector bounce_qiov;
> +    void *bounce_buffer = NULL;
> +    int ret = 0;
> +    int64_t start, end;
> +
> +    qemu_co_rwlock_rdlock(&job->flush_rwlock);
> +
> +    start = sector_num / BACKUP_BLOCKS_PER_CLUSTER;
> +    end = DIV_ROUND_UP(sector_num + nb_sectors, BACKUP_BLOCKS_PER_CLUSTER);
> +
> +    DPRINTF("brdv_co_backup_cow enter %s C%" PRId64 " %" PRId64 " %d\n",
> +            bdrv_get_device_name(bs), start, sector_num, nb_sectors);
> +
> +    wait_for_overlapping_requests(job, start, end);
> +    cow_request_begin(&cow_request, job, start, end);
> +
> +    for (; start < end; start++) {
> +        if (hbitmap_get(job->bitmap, start)) {
> +            DPRINTF("brdv_co_backup_cow skip C%" PRId64 "\n", start);
> +            continue; /* already copied */
> +        }
> +
> +        /* immediately set bitmap (avoid coroutine race) */
> +        hbitmap_set(job->bitmap, start, 1);

Hm, what kind of race? Doesn't wait_for_overlapping_requests() already
serialise everything so that it doesn't matter where exactly the bit is
set, as long as it's between cow_request_begin/end?

> +
> +        DPRINTF("brdv_co_backup_cow C%" PRId64 "\n", start);
> +
> +        if (!bounce_buffer) {
> +            iov.iov_len = BACKUP_CLUSTER_SIZE;
> +            iov.iov_base = bounce_buffer = qemu_blockalign(bs, iov.iov_len);
> +            qemu_iovec_init_external(&bounce_qiov, &iov, 1);
> +        }
> +
> +        ret = bdrv_co_readv(bs, start * BACKUP_BLOCKS_PER_CLUSTER,
> +                            BACKUP_BLOCKS_PER_CLUSTER,
> +                            &bounce_qiov);
> +        if (ret < 0) {
> +            DPRINTF("brdv_co_backup_cow bdrv_read C%" PRId64 " failed\n",
> +                    start);
> +            goto out;
> +        }
> +
> +        job->sectors_read += BACKUP_BLOCKS_PER_CLUSTER;
> +
> +        if (!buffer_is_zero(bounce_buffer, BACKUP_CLUSTER_SIZE)) {
> +            ret = bdrv_co_writev(job->target, start * BACKUP_BLOCKS_PER_CLUSTER,
> +                                 BACKUP_BLOCKS_PER_CLUSTER,
> +                                 &bounce_qiov);
> +            if (ret < 0) {
> +                DPRINTF("brdv_co_backup_cow dump_cluster_cb C%" PRId64
> +                        " failed\n", start);
> +                goto out;
> +            }
> +        }

This series seems to only allow standalone target images without a
backing file, so this is okay. But we've been talking about use cases
like using the original image as a backing file and exposing the backup
via NBD in order to provide consistent inspection. Then we can't simply
ignore all-zero clusters.

Maybe we should use bdrv_co_write_zeroes() from the beginning?

> +
> +        DPRINTF("brdv_co_backup_cow done C%" PRId64 "\n", start);
> +    }
> +
> +out:
> +    if (bounce_buffer) {
> +        qemu_vfree(bounce_buffer);
> +    }
> +
> +    cow_request_end(&cow_request);
> +
> +    qemu_co_rwlock_unlock(&job->flush_rwlock);
> +
> +    return ret;
> +}
> +
> +static void coroutine_fn backup_before_write_notify(Notifier *notifier,
> +                                                    void *opaque)
> +{
> +    BdrvTrackedRequest *req = opaque;
> +    backup_do_cow(req->bs, req->sector_num, req->nb_sectors);
> +}

I don't think you can ignore errors here. Not sure if we can stop the VM
and resume later or something like that, but if we can't, the backup
will be invalid and we must fail the job.

Kevin

Re: [Qemu-devel] [PATCH v4 3/8] block: add drive-backup QMP command

[Kevin Wolf]

Am 16.05.2013 um 10:36 hat Stefan Hajnoczi geschrieben:
> @drive-backup
> 
> Start a point-in-time copy of a block device to a new destination.  The
> status of ongoing drive-backup operations can be checked with
> query-block-jobs where the BlockJobInfo.type field has the value 'backup'.
> The operation can be stopped before it has completed using the
> block-job-cancel command.
> 
> @device: the name of the device whose writes should be mirrored.
> 
> @target: the target of the new image. If the file exists, or if it
>          is a device, the existing file/device will be used as the new
>          destination.  If it does not exist, a new file will be created.
> 
> @format: #optional the format of the new destination, default is to
>          probe if @mode is 'existing', else the format of the source
> 
> @mode: #optional whether and how QEMU should create a new image, default is
>        'absolute-paths'.
> 
> @speed: #optional the maximum speed, in bytes per second
> 
> Returns: nothing on success
>          If @device is not a valid block device, DeviceNotFound
> 
> Since 1.6
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  blockdev.c       | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json | 32 ++++++++++++++++++++
>  qmp-commands.hx  | 37 +++++++++++++++++++++++
>  3 files changed, 161 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index d1ec99a..c7a5e1e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1353,6 +1353,98 @@ void qmp_block_commit(const char *device,
>      drive_get_ref(drive_get_by_blockdev(bs));
>  }
>  
> +void qmp_drive_backup(const char *device, const char *target,
> +                      bool has_format, const char *format,
> +                      bool has_mode, enum NewImageMode mode,
> +                      bool has_speed, int64_t speed,
> +                      Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BlockDriverState *target_bs;
> +    BlockDriver *proto_drv;
> +    BlockDriver *drv = NULL;
> +    Error *local_err = NULL;
> +    int flags;
> +    uint64_t size;
> +    int ret;
> +
> +    if (!has_speed) {
> +        speed = 0;
> +    }
> +    if (!has_mode) {
> +        mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> +    }
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (!bdrv_is_inserted(bs)) {
> +        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> +        return;
> +    }
> +
> +    if (!has_format) {
> +        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
> +    }
> +    if (format) {
> +        drv = bdrv_find_format(format);
> +        if (!drv) {
> +            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> +            return;
> +        }
> +    }
> +
> +    if (bdrv_in_use(bs)) {
> +        error_set(errp, QERR_DEVICE_IN_USE, device);
> +        return;
> +    }
> +
> +    flags = bs->open_flags | BDRV_O_RDWR;
> +
> +    proto_drv = bdrv_find_protocol(target);
> +    if (!proto_drv) {
> +        error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> +        return;
> +    }

I see that other block job commands are doing the same, but there are
two things unclear for me about this:

1. Shouldn't bdrv_img_create() and/or bdrv_open() already return an error
   if the protocol can't be found? The error check is the only thing
   that proto_drv is used for.

2. Why do we complain about a wrong _format_ here? If I ask for a qcow2
   image called foo:bar.qcow2, qemu won't find the protocol "foo" and
   complain that qcow2 is an invalid format.

> +
> +    bdrv_get_geometry(bs, &size);
> +    size *= 512;

I was going to suggest BDRV_SECTOR_SIZE at first, but...

Why not use bdrv_getlength() in the first place if you need it in bytes
anyway? As a nice bonus you would get -errno instead of size 0 on
failure.

> +    if (mode != NEW_IMAGE_MODE_EXISTING) {
> +        assert(format && drv);
> +        bdrv_img_create(target, format,
> +                        NULL, NULL, NULL, size, flags, &local_err, false);
> +    }
> +
> +    if (error_is_set(&local_err)) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    target_bs = bdrv_new("");
> +    ret = bdrv_open(target_bs, target, NULL, flags, drv);
> +
> +    if (ret < 0) {

Why the empty line between bdrv_open and checking its return value?

> +        bdrv_delete(target_bs);
> +        error_set(errp, QERR_OPEN_FILE_FAILED, target);
> +        return;
> +    }
> +
> +    backup_start(bs, target_bs, speed, block_job_cb, bs, &local_err);
> +    if (local_err != NULL) {
> +        bdrv_delete(target_bs);
> +        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));
> +}
> +

Kevin

Re: [Qemu-devel] [PATCH v4 2/8] block: add basic backup support to block driver

[Paolo Bonzini]

Il 22/05/2013 11:38, Kevin Wolf ha scritto:
>> +
>> +        DPRINTF("brdv_co_backup_cow done C%" PRId64 "\n", start);
>> +    }
>> +
>> +out:
>> +    if (bounce_buffer) {
>> +        qemu_vfree(bounce_buffer);
>> +    }
>> +
>> +    cow_request_end(&cow_request);
>> +
>> +    qemu_co_rwlock_unlock(&job->flush_rwlock);
>> +
>> +    return ret;
>> +}
>> +
>> +static void coroutine_fn backup_before_write_notify(Notifier *notifier,
>> +                                                    void *opaque)
>> +{
>> +    BdrvTrackedRequest *req = opaque;
>> +    backup_do_cow(req->bs, req->sector_num, req->nb_sectors);
>> +}
> 
> I don't think you can ignore errors here. Not sure if we can stop the VM
> and resume later or something like that, but if we can't, the backup
> will be invalid and we must fail the job.

Yes, there is rerror/werror machinery for jobs that this patch is not using.

Paolo

Re: [Qemu-devel] [PATCH v4 2/8] block: add basic backup support to block driver

[Kevin Wolf]

Am 22.05.2013 um 11:54 hat Paolo Bonzini geschrieben:
> Il 22/05/2013 11:38, Kevin Wolf ha scritto:
> >> +
> >> +        DPRINTF("brdv_co_backup_cow done C%" PRId64 "\n", start);
> >> +    }
> >> +
> >> +out:
> >> +    if (bounce_buffer) {
> >> +        qemu_vfree(bounce_buffer);
> >> +    }
> >> +
> >> +    cow_request_end(&cow_request);
> >> +
> >> +    qemu_co_rwlock_unlock(&job->flush_rwlock);
> >> +
> >> +    return ret;
> >> +}
> >> +
> >> +static void coroutine_fn backup_before_write_notify(Notifier *notifier,
> >> +                                                    void *opaque)
> >> +{
> >> +    BdrvTrackedRequest *req = opaque;
> >> +    backup_do_cow(req->bs, req->sector_num, req->nb_sectors);
> >> +}
> > 
> > I don't think you can ignore errors here. Not sure if we can stop the VM
> > and resume later or something like that, but if we can't, the backup
> > will be invalid and we must fail the job.
> 
> Yes, there is rerror/werror machinery for jobs that this patch is not using.

This is not enough here. The guest write can't continue before the old
content is saved to the backup image.

Kevin

Re: [Qemu-devel] [PATCH v4 4/8] qemu-iotests: add 055 drive-backup test case

[Kevin Wolf]

Am 16.05.2013 um 10:36 hat Stefan Hajnoczi geschrieben:
> Testing drive-backup is similar to image streaming and drive mirroring.
> This test case is based on 041.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/055     | 230 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/055.out |   5 +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 236 insertions(+)
>  create mode 100755 tests/qemu-iotests/055
>  create mode 100644 tests/qemu-iotests/055.out
> 
> diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
> new file mode 100755
> index 0000000..bc2eebf
> --- /dev/null
> +++ b/tests/qemu-iotests/055
> @@ -0,0 +1,230 @@
> +#!/usr/bin/env python
> +#
> +# Tests for drive-backup
> +#
> +# Copyright (C) 2013 Red Hat, Inc.
> +#
> +# Based on 041.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import time
> +import os
> +import iotests
> +from iotests import qemu_img, qemu_io
> +
> +test_img = os.path.join(iotests.test_dir, 'test.img')
> +target_img = os.path.join(iotests.test_dir, 'target.img')
> +
> +class DriveBackupTestCase(iotests.QMPTestCase):
> +    '''Abstract base class for drive-backup test cases'''
> +
> +    def assert_no_active_backups(self):
> +        result = self.vm.qmp('query-block-jobs')
> +        self.assert_qmp(result, 'return', [])
> +
> +    def cancel_and_wait(self, drive='drive0'):
> +        '''Cancel a block job and wait for it to finish'''
> +        result = self.vm.qmp('block-job-cancel', device=drive)
> +        self.assert_qmp(result, 'return', {})
> +
> +        cancelled = False
> +        while not cancelled:
> +            for event in self.vm.get_qmp_events(wait=True):
> +                if event['event'] == 'BLOCK_JOB_COMPLETED' or \
> +                   event['event'] == 'BLOCK_JOB_CANCELLED':
> +                    self.assert_qmp(event, 'data/type', 'backup')
> +                    self.assert_qmp(event, 'data/device', drive)
> +                    cancelled = True
> +
> +        self.assert_no_active_backups()
> +
> +    def complete_and_wait(self):
> +        completed = False
> +        while not completed:
> +            for event in self.vm.get_qmp_events(wait=True):
> +                if event['event'] == 'BLOCK_JOB_COMPLETED':
> +                    self.assert_qmp(event, 'data/type', 'backup')
> +                    self.assert_qmp(event, 'data/device', 'drive0')
> +                    self.assert_qmp(event, 'data/offset', self.image_len)
> +                    self.assert_qmp(event, 'data/len', self.image_len)
> +                    completed = True
> +        self.assert_no_active_backups()
> +
> +    def compare_images(self, img1, img2):
> +        try:
> +            qemu_img('convert', '-f', iotests.imgfmt, '-O', 'raw', img1, img1 + '.raw')
> +            qemu_img('convert', '-f', iotests.imgfmt, '-O', 'raw', img2, img2 + '.raw')
> +            file1 = open(img1 + '.raw', 'r')
> +            file2 = open(img2 + '.raw', 'r')
> +            return file1.read() == file2.read()
> +        finally:
> +            if file1 is not None:
> +                file1.close()
> +            if file2 is not None:
> +                file2.close()
> +            try:
> +                os.remove(img1 + '.raw')
> +            except OSError:
> +                pass
> +            try:
> +                os.remove(img2 + '.raw')
> +            except OSError:
> +                pass

compare_images() is an exact copy of its 041 counterparts,
complete_and_wait() and cancel_and_wait() are the same as in 041 in the
wait_ready = False case. Sounds like candidates for factoring out.

Looks good otherwise.

Kevin

Re: [Qemu-devel] [PATCH v4 0/8] block: drive-backup live backup command

[Kevin Wolf]

Am 16.05.2013 um 10:36 hat Stefan Hajnoczi geschrieben:
> Note: These patches apply to my block-next tree.  You can also grab the code
> from git here:
> git://github.com/stefanha/qemu.git block-backup-core
> 
> This series adds a new QMP command, drive-backup, which takes a point-in-time
> snapshot of a block device.  The snapshot is copied out to a target block
> device.  A simple example is:
> 
>   drive-backup device=virtio0 format=qcow2 target=backup-20130401.qcow2

> Dietmar Maurer (1):
>   block: add basic backup support to block driver
> 
> Stefan Hajnoczi (7):
>   block: add bdrv_add_before_write_notifier()
>   block: add drive-backup QMP command
>   qemu-iotests: add 055 drive-backup test case
>   blockdev: rename BlkTransactionStates to singular
>   blockdev: add DriveBackup transaction
>   blockdev: add Abort transaction
>   qemu-iotests: test 'drive-backup' transaction in 055
> 
>  block.c                    |  18 ++-
>  block/Makefile.objs        |   1 +
>  block/backup.c             | 283 ++++++++++++++++++++++++++++++++++++
>  blockdev.c                 | 264 +++++++++++++++++++++++++++-------
>  include/block/block_int.h  |  38 ++++-
>  qapi-schema.json           |  69 ++++++++-
>  qmp-commands.hx            |  37 +++++
>  tests/qemu-iotests/055     | 348 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/055.out |   5 +
>  tests/qemu-iotests/group   |   1 +
>  10 files changed, 1000 insertions(+), 64 deletions(-)
>  create mode 100644 block/backup.c
>  create mode 100755 tests/qemu-iotests/055
>  create mode 100644 tests/qemu-iotests/055.out

Commented on patches 2, 3 and 4. The others are:

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

Re: [Qemu-devel] [PATCH v4 2/8] block: add basic backup support to block driver

[Stefan Hajnoczi]

On Wed, May 22, 2013 at 11:56:45AM +0200, Kevin Wolf wrote:
> Am 22.05.2013 um 11:54 hat Paolo Bonzini geschrieben:
> > Il 22/05/2013 11:38, Kevin Wolf ha scritto:
> > >> +
> > >> +        DPRINTF("brdv_co_backup_cow done C%" PRId64 "\n", start);
> > >> +    }
> > >> +
> > >> +out:
> > >> +    if (bounce_buffer) {
> > >> +        qemu_vfree(bounce_buffer);
> > >> +    }
> > >> +
> > >> +    cow_request_end(&cow_request);
> > >> +
> > >> +    qemu_co_rwlock_unlock(&job->flush_rwlock);
> > >> +
> > >> +    return ret;
> > >> +}
> > >> +
> > >> +static void coroutine_fn backup_before_write_notify(Notifier *notifier,
> > >> +                                                    void *opaque)
> > >> +{
> > >> +    BdrvTrackedRequest *req = opaque;
> > >> +    backup_do_cow(req->bs, req->sector_num, req->nb_sectors);
> > >> +}
> > > 
> > > I don't think you can ignore errors here. Not sure if we can stop the VM
> > > and resume later or something like that, but if we can't, the backup
> > > will be invalid and we must fail the job.
> > 
> > Yes, there is rerror/werror machinery for jobs that this patch is not using.
> 
> This is not enough here. The guest write can't continue before the old
> content is saved to the backup image.

Are you saying just the vm_stop(RUN_STATE_IO_ERROR) call is missing?

I think the reason it's not there is because there's an assumption that
block jobs are in the background and do not affect the guest.  The guest
continues running while the block job is in an error state.

But perhaps we can make the vm_stop() call optional so that drive-backup
can use it.

Stefan

Re: [Qemu-devel] [PATCH v4 2/8] block: add basic backup support to block driver

[Kevin Wolf]

Am 22.05.2013 um 15:58 hat Stefan Hajnoczi geschrieben:
> On Wed, May 22, 2013 at 11:56:45AM +0200, Kevin Wolf wrote:
> > Am 22.05.2013 um 11:54 hat Paolo Bonzini geschrieben:
> > > Il 22/05/2013 11:38, Kevin Wolf ha scritto:
> > > >> +
> > > >> +        DPRINTF("brdv_co_backup_cow done C%" PRId64 "\n", start);
> > > >> +    }
> > > >> +
> > > >> +out:
> > > >> +    if (bounce_buffer) {
> > > >> +        qemu_vfree(bounce_buffer);
> > > >> +    }
> > > >> +
> > > >> +    cow_request_end(&cow_request);
> > > >> +
> > > >> +    qemu_co_rwlock_unlock(&job->flush_rwlock);
> > > >> +
> > > >> +    return ret;
> > > >> +}
> > > >> +
> > > >> +static void coroutine_fn backup_before_write_notify(Notifier *notifier,
> > > >> +                                                    void *opaque)
> > > >> +{
> > > >> +    BdrvTrackedRequest *req = opaque;
> > > >> +    backup_do_cow(req->bs, req->sector_num, req->nb_sectors);
> > > >> +}
> > > > 
> > > > I don't think you can ignore errors here. Not sure if we can stop the VM
> > > > and resume later or something like that, but if we can't, the backup
> > > > will be invalid and we must fail the job.
> > > 
> > > Yes, there is rerror/werror machinery for jobs that this patch is not using.
> > 
> > This is not enough here. The guest write can't continue before the old
> > content is saved to the backup image.
> 
> Are you saying just the vm_stop(RUN_STATE_IO_ERROR) call is missing?

No. Stopping the VM and on 'cont' assuming that the request was
successful (which it wasn't) would be wrong as well. You need the full
failed request handling, with restarting the request from the device
emulation and everything.

> I think the reason it's not there is because there's an assumption that
> block jobs are in the background and do not affect the guest.  The guest
> continues running while the block job is in an error state.

But this is _not_ a background job. This is the write request hook, it
is active on the guest write path. If you can't backup a given sector,
you can't overwrite it and must either fail the write request or the
backup job. Failing the write request is probably nicer because you get
the usual werror handling then, but it means that your assumption
doesn't hold true.

(See, this is one of the reasons why I was for a BlockDriver instead of
notifiers into block jobs. Things would be clearer that way because the
control flow would be explicit in the filter code.)

> But perhaps we can make the vm_stop() call optional so that drive-backup
> can use it.

Not sure what you're trying to do here.

Kevin

Re: [Qemu-devel] [PATCH v4 3/8] block: add drive-backup QMP command

[Stefan Hajnoczi]

On Wed, May 22, 2013 at 11:53:44AM +0200, Kevin Wolf wrote:
> Am 16.05.2013 um 10:36 hat Stefan Hajnoczi geschrieben:
> > +    proto_drv = bdrv_find_protocol(target);
> > +    if (!proto_drv) {
> > +        error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> > +        return;
> > +    }
> 
> I see that other block job commands are doing the same, but there are
> two things unclear for me about this:
> 
> 1. Shouldn't bdrv_img_create() and/or bdrv_open() already return an error
>    if the protocol can't be found? The error check is the only thing
>    that proto_drv is used for.
> 
> 2. Why do we complain about a wrong _format_ here? If I ask for a qcow2
>    image called foo:bar.qcow2, qemu won't find the protocol "foo" and
>    complain that qcow2 is an invalid format.

You are right.  It doesn't seem necessary, we'll get an error message
later from bdrv_open() or bdrv_img_create().

Will drop in the next revision.

> > +
> > +    bdrv_get_geometry(bs, &size);
> > +    size *= 512;
> 
> I was going to suggest BDRV_SECTOR_SIZE at first, but...
> 
> Why not use bdrv_getlength() in the first place if you need it in bytes
> anyway? As a nice bonus you would get -errno instead of size 0 on
> failure.

Yes.  Will fix and update qmp_drive_mirror() too.

> > +    if (mode != NEW_IMAGE_MODE_EXISTING) {
> > +        assert(format && drv);
> > +        bdrv_img_create(target, format,
> > +                        NULL, NULL, NULL, size, flags, &local_err, false);
> > +    }
> > +
> > +    if (error_is_set(&local_err)) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    target_bs = bdrv_new("");
> > +    ret = bdrv_open(target_bs, target, NULL, flags, drv);
> > +
> > +    if (ret < 0) {
> 
> Why the empty line between bdrv_open and checking its return value?

A dramatic pause, to build up suspense :).

Will drop in the next revision and update qmp_drive_mirror() too.

Re: [Qemu-devel] [PATCH v4 4/8] qemu-iotests: add 055 drive-backup test case

[Stefan Hajnoczi]

On Wed, May 22, 2013 at 01:19:58PM +0200, Kevin Wolf wrote:
> Am 16.05.2013 um 10:36 hat Stefan Hajnoczi geschrieben:
> > Testing drive-backup is similar to image streaming and drive mirroring.
> > This test case is based on 041.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  tests/qemu-iotests/055     | 230 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/055.out |   5 +
> >  tests/qemu-iotests/group   |   1 +
> >  3 files changed, 236 insertions(+)
> >  create mode 100755 tests/qemu-iotests/055
> >  create mode 100644 tests/qemu-iotests/055.out
> > 
> > diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
> > new file mode 100755
> > index 0000000..bc2eebf
> > --- /dev/null
> > +++ b/tests/qemu-iotests/055
> > @@ -0,0 +1,230 @@
> > +#!/usr/bin/env python
> > +#
> > +# Tests for drive-backup
> > +#
> > +# Copyright (C) 2013 Red Hat, Inc.
> > +#
> > +# Based on 041.
> > +#
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +#
> > +
> > +import time
> > +import os
> > +import iotests
> > +from iotests import qemu_img, qemu_io
> > +
> > +test_img = os.path.join(iotests.test_dir, 'test.img')
> > +target_img = os.path.join(iotests.test_dir, 'target.img')
> > +
> > +class DriveBackupTestCase(iotests.QMPTestCase):
> > +    '''Abstract base class for drive-backup test cases'''
> > +
> > +    def assert_no_active_backups(self):
> > +        result = self.vm.qmp('query-block-jobs')
> > +        self.assert_qmp(result, 'return', [])
> > +
> > +    def cancel_and_wait(self, drive='drive0'):
> > +        '''Cancel a block job and wait for it to finish'''
> > +        result = self.vm.qmp('block-job-cancel', device=drive)
> > +        self.assert_qmp(result, 'return', {})
> > +
> > +        cancelled = False
> > +        while not cancelled:
> > +            for event in self.vm.get_qmp_events(wait=True):
> > +                if event['event'] == 'BLOCK_JOB_COMPLETED' or \
> > +                   event['event'] == 'BLOCK_JOB_CANCELLED':
> > +                    self.assert_qmp(event, 'data/type', 'backup')
> > +                    self.assert_qmp(event, 'data/device', drive)
> > +                    cancelled = True
> > +
> > +        self.assert_no_active_backups()
> > +
> > +    def complete_and_wait(self):
> > +        completed = False
> > +        while not completed:
> > +            for event in self.vm.get_qmp_events(wait=True):
> > +                if event['event'] == 'BLOCK_JOB_COMPLETED':
> > +                    self.assert_qmp(event, 'data/type', 'backup')
> > +                    self.assert_qmp(event, 'data/device', 'drive0')
> > +                    self.assert_qmp(event, 'data/offset', self.image_len)
> > +                    self.assert_qmp(event, 'data/len', self.image_len)
> > +                    completed = True
> > +        self.assert_no_active_backups()
> > +
> > +    def compare_images(self, img1, img2):
> > +        try:
> > +            qemu_img('convert', '-f', iotests.imgfmt, '-O', 'raw', img1, img1 + '.raw')
> > +            qemu_img('convert', '-f', iotests.imgfmt, '-O', 'raw', img2, img2 + '.raw')
> > +            file1 = open(img1 + '.raw', 'r')
> > +            file2 = open(img2 + '.raw', 'r')
> > +            return file1.read() == file2.read()
> > +        finally:
> > +            if file1 is not None:
> > +                file1.close()
> > +            if file2 is not None:
> > +                file2.close()
> > +            try:
> > +                os.remove(img1 + '.raw')
> > +            except OSError:
> > +                pass
> > +            try:
> > +                os.remove(img2 + '.raw')
> > +            except OSError:
> > +                pass
> 
> compare_images() is an exact copy of its 041 counterparts,
> complete_and_wait() and cancel_and_wait() are the same as in 041 in the
> wait_ready = False case. Sounds like candidates for factoring out.

Will fix.