Pradeep Jagadeesh <pradeepkiruv...@gmail.com> writes: > This patch introduces 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> > --- > Makefile | 4 +++ > fsdev/qemu-fsdev-dummy.c | 10 ++++++ > fsdev/qemu-fsdev-throttle.c | 76 +++++++++++++++++++++++++++++++++++++++- > fsdev/qemu-fsdev-throttle.h | 13 +++++++ > fsdev/qemu-fsdev.c | 37 ++++++++++++++++++++ > monitor.c | 5 +++ > qapi-schema.json | 3 ++ > qapi/fsdev.json | 84 > +++++++++++++++++++++++++++++++++++++++++++++ > qmp.c | 14 ++++++++ > 9 files changed, 245 insertions(+), 1 deletion(-) > create mode 100644 qapi/fsdev.json > > diff --git a/Makefile b/Makefile > index 16a0430..4fd7625 100644 > --- a/Makefile > +++ b/Makefile > @@ -416,6 +416,10 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json > $(SRC_PATH)/qapi/common.json \ > $(SRC_PATH)/qapi/crypto.json $(SRC_PATH)/qapi/rocker.json \ > $(SRC_PATH)/qapi/trace.json > > +ifdef CONFIG_VIRTFS
Uh, qapi-schema.json includes fsdev.json *unconditionally*, doesn't it? > +qapi-modules += $(SRC_PATH)/qapi/fsdev.json > +endif > + > qapi-types.c qapi-types.h :\ > $(qapi-modules) $(SRC_PATH)/scripts/qapi-types.py $(qapi-py) > $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py \ > diff --git a/fsdev/qemu-fsdev-dummy.c b/fsdev/qemu-fsdev-dummy.c > index 6dc0fbc..f33305d 100644 > --- a/fsdev/qemu-fsdev-dummy.c > +++ b/fsdev/qemu-fsdev-dummy.c > @@ -19,3 +19,13 @@ int qemu_fsdev_add(QemuOpts *opts) > { > return 0; > } > + > +void qmp_fsdev_set_io_throttle(IOThrottle *arg, Error **errp) > +{ > + return; Indentation is off. > +} > + > +IOThrottleList *qmp_query_fsdev_io_throttle(Error **errp) > +{ > + abort(); > +} Any particular reason why one of the stubs abort()s, but not the other? > diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c > index c5e2499..4483533 100644 > --- a/fsdev/qemu-fsdev-throttle.c > +++ b/fsdev/qemu-fsdev-throttle.c > @@ -16,7 +16,6 @@ > #include "qemu/error-report.h" > #include "qemu-fsdev-throttle.h" > #include "qemu/iov.h" > -#include "qemu/throttle-options.h" > > static void fsdev_throttle_read_timer_cb(void *opaque) > { > @@ -30,6 +29,81 @@ 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_set_io_limits(&cfg, arg); > + > + if (throttle_is_valid(&cfg, errp)) { > + fst->cfg = cfg; > + fsdev_throttle_init(fst); > + } > +} > + > +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; > + > + fscfg->iops = cfg.buckets[THROTTLE_OPS_TOTAL].avg; > + fscfg->iops_rd = cfg.buckets[THROTTLE_OPS_READ].avg; > + fscfg->iops_wr = cfg.buckets[THROTTLE_OPS_WRITE].avg; > + > + fscfg->has_bps_max = cfg.buckets[THROTTLE_BPS_TOTAL].max; > + fscfg->bps_max = cfg.buckets[THROTTLE_BPS_TOTAL].max; > + fscfg->has_bps_rd_max = cfg.buckets[THROTTLE_BPS_READ].max; > + fscfg->bps_rd_max = cfg.buckets[THROTTLE_BPS_READ].max; > + fscfg->has_bps_wr_max = cfg.buckets[THROTTLE_BPS_WRITE].max; > + fscfg->bps_wr_max = cfg.buckets[THROTTLE_BPS_WRITE].max; > + > + fscfg->has_iops_max = cfg.buckets[THROTTLE_OPS_TOTAL].max; > + fscfg->iops_max = cfg.buckets[THROTTLE_OPS_TOTAL].max; > + fscfg->has_iops_rd_max = cfg.buckets[THROTTLE_OPS_READ].max; > + fscfg->iops_rd_max = cfg.buckets[THROTTLE_OPS_READ].max; > + fscfg->has_iops_wr_max = cfg.buckets[THROTTLE_OPS_WRITE].max; > + fscfg->iops_wr_max = cfg.buckets[THROTTLE_OPS_WRITE].max; > + > + fscfg->has_bps_max_length = fscfg->has_bps_max; > + fscfg->bps_max_length = > + cfg.buckets[THROTTLE_BPS_TOTAL].burst_length; > + fscfg->has_bps_rd_max_length = fscfg->has_bps_rd_max; > + fscfg->bps_rd_max_length = > + cfg.buckets[THROTTLE_BPS_READ].burst_length; > + fscfg->has_bps_wr_max_length = fscfg->has_bps_wr_max; > + fscfg->bps_wr_max_length = > + cfg.buckets[THROTTLE_BPS_WRITE].burst_length; > + > + fscfg->has_iops_max_length = fscfg->has_iops_max; > + fscfg->iops_max_length = > + cfg.buckets[THROTTLE_OPS_TOTAL].burst_length; > + fscfg->has_iops_rd_max_length = fscfg->has_iops_rd_max; > + fscfg->iops_rd_max_length = > + cfg.buckets[THROTTLE_OPS_READ].burst_length; > + fscfg->has_iops_wr_max_length = fscfg->has_iops_wr_max; > + fscfg->iops_wr_max_length = > + cfg.buckets[THROTTLE_OPS_WRITE].burst_length; > + > + fscfg->bps_max_length = cfg.buckets[THROTTLE_BPS_TOTAL].burst_length; > + fscfg->bps_rd_max_length = cfg.buckets[THROTTLE_BPS_READ].burst_length; > + fscfg->bps_wr_max_length = cfg.buckets[THROTTLE_BPS_WRITE].burst_length; > + fscfg->iops_max_length = cfg.buckets[THROTTLE_OPS_TOTAL].burst_length; > + fscfg->iops_rd_max_length = cfg.buckets[THROTTLE_OPS_READ].burst_length; > + fscfg->iops_wr_max_length = cfg.buckets[THROTTLE_OPS_WRITE].burst_length; > + > + fscfg->iops_size = cfg.op_size; Duplicates bdrv_block_device_info(), which makes me sad. Could the common code be factored out? > + > + *fs9pcfg = fscfg; > +} Why do you need to store through a parameter? What's wrong with simply returning fscfg? > + > void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp) > { > throttle_parse_options(&fst->cfg, opts); > diff --git a/fsdev/qemu-fsdev-throttle.h b/fsdev/qemu-fsdev-throttle.h > index e418643..a49b2e5 100644 > --- a/fsdev/qemu-fsdev-throttle.h > +++ b/fsdev/qemu-fsdev-throttle.h > @@ -20,6 +20,13 @@ #include "block/aio.h" #include "qemu/main-loop.h" > #include "qemu/coroutine.h" > #include "qapi/error.h" > #include "qemu/throttle.h" > +#include "qemu/throttle-options.h" > +#include "qapi/qmp/qerror.h" > +#include "qapi/qmp/types.h" > +#include "qapi-visit.h" > +#include "qapi/qobject-output-visitor.h" > +#include "qapi/util.h" > +#include "qmp-commands.h" The header actually needs two out of these twelve: coroutine.h and throttle.h. Lazy use of include makes for slow compiles. Drop the ten unused ones, then fix up the .c to include what they need. > > 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 **errp); > + > +void fsdev_get_io_throttle(FsThrottle *, IOThrottle **iothp, > + char *, Error **errp); > + > #endif /* _FSDEV_THROTTLE_H */ > diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c > index 266e442..0eca7c3 100644 > --- a/fsdev/qemu-fsdev.c > +++ b/fsdev/qemu-fsdev.c > @@ -16,6 +16,7 @@ > #include "qemu-common.h" > #include "qemu/config-file.h" > #include "qemu/error-report.h" > +#include "qmp-commands.h" > > static QTAILQ_HEAD(FsDriverEntry_head, FsDriverListEntry) fsdriver_entries = > QTAILQ_HEAD_INITIALIZER(fsdriver_entries); > @@ -98,3 +99,39 @@ FsDriverEntry *get_fsdev_fsentry(char *id) > } > return NULL; > } > + > +void qmp_fsdev_set_io_throttle(IOThrottle *arg, Error **errp) > +{ > + > + FsDriverEntry *fse; > + > + fse = get_fsdev_fsentry(arg->has_id ? arg->id : NULL); > + if (!fse) { > + return; Why isn't this an error? > + } > + > + fsdev_set_io_throttle(arg, &fse->fst, errp); > +} > + > +IOThrottleList *qmp_query_fsdev_io_throttle(Error **errp) > +{ > + IOThrottleList *head = NULL, *p_next; > + struct FsDriverListEntry *fsle; > + Error *local_err = NULL; > + > + QTAILQ_FOREACH(fsle, &fsdriver_entries, next) { > + p_next = g_malloc0(sizeof(*p_next)); Okay, but you might want to consider g_new0(IOThrottleList) for a bit of extra type checking. > + fsdev_get_io_throttle(&fsle->fse.fst, &p_next->value, > + fsle->fse.fsdev_id, &local_err); Indentation is off. > + if (local_err) { > + error_propagate(errp, local_err); > + g_free(p_next); > + qapi_free_IOThrottleList(head); > + return NULL; > + } > + > + p_next->next = head; > + head = p_next; > + } > + return head; > +} > diff --git a/monitor.c b/monitor.c > index 3c369f4..592a39e 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -997,6 +997,11 @@ static void qmp_unregister_commands_hack(void) > && !defined(TARGET_S390X) > qmp_unregister_command(&qmp_commands, "query-cpu-definitions"); > #endif > +#ifndef CONFIG_VIRTFS > + qmp_unregister_command(&qmp_commands, "fsdev-set-io-throttle"); > + qmp_unregister_command(&qmp_commands, "query-fsdev-io-throttle"); > +#endif > + > } > > void monitor_init_qmp_commands(void) > diff --git a/qapi-schema.json b/qapi-schema.json > index 4b50b65..dc676be 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -81,6 +81,9 @@ > # QAPI block definitions > { 'include': 'qapi/block.json' } > > +# QAPI fsdev definitions > +{ 'include': 'qapi/fsdev.json' } > + > # QAPI event definitions > { 'include': 'qapi/event.json' } > > diff --git a/qapi/fsdev.json b/qapi/fsdev.json > new file mode 100644 > index 0000000..eff1efe > --- /dev/null > +++ 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 Aha, qmp_fsdev_set_io_throttle()'s early return should be an error! Make it GenericError rather than DeviceNotFound, please; just use error_setg(). > +# > +# 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' } > +## > +# @query-fsdev-io-throttle: > +# > +# Returns: a list of @IOThrottle describing io throttle values of each fsdev > device "io" is not a word, make it "I/O". Also: long line. Please wrap your comment lines around column 70. > +# > +# Since: 2.10 > +# > +# Example: > +# > +# -> { "Execute": "query-fsdev-io-throttle" } > +# <- { "returns" : [ > +# { > +# "id": "id0-hd0", Indentation is off again. > +# "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' ] } > + Trailing blank line. > diff --git a/qmp.c b/qmp.c > index 7ee9bcf..8a60f2c 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -130,6 +130,20 @@ void qmp_cpu_add(int64_t id, Error **errp) > } > } > > +#if defined(_WIN64) || defined(_WIN32) || defined(__FreeBSD__) > + > +void qmp_fsdev_set_io_throttle(IOThrottle *arg, Error **errp) > +{ > + return; Indentation is off. > +} > + > +IOThrottleList *qmp_query_fsdev_io_throttle(Error **errp) > +{ > + abort(); > +} > + > +#endif > + Why do you need stubs both here and in fsdev/qemu-fsdev-dummy.c? > #ifndef CONFIG_VNC > /* If VNC support is enabled, the "true" query-vnc command is > defined in the VNC subsystem */