Re: [Qemu-devel] [PATCH v4 1/6] block-commit: Expose granularity

[Fam Zheng]

On Sat, 04/12 20:57, Max Reitz wrote:
> Allow QMP users to manipulate the granularity used in the block-commit
> command.
> 
> Signed-off-by: Max Reitz <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  block/commit.c            | 16 +++++++++++++---
>  block/mirror.c            |  4 ++--
>  blockdev.c                | 22 ++++++++++++++++------
>  include/block/block_int.h | 10 ++++++----
>  qapi-schema.json          |  6 +++++-
>  5 files changed, 42 insertions(+), 16 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index acec4ac..cf0e500 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -37,6 +37,7 @@ typedef struct CommitBlockJob {
>      BlockdevOnError on_error;
>      int base_flags;
>      int orig_overlay_flags;
> +    int64_t granularity;
>  } CommitBlockJob;
>  
>  static int coroutine_fn commit_populate(BlockDriverState *bs,
> @@ -93,7 +94,7 @@ static void coroutine_fn commit_run(void *opaque)
>      }
>  
>      end = s->common.len >> BDRV_SECTOR_BITS;
> -    buf = qemu_blockalign(top, COMMIT_BUFFER_SIZE);
> +    buf = qemu_blockalign(top, s->granularity);
>  
>      for (sector_num = 0; sector_num < end; sector_num += n) {
>          uint64_t delay_ns = 0;
> @@ -109,7 +110,7 @@ wait:
>          }
>          /* Copy if allocated above the base */
>          ret = bdrv_is_allocated_above(top, base, sector_num,
> -                                      COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE,
> +                                      s->granularity / BDRV_SECTOR_SIZE,
>                                        &n);
>          copy = (ret == 1);
>          trace_commit_one_iteration(s, sector_num, n, ret);
> @@ -180,7 +181,7 @@ static const BlockJobDriver commit_job_driver = {
>  };
>  
>  void commit_start(BlockDriverState *bs, BlockDriverState *base,
> -                  BlockDriverState *top, int64_t speed,
> +                  BlockDriverState *top, int64_t speed, int64_t granularity,
>                    BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
>                    void *opaque, Error **errp)
>  {
> @@ -214,6 +215,13 @@ void commit_start(BlockDriverState *bs, BlockDriverState 
> *base,
>      orig_base_flags    = bdrv_get_flags(base);
>      orig_overlay_flags = bdrv_get_flags(overlay_bs);
>  
> +    if (!granularity) {
> +        granularity = COMMIT_BUFFER_SIZE;
> +    }
> +
> +    assert(granularity >= BDRV_SECTOR_SIZE);
> +    assert(is_power_of_2(granularity));
> +
>      /* convert base & overlay_bs to r/w, if necessary */
>      if (!(orig_base_flags & BDRV_O_RDWR)) {
>          reopen_queue = bdrv_reopen_queue(reopen_queue, base,
> @@ -244,6 +252,8 @@ void commit_start(BlockDriverState *bs, BlockDriverState 
> *base,
>      s->base_flags          = orig_base_flags;
>      s->orig_overlay_flags  = orig_overlay_flags;
>  
> +    s->granularity = granularity;
> +
>      s->on_error = on_error;
>      s->common.co = qemu_coroutine_create(commit_run);
>  
> diff --git a/block/mirror.c b/block/mirror.c
> index 0ef41f9..5b1ebb2 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -632,7 +632,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState 
> *target,
>  }
>  
>  void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
> -                         int64_t speed,
> +                         int64_t speed, int64_t granularity,
>                           BlockdevOnError on_error,
>                           BlockDriverCompletionFunc *cb,
>                           void *opaque, Error **errp)
> @@ -674,7 +674,7 @@ void commit_active_start(BlockDriverState *bs, 
> BlockDriverState *base,
>      }
>  
>      bdrv_ref(base);
> -    mirror_start_job(bs, base, speed, 0, 0,
> +    mirror_start_job(bs, base, speed, granularity, 0,
>                       on_error, on_error, cb, opaque, &local_err,
>                       &commit_active_job_driver, false, base);
>      if (error_is_set(&local_err)) {
> diff --git a/blockdev.c b/blockdev.c
> index 5dd01ea..9d7bd04 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1865,8 +1865,8 @@ void qmp_block_stream(const char *device, bool has_base,
>  
>  void qmp_block_commit(const char *device,
>                        bool has_base, const char *base, const char *top,
> -                      bool has_speed, int64_t speed,
> -                      Error **errp)
> +                      bool has_speed, int64_t speed, bool has_granularity,
> +                      int64_t granularity, Error **errp)
>  {
>      BlockDriverState *bs;
>      BlockDriverState *base_bs, *top_bs;
> @@ -1879,6 +1879,16 @@ void qmp_block_commit(const char *device,
>      if (!has_speed) {
>          speed = 0;
>      }
> +    if (!has_granularity) {
> +        granularity = 0;
> +    }
> +
> +    if (granularity &&
> +        (granularity < BDRV_SECTOR_SIZE || !is_power_of_2(granularity)))
> +    {
> +        error_set(errp, QERR_INVALID_PARAMETER, "granularity");
> +        return;
> +    }
>  
>      /* drain all i/o before commits */
>      bdrv_drain_all();
> @@ -1915,11 +1925,11 @@ void qmp_block_commit(const char *device,
>      }
>  
>      if (top_bs == bs) {
> -        commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
> -                            bs, &local_err);
> +        commit_active_start(bs, base_bs, speed, granularity, on_error,
> +                            block_job_cb, bs, &local_err);
>      } else {
> -        commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
> -                    &local_err);
> +        commit_start(bs, base_bs, top_bs, speed, granularity, on_error,
> +                     block_job_cb, bs, &local_err);
>      }
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index cd5bc73..c01b4aa 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -426,6 +426,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState 
> *base,
>   * @top: Top block device to be committed.
>   * @base: Block device that will be written into, and become the new top.
>   * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> + * @granularity: The granularity, in bytes, or 0 for a default value.
>   * @on_error: The action to take upon error.
>   * @cb: Completion function for the job.
>   * @opaque: Opaque pointer value passed to @cb.
> @@ -433,14 +434,15 @@ void stream_start(BlockDriverState *bs, 
> BlockDriverState *base,
>   *
>   */
>  void commit_start(BlockDriverState *bs, BlockDriverState *base,
> -                 BlockDriverState *top, int64_t speed,
> -                 BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
> -                 void *opaque, Error **errp);
> +                  BlockDriverState *top, int64_t speed, int64_t granularity,
> +                  BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
> +                  void *opaque, Error **errp);
>  /**
>   * commit_active_start:
>   * @bs: Active block device to be committed.
>   * @base: Block device that will be written into, and become the new top.
>   * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> + * @granularity: The granularity, in bytes, or 0 for cluster size.
>   * @on_error: The action to take upon error.
>   * @cb: Completion function for the job.
>   * @opaque: Opaque pointer value passed to @cb.
> @@ -448,7 +450,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState 
> *base,
>   *
>   */
>  void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
> -                         int64_t speed,
> +                         int64_t speed, int64_t granularity,
>                           BlockdevOnError on_error,
>                           BlockDriverCompletionFunc *cb,
>                           void *opaque, Error **errp);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 391356f..151fad3 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2112,6 +2112,10 @@
>  #
>  # @speed:  #optional the maximum speed, in bytes per second
>  #
> +# @granularity: #optional the granularity to be used for the operation, in
> +#               bytes; has to be a power of two and at least 512, or 0 for 
> the
> +#               default value (since 2.1)
> +#
>  # Returns: Nothing on success
>  #          If commit or stream is already active on this device, DeviceInUse
>  #          If @device does not exist, DeviceNotFound
> @@ -2124,7 +2128,7 @@
>  ##
>  { 'command': 'block-commit',
>    'data': { 'device': 'str', '*base': 'str', 'top': 'str',
> -            '*speed': 'int' } }
> +            '*speed': 'int', '*granularity': 'int' } }
>  
>  ##
>  # @drive-backup
> -- 
> 1.9.2
> 
> 

Reviewed-by: Fam Zheng <address@hidden>