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

[Vladimir Sementsov-Ogievskiy]

Hi!

it's a very brief review, but hope it will help.

20.12.2018 11:38, mahaocong wrote:
> From: mahaocong <address@hidden>
> 
> Signed-off-by: mahaocong <address@hidden>
> ---
>   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         |  9 ++++++-
>   util/hbitmap.c               | 28 ++++++++++++++++++++
>   8 files changed, 141 insertions(+), 17 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 89fd1d7f8b..118447d0db 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -348,6 +348,20 @@ BdrvDirtyBitmap 
> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>       return ret;
>   }
>   
> +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap_locked(BdrvDirtyBitmap *src,
> +                                               BdrvDirtyBitmap *dest,
> +                                               Error **errp)

common notation in the file is opposite: _locked for functions, which need 
external locking

> +{
> +    qemu_mutex_lock(src->mutex);
> +    if (!hbitmap_copy(dest->bitmap, src->bitmap)) {
> +        error_setg(errp, "Error: copy src bitmap failed");
> +        return NULL;
> +    }
> +    qemu_mutex_unlock(src->mutex);
> +
> +    return dest;
> +}

please, keep new bitmap related feature (copy) in a separate patch

> +
>   /**
>    * Truncates _all_ bitmaps attached to a BDS.
>    * Called with BQL taken.
> diff --git a/block/mirror.c b/block/mirror.c
> index ab59ad77e8..a860fe75a7 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -50,6 +50,8 @@ typedef struct MirrorBlockJob {
>       /* Used to block operations on the drive-mirror-replace target */
>       Error *replace_blocker;
>       bool is_none_mode;
> +    bool is_inc_mode;
> +    char *bitmap_name;
>       BlockMirrorBackingMode backing_mode;
>       MirrorCopyMode copy_mode;
>       BlockdevOnError on_source_error, on_target_error;
> @@ -814,6 +816,28 @@ 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)
> +{
> +    BlockDriverState *bs = s->mirror_top_bs->backing->bs;
> +    BdrvDirtyBitmap *src, *dest;
> +
> +    dest = s->dirty_bitmap;
> +    src = bdrv_find_dirty_bitmap(bs, s->bitmap_name);
> +    if (!src) {
> +        return -1;
> +    }
> +
> +    if (!bdrv_copy_dirty_bitmap_locked(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 +937,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->is_inc_mode) {
> +            ret = mirror_dirty_init_incremental(s);
> +            if (ret < 0 || job_is_cancelled(&s->common.job)) {

bitmap copying don't have yield point, so checking for cancelled is not needed 
here

> +                goto immediate_exit;
> +            }
> +        } else {
> +            ret = mirror_dirty_init(s);
> +            if (ret < 0 || job_is_cancelled(&s->common.job)) {
> +                goto immediate_exit;
> +            }
>           }
>       }
>   
> @@ -1484,7 +1516,9 @@ 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, bool is_inc_mode,
> +                             const char *bitmap_name,
> +                             BlockDriverState *base,
>                                bool auto_complete, const char 
> *filter_node_name,
>                                bool is_mirror, MirrorCopyMode copy_mode,
>                                Error **errp)
> @@ -1598,6 +1632,8 @@ 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->is_inc_mode = is_inc_mode;
> +    s->bitmap_name = g_strdup(bitmap_name);
>       s->backing_mode = backing_mode;
>       s->copy_mode = copy_mode;
>       s->base = base;
> @@ -1648,6 +1684,7 @@ fail:
>           bdrv_ref(mirror_top_bs);
>   
>           g_free(s->replaces);
> +        g_free(s->bitmap_name);
>           blk_unref(s->target);
>           bs_opaque->job = NULL;
>           job_early_fail(&s->common.job);
> @@ -1664,26 +1701,25 @@ 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, const char *bitmap_name,
> +                  BlockMirrorBackingMode backing_mode,
>                     BlockdevOnError on_source_error,
>                     BlockdevOnError on_target_error,
>                     bool unmap, const char *filter_node_name,
>                     MirrorCopyMode copy_mode, Error **errp)
>   {
> -    bool is_none_mode;
> +    bool is_none_mode, is_inc_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;
> +    is_inc_mode = mode == MIRROR_SYNC_MODE_INCREMENTAL;
>       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, is_inc_mode,

semantics becomes strange: why not to just pass mode instead of two bools?

On the other hand, even bitmap_name != NULL is enough to understand that mode
is incremental, so we don't need new paramter

> +                     bitmap_name, base, false, filter_node_name, true,
> +                     copy_mode, errp);
>   }
>   
>   void commit_active_start(const char *job_id, BlockDriverState *bs,
> @@ -1707,7 +1743,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, 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 e6c8349409..4ac36f33dc 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3663,6 +3663,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_name,
> +                                   const char *bitmap_name,
>                                      BlockMirrorBackingMode backing_mode,
>                                      bool has_speed, int64_t speed,
>                                      bool has_granularity, uint32_t 
> granularity,
> @@ -3680,6 +3682,7 @@ static void blockdev_mirror_common(const char *job_id, 
> BlockDriverState *bs,
>                                      Error **errp)
>   {
>       int job_flags = JOB_DEFAULT;
> +    BdrvDirtyBitmap *src_bitmap;
>   
>       if (!has_speed) {
>           speed = 0;
> @@ -3702,6 +3705,23 @@ static void blockdev_mirror_common(const char *job_id, 
> BlockDriverState *bs,
>       if (!has_filter_node_name) {
>           filter_node_name = NULL;
>       }
> +    if (!has_bitmap_name) {
> +        bitmap_name = NULL;
> +    }
> +    /*
> +     * In incremental mode, we should create null name bitmap by
> +     * using user bitmap's granularity.
> +     */
> +    if (sync == MIRROR_SYNC_MODE_INCREMENTAL && bitmap_name != NULL) {

I think, these two ^^^ conditions must be equal, so, better assert it than 
check both

> +        src_bitmap = bdrv_find_dirty_bitmap(bs, bitmap_name);

and if we have to find bitmap here, why to ...

> +        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;
>       }
> @@ -3739,7 +3759,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, bitmap_name, 
> backing_mode,

... still pass bitmap_name here, and then find bitmap again?

>                    on_source_error, on_target_error, unmap, filter_node_name,
>                    copy_mode, errp);
>   }
> @@ -3786,6 +3806,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_name)) {
> +        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) {
> @@ -3880,6 +3910,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_name, arg->bitmap_name,
>                              backing_mode, arg->has_speed, arg->speed,
>                              arg->has_granularity, arg->granularity,
>                              arg->has_buf_size, arg->buf_size,
> @@ -3937,7 +3968,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..2f4725e1c5 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, const char *bitmap_name,
> +                  BlockMirrorBackingMode backing_mode,
>                     BlockdevOnError on_source_error,
>                     BlockdevOnError on_target_error,
>                     bool unmap, const char *filter_node_name,
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 8f38a3dec1..d354d8f950 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -21,6 +21,9 @@ BdrvDirtyBitmap 
> *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>   BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>                                              BdrvDirtyBitmap *bitmap,
>                                              Error **errp);
> +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap_locked(BdrvDirtyBitmap *src,
> +                                               BdrvDirtyBitmap *dest,
> +                                               Error **errp);
>   void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
>   BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
>                                           const char *name);
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index a7cb780592..6de3b4eb7e 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -91,6 +91,8 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, 
> HBitmap *result);
>    */
>   bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b);
>   
> +bool hbitmap_copy(HBitmap *dst, const HBitmap *src);
> +
>   /**
>    * hbitmap_empty:
>    * @hb: HBitmap to operate on.
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 762000f31f..7230dfb40b 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-name: 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.

1. agree with Eric, that it should be just @bitmap.

2. I don't sure in common semantics of new incremental mode: you implement it 
in the way
    you describe, just init mirror bitmap from user chosen bitmap. But if guest 
writes to
    the non-dirty range of given bitmap, this data will be mirrored. However, 
as I understand
    from your cover letter, you don't need such behavior.

> +#
>   # @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-name': 'str', '*mode': 
> 'NewImageMode',
>               '*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 8d402c59d9..7ae2fc270c 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -765,6 +765,34 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, 
> HBitmap *result)
>       return true;
>   }
>   
> +/*
> + * Copy HBitmaps form src to dst.
> + *
> + * @return true if the copy was successful,
> + *         false if it was not attempted.
> + */
> +bool hbitmap_copy(HBitmap *dst, const HBitmap *src)
> +{
> +    int i;
> +
> +    if ((dst->size != src->size) || (dst->granularity != src->granularity)) {
> +        return false;
> +    }
> +
> +    if (hbitmap_count(src) == 0) {
> +        return true;
> +    }
> +
> +    for (i = HBITMAP_LEVELS - 1; i >= 0; i--) {
> +        memcpy(dst->levels[i], src->levels[i],
> +               src->sizes[i] * sizeof(unsigned long));
> +    }
> +
> +    dst->count = src->count;
> +    return true;
> +}
> +
> +
>   HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size)
>   {
>       assert(!(chunk_size & (chunk_size - 1)));
> 


-- 
Best regards,
Vladimir