Re: [Qemu-devel] [PATCH v2 15/16] block/mirror: Add copy mode QAPI interface

[Fam Zheng]

On Mon, 01/22 23:08, Max Reitz wrote:
> This patch allows the user to specify whether to use active or only
> passive mode for mirror block jobs.  Currently, this setting will remain

I think you want s/passive/background/ in the whole patch.

> constant for the duration of the entire block job.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  qapi/block-core.json      | 11 +++++++++--
>  include/block/block_int.h |  4 +++-
>  block/mirror.c            | 12 +++++++-----
>  blockdev.c                |  9 ++++++++-
>  4 files changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ba1fd736f5..5fafa5fcac 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1573,6 +1573,9 @@
>  #         written. Both will result in identical contents.
>  #         Default is true. (Since 2.4)
>  #
> +# @copy-mode: when to copy data to the destination; defaults to 'passive'
> +#             (Since: 2.12)
> +#
>  # Since: 1.3
>  ##
>  { 'struct': 'DriveMirror',
> @@ -1582,7 +1585,7 @@
>              '*speed': 'int', '*granularity': 'uint32',
>              '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
>              '*on-target-error': 'BlockdevOnError',
> -            '*unmap': 'bool' } }
> +            '*unmap': 'bool', '*copy-mode': 'MirrorCopyMode' } }
>  
>  ##
>  # @BlockDirtyBitmap:
> @@ -1761,6 +1764,9 @@
>  #                    above @device. If this option is not given, a node name 
> is
>  #                    autogenerated. (Since: 2.9)
>  #
> +# @copy-mode: when to copy data to the destination; defaults to 'passive'
> +#             (Since: 2.12)
> +#
>  # Returns: nothing on success.
>  #
>  # Since: 2.6
> @@ -1781,7 +1787,8 @@
>              '*speed': 'int', '*granularity': 'uint32',
>              '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
>              '*on-target-error': 'BlockdevOnError',
> -            '*filter-node-name': 'str' } }
> +            '*filter-node-name': 'str',
> +            '*copy-mode': 'MirrorCopyMode' } }
>  
>  ##
>  # @block_set_io_throttle:
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 03f3fdd129..1fda4d3d43 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -948,6 +948,7 @@ void commit_active_start(const char *job_id, 
> BlockDriverState *bs,
>   * @filter_node_name: The node name that should be assigned to the filter
>   * driver that the mirror job inserts into the graph above @bs. NULL means 
> that
>   * a node name should be autogenerated.
> + * @copy_mode: When to trigger writes to the target.
>   * @errp: Error object.
>   *
>   * Start a mirroring operation on @bs.  Clusters that are allocated
> @@ -961,7 +962,8 @@ void mirror_start(const char *job_id, BlockDriverState 
> *bs,
>                    MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
>                    BlockdevOnError on_source_error,
>                    BlockdevOnError on_target_error,
> -                  bool unmap, const char *filter_node_name, Error **errp);
> +                  bool unmap, const char *filter_node_name,
> +                  MirrorCopyMode copy_mode, Error **errp);
>  
>  /*
>   * backup_job_create:
> diff --git a/block/mirror.c b/block/mirror.c
> index 83082adb64..3b23886a5a 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1409,7 +1409,7 @@ static void mirror_start_job(const char *job_id, 
> BlockDriverState *bs,
>                               const BlockJobDriver *driver,
>                               bool is_none_mode, BlockDriverState *base,
>                               bool auto_complete, const char 
> *filter_node_name,
> -                             bool is_mirror,
> +                             bool is_mirror, MirrorCopyMode copy_mode,
>                               Error **errp)
>  {
>      MirrorBlockJob *s;
> @@ -1515,7 +1515,7 @@ static void mirror_start_job(const char *job_id, 
> BlockDriverState *bs,
>      s->on_target_error = on_target_error;
>      s->is_none_mode = is_none_mode;
>      s->backing_mode = backing_mode;
> -    s->copy_mode = MIRROR_COPY_MODE_BACKGROUND;
> +    s->copy_mode = copy_mode;
>      s->base = base;
>      s->granularity = granularity;
>      s->buf_size = ROUND_UP(buf_size, granularity);
> @@ -1582,7 +1582,8 @@ void mirror_start(const char *job_id, BlockDriverState 
> *bs,
>                    MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
>                    BlockdevOnError on_source_error,
>                    BlockdevOnError on_target_error,
> -                  bool unmap, const char *filter_node_name, Error **errp)
> +                  bool unmap, const char *filter_node_name,
> +                  MirrorCopyMode copy_mode, Error **errp)
>  {
>      bool is_none_mode;
>      BlockDriverState *base;
> @@ -1597,7 +1598,7 @@ void mirror_start(const char *job_id, BlockDriverState 
> *bs,
>                       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, errp);
> +                     filter_node_name, true, copy_mode, errp);
>  }
>  
>  void commit_active_start(const char *job_id, BlockDriverState *bs,
> @@ -1620,7 +1621,8 @@ void commit_active_start(const char *job_id, 
> BlockDriverState *bs,
>                       MIRROR_LEAVE_BACKING_CHAIN,
>                       on_error, on_error, true, cb, opaque,
>                       &commit_active_job_driver, false, base, auto_complete,
> -                     filter_node_name, false, &local_err);
> +                     filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,

I think for active commit, MIRROR_COPY_MODE_WRITE_BLOCKING is more appealing?
Can be a task for another day, though.

Fam