Re: [Qemu-devel] [PATCH v3 2/4] fsdev: QMP interface for throttling

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6664 bytes --]

On 05/02/2017 09:29 AM, Pradeep Jagadeesh wrote:
> This patchset enables qmp interfaces for the fsdev
> devices. This provides two interfaces one 
> for querying info of all the fsdev devices. The second one
> to set the IO limits for the required fsdev device.
> 
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> ---

> +++ b/fsdev/qemu-fsdev-throttle.c
> @@ -29,6 +29,102 @@ static void fsdev_throttle_write_timer_cb(void *opaque)
>      qemu_co_enter_next(&fst->throttled_reqs[true]);
>  }
>  
> +void fsdev_set_io_throttle(IOThrottle *arg, FsThrottle *fst, Error **errp)
> +{
> +    ThrottleConfig cfg;
> +
> +    throttle_config_init(&cfg);
> +    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 = arg->iops;
> +    cfg.buckets[THROTTLE_OPS_READ].avg  = arg->iops_rd;
> +    cfg.buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr;
> +
> +    if (arg->has_bps_max) {
> +        cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max;
> +    }

Should the bulk of this be replaced by a call to a common IOThrottle
helper function, rather than open-coded duplication?


> +
> +void fsdev_get_io_throttle(FsThrottle *fst, IOThrottle **fs9pcfg,
> +                           char *fsdevice, Error **errp)
> +{
> +
> +    ThrottleConfig cfg = fst->cfg;
> +    IOThrottle *fscfg = g_malloc0(sizeof(*fscfg));
> +
> +    fscfg->has_id = true;
> +    fscfg->id = g_strdup(fsdevice);
> +    fscfg->bps = cfg.buckets[THROTTLE_BPS_TOTAL].avg;
> +    fscfg->bps_rd = cfg.buckets[THROTTLE_BPS_READ].avg;
> +    fscfg->bps_wr = cfg.buckets[THROTTLE_BPS_WRITE].avg;

Shouldn't you be setting has_bps, has_bps_rd, has_bps_wr, and so on, to
true?


> +++ b/fsdev/qemu-fsdev-throttle.h
> @@ -19,7 +19,14 @@
>  #include "qemu/main-loop.h"
>  #include "qemu/coroutine.h"
>  #include "qapi/error.h"
> +#include "qapi/qmp/qerror.h"
>  #include "qemu/throttle.h"
> +#include "qapi/qmp/types.h"
> +#include "qapi-visit.h"
> +#include "qapi/qobject-output-visitor.h"
> +#include "qapi/util.h"
> +#include "qmp-commands.h"
> +#include "qemu/throttle-options.h"
>  
>  typedef struct FsThrottle {
>      ThrottleState ts;
> @@ -36,4 +43,10 @@ void coroutine_fn fsdev_co_throttle_request(FsThrottle *, bool ,
>                                              struct iovec *, int);
>  
>  void fsdev_throttle_cleanup(FsThrottle *);
> +
> +void fsdev_set_io_throttle(IOThrottle *, FsThrottle *, Error **);

Even though it's not necessary per C, we tend to spell parameter names
in function declarations as it aids legibility.  (Seeing 'Error **' is
weird compared to the usual 'Error **errp')


> @@ -1570,6 +1571,80 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &err);
>  }
>  
> +#ifdef CONFIG_VIRTFS
> +
> +void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    IOThrottle throttle = {
> +        .has_id = true,
> +        .id = (char *) qdict_get_str(qdict, "id"),
> +        .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"),

Again, don't you need to be setting .has_bps=true and so on?


> +++ b/qapi/fsdev.json
> @@ -0,0 +1,84 @@
> +# -*- Mode: Python -*-
> +
> +##
> +# == QAPI fsdev definitions
> +##
> +
> +# QAPI common definitions
> +{ 'include': 'iothrottle.json' }
> +
> +##
> +# @fsdev-set-io-throttle:
> +#
> +# Change I/O limits for a 9p/fsdev device.
> +#
> +# I/O limits can be enabled by setting throttle value to non-zero number.
> +#
> +# I/O limits can be disabled by setting all throttle values to 0.
> +#
> +# Returns: Nothing on success
> +#          If @device is not a valid fsdev device, DeviceNotFound
> +#
> +# Since: 2.10
> +#
> +# Example:
> +#
> +# -> { "execute": "fsdev-set-io-throttle",
> +#      "arguments": { "id": "id0-1-0",
> +#                     "bps": 1000000,
> +#                     "bps_rd": 0,
> +#                     "bps_wr": 0,
> +#                     "iops": 0,
> +#                     "iops_rd": 0,
> +#                     "iops_wr": 0,
> +#                     "bps_max": 8000000,
> +#                     "bps_rd_max": 0,
> +#                     "bps_wr_max": 0,
> +#                     "iops_max": 0,
> +#                     "iops_rd_max": 0,
> +#                     "iops_wr_max": 0,
> +#                     "bps_max_length": 60,
> +#                     "iops_size": 0 } }
> +# <- { "returns": {} }
> +##
> +{ 'command': 'fsdev-set-io-throttle', 'boxed': true,
> +  'data': 'IOThrottle' }

This part looks okay.

> +##
> +# @query-fsdev-io-throttle:
> +#
> +# Returns: a list of @IOThrottle describing io throttle values of each fsdev device
> +#
> +# Since: 2.10
> +#
> +# Example:
> +#
> +# -> { "Execute": "query-fsdev-io-throttle" }
> +# <- { "returns" : [
> +#          {
> +#             "id": "id0-hd0",
> +#              "bps":1000000,
> +#              "bps_rd":0,
> +#              "bps_wr":0,
> +#              "iops":1000000,
> +#              "iops_rd":0,
> +#              "iops_wr":0,
> +#              "bps_max": 8000000,
> +#              "bps_rd_max": 0,
> +#              "bps_wr_max": 0,
> +#              "iops_max": 0,
> +#              "iops_rd_max": 0,
> +#              "iops_wr_max": 0,
> +#              "bps_max_length": 0,
> +#              "bps_rd_max_length": 0,
> +#              "bps_wr_max_length": 0,
> +#              "iops_max_length": 0,
> +#              "iops_rd_max_length": 0,
> +#              "iops_wr_max_length": 0,
> +#              "iops_size": 0
> +#            }
> +#          ]
> +#      }
> +#
> +##
> +{ 'command': 'query-fsdev-io-throttle', 'returns': [ 'IOThrottle' ] }
> +
> diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
> index 124ab40..698d4bc 100644
> --- a/qapi/iothrottle.json
> +++ b/qapi/iothrottle.json
> @@ -3,6 +3,7 @@
>  ##
>  # == QAPI IOThrottle definitions
>  ##
> +##
>  # @IOThrottle:

This looks like a spurious change

>  #
>  # A set of parameters describing iothrottle
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]