[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: |
滴滴云 |
Subject: |
Re: [Qemu-devel] [PATCH v1] migration: add incremental mode on drive-mirror |
Date: |
Mon, 24 Dec 2018 09:33:50 +0000 |
I redescribe details on cover letter and send a new mail in December 20, 2018.
Please help me review again, thank you.
在 2018/12/19 下午10:48,“Eric Blake”<address@hidden> 写入:
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