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

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

[Fam Zheng]

v2: 01: Move bdrv_op_block_all down. [Max]
    02, 04: Add Max's rev-by.
    03: Check has_mode and fix "return;". [Max]
    05: Check target->blk.
        Drop superfluous whitespace. [Max]
    06: Drop superfluous whitespace hunk and add Max's rev-by. [Max]

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                      | 175 ++++++++++++++++++++++++++++++----------
 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          |  99 ++++++++++++++++++-----
 tests/qemu-iotests/041.out      |   4 +-
 8 files changed, 314 insertions(+), 66 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v2 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..0afe585 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);
@@ -695,6 +696,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     if (!s->dirty_bitmap) {
         return;
     }
+    bdrv_op_block_all(target, s->common.blocker);
     bdrv_set_enable_write_cache(s->target, true);
     if (s->target->blk) {
         blk_set_on_error(s->target->blk, on_target_error, on_target_error);
-- 
2.4.3

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

[Max Reitz]

On 25.06.2015 05:22, 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(+)

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

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Max Reitz <mreitz@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.3

[Qemu-devel] [PATCH v2 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 | 155 ++++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 92 insertions(+), 63 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b573e56..2a0c0e2 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,76 @@ 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_mode) {
+        mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+    }
+
+    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;
@@ -3057,20 +3088,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.3

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

[Max Reitz]

On 25.06.2015 05:22, Fam Zheng wrote:
> This is the part that will be reused by blockdev-mirror.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   blockdev.c | 155 ++++++++++++++++++++++++++++++++++++-------------------------
>   1 file changed, 92 insertions(+), 63 deletions(-)

As you probably know (since you introduced it), this needs fixup due to 
0fc9f8ea28.
(And the changes are not trivial, unfortunately, so I had to give up 
fixing it locally.)

Generally, the set of changes looks good to me, though.

Max

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

[Fam Zheng]

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

diff --git a/blockdev.c b/blockdev.c
index 2a0c0e2..de6383f 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.3

[Qemu-devel] [PATCH v2 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           | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx      | 48 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 156 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index de6383f..e0dba07 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2932,6 +2932,10 @@ static void blockdev_mirror_common(BlockDriverState *bs,
     if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) {
         return;
     }
+    if (target->blk) {
+        error_setg(errp, "Cannot mirror to an attached block device");
+        return;
+    }
 
     if (!bs->backing_hd && sync == MIRROR_SYNC_MODE_TOP) {
         sync = MIRROR_SYNC_MODE_FULL;
@@ -3107,6 +3111,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..fe440e1 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.3

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

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

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

Reviewing only the QAPI/QMP interface.

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b5c4559..fe440e1 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

If you abbreviate, go all the way and call it bufsize.  But we tend to
avoid abbreviations in QAPI/QMP, so make it buffer-size, please.

> +#
> +# @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.

All commands return an error on failure, that's the nature of the
protocol.  It's an error *reply*, though, not just a *message*.

I'd drop this sentence.

> +#
> +# 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)

buf_size doesn't match qapi-schema.json's buf-size.

> +- "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,

Fix at least the name mismatch between qmp-commands.hx and
block-core.json, and you can add

Acked-by: Markus Armbruster <armbru@redhat.com>

Only Acked- because my review is partial.

Adressing my other nitpicks is desirable, but I'm not insisting on it.

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

[Max Reitz]

On 25.06.2015 05:22, 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           | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++
>   qmp-commands.hx      | 48 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 156 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index de6383f..e0dba07 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2932,6 +2932,10 @@ static void blockdev_mirror_common(BlockDriverState *bs,
>       if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) {
>           return;
>       }
> +    if (target->blk) {
> +        error_setg(errp, "Cannot mirror to an attached block device");
> +        return;
> +    }

1) Why?

2) I think this should be noted in the QMP interface documentation. "the 
name of the device to mirror to" sounds like it is actually meant to be 
an attached block device.

Max

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

[Fam Zheng]

On Mon, 07/20 17:33, Max Reitz wrote:
> On 25.06.2015 05:22, 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           | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++
> >  qmp-commands.hx      | 48 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 156 insertions(+)
> >
> >diff --git a/blockdev.c b/blockdev.c
> >index de6383f..e0dba07 100644
> >--- a/blockdev.c
> >+++ b/blockdev.c
> >@@ -2932,6 +2932,10 @@ static void blockdev_mirror_common(BlockDriverState *bs,
> >      if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) {
> >          return;
> >      }
> >+    if (target->blk) {
> >+        error_setg(errp, "Cannot mirror to an attached block device");
> >+        return;
> >+    }
> 
> 1) Why?

To match the limitation of drive-mirror. We don't yet have a block job that
writes to attached device yet (except for stream, but that's only copy on
read). I've no idea what that implies, and I don't know if there is even a use
case.

> 
> 2) I think this should be noted in the QMP interface documentation. "the
> name of the device to mirror to" sounds like it is actually meant to be an
> attached block device.

I'll update the documentation.

Fam

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

[Fam Zheng]

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

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 59a8f73..f708a06 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
 
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.3

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

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2015 at 11:22:44AM +0800, Fam Zheng wrote:
> 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=").

Probably QEMU 2.5 material.  I'm not planning to merge "blockdev:
BlockBackend and media" before Kevin returns from vacation.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]