[Qemu-devel] [PATCH v2 0/1] drive-mirror: add incremental mode

[Qemu-devel] [PATCH v2 0/1] drive-mirror: add incremental mode

[mahaocong]

From: mahaocong <mahaocong@didichuxing.com>

This patch adds possibility to start mirroring with user-created-bitmap.
Compare with v1, this patch fix some shortcomings.
1.separate feature about copy dirty-bitmap to an individual patch.
2.remove checking for cancelled after mirror_dirty_init_incremental for bitmap
  copyimg don't have yield point.
3.adjuest input parameters on mirror_start_job and mirror_start, and so It is
  no need to find bitmap on mirror_dirty_init_incremental again.
4.assert the bitmap name is NULL on blockdev_mirror_common.
5.change the parameter's name in qmp command 'drive-mirror' from 'bitmap_name'
  to 'bitmap'.

As for the doubt about the new incremental mode, I think it is means that the
initial bitmap is appointed by user, compare with the full mode and top mode.
There is no different in other behaves, such as in iteration and complete
action.

mahaocong (1):
  drive-mirror: add incremental mode
  mirror: add incremental mode. we must add a user-named-bitmap first, then set
          this bitmap as the initial bitmap on incremental mode drive-mirror.
          in details, I first create unnamed-bitmap with the same granularity of
          user-bitmap. Next, copy It's hbitmap to unnamed-bitmap. Then, start
          mirror with this unname-bitmap.
  dirty-bitmap: add new API to copy dirty-bitmap. This feature is add in a
          separate patch called "add new function to copy dirty-bitmap"

 block/mirror.c            | 55 ++++++++++++++++++++++++++++++++++++-----------
 blockdev.c                | 37 +++++++++++++++++++++++++++++--
 include/block/block_int.h |  3 ++-
 qapi/block-core.json      |  9 +++++++-
 4 files changed, 88 insertions(+), 16 deletions(-)

-- 
2.14.1

[Qemu-devel] [PATCH v2 1/1] drive-mirror: add incremental mode

[mahaocong]

From: mahaocong <mahaocong@didichuxing.com>

Signed-off-by: mahaocong <mahaocong@didichuxing.com>
---
 block/mirror.c            | 55 ++++++++++++++++++++++++++++++++++++-----------
 blockdev.c                | 37 +++++++++++++++++++++++++++++--
 include/block/block_int.h |  3 ++-
 qapi/block-core.json      |  9 +++++++-
 4 files changed, 88 insertions(+), 16 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index ab59ad77e8..1a16a94bea 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -50,6 +50,7 @@ typedef struct MirrorBlockJob {
     /* Used to block operations on the drive-mirror-replace target */
     Error *replace_blocker;
     bool is_none_mode;
+    BdrvDirtyBitmap *src_bitmap;
     BlockMirrorBackingMode backing_mode;
     MirrorCopyMode copy_mode;
     BlockdevOnError on_source_error, on_target_error;
@@ -814,6 +815,27 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
     return 0;
 }
 
+/*
+ * init dirty bitmap by using user bitmap. usr->hbitmap will be copy to
+ * mirror bitmap->hbitmap instead of reuse it.
+ */
+static int coroutine_fn mirror_dirty_init_incremental(MirrorBlockJob *s)
+{
+    BdrvDirtyBitmap *src, *dest;
+
+    dest = s->dirty_bitmap;
+    src = s->src_bitmap;
+    if (!src) {
+        return -1;
+    }
+
+    if (!bdrv_copy_dirty_bitmap(src, dest, NULL)) {
+        return -1;
+    }
+
+    return 0;
+}
+
 /* Called when going out of the streaming phase to flush the bulk of the
  * data to the medium, or just before completing.
  */
@@ -913,9 +935,17 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
 
     s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     if (!s->is_none_mode) {
-        ret = mirror_dirty_init(s);
-        if (ret < 0 || job_is_cancelled(&s->common.job)) {
-            goto immediate_exit;
+        /* incremental mode */
+        if (s->src_bitmap) {
+            ret = mirror_dirty_init_incremental(s);
+            if (ret < 0) {
+                goto immediate_exit;
+            }
+        } else {
+            ret = mirror_dirty_init(s);
+            if (ret < 0 || job_is_cancelled(&s->common.job)) {
+                goto immediate_exit;
+            }
         }
     }
 
@@ -1484,7 +1514,8 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
                              BlockCompletionFunc *cb,
                              void *opaque,
                              const BlockJobDriver *driver,
-                             bool is_none_mode, BlockDriverState *base,
+                             bool is_none_mode, BdrvDirtyBitmap *src_bitmap,
+                             BlockDriverState *base,
                              bool auto_complete, const char *filter_node_name,
                              bool is_mirror, MirrorCopyMode copy_mode,
                              Error **errp)
@@ -1598,6 +1629,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
     s->on_source_error = on_source_error;
     s->on_target_error = on_target_error;
     s->is_none_mode = is_none_mode;
+    s->src_bitmap = src_bitmap;
     s->backing_mode = backing_mode;
     s->copy_mode = copy_mode;
     s->base = base;
@@ -1664,7 +1696,8 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *target, const char *replaces,
                   int creation_flags, int64_t speed,
                   uint32_t granularity, int64_t buf_size,
-                  MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
+                  MirrorSyncMode mode, BdrvDirtyBitmap *src_bitmap,
+                  BlockMirrorBackingMode backing_mode,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   bool unmap, const char *filter_node_name,
@@ -1673,17 +1706,14 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
     bool is_none_mode;
     BlockDriverState *base;
 
-    if (mode == MIRROR_SYNC_MODE_INCREMENTAL) {
-        error_setg(errp, "Sync mode 'incremental' not supported");
-        return;
-    }
     is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
     base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
     mirror_start_job(job_id, bs, creation_flags, target, replaces,
                      speed, granularity, buf_size, backing_mode,
                      on_source_error, on_target_error, unmap, NULL, NULL,
-                     &mirror_job_driver, is_none_mode, base, false,
-                     filter_node_name, true, copy_mode, errp);
+                     &mirror_job_driver, is_none_mode,
+                     src_bitmap, base, false, filter_node_name, true,
+                     copy_mode, errp);
 }
 
 void commit_active_start(const char *job_id, BlockDriverState *bs,
@@ -1707,7 +1737,8 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
     mirror_start_job(job_id, bs, creation_flags, base, NULL, speed, 0, 0,
                      MIRROR_LEAVE_BACKING_CHAIN,
                      on_error, on_error, true, cb, opaque,
-                     &commit_active_job_driver, false, base, auto_complete,
+                     &commit_active_job_driver, false,
+                     NULL, base, auto_complete,
                      filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
                      &local_err);
     if (local_err) {
diff --git a/blockdev.c b/blockdev.c
index a6f71f9d83..969362d36a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3661,6 +3661,8 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
                                    BlockDriverState *target,
                                    bool has_replaces, const char *replaces,
                                    enum MirrorSyncMode sync,
+                                   bool has_bitmap,
+                                   const char *bitmap_name,
                                    BlockMirrorBackingMode backing_mode,
                                    bool has_speed, int64_t speed,
                                    bool has_granularity, uint32_t granularity,
@@ -3678,6 +3680,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
                                    Error **errp)
 {
     int job_flags = JOB_DEFAULT;
+    BdrvDirtyBitmap *src_bitmap = NULL;
 
     if (!has_speed) {
         speed = 0;
@@ -3700,6 +3703,24 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
     if (!has_filter_node_name) {
         filter_node_name = NULL;
     }
+    if (!has_bitmap) {
+        bitmap_name = NULL;
+    }
+    /*
+     * In incremental mode, we should create null name bitmap by
+     * using user bitmap's granularity.
+     */
+    if (sync == MIRROR_SYNC_MODE_INCREMENTAL) {
+        assert(bitmap_name);
+        src_bitmap = bdrv_find_dirty_bitmap(bs, bitmap_name);
+        if (!src_bitmap) {
+            error_setg(errp, "Error: can't find dirty bitmap "
+                       "before start incremental drive-mirror");
+            return;
+        }
+        granularity = bdrv_dirty_bitmap_granularity(src_bitmap);
+    }
+
     if (!has_copy_mode) {
         copy_mode = MIRROR_COPY_MODE_BACKGROUND;
     }
@@ -3737,7 +3758,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
      */
     mirror_start(job_id, bs, target,
                  has_replaces ? replaces : NULL, job_flags,
-                 speed, granularity, buf_size, sync, backing_mode,
+                 speed, granularity, buf_size, sync, src_bitmap, backing_mode,
                  on_source_error, on_target_error, unmap, filter_node_name,
                  copy_mode, errp);
 }
@@ -3784,6 +3805,16 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
     if (arg->sync == MIRROR_SYNC_MODE_NONE) {
         source = bs;
     }
+    if ((arg->sync == MIRROR_SYNC_MODE_INCREMENTAL) &&
+        (!arg->has_bitmap)) {
+        error_setg(errp, "incremental mode must specify the bitmap name");
+        goto out;
+    }
+    if ((arg->sync == MIRROR_SYNC_MODE_INCREMENTAL) &&
+        (arg->has_granularity)) {
+        error_setg(errp, "incremental mode can not set bitmap granularity");
+        goto out;
+    }
 
     size = bdrv_getlength(bs);
     if (size < 0) {
@@ -3878,6 +3909,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
 
     blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs, target_bs,
                            arg->has_replaces, arg->replaces, arg->sync,
+                           arg->has_bitmap, arg->bitmap,
                            backing_mode, arg->has_speed, arg->speed,
                            arg->has_granularity, arg->granularity,
                            arg->has_buf_size, arg->buf_size,
@@ -3935,7 +3967,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
     bdrv_set_aio_context(target_bs, aio_context);
 
     blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs,
-                           has_replaces, replaces, sync, backing_mode,
+                           has_replaces, replaces, sync, false, NULL,
+                           backing_mode,
                            has_speed, speed,
                            has_granularity, granularity,
                            has_buf_size, buf_size,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f605622216..57a441f992 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1054,7 +1054,8 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *target, const char *replaces,
                   int creation_flags, int64_t speed,
                   uint32_t granularity, int64_t buf_size,
-                  MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
+                  MirrorSyncMode mode, BdrvDirtyBitmap *src_bitmap,
+                  BlockMirrorBackingMode backing_mode,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   bool unmap, const char *filter_node_name,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 762000f31f..2a453c08ce 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1727,6 +1727,13 @@
 #        (all the disk, only the sectors allocated in the topmost image, or
 #        only new I/O).
 #
+# @bitmap: the name of user-created-bitmap which is used to initialize the
+#          unnamed-bitmap used on incremental drive-mirror. It's hbitmap
+#          will be copy to mirror bitmap. If user select incremental mode,
+#          bitmap-name should not be null, and can not set granularity for
+#          the mirror bitmap should have the same granularity with
+#          user-created-bitmap.
+#
 # @granularity: 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
@@ -1768,7 +1775,7 @@
 { 'struct': 'DriveMirror',
   'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
             '*format': 'str', '*node-name': 'str', '*replaces': 'str',
-            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
+            'sync': 'MirrorSyncMode', '*bitmap': 'str', '*mode': 'NewImageMode',
             '*speed': 'int', '*granularity': 'uint32',
             '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError',
-- 
2.14.1

Re: [Qemu-devel] [PATCH v2 1/1] drive-mirror: add incremental mode

[Eric Blake]

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

On 1/2/19 4:04 AM, mahaocong wrote:
> From: mahaocong <mahaocong@didichuxing.com>
> 
> Signed-off-by: mahaocong <mahaocong@didichuxing.com>

The subject line says "what" changed, but you are missing a commit body
that says "why" the change is useful.  Much of the content in your cover
letter should be copied into this commit body, as the cover letter does
not get stored into git.

> ---
>  block/mirror.c            | 55 ++++++++++++++++++++++++++++++++++++-----------
>  blockdev.c                | 37 +++++++++++++++++++++++++++++--
>  include/block/block_int.h |  3 ++-
>  qapi/block-core.json      |  9 +++++++-
>  4 files changed, 88 insertions(+), 16 deletions(-)
> 

> +++ b/qapi/block-core.json
> @@ -1727,6 +1727,13 @@
>  #        (all the disk, only the sectors allocated in the topmost image, or
>  #        only new I/O).
>  #
> +# @bitmap: the name of user-created-bitmap which is used to initialize the
> +#          unnamed-bitmap used on incremental drive-mirror. It's hbitmap

"It's" should only be used when you could also have said "It is" or "It
has" - but in this sentence, you are using a possessive which is spelled
"Its".  However, the entire concept of an hbitmap is an internal
implementation detail, so I don't know if it makes sense to be
mentioning an hbitmap in this location.

> +#          will be copy to mirror bitmap. If user select incremental mode,
> +#          bitmap-name should not be null, and can not set granularity for
> +#          the mirror bitmap should have the same granularity with
> +#          user-created-bitmap.

Missing a '(since 4.0)' tag.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 0/1] drive-mirror: add incremental mode

[Eric Blake]

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

On 1/2/19 4:04 AM, mahaocong wrote:
> From: mahaocong <mahaocong@didichuxing.com>
> 
> This patch adds possibility to start mirroring with user-created-bitmap.
> Compare with v1, this patch fix some shortcomings.
> 1.separate feature about copy dirty-bitmap to an individual patch.
> 2.remove checking for cancelled after mirror_dirty_init_incremental for bitmap
>   copyimg don't have yield point.
> 3.adjuest input parameters on mirror_start_job and mirror_start, and so It is
>   no need to find bitmap on mirror_dirty_init_incremental again.
> 4.assert the bitmap name is NULL on blockdev_mirror_common.
> 5.change the parameter's name in qmp command 'drive-mirror' from 'bitmap_name'
>   to 'bitmap'.
> 
> As for the doubt about the new incremental mode, I think it is means that the
> initial bitmap is appointed by user, compare with the full mode and top mode.
> There is no different in other behaves, such as in iteration and complete
> action.
> 
> mahaocong (1):
>   drive-mirror: add incremental mode
>   mirror: add incremental mode. we must add a user-named-bitmap first, then set
>           this bitmap as the initial bitmap on incremental mode drive-mirror.
>           in details, I first create unnamed-bitmap with the same granularity of
>           user-bitmap. Next, copy It's hbitmap to unnamed-bitmap. Then, start
>           mirror with this unname-bitmap.
>   dirty-bitmap: add new API to copy dirty-bitmap. This feature is add in a
>           separate patch called "add new function to copy dirty-bitmap"

This is confusing. The first line 'mahaocong (1):' looks like you are
sending a patch series with only one patch (by the way, you can use 'git
commit format.coverletter auto' to only send a 0/N cover letter for N >
1, since a single patch is the only case where you don't technically
need a cover letter).  But the remaining three entries make it look like
you have three patches intended to be sent (and where the patch titled
"mirror: add incremental mode. we must..." is an awfully long subject line).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 0/1] drive-mirror: add incremental mode

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190102100415.24680-1-mahaocong_work@163.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-quick@centos7 SHOW_ENV=1 J=8
=== TEST SCRIPT END ===

libpmem support   no
libudev           no

WARNING: Use of SDL 1.2 is deprecated and will be removed in
WARNING: future releases. Please switch to using SDL 2.0

NOTE: cross-compilers enabled:  'cc'
  GEN     x86_64-softmmu/config-devices.mak.tmp
---
  CC      block/io.o
  CC      block/create.o
/tmp/qemu-test/src/block/mirror.c: In function 'mirror_dirty_init_incremental':
/tmp/qemu-test/src/block/mirror.c:832:5: error: implicit declaration of function 'bdrv_copy_dirty_bitmap' [-Werror=implicit-function-declaration]
     if (!bdrv_copy_dirty_bitmap(src, dest, NULL)) {
     ^
/tmp/qemu-test/src/block/mirror.c:832:5: error: nested extern declaration of 'bdrv_copy_dirty_bitmap' [-Werror=nested-externs]
/tmp/qemu-test/src/block/mirror.c: At top level:
cc1: error: unrecognized command line option "-Wno-format-truncation" [-Werror]
cc1: all warnings being treated as errors
make: *** [block/mirror.o] Error 1
make: *** Waiting for unfinished jobs....


The full log is available at
http://patchew.org/logs/20190102100415.24680-1-mahaocong_work@163.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 0/1] drive-mirror: add incremental mode

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190102100415.24680-1-mahaocong_work@163.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=8
=== TEST SCRIPT END ===

  CC      block/sheepdog.o
  CC      block/accounting.o
/tmp/qemu-test/src/block/mirror.c: In function 'mirror_dirty_init_incremental':
/tmp/qemu-test/src/block/mirror.c:832:10: error: implicit declaration of function 'bdrv_copy_dirty_bitmap'; did you mean 'bdrv_clear_dirty_bitmap'? [-Werror=implicit-function-declaration]
     if (!bdrv_copy_dirty_bitmap(src, dest, NULL)) {
          ^~~~~~~~~~~~~~~~~~~~~~
          bdrv_clear_dirty_bitmap
/tmp/qemu-test/src/block/mirror.c:832:10: error: nested extern declaration of 'bdrv_copy_dirty_bitmap' [-Werror=nested-externs]
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: block/mirror.o] Error 1
make: *** Waiting for unfinished jobs....


The full log is available at
http://patchew.org/logs/20190102100415.24680-1-mahaocong_work@163.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com