Re: [Qemu-devel] [PULL 02/17] qmp: Add optional bool "unmap" to drive-mirror

[Fam Zheng]

On Fri, 06/05 12:57, Stefan Hajnoczi wrote:
> From: Fam Zheng <address@hidden>
> 
> If specified as "true", it allows discarding on target sectors where source is
> not allocated.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> Reviewed-by: Paolo Bonzini <address@hidden>
> Message-id: address@hidden
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  block/mirror.c            | 7 +++++--
>  blockdev.c                | 5 +++++
>  hmp.c                     | 2 +-
>  include/block/block_int.h | 2 ++
>  qapi/block-core.json      | 8 +++++++-
>  qmp-commands.hx           | 3 +++
>  6 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 58f391a..85995b2 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -57,6 +57,7 @@ typedef struct MirrorBlockJob {
>      int in_flight;
>      int sectors_in_flight;
>      int ret;
> +    bool unmap;
>  } MirrorBlockJob;
>  
>  typedef struct MirrorOp {
> @@ -651,6 +652,7 @@ static void mirror_start_job(BlockDriverState *bs, 
> BlockDriverState *target,
>                               int64_t buf_size,
>                               BlockdevOnError on_source_error,
>                               BlockdevOnError on_target_error,
> +                             bool unmap,
>                               BlockCompletionFunc *cb,
>                               void *opaque, Error **errp,
>                               const BlockJobDriver *driver,

I forgot to assign unmap to s->unmap. I'll respin the series.

Fam

> @@ -703,6 +705,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState 
> *target,
>                    int64_t speed, uint32_t granularity, int64_t buf_size,
>                    MirrorSyncMode mode, BlockdevOnError on_source_error,
>                    BlockdevOnError on_target_error,
> +                  bool unmap,
>                    BlockCompletionFunc *cb,
>                    void *opaque, Error **errp)
>  {
> @@ -717,7 +720,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState 
> *target,
>      base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL;
>      mirror_start_job(bs, target, replaces,
>                       speed, granularity, buf_size,
> -                     on_source_error, on_target_error, cb, opaque, errp,
> +                     on_source_error, on_target_error, unmap, cb, opaque, 
> errp,
>                       &mirror_job_driver, is_none_mode, base);
>  }
>  
> @@ -765,7 +768,7 @@ void commit_active_start(BlockDriverState *bs, 
> BlockDriverState *base,
>  
>      bdrv_ref(base);
>      mirror_start_job(bs, base, NULL, speed, 0, 0,
> -                     on_error, on_error, cb, opaque, &local_err,
> +                     on_error, on_error, false, cb, opaque, &local_err,
>                       &commit_active_job_driver, false, base);
>      if (local_err) {
>          error_propagate(errp, local_err);
> diff --git a/blockdev.c b/blockdev.c
> index de94a8b..6a45555 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2631,6 +2631,7 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>                        bool has_buf_size, int64_t buf_size,
>                        bool has_on_source_error, BlockdevOnError 
> on_source_error,
>                        bool has_on_target_error, BlockdevOnError 
> on_target_error,
> +                      bool has_unmap, bool unmap,
>                        Error **errp)
>  {
>      BlockBackend *blk;
> @@ -2662,6 +2663,9 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>      if (!has_buf_size) {
>          buf_size = DEFAULT_MIRROR_BUF_SIZE;
>      }
> +    if (!has_unmap) {
> +        unmap = true;
> +    }
>  
>      if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 
> 64)) {
>          error_set(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
> @@ -2801,6 +2805,7 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>                   has_replaces ? replaces : NULL,
>                   speed, granularity, buf_size, sync,
>                   on_source_error, on_target_error,
> +                 unmap,
>                   block_job_cb, bs, &local_err);
>      if (local_err != NULL) {
>          bdrv_unref(target_bs);
> diff --git a/hmp.c b/hmp.c
> index 514f22f..d5b9ebb 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1057,7 +1057,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
>                       false, NULL, false, NULL,
>                       full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
>                       true, mode, false, 0, false, 0, false, 0,
> -                     false, 0, false, 0, &err);
> +                     false, 0, false, 0, false, true, &err);
>      hmp_handle_error(mon, &err);
>  }
>  
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f004378..4e0d700 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -590,6 +590,7 @@ void commit_active_start(BlockDriverState *bs, 
> BlockDriverState *base,
>   * @mode: Whether to collapse all images in the chain to the target.
>   * @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.
> + * @unmap: Whether to unmap target where source sectors only contain zeroes.
>   * @cb: Completion function for the job.
>   * @opaque: Opaque pointer value passed to @cb.
>   * @errp: Error object.
> @@ -604,6 +605,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState 
> *target,
>                    int64_t speed, uint32_t granularity, int64_t buf_size,
>                    MirrorSyncMode mode, BlockdevOnError on_source_error,
>                    BlockdevOnError on_target_error,
> +                  bool unmap,
>                    BlockCompletionFunc *cb,
>                    void *opaque, Error **errp);
>  
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8411d4f..71ed838 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -973,6 +973,11 @@
>  # @on-target-error: #optional the action to take on an error on the target,
>  #                   default 'report' (no limitations, since this applies to
>  #                   a different block device than @device).
> +# @unmap: #optional Whether to try to unmap target sectors where source has
> +#         only zero. If true, and target unallocated sectors will read as 
> zero,
> +#         target image sectors will be unmapped; otherwise, zeroes will be
> +#         written. Both will result in identical contents.
> +#         Default is true. (Since 2.4)
>  #
>  # Returns: nothing on success
>  #          If @device is not a valid block device, DeviceNotFound
> @@ -985,7 +990,8 @@
>              'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
>              '*speed': 'int', '*granularity': 'uint32',
>              '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> -            '*on-target-error': 'BlockdevOnError' } }
> +            '*on-target-error': 'BlockdevOnError',
> +            '*unmap': 'bool' } }
>  
>  ##
>  # @BlockDirtyBitmap
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 867a21f..3b33496 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1503,6 +1503,7 @@ EQMP
>          .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
>                        "node-name:s?,replaces:s?,"
>                        "on-source-error:s?,on-target-error:s?,"
> +                      "unmap:b?,"
>                        "granularity:i?,buf-size:i?",
>          .mhandler.cmd_new = qmp_marshal_input_drive_mirror,
>      },
> @@ -1542,6 +1543,8 @@ Arguments:
>    (BlockdevOnError, default 'report')
>  - "on-target-error": the action to take on an error on the target
>    (BlockdevOnError, default 'report')
> +- "unmap": whether the target sectors should be discarded where source has 
> only
> +  zeroes. (json-bool, optional, default true)
>  
>  The default value of the granularity is the image cluster size clamped
>  between 4096 and 65536, if the image format defines one.  If the format
> -- 
> 2.4.2
>