[Qemu-devel] [PATCH 0/6] qmp: Add blockdev-mirror

[Qemu-devel] [PATCH 0/6] qmp: Add blockdev-mirror

[Fam Zheng]

This is the counterpart of blockdev-backup. The biggest value of this command
is to allow full flexibility on target image open options, via blockdev-add.
For example this could help solve the target provisioning issue in:

http://lists.gnu.org/archive/html/qemu-devel/2015-06/msg02139.html

Because mirror job uses bdrv_swap, this series depends on Max's BlockBackend
series:

    [PATCH v3 00/38] blockdev: BlockBackend and media
    http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg01109.html

which makes it possible for blockdev-add to insert a BDS without BB (by
omitting "id=" while only specifying "node-name=").


Fam Zheng (6):
  block: Add blocker on mirror target
  block: Rename BLOCK_OP_TYPE_MIRROR to BLOCK_OP_TYPE_MIRROR_SOURCE
  block: Extract blockdev part of qmp_drive_mirror
  block: Add check on mirror target
  qmp: Add blockdev-mirror command
  iotests: Add test cases for blockdev-mirror

 block/mirror.c                  |   2 +
 blockdev.c                      | 177 ++++++++++++++++++++++++++++++----------
 hw/block/dataplane/virtio-blk.c |   2 +-
 include/block/block.h           |   3 +-
 qapi/block-core.json            |  47 +++++++++++
 qmp-commands.hx                 |  48 +++++++++++
 tests/qemu-iotests/041          | 100 ++++++++++++++++++-----
 tests/qemu-iotests/041.out      |   4 +-
 8 files changed, 313 insertions(+), 70 deletions(-)

-- 
2.4.2

[Qemu-devel] [PATCH 1/6] block: Add blocker on mirror target

[Fam Zheng]

In block/backup.c, we already check and add blocker on the target bs,
which is necessary so that it won't be intervened with other operations.

In block/mirror.c we should also protect the mirror target bs, because it
could have a node-name (drive-mirror ... node-name=XXX), and on top of
that it's safe to add blockdev-mirror.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index 33c640f..8b23ddd 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -360,6 +360,7 @@ static void mirror_exit(BlockJob *job, void *opaque)
         aio_context_release(replace_aio_context);
     }
     g_free(s->replaces);
+    bdrv_op_unblock_all(s->target, s->common.blocker);
     bdrv_unref(s->target);
     block_job_completed(&s->common, data->ret);
     g_free(data);
@@ -682,6 +683,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    bdrv_op_block_all(target, s->common.blocker);
     s->replaces = g_strdup(replaces);
     s->on_source_error = on_source_error;
     s->on_target_error = on_target_error;
-- 
2.4.2

[Qemu-devel] [PATCH 2/6] block: Rename BLOCK_OP_TYPE_MIRROR to BLOCK_OP_TYPE_MIRROR_SOURCE

[Fam Zheng]

It's necessary to distinguish source and target before we can add
blockdev-mirror, because we would want a concrete type of operation to
check on target bs before starting.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c                      | 2 +-
 hw/block/dataplane/virtio-blk.c | 2 +-
 include/block/block.h           | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 90f6e77..b573e56 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2962,7 +2962,7 @@ void qmp_drive_mirror(const char *device, const char *target,
         }
     }
 
-    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR, errp)) {
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
         goto out;
     }
 
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 3db139b..dac37de 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -206,7 +206,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
     blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, s->blocker);
     blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
                    s->blocker);
-    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker);
+    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_MIRROR_SOURCE, s->blocker);
     blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker);
     blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker);
 
diff --git a/include/block/block.h b/include/block/block.h
index f5c0a90..69cb676 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -155,7 +155,7 @@ typedef enum BlockOpType {
     BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
     BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
     BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
-    BLOCK_OP_TYPE_MIRROR,
+    BLOCK_OP_TYPE_MIRROR_SOURCE,
     BLOCK_OP_TYPE_RESIZE,
     BLOCK_OP_TYPE_STREAM,
     BLOCK_OP_TYPE_REPLACE,
-- 
2.4.2

[Qemu-devel] [PATCH 3/6] block: Extract blockdev part of qmp_drive_mirror

[Fam Zheng]

This is the part that will be reused by blockdev-mirror.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 158 +++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 92 insertions(+), 66 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b573e56..c32a9a9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2883,29 +2883,22 @@ out:
 
 #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
 
-void qmp_drive_mirror(const char *device, const char *target,
-                      bool has_format, const char *format,
-                      bool has_node_name, const char *node_name,
-                      bool has_replaces, const char *replaces,
-                      enum MirrorSyncMode sync,
-                      bool has_mode, enum NewImageMode mode,
-                      bool has_speed, int64_t speed,
-                      bool has_granularity, uint32_t granularity,
-                      bool has_buf_size, int64_t buf_size,
-                      bool has_on_source_error, BlockdevOnError on_source_error,
-                      bool has_on_target_error, BlockdevOnError on_target_error,
-                      Error **errp)
+/* Parameter check and block job starting for mirroring.
+ * Caller should hold @device and @target's aio context (They must to be on the
+ * same aio context). */
+static void blockdev_mirror_common(BlockDriverState *bs,
+                                   BlockDriverState *target,
+                                   bool has_replaces, const char *replaces,
+                                   enum MirrorSyncMode sync,
+                                   bool has_speed, int64_t speed,
+                                   bool has_granularity, uint32_t granularity,
+                                   bool has_buf_size, int64_t buf_size,
+                                   bool has_on_source_error,
+                                   BlockdevOnError on_source_error,
+                                   bool has_on_target_error,
+                                   BlockdevOnError on_target_error,
+                                   Error **errp)
 {
-    BlockBackend *blk;
-    BlockDriverState *bs;
-    BlockDriverState *source, *target_bs;
-    AioContext *aio_context;
-    BlockDriver *drv = NULL;
-    Error *local_err = NULL;
-    QDict *options = NULL;
-    int flags;
-    int64_t size;
-    int ret;
 
     if (!has_speed) {
         speed = 0;
@@ -2916,9 +2909,6 @@ void qmp_drive_mirror(const char *device, const char *target,
     if (!has_on_target_error) {
         on_target_error = BLOCKDEV_ON_ERROR_REPORT;
     }
-    if (!has_mode) {
-        mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-    }
     if (!has_granularity) {
         granularity = 0;
     }
@@ -2936,35 +2926,73 @@ void qmp_drive_mirror(const char *device, const char *target,
         return;
     }
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
-        return;
-    }
-
-    aio_context = blk_get_aio_context(blk);
-    aio_context_acquire(aio_context);
-
-    if (!blk_is_available(blk)) {
-        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
-        goto out;
-    }
-    bs = blk_bs(blk);
-
-    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);
-            goto out;
-        }
-    }
-
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
+        return;
+    }
+
+    if (!bs->backing_hd && sync == MIRROR_SYNC_MODE_TOP) {
+        sync = MIRROR_SYNC_MODE_FULL;
+    }
+
+    /* pass the node name to replace to mirror start since it's loose coupling
+     * and will allow to check whether the node still exist at mirror completion
+     */
+    mirror_start(bs, target,
+                 has_replaces ? replaces : NULL,
+                 speed, granularity, buf_size, sync,
+                 on_source_error, on_target_error,
+                 block_job_cb, bs, errp);
+}
+
+void qmp_drive_mirror(const char *device, const char *target,
+                      bool has_format, const char *format,
+                      bool has_node_name, const char *node_name,
+                      bool has_replaces, const char *replaces,
+                      enum MirrorSyncMode sync,
+                      bool has_mode, enum NewImageMode mode,
+                      bool has_speed, int64_t speed,
+                      bool has_granularity, uint32_t granularity,
+                      bool has_buf_size, int64_t buf_size,
+                      bool has_on_source_error, BlockdevOnError on_source_error,
+                      bool has_on_target_error, BlockdevOnError on_target_error,
+                      Error **errp)
+{
+    BlockDriverState *bs;
+    BlockBackend *blk;
+    BlockDriverState *source, *target_bs;
+    AioContext *aio_context;
+    BlockDriver *drv = NULL;
+    Error *local_err = NULL;
+    QDict *options = NULL;
+    int flags;
+    int64_t size;
+    int ret;
+
+    blk = blk_by_name(device);
+    if (!blk) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    aio_context = blk_get_aio_context(blk);
+    aio_context_acquire(aio_context);
+
+    if (!blk_is_available(blk)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
         goto out;
     }
+    bs = blk_bs(blk);
+
+    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);
+            goto out;
+        }
+    }
 
     flags = bs->open_flags | BDRV_O_RDWR;
     source = bs->backing_hd;
@@ -2989,14 +3017,14 @@ void qmp_drive_mirror(const char *device, const char *target,
         if (!has_node_name) {
             error_setg(errp, "a node-name must be provided when replacing a"
                              " named node of the graph");
-            goto out;
+            return;
         }
 
         to_replace_bs = check_to_replace_node(replaces, &local_err);
 
         if (!to_replace_bs) {
             error_propagate(errp, local_err);
-            goto out;
+            return;
         }
 
         replace_aio_context = bdrv_get_aio_context(to_replace_bs);
@@ -3007,7 +3035,7 @@ void qmp_drive_mirror(const char *device, const char *target,
         if (size != replace_size) {
             error_setg(errp, "cannot replace image with a mirror image of "
                              "different size");
-            goto out;
+            return;
         }
     }
 
@@ -3057,20 +3085,18 @@ void qmp_drive_mirror(const char *device, const char *target,
 
     bdrv_set_aio_context(target_bs, aio_context);
 
-    /* pass the node name to replace to mirror start since it's loose coupling
-     * and will allow to check whether the node still exist at mirror completion
-     */
-    mirror_start(bs, target_bs,
-                 has_replaces ? replaces : NULL,
-                 speed, granularity, buf_size, sync,
-                 on_source_error, on_target_error,
-                 block_job_cb, bs, &local_err);
-    if (local_err != NULL) {
-        bdrv_unref(target_bs);
+    blockdev_mirror_common(bs, target_bs,
+                           has_replaces, replaces, sync,
+                           has_speed, speed,
+                           has_granularity, granularity,
+                           has_buf_size, buf_size,
+                           has_on_source_error, on_source_error,
+                           has_on_target_error, on_target_error,
+                           &local_err);
+    if (local_err) {
         error_propagate(errp, local_err);
-        goto out;
+        bdrv_unref(target_bs);
     }
-
 out:
     aio_context_release(aio_context);
 }
-- 
2.4.2

[Qemu-devel] [PATCH 4/6] block: Add check on mirror target

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c            | 3 +++
 include/block/block.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index c32a9a9..44030da 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2929,6 +2929,9 @@ static void blockdev_mirror_common(BlockDriverState *bs,
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
         return;
     }
+    if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) {
+        return;
+    }
 
     if (!bs->backing_hd && sync == MIRROR_SYNC_MODE_TOP) {
         sync = MIRROR_SYNC_MODE_FULL;
diff --git a/include/block/block.h b/include/block/block.h
index 69cb676..c96d47b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -156,6 +156,7 @@ typedef enum BlockOpType {
     BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
     BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
     BLOCK_OP_TYPE_MIRROR_SOURCE,
+    BLOCK_OP_TYPE_MIRROR_TARGET,
     BLOCK_OP_TYPE_RESIZE,
     BLOCK_OP_TYPE_STREAM,
     BLOCK_OP_TYPE_REPLACE,
-- 
2.4.2

[Qemu-devel] [PATCH 5/6] qmp: Add blockdev-mirror command

[Fam Zheng]

This will start a mirror job from a named device to another named
device, its relation with drive-mirror is similar with blockdev-backup
to drive-backup.

In blockdev-mirror, the target node should be prepared by blockdev-add,
which will be responsible for assigning a name to the new node, so
'node-name' in drive-mirror is dropped.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c           | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx      | 48 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 155 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 44030da..6726b8d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2986,6 +2986,9 @@ void qmp_drive_mirror(const char *device, const char *target,
     }
     bs = blk_bs(blk);
 
+    if (!has_mode) {
+        mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+    }
     if (!has_format) {
         format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
     }
@@ -3104,6 +3107,63 @@ out:
     aio_context_release(aio_context);
 }
 
+void qmp_blockdev_mirror(const char *device, const char *target,
+                         bool has_replaces, const char *replaces,
+                         MirrorSyncMode sync,
+                         bool has_speed, int64_t speed,
+                         bool has_granularity, uint32_t granularity,
+                         bool has_buf_size, int64_t buf_size,
+                         bool has_on_source_error,
+                         BlockdevOnError on_source_error,
+                         bool has_on_target_error,
+                         BlockdevOnError on_target_error,
+                         Error **errp)
+{
+    BlockDriverState *bs;
+    BlockBackend *blk;
+    BlockDriverState *target_bs;
+    AioContext *aio_context;
+    Error *local_err = NULL;
+
+    blk = blk_by_name(device);
+    if (!blk) {
+        error_setg(errp, "Device '%s' not found", device);
+        return;
+    }
+    bs = blk_bs(blk);
+
+    if (!bs) {
+        error_setg(errp, "Device '%s' has no media", device);
+        return;
+    }
+
+    target_bs = bdrv_lookup_bs(target, target, errp);
+    if (!target_bs) {
+        return;
+    }
+
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
+    bdrv_ref(target_bs);
+    bdrv_set_aio_context(target_bs, aio_context);
+
+    blockdev_mirror_common(bs, target_bs,
+                           has_replaces, replaces, sync,
+                           has_speed, speed,
+                           has_granularity, granularity,
+                           has_buf_size, buf_size,
+                           has_on_source_error, on_source_error,
+                           has_on_target_error, on_target_error,
+                           &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        bdrv_unref(target_bs);
+    }
+
+    aio_context_release(aio_context);
+}
+
 /* Get the block job for a given device name and acquire its AioContext */
 static BlockJob *find_block_job(const char *device, AioContext **aio_context,
                                 Error **errp)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b5c4559..bd163f7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1059,6 +1059,53 @@
   'data': 'BlockDirtyBitmap' }
 
 ##
+# @blockdev-mirror
+#
+# Start mirroring a block device's writes to a new destination.
+#
+# @device:  the name of the device whose writes should be mirrored.
+#
+# @target: the name of the device to mirror to.
+#
+# @replaces: #optional with sync=full graph node name to be replaced by the new
+#            image when a whole image copy is done. This can be used to repair
+#            broken Quorum files.
+#
+# @speed:  #optional the maximum speed, in bytes per second
+#
+# @sync: what parts of the disk image should be copied to the destination
+#        (all the disk, only the sectors allocated in the topmost image, or
+#        only new I/O).
+#
+# @granularity: #optional granularity of the dirty bitmap, default is 64K
+#               if the image format doesn't have clusters, 4K if the clusters
+#               are smaller than that, else the cluster size.  Must be a
+#               power of 2 between 512 and 64M
+#
+# @buf-size: #optional maximum amount of data in flight from source to
+#            target
+#
+# @on-source-error: #optional the action to take on an error on the source,
+#                   default 'report'.  'stop' and 'enospc' can only be used
+#                   if the block device supports io-status (see BlockInfo).
+#
+# @on-target-error: #optional the action to take on an error on the target,
+#                   default 'report' (no limitations, since this applies to
+#                   a different block device than @device).
+#
+# Returns: nothing on success; error message on failure.
+#
+# Since 2.4
+##
+{ 'command': 'blockdev-mirror',
+  'data': { 'device': 'str', 'target': 'str',
+            '*replaces': 'str',
+            'sync': 'MirrorSyncMode',
+            '*speed': 'int', '*granularity': 'uint32',
+            '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
+            '*on-target-error': 'BlockdevOnError' } }
+
+##
 # @block_set_io_throttle:
 #
 # Change I/O throttle limits for a block drive.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 90e0135..646db78 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1560,6 +1560,54 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-mirror",
+        .args_type  = "sync:s,device:B,target:B,replaces:s?,speed:i?,"
+                      "on-source-error:s?,on-target-error:s?,"
+                      "granularity:i?,buf-size:i?",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_mirror,
+    },
+
+SQMP
+blockdev-mirror
+------------
+
+Start mirroring a block device's writes to another block device. target
+specifies the target of mirror operation.
+
+Arguments:
+
+- "device": device name to operate on (json-string)
+- "target": device name to mirror to (json-string)
+- "replaces": the block driver node name to replace when finished
+              (json-string, optional)
+- "speed": maximum speed of the streaming job, in bytes per second
+  (json-int)
+- "granularity": granularity of the dirty bitmap, in bytes (json-int, optional)
+- "buf_size": maximum amount of data in flight from source to target, in bytes
+  (json-int, default 10M)
+- "sync": what parts of the disk image should be copied to the destination;
+  possibilities include "full" for all the disk, "top" for only the sectors
+  allocated in the topmost image, or "none" to only replicate new I/O
+  (MirrorSyncMode).
+- "on-source-error": the action to take on an error on the source
+  (BlockdevOnError, default 'report')
+- "on-target-error": the action to take on an error on the target
+  (BlockdevOnError, default 'report')
+
+The default value of the granularity is the image cluster size clamped
+between 4096 and 65536, if the image format defines one.  If the format
+does not define a cluster size, the default value of the granularity
+is 65536.
+
+Example:
+
+-> { "execute": "blockdev-mirror", "arguments": { "device": "ide-hd0",
+                                                  "target": "target0",
+                                                  "sync": "full" } }
+<- { "return": {} }
+
+EQMP
+    {
         .name       = "change-backing-file",
         .args_type  = "device:s,image-node-name:s,backing-file:s",
         .mhandler.cmd_new = qmp_marshal_input_change_backing_file,
-- 
2.4.2

[Qemu-devel] [PATCH 6/6] iotests: Add test cases for blockdev-mirror

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/041     | 100 +++++++++++++++++++++++++++++++++++----------
 tests/qemu-iotests/041.out |   4 +-
 2 files changed, 80 insertions(+), 24 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 59a8f73..922f53c 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -65,8 +65,23 @@ class ImageMirroringTestCase(iotests.QMPTestCase):
         event = self.wait_until_completed(drive=drive)
         self.assert_qmp(event, 'data/type', 'mirror')
 
+    def blockdev_add(self, filename, drive_id=None, node_name=None):
+        options = {'driver': iotests.imgfmt,
+                   'file': {
+                       'filename': filename,
+                       'driver': 'file'}}
+        if drive_id:
+            options['id'] = drive_id
+        if node_name:
+            options['node-name'] = node_name
+        result = self.vm.qmp('blockdev-add', options=options)
+        self.assert_qmp(result, 'return', {})
+
 class TestSingleDrive(ImageMirroringTestCase):
     image_len = 1 * 1024 * 1024 # MB
+    qmp_cmd = 'drive-mirror'
+    qmp_target = target_img
+    not_found_error = 'DeviceNotFound'
 
     def setUp(self):
         iotests.create_image(backing_img, self.image_len)
@@ -86,8 +101,8 @@ class TestSingleDrive(ImageMirroringTestCase):
     def test_complete(self):
         self.assert_no_active_block_jobs()
 
-        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
-                             target=target_img)
+        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+                             target=self.qmp_target)
         self.assert_qmp(result, 'return', {})
 
         self.complete_and_wait()
@@ -100,8 +115,8 @@ class TestSingleDrive(ImageMirroringTestCase):
     def test_cancel(self):
         self.assert_no_active_block_jobs()
 
-        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
-                             target=target_img)
+        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+                             target=self.qmp_target)
         self.assert_qmp(result, 'return', {})
 
         self.cancel_and_wait(force=True)
@@ -112,8 +127,8 @@ class TestSingleDrive(ImageMirroringTestCase):
     def test_cancel_after_ready(self):
         self.assert_no_active_block_jobs()
 
-        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
-                             target=target_img)
+        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+                             target=self.qmp_target)
         self.assert_qmp(result, 'return', {})
 
         self.wait_ready_and_cancel()
@@ -126,8 +141,8 @@ class TestSingleDrive(ImageMirroringTestCase):
     def test_pause(self):
         self.assert_no_active_block_jobs()
 
-        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
-                             target=target_img)
+        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+                             target=self.qmp_target)
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-job-pause', device='drive0')
@@ -153,8 +168,8 @@ class TestSingleDrive(ImageMirroringTestCase):
         self.assert_no_active_block_jobs()
 
         # A small buffer is rounded up automatically
-        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
-                             buf_size=4096, target=target_img)
+        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+                             buf_size=4096, target=self.qmp_target)
         self.assert_qmp(result, 'return', {})
 
         self.complete_and_wait()
@@ -169,8 +184,8 @@ class TestSingleDrive(ImageMirroringTestCase):
 
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d,size=%d'
                         % (self.image_len, self.image_len), target_img)
-        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
-                             buf_size=65536, mode='existing', target=target_img)
+        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+                             buf_size=65536, mode='existing', target=self.qmp_target)
         self.assert_qmp(result, 'return', {})
 
         self.complete_and_wait()
@@ -185,8 +200,8 @@ class TestSingleDrive(ImageMirroringTestCase):
 
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d,backing_file=%s'
                         % (self.image_len, backing_img), target_img)
-        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
-                             mode='existing', target=target_img)
+        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+                             mode='existing', target=self.qmp_target)
         self.assert_qmp(result, 'return', {})
 
         self.complete_and_wait()
@@ -197,30 +212,72 @@ class TestSingleDrive(ImageMirroringTestCase):
                         'target image does not match source after mirroring')
 
     def test_medium_not_found(self):
-        result = self.vm.qmp('drive-mirror', device='ide1-cd0', sync='full',
-                             target=target_img)
+        result = self.vm.qmp(self.qmp_cmd, device='ide1-cd0', sync='full',
+                             target=self.qmp_target)
         self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_image_not_found(self):
-        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
-                             mode='existing', target=target_img)
+        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+                             mode='existing', target=self.qmp_target)
         self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_device_not_found(self):
-        result = self.vm.qmp('drive-mirror', device='nonexistent', sync='full',
-                             target=target_img)
-        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+        result = self.vm.qmp(self.qmp_cmd, device='nonexistent', sync='full',
+                             target=self.qmp_target)
+        self.assert_qmp(result, 'error/class', self.not_found_error)
+
+class TestSingleBlockdev(TestSingleDrive):
+    qmp_cmd = 'blockdev-mirror'
+    qmp_target = 'drive1'
+    not_found_error = 'GenericError'
+
+    def setUp(self):
+        TestSingleDrive.setUp(self)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, target_img)
+        self.blockdev_add(target_img, node_name='drive1')
+
+    test_large_cluster = None
+    test_image_not_found = None
+    test_small_buffer2 = None
+
+class TestBlockdevAttached(ImageMirroringTestCase):
+    image_len = 1 * 1024 * 1024 # MB
+
+    def setUp(self):
+        iotests.create_image(backing_img, self.image_len)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, target_img)
+        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_blockdev_attached(self):
+        self.assert_no_active_block_jobs()
+        self.blockdev_add(target_img, drive_id='drive1')
+        result = self.vm.qmp('blockdev-mirror', device='drive0', sync='full',
+                             target='drive1')
+        self.assert_qmp(result, 'error/class', 'GenericError')
 
 class TestSingleDriveZeroLength(TestSingleDrive):
     image_len = 0
     test_small_buffer2 = None
     test_large_cluster = None
 
+class TestSingleBlockdevZeroLength(TestSingleBlockdev):
+    image_len = 0
+
 class TestSingleDriveUnalignedLength(TestSingleDrive):
     image_len = 1025 * 1024
     test_small_buffer2 = None
     test_large_cluster = None
 
+class TestSingleBlockdevUnalignedLength(TestSingleBlockdev):
+    image_len = 1025 * 1024
+
 class TestMirrorNoBacking(ImageMirroringTestCase):
     image_len = 2 * 1024 * 1024 # MB
 
@@ -949,6 +1006,5 @@ class TestRepairQuorum(ImageMirroringTestCase):
         # TODO: a better test requiring some QEMU infrastructure will be added
         #       to check that this file is really driven by quorum
         self.vm.shutdown()
-
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index 24093bc..b67d050 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-......................................................
+............................................................................
 ----------------------------------------------------------------------
-Ran 54 tests
+Ran 76 tests
 
 OK
-- 
2.4.2

Re: [Qemu-devel] [PATCH 1/6] block: Add blocker on mirror target

[Max Reitz]

On 09.06.2015 04:13, Fam Zheng wrote:
> In block/backup.c, we already check and add blocker on the target bs,
> which is necessary so that it won't be intervened with other operations.
>
> In block/mirror.c we should also protect the mirror target bs, because it
> could have a node-name (drive-mirror ... node-name=XXX), and on top of
> that it's safe to add blockdev-mirror.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block/mirror.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 33c640f..8b23ddd 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -360,6 +360,7 @@ static void mirror_exit(BlockJob *job, void *opaque)
>           aio_context_release(replace_aio_context);
>       }
>       g_free(s->replaces);
> +    bdrv_op_unblock_all(s->target, s->common.blocker);
>       bdrv_unref(s->target);
>       block_job_completed(&s->common, data->ret);
>       g_free(data);
> @@ -682,6 +683,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
>           return;
>       }
>   
> +    bdrv_op_block_all(target, s->common.blocker);
>       s->replaces = g_strdup(replaces);
>       s->on_source_error = on_source_error;
>       s->on_target_error = on_target_error;

Do we need a bdrv_op_unblock_all() if bdrv_create_dirty_bitmap() fails?

Max

Re: [Qemu-devel] [PATCH 2/6] block: Rename BLOCK_OP_TYPE_MIRROR to BLOCK_OP_TYPE_MIRROR_SOURCE

[Max Reitz]

On 09.06.2015 04:13, Fam Zheng wrote:
> It's necessary to distinguish source and target before we can add
> blockdev-mirror, because we would want a concrete type of operation to
> check on target bs before starting.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   blockdev.c                      | 2 +-
>   hw/block/dataplane/virtio-blk.c | 2 +-
>   include/block/block.h           | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH 3/6] block: Extract blockdev part of qmp_drive_mirror

[Max Reitz]

On 09.06.2015 04:13, Fam Zheng wrote:
> This is the part that will be reused by blockdev-mirror.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   blockdev.c | 158 +++++++++++++++++++++++++++++++++++--------------------------
>   1 file changed, 92 insertions(+), 66 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index b573e56..c32a9a9 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2883,29 +2883,22 @@ out:
>   
>   #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
>   
> -void qmp_drive_mirror(const char *device, const char *target,
> -                      bool has_format, const char *format,
> -                      bool has_node_name, const char *node_name,
> -                      bool has_replaces, const char *replaces,
> -                      enum MirrorSyncMode sync,
> -                      bool has_mode, enum NewImageMode mode,
> -                      bool has_speed, int64_t speed,
> -                      bool has_granularity, uint32_t granularity,
> -                      bool has_buf_size, int64_t buf_size,
> -                      bool has_on_source_error, BlockdevOnError on_source_error,
> -                      bool has_on_target_error, BlockdevOnError on_target_error,
> -                      Error **errp)
> +/* Parameter check and block job starting for mirroring.
> + * Caller should hold @device and @target's aio context (They must to be on the
> + * same aio context). */
> +static void blockdev_mirror_common(BlockDriverState *bs,
> +                                   BlockDriverState *target,
> +                                   bool has_replaces, const char *replaces,
> +                                   enum MirrorSyncMode sync,
> +                                   bool has_speed, int64_t speed,
> +                                   bool has_granularity, uint32_t granularity,
> +                                   bool has_buf_size, int64_t buf_size,
> +                                   bool has_on_source_error,
> +                                   BlockdevOnError on_source_error,
> +                                   bool has_on_target_error,
> +                                   BlockdevOnError on_target_error,
> +                                   Error **errp)
>   {
> -    BlockBackend *blk;
> -    BlockDriverState *bs;
> -    BlockDriverState *source, *target_bs;
> -    AioContext *aio_context;
> -    BlockDriver *drv = NULL;
> -    Error *local_err = NULL;
> -    QDict *options = NULL;
> -    int flags;
> -    int64_t size;
> -    int ret;
>   
>       if (!has_speed) {
>           speed = 0;
> @@ -2916,9 +2909,6 @@ void qmp_drive_mirror(const char *device, const char *target,
>       if (!has_on_target_error) {
>           on_target_error = BLOCKDEV_ON_ERROR_REPORT;
>       }
> -    if (!has_mode) {
> -        mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> -    }
>       if (!has_granularity) {
>           granularity = 0;
>       }
> @@ -2936,35 +2926,73 @@ void qmp_drive_mirror(const char *device, const char *target,
>           return;
>       }
>   
> -    blk = blk_by_name(device);
> -    if (!blk) {
> -        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> -        return;
> -    }
> -
> -    aio_context = blk_get_aio_context(blk);
> -    aio_context_acquire(aio_context);
> -
> -    if (!blk_is_available(blk)) {
> -        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> -        goto out;
> -    }
> -    bs = blk_bs(blk);
> -
> -    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);
> -            goto out;
> -        }
> -    }
> -
>       if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
> +        return;
> +    }
> +
> +    if (!bs->backing_hd && sync == MIRROR_SYNC_MODE_TOP) {
> +        sync = MIRROR_SYNC_MODE_FULL;
> +    }
> +
> +    /* pass the node name to replace to mirror start since it's loose coupling
> +     * and will allow to check whether the node still exist at mirror completion
> +     */
> +    mirror_start(bs, target,
> +                 has_replaces ? replaces : NULL,
> +                 speed, granularity, buf_size, sync,
> +                 on_source_error, on_target_error,
> +                 block_job_cb, bs, errp);
> +}
> +
> +void qmp_drive_mirror(const char *device, const char *target,
> +                      bool has_format, const char *format,
> +                      bool has_node_name, const char *node_name,
> +                      bool has_replaces, const char *replaces,
> +                      enum MirrorSyncMode sync,
> +                      bool has_mode, enum NewImageMode mode,
> +                      bool has_speed, int64_t speed,
> +                      bool has_granularity, uint32_t granularity,
> +                      bool has_buf_size, int64_t buf_size,
> +                      bool has_on_source_error, BlockdevOnError on_source_error,
> +                      bool has_on_target_error, BlockdevOnError on_target_error,
> +                      Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BlockBackend *blk;
> +    BlockDriverState *source, *target_bs;
> +    AioContext *aio_context;
> +    BlockDriver *drv = NULL;
> +    Error *local_err = NULL;
> +    QDict *options = NULL;
> +    int flags;
> +    int64_t size;
> +    int ret;
> +
> +    blk = blk_by_name(device);
> +    if (!blk) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    aio_context = blk_get_aio_context(blk);
> +    aio_context_acquire(aio_context);
> +
> +    if (!blk_is_available(blk)) {
> +        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
>           goto out;
>       }
> +    bs = blk_bs(blk);
> +
> +    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);
> +            goto out;
> +        }
> +    }
>   
>       flags = bs->open_flags | BDRV_O_RDWR;
>       source = bs->backing_hd;
> @@ -2989,14 +3017,14 @@ void qmp_drive_mirror(const char *device, const char *target,
>           if (!has_node_name) {
>               error_setg(errp, "a node-name must be provided when replacing a"
>                                " named node of the graph");
> -            goto out;
> +            return;
>           }
>   
>           to_replace_bs = check_to_replace_node(replaces, &local_err);
>   
>           if (!to_replace_bs) {
>               error_propagate(errp, local_err);
> -            goto out;
> +            return;
>           }
>   
>           replace_aio_context = bdrv_get_aio_context(to_replace_bs);
> @@ -3007,7 +3035,7 @@ void qmp_drive_mirror(const char *device, const char *target,
>           if (size != replace_size) {
>               error_setg(errp, "cannot replace image with a mirror image of "
>                                "different size");
> -            goto out;
> +            return;
>           }
>       }

There are still a couple of "return;" statements left in the "if 
(has_replaces)" block. I think they should be replaced, too.

Max

Re: [Qemu-devel] [PATCH 4/6] block: Add check on mirror target

[Max Reitz]

On 09.06.2015 04:13, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   blockdev.c            | 3 +++
>   include/block/block.h | 1 +
>   2 files changed, 4 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH 5/6] qmp: Add blockdev-mirror command

[Max Reitz]

On 09.06.2015 04:13, Fam Zheng wrote:
> This will start a mirror job from a named device to another named
> device, its relation with drive-mirror is similar with blockdev-backup
> to drive-backup.
>
> In blockdev-mirror, the target node should be prepared by blockdev-add,
> which will be responsible for assigning a name to the new node, so
> 'node-name' in drive-mirror is dropped.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   blockdev.c           | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++
>   qmp-commands.hx      | 48 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 155 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index 44030da..6726b8d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2986,6 +2986,9 @@ void qmp_drive_mirror(const char *device, const char *target,
>       }
>       bs = blk_bs(blk);
>   
> +    if (!has_mode) {
> +        mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> +    }

I think this hunk should be in patch 3, not here.

Other than that, looks good. (apart from a pedantic comment below)

Max

>       if (!has_format) {
>           format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
>       }
> @@ -3104,6 +3107,63 @@ out:
>       aio_context_release(aio_context);
>   }
>   
> +void qmp_blockdev_mirror(const char *device, const char *target,
> +                         bool has_replaces, const char *replaces,
> +                         MirrorSyncMode sync,
> +                         bool has_speed, int64_t speed,
> +                         bool has_granularity, uint32_t granularity,
> +                         bool has_buf_size, int64_t buf_size,
> +                         bool has_on_source_error,
> +                         BlockdevOnError on_source_error,
> +                         bool has_on_target_error,
> +                         BlockdevOnError on_target_error,
> +                         Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BlockBackend *blk;
> +    BlockDriverState *target_bs;
> +    AioContext *aio_context;
> +    Error *local_err = NULL;
> +
> +    blk = blk_by_name(device);
> +    if (!blk) {
> +        error_setg(errp, "Device '%s' not found", device);
> +        return;
> +    }
> +    bs = blk_bs(blk);
> +
> +    if (!bs) {
> +        error_setg(errp, "Device '%s' has no media", device);
> +        return;
> +    }
> +
> +    target_bs = bdrv_lookup_bs(target, target, errp);
> +    if (!target_bs) {
> +        return;
> +    }
> +
> +    aio_context = bdrv_get_aio_context(bs);
> +    aio_context_acquire(aio_context);
> +
> +    bdrv_ref(target_bs);
> +    bdrv_set_aio_context(target_bs, aio_context);
> +
> +    blockdev_mirror_common(bs, target_bs,
> +                           has_replaces, replaces, sync,
> +                           has_speed, speed,
> +                           has_granularity, granularity,
> +                           has_buf_size, buf_size,
> +                           has_on_source_error, on_source_error,
> +                           has_on_target_error, on_target_error,
> +                           &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        bdrv_unref(target_bs);
> +    }
> +
> +    aio_context_release(aio_context);
> +}
> +
>   /* Get the block job for a given device name and acquire its AioContext */
>   static BlockJob *find_block_job(const char *device, AioContext **aio_context,
>                                   Error **errp)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b5c4559..bd163f7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1059,6 +1059,53 @@
>     'data': 'BlockDirtyBitmap' }
>   
>   ##
> +# @blockdev-mirror
> +#
> +# Start mirroring a block device's writes to a new destination.
> +#
> +# @device:  the name of the device whose writes should be mirrored.

Superfluous space here. (</pedantic>)

> +#
> +# @target: the name of the device to mirror to.
> +#
> +# @replaces: #optional with sync=full graph node name to be replaced by the new
> +#            image when a whole image copy is done. This can be used to repair
> +#            broken Quorum files.
> +#
> +# @speed:  #optional the maximum speed, in bytes per second
> +#
> +# @sync: what parts of the disk image should be copied to the destination
> +#        (all the disk, only the sectors allocated in the topmost image, or
> +#        only new I/O).
> +#
> +# @granularity: #optional granularity of the dirty bitmap, default is 64K
> +#               if the image format doesn't have clusters, 4K if the clusters
> +#               are smaller than that, else the cluster size.  Must be a
> +#               power of 2 between 512 and 64M
> +#
> +# @buf-size: #optional maximum amount of data in flight from source to
> +#            target
> +#
> +# @on-source-error: #optional the action to take on an error on the source,
> +#                   default 'report'.  'stop' and 'enospc' can only be used
> +#                   if the block device supports io-status (see BlockInfo).
> +#
> +# @on-target-error: #optional the action to take on an error on the target,
> +#                   default 'report' (no limitations, since this applies to
> +#                   a different block device than @device).
> +#
> +# Returns: nothing on success; error message on failure.
> +#
> +# Since 2.4
> +##
> +{ 'command': 'blockdev-mirror',
> +  'data': { 'device': 'str', 'target': 'str',
> +            '*replaces': 'str',
> +            'sync': 'MirrorSyncMode',
> +            '*speed': 'int', '*granularity': 'uint32',
> +            '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> +            '*on-target-error': 'BlockdevOnError' } }
> +
> +##
>   # @block_set_io_throttle:
>   #
>   # Change I/O throttle limits for a block drive.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 90e0135..646db78 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1560,6 +1560,54 @@ Example:
>   EQMP
>   
>       {
> +        .name       = "blockdev-mirror",
> +        .args_type  = "sync:s,device:B,target:B,replaces:s?,speed:i?,"
> +                      "on-source-error:s?,on-target-error:s?,"
> +                      "granularity:i?,buf-size:i?",
> +        .mhandler.cmd_new = qmp_marshal_input_blockdev_mirror,
> +    },
> +
> +SQMP
> +blockdev-mirror
> +------------
> +
> +Start mirroring a block device's writes to another block device. target
> +specifies the target of mirror operation.
> +
> +Arguments:
> +
> +- "device": device name to operate on (json-string)
> +- "target": device name to mirror to (json-string)
> +- "replaces": the block driver node name to replace when finished
> +              (json-string, optional)
> +- "speed": maximum speed of the streaming job, in bytes per second
> +  (json-int)
> +- "granularity": granularity of the dirty bitmap, in bytes (json-int, optional)
> +- "buf_size": maximum amount of data in flight from source to target, in bytes
> +  (json-int, default 10M)
> +- "sync": what parts of the disk image should be copied to the destination;
> +  possibilities include "full" for all the disk, "top" for only the sectors
> +  allocated in the topmost image, or "none" to only replicate new I/O
> +  (MirrorSyncMode).
> +- "on-source-error": the action to take on an error on the source
> +  (BlockdevOnError, default 'report')
> +- "on-target-error": the action to take on an error on the target
> +  (BlockdevOnError, default 'report')
> +
> +The default value of the granularity is the image cluster size clamped
> +between 4096 and 65536, if the image format defines one.  If the format
> +does not define a cluster size, the default value of the granularity
> +is 65536.
> +
> +Example:
> +
> +-> { "execute": "blockdev-mirror", "arguments": { "device": "ide-hd0",
> +                                                  "target": "target0",
> +                                                  "sync": "full" } }
> +<- { "return": {} }
> +
> +EQMP
> +    {
>           .name       = "change-backing-file",
>           .args_type  = "device:s,image-node-name:s,backing-file:s",
>           .mhandler.cmd_new = qmp_marshal_input_change_backing_file,

Re: [Qemu-devel] [PATCH 6/6] iotests: Add test cases for blockdev-mirror

[Max Reitz]

On 09.06.2015 04:13, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   tests/qemu-iotests/041     | 100 +++++++++++++++++++++++++++++++++++----------
>   tests/qemu-iotests/041.out |   4 +-
>   2 files changed, 80 insertions(+), 24 deletions(-)
>
> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> index 59a8f73..922f53c 100755
> --- a/tests/qemu-iotests/041
> +++ b/tests/qemu-iotests/041
> @@ -65,8 +65,23 @@ class ImageMirroringTestCase(iotests.QMPTestCase):
>           event = self.wait_until_completed(drive=drive)
>           self.assert_qmp(event, 'data/type', 'mirror')
>   
> +    def blockdev_add(self, filename, drive_id=None, node_name=None):
> +        options = {'driver': iotests.imgfmt,
> +                   'file': {
> +                       'filename': filename,
> +                       'driver': 'file'}}
> +        if drive_id:
> +            options['id'] = drive_id
> +        if node_name:
> +            options['node-name'] = node_name
> +        result = self.vm.qmp('blockdev-add', options=options)
> +        self.assert_qmp(result, 'return', {})
> +
>   class TestSingleDrive(ImageMirroringTestCase):
>       image_len = 1 * 1024 * 1024 # MB
> +    qmp_cmd = 'drive-mirror'
> +    qmp_target = target_img
> +    not_found_error = 'DeviceNotFound'
>   
>       def setUp(self):
>           iotests.create_image(backing_img, self.image_len)
> @@ -86,8 +101,8 @@ class TestSingleDrive(ImageMirroringTestCase):
>       def test_complete(self):
>           self.assert_no_active_block_jobs()
>   
> -        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
> -                             target=target_img)
> +        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
> +                             target=self.qmp_target)
>           self.assert_qmp(result, 'return', {})
>   
>           self.complete_and_wait()
> @@ -100,8 +115,8 @@ class TestSingleDrive(ImageMirroringTestCase):
>       def test_cancel(self):
>           self.assert_no_active_block_jobs()
>   
> -        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
> -                             target=target_img)
> +        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
> +                             target=self.qmp_target)
>           self.assert_qmp(result, 'return', {})
>   
>           self.cancel_and_wait(force=True)
> @@ -112,8 +127,8 @@ class TestSingleDrive(ImageMirroringTestCase):
>       def test_cancel_after_ready(self):
>           self.assert_no_active_block_jobs()
>   
> -        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
> -                             target=target_img)
> +        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
> +                             target=self.qmp_target)
>           self.assert_qmp(result, 'return', {})
>   
>           self.wait_ready_and_cancel()
> @@ -126,8 +141,8 @@ class TestSingleDrive(ImageMirroringTestCase):
>       def test_pause(self):
>           self.assert_no_active_block_jobs()
>   
> -        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
> -                             target=target_img)
> +        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
> +                             target=self.qmp_target)
>           self.assert_qmp(result, 'return', {})
>   
>           result = self.vm.qmp('block-job-pause', device='drive0')
> @@ -153,8 +168,8 @@ class TestSingleDrive(ImageMirroringTestCase):
>           self.assert_no_active_block_jobs()
>   
>           # A small buffer is rounded up automatically
> -        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
> -                             buf_size=4096, target=target_img)
> +        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
> +                             buf_size=4096, target=self.qmp_target)
>           self.assert_qmp(result, 'return', {})
>   
>           self.complete_and_wait()
> @@ -169,8 +184,8 @@ class TestSingleDrive(ImageMirroringTestCase):
>   
>           qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d,size=%d'
>                           % (self.image_len, self.image_len), target_img)
> -        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
> -                             buf_size=65536, mode='existing', target=target_img)
> +        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
> +                             buf_size=65536, mode='existing', target=self.qmp_target)
>           self.assert_qmp(result, 'return', {})
>   
>           self.complete_and_wait()
> @@ -185,8 +200,8 @@ class TestSingleDrive(ImageMirroringTestCase):
>   
>           qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d,backing_file=%s'
>                           % (self.image_len, backing_img), target_img)
> -        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
> -                             mode='existing', target=target_img)
> +        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
> +                             mode='existing', target=self.qmp_target)
>           self.assert_qmp(result, 'return', {})
>   
>           self.complete_and_wait()
> @@ -197,30 +212,72 @@ class TestSingleDrive(ImageMirroringTestCase):
>                           'target image does not match source after mirroring')
>   
>       def test_medium_not_found(self):
> -        result = self.vm.qmp('drive-mirror', device='ide1-cd0', sync='full',
> -                             target=target_img)
> +        result = self.vm.qmp(self.qmp_cmd, device='ide1-cd0', sync='full',
> +                             target=self.qmp_target)
>           self.assert_qmp(result, 'error/class', 'GenericError')
>   
>       def test_image_not_found(self):
> -        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
> -                             mode='existing', target=target_img)
> +        result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
> +                             mode='existing', target=self.qmp_target)
>           self.assert_qmp(result, 'error/class', 'GenericError')
>   
>       def test_device_not_found(self):
> -        result = self.vm.qmp('drive-mirror', device='nonexistent', sync='full',
> -                             target=target_img)
> -        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
> +        result = self.vm.qmp(self.qmp_cmd, device='nonexistent', sync='full',
> +                             target=self.qmp_target)
> +        self.assert_qmp(result, 'error/class', self.not_found_error)
> +
> +class TestSingleBlockdev(TestSingleDrive):
> +    qmp_cmd = 'blockdev-mirror'
> +    qmp_target = 'drive1'
> +    not_found_error = 'GenericError'
> +
> +    def setUp(self):
> +        TestSingleDrive.setUp(self)
> +        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, target_img)
> +        self.blockdev_add(target_img, node_name='drive1')
> +
> +    test_large_cluster = None
> +    test_image_not_found = None
> +    test_small_buffer2 = None
> +
> +class TestBlockdevAttached(ImageMirroringTestCase):
> +    image_len = 1 * 1024 * 1024 # MB
> +
> +    def setUp(self):
> +        iotests.create_image(backing_img, self.image_len)
> +        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
> +        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, target_img)
> +        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_blockdev_attached(self):
> +        self.assert_no_active_block_jobs()
> +        self.blockdev_add(target_img, drive_id='drive1')
> +        result = self.vm.qmp('blockdev-mirror', device='drive0', sync='full',
> +                             target='drive1')
> +        self.assert_qmp(result, 'error/class', 'GenericError')
>   
>   class TestSingleDriveZeroLength(TestSingleDrive):
>       image_len = 0
>       test_small_buffer2 = None
>       test_large_cluster = None
>   
> +class TestSingleBlockdevZeroLength(TestSingleBlockdev):
> +    image_len = 0
> +
>   class TestSingleDriveUnalignedLength(TestSingleDrive):
>       image_len = 1025 * 1024
>       test_small_buffer2 = None
>       test_large_cluster = None
>   
> +class TestSingleBlockdevUnalignedLength(TestSingleBlockdev):
> +    image_len = 1025 * 1024
> +
>   class TestMirrorNoBacking(ImageMirroringTestCase):
>       image_len = 2 * 1024 * 1024 # MB
>   
> @@ -949,6 +1006,5 @@ class TestRepairQuorum(ImageMirroringTestCase):
>           # TODO: a better test requiring some QEMU infrastructure will be added
>           #       to check that this file is really driven by quorum
>           self.vm.shutdown()
> -

Intentional?

Without this hunk:

Reviewed-by: Max Reitz <mreitz@redhat.com>

>   if __name__ == '__main__':
>       iotests.main(supported_fmts=['qcow2', 'qed'])
> diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
> index 24093bc..b67d050 100644
> --- a/tests/qemu-iotests/041.out
> +++ b/tests/qemu-iotests/041.out
> @@ -1,5 +1,5 @@
> -......................................................
> +............................................................................
>   ----------------------------------------------------------------------
> -Ran 54 tests
> +Ran 76 tests
>   
>   OK

Re: [Qemu-devel] [PATCH 1/6] block: Add blocker on mirror target

[Fam Zheng]

On Wed, 06/24 18:14, Max Reitz wrote:
> On 09.06.2015 04:13, Fam Zheng wrote:
> >In block/backup.c, we already check and add blocker on the target bs,
> >which is necessary so that it won't be intervened with other operations.
> >
> >In block/mirror.c we should also protect the mirror target bs, because it
> >could have a node-name (drive-mirror ... node-name=XXX), and on top of
> >that it's safe to add blockdev-mirror.
> >
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  block/mirror.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> >diff --git a/block/mirror.c b/block/mirror.c
> >index 33c640f..8b23ddd 100644
> >--- a/block/mirror.c
> >+++ b/block/mirror.c
> >@@ -360,6 +360,7 @@ static void mirror_exit(BlockJob *job, void *opaque)
> >          aio_context_release(replace_aio_context);
> >      }
> >      g_free(s->replaces);
> >+    bdrv_op_unblock_all(s->target, s->common.blocker);
> >      bdrv_unref(s->target);
> >      block_job_completed(&s->common, data->ret);
> >      g_free(data);
> >@@ -682,6 +683,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
> >          return;
> >      }
> >+    bdrv_op_block_all(target, s->common.blocker);
> >      s->replaces = g_strdup(replaces);
> >      s->on_source_error = on_source_error;
> >      s->on_target_error = on_target_error;
> 
> Do we need a bdrv_op_unblock_all() if bdrv_create_dirty_bitmap() fails?
> 

Yes, good catch!

Fam

Re: [Qemu-devel] [PATCH 3/6] block: Extract blockdev part of qmp_drive_mirror

[Fam Zheng]

On Wed, 06/24 18:34, Max Reitz wrote:
> On 09.06.2015 04:13, Fam Zheng wrote:
> >This is the part that will be reused by blockdev-mirror.
> >
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  blockdev.c | 158 +++++++++++++++++++++++++++++++++++--------------------------
> >  1 file changed, 92 insertions(+), 66 deletions(-)
> >
> >diff --git a/blockdev.c b/blockdev.c
> >index b573e56..c32a9a9 100644
> >--- a/blockdev.c
> >+++ b/blockdev.c
> >@@ -2883,29 +2883,22 @@ out:
> >  #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
> >-void qmp_drive_mirror(const char *device, const char *target,
> >-                      bool has_format, const char *format,
> >-                      bool has_node_name, const char *node_name,
> >-                      bool has_replaces, const char *replaces,
> >-                      enum MirrorSyncMode sync,
> >-                      bool has_mode, enum NewImageMode mode,
> >-                      bool has_speed, int64_t speed,
> >-                      bool has_granularity, uint32_t granularity,
> >-                      bool has_buf_size, int64_t buf_size,
> >-                      bool has_on_source_error, BlockdevOnError on_source_error,
> >-                      bool has_on_target_error, BlockdevOnError on_target_error,
> >-                      Error **errp)
> >+/* Parameter check and block job starting for mirroring.
> >+ * Caller should hold @device and @target's aio context (They must to be on the
> >+ * same aio context). */
> >+static void blockdev_mirror_common(BlockDriverState *bs,
> >+                                   BlockDriverState *target,
> >+                                   bool has_replaces, const char *replaces,
> >+                                   enum MirrorSyncMode sync,
> >+                                   bool has_speed, int64_t speed,
> >+                                   bool has_granularity, uint32_t granularity,
> >+                                   bool has_buf_size, int64_t buf_size,
> >+                                   bool has_on_source_error,
> >+                                   BlockdevOnError on_source_error,
> >+                                   bool has_on_target_error,
> >+                                   BlockdevOnError on_target_error,
> >+                                   Error **errp)
> >  {
> >-    BlockBackend *blk;
> >-    BlockDriverState *bs;
> >-    BlockDriverState *source, *target_bs;
> >-    AioContext *aio_context;
> >-    BlockDriver *drv = NULL;
> >-    Error *local_err = NULL;
> >-    QDict *options = NULL;
> >-    int flags;
> >-    int64_t size;
> >-    int ret;
> >      if (!has_speed) {
> >          speed = 0;
> >@@ -2916,9 +2909,6 @@ void qmp_drive_mirror(const char *device, const char *target,
> >      if (!has_on_target_error) {
> >          on_target_error = BLOCKDEV_ON_ERROR_REPORT;
> >      }
> >-    if (!has_mode) {
> >-        mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> >-    }
> >      if (!has_granularity) {
> >          granularity = 0;
> >      }
> >@@ -2936,35 +2926,73 @@ void qmp_drive_mirror(const char *device, const char *target,
> >          return;
> >      }
> >-    blk = blk_by_name(device);
> >-    if (!blk) {
> >-        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> >-        return;
> >-    }
> >-
> >-    aio_context = blk_get_aio_context(blk);
> >-    aio_context_acquire(aio_context);
> >-
> >-    if (!blk_is_available(blk)) {
> >-        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> >-        goto out;
> >-    }
> >-    bs = blk_bs(blk);
> >-
> >-    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);
> >-            goto out;
> >-        }
> >-    }
> >-
> >      if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
> >+        return;
> >+    }
> >+
> >+    if (!bs->backing_hd && sync == MIRROR_SYNC_MODE_TOP) {
> >+        sync = MIRROR_SYNC_MODE_FULL;
> >+    }
> >+
> >+    /* pass the node name to replace to mirror start since it's loose coupling
> >+     * and will allow to check whether the node still exist at mirror completion
> >+     */
> >+    mirror_start(bs, target,
> >+                 has_replaces ? replaces : NULL,
> >+                 speed, granularity, buf_size, sync,
> >+                 on_source_error, on_target_error,
> >+                 block_job_cb, bs, errp);
> >+}
> >+
> >+void qmp_drive_mirror(const char *device, const char *target,
> >+                      bool has_format, const char *format,
> >+                      bool has_node_name, const char *node_name,
> >+                      bool has_replaces, const char *replaces,
> >+                      enum MirrorSyncMode sync,
> >+                      bool has_mode, enum NewImageMode mode,
> >+                      bool has_speed, int64_t speed,
> >+                      bool has_granularity, uint32_t granularity,
> >+                      bool has_buf_size, int64_t buf_size,
> >+                      bool has_on_source_error, BlockdevOnError on_source_error,
> >+                      bool has_on_target_error, BlockdevOnError on_target_error,
> >+                      Error **errp)
> >+{
> >+    BlockDriverState *bs;
> >+    BlockBackend *blk;
> >+    BlockDriverState *source, *target_bs;
> >+    AioContext *aio_context;
> >+    BlockDriver *drv = NULL;
> >+    Error *local_err = NULL;
> >+    QDict *options = NULL;
> >+    int flags;
> >+    int64_t size;
> >+    int ret;
> >+
> >+    blk = blk_by_name(device);
> >+    if (!blk) {
> >+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> >+        return;
> >+    }
> >+
> >+    aio_context = blk_get_aio_context(blk);
> >+    aio_context_acquire(aio_context);
> >+
> >+    if (!blk_is_available(blk)) {
> >+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> >          goto out;
> >      }
> >+    bs = blk_bs(blk);
> >+
> >+    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);
> >+            goto out;
> >+        }
> >+    }
> >      flags = bs->open_flags | BDRV_O_RDWR;
> >      source = bs->backing_hd;
> >@@ -2989,14 +3017,14 @@ void qmp_drive_mirror(const char *device, const char *target,
> >          if (!has_node_name) {
> >              error_setg(errp, "a node-name must be provided when replacing a"
> >                               " named node of the graph");
> >-            goto out;
> >+            return;
> >          }
> >          to_replace_bs = check_to_replace_node(replaces, &local_err);
> >          if (!to_replace_bs) {
> >              error_propagate(errp, local_err);
> >-            goto out;
> >+            return;
> >          }
> >          replace_aio_context = bdrv_get_aio_context(to_replace_bs);
> >@@ -3007,7 +3035,7 @@ void qmp_drive_mirror(const char *device, const char *target,
> >          if (size != replace_size) {
> >              error_setg(errp, "cannot replace image with a mirror image of "
> >                               "different size");
> >-            goto out;
> >+            return;
> >          }
> >      }
> 
> There are still a couple of "return;" statements left in the "if
> (has_replaces)" block. I think they should be replaced, too.

You're right! Will fix.

Fam

> 
> Max