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

[滴滴云]

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