* [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling
@ 2017-07-04 15:30 Pradeep Jagadeesh
2017-07-04 15:30 ` [Qemu-devel] [PATCH v7 1/6] throttle: factor out duplicate code Pradeep Jagadeesh
` (7 more replies)
0 siblings, 8 replies; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-07-04 15:30 UTC (permalink / raw)
To: eric blake, greg kurz
Cc: Pradeep Jagadeesh, alberto garcia, Markus Armbruster,
Dr. David Alan Gilbert, jani kokkonen, qemu-devel
These patches provide the qmp interface, to query the io throttle
status of the all fsdev devices that are present in a vm.
also, it provides an interface to set the io throttle parameters of a
fsdev to a required value. some of the patches also remove the duplicate
code that was present in block and fsdev files.
Pradeep Jagadeesh (6):
throttle: factor out duplicate code
qmp: Create IOThrottle structure
throttle: move out function to reuse the code
hmp: create a throttle initialization function for code reusability
fsdev: hmp interface for throttling
fsdev: QMP interface for throttling
Makefile | 4 ++
blockdev.c | 97 ++-------------------------------
fsdev/qemu-fsdev-dummy.c | 10 ++++
fsdev/qemu-fsdev-throttle.c | 118 ++++++++++++++++++++++++++--------------
fsdev/qemu-fsdev-throttle.h | 13 +++++
fsdev/qemu-fsdev.c | 37 +++++++++++++
hmp-commands-info.hx | 18 ++++++
hmp-commands.hx | 19 +++++++
hmp.c | 81 +++++++++++++++++++++++++--
hmp.h | 4 ++
include/qemu/throttle-options.h | 7 +++
include/qemu/throttle.h | 4 +-
include/qemu/typedefs.h | 1 +
monitor.c | 5 ++
qapi-schema.json | 3 +
qapi/block-core.json | 76 +-------------------------
qapi/fsdev.json | 84 ++++++++++++++++++++++++++++
qapi/iothrottle.json | 88 ++++++++++++++++++++++++++++++
qmp.c | 14 +++++
util/throttle.c | 110 +++++++++++++++++++++++++++++++++++++
20 files changed, 577 insertions(+), 216 deletions(-)
create mode 100644 qapi/fsdev.json
create mode 100644 qapi/iothrottle.json
v0 -> v1:
Addressed comments from Eric Blake, Greg Kurz and Daniel P.Berrange
Mainly renaming the functions and removing the redundant code.
v1 -> v2:
Addressed comments from Eric Blake and Greg Kurz.
As per the suggestion I split the patches into smaller patches.
Removed some more duplicate code.
v2 -> v3:
Addresssed comments from Alberto Garcia.
Changed the comment from block to iothrottle in the iothrottle.json
Added the dummy functions in qemu-fsdev-dummy.c to address the compilation
issues that were observed.
v3 -> v4:
Addressed comments from Eric Blake and Greg Kurz
Re-ordered the patches
Added the dummy functions in qmp.c to address the cross compilation issues
v4 -> v5:
Addressed comments from Eric Blake and Greg Kurz
Split the fsdev qmp patch into hmp and qmp related patches
Moved the common functionalities to throttle.c instead of creating
a new file
v5 -> v6:
Addressed comments from Greg Kurz and Markus Armbruster
Split the commits to specific to hmp and throttle as suggested by Greg
Moved ThrottleConfig typedef to qemu/typedefs.h
Addressed compilation issue on FreeBSD by adding flags in qmp.c
v6 -> v7:
Addressed comments from Albert Garcia and Dr. David Alan Gilbert
Fixed the hmp-commands-info.hx and hmp-commands.hx as per Dr. David's
comments.
Fixed the bug with the hmp fsdev_set_io_throttle and info fsdev_iothrottle
--
1.8.3.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v7 1/6] throttle: factor out duplicate code
2017-07-04 15:30 [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Pradeep Jagadeesh
@ 2017-07-04 15:30 ` Pradeep Jagadeesh
2017-07-10 14:33 ` Greg Kurz
2017-07-10 14:41 ` Eric Blake
2017-07-04 15:30 ` [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure Pradeep Jagadeesh
` (6 subsequent siblings)
7 siblings, 2 replies; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-07-04 15:30 UTC (permalink / raw)
To: eric blake, greg kurz
Cc: Pradeep Jagadeesh, alberto garcia, Markus Armbruster,
jani kokkonen, qemu-devel
This patch factor out the duplicate throttle code that was present in
block and fsdev devices.
Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
blockdev.c | 44 +----------------------------------
fsdev/qemu-fsdev-throttle.c | 44 ++---------------------------------
include/qemu/throttle-options.h | 4 ++++
include/qemu/throttle.h | 4 ++--
include/qemu/typedefs.h | 1 +
util/throttle.c | 51 +++++++++++++++++++++++++++++++++++++++++
6 files changed, 61 insertions(+), 87 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index f92dcf2..a95ea41 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -387,49 +387,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);
-
+ throttle_parse_options(throttle_cfg, opts);
if (!throttle_is_valid(throttle_cfg, errp)) {
return;
}
diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
index 7ae4e86..c5e2499 100644
--- a/fsdev/qemu-fsdev-throttle.c
+++ b/fsdev/qemu-fsdev-throttle.c
@@ -16,6 +16,7 @@
#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)
{
@@ -31,48 +32,7 @@ static void fsdev_throttle_write_timer_cb(void *opaque)
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);
-
+ throttle_parse_options(&fst->cfg, opts);
throttle_is_valid(&fst->cfg, errp);
}
diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
index 3133d1c..f63d38c 100644
--- a/include/qemu/throttle-options.h
+++ b/include/qemu/throttle-options.h
@@ -10,6 +10,8 @@
#ifndef THROTTLE_OPTIONS_H
#define THROTTLE_OPTIONS_H
+#include "typedefs.h"
+
#define THROTTLE_OPTS \
{ \
.name = "throttling.iops-total",\
@@ -89,4 +91,6 @@
.help = "when limiting by iops max size of an I/O in bytes",\
}
+void throttle_parse_options(ThrottleConfig *, QemuOpts *);
+
#endif
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 9109657..8eb5501 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -89,10 +89,10 @@ typedef struct LeakyBucket {
* However it allows to keep the code clean and the bucket field is reset to
* zero at the right time.
*/
-typedef struct ThrottleConfig {
+struct ThrottleConfig {
LeakyBucket buckets[BUCKETS_COUNT]; /* leaky buckets */
uint64_t op_size; /* size of an operation in bytes */
-} ThrottleConfig;
+};
typedef struct ThrottleState {
ThrottleConfig cfg; /* configuration */
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 2706aab..041c0c4 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -97,6 +97,7 @@ typedef struct uWireSlave uWireSlave;
typedef struct VirtIODevice VirtIODevice;
typedef struct Visitor Visitor;
typedef struct node_info NodeInfo;
+typedef struct ThrottleConfig ThrottleConfig;
typedef void SaveStateHandler(QEMUFile *f, void *opaque);
typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
diff --git a/util/throttle.c b/util/throttle.c
index 3570ed2..a751126 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -27,6 +27,7 @@
#include "qemu/throttle.h"
#include "qemu/timer.h"
#include "block/aio.h"
+#include "qemu/throttle-options.h"
/* This function make a bucket leak
*
@@ -514,3 +515,53 @@ void throttle_account(ThrottleState *ts, bool is_write, uint64_t size)
}
}
+/* parse the throttle options
+ *
+ * @opts: qemu options
+ * @throttle_cfg: throttle configuration
+ */
+void throttle_parse_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] 40+ messages in thread
* [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
2017-07-04 15:30 [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Pradeep Jagadeesh
2017-07-04 15:30 ` [Qemu-devel] [PATCH v7 1/6] throttle: factor out duplicate code Pradeep Jagadeesh
@ 2017-07-04 15:30 ` Pradeep Jagadeesh
2017-07-06 17:55 ` Markus Armbruster
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 3/6] throttle: move out function to reuse the code Pradeep Jagadeesh
` (5 subsequent siblings)
7 siblings, 1 reply; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-07-04 15:30 UTC (permalink / raw)
To: eric blake, greg kurz
Cc: Pradeep Jagadeesh, alberto garcia, jani kokkonen, qemu-devel
This patch enables qmp interfaces for the fsdev
devices. This provides two interfaces one
for querying info of all the fsdev devices. The second one
to set the IO limits for the required fsdev device.
Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
qapi/block-core.json | 76 ++-------------------------------------------
qapi/iothrottle.json | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 91 insertions(+), 73 deletions(-)
create mode 100644 qapi/iothrottle.json
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f85c223..9320974 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -6,6 +6,7 @@
# QAPI common definitions
{ 'include': 'common.json' }
+{ 'include': 'iothrottle.json' }
##
# @SnapshotInfo:
@@ -1761,84 +1762,13 @@
#
# @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)
-#
# @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' } }
+ 'base': 'IOThrottle',
+ 'data': { '*device': 'str', '*group': 'str' } }
##
# @block-stream:
diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
new file mode 100644
index 0000000..0f067c3
--- /dev/null
+++ b/qapi/iothrottle.json
@@ -0,0 +1,88 @@
+# -*- Mode: Python -*-
+
+##
+# == QAPI IOThrottle definitions
+##
+
+##
+# @IOThrottle:
+#
+# A set of parameters describing IO throttling
+#
+# @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)
+#
+#
+# Since: 2.10
+##
+{ 'struct': 'IOThrottle',
+ 'data': { '*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' } }
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v7 3/6] throttle: move out function to reuse the code
2017-07-04 15:30 [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Pradeep Jagadeesh
2017-07-04 15:30 ` [Qemu-devel] [PATCH v7 1/6] throttle: factor out duplicate code Pradeep Jagadeesh
2017-07-04 15:30 ` [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure Pradeep Jagadeesh
@ 2017-07-04 15:31 ` Pradeep Jagadeesh
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 4/6] hmp: create a throttle initialization function for code reusability Pradeep Jagadeesh
` (4 subsequent siblings)
7 siblings, 0 replies; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-07-04 15:31 UTC (permalink / raw)
To: eric blake, greg kurz
Cc: Pradeep Jagadeesh, alberto garcia, Markus Armbruster,
jani kokkonen, qemu-devel
This patch move out the throttle code to util/throttle.c to maximize
the reusability of the code.The same code is also used by fsdev.
Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
---
blockdev.c | 53 +++---------------------------------
include/qemu/throttle-options.h | 3 +++
util/throttle.c | 59 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 65 insertions(+), 50 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index a95ea41..8090798 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2579,6 +2579,7 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
BlockDriverState *bs;
BlockBackend *blk;
AioContext *aio_context;
+ IOThrottle *iothrottle;
blk = qmp_get_blk(arg->has_device ? arg->device : NULL,
arg->has_id ? arg->id : NULL,
@@ -2596,56 +2597,8 @@ 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;
- }
+ iothrottle = qapi_BlockIOThrottle_base(arg);
+ throttle_set_io_limits(&cfg, iothrottle);
if (!throttle_is_valid(&cfg, errp)) {
goto out;
diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
index f63d38c..a9deb8e 100644
--- a/include/qemu/throttle-options.h
+++ b/include/qemu/throttle-options.h
@@ -11,6 +11,7 @@
#define THROTTLE_OPTIONS_H
#include "typedefs.h"
+#include "qapi-types.h"
#define THROTTLE_OPTS \
{ \
@@ -93,4 +94,6 @@
void throttle_parse_options(ThrottleConfig *, QemuOpts *);
+void throttle_set_io_limits(ThrottleConfig *, IOThrottle *);
+
#endif
diff --git a/util/throttle.c b/util/throttle.c
index a751126..2cf9ec5 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -565,3 +565,62 @@ void throttle_parse_options(ThrottleConfig *throttle_cfg, QemuOpts *opts)
throttle_cfg->op_size =
qemu_opt_get_number(opts, "throttling.iops-size", 0);
}
+
+/* set the throttle limits
+ *
+ * @arg: iothrottle limits
+ * @cfg: throttle configuration
+ */
+void throttle_set_io_limits(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;
+ }
+}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v7 4/6] hmp: create a throttle initialization function for code reusability
2017-07-04 15:30 [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Pradeep Jagadeesh
` (2 preceding siblings ...)
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 3/6] throttle: move out function to reuse the code Pradeep Jagadeesh
@ 2017-07-04 15:31 ` Pradeep Jagadeesh
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 5/6] fsdev: hmp interface for throttling Pradeep Jagadeesh
` (3 subsequent siblings)
7 siblings, 0 replies; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-07-04 15:31 UTC (permalink / raw)
To: eric blake, greg kurz
Cc: Pradeep Jagadeesh, alberto garcia, Dr. David Alan Gilbert,
jani kokkonen, qemu-devel
This patch creates a throttle initialization function to maximize the
code reusability. The same code is also used by fsdev.
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
---
hmp.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/hmp.c b/hmp.c
index 8c72c58..3f76073 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1749,20 +1749,27 @@ 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->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;
+ IOThrottle *iothrottle;
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"),
};
+ iothrottle = qapi_BlockIOThrottle_base(&throttle);
+ hmp_initialize_io_throttle(iothrottle, qdict);
qmp_block_set_io_throttle(&throttle, &err);
hmp_handle_error(mon, &err);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v7 5/6] fsdev: hmp interface for throttling
2017-07-04 15:30 [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Pradeep Jagadeesh
` (3 preceding siblings ...)
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 4/6] hmp: create a throttle initialization function for code reusability Pradeep Jagadeesh
@ 2017-07-04 15:31 ` Pradeep Jagadeesh
2017-07-05 10:45 ` Dr. David Alan Gilbert
2017-07-06 18:15 ` Markus Armbruster
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 6/6] fsdev: QMP " Pradeep Jagadeesh
` (2 subsequent siblings)
7 siblings, 2 replies; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-07-04 15:31 UTC (permalink / raw)
To: eric blake, greg kurz
Cc: Pradeep Jagadeesh, alberto garcia, Dr. David Alan Gilbert,
jani kokkonen, qemu-devel
This patch introduces hmp interfaces for the fsdev
devices.
Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
---
hmp-commands-info.hx | 18 +++++++++++++++
hmp-commands.hx | 19 ++++++++++++++++
hmp.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
hmp.h | 4 ++++
4 files changed, 103 insertions(+)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ae16901..5e4ea51 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 = "fsdev_iothrottle",
+ .args_type = "",
+ .params = "",
+ .help = "show fsdev iothrottle information",
+ .cmd = hmp_info_fsdev_iothrottle,
+ },
+
+#endif
+
+STEXI
+@item info fsdev_iothrottle
+@findex fsdev_iothrottle
+Show fsdev device throttle info.
+ETEXI
+
{
.name = "block-jobs",
.args_type = "",
diff --git a/hmp-commands.hx b/hmp-commands.hx
index e763606..b7eb9a6 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1662,6 +1662,25 @@ 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 fs devices",
+ .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 fs devices 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 3f76073..97014d4 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1774,6 +1774,68 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, &err);
}
+#ifdef CONFIG_VIRTFS
+
+void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict)
+{
+ Error *err = NULL;
+ IOThrottle throttle = {
+ .has_id = true,
+ .id = (char *) qdict_get_str(qdict, "device"),
+ };
+
+ hmp_initialize_io_throttle(&throttle, qdict);
+ qmp_fsdev_set_io_throttle(&throttle, &err);
+ hmp_handle_error(mon, &err);
+}
+
+static void print_fsdev_throttle_config(Monitor *mon, IOThrottle *fscfg,
+ Error *err)
+{
+ monitor_printf(mon, "%s", fscfg->id);
+ 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
+ "\n",
+ 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_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict)
+{
+ Error *err = NULL;
+ IOThrottleList *fsdev_list, *info;
+ fsdev_list = qmp_query_fsdev_io_throttle(&err);
+
+ for (info = fsdev_list; info; info = info->next) {
+ print_fsdev_throttle_config(mon, info->value, err);
+ }
+ qapi_free_IOThrottleList(fsdev_list);
+}
+
+#endif
+
void hmp_block_stream(Monitor *mon, const QDict *qdict)
{
Error *error = NULL;
diff --git a/hmp.h b/hmp.h
index d8b94ce..dbf024d 100644
--- a/hmp.h
+++ b/hmp.h
@@ -81,6 +81,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_info_fsdev_iothrottle(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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v7 6/6] fsdev: QMP interface for throttling
2017-07-04 15:30 [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Pradeep Jagadeesh
` (4 preceding siblings ...)
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 5/6] fsdev: hmp interface for throttling Pradeep Jagadeesh
@ 2017-07-04 15:31 ` Pradeep Jagadeesh
2017-07-06 18:47 ` Markus Armbruster
2017-07-07 6:14 ` [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Markus Armbruster
2017-07-14 12:22 ` Manos Pitsidianakis
7 siblings, 1 reply; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-07-04 15:31 UTC (permalink / raw)
To: eric blake, greg kurz
Cc: Pradeep Jagadeesh, alberto garcia, Dr. David Alan Gilbert,
jani kokkonen, qemu-devel
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
+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 */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 5/6] fsdev: hmp interface for throttling
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 5/6] fsdev: hmp interface for throttling Pradeep Jagadeesh
@ 2017-07-05 10:45 ` Dr. David Alan Gilbert
2017-07-06 18:15 ` Markus Armbruster
1 sibling, 0 replies; 40+ messages in thread
From: Dr. David Alan Gilbert @ 2017-07-05 10:45 UTC (permalink / raw)
To: Pradeep Jagadeesh
Cc: eric blake, greg kurz, Pradeep Jagadeesh, alberto garcia,
jani kokkonen, qemu-devel
* Pradeep Jagadeesh (pradeepkiruvale@gmail.com) wrote:
> This patch introduces hmp interfaces for the fsdev
> devices.
>
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
Yes I think that's better;
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> hmp-commands-info.hx | 18 +++++++++++++++
> hmp-commands.hx | 19 ++++++++++++++++
> hmp.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> hmp.h | 4 ++++
> 4 files changed, 103 insertions(+)
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index ae16901..5e4ea51 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 = "fsdev_iothrottle",
> + .args_type = "",
> + .params = "",
> + .help = "show fsdev iothrottle information",
> + .cmd = hmp_info_fsdev_iothrottle,
> + },
> +
> +#endif
> +
> +STEXI
> +@item info fsdev_iothrottle
> +@findex fsdev_iothrottle
> +Show fsdev device throttle info.
> +ETEXI
> +
> {
> .name = "block-jobs",
> .args_type = "",
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index e763606..b7eb9a6 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1662,6 +1662,25 @@ 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 fs devices",
> + .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 fs devices 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 3f76073..97014d4 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1774,6 +1774,68 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
> hmp_handle_error(mon, &err);
> }
>
> +#ifdef CONFIG_VIRTFS
> +
> +void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict)
> +{
> + Error *err = NULL;
> + IOThrottle throttle = {
> + .has_id = true,
> + .id = (char *) qdict_get_str(qdict, "device"),
> + };
> +
> + hmp_initialize_io_throttle(&throttle, qdict);
> + qmp_fsdev_set_io_throttle(&throttle, &err);
> + hmp_handle_error(mon, &err);
> +}
> +
> +static void print_fsdev_throttle_config(Monitor *mon, IOThrottle *fscfg,
> + Error *err)
> +{
> + monitor_printf(mon, "%s", fscfg->id);
> + 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
> + "\n",
> + 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_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict)
> +{
> + Error *err = NULL;
> + IOThrottleList *fsdev_list, *info;
> + fsdev_list = qmp_query_fsdev_io_throttle(&err);
> +
> + for (info = fsdev_list; info; info = info->next) {
> + print_fsdev_throttle_config(mon, info->value, err);
> + }
> + qapi_free_IOThrottleList(fsdev_list);
> +}
> +
> +#endif
> +
> void hmp_block_stream(Monitor *mon, const QDict *qdict)
> {
> Error *error = NULL;
> diff --git a/hmp.h b/hmp.h
> index d8b94ce..dbf024d 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -81,6 +81,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_info_fsdev_iothrottle(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);
> --
> 1.8.3.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
2017-07-04 15:30 ` [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure Pradeep Jagadeesh
@ 2017-07-06 17:55 ` Markus Armbruster
2017-08-07 9:59 ` Pradeep Jagadeesh
0 siblings, 1 reply; 40+ messages in thread
From: Markus Armbruster @ 2017-07-06 17:55 UTC (permalink / raw)
To: Pradeep Jagadeesh
Cc: eric blake, greg kurz, jani kokkonen, alberto garcia,
Pradeep Jagadeesh, qemu-devel
Pradeep Jagadeesh <pradeepkiruvale@gmail.com> writes:
> This patch enables qmp interfaces for the fsdev
> devices. This provides two interfaces one
> for querying info of all the fsdev devices. The second one
> to set the IO limits for the required fsdev device.
>
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
> qapi/block-core.json | 76 ++-------------------------------------------
> qapi/iothrottle.json | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 91 insertions(+), 73 deletions(-)
> create mode 100644 qapi/iothrottle.json
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index f85c223..9320974 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -6,6 +6,7 @@
>
> # QAPI common definitions
> { 'include': 'common.json' }
> +{ 'include': 'iothrottle.json' }
>
> ##
> # @SnapshotInfo:
> @@ -1761,84 +1762,13 @@
> #
> # @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)
> -#
> # @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' } }
> + 'base': 'IOThrottle',
> + 'data': { '*device': 'str', '*group': 'str' } }
>
> ##
> # @block-stream:
> diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
> new file mode 100644
> index 0000000..0f067c3
> --- /dev/null
> +++ b/qapi/iothrottle.json
> @@ -0,0 +1,88 @@
> +# -*- Mode: Python -*-
> +
> +##
> +# == QAPI IOThrottle definitions
> +##
> +
> +##
> +# @IOThrottle:
> +#
> +# A set of parameters describing IO throttling
> +#
> +# @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)
> +#
> +#
> +# Since: 2.10
> +##
> +{ 'struct': 'IOThrottle',
> + 'data': { '*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' } }
Awkward question for a v7, but here goes anyway: why is IOThrottle worth
its very own .json file?
Diff can't show the differences (if any), because you factor IOThrottle
out of BlockIOThrottle and move it in a single patch. I had to extract
and diff by hand.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 5/6] fsdev: hmp interface for throttling
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 5/6] fsdev: hmp interface for throttling Pradeep Jagadeesh
2017-07-05 10:45 ` Dr. David Alan Gilbert
@ 2017-07-06 18:15 ` Markus Armbruster
2017-08-07 9:57 ` Pradeep Jagadeesh
1 sibling, 1 reply; 40+ messages in thread
From: Markus Armbruster @ 2017-07-06 18:15 UTC (permalink / raw)
To: Pradeep Jagadeesh
Cc: eric blake, greg kurz, qemu-devel, jani kokkonen, alberto garcia,
Pradeep Jagadeesh, Dr. David Alan Gilbert
Pradeep Jagadeesh <pradeepkiruvale@gmail.com> writes:
> This patch introduces hmp interfaces for the fsdev
> devices.
>
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
Fails to compile, first error:
/work/armbru/qemu/hmp.c: In function ‘hmp_fsdev_set_io_throttle’:
/work/armbru/qemu/hmp.c:1791:5: warning: implicit declaration of function ‘qmp_fsdev_set_io_throttle’ [-Wimplicit-function-declaration]
qmp_fsdev_set_io_throttle(&throttle, &err);
^~~~~~~~~~~~~~~~~~~~~~~~~
Do you need to swap PATCH 5 and 6?
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 6/6] fsdev: QMP interface for throttling
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 6/6] fsdev: QMP " Pradeep Jagadeesh
@ 2017-07-06 18:47 ` Markus Armbruster
2017-07-07 14:50 ` Pradeep Jagadeesh
2017-08-07 9:35 ` Pradeep Jagadeesh
0 siblings, 2 replies; 40+ messages in thread
From: Markus Armbruster @ 2017-07-06 18:47 UTC (permalink / raw)
To: Pradeep Jagadeesh
Cc: eric blake, greg kurz, qemu-devel, jani kokkonen, alberto garcia,
Pradeep Jagadeesh, Dr. David Alan Gilbert
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 */
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling
2017-07-04 15:30 [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Pradeep Jagadeesh
` (5 preceding siblings ...)
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 6/6] fsdev: QMP " Pradeep Jagadeesh
@ 2017-07-07 6:14 ` Markus Armbruster
2017-07-07 6:24 ` Markus Armbruster
` (2 more replies)
2017-07-14 12:22 ` Manos Pitsidianakis
7 siblings, 3 replies; 40+ messages in thread
From: Markus Armbruster @ 2017-07-07 6:14 UTC (permalink / raw)
To: Pradeep Jagadeesh
Cc: eric blake, greg kurz, alberto garcia, qemu-devel,
Dr. David Alan Gilbert, Pradeep Jagadeesh, jani kokkonen
Pradeep Jagadeesh <pradeepkiruvale@gmail.com> writes:
> These patches provide the qmp interface, to query the io throttle
> status of the all fsdev devices that are present in a vm.
> also, it provides an interface to set the io throttle parameters of a
> fsdev to a required value. some of the patches also remove the duplicate
> code that was present in block and fsdev files.
>
> Pradeep Jagadeesh (6):
> throttle: factor out duplicate code
> qmp: Create IOThrottle structure
> throttle: move out function to reuse the code
> hmp: create a throttle initialization function for code reusability
> fsdev: hmp interface for throttling
> fsdev: QMP interface for throttling
>
> Makefile | 4 ++
> blockdev.c | 97 ++-------------------------------
> fsdev/qemu-fsdev-dummy.c | 10 ++++
> fsdev/qemu-fsdev-throttle.c | 118 ++++++++++++++++++++++++++--------------
> fsdev/qemu-fsdev-throttle.h | 13 +++++
> fsdev/qemu-fsdev.c | 37 +++++++++++++
> hmp-commands-info.hx | 18 ++++++
> hmp-commands.hx | 19 +++++++
> hmp.c | 81 +++++++++++++++++++++++++--
> hmp.h | 4 ++
> include/qemu/throttle-options.h | 7 +++
> include/qemu/throttle.h | 4 +-
> include/qemu/typedefs.h | 1 +
> monitor.c | 5 ++
> qapi-schema.json | 3 +
> qapi/block-core.json | 76 +-------------------------
> qapi/fsdev.json | 84 ++++++++++++++++++++++++++++
> qapi/iothrottle.json | 88 ++++++++++++++++++++++++++++++
> qmp.c | 14 +++++
> util/throttle.c | 110 +++++++++++++++++++++++++++++++++++++
> 20 files changed, 577 insertions(+), 216 deletions(-)
> create mode 100644 qapi/fsdev.json
> create mode 100644 qapi/iothrottle.json
No test coverage?
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling
2017-07-07 6:14 ` [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Markus Armbruster
@ 2017-07-07 6:24 ` Markus Armbruster
2017-07-07 13:16 ` Pradeep Jagadeesh
2017-08-07 7:52 ` Pradeep Jagadeesh
2 siblings, 0 replies; 40+ messages in thread
From: Markus Armbruster @ 2017-07-07 6:24 UTC (permalink / raw)
To: Pradeep Jagadeesh
Cc: eric blake, greg kurz, alberto garcia, qemu-devel,
Dr. David Alan Gilbert, Pradeep Jagadeesh, jani kokkonen
PS: Sorry for the late review, got a bit overwhelmed...
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling
2017-07-07 6:14 ` [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Markus Armbruster
2017-07-07 6:24 ` Markus Armbruster
@ 2017-07-07 13:16 ` Pradeep Jagadeesh
2017-08-07 7:52 ` Pradeep Jagadeesh
2 siblings, 0 replies; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-07-07 13:16 UTC (permalink / raw)
To: Markus Armbruster, Pradeep Jagadeesh
Cc: eric blake, greg kurz, alberto garcia, qemu-devel,
Dr. David Alan Gilbert, jani kokkonen
On 7/7/2017 8:14 AM, Markus Armbruster wrote:
> Pradeep Jagadeesh <pradeepkiruvale@gmail.com> writes:
>
>> These patches provide the qmp interface, to query the io throttle
>> status of the all fsdev devices that are present in a vm.
>> also, it provides an interface to set the io throttle parameters of a
>> fsdev to a required value. some of the patches also remove the duplicate
>> code that was present in block and fsdev files.
>>
>> Pradeep Jagadeesh (6):
>> throttle: factor out duplicate code
>> qmp: Create IOThrottle structure
>> throttle: move out function to reuse the code
>> hmp: create a throttle initialization function for code reusability
>> fsdev: hmp interface for throttling
>> fsdev: QMP interface for throttling
>>
>> Makefile | 4 ++
>> blockdev.c | 97 ++-------------------------------
>> fsdev/qemu-fsdev-dummy.c | 10 ++++
>> fsdev/qemu-fsdev-throttle.c | 118 ++++++++++++++++++++++++++--------------
>> fsdev/qemu-fsdev-throttle.h | 13 +++++
>> fsdev/qemu-fsdev.c | 37 +++++++++++++
>> hmp-commands-info.hx | 18 ++++++
>> hmp-commands.hx | 19 +++++++
>> hmp.c | 81 +++++++++++++++++++++++++--
>> hmp.h | 4 ++
>> include/qemu/throttle-options.h | 7 +++
>> include/qemu/throttle.h | 4 +-
>> include/qemu/typedefs.h | 1 +
>> monitor.c | 5 ++
>> qapi-schema.json | 3 +
>> qapi/block-core.json | 76 +-------------------------
>> qapi/fsdev.json | 84 ++++++++++++++++++++++++++++
>> qapi/iothrottle.json | 88 ++++++++++++++++++++++++++++++
>> qmp.c | 14 +++++
>> util/throttle.c | 110 +++++++++++++++++++++++++++++++++++++
>> 20 files changed, 577 insertions(+), 216 deletions(-)
>> create mode 100644 qapi/fsdev.json
>> create mode 100644 qapi/iothrottle.json
>
> No test coverage?
Not yet, I am yet to write the tests. I can do it only after I finish
this work.
-Pradeep
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 6/6] fsdev: QMP interface for throttling
2017-07-06 18:47 ` Markus Armbruster
@ 2017-07-07 14:50 ` Pradeep Jagadeesh
2017-08-07 9:35 ` Pradeep Jagadeesh
1 sibling, 0 replies; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-07-07 14:50 UTC (permalink / raw)
To: Markus Armbruster, Pradeep Jagadeesh
Cc: eric blake, greg kurz, qemu-devel, jani kokkonen, alberto garcia,
Dr. David Alan Gilbert
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 */
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/6] throttle: factor out duplicate code
2017-07-04 15:30 ` [Qemu-devel] [PATCH v7 1/6] throttle: factor out duplicate code Pradeep Jagadeesh
@ 2017-07-10 14:33 ` Greg Kurz
2017-07-10 14:41 ` Eric Blake
1 sibling, 0 replies; 40+ messages in thread
From: Greg Kurz @ 2017-07-10 14:33 UTC (permalink / raw)
To: Pradeep Jagadeesh
Cc: eric blake, Pradeep Jagadeesh, alberto garcia, Markus Armbruster,
jani kokkonen, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 11910 bytes --]
On Tue, 4 Jul 2017 11:30:58 -0400
Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote:
> This patch factor out the duplicate throttle code that was present in
> block and fsdev devices.
>
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> blockdev.c | 44 +----------------------------------
> fsdev/qemu-fsdev-throttle.c | 44 ++---------------------------------
> include/qemu/throttle-options.h | 4 ++++
> include/qemu/throttle.h | 4 ++--
> include/qemu/typedefs.h | 1 +
> util/throttle.c | 51 +++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 61 insertions(+), 87 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index f92dcf2..a95ea41 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -387,49 +387,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);
> -
> + throttle_parse_options(throttle_cfg, opts);
> if (!throttle_is_valid(throttle_cfg, errp)) {
> return;
> }
> diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
> index 7ae4e86..c5e2499 100644
> --- a/fsdev/qemu-fsdev-throttle.c
> +++ b/fsdev/qemu-fsdev-throttle.c
> @@ -16,6 +16,7 @@
> #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)
> {
> @@ -31,48 +32,7 @@ static void fsdev_throttle_write_timer_cb(void *opaque)
>
> 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);
> -
> + throttle_parse_options(&fst->cfg, opts);
> throttle_is_valid(&fst->cfg, errp);
> }
>
> diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
> index 3133d1c..f63d38c 100644
> --- a/include/qemu/throttle-options.h
> +++ b/include/qemu/throttle-options.h
> @@ -10,6 +10,8 @@
> #ifndef THROTTLE_OPTIONS_H
> #define THROTTLE_OPTIONS_H
>
> +#include "typedefs.h"
> +
> #define THROTTLE_OPTS \
> { \
> .name = "throttling.iops-total",\
> @@ -89,4 +91,6 @@
> .help = "when limiting by iops max size of an I/O in bytes",\
> }
>
> +void throttle_parse_options(ThrottleConfig *, QemuOpts *);
> +
> #endif
> diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
> index 9109657..8eb5501 100644
> --- a/include/qemu/throttle.h
> +++ b/include/qemu/throttle.h
> @@ -89,10 +89,10 @@ typedef struct LeakyBucket {
> * However it allows to keep the code clean and the bucket field is reset to
> * zero at the right time.
> */
> -typedef struct ThrottleConfig {
> +struct ThrottleConfig {
> LeakyBucket buckets[BUCKETS_COUNT]; /* leaky buckets */
> uint64_t op_size; /* size of an operation in bytes */
> -} ThrottleConfig;
> +};
>
> typedef struct ThrottleState {
> ThrottleConfig cfg; /* configuration */
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 2706aab..041c0c4 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -97,6 +97,7 @@ typedef struct uWireSlave uWireSlave;
> typedef struct VirtIODevice VirtIODevice;
> typedef struct Visitor Visitor;
> typedef struct node_info NodeInfo;
> +typedef struct ThrottleConfig ThrottleConfig;
> typedef void SaveStateHandler(QEMUFile *f, void *opaque);
> typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
>
> diff --git a/util/throttle.c b/util/throttle.c
> index 3570ed2..a751126 100644
> --- a/util/throttle.c
> +++ b/util/throttle.c
> @@ -27,6 +27,7 @@
> #include "qemu/throttle.h"
> #include "qemu/timer.h"
> #include "block/aio.h"
> +#include "qemu/throttle-options.h"
>
> /* This function make a bucket leak
> *
> @@ -514,3 +515,53 @@ void throttle_account(ThrottleState *ts, bool is_write, uint64_t size)
> }
> }
>
> +/* parse the throttle options
> + *
> + * @opts: qemu options
> + * @throttle_cfg: throttle configuration
> + */
> +void throttle_parse_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);
> +}
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/6] throttle: factor out duplicate code
2017-07-04 15:30 ` [Qemu-devel] [PATCH v7 1/6] throttle: factor out duplicate code Pradeep Jagadeesh
2017-07-10 14:33 ` Greg Kurz
@ 2017-07-10 14:41 ` Eric Blake
2017-08-07 9:44 ` Pradeep Jagadeesh
1 sibling, 1 reply; 40+ messages in thread
From: Eric Blake @ 2017-07-10 14:41 UTC (permalink / raw)
To: Pradeep Jagadeesh, greg kurz
Cc: Pradeep Jagadeesh, alberto garcia, Markus Armbruster,
jani kokkonen, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 787 bytes --]
On 07/04/2017 10:30 AM, Pradeep Jagadeesh wrote:
> This patch factor out the duplicate throttle code that was present in
s/This patch factor/Factor/
It's okay to write commit messages in the imperative tense; the easiest
way I know to start a good message is to use an implied "Apply this
patch to ..." in front of the sentence. But "Apply this patch to This
patch ..." obviously doesn't flow, compared to "Apply this patch to
factor ..."
> block and fsdev devices.
>
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling
2017-07-04 15:30 [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Pradeep Jagadeesh
` (6 preceding siblings ...)
2017-07-07 6:14 ` [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Markus Armbruster
@ 2017-07-14 12:22 ` Manos Pitsidianakis
2017-07-14 13:15 ` Pradeep Jagadeesh
7 siblings, 1 reply; 40+ messages in thread
From: Manos Pitsidianakis @ 2017-07-14 12:22 UTC (permalink / raw)
To: Pradeep Jagadeesh
Cc: eric blake, greg kurz, alberto garcia, Markus Armbruster,
qemu-devel, Dr. David Alan Gilbert, Pradeep Jagadeesh,
jani kokkonen
[-- Attachment #1: Type: text/plain, Size: 594 bytes --]
Hello Pradeep, you might be interested in my work on refactoring the
block layer's throttling interface in my series:
https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg04191.html
In this series you copy the existing legacy interface we want to get rid
of. I think it will be easier work for you to use the changes introduced
in my patches when they are merged.
Have you thought about using throttle groups? It'd mean many devices
sharing the same limits. This way the interfaces can be unified. Please
read docs/throttle.txt and see if it would be useful for you.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling
2017-07-14 12:22 ` Manos Pitsidianakis
@ 2017-07-14 13:15 ` Pradeep Jagadeesh
2017-07-14 14:26 ` Manos Pitsidianakis
0 siblings, 1 reply; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-07-14 13:15 UTC (permalink / raw)
To: Manos Pitsidianakis, Pradeep Jagadeesh, eric blake, greg kurz,
alberto garcia, Markus Armbruster, qemu-devel,
Dr. David Alan Gilbert, jani kokkonen
Hi Manos,
Thanks for sharing the link to your code patch.
On 7/14/2017 2:22 PM, Manos Pitsidianakis wrote:
> Hello Pradeep, you might be interested in my work on refactoring the
> block layer's throttling interface in my series:
> https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg04191.html
Sure will have a look.
> In this series you copy the existing legacy interface we want to get rid
> of. I think it will be easier work for you to use the changes introduced
> in my patches when they are merged.
So, should I wait till they are in?. Actually my throttle patches for
fsdev are already upstream (2.9). Now I am just introducing the qmp/hmp
interfaces for the same.
> Have you thought about using throttle groups? It'd mean many devices
> sharing the same limits. This way the interfaces can be unified. Please
> read docs/throttle.txt and see if it would be useful for you.
I have read about the throttle group, last year when I was implementing
the throttle feature for fsdev.My open source work depends on the
projects I/my group work on. So, when I have more bandwidth work on
those, I will surely take it up. Throttle group option may be useful
feature even in case of fsdev devices.
Regards,
Pradeep
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling
2017-07-14 13:15 ` Pradeep Jagadeesh
@ 2017-07-14 14:26 ` Manos Pitsidianakis
2017-07-14 15:27 ` Pradeep Jagadeesh
0 siblings, 1 reply; 40+ messages in thread
From: Manos Pitsidianakis @ 2017-07-14 14:26 UTC (permalink / raw)
To: Pradeep Jagadeesh
Cc: Pradeep Jagadeesh, eric blake, greg kurz, alberto garcia,
Markus Armbruster, qemu-devel, Dr. David Alan Gilbert,
jani kokkonen
[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]
On Fri, Jul 14, 2017 at 03:15:06PM +0200, Pradeep Jagadeesh wrote:
>Hi Manos,
>
>Thanks for sharing the link to your code patch.
>
>On 7/14/2017 2:22 PM, Manos Pitsidianakis wrote:
>>Hello Pradeep, you might be interested in my work on refactoring the
>>block layer's throttling interface in my series:
>>https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg04191.html
>Sure will have a look.
>>In this series you copy the existing legacy interface we want to get rid
>>of. I think it will be easier work for you to use the changes introduced
>>in my patches when they are merged.
>So, should I wait till they are in?. Actually my throttle patches for
>fsdev are already upstream (2.9). Now I am just introducing the
>qmp/hmp interfaces for the same.
If you plan on using ThrottleGroups, probably yes, I think, because we'd
have duplicate interfaces afterwards instead of a unified one. You'd
have to introduce a qmp/hmp command to set fsdev throttle now and after
you use throttle groups it will become obsolete.
>>Have you thought about using throttle groups? It'd mean many devices
>>sharing the same limits. This way the interfaces can be unified. Please
>>read docs/throttle.txt and see if it would be useful for you.
>
>I have read about the throttle group, last year when I was
>implementing the throttle feature for fsdev.My open source work
>depends on the projects I/my group work on. So, when I have more
>bandwidth work on those, I will surely take it up. Throttle group
>option may be useful feature even in case of fsdev devices.
>
>Regards,
>Pradeep
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling
2017-07-14 14:26 ` Manos Pitsidianakis
@ 2017-07-14 15:27 ` Pradeep Jagadeesh
0 siblings, 0 replies; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-07-14 15:27 UTC (permalink / raw)
To: Manos Pitsidianakis, Pradeep Jagadeesh, eric blake, greg kurz,
alberto garcia, Markus Armbruster, qemu-devel,
Dr. David Alan Gilbert, jani kokkonen
On 7/14/2017 4:26 PM, Manos Pitsidianakis wrote:
> On Fri, Jul 14, 2017 at 03:15:06PM +0200, Pradeep Jagadeesh wrote:
>> Hi Manos,
>>
>> Thanks for sharing the link to your code patch.
>>
>> On 7/14/2017 2:22 PM, Manos Pitsidianakis wrote:
>>> Hello Pradeep, you might be interested in my work on refactoring the
>>> block layer's throttling interface in my series:
>>> https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg04191.html
>> Sure will have a look.
>>> In this series you copy the existing legacy interface we want to get rid
>>> of. I think it will be easier work for you to use the changes introduced
>>> in my patches when they are merged.
>> So, should I wait till they are in?. Actually my throttle patches for
>> fsdev are already upstream (2.9). Now I am just introducing the
>> qmp/hmp interfaces for the same.
>
> If you plan on using ThrottleGroups, probably yes, I think, because we'd
> have duplicate interfaces afterwards instead of a unified one. You'd
> have to introduce a qmp/hmp command to set fsdev throttle now and after
> you use throttle groups it will become obsolete.
I am not using ThrottleGroups in near future.
-Pradeep
>
>>> Have you thought about using throttle groups? It'd mean many devices
>>> sharing the same limits. This way the interfaces can be unified. Please
>>> read docs/throttle.txt and see if it would be useful for you.
>>
>> I have read about the throttle group, last year when I was
>> implementing the throttle feature for fsdev.My open source work
>> depends on the projects I/my group work on. So, when I have more
>> bandwidth work on those, I will surely take it up. Throttle group
>> option may be useful feature even in case of fsdev devices.
>>
>> Regards,
>> Pradeep
>>
>>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling
2017-07-07 6:14 ` [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Markus Armbruster
2017-07-07 6:24 ` Markus Armbruster
2017-07-07 13:16 ` Pradeep Jagadeesh
@ 2017-08-07 7:52 ` Pradeep Jagadeesh
2017-08-07 14:55 ` Markus Armbruster
2 siblings, 1 reply; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-08-07 7:52 UTC (permalink / raw)
To: Markus Armbruster, Pradeep Jagadeesh
Cc: eric blake, greg kurz, alberto garcia, qemu-devel,
Dr. David Alan Gilbert, jani kokkonen
On 7/7/2017 8:14 AM, Markus Armbruster wrote:
> Pradeep Jagadeesh <pradeepkiruvale@gmail.com> writes:
>
>> These patches provide the qmp interface, to query the io throttle
>> status of the all fsdev devices that are present in a vm.
>> also, it provides an interface to set the io throttle parameters of a
>> fsdev to a required value. some of the patches also remove the duplicate
>> code that was present in block and fsdev files.
>>
>> Pradeep Jagadeesh (6):
>> throttle: factor out duplicate code
>> qmp: Create IOThrottle structure
>> throttle: move out function to reuse the code
>> hmp: create a throttle initialization function for code reusability
>> fsdev: hmp interface for throttling
>> fsdev: QMP interface for throttling
>>
>> Makefile | 4 ++
>> blockdev.c | 97 ++-------------------------------
>> fsdev/qemu-fsdev-dummy.c | 10 ++++
>> fsdev/qemu-fsdev-throttle.c | 118 ++++++++++++++++++++++++++--------------
>> fsdev/qemu-fsdev-throttle.h | 13 +++++
>> fsdev/qemu-fsdev.c | 37 +++++++++++++
>> hmp-commands-info.hx | 18 ++++++
>> hmp-commands.hx | 19 +++++++
>> hmp.c | 81 +++++++++++++++++++++++++--
>> hmp.h | 4 ++
>> include/qemu/throttle-options.h | 7 +++
>> include/qemu/throttle.h | 4 +-
>> include/qemu/typedefs.h | 1 +
>> monitor.c | 5 ++
>> qapi-schema.json | 3 +
>> qapi/block-core.json | 76 +-------------------------
>> qapi/fsdev.json | 84 ++++++++++++++++++++++++++++
>> qapi/iothrottle.json | 88 ++++++++++++++++++++++++++++++
>> qmp.c | 14 +++++
>> util/throttle.c | 110 +++++++++++++++++++++++++++++++++++++
>> 20 files changed, 577 insertions(+), 216 deletions(-)
>> create mode 100644 qapi/fsdev.json
>> create mode 100644 qapi/iothrottle.json
>
> No test coverage?
I wanted to upstream these first then I am planning to write the tests.
-Pradeep
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 6/6] fsdev: QMP interface for throttling
2017-07-06 18:47 ` Markus Armbruster
2017-07-07 14:50 ` Pradeep Jagadeesh
@ 2017-08-07 9:35 ` Pradeep Jagadeesh
2017-08-07 14:54 ` Markus Armbruster
1 sibling, 1 reply; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-08-07 9:35 UTC (permalink / raw)
To: Markus Armbruster, Pradeep Jagadeesh
Cc: eric blake, greg kurz, qemu-devel, jani kokkonen, alberto garcia,
Dr. David Alan Gilbert
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 */
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 1/6] throttle: factor out duplicate code
2017-07-10 14:41 ` Eric Blake
@ 2017-08-07 9:44 ` Pradeep Jagadeesh
0 siblings, 0 replies; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-08-07 9:44 UTC (permalink / raw)
To: Eric Blake, Pradeep Jagadeesh, greg kurz
Cc: alberto garcia, Markus Armbruster, jani kokkonen, qemu-devel
On 7/10/2017 4:41 PM, Eric Blake wrote:
> On 07/04/2017 10:30 AM, Pradeep Jagadeesh wrote:
>> This patch factor out the duplicate throttle code that was present in
>
> s/This patch factor/Factor/
>
> It's okay to write commit messages in the imperative tense; the easiest
> way I know to start a good message is to use an implied "Apply this
> patch to ..." in front of the sentence. But "Apply this patch to This
> patch ..." obviously doesn't flow, compared to "Apply this patch to
> factor ..."
OK
>
>> block and fsdev devices.
>>
>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 5/6] fsdev: hmp interface for throttling
2017-07-06 18:15 ` Markus Armbruster
@ 2017-08-07 9:57 ` Pradeep Jagadeesh
0 siblings, 0 replies; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-08-07 9:57 UTC (permalink / raw)
To: Markus Armbruster, Pradeep Jagadeesh
Cc: eric blake, greg kurz, qemu-devel, jani kokkonen, alberto garcia,
Dr. David Alan Gilbert
On 7/6/2017 8:15 PM, Markus Armbruster wrote:
> Pradeep Jagadeesh <pradeepkiruvale@gmail.com> writes:
>
>> This patch introduces hmp interfaces for the fsdev
>> devices.
>>
>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>
> Fails to compile, first error:
>
> /work/armbru/qemu/hmp.c: In function ‘hmp_fsdev_set_io_throttle’:
> /work/armbru/qemu/hmp.c:1791:5: warning: implicit declaration of function ‘qmp_fsdev_set_io_throttle’ [-Wimplicit-function-declaration]
> qmp_fsdev_set_io_throttle(&throttle, &err);
> ^~~~~~~~~~~~~~~~~~~~~~~~~
>
> Do you need to swap PATCH 5 and 6?
Hmm, yes better to swap, because of dependency.
I will do in V8.
-Pradeep
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
2017-07-06 17:55 ` Markus Armbruster
@ 2017-08-07 9:59 ` Pradeep Jagadeesh
2017-08-07 14:48 ` Markus Armbruster
0 siblings, 1 reply; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-08-07 9:59 UTC (permalink / raw)
To: Markus Armbruster, Pradeep Jagadeesh
Cc: eric blake, greg kurz, jani kokkonen, alberto garcia, qemu-devel
On 7/6/2017 7:55 PM, Markus Armbruster wrote:
> Pradeep Jagadeesh <pradeepkiruvale@gmail.com> writes:
>
>> This patch enables qmp interfaces for the fsdev
>> devices. This provides two interfaces one
>> for querying info of all the fsdev devices. The second one
>> to set the IO limits for the required fsdev device.
>>
>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>> Reviewed-by: Greg Kurz <groug@kaod.org>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>> qapi/block-core.json | 76 ++-------------------------------------------
>> qapi/iothrottle.json | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 91 insertions(+), 73 deletions(-)
>> create mode 100644 qapi/iothrottle.json
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index f85c223..9320974 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -6,6 +6,7 @@
>>
>> # QAPI common definitions
>> { 'include': 'common.json' }
>> +{ 'include': 'iothrottle.json' }
>>
>> ##
>> # @SnapshotInfo:
>> @@ -1761,84 +1762,13 @@
>> #
>> # @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)
>> -#
>> # @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' } }
>> + 'base': 'IOThrottle',
>> + 'data': { '*device': 'str', '*group': 'str' } }
>>
>> ##
>> # @block-stream:
>> diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
>> new file mode 100644
>> index 0000000..0f067c3
>> --- /dev/null
>> +++ b/qapi/iothrottle.json
>> @@ -0,0 +1,88 @@
>> +# -*- Mode: Python -*-
>> +
>> +##
>> +# == QAPI IOThrottle definitions
>> +##
>> +
>> +##
>> +# @IOThrottle:
>> +#
>> +# A set of parameters describing IO throttling
>> +#
>> +# @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)
>> +#
>> +#
>> +# Since: 2.10
>> +##
>> +{ 'struct': 'IOThrottle',
>> + 'data': { '*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' } }
>
> Awkward question for a v7, but here goes anyway: why is IOThrottle worth
> its very own .json file?
I feel this is a common throttle structure that is used by block devices
as well as fsdev, so moved to a separate file.
-Pradeep
>
> Diff can't show the differences (if any), because you factor IOThrottle
> out of BlockIOThrottle and move it in a single patch. I had to extract
> and diff by hand.
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
2017-08-07 9:59 ` Pradeep Jagadeesh
@ 2017-08-07 14:48 ` Markus Armbruster
2017-08-08 11:30 ` Alberto Garcia
0 siblings, 1 reply; 40+ messages in thread
From: Markus Armbruster @ 2017-08-07 14:48 UTC (permalink / raw)
To: Pradeep Jagadeesh
Cc: Pradeep Jagadeesh, alberto garcia, jani kokkonen, greg kurz,
qemu-devel, Kevin Wolf
Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> writes:
> On 7/6/2017 7:55 PM, Markus Armbruster wrote:
>> Pradeep Jagadeesh <pradeepkiruvale@gmail.com> writes:
>>
>>> This patch enables qmp interfaces for the fsdev
>>> devices. This provides two interfaces one
>>> for querying info of all the fsdev devices. The second one
>>> to set the IO limits for the required fsdev device.
>>>
>>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>>> ---
>>> qapi/block-core.json | 76 ++-------------------------------------------
>>> qapi/iothrottle.json | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 91 insertions(+), 73 deletions(-)
>>> create mode 100644 qapi/iothrottle.json
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index f85c223..9320974 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -6,6 +6,7 @@
>>>
>>> # QAPI common definitions
>>> { 'include': 'common.json' }
>>> +{ 'include': 'iothrottle.json' }
>>>
>>> ##
>>> # @SnapshotInfo:
>>> @@ -1761,84 +1762,13 @@
>>> #
>>> # @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)
>>> -#
>>> # @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' } }
>>> + 'base': 'IOThrottle',
>>> + 'data': { '*device': 'str', '*group': 'str' } }
>>>
>>> ##
>>> # @block-stream:
>>> diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
>>> new file mode 100644
>>> index 0000000..0f067c3
>>> --- /dev/null
>>> +++ b/qapi/iothrottle.json
>>> @@ -0,0 +1,88 @@
>>> +# -*- Mode: Python -*-
>>> +
>>> +##
>>> +# == QAPI IOThrottle definitions
>>> +##
>>> +
>>> +##
>>> +# @IOThrottle:
>>> +#
>>> +# A set of parameters describing IO throttling
>>> +#
>>> +# @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)
>>> +#
>>> +#
>>> +# Since: 2.10
>>> +##
>>> +{ 'struct': 'IOThrottle',
>>> + 'data': { '*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' } }
>>
>> Awkward question for a v7, but here goes anyway: why is IOThrottle worth
>> its very own .json file?
> I feel this is a common throttle structure that is used by block
> devices as well as fsdev, so moved to a separate file.
I'm not sure that's a good idea. Kevin, Berto, what do you think?
>
> -Pradeep
>>
>> Diff can't show the differences (if any), because you factor IOThrottle
>> out of BlockIOThrottle and move it in a single patch. I had to extract
>> and diff by hand.
>>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 6/6] fsdev: QMP interface for throttling
2017-08-07 9:35 ` Pradeep Jagadeesh
@ 2017-08-07 14:54 ` Markus Armbruster
0 siblings, 0 replies; 40+ messages in thread
From: Markus Armbruster @ 2017-08-07 14:54 UTC (permalink / raw)
To: Pradeep Jagadeesh
Cc: Pradeep Jagadeesh, alberto garcia, qemu-devel, greg kurz,
Dr. David Alan Gilbert, jani kokkonen
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".
[...]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling
2017-08-07 7:52 ` Pradeep Jagadeesh
@ 2017-08-07 14:55 ` Markus Armbruster
0 siblings, 0 replies; 40+ messages in thread
From: Markus Armbruster @ 2017-08-07 14:55 UTC (permalink / raw)
To: Pradeep Jagadeesh
Cc: Pradeep Jagadeesh, alberto garcia, qemu-devel, greg kurz,
Dr. David Alan Gilbert, jani kokkonen
Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> writes:
> On 7/7/2017 8:14 AM, Markus Armbruster wrote:
>> Pradeep Jagadeesh <pradeepkiruvale@gmail.com> writes:
>>
>>> These patches provide the qmp interface, to query the io throttle
>>> status of the all fsdev devices that are present in a vm.
>>> also, it provides an interface to set the io throttle parameters of a
>>> fsdev to a required value. some of the patches also remove the duplicate
>>> code that was present in block and fsdev files.
>>>
>>> Pradeep Jagadeesh (6):
>>> throttle: factor out duplicate code
>>> qmp: Create IOThrottle structure
>>> throttle: move out function to reuse the code
>>> hmp: create a throttle initialization function for code reusability
>>> fsdev: hmp interface for throttling
>>> fsdev: QMP interface for throttling
>>>
>>> Makefile | 4 ++
>>> blockdev.c | 97 ++-------------------------------
>>> fsdev/qemu-fsdev-dummy.c | 10 ++++
>>> fsdev/qemu-fsdev-throttle.c | 118 ++++++++++++++++++++++++++--------------
>>> fsdev/qemu-fsdev-throttle.h | 13 +++++
>>> fsdev/qemu-fsdev.c | 37 +++++++++++++
>>> hmp-commands-info.hx | 18 ++++++
>>> hmp-commands.hx | 19 +++++++
>>> hmp.c | 81 +++++++++++++++++++++++++--
>>> hmp.h | 4 ++
>>> include/qemu/throttle-options.h | 7 +++
>>> include/qemu/throttle.h | 4 +-
>>> include/qemu/typedefs.h | 1 +
>>> monitor.c | 5 ++
>>> qapi-schema.json | 3 +
>>> qapi/block-core.json | 76 +-------------------------
>>> qapi/fsdev.json | 84 ++++++++++++++++++++++++++++
>>> qapi/iothrottle.json | 88 ++++++++++++++++++++++++++++++
>>> qmp.c | 14 +++++
>>> util/throttle.c | 110 +++++++++++++++++++++++++++++++++++++
>>> 20 files changed, 577 insertions(+), 216 deletions(-)
>>> create mode 100644 qapi/fsdev.json
>>> create mode 100644 qapi/iothrottle.json
>>
>> No test coverage?
> I wanted to upstream these first then I am planning to write the tests.
Feels backwards to me :)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
2017-08-07 14:48 ` Markus Armbruster
@ 2017-08-08 11:30 ` Alberto Garcia
2017-08-08 12:30 ` Pradeep Jagadeesh
0 siblings, 1 reply; 40+ messages in thread
From: Alberto Garcia @ 2017-08-08 11:30 UTC (permalink / raw)
To: Markus Armbruster, Pradeep Jagadeesh
Cc: Pradeep Jagadeesh, jani kokkonen, greg kurz, qemu-devel, Kevin Wolf
On Mon 07 Aug 2017 04:48:38 PM CEST, Markus Armbruster wrote:
>>> Awkward question for a v7, but here goes anyway: why is IOThrottle
>>> worth its very own .json file?
>> I feel this is a common throttle structure that is used by block
>> devices as well as fsdev, so moved to a separate file.
> I'm not sure that's a good idea. Kevin, Berto, what do you think?
Mmm... I don't have a very strong opinion, but if there's no actual need
to move it to a separate file I'd prefer to leave it where it is.
Berto
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
2017-08-08 11:30 ` Alberto Garcia
@ 2017-08-08 12:30 ` Pradeep Jagadeesh
2017-08-08 14:33 ` Alberto Garcia
0 siblings, 1 reply; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-08-08 12:30 UTC (permalink / raw)
To: Alberto Garcia, Markus Armbruster
Cc: Pradeep Jagadeesh, jani kokkonen, greg kurz, qemu-devel, Kevin Wolf
On 8/8/2017 1:30 PM, Alberto Garcia wrote:
> On Mon 07 Aug 2017 04:48:38 PM CEST, Markus Armbruster wrote:
>>>> Awkward question for a v7, but here goes anyway: why is IOThrottle
>>>> worth its very own .json file?
>>> I feel this is a common throttle structure that is used by block
>>> devices as well as fsdev, so moved to a separate file.
>> I'm not sure that's a good idea. Kevin, Berto, what do you think?
>
> Mmm... I don't have a very strong opinion, but if there's no actual need
> to move it to a separate file I'd prefer to leave it where it is.
The segregation is the solid reason. Because throttling is a feature
that is used by fsdev, block may many more in future. I do not see
moving it back to block does it make any sense?
Regards,
Pradeep
>
> Berto
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
2017-08-08 12:30 ` Pradeep Jagadeesh
@ 2017-08-08 14:33 ` Alberto Garcia
2017-08-08 15:18 ` Markus Armbruster
0 siblings, 1 reply; 40+ messages in thread
From: Alberto Garcia @ 2017-08-08 14:33 UTC (permalink / raw)
To: Pradeep Jagadeesh, Markus Armbruster
Cc: Pradeep Jagadeesh, jani kokkonen, greg kurz, qemu-devel, Kevin Wolf
On Tue 08 Aug 2017 02:30:43 PM CEST, Pradeep Jagadeesh wrote:
> On 8/8/2017 1:30 PM, Alberto Garcia wrote:
>> On Mon 07 Aug 2017 04:48:38 PM CEST, Markus Armbruster wrote:
>>>>> Awkward question for a v7, but here goes anyway: why is IOThrottle
>>>>> worth its very own .json file?
>>>> I feel this is a common throttle structure that is used by block
>>>> devices as well as fsdev, so moved to a separate file.
>>> I'm not sure that's a good idea. Kevin, Berto, what do you think?
>>
>> Mmm... I don't have a very strong opinion, but if there's no actual need
>> to move it to a separate file I'd prefer to leave it where it is.
> The segregation is the solid reason. Because throttling is a feature
> that is used by fsdev, block may many more in future. I do not see
> moving it back to block does it make any sense?
It's not "moving it back", it's keeping it where it is. But I see no big
problem with moving it to a common file either.
Berto
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
2017-08-08 14:33 ` Alberto Garcia
@ 2017-08-08 15:18 ` Markus Armbruster
2017-08-10 14:06 ` Pradeep Jagadeesh
0 siblings, 1 reply; 40+ messages in thread
From: Markus Armbruster @ 2017-08-08 15:18 UTC (permalink / raw)
To: Alberto Garcia
Cc: Pradeep Jagadeesh, Kevin Wolf, jani kokkonen, greg kurz,
Pradeep Jagadeesh, qemu-devel
Alberto Garcia <berto@igalia.com> writes:
> On Tue 08 Aug 2017 02:30:43 PM CEST, Pradeep Jagadeesh wrote:
>> On 8/8/2017 1:30 PM, Alberto Garcia wrote:
>>> On Mon 07 Aug 2017 04:48:38 PM CEST, Markus Armbruster wrote:
>>>>>> Awkward question for a v7, but here goes anyway: why is IOThrottle
>>>>>> worth its very own .json file?
>>>>> I feel this is a common throttle structure that is used by block
>>>>> devices as well as fsdev, so moved to a separate file.
>>>> I'm not sure that's a good idea. Kevin, Berto, what do you think?
>>>
>>> Mmm... I don't have a very strong opinion, but if there's no actual need
>>> to move it to a separate file I'd prefer to leave it where it is.
>> The segregation is the solid reason. Because throttling is a feature
>> that is used by fsdev, block may many more in future. I do not see
>> moving it back to block does it make any sense?
>
> It's not "moving it back", it's keeping it where it is. But I see no big
> problem with moving it to a common file either.
I'd rather not put every struct shared across subsystem boundaries in
its own file.
We can keep it right where it is for now. Bonus: more readable diff.
If we start sharing more throttle-related material than just a struct,
we can reconsider.
We could also move it to the existing file for common stuff:
qapi/common.json. Not a great fit, though.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
2017-08-08 15:18 ` Markus Armbruster
@ 2017-08-10 14:06 ` Pradeep Jagadeesh
2017-08-10 15:14 ` Eric Blake
0 siblings, 1 reply; 40+ messages in thread
From: Pradeep Jagadeesh @ 2017-08-10 14:06 UTC (permalink / raw)
To: Markus Armbruster, Alberto Garcia
Cc: Kevin Wolf, jani kokkonen, greg kurz, Pradeep Jagadeesh, qemu-devel
On 8/8/2017 5:18 PM, Markus Armbruster wrote:
> Alberto Garcia <berto@igalia.com> writes:
>
>> On Tue 08 Aug 2017 02:30:43 PM CEST, Pradeep Jagadeesh wrote:
>>> On 8/8/2017 1:30 PM, Alberto Garcia wrote:
>>>> On Mon 07 Aug 2017 04:48:38 PM CEST, Markus Armbruster wrote:
>>>>>>> Awkward question for a v7, but here goes anyway: why is IOThrottle
>>>>>>> worth its very own .json file?
>>>>>> I feel this is a common throttle structure that is used by block
>>>>>> devices as well as fsdev, so moved to a separate file.
>>>>> I'm not sure that's a good idea. Kevin, Berto, what do you think?
>>>>
>>>> Mmm... I don't have a very strong opinion, but if there's no actual need
>>>> to move it to a separate file I'd prefer to leave it where it is.
>>> The segregation is the solid reason. Because throttling is a feature
>>> that is used by fsdev, block may many more in future. I do not see
>>> moving it back to block does it make any sense?
>>
>> It's not "moving it back", it's keeping it where it is. But I see no big
>> problem with moving it to a common file either.
>
> I'd rather not put every struct shared across subsystem boundaries in
> its own file.
>
> We can keep it right where it is for now. Bonus: more readable diff.
> If we start sharing more throttle-related material than just a struct,
> we can reconsider.
>
> We could also move it to the existing file for common stuff:
> qapi/common.json. Not a great fit, though.
So, the final conclusion is to move to common.json?
Regards,
Pradeep
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
2017-08-10 14:06 ` Pradeep Jagadeesh
@ 2017-08-10 15:14 ` Eric Blake
2017-08-10 16:25 ` Markus Armbruster
0 siblings, 1 reply; 40+ messages in thread
From: Eric Blake @ 2017-08-10 15:14 UTC (permalink / raw)
To: Pradeep Jagadeesh, Markus Armbruster, Alberto Garcia
Cc: Kevin Wolf, jani kokkonen, greg kurz, Pradeep Jagadeesh, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]
On 08/10/2017 09:06 AM, Pradeep Jagadeesh wrote:
>>> It's not "moving it back", it's keeping it where it is. But I see no big
>>> problem with moving it to a common file either.
>>
>> I'd rather not put every struct shared across subsystem boundaries in
>> its own file.
>>
>> We can keep it right where it is for now. Bonus: more readable diff.
>> If we start sharing more throttle-related material than just a struct,
>> we can reconsider.
>>
>> We could also move it to the existing file for common stuff:
>> qapi/common.json. Not a great fit, though.
> So, the final conclusion is to move to common.json?
No.
If more than one .json file would benefit by including the definition,
then put it in a separate file that both .json include from.
But if only one .json file would be including a new file, then just
inline the struct directly into that one original file (in this case,
block-core.json) instead of creating a separate file (so no to needing
iothrottle.json), or putting the code in yet a different file than the
one that is using the struct (so no to putting it in common.json).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
2017-08-10 15:14 ` Eric Blake
@ 2017-08-10 16:25 ` Markus Armbruster
2017-08-16 16:13 ` Markus Armbruster
0 siblings, 1 reply; 40+ messages in thread
From: Markus Armbruster @ 2017-08-10 16:25 UTC (permalink / raw)
To: Eric Blake
Cc: Pradeep Jagadeesh, Alberto Garcia, Kevin Wolf, jani kokkonen,
greg kurz, Pradeep Jagadeesh, qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 08/10/2017 09:06 AM, Pradeep Jagadeesh wrote:
>
>>>> It's not "moving it back", it's keeping it where it is. But I see no big
>>>> problem with moving it to a common file either.
>>>
>>> I'd rather not put every struct shared across subsystem boundaries in
>>> its own file.
>>>
>>> We can keep it right where it is for now. Bonus: more readable diff.
>>> If we start sharing more throttle-related material than just a struct,
>>> we can reconsider.
>>>
>>> We could also move it to the existing file for common stuff:
>>> qapi/common.json. Not a great fit, though.
>>
>> So, the final conclusion is to move to common.json?
>
> No.
>
> If more than one .json file would benefit by including the definition,
> then put it in a separate file that both .json include from.
This is the case.
Your opinion is incompatible with mine, stated above.
> But if only one .json file would be including a new file, then just
> inline the struct directly into that one original file (in this case,
> block-core.json) instead of creating a separate file (so no to needing
> iothrottle.json), or putting the code in yet a different file than the
> one that is using the struct (so no to putting it in common.json).
This is no longer the case.
Conclusion: no consensus, yet.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
2017-08-10 16:25 ` Markus Armbruster
@ 2017-08-16 16:13 ` Markus Armbruster
2017-08-16 17:13 ` Eric Blake
0 siblings, 1 reply; 40+ messages in thread
From: Markus Armbruster @ 2017-08-16 16:13 UTC (permalink / raw)
To: Eric Blake
Cc: Pradeep Jagadeesh, Alberto Garcia, Kevin Wolf, jani kokkonen,
greg kurz, Pradeep Jagadeesh, qemu-devel
Markus Armbruster <armbru@redhat.com> writes:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 08/10/2017 09:06 AM, Pradeep Jagadeesh wrote:
>>
>>>>> It's not "moving it back", it's keeping it where it is. But I see no big
>>>>> problem with moving it to a common file either.
>>>>
>>>> I'd rather not put every struct shared across subsystem boundaries in
>>>> its own file.
>>>>
>>>> We can keep it right where it is for now. Bonus: more readable diff.
>>>> If we start sharing more throttle-related material than just a struct,
>>>> we can reconsider.
>>>>
>>>> We could also move it to the existing file for common stuff:
>>>> qapi/common.json. Not a great fit, though.
>>>
>>> So, the final conclusion is to move to common.json?
>>
>> No.
>>
>> If more than one .json file would benefit by including the definition,
>> then put it in a separate file that both .json include from.
>
> This is the case.
>
> Your opinion is incompatible with mine, stated above.
>
>> But if only one .json file would be including a new file, then just
>> inline the struct directly into that one original file (in this case,
>> block-core.json) instead of creating a separate file (so no to needing
>> iothrottle.json), or putting the code in yet a different file than the
>> one that is using the struct (so no to putting it in common.json).
>
> This is no longer the case.
>
> Conclusion: no consensus, yet.
All right, let's start over and try to resolve the impasse and/or
misunderstanding.
Type BlockIOThrottle lives in qapi/block-core.json, and is used by QMP
command block_set_io_throttle. Since 1.1.
Pradeep has a use case for throttling in fsdev. Instead of duplicating
the relevant parts of BlockIOThrottle, qmp_block_set_io_throttle() and
hmp_block_set_io_throttle(), he factors them out smartly, into
* [PATCH 2] IOThrottle, base type of BlockIOThrottle
* [PATCH 3] throttle_set_io_limits(), called by
qmp_block_set_io_throttle()
* [PATCH 4] hmp_initialize_io_throttle(), called by
hmp_block_set_io_throttle()
throttle_set_io_limits() goes into existing util/throttle.c, and
hmp_initialize_io_throttle() goes into existing hmp.c. The question is
where IOThrottle should go.
Pradeep proposes to put it in new qapi/throttle.json. Certainly
defensible, but I really don't like putting every little thing shared
across subsystem boundaries into its own schema file.
Let me step back and discuss why we split the QAPI schema into multiple
files in the first place. For me, the one and only reason is
MAINTAINERS.
If the block folks should continue to maintain IOThrottle, then it
should stay put in block-core.json.
If somebody else should start maintaining it, it should move. We'd need
a suitable entry in MAINTAINERS then.
I don't see why maintenance should change, and therefore believe it
should stay put.
Eric?
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
2017-08-16 16:13 ` Markus Armbruster
@ 2017-08-16 17:13 ` Eric Blake
2017-08-16 18:11 ` Markus Armbruster
0 siblings, 1 reply; 40+ messages in thread
From: Eric Blake @ 2017-08-16 17:13 UTC (permalink / raw)
To: Markus Armbruster
Cc: Pradeep Jagadeesh, Alberto Garcia, Kevin Wolf, jani kokkonen,
greg kurz, Pradeep Jagadeesh, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2480 bytes --]
On 08/16/2017 11:13 AM, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
>
>>
>> Conclusion: no consensus, yet.
>
> All right, let's start over and try to resolve the impasse and/or
> misunderstanding.
>
> Type BlockIOThrottle lives in qapi/block-core.json, and is used by QMP
> command block_set_io_throttle. Since 1.1.
>
> Pradeep has a use case for throttling in fsdev. Instead of duplicating
> the relevant parts of BlockIOThrottle, qmp_block_set_io_throttle() and
> hmp_block_set_io_throttle(), he factors them out smartly, into
>
> * [PATCH 2] IOThrottle, base type of BlockIOThrottle
>
> * [PATCH 3] throttle_set_io_limits(), called by
> qmp_block_set_io_throttle()
>
> * [PATCH 4] hmp_initialize_io_throttle(), called by
> hmp_block_set_io_throttle()
>
> throttle_set_io_limits() goes into existing util/throttle.c, and
> hmp_initialize_io_throttle() goes into existing hmp.c. The question is
> where IOThrottle should go.
Good summary.
>
> Pradeep proposes to put it in new qapi/throttle.json. Certainly
> defensible, but I really don't like putting every little thing shared
> across subsystem boundaries into its own schema file.
I agree with the dislike of creating new files, if an existing file is
adequate.
>
> Let me step back and discuss why we split the QAPI schema into multiple
> files in the first place. For me, the one and only reason is
> MAINTAINERS.
Indeed, that's a good description of why splits would be appropriate.
So the obvious next question is if this is a case that needs a new
maintainer.
>
> If the block folks should continue to maintain IOThrottle, then it
> should stay put in block-core.json.
I think Manos' work on making throttling a filter driver at the block
layer is proof enough that it it is still fine to keep throttling
maintained in block-core.json.
>
> If somebody else should start maintaining it, it should move. We'd need
> a suitable entry in MAINTAINERS then.
>
> I don't see why maintenance should change, and therefore believe it
> should stay put.
>
> Eric?
I think we're in violent agreement: don't create a new file, and having
the new factored type live in block-core.json is the best fit because we
haven't come up with any reasons why it needs to be split.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
2017-08-16 17:13 ` Eric Blake
@ 2017-08-16 18:11 ` Markus Armbruster
[not found] ` <CAJ2SuLnH9dvfWs1XX-Pd+GE9s0kZP_3LkMs_Crt9QTkz48NsyA@mail.gmail.com>
0 siblings, 1 reply; 40+ messages in thread
From: Markus Armbruster @ 2017-08-16 18:11 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Alberto Garcia, greg kurz, Pradeep Jagadeesh,
qemu-devel, Pradeep Jagadeesh, jani kokkonen
Eric Blake <eblake@redhat.com> writes:
> On 08/16/2017 11:13 AM, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>
>>>
>>> Conclusion: no consensus, yet.
>>
>> All right, let's start over and try to resolve the impasse and/or
>> misunderstanding.
>>
>> Type BlockIOThrottle lives in qapi/block-core.json, and is used by QMP
>> command block_set_io_throttle. Since 1.1.
>>
>> Pradeep has a use case for throttling in fsdev. Instead of duplicating
>> the relevant parts of BlockIOThrottle, qmp_block_set_io_throttle() and
>> hmp_block_set_io_throttle(), he factors them out smartly, into
>>
>> * [PATCH 2] IOThrottle, base type of BlockIOThrottle
>>
>> * [PATCH 3] throttle_set_io_limits(), called by
>> qmp_block_set_io_throttle()
>>
>> * [PATCH 4] hmp_initialize_io_throttle(), called by
>> hmp_block_set_io_throttle()
>>
>> throttle_set_io_limits() goes into existing util/throttle.c, and
>> hmp_initialize_io_throttle() goes into existing hmp.c. The question is
>> where IOThrottle should go.
>
> Good summary.
>
>>
>> Pradeep proposes to put it in new qapi/throttle.json. Certainly
>> defensible, but I really don't like putting every little thing shared
>> across subsystem boundaries into its own schema file.
>
> I agree with the dislike of creating new files, if an existing file is
> adequate.
>
>>
>> Let me step back and discuss why we split the QAPI schema into multiple
>> files in the first place. For me, the one and only reason is
>> MAINTAINERS.
>
> Indeed, that's a good description of why splits would be appropriate.
> So the obvious next question is if this is a case that needs a new
> maintainer.
>
>>
>> If the block folks should continue to maintain IOThrottle, then it
>> should stay put in block-core.json.
>
> I think Manos' work on making throttling a filter driver at the block
> layer is proof enough that it it is still fine to keep throttling
> maintained in block-core.json.
>
>>
>> If somebody else should start maintaining it, it should move. We'd need
>> a suitable entry in MAINTAINERS then.
>>
>> I don't see why maintenance should change, and therefore believe it
>> should stay put.
>>
>> Eric?
>
> I think we're in violent agreement: don't create a new file, and having
> the new factored type live in block-core.json is the best fit because we
> haven't come up with any reasons why it needs to be split.
Thanks, Eric.
Pradeep, please put IOThrottle right before BlocIOThrottle in
block-core.json. Use it in fsdev.json without including
block-core.json. Sorry for the delay.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
[not found] ` <CAJ2SuL=F3oAA6KbURSQRaqNS_DPSZ7tGOjgEcgNkfzdXxw7tTQ@mail.gmail.com>
@ 2017-08-19 1:16 ` Pradeep Kiruvale
0 siblings, 0 replies; 40+ messages in thread
From: Pradeep Kiruvale @ 2017-08-19 1:16 UTC (permalink / raw)
To: Markus Armbruster
Cc: Alberto Garcia, Kevin Wolf, jani kokkonen, qemu-devel, greg kurz,
Eric Blake, Pradeep Jagadeesh
On Aug 16, 2017 11:41 PM, "Markus Armbruster" <armbru@redhat.com> wrote:
Eric Blake <eblake@redhat.com> writes:
> On 08/16/2017 11:13 AM, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>
>>>
>>> Conclusion: no consensus, yet.
>>
>> All right, let's start over and try to resolve the impasse and/or
>> misunderstanding.
>>
>> Type BlockIOThrottle lives in qapi/block-core.json, and is used by QMP
>> command block_set_io_throttle. Since 1.1.
>>
>> Pradeep has a use case for throttling in fsdev. Instead of duplicating
>> the relevant parts of BlockIOThrottle, qmp_block_set_io_throttle() and
>> hmp_block_set_io_throttle(), he factors them out smartly, into
>>
>> * [PATCH 2] IOThrottle, base type of BlockIOThrottle
>>
>> * [PATCH 3] throttle_set_io_limits(), called by
>> qmp_block_set_io_throttle()
>>
>> * [PATCH 4] hmp_initialize_io_throttle(), called by
>> hmp_block_set_io_throttle()
>>
>> throttle_set_io_limits() goes into existing util/throttle.c, and
>> hmp_initialize_io_throttle() goes into existing hmp.c. The question is
>> where IOThrottle should go.
>
> Good summary.
>
>>
>> Pradeep proposes to put it in new qapi/throttle.json. Certainly
>> defensible, but I really don't like putting every little thing shared
>> across subsystem boundaries into its own schema file.
>
> I agree with the dislike of creating new files, if an existing file is
> adequate.
>
>>
>> Let me step back and discuss why we split the QAPI schema into multiple
>> files in the first place. For me, the one and only reason is
>> MAINTAINERS.
>
> Indeed, that's a good description of why splits would be appropriate.
> So the obvious next question is if this is a case that needs a new
> maintainer.
>
>>
>> If the block folks should continue to maintain IOThrottle, then it
>> should stay put in block-core.json.
>
> I think Manos' work on making throttling a filter driver at the block
> layer is proof enough that it it is still fine to keep throttling
> maintained in block-core.json.
>
>>
>> If somebody else should start maintaining it, it should move. We'd need
>> a suitable entry in MAINTAINERS then.
>>
>> I don't see why maintenance should change, and therefore believe it
>> should stay put.
>>
>> Eric?
>
> I think we're in violent agreement: don't create a new file, and having
> the new factored type live in block-core.json is the best fit because we
> haven't come up with any reasons why it needs to be split.
Thanks, Eric.
Pradeep, please put IOThrottle right before BlocIOThrottle in
block-core.json. Use it in fsdev.json without including
block-core.json. Sorry for the delay.
Thanks for all the clarifications, sure I will move iothrottle code to
block-core.json.
Thanks,
Pradeep
^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2017-08-19 1:16 UTC | newest]
Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-04 15:30 [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Pradeep Jagadeesh
2017-07-04 15:30 ` [Qemu-devel] [PATCH v7 1/6] throttle: factor out duplicate code Pradeep Jagadeesh
2017-07-10 14:33 ` Greg Kurz
2017-07-10 14:41 ` Eric Blake
2017-08-07 9:44 ` Pradeep Jagadeesh
2017-07-04 15:30 ` [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure Pradeep Jagadeesh
2017-07-06 17:55 ` Markus Armbruster
2017-08-07 9:59 ` Pradeep Jagadeesh
2017-08-07 14:48 ` Markus Armbruster
2017-08-08 11:30 ` Alberto Garcia
2017-08-08 12:30 ` Pradeep Jagadeesh
2017-08-08 14:33 ` Alberto Garcia
2017-08-08 15:18 ` Markus Armbruster
2017-08-10 14:06 ` Pradeep Jagadeesh
2017-08-10 15:14 ` Eric Blake
2017-08-10 16:25 ` Markus Armbruster
2017-08-16 16:13 ` Markus Armbruster
2017-08-16 17:13 ` Eric Blake
2017-08-16 18:11 ` Markus Armbruster
[not found] ` <CAJ2SuLnH9dvfWs1XX-Pd+GE9s0kZP_3LkMs_Crt9QTkz48NsyA@mail.gmail.com>
[not found] ` <CAJ2SuL=F3oAA6KbURSQRaqNS_DPSZ7tGOjgEcgNkfzdXxw7tTQ@mail.gmail.com>
2017-08-19 1:16 ` Pradeep Kiruvale
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 3/6] throttle: move out function to reuse the code Pradeep Jagadeesh
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 4/6] hmp: create a throttle initialization function for code reusability Pradeep Jagadeesh
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 5/6] fsdev: hmp interface for throttling Pradeep Jagadeesh
2017-07-05 10:45 ` Dr. David Alan Gilbert
2017-07-06 18:15 ` Markus Armbruster
2017-08-07 9:57 ` Pradeep Jagadeesh
2017-07-04 15:31 ` [Qemu-devel] [PATCH v7 6/6] fsdev: QMP " Pradeep Jagadeesh
2017-07-06 18:47 ` Markus Armbruster
2017-07-07 14:50 ` Pradeep Jagadeesh
2017-08-07 9:35 ` Pradeep Jagadeesh
2017-08-07 14:54 ` Markus Armbruster
2017-07-07 6:14 ` [Qemu-devel] [PATCH v7 0/6] fsdev: qmp interface for io throttling Markus Armbruster
2017-07-07 6:24 ` Markus Armbruster
2017-07-07 13:16 ` Pradeep Jagadeesh
2017-08-07 7:52 ` Pradeep Jagadeesh
2017-08-07 14:55 ` Markus Armbruster
2017-07-14 12:22 ` Manos Pitsidianakis
2017-07-14 13:15 ` Pradeep Jagadeesh
2017-07-14 14:26 ` Manos Pitsidianakis
2017-07-14 15: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.