* [Qemu-devel] [PATCH 1/2 v1] fsdev: QMP interface for throttling
@ 2017-03-23 12:20 Pradeep Jagadeesh
2017-03-23 12:20 ` [Qemu-devel] [PATCH 2/2 v1] throttle: factor out the redundant code Pradeep Jagadeesh
2017-03-23 14:32 ` [Qemu-devel] [PATCH 1/2 v1] fsdev: QMP interface for throttling Eric Blake
0 siblings, 2 replies; 8+ messages in thread
From: Pradeep Jagadeesh @ 2017-03-23 12:20 UTC (permalink / raw)
To: Eric Blake, Greg Kurz, Daniel P.Berrange
Cc: Pradeep Jagadeesh, Alberto Garcia, Jani Kokkonen, qemu-devel
This patchset enables qmp interfaces for the 9pfs
devices (fsdev). This provides two interfaces one
for querying all the 9pfs devices info. The second one
to set the IO limits for the required 9pfs device.
Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
---
Makefile | 5 ++-
fsdev/qemu-fsdev-throttle.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
fsdev/qemu-fsdev-throttle.h | 11 ++++++
fsdev/qemu-fsdev.c | 38 +++++++++++++++++-
fsdev/qemu-fsdev.h | 2 +-
hmp-commands-info.hx | 18 +++++++++
hmp-commands.hx | 18 +++++++++
hmp.c | 74 ++++++++++++++++++++++++++++++++++
hmp.h | 5 +++
qapi-schema.json | 6 +++
qapi/fsdev.json | 87 ++++++++++++++++++++++++++++++++++++++++
qapi/iothrottle.json | 93 +++++++++++++++++++++++++++++++++++++++++++
qmp.c | 14 +++++++
13 files changed, 464 insertions(+), 3 deletions(-)
create mode 100644 qapi/fsdev.json
create mode 100644 qapi/iothrottle.json
v0 -> v1:
Addressed the comments by Erik Blake, Greg Kurz and Daniel P.Berrange
Mainly renaming the functions and removing the redundant code
diff --git a/Makefile b/Makefile
index 73e0c12..c33b46d 100644
--- a/Makefile
+++ b/Makefile
@@ -413,7 +413,10 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
$(SRC_PATH)/qapi/block.json $(SRC_PATH)/qapi/block-core.json \
$(SRC_PATH)/qapi/event.json $(SRC_PATH)/qapi/introspect.json \
$(SRC_PATH)/qapi/crypto.json $(SRC_PATH)/qapi/rocker.json \
- $(SRC_PATH)/qapi/trace.json
+ $(SRC_PATH)/qapi/trace.json $(SRC_PATH)/qapi/iothrottle.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)
diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
index 7ae4e86..e3ea095 100644
--- a/fsdev/qemu-fsdev-throttle.c
+++ 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;
+ }
+ if (arg->has_bps_rd_max) {
+ cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max;
+ }
+ if (arg->has_bps_wr_max) {
+ cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max;
+ }
+ if (arg->has_iops_max) {
+ cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max;
+ }
+ if (arg->has_iops_rd_max) {
+ cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max;
+ }
+ if (arg->has_iops_wr_max) {
+ cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max;
+ }
+
+ if (arg->has_bps_max_length) {
+ cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length;
+ }
+ if (arg->has_bps_rd_max_length) {
+ cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length;
+ }
+ if (arg->has_bps_wr_max_length) {
+ cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length;
+ }
+ if (arg->has_iops_max_length) {
+ cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length;
+ }
+ if (arg->has_iops_rd_max_length) {
+ cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length;
+ }
+ if (arg->has_iops_wr_max_length) {
+ cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length;
+ }
+
+ if (arg->has_iops_size) {
+ cfg.op_size = arg->iops_size;
+ }
+
+ 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_device = true;
+ fscfg->device = 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->bps_max = cfg.buckets[THROTTLE_BPS_TOTAL].max;
+ fscfg->bps_rd_max = cfg.buckets[THROTTLE_BPS_READ].max;
+ fscfg->bps_wr_max = cfg.buckets[THROTTLE_BPS_WRITE].max;
+ fscfg->bps_max = cfg.buckets[THROTTLE_OPS_TOTAL].max;
+ fscfg->iops_rd_max = cfg.buckets[THROTTLE_OPS_READ].max;
+ fscfg->iops_wr_max = cfg.buckets[THROTTLE_OPS_WRITE].max;
+
+ 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_config_init(&fst->cfg);
diff --git a/fsdev/qemu-fsdev-throttle.h b/fsdev/qemu-fsdev-throttle.h
index e418643..f6b4d86 100644
--- a/fsdev/qemu-fsdev-throttle.h
+++ b/fsdev/qemu-fsdev-throttle.h
@@ -20,6 +20,12 @@
#include "qemu/coroutine.h"
#include "qapi/error.h"
#include "qemu/throttle.h"
+#include "qapi/qmp/types.h"
+#include "qapi-visit.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi/qobject-output-visitor.h"
+#include "qapi/util.h"
+#include "qmp-commands.h"
typedef struct FsThrottle {
ThrottleState ts;
@@ -36,4 +42,9 @@ 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 **);
+
+void fsdev_get_io_throttle(FsThrottle *, IOThrottle **, char *, Error **);
+
#endif /* _FSDEV_THROTTLE_H */
diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
index 266e442..718646e 100644
--- a/fsdev/qemu-fsdev.c
+++ b/fsdev/qemu-fsdev.c
@@ -17,7 +17,7 @@
#include "qemu/config-file.h"
#include "qemu/error-report.h"
-static QTAILQ_HEAD(FsDriverEntry_head, FsDriverListEntry) fsdriver_entries =
+QTAILQ_HEAD(FsDriverEntry_head, FsDriverListEntry) fsdriver_entries =
QTAILQ_HEAD_INITIALIZER(fsdriver_entries);
static FsDriverTable FsDrivers[] = {
@@ -98,3 +98,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_device ? arg->device : 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 = &head;
+ struct FsDriverListEntry *fsle;
+ Error *local_err = NULL;
+
+ QTAILQ_FOREACH(fsle, &fsdriver_entries, next) {
+ IOThrottleList *fscfg = g_malloc0(sizeof(*fscfg));
+ fsdev_get_io_throttle(&fsle->fse.fst, &fscfg->value,
+ fsle->fse.fsdev_id, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ g_free(fscfg);
+ qapi_free_IOThrottleList(head);
+ return NULL;
+ }
+
+ *p_next = fscfg;
+ p_next = &fscfg->next;
+ }
+ return head;
+}
diff --git a/fsdev/qemu-fsdev.h b/fsdev/qemu-fsdev.h
index 29c9622..8e794d6 100644
--- a/fsdev/qemu-fsdev.h
+++ b/fsdev/qemu-fsdev.h
@@ -39,8 +39,8 @@ typedef struct FsDriverListEntry {
QTAILQ_ENTRY(FsDriverListEntry) next;
} FsDriverListEntry;
+FsDriverEntry *get_fsdev_fsentry(char *);
int qemu_fsdev_add(QemuOpts *opts);
-FsDriverEntry *get_fsdev_fsentry(char *id);
extern FileOperations local_ops;
extern FileOperations handle_ops;
extern FileOperations synth_ops;
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index a53f105..4b8b3e5 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -84,6 +84,24 @@ STEXI
Show block device statistics.
ETEXI
+#if defined(CONFIG_VIRTFS)
+
+ {
+ .name = "fsdevthrottle",
+ .args_type = "",
+ .params = "",
+ .help = "show fsdev device throttle information",
+ .cmd = hmp_fsdev_get_io_throttle,
+ },
+
+#endif
+
+STEXI
+@item info fsdev throttle
+@findex fsdevthrottleinfo
+Show fsdev device throttleinfo.
+ETEXI
+
{
.name = "block-jobs",
.args_type = "",
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8819281..8336266 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1628,6 +1628,24 @@ STEXI
Change I/O throttle limits for a block drive to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
ETEXI
+#if defined(CONFIG_VIRTFS)
+
+ {
+ .name = "fsdev_set_io_throttle",
+ .args_type = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l",
+ .params = "device bps bps_rd bps_wr iops iops_rd iops_wr",
+ .help = "change I/O throttle limits for a block drive",
+ .cmd = hmp_fsdev_set_io_throttle,
+ },
+
+#endif
+
+STEXI
+@item fsdev_set_io_throttle @var{device} @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
+@findex fsdev_set_io_throttle
+Change I/O throttle limits for a block drive to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
+ETEXI
+
{
.name = "set_password",
.args_type = "protocol:s,password:s,connected:s?",
diff --git a/hmp.c b/hmp.c
index edb8970..008ff3b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -38,6 +38,7 @@
#include "qemu/cutils.h"
#include "qemu/error-report.h"
#include "hw/intc/intc.h"
+#include "fsdev/qemu-fsdev-throttle.h"
#ifdef CONFIG_SPICE
#include <spice/enums.h>
@@ -1571,6 +1572,79 @@ 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 = {
+ .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_fsdev_set_io_throttle(&throttle, &err);
+ hmp_handle_error(mon, &err);
+}
+
+static void print_fsdev_throttle_config(Monitor *mon, IOThrottle *fscfg,
+ Error *err)
+{
+ if (fscfg->bps || fscfg->bps_rd || fscfg->bps_wr ||
+ fscfg->iops || fscfg->iops_rd || fscfg->iops_wr)
+ {
+ monitor_printf(mon, "%s", fscfg->device);
+ monitor_printf(mon, " I/O throttling:"
+ " bps=%" PRId64
+ " bps_rd=%" PRId64 " bps_wr=%" PRId64
+ " bps_max=%" PRId64
+ " bps_rd_max=%" PRId64
+ " bps_wr_max=%" PRId64
+ " iops=%" PRId64 " iops_rd=%" PRId64
+ " iops_wr=%" PRId64
+ " iops_max=%" PRId64
+ " iops_rd_max=%" PRId64
+ " iops_wr_max=%" PRId64
+ " iops_size=%" PRId64,
+ fscfg->bps,
+ fscfg->bps_rd,
+ fscfg->bps_wr,
+ fscfg->bps_max,
+ fscfg->bps_rd_max,
+ fscfg->bps_wr_max,
+ fscfg->iops,
+ fscfg->iops_rd,
+ fscfg->iops_wr,
+ fscfg->iops_max,
+ fscfg->iops_rd_max,
+ fscfg->iops_wr_max,
+ fscfg->iops_size);
+ }
+ hmp_handle_error(mon, &err);
+}
+
+void hmp_fsdev_get_io_throttle(Monitor *mon, const QDict *qdict)
+{
+ Error *err = NULL;
+ IOThrottleList *fs9p_list, *info;
+ fs9p_list = qmp_query_fsdev_io_throttle(&err);
+
+ for (info = fs9p_list; info; info = info->next) {
+ if (info != fs9p_list) {
+ monitor_printf(mon, "\n");
+ }
+ print_fsdev_throttle_config(mon, info->value, err);
+ qapi_free_IOThrottle(info->value);
+ }
+ qapi_free_IOThrottleList(fs9p_list);
+}
+
+#endif
+
void hmp_block_stream(Monitor *mon, const QDict *qdict)
{
Error *error = NULL;
diff --git a/hmp.h b/hmp.h
index 799fd37..66b27eb 100644
--- a/hmp.h
+++ b/hmp.h
@@ -56,6 +56,7 @@ void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
void hmp_nmi(Monitor *mon, const QDict *qdict);
void hmp_set_link(Monitor *mon, const QDict *qdict);
void hmp_block_passwd(Monitor *mon, const QDict *qdict);
+void hmp_9p_passwd(Monitor *mon, const QDict *qdict);
void hmp_balloon(Monitor *mon, const QDict *qdict);
void hmp_block_resize(Monitor *mon, const QDict *qdict);
void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
@@ -77,6 +78,10 @@ void hmp_set_password(Monitor *mon, const QDict *qdict);
void hmp_expire_password(Monitor *mon, const QDict *qdict);
void hmp_eject(Monitor *mon, const QDict *qdict);
void hmp_change(Monitor *mon, const QDict *qdict);
+#ifdef CONFIG_VIRTFS
+void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict);
+void hmp_fsdev_get_io_throttle(Monitor *mon, const QDict *qdict);
+#endif
void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
void hmp_block_stream(Monitor *mon, const QDict *qdict);
void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
diff --git a/qapi-schema.json b/qapi-schema.json
index 68a4327..cd483a0 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -78,9 +78,15 @@
# QAPI crypto definitions
{ 'include': 'qapi/crypto.json' }
+# QAPI IOThrottle definitions
+{ 'include': 'qapi/iothrottle.json' }
+
# 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..7fb3c25
--- /dev/null
+++ b/qapi/fsdev.json
@@ -0,0 +1,87 @@
+# -*- Mode: Python -*-
+
+##
+# == QAPI fsdev definitions
+##
+
+# QAPI common definitions
+{ 'include': 'common.json' }
+{ '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 vaue to non-zero numbers.
+#
+# I/O limits can be disabled by setting all of them 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": { "device": "ide0-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:
+#
+# Return a list of information about fsdev device
+#
+# Returns: @FsdevIOIOThrottle
+#
+# Since: 2.10
+#
+# Example:
+#
+# -> { "Execute": "query-fsdev-io-throttle" }
+# <- { "returns" : [
+# {
+# "device": "ide0-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
new file mode 100644
index 0000000..f7b615d
--- /dev/null
+++ b/qapi/iothrottle.json
@@ -0,0 +1,93 @@
+# -*- Mode: Python -*-
+
+##
+# == QAPI IOThrottle definitions
+##
+
+# QAPI common definitions
+{ 'include': 'common.json' }
+
+##
+# @IOThrottle:
+#
+# A set of parameters describing block
+#
+# @device: Block device name
+#
+# @bps: total throughput limit in bytes per second
+#
+# @bps_rd: read throughput limit in bytes per second
+#
+# @bps_wr: write throughput limit in bytes per second
+#
+# @iops: total I/O operations per second
+#
+# @iops_rd: read I/O operations per second
+#
+# @iops_wr: write I/O operations per second
+#
+# @bps_max: total throughput limit during bursts,
+# in bytes (Since 1.7)
+#
+# @bps_rd_max: read throughput limit during bursts,
+# in bytes (Since 1.7)
+#
+# @bps_wr_max: write throughput limit during bursts,
+# in bytes (Since 1.7)
+#
+# @iops_max: total I/O operations per second during bursts,
+# in bytes (Since 1.7)
+#
+# @iops_rd_max: read I/O operations per second during bursts,
+# in bytes (Since 1.7)
+#
+# @iops_wr_max: write I/O operations per second during bursts,
+# in bytes (Since 1.7)
+#
+# @bps_max_length: maximum length of the @bps_max burst
+# period, in seconds. It must only
+# be set if @bps_max is set as well.
+# Defaults to 1. (Since 2.6)
+#
+# @bps_rd_max_length: maximum length of the @bps_rd_max
+# burst period, in seconds. It must only
+# be set if @bps_rd_max is set as well.
+# Defaults to 1. (Since 2.6)
+#
+# @bps_wr_max_length: maximum length of the @bps_wr_max
+# burst period, in seconds. It must only
+# be set if @bps_wr_max is set as well.
+# Defaults to 1. (Since 2.6)
+#
+# @iops_max_length: maximum length of the @iops burst
+# period, in seconds. It must only
+# be set if @iops_max is set as well.
+# Defaults to 1. (Since 2.6)
+#
+# @iops_rd_max_length: maximum length of the @iops_rd_max
+# burst period, in seconds. It must only
+# be set if @iops_rd_max is set as well.
+# Defaults to 1. (Since 2.6)
+#
+# @iops_wr_max_length: maximum length of the @iops_wr_max
+# burst period, in seconds. It must only
+# be set if @iops_wr_max is set as well.
+# Defaults to 1. (Since 2.6)
+#
+# @iops_size: an I/O size in bytes (Since 1.7)
+#
+#
+# Since: 2.10
+##
+{ 'struct': 'IOThrottle',
+ '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',
+ '*bps_wr_max': 'int', '*iops_max': 'int',
+ '*iops_rd_max': 'int', '*iops_wr_max': 'int',
+ '*bps_max_length': 'int', '*bps_rd_max_length': 'int',
+ '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
+ '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
+ '*iops_size': 'int' } }
+
+
diff --git a/qmp.c b/qmp.c
index fa82b59..743ed37 100644
--- a/qmp.c
+++ b/qmp.c
@@ -162,6 +162,20 @@ SpiceInfo *qmp_query_spice(Error **errp)
};
#endif
+#ifndef CONFIG_VIRTFS
+
+void qmp_fsdev_set_io_throttle(IOThrottle *arg, Error **errp)
+{
+
+}
+
+IOThrottleList *qmp_query_fsdev_io_throttle(Error **errp)
+{
+ abort();
+}
+
+#endif
+
void qmp_cont(Error **errp)
{
Error *local_err = NULL;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 2/2 v1] throttle: factor out the redundant code
2017-03-23 12:20 [Qemu-devel] [PATCH 1/2 v1] fsdev: QMP interface for throttling Pradeep Jagadeesh
@ 2017-03-23 12:20 ` Pradeep Jagadeesh
2017-03-23 14:46 ` Eric Blake
2017-03-23 14:32 ` [Qemu-devel] [PATCH 1/2 v1] fsdev: QMP interface for throttling Eric Blake
1 sibling, 1 reply; 8+ messages in thread
From: Pradeep Jagadeesh @ 2017-03-23 12:20 UTC (permalink / raw)
To: Eric Blake, Greg Kurz
Cc: Pradeep Jagadeesh, Alberto Garcia, Jani Kokkonen, qemu-devel
This patchset removes the throttle redundant code that was present
in block and fsdev files.
Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
---
blockdev.c | 106 ++++-----------------------------------
fsdev/Makefile.objs | 1 +
fsdev/qemu-fsdev-throttle.c | 94 +---------------------------------
fsdev/qemu-fsdev-throttle.h | 3 +-
hmp.c | 39 +++++++--------
include/qemu/throttle-options.h | 5 ++
qapi/block-core.json | 76 +---------------------------
qapi/iothrottle.json | 4 +-
util/Makefile.objs | 1 +
util/throttle-options.c | 108 ++++++++++++++++++++++++++++++++++++++++
10 files changed, 151 insertions(+), 286 deletions(-)
create mode 100644 util/throttle-options.c
diff --git a/blockdev.c b/blockdev.c
index c5b2c2c..2f4ec3a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -386,49 +386,7 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
}
if (throttle_cfg) {
- throttle_config_init(throttle_cfg);
- throttle_cfg->buckets[THROTTLE_BPS_TOTAL].avg =
- qemu_opt_get_number(opts, "throttling.bps-total", 0);
- throttle_cfg->buckets[THROTTLE_BPS_READ].avg =
- qemu_opt_get_number(opts, "throttling.bps-read", 0);
- throttle_cfg->buckets[THROTTLE_BPS_WRITE].avg =
- qemu_opt_get_number(opts, "throttling.bps-write", 0);
- throttle_cfg->buckets[THROTTLE_OPS_TOTAL].avg =
- qemu_opt_get_number(opts, "throttling.iops-total", 0);
- throttle_cfg->buckets[THROTTLE_OPS_READ].avg =
- qemu_opt_get_number(opts, "throttling.iops-read", 0);
- throttle_cfg->buckets[THROTTLE_OPS_WRITE].avg =
- qemu_opt_get_number(opts, "throttling.iops-write", 0);
-
- throttle_cfg->buckets[THROTTLE_BPS_TOTAL].max =
- qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
- throttle_cfg->buckets[THROTTLE_BPS_READ].max =
- qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
- throttle_cfg->buckets[THROTTLE_BPS_WRITE].max =
- qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
- throttle_cfg->buckets[THROTTLE_OPS_TOTAL].max =
- qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
- throttle_cfg->buckets[THROTTLE_OPS_READ].max =
- qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
- throttle_cfg->buckets[THROTTLE_OPS_WRITE].max =
- qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
-
- throttle_cfg->buckets[THROTTLE_BPS_TOTAL].burst_length =
- qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
- throttle_cfg->buckets[THROTTLE_BPS_READ].burst_length =
- qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
- throttle_cfg->buckets[THROTTLE_BPS_WRITE].burst_length =
- qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
- throttle_cfg->buckets[THROTTLE_OPS_TOTAL].burst_length =
- qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
- throttle_cfg->buckets[THROTTLE_OPS_READ].burst_length =
- qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
- throttle_cfg->buckets[THROTTLE_OPS_WRITE].burst_length =
- qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
-
- throttle_cfg->op_size =
- qemu_opt_get_number(opts, "throttling.iops-size", 0);
-
+ parse_io_throttle_options(throttle_cfg, opts);
if (!throttle_is_valid(throttle_cfg, errp)) {
return;
}
@@ -2635,9 +2593,12 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
BlockDriverState *bs;
BlockBackend *blk;
AioContext *aio_context;
+ IOThrottle *iothrottle;
+
+ iothrottle = arg->iothrottle;
- blk = qmp_get_blk(arg->has_device ? arg->device : NULL,
- arg->has_id ? arg->id : NULL,
+ blk = qmp_get_blk(iothrottle->has_device ? iothrottle->device : NULL,
+ iothrottle->has_id ? iothrottle->id : NULL,
errp);
if (!blk) {
return;
@@ -2652,56 +2613,7 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
goto out;
}
- 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;
- }
- if (arg->has_bps_rd_max) {
- cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max;
- }
- if (arg->has_bps_wr_max) {
- cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max;
- }
- if (arg->has_iops_max) {
- cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max;
- }
- if (arg->has_iops_rd_max) {
- cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max;
- }
- if (arg->has_iops_wr_max) {
- cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max;
- }
-
- if (arg->has_bps_max_length) {
- cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length;
- }
- if (arg->has_bps_rd_max_length) {
- cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length;
- }
- if (arg->has_bps_wr_max_length) {
- cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length;
- }
- if (arg->has_iops_max_length) {
- cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length;
- }
- if (arg->has_iops_rd_max_length) {
- cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length;
- }
- if (arg->has_iops_wr_max_length) {
- cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length;
- }
-
- if (arg->has_iops_size) {
- cfg.op_size = arg->iops_size;
- }
+ qmp_set_io_throttle(&cfg, iothrottle);
if (!throttle_is_valid(&cfg, errp)) {
goto out;
@@ -2713,8 +2625,8 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
if (!blk_get_public(blk)->throttle_state) {
blk_io_limits_enable(blk,
arg->has_group ? arg->group :
- arg->has_device ? arg->device :
- arg->id);
+ iothrottle->has_device ? iothrottle->device :
+ iothrottle->id);
} else if (arg->has_group) {
blk_io_limits_update_group(blk, arg->group);
}
diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs
index 659df6e..31eed64 100644
--- a/fsdev/Makefile.objs
+++ b/fsdev/Makefile.objs
@@ -5,6 +5,7 @@ common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o
else
common-obj-y = qemu-fsdev-dummy.o
endif
+#common-obj-y += qemu-fsdev-opts.o prin.o qemu-fsdev-throttle.o qmp-throttle.o
common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o
# Toplevel always builds this; targets without virtio will put it in
diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
index e3ea095..275a565 100644
--- a/fsdev/qemu-fsdev-throttle.c
+++ b/fsdev/qemu-fsdev-throttle.c
@@ -33,56 +33,7 @@ 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;
- }
- if (arg->has_bps_rd_max) {
- cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max;
- }
- if (arg->has_bps_wr_max) {
- cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max;
- }
- if (arg->has_iops_max) {
- cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max;
- }
- if (arg->has_iops_rd_max) {
- cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max;
- }
- if (arg->has_iops_wr_max) {
- cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max;
- }
-
- if (arg->has_bps_max_length) {
- cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length;
- }
- if (arg->has_bps_rd_max_length) {
- cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length;
- }
- if (arg->has_bps_wr_max_length) {
- cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length;
- }
- if (arg->has_iops_max_length) {
- cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length;
- }
- if (arg->has_iops_rd_max_length) {
- cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length;
- }
- if (arg->has_iops_wr_max_length) {
- cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length;
- }
-
- if (arg->has_iops_size) {
- cfg.op_size = arg->iops_size;
- }
+ qmp_set_io_throttle(&cfg, arg);
if (throttle_is_valid(&cfg, errp)) {
fst->cfg = cfg;
@@ -127,48 +78,7 @@ void fsdev_get_io_throttle(FsThrottle *fst, IOThrottle **fs9pcfg,
void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp)
{
- throttle_config_init(&fst->cfg);
- fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
- qemu_opt_get_number(opts, "throttling.bps-total", 0);
- fst->cfg.buckets[THROTTLE_BPS_READ].avg =
- qemu_opt_get_number(opts, "throttling.bps-read", 0);
- fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
- qemu_opt_get_number(opts, "throttling.bps-write", 0);
- fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
- qemu_opt_get_number(opts, "throttling.iops-total", 0);
- fst->cfg.buckets[THROTTLE_OPS_READ].avg =
- qemu_opt_get_number(opts, "throttling.iops-read", 0);
- fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
- qemu_opt_get_number(opts, "throttling.iops-write", 0);
-
- fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
- qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
- fst->cfg.buckets[THROTTLE_BPS_READ].max =
- qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
- fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
- qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
- fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
- qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
- fst->cfg.buckets[THROTTLE_OPS_READ].max =
- qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
- fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
- qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
-
- fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length =
- qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
- fst->cfg.buckets[THROTTLE_BPS_READ].burst_length =
- qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
- fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length =
- qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
- fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length =
- qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
- fst->cfg.buckets[THROTTLE_OPS_READ].burst_length =
- qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
- fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length =
- qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
- fst->cfg.op_size =
- qemu_opt_get_number(opts, "throttling.iops-size", 0);
-
+ parse_io_throttle_options(&fst->cfg, opts);
throttle_is_valid(&fst->cfg, errp);
}
diff --git a/fsdev/qemu-fsdev-throttle.h b/fsdev/qemu-fsdev-throttle.h
index f6b4d86..14cd183 100644
--- a/fsdev/qemu-fsdev-throttle.h
+++ b/fsdev/qemu-fsdev-throttle.h
@@ -19,13 +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/qmp/qerror.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;
diff --git a/hmp.c b/hmp.c
index 008ff3b..f489d49 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1554,20 +1554,25 @@ void hmp_change(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, &err);
}
+static void hmp_initialize_io_throttle(IOThrottle *iot, const QDict *qdict)
+{
+ iot->has_device = true;
+ iot->device = (char *) qdict_get_str(qdict, "device");
+ iot->bps = qdict_get_int(qdict, "bps");
+ iot->bps_rd = qdict_get_int(qdict, "bps_rd");
+ iot->bps_wr = qdict_get_int(qdict, "bps_wr");
+ iot->iops = qdict_get_int(qdict, "iops");
+ iot->iops_rd = qdict_get_int(qdict, "iops_rd");
+ iot->iops_wr = qdict_get_int(qdict, "iops_wr");
+}
+
void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
- BlockIOThrottle throttle = {
- .has_device = true,
- .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"),
- };
-
+ BlockIOThrottle throttle;
+ IOThrottle iothrottle;
+ hmp_initialize_io_throttle(&iothrottle, qdict);
+ throttle.iothrottle = &iothrottle;
qmp_block_set_io_throttle(&throttle, &err);
hmp_handle_error(mon, &err);
}
@@ -1577,16 +1582,8 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
- IOThrottle 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"),
- };
-
+ IOThrottle throttle;
+ hmp_initialize_io_throttle(&throttle, qdict);
qmp_fsdev_set_io_throttle(&throttle, &err);
hmp_handle_error(mon, &err);
}
diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
index 3133d1c..59a575e 100644
--- a/include/qemu/throttle-options.h
+++ b/include/qemu/throttle-options.h
@@ -10,6 +10,9 @@
#ifndef THROTTLE_OPTIONS_H
#define THROTTLE_OPTIONS_H
+#include "qemu/throttle.h"
+#include "qmp-commands.h"
+
#define THROTTLE_OPTS \
{ \
.name = "throttling.iops-total",\
@@ -89,4 +92,6 @@
.help = "when limiting by iops max size of an I/O in bytes",\
}
+void qmp_set_io_throttle(ThrottleConfig *, IOThrottle *);
+void parse_io_throttle_options(ThrottleConfig *, QemuOpts *);
#endif
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0f132fc..3d0a3fb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1758,86 +1758,14 @@
#
# A set of parameters describing block throttling.
#
-# @device: Block device name (deprecated, use @id instead)
-#
-# @id: The name or QOM path of the guest device (since: 2.8)
-#
-# @bps: total throughput limit in bytes per second
-#
-# @bps_rd: read throughput limit in bytes per second
-#
-# @bps_wr: write throughput limit in bytes per second
-#
-# @iops: total I/O operations per second
-#
-# @iops_rd: read I/O operations per second
-#
-# @iops_wr: write I/O operations per second
-#
-# @bps_max: total throughput limit during bursts,
-# in bytes (Since 1.7)
-#
-# @bps_rd_max: read throughput limit during bursts,
-# in bytes (Since 1.7)
-#
-# @bps_wr_max: write throughput limit during bursts,
-# in bytes (Since 1.7)
-#
-# @iops_max: total I/O operations per second during bursts,
-# in bytes (Since 1.7)
-#
-# @iops_rd_max: read I/O operations per second during bursts,
-# in bytes (Since 1.7)
-#
-# @iops_wr_max: write I/O operations per second during bursts,
-# in bytes (Since 1.7)
-#
-# @bps_max_length: maximum length of the @bps_max burst
-# period, in seconds. It must only
-# be set if @bps_max is set as well.
-# Defaults to 1. (Since 2.6)
-#
-# @bps_rd_max_length: maximum length of the @bps_rd_max
-# burst period, in seconds. It must only
-# be set if @bps_rd_max is set as well.
-# Defaults to 1. (Since 2.6)
-#
-# @bps_wr_max_length: maximum length of the @bps_wr_max
-# burst period, in seconds. It must only
-# be set if @bps_wr_max is set as well.
-# Defaults to 1. (Since 2.6)
-#
-# @iops_max_length: maximum length of the @iops burst
-# period, in seconds. It must only
-# be set if @iops_max is set as well.
-# Defaults to 1. (Since 2.6)
-#
-# @iops_rd_max_length: maximum length of the @iops_rd_max
-# burst period, in seconds. It must only
-# be set if @iops_rd_max is set as well.
-# Defaults to 1. (Since 2.6)
-#
-# @iops_wr_max_length: maximum length of the @iops_wr_max
-# burst period, in seconds. It must only
-# be set if @iops_wr_max is set as well.
-# Defaults to 1. (Since 2.6)
-#
-# @iops_size: an I/O size in bytes (Since 1.7)
+# @iothrottle: throttle configuration (Since 1.7)
#
# @group: throttle group name (Since 2.4)
#
# Since: 1.1
##
{ 'struct': 'BlockIOThrottle',
- 'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
- 'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
- '*bps_max': 'int', '*bps_rd_max': 'int',
- '*bps_wr_max': 'int', '*iops_max': 'int',
- '*iops_rd_max': 'int', '*iops_wr_max': 'int',
- '*bps_max_length': 'int', '*bps_rd_max_length': 'int',
- '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
- '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
- '*iops_size': 'int', '*group': 'str' } }
+ 'data': { '*iothrottle': 'IOThrottle', '*group': 'str' } }
##
# @block-stream:
diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
index f7b615d..58f4520 100644
--- a/qapi/iothrottle.json
+++ b/qapi/iothrottle.json
@@ -14,6 +14,8 @@
#
# @device: Block device name
#
+# @id: The name or QOM path of the guest device (since: 2.8)
+#
# @bps: total throughput limit in bytes per second
#
# @bps_rd: read throughput limit in bytes per second
@@ -80,7 +82,7 @@
# Since: 2.10
##
{ 'struct': 'IOThrottle',
- 'data': { '*device': 'str', 'bps': 'int', 'bps_rd': 'int',
+ 'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
'*bps_max': 'int', '*bps_rd_max': 'int',
'*bps_wr_max': 'int', '*iops_max': 'int',
diff --git a/util/Makefile.objs b/util/Makefile.objs
index 06366b5..401eaf5 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -42,3 +42,4 @@ util-obj-y += log.o
util-obj-y += qdist.o
util-obj-y += qht.o
util-obj-y += range.o
+util-obj-y += throttle-options.o
diff --git a/util/throttle-options.c b/util/throttle-options.c
new file mode 100644
index 0000000..e428938
--- /dev/null
+++ b/util/throttle-options.c
@@ -0,0 +1,108 @@
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "qemu/throttle-options.h"
+#include "qemu/iov.h"
+
+void qmp_set_io_throttle(ThrottleConfig *cfg, IOThrottle *arg)
+{
+
+ 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;
+ }
+ if (arg->has_bps_rd_max) {
+ cfg->buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max;
+ }
+ if (arg->has_bps_wr_max) {
+ cfg->buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max;
+ }
+ if (arg->has_iops_max) {
+ cfg->buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max;
+ }
+ if (arg->has_iops_rd_max) {
+ cfg->buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max;
+ }
+ if (arg->has_iops_wr_max) {
+ cfg->buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max;
+ }
+
+ if (arg->has_bps_max_length) {
+ cfg->buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length;
+ }
+ if (arg->has_bps_rd_max_length) {
+ cfg->buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length;
+ }
+ if (arg->has_bps_wr_max_length) {
+ cfg->buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length;
+ }
+ if (arg->has_iops_max_length) {
+ cfg->buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length;
+ }
+ if (arg->has_iops_rd_max_length) {
+ cfg->buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length;
+ }
+ if (arg->has_iops_wr_max_length) {
+ cfg->buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length;
+ }
+
+ if (arg->has_iops_size) {
+ cfg->op_size = arg->iops_size;
+ }
+}
+
+void parse_io_throttle_options(ThrottleConfig *throttle_cfg, QemuOpts *opts)
+{
+ throttle_config_init(throttle_cfg);
+ throttle_cfg->buckets[THROTTLE_BPS_TOTAL].avg =
+ qemu_opt_get_number(opts, "throttling.bps-total", 0);
+ throttle_cfg->buckets[THROTTLE_BPS_READ].avg =
+ qemu_opt_get_number(opts, "throttling.bps-read", 0);
+ throttle_cfg->buckets[THROTTLE_BPS_WRITE].avg =
+ qemu_opt_get_number(opts, "throttling.bps-write", 0);
+ throttle_cfg->buckets[THROTTLE_OPS_TOTAL].avg =
+ qemu_opt_get_number(opts, "throttling.iops-total", 0);
+ throttle_cfg->buckets[THROTTLE_OPS_READ].avg =
+ qemu_opt_get_number(opts, "throttling.iops-read", 0);
+ throttle_cfg->buckets[THROTTLE_OPS_WRITE].avg =
+ qemu_opt_get_number(opts, "throttling.iops-write", 0);
+
+ throttle_cfg->buckets[THROTTLE_BPS_TOTAL].max =
+ qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
+ throttle_cfg->buckets[THROTTLE_BPS_READ].max =
+ qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
+ throttle_cfg->buckets[THROTTLE_BPS_WRITE].max =
+ qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
+ throttle_cfg->buckets[THROTTLE_OPS_TOTAL].max =
+ qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
+ throttle_cfg->buckets[THROTTLE_OPS_READ].max =
+ qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
+ throttle_cfg->buckets[THROTTLE_OPS_WRITE].max =
+ qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
+
+ throttle_cfg->buckets[THROTTLE_BPS_TOTAL].burst_length =
+ qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
+ throttle_cfg->buckets[THROTTLE_BPS_READ].burst_length =
+ qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
+ throttle_cfg->buckets[THROTTLE_BPS_WRITE].burst_length =
+ qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
+ throttle_cfg->buckets[THROTTLE_OPS_TOTAL].burst_length =
+ qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
+ throttle_cfg->buckets[THROTTLE_OPS_READ].burst_length =
+ qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
+ throttle_cfg->buckets[THROTTLE_OPS_WRITE].burst_length =
+ qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
+
+ throttle_cfg->op_size =
+ qemu_opt_get_number(opts, "throttling.iops-size", 0);
+
+}
+
+
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2 v1] fsdev: QMP interface for throttling
2017-03-23 12:20 [Qemu-devel] [PATCH 1/2 v1] fsdev: QMP interface for throttling Pradeep Jagadeesh
2017-03-23 12:20 ` [Qemu-devel] [PATCH 2/2 v1] throttle: factor out the redundant code Pradeep Jagadeesh
@ 2017-03-23 14:32 ` Eric Blake
2017-03-28 11:27 ` Pradeep Jagadeesh
1 sibling, 1 reply; 8+ messages in thread
From: Eric Blake @ 2017-03-23 14:32 UTC (permalink / raw)
To: Pradeep Jagadeesh, Greg Kurz, Daniel P.Berrange
Cc: Pradeep Jagadeesh, Alberto Garcia, Jani Kokkonen, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6402 bytes --]
On 03/23/2017 07:20 AM, Pradeep Jagadeesh wrote:
> This patchset enables qmp interfaces for the 9pfs
> devices (fsdev). This provides two interfaces one
s/interfaces/interfaces,/
Also, I just noticed you are using trailing spaces in your commit
message (not fatal, but unusual).
> for querying all the 9pfs devices info. The second one
> to set the IO limits for the required 9pfs device.
When sending a multi-patch series, please remember to include the 0/2
cover letter ('git config format.coverletter auto' can help here).
>
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> ---
> Makefile | 5 ++-
> fsdev/qemu-fsdev-throttle.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
> fsdev/qemu-fsdev-throttle.h | 11 ++++++
> fsdev/qemu-fsdev.c | 38 +++++++++++++++++-
> fsdev/qemu-fsdev.h | 2 +-
> hmp-commands-info.hx | 18 +++++++++
> hmp-commands.hx | 18 +++++++++
> hmp.c | 74 ++++++++++++++++++++++++++++++++++
> hmp.h | 5 +++
> qapi-schema.json | 6 +++
> qapi/fsdev.json | 87 ++++++++++++++++++++++++++++++++++++++++
> qapi/iothrottle.json | 93 +++++++++++++++++++++++++++++++++++++++++++
> qmp.c | 14 +++++++
> 13 files changed, 464 insertions(+), 3 deletions(-)
That's a big patch to review in one go. Does it make sense to break it
down into smaller chunks?
> create mode 100644 qapi/fsdev.json
> create mode 100644 qapi/iothrottle.json
>
> v0 -> v1:
>
> Addressed the comments by Erik Blake, Greg Kurz and Daniel P.Berrange
It's Eric, but you're not the first (and probably not the last) to get
it wrong.
> Mainly renaming the functions and removing the redundant code
>
> diff --git a/Makefile b/Makefile
> index 73e0c12..c33b46d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -413,7 +413,10 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
> $(SRC_PATH)/qapi/block.json $(SRC_PATH)/qapi/block-core.json \
> $(SRC_PATH)/qapi/event.json $(SRC_PATH)/qapi/introspect.json \
> $(SRC_PATH)/qapi/crypto.json $(SRC_PATH)/qapi/rocker.json \
> - $(SRC_PATH)/qapi/trace.json
> + $(SRC_PATH)/qapi/trace.json $(SRC_PATH)/qapi/iothrottle.json
Why two spaces?
> +++ b/qapi-schema.json
> @@ -78,9 +78,15 @@
> # QAPI crypto definitions
> { 'include': 'qapi/crypto.json' }
>
> +# QAPI IOThrottle definitions
> +{ 'include': 'qapi/iothrottle.json' }
> +
> # QAPI block definitions
> { 'include': 'qapi/block.json' }
>
> +# QAPI fsdev definitions
> +{ 'include': 'qapi/fsdev.json' }
> +
Adding two new files definitely feels like something that could be
split. Also, is it possible that existing code could take advantage of
qapi/iothrottle.json, rather than it just being additional code?
> # QAPI event definitions
> { 'include': 'qapi/event.json' }
>
> diff --git a/qapi/fsdev.json b/qapi/fsdev.json
> new file mode 100644
> index 0000000..7fb3c25
> --- /dev/null
> +++ b/qapi/fsdev.json
> @@ -0,0 +1,87 @@
> +# -*- Mode: Python -*-
> +
> +##
> +# == QAPI fsdev definitions
> +##
> +
> +# QAPI common definitions
> +{ 'include': 'common.json' }
> +{ 'include': 'iothrottle.json' }
Is this really necessary? Given that the top level already included
these, I think it serves more as a documentation purpose than something
you actually need.
> +
> +##
> +# @fsdev_set_io_throttle:
> +#
> +# Change I/O limits for a 9p/fsdev device.
> +#
> +# I/O limits can be enabled by setting throttle vaue to non-zero numbers.
s/vaue/value/
> +#
> +# I/O limits can be disabled by setting all of them to 0.
> +#
> +# Returns: Nothing on success
> +# If @device is not a valid fsdev device, DeviceNotFound
> +#
> +# Since: 2.10
> +#
> +# Example:
> +#
> +# -> { "execute": "fsdev_set_io_throttle",
Please name this fsdev-set-io-throttle. New commands should favor the
use of '-' rather than '_'.
> +##
> +# @query-fsdev-io-throttle:
> +#
> +# Return a list of information about fsdev device
> +#
> +# Returns: @FsdevIOIOThrottle
Typo in the type name, should be merely IOThrottle
> +#
> +# Since: 2.10
> +#
> +# Example:
> +#
> +# -> { "Execute": "query-fsdev-io-throttle" }
> +#
> +##
> +{ 'command': 'query-fsdev-io-throttle', 'returns': [ 'IOThrottle' ] }
> +
> diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
> new file mode 100644
> index 0000000..f7b615d
> --- /dev/null
> +++ b/qapi/iothrottle.json
> @@ -0,0 +1,93 @@
> +# -*- Mode: Python -*-
> +
> +##
> +# == QAPI IOThrottle definitions
> +##
> +
> +# QAPI common definitions
> +{ 'include': 'common.json' }
Again, is this necessary?
> +# @iops_size: an I/O size in bytes (Since 1.7)
> +#
> +#
> +# Since: 2.10
It's weird to state that this struct is since 2.10, but its members are
since 1.7. Either you should list a 'since' for when the members of the
struct were first useful (0.14.0), or eliminate all the (since ...) tags
on the members and keep the overall struct at 2.10. I'd lean towards
the former, at least if BlockDeviceInfo is properly refactored to
inherit from this type.
> +##
> +{ 'struct': 'IOThrottle',
> + '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',
> + '*bps_wr_max': 'int', '*iops_max': 'int',
> + '*iops_rd_max': 'int', '*iops_wr_max': 'int',
> + '*bps_max_length': 'int', '*bps_rd_max_length': 'int',
> + '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
> + '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
> + '*iops_size': 'int' } }
So yeah, you definitely need to split this patch. Your FIRST patch
should be the creation of IOThrottle, and fixing BlockDeviceInfo to
inherit from it; then a later patch would be the introduction of the
fsdev throttle commands that are additional clients of the new
IOThrottle type.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2 v1] throttle: factor out the redundant code
2017-03-23 12:20 ` [Qemu-devel] [PATCH 2/2 v1] throttle: factor out the redundant code Pradeep Jagadeesh
@ 2017-03-23 14:46 ` Eric Blake
2017-03-23 18:14 ` Eric Blake
2017-03-28 11:11 ` Pradeep Jagadeesh
0 siblings, 2 replies; 8+ messages in thread
From: Eric Blake @ 2017-03-23 14:46 UTC (permalink / raw)
To: Pradeep Jagadeesh, Greg Kurz
Cc: Pradeep Jagadeesh, Alberto Garcia, Jani Kokkonen, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6292 bytes --]
On 03/23/2017 07:20 AM, Pradeep Jagadeesh wrote:
> This patchset removes the throttle redundant code that was present
> in block and fsdev files.
>
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> ---
I think you want portions of this patch to come first; you want to
refactor out the IOThrottle portion of existing types, and then extend
IOThrottle to be used in more places.
> blockdev.c | 106 ++++-----------------------------------
> fsdev/Makefile.objs | 1 +
> fsdev/qemu-fsdev-throttle.c | 94 +---------------------------------
> fsdev/qemu-fsdev-throttle.h | 3 +-
> hmp.c | 39 +++++++--------
> include/qemu/throttle-options.h | 5 ++
> qapi/block-core.json | 76 +---------------------------
> qapi/iothrottle.json | 4 +-
> util/Makefile.objs | 1 +
> util/throttle-options.c | 108 ++++++++++++++++++++++++++++++++++++++++
> 10 files changed, 151 insertions(+), 286 deletions(-)
> create mode 100644 util/throttle-options.c
>
> +++ b/fsdev/qemu-fsdev-throttle.c
> @@ -33,56 +33,7 @@ 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;
> -
This code was added in the last patch - which makes for a lot of
unnecessary churn. Again, the best series does refactoring of existing
code first, then adds the new code that takes advantage of the
refactored entry points, so that the new code isn't churning.
> +++ b/hmp.c
> @@ -1554,20 +1554,25 @@ void hmp_change(Monitor *mon, const QDict *qdict)
> hmp_handle_error(mon, &err);
> }
>
> +static void hmp_initialize_io_throttle(IOThrottle *iot, const QDict *qdict)
> +{
> + iot->has_device = true;
> + iot->device = (char *) qdict_get_str(qdict, "device");
Is casting away const going to result in a double free of memory later
on? Or put another way, should this be a g_strdup'd copy (which you
then have to make sure is not leaked)?
> + iot->bps = qdict_get_int(qdict, "bps");
> + iot->bps_rd = qdict_get_int(qdict, "bps_rd");
> + iot->bps_wr = qdict_get_int(qdict, "bps_wr");
> + iot->iops = qdict_get_int(qdict, "iops");
> + iot->iops_rd = qdict_get_int(qdict, "iops_rd");
> + iot->iops_wr = qdict_get_int(qdict, "iops_wr");
> +}
> +
> void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
> {
> Error *err = NULL;
> - BlockIOThrottle throttle = {
> - .has_device = true,
> - .device = (char *) qdict_get_str(qdict, "device"),
At any rate, I see it is just code motion. You are refactoring too much
in one patch here; I'd consider splitting it along the lines of one
patch that adds hmp_initialize_io_throttle; and another patch that adds
parse_io_throttle_options(), so that it's easier to see just one piece
of code motion at a time, rather than trying to track multiple motions
in the same large patch.
> +++ b/qapi/block-core.json
> @@ -1758,86 +1758,14 @@
> #
> # A set of parameters describing block throttling.
> #
> -# @iops_size: an I/O size in bytes (Since 1.7)
> +# @iothrottle: throttle configuration (Since 1.7)
NACK. This is an incompatible change; you are breaking the QMP wire
structure (that is, where I used to pass "arguments":{"device":"foo",
"bps_rd":1, "group":"bar" }, your change would now require me to pass
"arguments":{"iothrottle":{"device":"foo", "bps_rd":1}, "group":"bar"};
the extra nesting is what breaks things).
> #
> # @group: throttle group name (Since 2.4)
> #
> # Since: 1.1
> ##
> { 'struct': 'BlockIOThrottle',
> - 'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
> - 'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
> - '*bps_max': 'int', '*bps_rd_max': 'int',
> - '*bps_wr_max': 'int', '*iops_max': 'int',
> - '*iops_rd_max': 'int', '*iops_wr_max': 'int',
> - '*bps_max_length': 'int', '*bps_rd_max_length': 'int',
> - '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
> - '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
> - '*iops_size': 'int', '*group': 'str' } }
> + 'data': { '*iothrottle': 'IOThrottle', '*group': 'str' } }
What you REALLY want is a compatible change, namely:
{ 'struct': 'BlockIOThrottle', 'base': 'IOThrottle',
'data': { '*group': 'str' } }
which says that all fields of IOThrottle are inherited at the same level
as the additional field 'group'.
For that matter, are you sure that 'group' is the only field that should
not be directly in IOThrottle, or should 'device' also be specific to
BlockIOThrottle (I'm not sure whether you were actually using 'device'
in the fsdev case, or if 'id' was sufficient).
>
> ##
> # @block-stream:
> diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
> index f7b615d..58f4520 100644
> --- a/qapi/iothrottle.json
> +++ b/qapi/iothrottle.json
> @@ -14,6 +14,8 @@
> #
> # @device: Block device name
> #
> +# @id: The name or QOM path of the guest device (since: 2.8)
> +#
You've missed 2.8 by a long shot. Oh - this is an argument that it is
'id' (and not 'device') that does not belong here, but should be placed
alongside 'group' in the BlockIOThrottle subtype.
> # @bps: total throughput limit in bytes per second
> #
> # @bps_rd: read throughput limit in bytes per second
> @@ -80,7 +82,7 @@
> # Since: 2.10
> ##
> { 'struct': 'IOThrottle',
> - 'data': { '*device': 'str', 'bps': 'int', 'bps_rd': 'int',
> + 'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
> 'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
> '*bps_max': 'int', '*bps_rd_max': 'int',
> '*bps_wr_max': 'int', '*iops_max': 'int',
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2 v1] throttle: factor out the redundant code
2017-03-23 14:46 ` Eric Blake
@ 2017-03-23 18:14 ` Eric Blake
2017-03-24 14:09 ` Pradeep Jagadeesh
2017-03-28 11:11 ` Pradeep Jagadeesh
1 sibling, 1 reply; 8+ messages in thread
From: Eric Blake @ 2017-03-23 18:14 UTC (permalink / raw)
To: Pradeep Jagadeesh, Greg Kurz
Cc: Jani Kokkonen, Alberto Garcia, Pradeep Jagadeesh, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 747 bytes --]
On 03/23/2017 09:46 AM, Eric Blake wrote:
> On 03/23/2017 07:20 AM, Pradeep Jagadeesh wrote:
>> This patchset removes the throttle redundant code that was present
>> in block and fsdev files.
>>
>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>> ---
>
> I think you want portions of this patch to come first; you want to
> refactor out the IOThrottle portion of existing types, and then extend
> IOThrottle to be used in more places.
For the record, here's a nice example of how to factor out common fields
into a new base class:
https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg04649.html
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2 v1] throttle: factor out the redundant code
2017-03-23 18:14 ` Eric Blake
@ 2017-03-24 14:09 ` Pradeep Jagadeesh
0 siblings, 0 replies; 8+ messages in thread
From: Pradeep Jagadeesh @ 2017-03-24 14:09 UTC (permalink / raw)
To: Eric Blake, Pradeep Jagadeesh, Greg Kurz
Cc: Jani Kokkonen, Alberto Garcia, qemu-devel
On 3/23/2017 7:14 PM, Eric Blake wrote:
> On 03/23/2017 09:46 AM, Eric Blake wrote:
>> On 03/23/2017 07:20 AM, Pradeep Jagadeesh wrote:
>>> This patchset removes the throttle redundant code that was present
>>> in block and fsdev files.
>>>
>>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>>> ---
>>
>> I think you want portions of this patch to come first; you want to
>> refactor out the IOThrottle portion of existing types, and then extend
>> IOThrottle to be used in more places.
>
> For the record, here's a nice example of how to factor out common fields
> into a new base class:
>
> https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg04649.html
Nice one, but If I do this way, I can not remove some duplicate code
from initialization code. For example below code.
void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
- BlockIOThrottle throttle = {
- .has_device = true,
- .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"),
- };
>
Also
void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
- IOThrottle 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"),
- };
-
Regards,
Pradeep
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2 v1] throttle: factor out the redundant code
2017-03-23 14:46 ` Eric Blake
2017-03-23 18:14 ` Eric Blake
@ 2017-03-28 11:11 ` Pradeep Jagadeesh
1 sibling, 0 replies; 8+ messages in thread
From: Pradeep Jagadeesh @ 2017-03-28 11:11 UTC (permalink / raw)
To: Eric Blake, Pradeep Jagadeesh, Greg Kurz
Cc: Alberto Garcia, Jani Kokkonen, qemu-devel
On 3/23/2017 3:46 PM, Eric Blake wrote:
> On 03/23/2017 07:20 AM, Pradeep Jagadeesh wrote:
>> This patchset removes the throttle redundant code that was present
>> in block and fsdev files.
>>
>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>> ---
>
> I think you want portions of this patch to come first; you want to
> refactor out the IOThrottle portion of existing types, and then extend
> IOThrottle to be used in more places.
OK, I will do that.
>
>> blockdev.c | 106 ++++-----------------------------------
>> fsdev/Makefile.objs | 1 +
>> fsdev/qemu-fsdev-throttle.c | 94 +---------------------------------
>> fsdev/qemu-fsdev-throttle.h | 3 +-
>> hmp.c | 39 +++++++--------
>> include/qemu/throttle-options.h | 5 ++
>> qapi/block-core.json | 76 +---------------------------
>> qapi/iothrottle.json | 4 +-
>> util/Makefile.objs | 1 +
>> util/throttle-options.c | 108 ++++++++++++++++++++++++++++++++++++++++
>> 10 files changed, 151 insertions(+), 286 deletions(-)
>> create mode 100644 util/throttle-options.c
>>
>
>> +++ b/fsdev/qemu-fsdev-throttle.c
>> @@ -33,56 +33,7 @@ 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;
>> -
>
> This code was added in the last patch - which makes for a lot of
> unnecessary churn. Again, the best series does refactoring of existing
> code first, then adds the new code that takes advantage of the
> refactored entry points, so that the new code isn't churning.
>
OK
>
>> +++ b/hmp.c
>> @@ -1554,20 +1554,25 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>> hmp_handle_error(mon, &err);
>> }
>>
>> +static void hmp_initialize_io_throttle(IOThrottle *iot, const QDict *qdict)
>> +{
>> + iot->has_device = true;
>> + iot->device = (char *) qdict_get_str(qdict, "device");
>
> Is casting away const going to result in a double free of memory later
> on? Or put another way, should this be a g_strdup'd copy (which you
> then have to make sure is not leaked)?
This code was there in block_set_io_throttle(). I have just moved to new
function. But I will think about it.
>
>> + iot->bps = qdict_get_int(qdict, "bps");
>> + iot->bps_rd = qdict_get_int(qdict, "bps_rd");
>> + iot->bps_wr = qdict_get_int(qdict, "bps_wr");
>> + iot->iops = qdict_get_int(qdict, "iops");
>> + iot->iops_rd = qdict_get_int(qdict, "iops_rd");
>> + iot->iops_wr = qdict_get_int(qdict, "iops_wr");
>> +}
>> +
>> void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
>> {
>> Error *err = NULL;
>> - BlockIOThrottle throttle = {
>> - .has_device = true,
>> - .device = (char *) qdict_get_str(qdict, "device"),
>
> At any rate, I see it is just code motion. You are refactoring too much
> in one patch here; I'd consider splitting it along the lines of one
> patch that adds hmp_initialize_io_throttle; and another patch that adds
> parse_io_throttle_options(), so that it's easier to see just one piece
> of code motion at a time, rather than trying to track multiple motions
> in the same large patch.
I will split the code into different patches.
>
>> +++ b/qapi/block-core.json
>> @@ -1758,86 +1758,14 @@
>> #
>> # A set of parameters describing block throttling.
>> #
>
>> -# @iops_size: an I/O size in bytes (Since 1.7)
>> +# @iothrottle: throttle configuration (Since 1.7)
>
> NACK. This is an incompatible change; you are breaking the QMP wire
> structure (that is, where I used to pass "arguments":{"device":"foo",
> "bps_rd":1, "group":"bar" }, your change would now require me to pass
> "arguments":{"iothrottle":{"device":"foo", "bps_rd":1}, "group":"bar"};
> the extra nesting is what breaks things).
>
>> #
>> # @group: throttle group name (Since 2.4)
>> #
>> # Since: 1.1
>> ##
>> { 'struct': 'BlockIOThrottle',
>> - 'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
>> - 'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
>> - '*bps_max': 'int', '*bps_rd_max': 'int',
>> - '*bps_wr_max': 'int', '*iops_max': 'int',
>> - '*iops_rd_max': 'int', '*iops_wr_max': 'int',
>> - '*bps_max_length': 'int', '*bps_rd_max_length': 'int',
>> - '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
>> - '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
>> - '*iops_size': 'int', '*group': 'str' } }
>> + 'data': { '*iothrottle': 'IOThrottle', '*group': 'str' } }
>
> What you REALLY want is a compatible change, namely:
>
> { 'struct': 'BlockIOThrottle', 'base': 'IOThrottle',
> 'data': { '*group': 'str' } }
>
> which says that all fields of IOThrottle are inherited at the same level
> as the additional field 'group'.
>
> For that matter, are you sure that 'group' is the only field that should
> not be directly in IOThrottle, or should 'device' also be specific to
> BlockIOThrottle (I'm not sure whether you were actually using 'device'
> in the fsdev case, or if 'id' was sufficient).
Yes, I just need id in case of fsdev. I will fix that. But if I do this
way, some of the code refactoring is not possible.Because they will have
different structures for fsdev and block.
>
>>
>> ##
>> # @block-stream:
>> diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
>> index f7b615d..58f4520 100644
>> --- a/qapi/iothrottle.json
>> +++ b/qapi/iothrottle.json
>> @@ -14,6 +14,8 @@
>> #
>> # @device: Block device name
>> #
>> +# @id: The name or QOM path of the guest device (since: 2.8)
>> +#
>
> You've missed 2.8 by a long shot. Oh - this is an argument that it is
> 'id' (and not 'device') that does not belong here, but should be placed
> alongside 'group' in the BlockIOThrottle subtype.
id is required for the fsdev/block both. This was part of the
BlockIOThrottle. So I just took it and kept the description as it is.
What should I changed to? 2.10?
>
>> # @bps: total throughput limit in bytes per second
>> #
>> # @bps_rd: read throughput limit in bytes per second
>> @@ -80,7 +82,7 @@
>> # Since: 2.10
>> ##
>> { 'struct': 'IOThrottle',
>> - 'data': { '*device': 'str', 'bps': 'int', 'bps_rd': 'int',
>> + 'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
>> 'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
>> '*bps_max': 'int', '*bps_rd_max': 'int',
>> '*bps_wr_max': 'int', '*iops_max': 'int',
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2 v1] fsdev: QMP interface for throttling
2017-03-23 14:32 ` [Qemu-devel] [PATCH 1/2 v1] fsdev: QMP interface for throttling Eric Blake
@ 2017-03-28 11:27 ` Pradeep Jagadeesh
0 siblings, 0 replies; 8+ messages in thread
From: Pradeep Jagadeesh @ 2017-03-28 11:27 UTC (permalink / raw)
To: Eric Blake, Pradeep Jagadeesh, Greg Kurz, Daniel P.Berrange
Cc: Alberto Garcia, Jani Kokkonen, qemu-devel
On 3/23/2017 3:32 PM, Eric Blake wrote:
> On 03/23/2017 07:20 AM, Pradeep Jagadeesh wrote:
>> This patchset enables qmp interfaces for the 9pfs
>> devices (fsdev). This provides two interfaces one
>
> s/interfaces/interfaces,/
>
> Also, I just noticed you are using trailing spaces in your commit
> message (not fatal, but unusual).
>
Will fix.
>> for querying all the 9pfs devices info. The second one
>> to set the IO limits for the required 9pfs device.
>
> When sending a multi-patch series, please remember to include the 0/2
> cover letter ('git config format.coverletter auto' can help here).
OK, I will have a look.
>
>>
>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>> ---
>> Makefile | 5 ++-
>> fsdev/qemu-fsdev-throttle.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
>> fsdev/qemu-fsdev-throttle.h | 11 ++++++
>> fsdev/qemu-fsdev.c | 38 +++++++++++++++++-
>> fsdev/qemu-fsdev.h | 2 +-
>> hmp-commands-info.hx | 18 +++++++++
>> hmp-commands.hx | 18 +++++++++
>> hmp.c | 74 ++++++++++++++++++++++++++++++++++
>> hmp.h | 5 +++
>> qapi-schema.json | 6 +++
>> qapi/fsdev.json | 87 ++++++++++++++++++++++++++++++++++++++++
>> qapi/iothrottle.json | 93 +++++++++++++++++++++++++++++++++++++++++++
>> qmp.c | 14 +++++++
>> 13 files changed, 464 insertions(+), 3 deletions(-)
>
> That's a big patch to review in one go. Does it make sense to break it
> down into smaller chunks?
>
>> create mode 100644 qapi/fsdev.json
>> create mode 100644 qapi/iothrottle.json
>>
>> v0 -> v1:
>>
>> Addressed the comments by Erik Blake, Greg Kurz and Daniel P.Berrange
>
> It's Eric, but you're not the first (and probably not the last) to get
> it wrong.
:) will remember.
>
>> Mainly renaming the functions and removing the redundant code
>>
>> diff --git a/Makefile b/Makefile
>> index 73e0c12..c33b46d 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -413,7 +413,10 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
>> $(SRC_PATH)/qapi/block.json $(SRC_PATH)/qapi/block-core.json \
>> $(SRC_PATH)/qapi/event.json $(SRC_PATH)/qapi/introspect.json \
>> $(SRC_PATH)/qapi/crypto.json $(SRC_PATH)/qapi/rocker.json \
>> - $(SRC_PATH)/qapi/trace.json
>> + $(SRC_PATH)/qapi/trace.json $(SRC_PATH)/qapi/iothrottle.json
>
> Why two spaces?
>
Will fix it.
>
>> +++ b/qapi-schema.json
>> @@ -78,9 +78,15 @@
>> # QAPI crypto definitions
>> { 'include': 'qapi/crypto.json' }
>>
>> +# QAPI IOThrottle definitions
>> +{ 'include': 'qapi/iothrottle.json' }
>> +
>> # QAPI block definitions
>> { 'include': 'qapi/block.json' }
>>
>> +# QAPI fsdev definitions
>> +{ 'include': 'qapi/fsdev.json' }
>> +
>
> Adding two new files definitely feels like something that could be
> split. Also, is it possible that existing code could take advantage of
> qapi/iothrottle.json, rather than it just being additional code?
I will check and remove unnecessary includes.
>
>> # QAPI event definitions
>> { 'include': 'qapi/event.json' }
>>
>> diff --git a/qapi/fsdev.json b/qapi/fsdev.json
>> new file mode 100644
>> index 0000000..7fb3c25
>> --- /dev/null
>> +++ b/qapi/fsdev.json
>> @@ -0,0 +1,87 @@
>> +# -*- Mode: Python -*-
>> +
>> +##
>> +# == QAPI fsdev definitions
>> +##
>> +
>> +# QAPI common definitions
>> +{ 'include': 'common.json' }
>> +{ 'include': 'iothrottle.json' }
>
> Is this really necessary? Given that the top level already included
> these, I think it serves more as a documentation purpose than something
> you actually need.
Not really, will fix this.
>
>> +
>> +##
>> +# @fsdev_set_io_throttle:
>> +#
>> +# Change I/O limits for a 9p/fsdev device.
>> +#
>> +# I/O limits can be enabled by setting throttle vaue to non-zero numbers.
>
> s/vaue/value/
ok
>
>> +#
>> +# I/O limits can be disabled by setting all of them to 0.
>> +#
>> +# Returns: Nothing on success
>> +# If @device is not a valid fsdev device, DeviceNotFound
>> +#
>> +# Since: 2.10
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "fsdev_set_io_throttle",
>
> Please name this fsdev-set-io-throttle. New commands should favor the
> use of '-' rather than '_'.
>
>
ok
>> +##
>> +# @query-fsdev-io-throttle:
>> +#
>> +# Return a list of information about fsdev device
>> +#
>> +# Returns: @FsdevIOIOThrottle
>
> Typo in the type name, should be merely IOThrottle
ok
>
>> +#
>> +# Since: 2.10
>> +#
>> +# Example:
>> +#
>> +# -> { "Execute": "query-fsdev-io-throttle" }
>
>> +#
>> +##
>> +{ 'command': 'query-fsdev-io-throttle', 'returns': [ 'IOThrottle' ] }
>> +
>> diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
>> new file mode 100644
>> index 0000000..f7b615d
>> --- /dev/null
>> +++ b/qapi/iothrottle.json
>> @@ -0,0 +1,93 @@
>> +# -*- Mode: Python -*-
>> +
>> +##
>> +# == QAPI IOThrottle definitions
>> +##
>> +
>> +# QAPI common definitions
>> +{ 'include': 'common.json' }
>
> Again, is this necessary?
Removed.
>
>
>> +# @iops_size: an I/O size in bytes (Since 1.7)
>> +#
>> +#
>> +# Since: 2.10
>
> It's weird to state that this struct is since 2.10, but its members are
> since 1.7. Either you should list a 'since' for when the members of the
> struct were first useful (0.14.0), or eliminate all the (since ...) tags
> on the members and keep the overall struct at 2.10. I'd lean towards
> the former, at least if BlockDeviceInfo is properly refactored to
> inherit from this type.
Some members of this structure have Since, not all of them. So, shall I
remove since tag from all?
As of now I used as it is from BlockIOThrottle. Not changed any since tags.
OK, I will try to refactor the BlockDeviceInfo using this structure.
>
>> +##
>> +{ 'struct': 'IOThrottle',
>> + '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',
>> + '*bps_wr_max': 'int', '*iops_max': 'int',
>> + '*iops_rd_max': 'int', '*iops_wr_max': 'int',
>> + '*bps_max_length': 'int', '*bps_rd_max_length': 'int',
>> + '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
>> + '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
>> + '*iops_size': 'int' } }
>
> So yeah, you definitely need to split this patch. Your FIRST patch
> should be the creation of IOThrottle, and fixing BlockDeviceInfo to
> inherit from it; then a later patch would be the introduction of the
> fsdev throttle commands that are additional clients of the new
> IOThrottle type.
OK
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-03-28 11:28 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-23 12:20 [Qemu-devel] [PATCH 1/2 v1] fsdev: QMP interface for throttling Pradeep Jagadeesh
2017-03-23 12:20 ` [Qemu-devel] [PATCH 2/2 v1] throttle: factor out the redundant code Pradeep Jagadeesh
2017-03-23 14:46 ` Eric Blake
2017-03-23 18:14 ` Eric Blake
2017-03-24 14:09 ` Pradeep Jagadeesh
2017-03-28 11:11 ` Pradeep Jagadeesh
2017-03-23 14:32 ` [Qemu-devel] [PATCH 1/2 v1] fsdev: QMP interface for throttling Eric Blake
2017-03-28 11:27 ` Pradeep Jagadeesh
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.