qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v1] migration: add incremental mode on drive-mir


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v1] migration: add incremental mode on drive-mirror
Date: Wed, 19 Dec 2018 08:47:50 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1

On 12/19/18 12:50 AM, mahaocong wrote:
From: mahaocong <address@hidden>

Signed-off-by: mahaocong <address@hidden>

The subject line tells "what", but you are missing a commit message body that gives the "why". Details about the new feature, and how it is useful, go a long way to making the code easier to review. How much of your cover letter should be here as well?

At a first glance, I'm just going to focus on the interface:

---
  block/dirty-bitmap.c         | 14 ++++++++++
  block/mirror.c               | 63 +++++++++++++++++++++++++++++++++++---------
  blockdev.c                   | 36 +++++++++++++++++++++++--
  include/block/block_int.h    |  3 ++-
  include/block/dirty-bitmap.h |  3 +++
  include/qemu/hbitmap.h       |  2 ++
  qapi/block-core.json         |  4 ++-
  util/hbitmap.c               | 28 ++++++++++++++++++++
  8 files changed, 136 insertions(+), 17 deletions(-)


+++ b/qapi/block-core.json
@@ -1727,6 +1727,8 @@
  #        (all the disk, only the sectors allocated in the topmost image, or
  #        only new I/O).
  #
+# @bitmap-name: the bitmap to be used in incremental mode.

Missing a '(since 4.0)' tag. And doesn't really explain what incremental mode in a mirroring operation actually means.

+#
  # @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 +1770,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-name': 'str', '*mode': 
'NewImageMode',

Can this be named just 'bitmap', instead of the longer 'bitmap-name'?

              '*speed': 'int', '*granularity': 'uint32',
              '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
              '*on-target-error': 'BlockdevOnError',
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 8d402c5..7ae2fc2 100644
--- a/util/hbitmap.c


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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]