| Message ID | 1499182263-19139-7-git-send-email-pradeep.jagadeesh@huawei.com |
|---|---|
| State | New |
| Headers | show |
Pradeep Jagadeesh <pradeepkiruvale@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.jagadeesh@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 */
On 7/6/2017 8:47 PM, Markus Armbruster wrote: > Pradeep Jagadeesh <pradeepkiruvale@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.jagadeesh@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? Yes, that is right. I did not find a way to include fsdev.json conditionally. If I can do that, mostly can avoid many dump functions that are added in qmp.c,monitor.c and qemu-fsdev-dummy.c >> +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. I will fix. > >> +} >> + >> +IOThrottleList *qmp_query_fsdev_io_throttle(Error **errp) >> +{ >> + abort(); >> +} > > Any particular reason why one of the stubs abort()s, but not the other? Just to avoid proceeding further. > >> 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? > May be in the next patch. >> + >> + *fs9pcfg = fscfg; >> +} > > Why do you need to store through a parameter? What's wrong with simply > returning fscfg? > OK >> + >> 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. OK > >> >> 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? You mean returning error or printing error message? > >> + } >> + >> + 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. OK > >> + fsdev_get_io_throttle(&fsle->fse.fst, &p_next->value, >> + fsle->fse.fsdev_id, &local_err); > > Indentation is off. Will fix > >> + 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(). ok > >> +# >> +# 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. ok > >> +# >> +# Since: 2.10 >> +# >> +# Example: >> +# >> +# -> { "Execute": "query-fsdev-io-throttle" } >> +# <- { "returns" : [ >> +# { >> +# "id": "id0-hd0", > > Indentation is off again. ok > >> +# "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. > Removed >> 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. ok > >> +} >> + >> +IOThrottleList *qmp_query_fsdev_io_throttle(Error **errp) >> +{ >> + abort(); >> +} >> + >> +#endif >> + > > Why do you need stubs both here and in fsdev/qemu-fsdev-dummy.c? Had compilation issues, as explained above, while compiling on different pltforms. -Pradeep > >> #ifndef CONFIG_VNC >> /* If VNC support is enabled, the "true" query-vnc command is >> defined in the VNC subsystem */
On 7/6/2017 8:47 PM, Markus Armbruster wrote: > Pradeep Jagadeesh <pradeepkiruvale@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.jagadeesh@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? Yes, I did not find any ways to include with some condition. > >> +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. Will fix > >> +} >> + >> +IOThrottleList *qmp_query_fsdev_io_throttle(Error **errp) >> +{ >> + abort(); >> +} > > Any particular reason why one of the stubs abort()s, but not the other? No idea, I think can even return NULL. I will change it. > >> 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? Yes, could be. I would like to do it as part of another patch. > >> + >> + *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. Done. > >> >> 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? I set errp using here an error_setg() > >> + } >> + >> + 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. ok > >> + fsdev_get_io_throttle(&fsle->fse.fst, &p_next->value, >> + fsle->fse.fsdev_id, &local_err); > > Indentation is off. Done > >> + 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(). Done > >> +# >> +# 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. Done > >> +# >> +# Since: 2.10 >> +# >> +# Example: >> +# >> +# -> { "Execute": "query-fsdev-io-throttle" } >> +# <- { "returns" : [ >> +# { >> +# "id": "id0-hd0", > > Indentation is off again. done > >> +# "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. Deleted > >> 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? As mentioned (ifdef) above just to address the issues while building for different cases. I feel this is because of uncondtional inclusion of fsdev.json in qapi-schema.json -Pradeep > >> #ifndef CONFIG_VNC >> /* If VNC support is enabled, the "true" query-vnc command is >> defined in the VNC subsystem */
Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> writes: > On 7/6/2017 8:47 PM, Markus Armbruster wrote: >> Pradeep Jagadeesh <pradeepkiruvale@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.jagadeesh@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? > Yes, I did not find any ways to include with some condition. Hiding dependencies from Make is unlikely to help. Please drop the ifdef. QAPI doesn't currently support conditional inclusion or any compile-time conditionals for that matter. Marc-André is trying to change that: "[PATCH 00/26] qapi: add #if pre-processor conditions to generated code". [...]
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 +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; +} + +IOThrottleList *qmp_query_fsdev_io_throttle(Error **errp) +{ + abort(); +} 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; + + *fs9pcfg = 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 "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" 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; + } + + 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)); + fsdev_get_io_throttle(&fsle->fse.fst, &p_next->value, + fsle->fse.fsdev_id, &local_err); + 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 +# +# 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 +# +# 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/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; +} + +IOThrottleList *qmp_query_fsdev_io_throttle(Error **errp) +{ + abort(); +} + +#endif + #ifndef CONFIG_VNC /* If VNC support is enabled, the "true" query-vnc command is defined in the VNC subsystem */
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.jagadeesh@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