qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v7 08/15] block: Simplify block_set_io_throttle


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v7 08/15] block: Simplify block_set_io_throttle
Date: Tue, 14 Jun 2016 17:34:42 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Eric Blake <address@hidden> writes:

> Now that we can support boxed commands, use it to greatly
> reduce the number of parameters (and likelihood of getting
> out of sync) when adjusting throttle parameters.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v7: new patch
> ---
>  qapi/block-core.json |  20 ++++++++--
>  blockdev.c           | 111 
> +++++++++++++++++++--------------------------------
>  hmp.c                |  45 +++++----------------
>  3 files changed, 66 insertions(+), 110 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 98a20d2..26f7c0e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1312,6 +1312,21 @@
>  # the device will be removed from its group and the rest of its
>  # members will not be affected. The 'group' parameter is ignored.
>  #
> +# See BlockIOThrottle for parameter descriptions.

This comment will be trivial as soon as we got used to the 'box' feature
:)

> +#
> +# Returns: Nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#
> +# Since: 1.1
> +##
> +{ 'command': 'block_set_io_throttle', 'box': true,
> +  'data': 'BlockIOThrottle' }
> +
> +##
> +# BlockIOThrottle
> +#
> +# The parameters for the block_set_io_throttle command.

This comment is prone to go stale.

> +#
>  # @device: The name of the device
>  #
>  # @bps: total throughput limit in bytes per second
> @@ -1378,12 +1393,9 @@
>  #
>  # @group: #optional throttle group name (Since 2.4)
>  #
> -# Returns: Nothing on success
> -#          If @device is not a valid block device, DeviceNotFound
> -#
>  # Since: 1.1
>  ##
> -{ 'command': 'block_set_io_throttle',
> +{ 'struct': 'BlockIOThrottle',
>    'data': { 'device': 'str', 'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
>              'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
>              '*bps_max': 'int', '*bps_rd_max': 'int',
> diff --git a/blockdev.c b/blockdev.c
> index cf5afa3..b8db496 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2625,49 +2625,17 @@ fail:
>  }
>
>  /* throttling disk I/O limits */
> -void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t 
> bps_rd,
> -                               int64_t bps_wr,
> -                               int64_t iops,
> -                               int64_t iops_rd,
> -                               int64_t iops_wr,
> -                               bool has_bps_max,
> -                               int64_t bps_max,
> -                               bool has_bps_rd_max,
> -                               int64_t bps_rd_max,
> -                               bool has_bps_wr_max,
> -                               int64_t bps_wr_max,
> -                               bool has_iops_max,
> -                               int64_t iops_max,
> -                               bool has_iops_rd_max,
> -                               int64_t iops_rd_max,
> -                               bool has_iops_wr_max,
> -                               int64_t iops_wr_max,
> -                               bool has_bps_max_length,
> -                               int64_t bps_max_length,
> -                               bool has_bps_rd_max_length,
> -                               int64_t bps_rd_max_length,
> -                               bool has_bps_wr_max_length,
> -                               int64_t bps_wr_max_length,
> -                               bool has_iops_max_length,
> -                               int64_t iops_max_length,
> -                               bool has_iops_rd_max_length,
> -                               int64_t iops_rd_max_length,
> -                               bool has_iops_wr_max_length,
> -                               int64_t iops_wr_max_length,
> -                               bool has_iops_size,
> -                               int64_t iops_size,
> -                               bool has_group,
> -                               const char *group, Error **errp)
> +void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
>  {

This hunk together with the last one are worth a good chunk of the work
you had to do to get here :)

>      ThrottleConfig cfg;
>      BlockDriverState *bs;
>      BlockBackend *blk;
>      AioContext *aio_context;
>
> -    blk = blk_by_name(device);
> +    blk = blk_by_name(arg->device);
>      if (!blk) {
>          error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> -                  "Device '%s' not found", device);
> +                  "Device '%s' not found", arg->device);
>          return;
>      }
>
> @@ -2676,59 +2644,59 @@ void qmp_block_set_io_throttle(const char *device, 
> int64_t bps, int64_t bps_rd,
>
>      bs = blk_bs(blk);
>      if (!bs) {
> -        error_setg(errp, "Device '%s' has no medium", device);
> +        error_setg(errp, "Device '%s' has no medium", arg->device);
>          goto out;
>      }
>
>      throttle_config_init(&cfg);
> -    cfg.buckets[THROTTLE_BPS_TOTAL].avg = bps;
> -    cfg.buckets[THROTTLE_BPS_READ].avg  = bps_rd;
> -    cfg.buckets[THROTTLE_BPS_WRITE].avg = bps_wr;
> +    cfg.buckets[THROTTLE_BPS_TOTAL].avg = arg->bps;
> +    cfg.buckets[THROTTLE_BPS_READ].avg  = arg->bps_rd;
> +    cfg.buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr;
>
> -    cfg.buckets[THROTTLE_OPS_TOTAL].avg = iops;
> -    cfg.buckets[THROTTLE_OPS_READ].avg  = iops_rd;
> -    cfg.buckets[THROTTLE_OPS_WRITE].avg = iops_wr;
> +    cfg.buckets[THROTTLE_OPS_TOTAL].avg = arg->iops;
> +    cfg.buckets[THROTTLE_OPS_READ].avg  = arg->iops_rd;
> +    cfg.buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr;
>
> -    if (has_bps_max) {
> -        cfg.buckets[THROTTLE_BPS_TOTAL].max = bps_max;
> +    if (arg->has_bps_max) {
> +        cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max;
>      }
> -    if (has_bps_rd_max) {
> -        cfg.buckets[THROTTLE_BPS_READ].max = bps_rd_max;
> +    if (arg->has_bps_rd_max) {
> +        cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max;
>      }
> -    if (has_bps_wr_max) {
> -        cfg.buckets[THROTTLE_BPS_WRITE].max = bps_wr_max;
> +    if (arg->has_bps_wr_max) {
> +        cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max;
>      }
> -    if (has_iops_max) {
> -        cfg.buckets[THROTTLE_OPS_TOTAL].max = iops_max;
> +    if (arg->has_iops_max) {
> +        cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max;
>      }
> -    if (has_iops_rd_max) {
> -        cfg.buckets[THROTTLE_OPS_READ].max = iops_rd_max;
> +    if (arg->has_iops_rd_max) {
> +        cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max;
>      }
> -    if (has_iops_wr_max) {
> -        cfg.buckets[THROTTLE_OPS_WRITE].max = iops_wr_max;
> +    if (arg->has_iops_wr_max) {
> +        cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max;
>      }
>
> -    if (has_bps_max_length) {
> -        cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = bps_max_length;
> +    if (arg->has_bps_max_length) {
> +        cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length;
>      }
> -    if (has_bps_rd_max_length) {
> -        cfg.buckets[THROTTLE_BPS_READ].burst_length = bps_rd_max_length;
> +    if (arg->has_bps_rd_max_length) {
> +        cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length;
>      }
> -    if (has_bps_wr_max_length) {
> -        cfg.buckets[THROTTLE_BPS_WRITE].burst_length = bps_wr_max_length;
> +    if (arg->has_bps_wr_max_length) {
> +        cfg.buckets[THROTTLE_BPS_WRITE].burst_length = 
> arg->bps_wr_max_length;
>      }
> -    if (has_iops_max_length) {
> -        cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = iops_max_length;
> +    if (arg->has_iops_max_length) {
> +        cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length;
>      }
> -    if (has_iops_rd_max_length) {
> -        cfg.buckets[THROTTLE_OPS_READ].burst_length = iops_rd_max_length;
> +    if (arg->has_iops_rd_max_length) {
> +        cfg.buckets[THROTTLE_OPS_READ].burst_length = 
> arg->iops_rd_max_length;
>      }
> -    if (has_iops_wr_max_length) {
> -        cfg.buckets[THROTTLE_OPS_WRITE].burst_length = iops_wr_max_length;
> +    if (arg->has_iops_wr_max_length) {
> +        cfg.buckets[THROTTLE_OPS_WRITE].burst_length = 
> arg->iops_wr_max_length;
>      }
>
> -    if (has_iops_size) {
> -        cfg.op_size = iops_size;
> +    if (arg->has_iops_size) {
> +        cfg.op_size = arg->iops_size;
>      }
>
>      if (!throttle_is_valid(&cfg, errp)) {
> @@ -2739,9 +2707,10 @@ void qmp_block_set_io_throttle(const char *device, 
> int64_t bps, int64_t bps_rd,
>          /* Enable I/O limits if they're not enabled yet, otherwise
>           * just update the throttling group. */
>          if (!blk_get_public(blk)->throttle_state) {
> -            blk_io_limits_enable(blk, has_group ? group : device);
> -        } else if (has_group) {
> -            blk_io_limits_update_group(blk, group);
> +            blk_io_limits_enable(blk,
> +                                 arg->has_group ? arg->group : arg->device);
> +        } else if (arg->has_group) {
> +            blk_io_limits_update_group(blk, arg->group);
>          }
>          /* Set the new throttling configuration */
>          blk_set_io_limits(blk, &cfg);
> diff --git a/hmp.c b/hmp.c
> index 9836227..a0c3f4e 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1395,42 +1395,17 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>  void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> +    BlockIOThrottle throttle = {
> +        .device = (char *) qdict_get_str(qdict, "device"),
> +        .bps = qdict_get_int(qdict, "bps"),
> +        .bps_rd = qdict_get_int(qdict, "bps_rd"),
> +        .bps_wr = qdict_get_int(qdict, "bps_wr"),
> +        .iops = qdict_get_int(qdict, "iops"),
> +        .iops_rd = qdict_get_int(qdict, "iops_rd"),
> +        .iops_wr = qdict_get_int(qdict, "iops_wr"),
> +    };
>
> -    qmp_block_set_io_throttle(qdict_get_str(qdict, "device"),
> -                              qdict_get_int(qdict, "bps"),
> -                              qdict_get_int(qdict, "bps_rd"),
> -                              qdict_get_int(qdict, "bps_wr"),
> -                              qdict_get_int(qdict, "iops"),
> -                              qdict_get_int(qdict, "iops_rd"),
> -                              qdict_get_int(qdict, "iops_wr"),
> -                              false, /* no burst max via HMP */
> -                              0,
> -                              false,
> -                              0,
> -                              false,
> -                              0,
> -                              false,
> -                              0,
> -                              false,
> -                              0,
> -                              false,
> -                              0,
> -                              false, /* no burst length via HMP */
> -                              0,
> -                              false,
> -                              0,
> -                              false,
> -                              0,
> -                              false,
> -                              0,
> -                              false,
> -                              0,
> -                              false,
> -                              0,
> -                              false, /* No default I/O size */
> -                              0,
> -                              false,
> -                              NULL, &err);
> +    qmp_block_set_io_throttle(&throttle, &err);
>      hmp_handle_error(mon, &err);
>  }



reply via email to

[Prev in Thread] Current Thread [Next in Thread]