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.jagade...@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
signature.asc
Description: OpenPGP digital signature
