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

[Eric Blake <eblake@redhat.com>]

On 12/19/18 12:50 AM, mahaocong wrote:
> From: mahaocong <mahaocong@didichuxing.com>
> 
> Signed-off-by: mahaocong <mahaocong@didichuxing.com>

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