Re: [Qemu-block] migration: add incremental drive-mirror and blockdev-mirror with dirtymap

[John Snow]

On 05/02/2017 10:26 AM, Daniel Kučera wrote:
> added parameter bitmap which will be used instead of newly created
> dirtymap in mirror_start_job
> 

What's the anticipated way in which this will be used? Do you have an
example?

Please see Eric's comments and resubmit with proper formatting to:

address@hidden
address@hidden
address@hidden   (blockdev maintainer)
address@hidden  (blockdev maintainer)
address@hidden   (Blockjobs Maintainer)

as per outout of scripts/get_maintainer.pl.
Please also copy me!

For tips and tricks on submitting patches:

http://wiki.qemu.org/Contribute/SubmitAPatch

Thanks,
--John

> diff --git i/block/mirror.c w/block/mirror.c
> index 9f5eb69..dc43d69 100644
> --- i/block/mirror.c
> +++ w/block/mirror.c
> @@ -49,7 +49,7 @@ typedef struct MirrorBlockJob {
>      BlockDriverState *to_replace;
>      /* Used to block operations on the drive-mirror-replace target */
>      Error *replace_blocker;
> -    bool is_none_mode;
> +    MirrorSyncMode sync_mode;
>      BlockMirrorBackingMode backing_mode;
>      BlockdevOnError on_source_error, on_target_error;
>      bool synced;
> @@ -523,7 +523,9 @@ static void mirror_exit(BlockJob *job, void *opaque)
>      bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
>                              &error_abort);
>      if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> -        BlockDriverState *backing = s->is_none_mode ? src : s->base;
> +        BlockDriverState *backing =
> +        (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) ||
> +        (s->sync_mode == MIRROR_SYNC_MODE_NONE) ? src : s->base;
>          if (backing_bs(target_bs) != backing) {
>              bdrv_set_backing_hd(target_bs, backing, &local_err);
>              if (local_err) {
> @@ -771,7 +773,8 @@ static void coroutine_fn mirror_run(void *opaque)
>      mirror_free_init(s);
>  
>      s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> -    if (!s->is_none_mode) {
> +    if ((s->sync_mode != MIRROR_SYNC_MODE_INCREMENTAL) &&
> +      (s->sync_mode != MIRROR_SYNC_MODE_NONE)) {
>          ret = mirror_dirty_init(s);
>          if (ret < 0 || block_job_is_cancelled(&s->common)) {
>              goto immediate_exit;
> @@ -1114,7 +1117,8 @@ static void mirror_start_job(const char *job_id,
> BlockDriverState *bs,
>                               BlockCompletionFunc *cb,
>                               void *opaque,
>                               const BlockJobDriver *driver,
> -                             bool is_none_mode, BlockDriverState *base,
> +                             MirrorSyncMode sync_mode, const char *bitmap,
> +                             BlockDriverState *base,
>                               bool auto_complete, const char
> *filter_node_name,
>                               Error **errp)
>  {
> @@ -1203,7 +1207,7 @@ static void mirror_start_job(const char *job_id,
> BlockDriverState *bs,
>      s->replaces = g_strdup(replaces);
>      s->on_source_error = on_source_error;
>      s->on_target_error = on_target_error;
> -    s->is_none_mode = is_none_mode;
> +    s->sync_mode = sync_mode;
>      s->backing_mode = backing_mode;
>      s->base = base;
>      s->granularity = granularity;
> @@ -1213,9 +1217,16 @@ static void mirror_start_job(const char *job_id,
> BlockDriverState *bs,
>          s->should_complete = true;
>      }
>  
> -    s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL,
> errp);
> -    if (!s->dirty_bitmap) {
> -        goto fail;
> +    if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
> +      s->dirty_bitmap = bdrv_find_dirty_bitmap(bs, bitmap);
> +      if (!s->dirty_bitmap) {
> +          goto fail;
> +      }
> +    } else {
> +      s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL,
> errp);
> +      if (!s->dirty_bitmap) {
> +          goto fail;
> +      }
>      }
>  
>      /* Required permissions are already taken with blk_new() */
> @@ -1265,24 +1276,19 @@ fail:
>  void mirror_start(const char *job_id, BlockDriverState *bs,
>                    BlockDriverState *target, const char *replaces,
>                    int64_t speed, uint32_t granularity, int64_t buf_size,
> -                  MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
> +                  MirrorSyncMode mode, const char *bitmap,
> +                  BlockMirrorBackingMode backing_mode,
>                    BlockdevOnError on_source_error,
>                    BlockdevOnError on_target_error,
>                    bool unmap, const char *filter_node_name, Error **errp)
>  {
> -    bool is_none_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;
>      base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
>      mirror_start_job(job_id, bs, BLOCK_JOB_DEFAULT, 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,
> +                     &mirror_job_driver, mode, bitmap, base, false,
>                       filter_node_name, errp);
>  }
>  
> @@ -1305,7 +1311,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, MIRROR_SYNC_MODE_FULL,
> +                     NULL, base, auto_complete,
>                       filter_node_name, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> diff --git i/blockdev.c w/blockdev.c
> index 6428206..2569924 100644
> --- i/blockdev.c
> +++ w/blockdev.c
> @@ -3379,6 +3379,7 @@ static void blockdev_mirror_common(const char
> *job_id, BlockDriverState *bs,
>                                     enum MirrorSyncMode sync,
>                                     BlockMirrorBackingMode backing_mode,
>                                     bool has_speed, int64_t speed,
> +                                   bool has_bitmap, const char *bitmap,
>                                     bool has_granularity, uint32_t
> granularity,
>                                     bool has_buf_size, int64_t buf_size,
>                                     bool has_on_source_error,
> @@ -3440,7 +3441,7 @@ static void blockdev_mirror_common(const char
> *job_id, BlockDriverState *bs,
>       */
>      mirror_start(job_id, bs, target,
>                   has_replaces ? replaces : NULL,
> -                 speed, granularity, buf_size, sync, backing_mode,
> +                 speed, granularity, buf_size, sync, bitmap, backing_mode,
>                   on_source_error, on_target_error, unmap, filter_node_name,
>                   errp);
>  }
> @@ -3575,6 +3576,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,
>                             backing_mode, arg->has_speed, arg->speed,
> +                           arg->has_bitmap, arg->bitmap,
>                             arg->has_granularity, arg->granularity,
>                             arg->has_buf_size, arg->buf_size,
>                             arg->has_on_source_error, arg->on_source_error,
> @@ -3593,6 +3595,7 @@ void qmp_blockdev_mirror(bool has_job_id, const
> char *job_id,
>                           bool has_replaces, const char *replaces,
>                           MirrorSyncMode sync,
>                           bool has_speed, int64_t speed,
> +                         bool has_bitmap, const char *bitmap,
>                           bool has_granularity, uint32_t granularity,
>                           bool has_buf_size, int64_t buf_size,
>                           bool has_on_source_error,
> @@ -3627,6 +3630,7 @@ void qmp_blockdev_mirror(bool has_job_id, const
> char *job_id,
>      blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs,
>                             has_replaces, replaces, sync, backing_mode,
>                             has_speed, speed,
> +                           has_bitmap, bitmap,
>                             has_granularity, granularity,
>                             has_buf_size, buf_size,
>                             has_on_source_error, on_source_error,
> diff --git i/include/block/block_int.h w/include/block/block_int.h
> index 4f8cd29..3c59d69 100644
> --- i/include/block/block_int.h
> +++ w/include/block/block_int.h
> @@ -827,6 +827,7 @@ void commit_active_start(const char *job_id,
> BlockDriverState *bs,
>   * @granularity: The chosen granularity for the dirty bitmap.
>   * @buf_size: The amount of data that can be in flight at one time.
>   * @mode: Whether to collapse all images in the chain to the target.
> + * @bitmap: The dirty bitmap if sync_mode is MIRROR_SYNC_MODE_INCREMENTAL.
>   * @backing_mode: How to establish the target's backing chain after
> completion.
>   * @on_source_error: The action to take upon error reading from the source.
>   * @on_target_error: The action to take upon error writing to the target.
> @@ -844,7 +845,8 @@ void commit_active_start(const char *job_id,
> BlockDriverState *bs,
>  void mirror_start(const char *job_id, BlockDriverState *bs,
>                    BlockDriverState *target, const char *replaces,
>                    int64_t speed, uint32_t granularity, int64_t buf_size,
> -                  MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
> +                  MirrorSyncMode mode, const char *bitmap,
> +                  BlockMirrorBackingMode backing_mode,
>                    BlockdevOnError on_source_error,
>                    BlockdevOnError on_target_error,
>                    bool unmap, const char *filter_node_name, Error **errp);
> diff --git i/qapi/block-core.json w/qapi/block-core.json
> index 87fb747..df4c2e7 100644
> --- i/qapi/block-core.json
> +++ w/qapi/block-core.json
> @@ -1502,6 +1502,10 @@
>  #
>  # @speed:  the maximum speed, in bytes per second
>  #
> +# @bitmap: the name of dirty bitmap if sync is "incremental".
> +#          Must be present if sync is "incremental", must NOT be present
> +#          otherwise. (Since 2.4)
> +#
>  # @sync: what parts of the disk image should be copied to the destination
>  #        (all the disk, only the sectors allocated in the topmost image, or
>  #        only new I/O).
> @@ -1533,7 +1537,7 @@
>    'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
>              '*format': 'str', '*node-name': 'str', '*replaces': 'str',
>              'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> -            '*speed': 'int', '*granularity': 'uint32',
> +            '*speed': 'int', '*bitmap': 'str', '*granularity': 'uint32',
>              '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
>              '*on-target-error': 'BlockdevOnError',
>              '*unmap': 'bool' } }

Please note that any QMP changes *must* be accompanied by new tests.

> @@ -1652,6 +1656,10 @@
>  #
>  # @speed:  the maximum speed, in bytes per second
>  #
> +# @bitmap: the name of dirty bitmap if sync is "incremental".
> +#          Must be present if sync is "incremental", must NOT be present
> +#          otherwise. (Since 2.4)
> +#
>  # @sync: what parts of the disk image should be copied to the destination
>  #        (all the disk, only the sectors allocated in the topmost image, or
>  #        only new I/O).
> @@ -1694,7 +1702,7 @@
>    'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
>              '*replaces': 'str',
>              'sync': 'MirrorSyncMode',
> -            '*speed': 'int', '*granularity': 'uint32',
> +            '*speed': 'int', '*bitmap': 'str', '*granularity': 'uint32',
>              '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
>              '*on-target-error': 'BlockdevOnError',
>              '*filter-node-name': 'str' } }
>