* [PATCH v1] qmp/hmp: add block_set_io_throttle and enhance query_block
@ 2011-08-04 4:34 ` Zhi Yong Wu
0 siblings, 0 replies; 6+ messages in thread
From: Zhi Yong Wu @ 2011-08-04 4:34 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, aliguori, stefanha, kvm, mtosatti, Zhi Yong Wu,
zwu.kernel, ryanh, luowenj
The brief intro:
This patch is created based on the code V4 for block I/O throttling. It mainly adds a new qmp/hmp command block_set_io_throttle, and enhances query_block(qmp) and info block(hmp) to display current I/O throttling settings.
Welcome to all kinds of comments from you.
Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
block.c | 155 ++++++++++++++++++++++++++++++++++++++----------------
block.h | 5 ++
block_int.h | 2 +
blockdev.c | 86 ++++++++++++++++++++++++++++++-
blockdev.h | 2 +
hmp-commands.hx | 15 +++++
qerror.c | 8 +++
qerror.h | 6 ++
qmp-commands.hx | 52 ++++++++++++++++++-
9 files changed, 283 insertions(+), 48 deletions(-)
diff --git a/block.c b/block.c
index 42763a3..8bdf31a 100644
--- a/block.c
+++ b/block.c
@@ -100,18 +100,83 @@ int is_windows_drive(const char *filename)
}
#endif
-static int bdrv_io_limits_enable(BlockIOLimit *io_limits)
+/* throttling disk I/O limits */
+void bdrv_io_limits_res_deinit(BlockDriverState *bs)
{
+ bs->io_limits_enabled = false;
+ bs->req_from_queue = false;
+ bs->io_unlimit = false;
+
+ if (bs->block_queue) {
+ qemu_del_block_queue(bs->block_queue);
+ }
+
+ if (bs->block_timer) {
+ qemu_del_timer(bs->block_timer);
+ qemu_free_timer(bs->block_timer);
+ }
+
+ bs->slice_start[0] = 0;
+ bs->slice_start[1] = 0;
+
+ bs->slice_end[0] = 0;
+ bs->slice_end[1] = 0;
+}
+
+static void bdrv_block_timer(void *opaque)
+{
+ BlockDriverState *bs = opaque;
+ BlockQueue *queue = bs->block_queue;
+
+ while (!QTAILQ_EMPTY(&queue->requests)) {
+ BlockIORequest *request = NULL;
+ int ret = 0;
+
+ request = QTAILQ_FIRST(&queue->requests);
+ QTAILQ_REMOVE(&queue->requests, request, entry);
+
+ ret = qemu_block_queue_handler(request);
+ if (ret == 0) {
+ QTAILQ_INSERT_HEAD(&queue->requests, request, entry);
+ break;
+ }
+
+ qemu_free(request);
+ }
+
+ if (bs->io_unlimit) {
+ bdrv_io_limits_res_deinit(bs);
+ }
+}
+
+void bdrv_io_limits_res_init(BlockDriverState *bs)
+{
+ bs->req_from_queue = false;
+ bs->io_unlimit = false;
+
+ bs->block_queue = qemu_new_block_queue();
+ bs->block_timer = qemu_new_timer_ns(vm_clock, bdrv_block_timer, bs);
+
+ bs->slice_start[0] = qemu_get_clock_ns(vm_clock);
+ bs->slice_start[1] = qemu_get_clock_ns(vm_clock);
+
+ bs->slice_end[0] = qemu_get_clock_ns(vm_clock) + BLOCK_IO_SLICE_TIME;
+ bs->slice_end[1] = qemu_get_clock_ns(vm_clock) + BLOCK_IO_SLICE_TIME;
+}
+
+bool bdrv_io_limits_enabled(BlockDriverState *bs)
+{
+ BlockIOLimit *io_limits = &bs->io_limits;
if ((io_limits->bps[0] == 0)
&& (io_limits->bps[1] == 0)
&& (io_limits->bps[2] == 0)
&& (io_limits->iops[0] == 0)
&& (io_limits->iops[1] == 0)
&& (io_limits->iops[2] == 0)) {
- return 0;
+ return false;
}
- return 1;
+ return true;
}
/* check if the path starts with "<protocol>:" */
@@ -191,28 +256,6 @@ void path_combine(char *dest, int dest_size,
}
}
-static void bdrv_block_timer(void *opaque)
-{
- BlockDriverState *bs = opaque;
- BlockQueue *queue = bs->block_queue;
-
- while (!QTAILQ_EMPTY(&queue->requests)) {
- BlockIORequest *request = NULL;
- int ret = 0;
-
- request = QTAILQ_FIRST(&queue->requests);
- QTAILQ_REMOVE(&queue->requests, request, entry);
-
- ret = qemu_block_queue_handler(request);
- if (ret == 0) {
- QTAILQ_INSERT_HEAD(&queue->requests, request, entry);
- break;
- }
-
- qemu_free(request);
- }
-}
-
void bdrv_register(BlockDriver *bdrv)
{
if (!bdrv->bdrv_aio_readv) {
@@ -689,16 +732,8 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
}
/* throttling disk I/O limits */
- if (bdrv_io_limits_enable(&bs->io_limits)) {
- bs->req_from_queue = false;
- bs->block_queue = qemu_new_block_queue();
- bs->block_timer = qemu_new_timer_ns(vm_clock, bdrv_block_timer, bs);
-
- bs->slice_start[0] = qemu_get_clock_ns(vm_clock);
- bs->slice_start[1] = qemu_get_clock_ns(vm_clock);
-
- bs->slice_end[0] = qemu_get_clock_ns(vm_clock) + BLOCK_IO_SLICE_TIME;
- bs->slice_end[1] = qemu_get_clock_ns(vm_clock) + BLOCK_IO_SLICE_TIME;
+ if (bs->io_limits_enabled) {
+ bdrv_io_limits_res_init(bs);
}
return 0;
@@ -1387,6 +1422,11 @@ void bdrv_set_io_limits(BlockDriverState *bs,
{
memset(&bs->io_limits, 0, sizeof(BlockIOLimit));
bs->io_limits = *io_limits;
+ if (bdrv_io_limits_enabled(bs)) {
+ bs->io_limits_enabled = true;
+ } else {
+ bs->io_limits_enabled = false;
+ }
}
/* Recognize floppy formats */
@@ -1621,6 +1661,7 @@ BlockDriverState *bdrv_find(const char *name)
return NULL;
}
+/* disk I/O throttling */
BlockDriverState *bdrv_next(BlockDriverState *bs)
{
if (!bs) {
@@ -1784,6 +1825,16 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
qdict_get_bool(qdict, "ro"),
qdict_get_str(qdict, "drv"),
qdict_get_bool(qdict, "encrypted"));
+
+ monitor_printf(mon, " bps=%" PRId64 " bps_rd=%" PRId64
+ " bps_wr=%" PRId64 " iops=%" PRId64
+ " iops_rd=%" PRId64 " iops_wr=%" PRId64,
+ qdict_get_int(qdict, "bps"),
+ qdict_get_int(qdict, "bps_rd"),
+ qdict_get_int(qdict, "bps_wr"),
+ qdict_get_int(qdict, "iops"),
+ qdict_get_int(qdict, "iops_rd"),
+ qdict_get_int(qdict, "iops_wr"));
} else {
monitor_printf(mon, " [not inserted]");
}
@@ -1816,10 +1867,22 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
QDict *bs_dict = qobject_to_qdict(bs_obj);
obj = qobject_from_jsonf("{ 'file': %s, 'ro': %i, 'drv': %s, "
- "'encrypted': %i }",
+ "'encrypted': %i, "
+ "'bps': %" PRId64 ","
+ "'bps_rd': %" PRId64 ","
+ "'bps_wr': %" PRId64 ","
+ "'iops': %" PRId64 ","
+ "'iops_rd': %" PRId64 ","
+ "'iops_wr': %" PRId64 "}",
bs->filename, bs->read_only,
bs->drv->format_name,
- bdrv_is_encrypted(bs));
+ bdrv_is_encrypted(bs),
+ bs->io_limits.bps[2],
+ bs->io_limits.bps[0],
+ bs->io_limits.bps[1],
+ bs->io_limits.iops[2],
+ bs->io_limits.iops[0],
+ bs->io_limits.iops[1]);
if (bs->backing_file[0] != '\0') {
QDict *qdict = qobject_to_qdict(obj);
qdict_put(qdict, "backing_file",
@@ -2307,7 +2370,7 @@ static bool bdrv_exceed_io_limits(BlockDriverState *bs, int nb_sectors,
}
/* If a limit was exceeded, immediately queue this request */
- if ((bs->req_from_queue == false)
+ if (!bs->req_from_queue
&& !QTAILQ_EMPTY(&bs->block_queue->requests)) {
if (bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL]
|| bs->io_limits.bps[is_write] || bs->io_limits.iops[is_write]
@@ -2362,14 +2425,14 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
trace_bdrv_aio_readv(bs, sector_num, nb_sectors, opaque);
if (!drv || bdrv_check_request(bs, sector_num, nb_sectors)) {
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
bs->req_from_queue = false;
}
return NULL;
}
/* throttling disk read I/O */
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
if (bdrv_exceed_io_limits(bs, nb_sectors, false, &wait_time)) {
ret = qemu_block_queue_enqueue(bs->block_queue, bs, bdrv_aio_readv,
sector_num, qiov, nb_sectors, cb, opaque);
@@ -2388,14 +2451,14 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
bs->rd_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
bs->rd_ops ++;
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
bs->io_disps.bytes[BLOCK_IO_LIMIT_READ] +=
(unsigned) nb_sectors * BDRV_SECTOR_SIZE;
bs->io_disps.ios[BLOCK_IO_LIMIT_READ]++;
}
}
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
bs->req_from_queue = false;
}
@@ -2451,7 +2514,7 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
if (!drv || bs->read_only
|| bdrv_check_request(bs, sector_num, nb_sectors)) {
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
bs->req_from_queue = false;
}
@@ -2466,7 +2529,7 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
}
/* throttling disk write I/O */
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
if (bdrv_exceed_io_limits(bs, nb_sectors, true, &wait_time)) {
ret = qemu_block_queue_enqueue(bs->block_queue, bs, bdrv_aio_writev,
sector_num, qiov, nb_sectors, cb, opaque);
@@ -2488,14 +2551,14 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
bs->wr_highest_sector = sector_num + nb_sectors - 1;
}
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
bs->io_disps.bytes[BLOCK_IO_LIMIT_WRITE] +=
(unsigned) nb_sectors * BDRV_SECTOR_SIZE;
bs->io_disps.ios[BLOCK_IO_LIMIT_WRITE]++;
}
}
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
bs->req_from_queue = false;
}
diff --git a/block.h b/block.h
index f0dac62..97d2177 100644
--- a/block.h
+++ b/block.h
@@ -57,6 +57,11 @@ void bdrv_info(Monitor *mon, QObject **ret_data);
void bdrv_stats_print(Monitor *mon, const QObject *data);
void bdrv_info_stats(Monitor *mon, QObject **ret_data);
+/* disk I/O throttling */
+void bdrv_io_limits_res_init(BlockDriverState *bs);
+void bdrv_io_limits_res_deinit(BlockDriverState *bs);
+bool bdrv_io_limits_enabled(BlockDriverState *bs);
+
void bdrv_init(void);
void bdrv_init_with_whitelist(void);
BlockDriver *bdrv_find_protocol(const char *filename);
diff --git a/block_int.h b/block_int.h
index 1ca826b..7c96094 100644
--- a/block_int.h
+++ b/block_int.h
@@ -199,6 +199,8 @@ struct BlockDriverState {
BlockIODisp io_disps;
BlockQueue *block_queue;
QEMUTimer *block_timer;
+ bool io_limits_enabled;
+ bool io_unlimit;
bool req_from_queue;
/* I/O stats (display with "info blockstats"). */
diff --git a/blockdev.c b/blockdev.c
index aff6bb2..dc2e8a9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -376,7 +376,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
return NULL;
}
- /* disk io limits */
+ /* disk io throttling */
iol_flag = qemu_opt_io_limits_enable_flag(opts, iol_opts);
if (iol_flag) {
memset(&io_limits, 0, sizeof(BlockIOLimit));
@@ -387,6 +387,14 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
io_limits.iops[2] = qemu_opt_get_number(opts, "iops", 0);
io_limits.iops[0] = qemu_opt_get_number(opts, "iops_rd", 0);
io_limits.iops[1] = qemu_opt_get_number(opts, "iops_wr", 0);
+
+ if (((io_limits.bps[2] != 0)
+ && ((io_limits.bps[0] != 0) || (io_limits.bps[1] != 0)))
+ || ((io_limits.iops[2] != 0)
+ && ((io_limits.iops[0] != 0) || (io_limits.iops[1] != 0)))) {
+ error_report("Some params for io throttling can not coexist");
+ return NULL;
+ }
}
on_write_error = BLOCK_ERR_STOP_ENOSPC;
@@ -765,6 +773,82 @@ int do_change_block(Monitor *mon, const char *device,
return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
}
+/* throttling disk I/O limits */
+int do_block_set_io_throttle(Monitor *mon,
+ const QDict *qdict, QObject **ret_data)
+{
+ const char *devname = qdict_get_str(qdict, "device");
+ uint64_t bps = qdict_get_try_int(qdict, "bps", -1);
+ uint64_t bps_rd = qdict_get_try_int(qdict, "bps_rd", -1);
+ uint64_t bps_wr = qdict_get_try_int(qdict, "bps_wr", -1);
+ uint64_t iops = qdict_get_try_int(qdict, "iops", -1);
+ uint64_t iops_rd = qdict_get_try_int(qdict, "iops_rd", -1);
+ uint64_t iops_wr = qdict_get_try_int(qdict, "iops_wr", -1);
+ BlockDriverState *bs;
+
+ bs = bdrv_find(devname);
+ if (!bs) {
+ qerror_report(QERR_DEVICE_NOT_FOUND, devname);
+ return -1;
+ }
+
+ if (devname && (bps == -1) && (bps_rd == -1) && (bps_wr == -1)
+ && (iops == -1) && (iops_rd == -1) && (iops_wr == -1)) {
+ qerror_report(QERR_IO_THROTTLE_NO_PARAM);
+ return -1;
+ }
+
+ if (((bps != -1) && ((bps_rd != -1) || (bps_wr != -1)))
+ || ((iops != -1) && ((iops_rd != -1) || (iops_wr != -1)))) {
+ qerror_report(QERR_IO_THROTTLE_PARAM_ERROR);
+ return -1;
+ }
+
+ if (bps != -1) {
+ bs->io_limits.bps[2] = bps;
+ bs->io_limits.bps[0] = 0;
+ bs->io_limits.bps[1] = 0;
+ }
+
+ if ((bps_rd != -1) || (bps_wr != -1)) {
+ bs->io_limits.bps[0] = (bps_rd == -1) ? bs->io_limits.bps[0] : bps_rd;
+ bs->io_limits.bps[1] = (bps_wr == -1) ? bs->io_limits.bps[1] : bps_wr;
+ bs->io_limits.bps[2] = 0;
+ }
+
+ if (iops != -1) {
+ bs->io_limits.iops[2] = iops;
+ bs->io_limits.iops[0] = 0;
+ bs->io_limits.iops[1] = 0;
+ }
+
+ if ((iops_rd != -1) || (iops_wr != -1)) {
+ bs->io_limits.iops[0] =
+ (iops_rd == -1) ? bs->io_limits.iops[0] : iops_rd;
+ bs->io_limits.iops[1] =
+ (iops_wr == -1) ? bs->io_limits.iops[1] : iops_wr;
+ bs->io_limits.iops[2] = 0;
+ }
+
+ if (bdrv_io_limits_enabled(bs)) {
+ if (!bs->io_limits_enabled) {
+ bdrv_io_limits_res_init(bs);
+ }
+ } else {
+ if (bs->io_limits_enabled) {
+ BlockQueue *queue = bs->block_queue;
+ if (!QTAILQ_EMPTY(&queue->requests)) {
+ bs->io_unlimit = true;
+ bs->io_limits_enabled = false;
+ } else {
+ bdrv_io_limits_res_deinit(bs);
+ }
+ }
+ }
+
+ return 0;
+}
+
int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
{
const char *id = qdict_get_str(qdict, "id");
diff --git a/blockdev.h b/blockdev.h
index 0a5144c..d0d0d77 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -63,6 +63,8 @@ int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
int do_change_block(Monitor *mon, const char *device,
const char *filename, const char *fmt);
int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_block_set_io_throttle(Monitor *mon,
+ const QDict *qdict, QObject **ret_data);
int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index aceba74..3ca496d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1210,6 +1210,21 @@ ETEXI
},
STEXI
+@item block_set_io_throttle @var{device} @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
+@findex block_set_io_throttle
+Change I/O throttle limits for a block drive to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
+ETEXI
+
+ {
+ .name = "block_set_io_throttle",
+ .args_type = "device:B,bps:i?,bps_rd:i?,bps_wr:i?,iops:i?,iops_rd:i?,iops_wr:i?",
+ .params = "device [bps] [bps_rd] [bps_wr] [iops] [iops_rd] [iops_wr]",
+ .help = "change I/O throttle limts for a block drive",
+ .user_print = monitor_user_noop,
+ .mhandler.cmd_new = do_block_set_io_throttle,
+ },
+
+STEXI
@item block_passwd @var{device} @var{password}
@findex block_passwd
Set the encrypted device @var{device} password to @var{password}
diff --git a/qerror.c b/qerror.c
index d7fcd93..db04df7 100644
--- a/qerror.c
+++ b/qerror.c
@@ -213,6 +213,14 @@ static const QErrorStringTable qerror_table[] = {
.error_fmt = QERR_VNC_SERVER_FAILED,
.desc = "Could not start VNC server on %(target)",
},
+ {
+ .error_fmt = QERR_IO_THROTTLE_PARAM_ERROR,
+ .desc = "Some params for io throttling can not coexist",
+ },
+ {
+ .error_fmt = QERR_IO_THROTTLE_NO_PARAM,
+ .desc = "No params for io throttling are specified",
+ },
{}
};
diff --git a/qerror.h b/qerror.h
index 16c830d..892abbb 100644
--- a/qerror.h
+++ b/qerror.h
@@ -181,4 +181,10 @@ QError *qobject_to_qerror(const QObject *obj);
#define QERR_FEATURE_DISABLED \
"{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
+#define QERR_IO_THROTTLE_PARAM_ERROR \
+ "{ 'class': 'ParamsCoexistNotPermitted', 'data': {} }"
+
+#define QERR_IO_THROTTLE_NO_PARAM \
+ "{ 'class': 'NoParamsSpecified', 'data': {} }"
+
#endif /* QERROR_H */
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 92c5c3a..0aaeae1 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -828,6 +828,44 @@ Example:
EQMP
{
+ .name = "block_set_io_throttle",
+ .args_type = "device:B,bps:i?,bps_rd:i?,bps_wr:i?,iops:i?,iops_rd:i?,iops_wr:i?",
+ .params = "device [bps[ [bps_rd] [bps_wr] [iops] [iops_rd] [iops_wr]",
+ .help = "change I/O throttle limts for a block drive",
+ .user_print = monitor_user_noop,
+ .mhandler.cmd_new = do_block_set_io_throttle,
+ },
+
+SQMP
+block_set_io_throttle
+------------
+
+Change I/O throttle limts for a block drive.
+
+Arguments:
+
+- "device": device name (json-string)
+- "bps": total throughput value per second(json-int, optional)
+- "bps_rd": read throughput value per second(json-int, optional)
+- "bps_wr": read throughput value per second(json-int, optional)
+- "iops": total I/O operations per second(json-int, optional)
+- "iops_rd": read I/O operations per second(json-int, optional)
+- "iops_wr": write I/O operations per second(json-int, optional)
+
+Example:
+
+-> { "execute": "block_set_io_throttle", "arguments": { "device": "virtio0",
+ "bps": "1000000",
+ "bps_rd": "0",
+ "bps_wr": "0",
+ "iops": "0",
+ "iops_rd": "0",
+ "iops_wr": "0" } }
+<- { "return": {} }
+
+EQMP
+
+ {
.name = "set_password",
.args_type = "protocol:s,password:s,connected:s?",
.params = "protocol password action-if-connected",
@@ -1082,6 +1120,12 @@ Each json-object contain the following:
"tftp", "vdi", "vmdk", "vpc", "vvfat"
- "backing_file": backing file name (json-string, optional)
- "encrypted": true if encrypted, false otherwise (json-bool)
+ - "bps": limit total bytes per second (json-int)
+ - "bps_rd": limit read bytes per second (json-int)
+ - "bps_wr": limit write bytes per second (json-int)
+ - "iops": limit total I/O operations per second (json-int)
+ - "iops_rd": limit read operations per second (json-int)
+ - "iops_wr": limit write operations per second (json-int)
Example:
@@ -1096,7 +1140,13 @@ Example:
"ro":false,
"drv":"qcow2",
"encrypted":false,
- "file":"disks/test.img"
+ "file":"disks/test.img",
+ "bps":1000000,
+ "bps_rd":0,
+ "bps_wr":0,
+ "iops":1000000,
+ "iops_rd":0,
+ "iops_wr":0,
},
"type":"unknown"
},
--
1.7.2.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v1] qmp/hmp: add block_set_io_throttle and enhance query_block
@ 2011-08-04 4:34 ` Zhi Yong Wu
0 siblings, 0 replies; 6+ messages in thread
From: Zhi Yong Wu @ 2011-08-04 4:34 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, aliguori, stefanha, kvm, mtosatti, Zhi Yong Wu,
zwu.kernel, ryanh, luowenj
The brief intro:
This patch is created based on the code V4 for block I/O throttling. It mainly adds a new qmp/hmp command block_set_io_throttle, and enhances query_block(qmp) and info block(hmp) to display current I/O throttling settings.
Welcome to all kinds of comments from you.
Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
block.c | 155 ++++++++++++++++++++++++++++++++++++++----------------
block.h | 5 ++
block_int.h | 2 +
blockdev.c | 86 ++++++++++++++++++++++++++++++-
blockdev.h | 2 +
hmp-commands.hx | 15 +++++
qerror.c | 8 +++
qerror.h | 6 ++
qmp-commands.hx | 52 ++++++++++++++++++-
9 files changed, 283 insertions(+), 48 deletions(-)
diff --git a/block.c b/block.c
index 42763a3..8bdf31a 100644
--- a/block.c
+++ b/block.c
@@ -100,18 +100,83 @@ int is_windows_drive(const char *filename)
}
#endif
-static int bdrv_io_limits_enable(BlockIOLimit *io_limits)
+/* throttling disk I/O limits */
+void bdrv_io_limits_res_deinit(BlockDriverState *bs)
{
+ bs->io_limits_enabled = false;
+ bs->req_from_queue = false;
+ bs->io_unlimit = false;
+
+ if (bs->block_queue) {
+ qemu_del_block_queue(bs->block_queue);
+ }
+
+ if (bs->block_timer) {
+ qemu_del_timer(bs->block_timer);
+ qemu_free_timer(bs->block_timer);
+ }
+
+ bs->slice_start[0] = 0;
+ bs->slice_start[1] = 0;
+
+ bs->slice_end[0] = 0;
+ bs->slice_end[1] = 0;
+}
+
+static void bdrv_block_timer(void *opaque)
+{
+ BlockDriverState *bs = opaque;
+ BlockQueue *queue = bs->block_queue;
+
+ while (!QTAILQ_EMPTY(&queue->requests)) {
+ BlockIORequest *request = NULL;
+ int ret = 0;
+
+ request = QTAILQ_FIRST(&queue->requests);
+ QTAILQ_REMOVE(&queue->requests, request, entry);
+
+ ret = qemu_block_queue_handler(request);
+ if (ret == 0) {
+ QTAILQ_INSERT_HEAD(&queue->requests, request, entry);
+ break;
+ }
+
+ qemu_free(request);
+ }
+
+ if (bs->io_unlimit) {
+ bdrv_io_limits_res_deinit(bs);
+ }
+}
+
+void bdrv_io_limits_res_init(BlockDriverState *bs)
+{
+ bs->req_from_queue = false;
+ bs->io_unlimit = false;
+
+ bs->block_queue = qemu_new_block_queue();
+ bs->block_timer = qemu_new_timer_ns(vm_clock, bdrv_block_timer, bs);
+
+ bs->slice_start[0] = qemu_get_clock_ns(vm_clock);
+ bs->slice_start[1] = qemu_get_clock_ns(vm_clock);
+
+ bs->slice_end[0] = qemu_get_clock_ns(vm_clock) + BLOCK_IO_SLICE_TIME;
+ bs->slice_end[1] = qemu_get_clock_ns(vm_clock) + BLOCK_IO_SLICE_TIME;
+}
+
+bool bdrv_io_limits_enabled(BlockDriverState *bs)
+{
+ BlockIOLimit *io_limits = &bs->io_limits;
if ((io_limits->bps[0] == 0)
&& (io_limits->bps[1] == 0)
&& (io_limits->bps[2] == 0)
&& (io_limits->iops[0] == 0)
&& (io_limits->iops[1] == 0)
&& (io_limits->iops[2] == 0)) {
- return 0;
+ return false;
}
- return 1;
+ return true;
}
/* check if the path starts with "<protocol>:" */
@@ -191,28 +256,6 @@ void path_combine(char *dest, int dest_size,
}
}
-static void bdrv_block_timer(void *opaque)
-{
- BlockDriverState *bs = opaque;
- BlockQueue *queue = bs->block_queue;
-
- while (!QTAILQ_EMPTY(&queue->requests)) {
- BlockIORequest *request = NULL;
- int ret = 0;
-
- request = QTAILQ_FIRST(&queue->requests);
- QTAILQ_REMOVE(&queue->requests, request, entry);
-
- ret = qemu_block_queue_handler(request);
- if (ret == 0) {
- QTAILQ_INSERT_HEAD(&queue->requests, request, entry);
- break;
- }
-
- qemu_free(request);
- }
-}
-
void bdrv_register(BlockDriver *bdrv)
{
if (!bdrv->bdrv_aio_readv) {
@@ -689,16 +732,8 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
}
/* throttling disk I/O limits */
- if (bdrv_io_limits_enable(&bs->io_limits)) {
- bs->req_from_queue = false;
- bs->block_queue = qemu_new_block_queue();
- bs->block_timer = qemu_new_timer_ns(vm_clock, bdrv_block_timer, bs);
-
- bs->slice_start[0] = qemu_get_clock_ns(vm_clock);
- bs->slice_start[1] = qemu_get_clock_ns(vm_clock);
-
- bs->slice_end[0] = qemu_get_clock_ns(vm_clock) + BLOCK_IO_SLICE_TIME;
- bs->slice_end[1] = qemu_get_clock_ns(vm_clock) + BLOCK_IO_SLICE_TIME;
+ if (bs->io_limits_enabled) {
+ bdrv_io_limits_res_init(bs);
}
return 0;
@@ -1387,6 +1422,11 @@ void bdrv_set_io_limits(BlockDriverState *bs,
{
memset(&bs->io_limits, 0, sizeof(BlockIOLimit));
bs->io_limits = *io_limits;
+ if (bdrv_io_limits_enabled(bs)) {
+ bs->io_limits_enabled = true;
+ } else {
+ bs->io_limits_enabled = false;
+ }
}
/* Recognize floppy formats */
@@ -1621,6 +1661,7 @@ BlockDriverState *bdrv_find(const char *name)
return NULL;
}
+/* disk I/O throttling */
BlockDriverState *bdrv_next(BlockDriverState *bs)
{
if (!bs) {
@@ -1784,6 +1825,16 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
qdict_get_bool(qdict, "ro"),
qdict_get_str(qdict, "drv"),
qdict_get_bool(qdict, "encrypted"));
+
+ monitor_printf(mon, " bps=%" PRId64 " bps_rd=%" PRId64
+ " bps_wr=%" PRId64 " iops=%" PRId64
+ " iops_rd=%" PRId64 " iops_wr=%" PRId64,
+ qdict_get_int(qdict, "bps"),
+ qdict_get_int(qdict, "bps_rd"),
+ qdict_get_int(qdict, "bps_wr"),
+ qdict_get_int(qdict, "iops"),
+ qdict_get_int(qdict, "iops_rd"),
+ qdict_get_int(qdict, "iops_wr"));
} else {
monitor_printf(mon, " [not inserted]");
}
@@ -1816,10 +1867,22 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
QDict *bs_dict = qobject_to_qdict(bs_obj);
obj = qobject_from_jsonf("{ 'file': %s, 'ro': %i, 'drv': %s, "
- "'encrypted': %i }",
+ "'encrypted': %i, "
+ "'bps': %" PRId64 ","
+ "'bps_rd': %" PRId64 ","
+ "'bps_wr': %" PRId64 ","
+ "'iops': %" PRId64 ","
+ "'iops_rd': %" PRId64 ","
+ "'iops_wr': %" PRId64 "}",
bs->filename, bs->read_only,
bs->drv->format_name,
- bdrv_is_encrypted(bs));
+ bdrv_is_encrypted(bs),
+ bs->io_limits.bps[2],
+ bs->io_limits.bps[0],
+ bs->io_limits.bps[1],
+ bs->io_limits.iops[2],
+ bs->io_limits.iops[0],
+ bs->io_limits.iops[1]);
if (bs->backing_file[0] != '\0') {
QDict *qdict = qobject_to_qdict(obj);
qdict_put(qdict, "backing_file",
@@ -2307,7 +2370,7 @@ static bool bdrv_exceed_io_limits(BlockDriverState *bs, int nb_sectors,
}
/* If a limit was exceeded, immediately queue this request */
- if ((bs->req_from_queue == false)
+ if (!bs->req_from_queue
&& !QTAILQ_EMPTY(&bs->block_queue->requests)) {
if (bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL]
|| bs->io_limits.bps[is_write] || bs->io_limits.iops[is_write]
@@ -2362,14 +2425,14 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
trace_bdrv_aio_readv(bs, sector_num, nb_sectors, opaque);
if (!drv || bdrv_check_request(bs, sector_num, nb_sectors)) {
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
bs->req_from_queue = false;
}
return NULL;
}
/* throttling disk read I/O */
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
if (bdrv_exceed_io_limits(bs, nb_sectors, false, &wait_time)) {
ret = qemu_block_queue_enqueue(bs->block_queue, bs, bdrv_aio_readv,
sector_num, qiov, nb_sectors, cb, opaque);
@@ -2388,14 +2451,14 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
bs->rd_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
bs->rd_ops ++;
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
bs->io_disps.bytes[BLOCK_IO_LIMIT_READ] +=
(unsigned) nb_sectors * BDRV_SECTOR_SIZE;
bs->io_disps.ios[BLOCK_IO_LIMIT_READ]++;
}
}
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
bs->req_from_queue = false;
}
@@ -2451,7 +2514,7 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
if (!drv || bs->read_only
|| bdrv_check_request(bs, sector_num, nb_sectors)) {
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
bs->req_from_queue = false;
}
@@ -2466,7 +2529,7 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
}
/* throttling disk write I/O */
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
if (bdrv_exceed_io_limits(bs, nb_sectors, true, &wait_time)) {
ret = qemu_block_queue_enqueue(bs->block_queue, bs, bdrv_aio_writev,
sector_num, qiov, nb_sectors, cb, opaque);
@@ -2488,14 +2551,14 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
bs->wr_highest_sector = sector_num + nb_sectors - 1;
}
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
bs->io_disps.bytes[BLOCK_IO_LIMIT_WRITE] +=
(unsigned) nb_sectors * BDRV_SECTOR_SIZE;
bs->io_disps.ios[BLOCK_IO_LIMIT_WRITE]++;
}
}
- if (bdrv_io_limits_enable(&bs->io_limits)) {
+ if (bs->io_limits_enabled) {
bs->req_from_queue = false;
}
diff --git a/block.h b/block.h
index f0dac62..97d2177 100644
--- a/block.h
+++ b/block.h
@@ -57,6 +57,11 @@ void bdrv_info(Monitor *mon, QObject **ret_data);
void bdrv_stats_print(Monitor *mon, const QObject *data);
void bdrv_info_stats(Monitor *mon, QObject **ret_data);
+/* disk I/O throttling */
+void bdrv_io_limits_res_init(BlockDriverState *bs);
+void bdrv_io_limits_res_deinit(BlockDriverState *bs);
+bool bdrv_io_limits_enabled(BlockDriverState *bs);
+
void bdrv_init(void);
void bdrv_init_with_whitelist(void);
BlockDriver *bdrv_find_protocol(const char *filename);
diff --git a/block_int.h b/block_int.h
index 1ca826b..7c96094 100644
--- a/block_int.h
+++ b/block_int.h
@@ -199,6 +199,8 @@ struct BlockDriverState {
BlockIODisp io_disps;
BlockQueue *block_queue;
QEMUTimer *block_timer;
+ bool io_limits_enabled;
+ bool io_unlimit;
bool req_from_queue;
/* I/O stats (display with "info blockstats"). */
diff --git a/blockdev.c b/blockdev.c
index aff6bb2..dc2e8a9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -376,7 +376,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
return NULL;
}
- /* disk io limits */
+ /* disk io throttling */
iol_flag = qemu_opt_io_limits_enable_flag(opts, iol_opts);
if (iol_flag) {
memset(&io_limits, 0, sizeof(BlockIOLimit));
@@ -387,6 +387,14 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
io_limits.iops[2] = qemu_opt_get_number(opts, "iops", 0);
io_limits.iops[0] = qemu_opt_get_number(opts, "iops_rd", 0);
io_limits.iops[1] = qemu_opt_get_number(opts, "iops_wr", 0);
+
+ if (((io_limits.bps[2] != 0)
+ && ((io_limits.bps[0] != 0) || (io_limits.bps[1] != 0)))
+ || ((io_limits.iops[2] != 0)
+ && ((io_limits.iops[0] != 0) || (io_limits.iops[1] != 0)))) {
+ error_report("Some params for io throttling can not coexist");
+ return NULL;
+ }
}
on_write_error = BLOCK_ERR_STOP_ENOSPC;
@@ -765,6 +773,82 @@ int do_change_block(Monitor *mon, const char *device,
return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
}
+/* throttling disk I/O limits */
+int do_block_set_io_throttle(Monitor *mon,
+ const QDict *qdict, QObject **ret_data)
+{
+ const char *devname = qdict_get_str(qdict, "device");
+ uint64_t bps = qdict_get_try_int(qdict, "bps", -1);
+ uint64_t bps_rd = qdict_get_try_int(qdict, "bps_rd", -1);
+ uint64_t bps_wr = qdict_get_try_int(qdict, "bps_wr", -1);
+ uint64_t iops = qdict_get_try_int(qdict, "iops", -1);
+ uint64_t iops_rd = qdict_get_try_int(qdict, "iops_rd", -1);
+ uint64_t iops_wr = qdict_get_try_int(qdict, "iops_wr", -1);
+ BlockDriverState *bs;
+
+ bs = bdrv_find(devname);
+ if (!bs) {
+ qerror_report(QERR_DEVICE_NOT_FOUND, devname);
+ return -1;
+ }
+
+ if (devname && (bps == -1) && (bps_rd == -1) && (bps_wr == -1)
+ && (iops == -1) && (iops_rd == -1) && (iops_wr == -1)) {
+ qerror_report(QERR_IO_THROTTLE_NO_PARAM);
+ return -1;
+ }
+
+ if (((bps != -1) && ((bps_rd != -1) || (bps_wr != -1)))
+ || ((iops != -1) && ((iops_rd != -1) || (iops_wr != -1)))) {
+ qerror_report(QERR_IO_THROTTLE_PARAM_ERROR);
+ return -1;
+ }
+
+ if (bps != -1) {
+ bs->io_limits.bps[2] = bps;
+ bs->io_limits.bps[0] = 0;
+ bs->io_limits.bps[1] = 0;
+ }
+
+ if ((bps_rd != -1) || (bps_wr != -1)) {
+ bs->io_limits.bps[0] = (bps_rd == -1) ? bs->io_limits.bps[0] : bps_rd;
+ bs->io_limits.bps[1] = (bps_wr == -1) ? bs->io_limits.bps[1] : bps_wr;
+ bs->io_limits.bps[2] = 0;
+ }
+
+ if (iops != -1) {
+ bs->io_limits.iops[2] = iops;
+ bs->io_limits.iops[0] = 0;
+ bs->io_limits.iops[1] = 0;
+ }
+
+ if ((iops_rd != -1) || (iops_wr != -1)) {
+ bs->io_limits.iops[0] =
+ (iops_rd == -1) ? bs->io_limits.iops[0] : iops_rd;
+ bs->io_limits.iops[1] =
+ (iops_wr == -1) ? bs->io_limits.iops[1] : iops_wr;
+ bs->io_limits.iops[2] = 0;
+ }
+
+ if (bdrv_io_limits_enabled(bs)) {
+ if (!bs->io_limits_enabled) {
+ bdrv_io_limits_res_init(bs);
+ }
+ } else {
+ if (bs->io_limits_enabled) {
+ BlockQueue *queue = bs->block_queue;
+ if (!QTAILQ_EMPTY(&queue->requests)) {
+ bs->io_unlimit = true;
+ bs->io_limits_enabled = false;
+ } else {
+ bdrv_io_limits_res_deinit(bs);
+ }
+ }
+ }
+
+ return 0;
+}
+
int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
{
const char *id = qdict_get_str(qdict, "id");
diff --git a/blockdev.h b/blockdev.h
index 0a5144c..d0d0d77 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -63,6 +63,8 @@ int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
int do_change_block(Monitor *mon, const char *device,
const char *filename, const char *fmt);
int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_block_set_io_throttle(Monitor *mon,
+ const QDict *qdict, QObject **ret_data);
int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index aceba74..3ca496d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1210,6 +1210,21 @@ ETEXI
},
STEXI
+@item block_set_io_throttle @var{device} @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
+@findex block_set_io_throttle
+Change I/O throttle limits for a block drive to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
+ETEXI
+
+ {
+ .name = "block_set_io_throttle",
+ .args_type = "device:B,bps:i?,bps_rd:i?,bps_wr:i?,iops:i?,iops_rd:i?,iops_wr:i?",
+ .params = "device [bps] [bps_rd] [bps_wr] [iops] [iops_rd] [iops_wr]",
+ .help = "change I/O throttle limts for a block drive",
+ .user_print = monitor_user_noop,
+ .mhandler.cmd_new = do_block_set_io_throttle,
+ },
+
+STEXI
@item block_passwd @var{device} @var{password}
@findex block_passwd
Set the encrypted device @var{device} password to @var{password}
diff --git a/qerror.c b/qerror.c
index d7fcd93..db04df7 100644
--- a/qerror.c
+++ b/qerror.c
@@ -213,6 +213,14 @@ static const QErrorStringTable qerror_table[] = {
.error_fmt = QERR_VNC_SERVER_FAILED,
.desc = "Could not start VNC server on %(target)",
},
+ {
+ .error_fmt = QERR_IO_THROTTLE_PARAM_ERROR,
+ .desc = "Some params for io throttling can not coexist",
+ },
+ {
+ .error_fmt = QERR_IO_THROTTLE_NO_PARAM,
+ .desc = "No params for io throttling are specified",
+ },
{}
};
diff --git a/qerror.h b/qerror.h
index 16c830d..892abbb 100644
--- a/qerror.h
+++ b/qerror.h
@@ -181,4 +181,10 @@ QError *qobject_to_qerror(const QObject *obj);
#define QERR_FEATURE_DISABLED \
"{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
+#define QERR_IO_THROTTLE_PARAM_ERROR \
+ "{ 'class': 'ParamsCoexistNotPermitted', 'data': {} }"
+
+#define QERR_IO_THROTTLE_NO_PARAM \
+ "{ 'class': 'NoParamsSpecified', 'data': {} }"
+
#endif /* QERROR_H */
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 92c5c3a..0aaeae1 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -828,6 +828,44 @@ Example:
EQMP
{
+ .name = "block_set_io_throttle",
+ .args_type = "device:B,bps:i?,bps_rd:i?,bps_wr:i?,iops:i?,iops_rd:i?,iops_wr:i?",
+ .params = "device [bps[ [bps_rd] [bps_wr] [iops] [iops_rd] [iops_wr]",
+ .help = "change I/O throttle limts for a block drive",
+ .user_print = monitor_user_noop,
+ .mhandler.cmd_new = do_block_set_io_throttle,
+ },
+
+SQMP
+block_set_io_throttle
+------------
+
+Change I/O throttle limts for a block drive.
+
+Arguments:
+
+- "device": device name (json-string)
+- "bps": total throughput value per second(json-int, optional)
+- "bps_rd": read throughput value per second(json-int, optional)
+- "bps_wr": read throughput value per second(json-int, optional)
+- "iops": total I/O operations per second(json-int, optional)
+- "iops_rd": read I/O operations per second(json-int, optional)
+- "iops_wr": write I/O operations per second(json-int, optional)
+
+Example:
+
+-> { "execute": "block_set_io_throttle", "arguments": { "device": "virtio0",
+ "bps": "1000000",
+ "bps_rd": "0",
+ "bps_wr": "0",
+ "iops": "0",
+ "iops_rd": "0",
+ "iops_wr": "0" } }
+<- { "return": {} }
+
+EQMP
+
+ {
.name = "set_password",
.args_type = "protocol:s,password:s,connected:s?",
.params = "protocol password action-if-connected",
@@ -1082,6 +1120,12 @@ Each json-object contain the following:
"tftp", "vdi", "vmdk", "vpc", "vvfat"
- "backing_file": backing file name (json-string, optional)
- "encrypted": true if encrypted, false otherwise (json-bool)
+ - "bps": limit total bytes per second (json-int)
+ - "bps_rd": limit read bytes per second (json-int)
+ - "bps_wr": limit write bytes per second (json-int)
+ - "iops": limit total I/O operations per second (json-int)
+ - "iops_rd": limit read operations per second (json-int)
+ - "iops_wr": limit write operations per second (json-int)
Example:
@@ -1096,7 +1140,13 @@ Example:
"ro":false,
"drv":"qcow2",
"encrypted":false,
- "file":"disks/test.img"
+ "file":"disks/test.img",
+ "bps":1000000,
+ "bps_rd":0,
+ "bps_wr":0,
+ "iops":1000000,
+ "iops_rd":0,
+ "iops_wr":0,
},
"type":"unknown"
},
--
1.7.2.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v1] qmp/hmp: add block_set_io_throttle and enhance query_block
2011-08-04 4:34 ` [Qemu-devel] " Zhi Yong Wu
@ 2011-08-04 13:07 ` Stefan Hajnoczi
-1 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2011-08-04 13:07 UTC (permalink / raw)
To: Zhi Yong Wu
Cc: qemu-devel, kwolf, aliguori, stefanha, kvm, mtosatti, zwu.kernel,
ryanh, luowenj
On Thu, Aug 4, 2011 at 5:34 AM, Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> wrote:
> @@ -1387,6 +1422,11 @@ void bdrv_set_io_limits(BlockDriverState *bs,
> {
> memset(&bs->io_limits, 0, sizeof(BlockIOLimit));
> bs->io_limits = *io_limits;
> + if (bdrv_io_limits_enabled(bs)) {
> + bs->io_limits_enabled = true;
> + } else {
> + bs->io_limits_enabled = false;
> + }
Or in one line:
bs->io_limits_enabled = bdrv_io_limits_enabled(bs);
> }
>
> /* Recognize floppy formats */
> @@ -1621,6 +1661,7 @@ BlockDriverState *bdrv_find(const char *name)
> return NULL;
> }
>
> +/* disk I/O throttling */
Looks like this should not be here.
> BlockDriverState *bdrv_next(BlockDriverState *bs)
> {
> if (!bs) {
> @@ -1784,6 +1825,16 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
> qdict_get_bool(qdict, "ro"),
> qdict_get_str(qdict, "drv"),
> qdict_get_bool(qdict, "encrypted"));
> +
> + monitor_printf(mon, " bps=%" PRId64 " bps_rd=%" PRId64
> + " bps_wr=%" PRId64 " iops=%" PRId64
> + " iops_rd=%" PRId64 " iops_wr=%" PRId64,
> + qdict_get_int(qdict, "bps"),
> + qdict_get_int(qdict, "bps_rd"),
> + qdict_get_int(qdict, "bps_wr"),
> + qdict_get_int(qdict, "iops"),
> + qdict_get_int(qdict, "iops_rd"),
> + qdict_get_int(qdict, "iops_wr"));
> } else {
> monitor_printf(mon, " [not inserted]");
> }
> @@ -1816,10 +1867,22 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
> QDict *bs_dict = qobject_to_qdict(bs_obj);
>
> obj = qobject_from_jsonf("{ 'file': %s, 'ro': %i, 'drv': %s, "
> - "'encrypted': %i }",
> + "'encrypted': %i, "
> + "'bps': %" PRId64 ","
> + "'bps_rd': %" PRId64 ","
> + "'bps_wr': %" PRId64 ","
> + "'iops': %" PRId64 ","
> + "'iops_rd': %" PRId64 ","
> + "'iops_wr': %" PRId64 "}",
> bs->filename, bs->read_only,
> bs->drv->format_name,
> - bdrv_is_encrypted(bs));
> + bdrv_is_encrypted(bs),
> + bs->io_limits.bps[2],
> + bs->io_limits.bps[0],
> + bs->io_limits.bps[1],
> + bs->io_limits.iops[2],
> + bs->io_limits.iops[0],
> + bs->io_limits.iops[1]);
> if (bs->backing_file[0] != '\0') {
> QDict *qdict = qobject_to_qdict(obj);
> qdict_put(qdict, "backing_file",
> @@ -2307,7 +2370,7 @@ static bool bdrv_exceed_io_limits(BlockDriverState *bs, int nb_sectors,
> }
>
> /* If a limit was exceeded, immediately queue this request */
> - if ((bs->req_from_queue == false)
> + if (!bs->req_from_queue
> && !QTAILQ_EMPTY(&bs->block_queue->requests)) {
> if (bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL]
> || bs->io_limits.bps[is_write] || bs->io_limits.iops[is_write]
> @@ -2362,14 +2425,14 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
> trace_bdrv_aio_readv(bs, sector_num, nb_sectors, opaque);
>
> if (!drv || bdrv_check_request(bs, sector_num, nb_sectors)) {
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> bs->req_from_queue = false;
> }
> return NULL;
> }
>
> /* throttling disk read I/O */
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> if (bdrv_exceed_io_limits(bs, nb_sectors, false, &wait_time)) {
> ret = qemu_block_queue_enqueue(bs->block_queue, bs, bdrv_aio_readv,
> sector_num, qiov, nb_sectors, cb, opaque);
> @@ -2388,14 +2451,14 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
> bs->rd_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
> bs->rd_ops ++;
>
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> bs->io_disps.bytes[BLOCK_IO_LIMIT_READ] +=
> (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
> bs->io_disps.ios[BLOCK_IO_LIMIT_READ]++;
> }
> }
>
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> bs->req_from_queue = false;
> }
>
> @@ -2451,7 +2514,7 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>
> if (!drv || bs->read_only
> || bdrv_check_request(bs, sector_num, nb_sectors)) {
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> bs->req_from_queue = false;
> }
>
> @@ -2466,7 +2529,7 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
> }
>
> /* throttling disk write I/O */
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> if (bdrv_exceed_io_limits(bs, nb_sectors, true, &wait_time)) {
> ret = qemu_block_queue_enqueue(bs->block_queue, bs, bdrv_aio_writev,
> sector_num, qiov, nb_sectors, cb, opaque);
> @@ -2488,14 +2551,14 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
> bs->wr_highest_sector = sector_num + nb_sectors - 1;
> }
>
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> bs->io_disps.bytes[BLOCK_IO_LIMIT_WRITE] +=
> (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
> bs->io_disps.ios[BLOCK_IO_LIMIT_WRITE]++;
> }
> }
>
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> bs->req_from_queue = false;
> }
>
> diff --git a/block.h b/block.h
> index f0dac62..97d2177 100644
> --- a/block.h
> +++ b/block.h
> @@ -57,6 +57,11 @@ void bdrv_info(Monitor *mon, QObject **ret_data);
> void bdrv_stats_print(Monitor *mon, const QObject *data);
> void bdrv_info_stats(Monitor *mon, QObject **ret_data);
>
> +/* disk I/O throttling */
> +void bdrv_io_limits_res_init(BlockDriverState *bs);
> +void bdrv_io_limits_res_deinit(BlockDriverState *bs);
> +bool bdrv_io_limits_enabled(BlockDriverState *bs);
> +
> void bdrv_init(void);
> void bdrv_init_with_whitelist(void);
> BlockDriver *bdrv_find_protocol(const char *filename);
> diff --git a/block_int.h b/block_int.h
> index 1ca826b..7c96094 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -199,6 +199,8 @@ struct BlockDriverState {
> BlockIODisp io_disps;
> BlockQueue *block_queue;
> QEMUTimer *block_timer;
> + bool io_limits_enabled;
> + bool io_unlimit;
> bool req_from_queue;
>
> /* I/O stats (display with "info blockstats"). */
> diff --git a/blockdev.c b/blockdev.c
> index aff6bb2..dc2e8a9 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -376,7 +376,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
> return NULL;
> }
>
> - /* disk io limits */
> + /* disk io throttling */
> iol_flag = qemu_opt_io_limits_enable_flag(opts, iol_opts);
> if (iol_flag) {
> memset(&io_limits, 0, sizeof(BlockIOLimit));
> @@ -387,6 +387,14 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
> io_limits.iops[2] = qemu_opt_get_number(opts, "iops", 0);
> io_limits.iops[0] = qemu_opt_get_number(opts, "iops_rd", 0);
> io_limits.iops[1] = qemu_opt_get_number(opts, "iops_wr", 0);
> +
> + if (((io_limits.bps[2] != 0)
> + && ((io_limits.bps[0] != 0) || (io_limits.bps[1] != 0)))
> + || ((io_limits.iops[2] != 0)
> + && ((io_limits.iops[0] != 0) || (io_limits.iops[1] != 0)))) {
Please define constants for read/write/total instead of using magic numbers.
> + error_report("Some params for io throttling can not coexist");
This error message is vague.
"bps and bps_rd/bps_wr cannot be used at the same time"
> + return NULL;
> + }
> }
>
> on_write_error = BLOCK_ERR_STOP_ENOSPC;
> @@ -765,6 +773,82 @@ int do_change_block(Monitor *mon, const char *device,
> return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
> }
>
> +/* throttling disk I/O limits */
> +int do_block_set_io_throttle(Monitor *mon,
> + const QDict *qdict, QObject **ret_data)
> +{
> + const char *devname = qdict_get_str(qdict, "device");
> + uint64_t bps = qdict_get_try_int(qdict, "bps", -1);
> + uint64_t bps_rd = qdict_get_try_int(qdict, "bps_rd", -1);
> + uint64_t bps_wr = qdict_get_try_int(qdict, "bps_wr", -1);
> + uint64_t iops = qdict_get_try_int(qdict, "iops", -1);
> + uint64_t iops_rd = qdict_get_try_int(qdict, "iops_rd", -1);
> + uint64_t iops_wr = qdict_get_try_int(qdict, "iops_wr", -1);
> + BlockDriverState *bs;
> +
> + bs = bdrv_find(devname);
> + if (!bs) {
> + qerror_report(QERR_DEVICE_NOT_FOUND, devname);
> + return -1;
> + }
> +
> + if (devname && (bps == -1) && (bps_rd == -1) && (bps_wr == -1)
> + && (iops == -1) && (iops_rd == -1) && (iops_wr == -1)) {
No need for the devname check. It must be non-NULL since it is a
mandatory argument, also bdrv_find() would have crashed since devname
is used unchecked.
> + qerror_report(QERR_IO_THROTTLE_NO_PARAM);
> + return -1;
> + }
> +
> + if (((bps != -1) && ((bps_rd != -1) || (bps_wr != -1)))
> + || ((iops != -1) && ((iops_rd != -1) || (iops_wr != -1)))) {
> + qerror_report(QERR_IO_THROTTLE_PARAM_ERROR);
> + return -1;
> + }
> +
> + if (bps != -1) {
> + bs->io_limits.bps[2] = bps;
> + bs->io_limits.bps[0] = 0;
> + bs->io_limits.bps[1] = 0;
> + }
> +
> + if ((bps_rd != -1) || (bps_wr != -1)) {
> + bs->io_limits.bps[0] = (bps_rd == -1) ? bs->io_limits.bps[0] : bps_rd;
> + bs->io_limits.bps[1] = (bps_wr == -1) ? bs->io_limits.bps[1] : bps_wr;
> + bs->io_limits.bps[2] = 0;
> + }
> +
> + if (iops != -1) {
> + bs->io_limits.iops[2] = iops;
> + bs->io_limits.iops[0] = 0;
> + bs->io_limits.iops[1] = 0;
> + }
> +
> + if ((iops_rd != -1) || (iops_wr != -1)) {
> + bs->io_limits.iops[0] =
> + (iops_rd == -1) ? bs->io_limits.iops[0] : iops_rd;
> + bs->io_limits.iops[1] =
> + (iops_wr == -1) ? bs->io_limits.iops[1] : iops_wr;
> + bs->io_limits.iops[2] = 0;
> + }
> +
> + if (bdrv_io_limits_enabled(bs)) {
> + if (!bs->io_limits_enabled) {
> + bdrv_io_limits_res_init(bs);
> + }
> + } else {
> + if (bs->io_limits_enabled) {
> + BlockQueue *queue = bs->block_queue;
> + if (!QTAILQ_EMPTY(&queue->requests)) {
> + bs->io_unlimit = true;
> + bs->io_limits_enabled = false;
> + } else {
> + bdrv_io_limits_res_deinit(bs);
> + }
> + }
> + }
bdrv_io_limits_res_init() and bdrv_io_limits_res_deinit() are
basically enable/disable functions. I would change this to:
if (!bs->io_limits_enabled && bdrv_io_limits_enabled(bs)) {
bdrv_io_limits_enable(bs);
} else if (bs->io_limits_enabled && !bdrv_io_limits_enabled(bs)) {
bdrv_io_limits_disable(bs);
}
And then remove the bs->io_unlimit field by immediately stopping I/O throttling:
void bdrv_io_limits_disable(BlockDriverState *bs)
{
bs->io_limits_enabled = false;
bs->req_from_queue = false;
if (bs->block_queue) {
qemu_block_queue_flush(bs->block_queue); /* <--- dispatch
queued requests */
qemu_del_block_queue(bs->block_queue);
}
if (bs->block_timer) {
qemu_del_timer(bs->block_timer);
qemu_free_timer(bs->block_timer);
}
bs->slice_start[0] = 0;
bs->slice_start[1] = 0;
bs->slice_end[0] = 0;
bs->slice_end[1] = 0;
}
The queue flushing code from bdrv_block_timer() could be moved into
qemu_block_queue_flush(). bdrv_block_timer() would call that function
instead of looping over the queue itself.
> +
> + return 0;
> +}
> +
> int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> {
> const char *id = qdict_get_str(qdict, "id");
> diff --git a/blockdev.h b/blockdev.h
> index 0a5144c..d0d0d77 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -63,6 +63,8 @@ int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
> int do_change_block(Monitor *mon, const char *device,
> const char *filename, const char *fmt);
> int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
> +int do_block_set_io_throttle(Monitor *mon,
> + const QDict *qdict, QObject **ret_data);
> int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
> int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index aceba74..3ca496d 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1210,6 +1210,21 @@ ETEXI
> },
>
> STEXI
> +@item block_set_io_throttle @var{device} @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
> +@findex block_set_io_throttle
> +Change I/O throttle limits for a block drive to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
> +ETEXI
> +
> + {
> + .name = "block_set_io_throttle",
> + .args_type = "device:B,bps:i?,bps_rd:i?,bps_wr:i?,iops:i?,iops_rd:i?,iops_wr:i?",
> + .params = "device [bps] [bps_rd] [bps_wr] [iops] [iops_rd] [iops_wr]",
> + .help = "change I/O throttle limts for a block drive",
> + .user_print = monitor_user_noop,
> + .mhandler.cmd_new = do_block_set_io_throttle,
> + },
> +
> +STEXI
> @item block_passwd @var{device} @var{password}
> @findex block_passwd
> Set the encrypted device @var{device} password to @var{password}
> diff --git a/qerror.c b/qerror.c
> index d7fcd93..db04df7 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -213,6 +213,14 @@ static const QErrorStringTable qerror_table[] = {
> .error_fmt = QERR_VNC_SERVER_FAILED,
> .desc = "Could not start VNC server on %(target)",
> },
> + {
> + .error_fmt = QERR_IO_THROTTLE_PARAM_ERROR,
> + .desc = "Some params for io throttling can not coexist",
> + },
> + {
> + .error_fmt = QERR_IO_THROTTLE_NO_PARAM,
> + .desc = "No params for io throttling are specified",
> + },
> {}
> };
>
> diff --git a/qerror.h b/qerror.h
> index 16c830d..892abbb 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -181,4 +181,10 @@ QError *qobject_to_qerror(const QObject *obj);
> #define QERR_FEATURE_DISABLED \
> "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
>
> +#define QERR_IO_THROTTLE_PARAM_ERROR \
> + "{ 'class': 'ParamsCoexistNotPermitted', 'data': {} }"
This error is not specific to I/O throttling. Please add a generic error:
#define QERR_INVALID_PARAMETER_COMBINATION \
"{ 'class': 'InvalidParameterCombination', 'data': { 'param1': %s,
'param2': %s } }"
> +#define QERR_IO_THROTTLE_NO_PARAM \
> + "{ 'class': 'NoParamsSpecified', 'data': {} }"
QERR_MISSING_PARAMETER already does this.
> +
> #endif /* QERROR_H */
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 92c5c3a..0aaeae1 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -828,6 +828,44 @@ Example:
> EQMP
>
> {
> + .name = "block_set_io_throttle",
> + .args_type = "device:B,bps:i?,bps_rd:i?,bps_wr:i?,iops:i?,iops_rd:i?,iops_wr:i?",
> + .params = "device [bps[ [bps_rd] [bps_wr] [iops] [iops_rd] [iops_wr]",
[bps]
> + .help = "change I/O throttle limts for a block drive",
> + .user_print = monitor_user_noop,
> + .mhandler.cmd_new = do_block_set_io_throttle,
> + },
> +
> +SQMP
> +block_set_io_throttle
> +------------
> +
> +Change I/O throttle limts for a block drive.
> +
> +Arguments:
> +
> +- "device": device name (json-string)
> +- "bps": total throughput value per second(json-int, optional)
Please document the units of bps ("bps" can mean bits per second or
bytes per second):
"total throughput limit in bytes per second (json-int, optional)"
> +- "bps_rd": read throughput value per second(json-int, optional)
> +- "bps_wr": read throughput value per second(json-int, optional)
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v1] qmp/hmp: add block_set_io_throttle and enhance query_block
@ 2011-08-04 13:07 ` Stefan Hajnoczi
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2011-08-04 13:07 UTC (permalink / raw)
To: Zhi Yong Wu
Cc: kwolf, aliguori, stefanha, kvm, mtosatti, qemu-devel, zwu.kernel,
ryanh, luowenj
On Thu, Aug 4, 2011 at 5:34 AM, Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> wrote:
> @@ -1387,6 +1422,11 @@ void bdrv_set_io_limits(BlockDriverState *bs,
> {
> memset(&bs->io_limits, 0, sizeof(BlockIOLimit));
> bs->io_limits = *io_limits;
> + if (bdrv_io_limits_enabled(bs)) {
> + bs->io_limits_enabled = true;
> + } else {
> + bs->io_limits_enabled = false;
> + }
Or in one line:
bs->io_limits_enabled = bdrv_io_limits_enabled(bs);
> }
>
> /* Recognize floppy formats */
> @@ -1621,6 +1661,7 @@ BlockDriverState *bdrv_find(const char *name)
> return NULL;
> }
>
> +/* disk I/O throttling */
Looks like this should not be here.
> BlockDriverState *bdrv_next(BlockDriverState *bs)
> {
> if (!bs) {
> @@ -1784,6 +1825,16 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
> qdict_get_bool(qdict, "ro"),
> qdict_get_str(qdict, "drv"),
> qdict_get_bool(qdict, "encrypted"));
> +
> + monitor_printf(mon, " bps=%" PRId64 " bps_rd=%" PRId64
> + " bps_wr=%" PRId64 " iops=%" PRId64
> + " iops_rd=%" PRId64 " iops_wr=%" PRId64,
> + qdict_get_int(qdict, "bps"),
> + qdict_get_int(qdict, "bps_rd"),
> + qdict_get_int(qdict, "bps_wr"),
> + qdict_get_int(qdict, "iops"),
> + qdict_get_int(qdict, "iops_rd"),
> + qdict_get_int(qdict, "iops_wr"));
> } else {
> monitor_printf(mon, " [not inserted]");
> }
> @@ -1816,10 +1867,22 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
> QDict *bs_dict = qobject_to_qdict(bs_obj);
>
> obj = qobject_from_jsonf("{ 'file': %s, 'ro': %i, 'drv': %s, "
> - "'encrypted': %i }",
> + "'encrypted': %i, "
> + "'bps': %" PRId64 ","
> + "'bps_rd': %" PRId64 ","
> + "'bps_wr': %" PRId64 ","
> + "'iops': %" PRId64 ","
> + "'iops_rd': %" PRId64 ","
> + "'iops_wr': %" PRId64 "}",
> bs->filename, bs->read_only,
> bs->drv->format_name,
> - bdrv_is_encrypted(bs));
> + bdrv_is_encrypted(bs),
> + bs->io_limits.bps[2],
> + bs->io_limits.bps[0],
> + bs->io_limits.bps[1],
> + bs->io_limits.iops[2],
> + bs->io_limits.iops[0],
> + bs->io_limits.iops[1]);
> if (bs->backing_file[0] != '\0') {
> QDict *qdict = qobject_to_qdict(obj);
> qdict_put(qdict, "backing_file",
> @@ -2307,7 +2370,7 @@ static bool bdrv_exceed_io_limits(BlockDriverState *bs, int nb_sectors,
> }
>
> /* If a limit was exceeded, immediately queue this request */
> - if ((bs->req_from_queue == false)
> + if (!bs->req_from_queue
> && !QTAILQ_EMPTY(&bs->block_queue->requests)) {
> if (bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL]
> || bs->io_limits.bps[is_write] || bs->io_limits.iops[is_write]
> @@ -2362,14 +2425,14 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
> trace_bdrv_aio_readv(bs, sector_num, nb_sectors, opaque);
>
> if (!drv || bdrv_check_request(bs, sector_num, nb_sectors)) {
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> bs->req_from_queue = false;
> }
> return NULL;
> }
>
> /* throttling disk read I/O */
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> if (bdrv_exceed_io_limits(bs, nb_sectors, false, &wait_time)) {
> ret = qemu_block_queue_enqueue(bs->block_queue, bs, bdrv_aio_readv,
> sector_num, qiov, nb_sectors, cb, opaque);
> @@ -2388,14 +2451,14 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
> bs->rd_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
> bs->rd_ops ++;
>
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> bs->io_disps.bytes[BLOCK_IO_LIMIT_READ] +=
> (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
> bs->io_disps.ios[BLOCK_IO_LIMIT_READ]++;
> }
> }
>
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> bs->req_from_queue = false;
> }
>
> @@ -2451,7 +2514,7 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>
> if (!drv || bs->read_only
> || bdrv_check_request(bs, sector_num, nb_sectors)) {
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> bs->req_from_queue = false;
> }
>
> @@ -2466,7 +2529,7 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
> }
>
> /* throttling disk write I/O */
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> if (bdrv_exceed_io_limits(bs, nb_sectors, true, &wait_time)) {
> ret = qemu_block_queue_enqueue(bs->block_queue, bs, bdrv_aio_writev,
> sector_num, qiov, nb_sectors, cb, opaque);
> @@ -2488,14 +2551,14 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
> bs->wr_highest_sector = sector_num + nb_sectors - 1;
> }
>
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> bs->io_disps.bytes[BLOCK_IO_LIMIT_WRITE] +=
> (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
> bs->io_disps.ios[BLOCK_IO_LIMIT_WRITE]++;
> }
> }
>
> - if (bdrv_io_limits_enable(&bs->io_limits)) {
> + if (bs->io_limits_enabled) {
> bs->req_from_queue = false;
> }
>
> diff --git a/block.h b/block.h
> index f0dac62..97d2177 100644
> --- a/block.h
> +++ b/block.h
> @@ -57,6 +57,11 @@ void bdrv_info(Monitor *mon, QObject **ret_data);
> void bdrv_stats_print(Monitor *mon, const QObject *data);
> void bdrv_info_stats(Monitor *mon, QObject **ret_data);
>
> +/* disk I/O throttling */
> +void bdrv_io_limits_res_init(BlockDriverState *bs);
> +void bdrv_io_limits_res_deinit(BlockDriverState *bs);
> +bool bdrv_io_limits_enabled(BlockDriverState *bs);
> +
> void bdrv_init(void);
> void bdrv_init_with_whitelist(void);
> BlockDriver *bdrv_find_protocol(const char *filename);
> diff --git a/block_int.h b/block_int.h
> index 1ca826b..7c96094 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -199,6 +199,8 @@ struct BlockDriverState {
> BlockIODisp io_disps;
> BlockQueue *block_queue;
> QEMUTimer *block_timer;
> + bool io_limits_enabled;
> + bool io_unlimit;
> bool req_from_queue;
>
> /* I/O stats (display with "info blockstats"). */
> diff --git a/blockdev.c b/blockdev.c
> index aff6bb2..dc2e8a9 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -376,7 +376,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
> return NULL;
> }
>
> - /* disk io limits */
> + /* disk io throttling */
> iol_flag = qemu_opt_io_limits_enable_flag(opts, iol_opts);
> if (iol_flag) {
> memset(&io_limits, 0, sizeof(BlockIOLimit));
> @@ -387,6 +387,14 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
> io_limits.iops[2] = qemu_opt_get_number(opts, "iops", 0);
> io_limits.iops[0] = qemu_opt_get_number(opts, "iops_rd", 0);
> io_limits.iops[1] = qemu_opt_get_number(opts, "iops_wr", 0);
> +
> + if (((io_limits.bps[2] != 0)
> + && ((io_limits.bps[0] != 0) || (io_limits.bps[1] != 0)))
> + || ((io_limits.iops[2] != 0)
> + && ((io_limits.iops[0] != 0) || (io_limits.iops[1] != 0)))) {
Please define constants for read/write/total instead of using magic numbers.
> + error_report("Some params for io throttling can not coexist");
This error message is vague.
"bps and bps_rd/bps_wr cannot be used at the same time"
> + return NULL;
> + }
> }
>
> on_write_error = BLOCK_ERR_STOP_ENOSPC;
> @@ -765,6 +773,82 @@ int do_change_block(Monitor *mon, const char *device,
> return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
> }
>
> +/* throttling disk I/O limits */
> +int do_block_set_io_throttle(Monitor *mon,
> + const QDict *qdict, QObject **ret_data)
> +{
> + const char *devname = qdict_get_str(qdict, "device");
> + uint64_t bps = qdict_get_try_int(qdict, "bps", -1);
> + uint64_t bps_rd = qdict_get_try_int(qdict, "bps_rd", -1);
> + uint64_t bps_wr = qdict_get_try_int(qdict, "bps_wr", -1);
> + uint64_t iops = qdict_get_try_int(qdict, "iops", -1);
> + uint64_t iops_rd = qdict_get_try_int(qdict, "iops_rd", -1);
> + uint64_t iops_wr = qdict_get_try_int(qdict, "iops_wr", -1);
> + BlockDriverState *bs;
> +
> + bs = bdrv_find(devname);
> + if (!bs) {
> + qerror_report(QERR_DEVICE_NOT_FOUND, devname);
> + return -1;
> + }
> +
> + if (devname && (bps == -1) && (bps_rd == -1) && (bps_wr == -1)
> + && (iops == -1) && (iops_rd == -1) && (iops_wr == -1)) {
No need for the devname check. It must be non-NULL since it is a
mandatory argument, also bdrv_find() would have crashed since devname
is used unchecked.
> + qerror_report(QERR_IO_THROTTLE_NO_PARAM);
> + return -1;
> + }
> +
> + if (((bps != -1) && ((bps_rd != -1) || (bps_wr != -1)))
> + || ((iops != -1) && ((iops_rd != -1) || (iops_wr != -1)))) {
> + qerror_report(QERR_IO_THROTTLE_PARAM_ERROR);
> + return -1;
> + }
> +
> + if (bps != -1) {
> + bs->io_limits.bps[2] = bps;
> + bs->io_limits.bps[0] = 0;
> + bs->io_limits.bps[1] = 0;
> + }
> +
> + if ((bps_rd != -1) || (bps_wr != -1)) {
> + bs->io_limits.bps[0] = (bps_rd == -1) ? bs->io_limits.bps[0] : bps_rd;
> + bs->io_limits.bps[1] = (bps_wr == -1) ? bs->io_limits.bps[1] : bps_wr;
> + bs->io_limits.bps[2] = 0;
> + }
> +
> + if (iops != -1) {
> + bs->io_limits.iops[2] = iops;
> + bs->io_limits.iops[0] = 0;
> + bs->io_limits.iops[1] = 0;
> + }
> +
> + if ((iops_rd != -1) || (iops_wr != -1)) {
> + bs->io_limits.iops[0] =
> + (iops_rd == -1) ? bs->io_limits.iops[0] : iops_rd;
> + bs->io_limits.iops[1] =
> + (iops_wr == -1) ? bs->io_limits.iops[1] : iops_wr;
> + bs->io_limits.iops[2] = 0;
> + }
> +
> + if (bdrv_io_limits_enabled(bs)) {
> + if (!bs->io_limits_enabled) {
> + bdrv_io_limits_res_init(bs);
> + }
> + } else {
> + if (bs->io_limits_enabled) {
> + BlockQueue *queue = bs->block_queue;
> + if (!QTAILQ_EMPTY(&queue->requests)) {
> + bs->io_unlimit = true;
> + bs->io_limits_enabled = false;
> + } else {
> + bdrv_io_limits_res_deinit(bs);
> + }
> + }
> + }
bdrv_io_limits_res_init() and bdrv_io_limits_res_deinit() are
basically enable/disable functions. I would change this to:
if (!bs->io_limits_enabled && bdrv_io_limits_enabled(bs)) {
bdrv_io_limits_enable(bs);
} else if (bs->io_limits_enabled && !bdrv_io_limits_enabled(bs)) {
bdrv_io_limits_disable(bs);
}
And then remove the bs->io_unlimit field by immediately stopping I/O throttling:
void bdrv_io_limits_disable(BlockDriverState *bs)
{
bs->io_limits_enabled = false;
bs->req_from_queue = false;
if (bs->block_queue) {
qemu_block_queue_flush(bs->block_queue); /* <--- dispatch
queued requests */
qemu_del_block_queue(bs->block_queue);
}
if (bs->block_timer) {
qemu_del_timer(bs->block_timer);
qemu_free_timer(bs->block_timer);
}
bs->slice_start[0] = 0;
bs->slice_start[1] = 0;
bs->slice_end[0] = 0;
bs->slice_end[1] = 0;
}
The queue flushing code from bdrv_block_timer() could be moved into
qemu_block_queue_flush(). bdrv_block_timer() would call that function
instead of looping over the queue itself.
> +
> + return 0;
> +}
> +
> int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> {
> const char *id = qdict_get_str(qdict, "id");
> diff --git a/blockdev.h b/blockdev.h
> index 0a5144c..d0d0d77 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -63,6 +63,8 @@ int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
> int do_change_block(Monitor *mon, const char *device,
> const char *filename, const char *fmt);
> int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
> +int do_block_set_io_throttle(Monitor *mon,
> + const QDict *qdict, QObject **ret_data);
> int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
> int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index aceba74..3ca496d 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1210,6 +1210,21 @@ ETEXI
> },
>
> STEXI
> +@item block_set_io_throttle @var{device} @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
> +@findex block_set_io_throttle
> +Change I/O throttle limits for a block drive to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
> +ETEXI
> +
> + {
> + .name = "block_set_io_throttle",
> + .args_type = "device:B,bps:i?,bps_rd:i?,bps_wr:i?,iops:i?,iops_rd:i?,iops_wr:i?",
> + .params = "device [bps] [bps_rd] [bps_wr] [iops] [iops_rd] [iops_wr]",
> + .help = "change I/O throttle limts for a block drive",
> + .user_print = monitor_user_noop,
> + .mhandler.cmd_new = do_block_set_io_throttle,
> + },
> +
> +STEXI
> @item block_passwd @var{device} @var{password}
> @findex block_passwd
> Set the encrypted device @var{device} password to @var{password}
> diff --git a/qerror.c b/qerror.c
> index d7fcd93..db04df7 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -213,6 +213,14 @@ static const QErrorStringTable qerror_table[] = {
> .error_fmt = QERR_VNC_SERVER_FAILED,
> .desc = "Could not start VNC server on %(target)",
> },
> + {
> + .error_fmt = QERR_IO_THROTTLE_PARAM_ERROR,
> + .desc = "Some params for io throttling can not coexist",
> + },
> + {
> + .error_fmt = QERR_IO_THROTTLE_NO_PARAM,
> + .desc = "No params for io throttling are specified",
> + },
> {}
> };
>
> diff --git a/qerror.h b/qerror.h
> index 16c830d..892abbb 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -181,4 +181,10 @@ QError *qobject_to_qerror(const QObject *obj);
> #define QERR_FEATURE_DISABLED \
> "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
>
> +#define QERR_IO_THROTTLE_PARAM_ERROR \
> + "{ 'class': 'ParamsCoexistNotPermitted', 'data': {} }"
This error is not specific to I/O throttling. Please add a generic error:
#define QERR_INVALID_PARAMETER_COMBINATION \
"{ 'class': 'InvalidParameterCombination', 'data': { 'param1': %s,
'param2': %s } }"
> +#define QERR_IO_THROTTLE_NO_PARAM \
> + "{ 'class': 'NoParamsSpecified', 'data': {} }"
QERR_MISSING_PARAMETER already does this.
> +
> #endif /* QERROR_H */
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 92c5c3a..0aaeae1 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -828,6 +828,44 @@ Example:
> EQMP
>
> {
> + .name = "block_set_io_throttle",
> + .args_type = "device:B,bps:i?,bps_rd:i?,bps_wr:i?,iops:i?,iops_rd:i?,iops_wr:i?",
> + .params = "device [bps[ [bps_rd] [bps_wr] [iops] [iops_rd] [iops_wr]",
[bps]
> + .help = "change I/O throttle limts for a block drive",
> + .user_print = monitor_user_noop,
> + .mhandler.cmd_new = do_block_set_io_throttle,
> + },
> +
> +SQMP
> +block_set_io_throttle
> +------------
> +
> +Change I/O throttle limts for a block drive.
> +
> +Arguments:
> +
> +- "device": device name (json-string)
> +- "bps": total throughput value per second(json-int, optional)
Please document the units of bps ("bps" can mean bits per second or
bytes per second):
"total throughput limit in bytes per second (json-int, optional)"
> +- "bps_rd": read throughput value per second(json-int, optional)
> +- "bps_wr": read throughput value per second(json-int, optional)
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v1] qmp/hmp: add block_set_io_throttle and enhance query_block
2011-08-04 13:07 ` Stefan Hajnoczi
@ 2011-08-05 2:03 ` Zhi Yong Wu
-1 siblings, 0 replies; 6+ messages in thread
From: Zhi Yong Wu @ 2011-08-05 2:03 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Zhi Yong Wu, qemu-devel, kwolf, aliguori, stefanha, kvm,
mtosatti, ryanh, luowenj
On Thu, Aug 4, 2011 at 9:07 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Aug 4, 2011 at 5:34 AM, Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> wrote:
>> @@ -1387,6 +1422,11 @@ void bdrv_set_io_limits(BlockDriverState *bs,
>> {
>> memset(&bs->io_limits, 0, sizeof(BlockIOLimit));
>> bs->io_limits = *io_limits;
>> + if (bdrv_io_limits_enabled(bs)) {
>> + bs->io_limits_enabled = true;
>> + } else {
>> + bs->io_limits_enabled = false;
>> + }
>
> Or in one line:
> bs->io_limits_enabled = bdrv_io_limits_enabled(bs);
nice.
>
>> }
>>
>> /* Recognize floppy formats */
>> @@ -1621,6 +1661,7 @@ BlockDriverState *bdrv_find(const char *name)
>> return NULL;
>> }
>>
>> +/* disk I/O throttling */
>
> Looks like this should not be here.\
Let me check when i will go to office next week.
>
>> BlockDriverState *bdrv_next(BlockDriverState *bs)
>> {
>> if (!bs) {
>> @@ -1784,6 +1825,16 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
>> qdict_get_bool(qdict, "ro"),
>> qdict_get_str(qdict, "drv"),
>> qdict_get_bool(qdict, "encrypted"));
>> +
>> + monitor_printf(mon, " bps=%" PRId64 " bps_rd=%" PRId64
>> + " bps_wr=%" PRId64 " iops=%" PRId64
>> + " iops_rd=%" PRId64 " iops_wr=%" PRId64,
>> + qdict_get_int(qdict, "bps"),
>> + qdict_get_int(qdict, "bps_rd"),
>> + qdict_get_int(qdict, "bps_wr"),
>> + qdict_get_int(qdict, "iops"),
>> + qdict_get_int(qdict, "iops_rd"),
>> + qdict_get_int(qdict, "iops_wr"));
>> } else {
>> monitor_printf(mon, " [not inserted]");
>> }
>> @@ -1816,10 +1867,22 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
>> QDict *bs_dict = qobject_to_qdict(bs_obj);
>>
>> obj = qobject_from_jsonf("{ 'file': %s, 'ro': %i, 'drv': %s, "
>> - "'encrypted': %i }",
>> + "'encrypted': %i, "
>> + "'bps': %" PRId64 ","
>> + "'bps_rd': %" PRId64 ","
>> + "'bps_wr': %" PRId64 ","
>> + "'iops': %" PRId64 ","
>> + "'iops_rd': %" PRId64 ","
>> + "'iops_wr': %" PRId64 "}",
>> bs->filename, bs->read_only,
>> bs->drv->format_name,
>> - bdrv_is_encrypted(bs));
>> + bdrv_is_encrypted(bs),
>> + bs->io_limits.bps[2],
>> + bs->io_limits.bps[0],
>> + bs->io_limits.bps[1],
>> + bs->io_limits.iops[2],
>> + bs->io_limits.iops[0],
>> + bs->io_limits.iops[1]);
>> if (bs->backing_file[0] != '\0') {
>> QDict *qdict = qobject_to_qdict(obj);
>> qdict_put(qdict, "backing_file",
>> @@ -2307,7 +2370,7 @@ static bool bdrv_exceed_io_limits(BlockDriverState *bs, int nb_sectors,
>> }
>>
>> /* If a limit was exceeded, immediately queue this request */
>> - if ((bs->req_from_queue == false)
>> + if (!bs->req_from_queue
>> && !QTAILQ_EMPTY(&bs->block_queue->requests)) {
>> if (bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL]
>> || bs->io_limits.bps[is_write] || bs->io_limits.iops[is_write]
>> @@ -2362,14 +2425,14 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
>> trace_bdrv_aio_readv(bs, sector_num, nb_sectors, opaque);
>>
>> if (!drv || bdrv_check_request(bs, sector_num, nb_sectors)) {
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> bs->req_from_queue = false;
>> }
>> return NULL;
>> }
>>
>> /* throttling disk read I/O */
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> if (bdrv_exceed_io_limits(bs, nb_sectors, false, &wait_time)) {
>> ret = qemu_block_queue_enqueue(bs->block_queue, bs, bdrv_aio_readv,
>> sector_num, qiov, nb_sectors, cb, opaque);
>> @@ -2388,14 +2451,14 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
>> bs->rd_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
>> bs->rd_ops ++;
>>
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> bs->io_disps.bytes[BLOCK_IO_LIMIT_READ] +=
>> (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
>> bs->io_disps.ios[BLOCK_IO_LIMIT_READ]++;
>> }
>> }
>>
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> bs->req_from_queue = false;
>> }
>>
>> @@ -2451,7 +2514,7 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>>
>> if (!drv || bs->read_only
>> || bdrv_check_request(bs, sector_num, nb_sectors)) {
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> bs->req_from_queue = false;
>> }
>>
>> @@ -2466,7 +2529,7 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>> }
>>
>> /* throttling disk write I/O */
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> if (bdrv_exceed_io_limits(bs, nb_sectors, true, &wait_time)) {
>> ret = qemu_block_queue_enqueue(bs->block_queue, bs, bdrv_aio_writev,
>> sector_num, qiov, nb_sectors, cb, opaque);
>> @@ -2488,14 +2551,14 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>> bs->wr_highest_sector = sector_num + nb_sectors - 1;
>> }
>>
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> bs->io_disps.bytes[BLOCK_IO_LIMIT_WRITE] +=
>> (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
>> bs->io_disps.ios[BLOCK_IO_LIMIT_WRITE]++;
>> }
>> }
>>
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> bs->req_from_queue = false;
>> }
>>
>> diff --git a/block.h b/block.h
>> index f0dac62..97d2177 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -57,6 +57,11 @@ void bdrv_info(Monitor *mon, QObject **ret_data);
>> void bdrv_stats_print(Monitor *mon, const QObject *data);
>> void bdrv_info_stats(Monitor *mon, QObject **ret_data);
>>
>> +/* disk I/O throttling */
>> +void bdrv_io_limits_res_init(BlockDriverState *bs);
>> +void bdrv_io_limits_res_deinit(BlockDriverState *bs);
>> +bool bdrv_io_limits_enabled(BlockDriverState *bs);
>> +
>> void bdrv_init(void);
>> void bdrv_init_with_whitelist(void);
>> BlockDriver *bdrv_find_protocol(const char *filename);
>> diff --git a/block_int.h b/block_int.h
>> index 1ca826b..7c96094 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -199,6 +199,8 @@ struct BlockDriverState {
>> BlockIODisp io_disps;
>> BlockQueue *block_queue;
>> QEMUTimer *block_timer;
>> + bool io_limits_enabled;
>> + bool io_unlimit;
>> bool req_from_queue;
>>
>> /* I/O stats (display with "info blockstats"). */
>> diff --git a/blockdev.c b/blockdev.c
>> index aff6bb2..dc2e8a9 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -376,7 +376,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>> return NULL;
>> }
>>
>> - /* disk io limits */
>> + /* disk io throttling */
>> iol_flag = qemu_opt_io_limits_enable_flag(opts, iol_opts);
>> if (iol_flag) {
>> memset(&io_limits, 0, sizeof(BlockIOLimit));
>> @@ -387,6 +387,14 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>> io_limits.iops[2] = qemu_opt_get_number(opts, "iops", 0);
>> io_limits.iops[0] = qemu_opt_get_number(opts, "iops_rd", 0);
>> io_limits.iops[1] = qemu_opt_get_number(opts, "iops_wr", 0);
>> +
>> + if (((io_limits.bps[2] != 0)
>> + && ((io_limits.bps[0] != 0) || (io_limits.bps[1] != 0)))
>> + || ((io_limits.iops[2] != 0)
>> + && ((io_limits.iops[0] != 0) || (io_limits.iops[1] != 0)))) {
>
> Please define constants for read/write/total instead of using magic numbers.
They have been defined. OK. let me replace those magic numbers with them.
>
>> + error_report("Some params for io throttling can not coexist");
>
> This error message is vague.
>
> "bps and bps_rd/bps_wr cannot be used at the same time"
>
>> + return NULL;
>> + }
>> }
>>
>> on_write_error = BLOCK_ERR_STOP_ENOSPC;
>> @@ -765,6 +773,82 @@ int do_change_block(Monitor *mon, const char *device,
>> return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
>> }
>>
>> +/* throttling disk I/O limits */
>> +int do_block_set_io_throttle(Monitor *mon,
>> + const QDict *qdict, QObject **ret_data)
>> +{
>> + const char *devname = qdict_get_str(qdict, "device");
>> + uint64_t bps = qdict_get_try_int(qdict, "bps", -1);
>> + uint64_t bps_rd = qdict_get_try_int(qdict, "bps_rd", -1);
>> + uint64_t bps_wr = qdict_get_try_int(qdict, "bps_wr", -1);
>> + uint64_t iops = qdict_get_try_int(qdict, "iops", -1);
>> + uint64_t iops_rd = qdict_get_try_int(qdict, "iops_rd", -1);
>> + uint64_t iops_wr = qdict_get_try_int(qdict, "iops_wr", -1);
>> + BlockDriverState *bs;
>> +
>> + bs = bdrv_find(devname);
>> + if (!bs) {
>> + qerror_report(QERR_DEVICE_NOT_FOUND, devname);
>> + return -1;
>> + }
>> +
>> + if (devname && (bps == -1) && (bps_rd == -1) && (bps_wr == -1)
>> + && (iops == -1) && (iops_rd == -1) && (iops_wr == -1)) {
>
> No need for the devname check. It must be non-NULL since it is a
> mandatory argument, also bdrv_find() would have crashed since devname
> is used unchecked.
OK. i will remove devname.
>
>> + qerror_report(QERR_IO_THROTTLE_NO_PARAM);
>> + return -1;
>> + }
>> +
>> + if (((bps != -1) && ((bps_rd != -1) || (bps_wr != -1)))
>> + || ((iops != -1) && ((iops_rd != -1) || (iops_wr != -1)))) {
>> + qerror_report(QERR_IO_THROTTLE_PARAM_ERROR);
>> + return -1;
>> + }
>> +
>> + if (bps != -1) {
>> + bs->io_limits.bps[2] = bps;
>> + bs->io_limits.bps[0] = 0;
>> + bs->io_limits.bps[1] = 0;
>> + }
>> +
>> + if ((bps_rd != -1) || (bps_wr != -1)) {
>> + bs->io_limits.bps[0] = (bps_rd == -1) ? bs->io_limits.bps[0] : bps_rd;
>> + bs->io_limits.bps[1] = (bps_wr == -1) ? bs->io_limits.bps[1] : bps_wr;
>> + bs->io_limits.bps[2] = 0;
>> + }
>> +
>> + if (iops != -1) {
>> + bs->io_limits.iops[2] = iops;
>> + bs->io_limits.iops[0] = 0;
>> + bs->io_limits.iops[1] = 0;
>> + }
>> +
>> + if ((iops_rd != -1) || (iops_wr != -1)) {
>> + bs->io_limits.iops[0] =
>> + (iops_rd == -1) ? bs->io_limits.iops[0] : iops_rd;
>> + bs->io_limits.iops[1] =
>> + (iops_wr == -1) ? bs->io_limits.iops[1] : iops_wr;
>> + bs->io_limits.iops[2] = 0;
>> + }
>> +
>> + if (bdrv_io_limits_enabled(bs)) {
>> + if (!bs->io_limits_enabled) {
>> + bdrv_io_limits_res_init(bs);
>> + }
>> + } else {
>> + if (bs->io_limits_enabled) {
>> + BlockQueue *queue = bs->block_queue;
>> + if (!QTAILQ_EMPTY(&queue->requests)) {
>> + bs->io_unlimit = true;
>> + bs->io_limits_enabled = false;
>> + } else {
>> + bdrv_io_limits_res_deinit(bs);
>> + }
>> + }
>> + }
>
> bdrv_io_limits_res_init() and bdrv_io_limits_res_deinit() are
> basically enable/disable functions. I would change this to:
>
> if (!bs->io_limits_enabled && bdrv_io_limits_enabled(bs)) {
> bdrv_io_limits_enable(bs);
> } else if (bs->io_limits_enabled && !bdrv_io_limits_enabled(bs)) {
> bdrv_io_limits_disable(bs);
> }
Good point.
>
> And then remove the bs->io_unlimit field by immediately stopping I/O throttling:
>
> void bdrv_io_limits_disable(BlockDriverState *bs)
> {
> bs->io_limits_enabled = false;
> bs->req_from_queue = false;
>
> if (bs->block_queue) {
> qemu_block_queue_flush(bs->block_queue); /* <--- dispatch
> queued requests */
> qemu_del_block_queue(bs->block_queue);
> }
>
> if (bs->block_timer) {
> qemu_del_timer(bs->block_timer);
> qemu_free_timer(bs->block_timer);
> }
>
> bs->slice_start[0] = 0;
> bs->slice_start[1] = 0;
>
> bs->slice_end[0] = 0;
> bs->slice_end[1] = 0;
> }
>
> The queue flushing code from bdrv_block_timer() could be moved into
> qemu_block_queue_flush(). bdrv_block_timer() would call that function
> instead of looping over the queue itself.
OK.
>
>> +
>> + return 0;
>> +}
>> +
>> int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>> {
>> const char *id = qdict_get_str(qdict, "id");
>> diff --git a/blockdev.h b/blockdev.h
>> index 0a5144c..d0d0d77 100644
>> --- a/blockdev.h
>> +++ b/blockdev.h
>> @@ -63,6 +63,8 @@ int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
>> int do_change_block(Monitor *mon, const char *device,
>> const char *filename, const char *fmt);
>> int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>> +int do_block_set_io_throttle(Monitor *mon,
>> + const QDict *qdict, QObject **ret_data);
>> int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
>> int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index aceba74..3ca496d 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1210,6 +1210,21 @@ ETEXI
>> },
>>
>> STEXI
>> +@item block_set_io_throttle @var{device} @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
>> +@findex block_set_io_throttle
>> +Change I/O throttle limits for a block drive to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
>> +ETEXI
>> +
>> + {
>> + .name = "block_set_io_throttle",
>> + .args_type = "device:B,bps:i?,bps_rd:i?,bps_wr:i?,iops:i?,iops_rd:i?,iops_wr:i?",
>> + .params = "device [bps] [bps_rd] [bps_wr] [iops] [iops_rd] [iops_wr]",
>> + .help = "change I/O throttle limts for a block drive",
>> + .user_print = monitor_user_noop,
>> + .mhandler.cmd_new = do_block_set_io_throttle,
>> + },
>> +
>> +STEXI
>> @item block_passwd @var{device} @var{password}
>> @findex block_passwd
>> Set the encrypted device @var{device} password to @var{password}
>> diff --git a/qerror.c b/qerror.c
>> index d7fcd93..db04df7 100644
>> --- a/qerror.c
>> +++ b/qerror.c
>> @@ -213,6 +213,14 @@ static const QErrorStringTable qerror_table[] = {
>> .error_fmt = QERR_VNC_SERVER_FAILED,
>> .desc = "Could not start VNC server on %(target)",
>> },
>> + {
>> + .error_fmt = QERR_IO_THROTTLE_PARAM_ERROR,
>> + .desc = "Some params for io throttling can not coexist",
>> + },
>> + {
>> + .error_fmt = QERR_IO_THROTTLE_NO_PARAM,
>> + .desc = "No params for io throttling are specified",
>> + },
>> {}
>> };
>>
>> diff --git a/qerror.h b/qerror.h
>> index 16c830d..892abbb 100644
>> --- a/qerror.h
>> +++ b/qerror.h
>> @@ -181,4 +181,10 @@ QError *qobject_to_qerror(const QObject *obj);
>> #define QERR_FEATURE_DISABLED \
>> "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
>>
>> +#define QERR_IO_THROTTLE_PARAM_ERROR \
>> + "{ 'class': 'ParamsCoexistNotPermitted', 'data': {} }"
>
> This error is not specific to I/O throttling. Please add a generic error:
>
> #define QERR_INVALID_PARAMETER_COMBINATION \
> "{ 'class': 'InvalidParameterCombination', 'data': { 'param1': %s,
> 'param2': %s } }"
>
>> +#define QERR_IO_THROTTLE_NO_PARAM \
>> + "{ 'class': 'NoParamsSpecified', 'data': {} }"
>
> QERR_MISSING_PARAMETER already does this.
>
>> +
>> #endif /* QERROR_H */
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 92c5c3a..0aaeae1 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -828,6 +828,44 @@ Example:
>> EQMP
>>
>> {
>> + .name = "block_set_io_throttle",
>> + .args_type = "device:B,bps:i?,bps_rd:i?,bps_wr:i?,iops:i?,iops_rd:i?,iops_wr:i?",
>> + .params = "device [bps[ [bps_rd] [bps_wr] [iops] [iops_rd] [iops_wr]",
>
> [bps]
Good catch.
>
>> + .help = "change I/O throttle limts for a block drive",
>> + .user_print = monitor_user_noop,
>> + .mhandler.cmd_new = do_block_set_io_throttle,
>> + },
>> +
>> +SQMP
>> +block_set_io_throttle
>> +------------
>> +
>> +Change I/O throttle limts for a block drive.
>> +
>> +Arguments:
>> +
>> +- "device": device name (json-string)
>> +- "bps": total throughput value per second(json-int, optional)
>
> Please document the units of bps ("bps" can mean bits per second or
> bytes per second):
>
> "total throughput limit in bytes per second (json-int, optional)"
OK.
>
>> +- "bps_rd": read throughput value per second(json-int, optional)
>> +- "bps_wr": read throughput value per second(json-int, optional)
>
> Stefan
>
Stefan, thanks for your good comments and spent time.
--
Regards,
Zhi Yong Wu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v1] qmp/hmp: add block_set_io_throttle and enhance query_block
@ 2011-08-05 2:03 ` Zhi Yong Wu
0 siblings, 0 replies; 6+ messages in thread
From: Zhi Yong Wu @ 2011-08-05 2:03 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kwolf, aliguori, stefanha, kvm, mtosatti, qemu-devel, ryanh,
Zhi Yong Wu, luowenj
On Thu, Aug 4, 2011 at 9:07 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Aug 4, 2011 at 5:34 AM, Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> wrote:
>> @@ -1387,6 +1422,11 @@ void bdrv_set_io_limits(BlockDriverState *bs,
>> {
>> memset(&bs->io_limits, 0, sizeof(BlockIOLimit));
>> bs->io_limits = *io_limits;
>> + if (bdrv_io_limits_enabled(bs)) {
>> + bs->io_limits_enabled = true;
>> + } else {
>> + bs->io_limits_enabled = false;
>> + }
>
> Or in one line:
> bs->io_limits_enabled = bdrv_io_limits_enabled(bs);
nice.
>
>> }
>>
>> /* Recognize floppy formats */
>> @@ -1621,6 +1661,7 @@ BlockDriverState *bdrv_find(const char *name)
>> return NULL;
>> }
>>
>> +/* disk I/O throttling */
>
> Looks like this should not be here.\
Let me check when i will go to office next week.
>
>> BlockDriverState *bdrv_next(BlockDriverState *bs)
>> {
>> if (!bs) {
>> @@ -1784,6 +1825,16 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
>> qdict_get_bool(qdict, "ro"),
>> qdict_get_str(qdict, "drv"),
>> qdict_get_bool(qdict, "encrypted"));
>> +
>> + monitor_printf(mon, " bps=%" PRId64 " bps_rd=%" PRId64
>> + " bps_wr=%" PRId64 " iops=%" PRId64
>> + " iops_rd=%" PRId64 " iops_wr=%" PRId64,
>> + qdict_get_int(qdict, "bps"),
>> + qdict_get_int(qdict, "bps_rd"),
>> + qdict_get_int(qdict, "bps_wr"),
>> + qdict_get_int(qdict, "iops"),
>> + qdict_get_int(qdict, "iops_rd"),
>> + qdict_get_int(qdict, "iops_wr"));
>> } else {
>> monitor_printf(mon, " [not inserted]");
>> }
>> @@ -1816,10 +1867,22 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
>> QDict *bs_dict = qobject_to_qdict(bs_obj);
>>
>> obj = qobject_from_jsonf("{ 'file': %s, 'ro': %i, 'drv': %s, "
>> - "'encrypted': %i }",
>> + "'encrypted': %i, "
>> + "'bps': %" PRId64 ","
>> + "'bps_rd': %" PRId64 ","
>> + "'bps_wr': %" PRId64 ","
>> + "'iops': %" PRId64 ","
>> + "'iops_rd': %" PRId64 ","
>> + "'iops_wr': %" PRId64 "}",
>> bs->filename, bs->read_only,
>> bs->drv->format_name,
>> - bdrv_is_encrypted(bs));
>> + bdrv_is_encrypted(bs),
>> + bs->io_limits.bps[2],
>> + bs->io_limits.bps[0],
>> + bs->io_limits.bps[1],
>> + bs->io_limits.iops[2],
>> + bs->io_limits.iops[0],
>> + bs->io_limits.iops[1]);
>> if (bs->backing_file[0] != '\0') {
>> QDict *qdict = qobject_to_qdict(obj);
>> qdict_put(qdict, "backing_file",
>> @@ -2307,7 +2370,7 @@ static bool bdrv_exceed_io_limits(BlockDriverState *bs, int nb_sectors,
>> }
>>
>> /* If a limit was exceeded, immediately queue this request */
>> - if ((bs->req_from_queue == false)
>> + if (!bs->req_from_queue
>> && !QTAILQ_EMPTY(&bs->block_queue->requests)) {
>> if (bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL]
>> || bs->io_limits.bps[is_write] || bs->io_limits.iops[is_write]
>> @@ -2362,14 +2425,14 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
>> trace_bdrv_aio_readv(bs, sector_num, nb_sectors, opaque);
>>
>> if (!drv || bdrv_check_request(bs, sector_num, nb_sectors)) {
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> bs->req_from_queue = false;
>> }
>> return NULL;
>> }
>>
>> /* throttling disk read I/O */
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> if (bdrv_exceed_io_limits(bs, nb_sectors, false, &wait_time)) {
>> ret = qemu_block_queue_enqueue(bs->block_queue, bs, bdrv_aio_readv,
>> sector_num, qiov, nb_sectors, cb, opaque);
>> @@ -2388,14 +2451,14 @@ BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
>> bs->rd_bytes += (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
>> bs->rd_ops ++;
>>
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> bs->io_disps.bytes[BLOCK_IO_LIMIT_READ] +=
>> (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
>> bs->io_disps.ios[BLOCK_IO_LIMIT_READ]++;
>> }
>> }
>>
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> bs->req_from_queue = false;
>> }
>>
>> @@ -2451,7 +2514,7 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>>
>> if (!drv || bs->read_only
>> || bdrv_check_request(bs, sector_num, nb_sectors)) {
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> bs->req_from_queue = false;
>> }
>>
>> @@ -2466,7 +2529,7 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>> }
>>
>> /* throttling disk write I/O */
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> if (bdrv_exceed_io_limits(bs, nb_sectors, true, &wait_time)) {
>> ret = qemu_block_queue_enqueue(bs->block_queue, bs, bdrv_aio_writev,
>> sector_num, qiov, nb_sectors, cb, opaque);
>> @@ -2488,14 +2551,14 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>> bs->wr_highest_sector = sector_num + nb_sectors - 1;
>> }
>>
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> bs->io_disps.bytes[BLOCK_IO_LIMIT_WRITE] +=
>> (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
>> bs->io_disps.ios[BLOCK_IO_LIMIT_WRITE]++;
>> }
>> }
>>
>> - if (bdrv_io_limits_enable(&bs->io_limits)) {
>> + if (bs->io_limits_enabled) {
>> bs->req_from_queue = false;
>> }
>>
>> diff --git a/block.h b/block.h
>> index f0dac62..97d2177 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -57,6 +57,11 @@ void bdrv_info(Monitor *mon, QObject **ret_data);
>> void bdrv_stats_print(Monitor *mon, const QObject *data);
>> void bdrv_info_stats(Monitor *mon, QObject **ret_data);
>>
>> +/* disk I/O throttling */
>> +void bdrv_io_limits_res_init(BlockDriverState *bs);
>> +void bdrv_io_limits_res_deinit(BlockDriverState *bs);
>> +bool bdrv_io_limits_enabled(BlockDriverState *bs);
>> +
>> void bdrv_init(void);
>> void bdrv_init_with_whitelist(void);
>> BlockDriver *bdrv_find_protocol(const char *filename);
>> diff --git a/block_int.h b/block_int.h
>> index 1ca826b..7c96094 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -199,6 +199,8 @@ struct BlockDriverState {
>> BlockIODisp io_disps;
>> BlockQueue *block_queue;
>> QEMUTimer *block_timer;
>> + bool io_limits_enabled;
>> + bool io_unlimit;
>> bool req_from_queue;
>>
>> /* I/O stats (display with "info blockstats"). */
>> diff --git a/blockdev.c b/blockdev.c
>> index aff6bb2..dc2e8a9 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -376,7 +376,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>> return NULL;
>> }
>>
>> - /* disk io limits */
>> + /* disk io throttling */
>> iol_flag = qemu_opt_io_limits_enable_flag(opts, iol_opts);
>> if (iol_flag) {
>> memset(&io_limits, 0, sizeof(BlockIOLimit));
>> @@ -387,6 +387,14 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>> io_limits.iops[2] = qemu_opt_get_number(opts, "iops", 0);
>> io_limits.iops[0] = qemu_opt_get_number(opts, "iops_rd", 0);
>> io_limits.iops[1] = qemu_opt_get_number(opts, "iops_wr", 0);
>> +
>> + if (((io_limits.bps[2] != 0)
>> + && ((io_limits.bps[0] != 0) || (io_limits.bps[1] != 0)))
>> + || ((io_limits.iops[2] != 0)
>> + && ((io_limits.iops[0] != 0) || (io_limits.iops[1] != 0)))) {
>
> Please define constants for read/write/total instead of using magic numbers.
They have been defined. OK. let me replace those magic numbers with them.
>
>> + error_report("Some params for io throttling can not coexist");
>
> This error message is vague.
>
> "bps and bps_rd/bps_wr cannot be used at the same time"
>
>> + return NULL;
>> + }
>> }
>>
>> on_write_error = BLOCK_ERR_STOP_ENOSPC;
>> @@ -765,6 +773,82 @@ int do_change_block(Monitor *mon, const char *device,
>> return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
>> }
>>
>> +/* throttling disk I/O limits */
>> +int do_block_set_io_throttle(Monitor *mon,
>> + const QDict *qdict, QObject **ret_data)
>> +{
>> + const char *devname = qdict_get_str(qdict, "device");
>> + uint64_t bps = qdict_get_try_int(qdict, "bps", -1);
>> + uint64_t bps_rd = qdict_get_try_int(qdict, "bps_rd", -1);
>> + uint64_t bps_wr = qdict_get_try_int(qdict, "bps_wr", -1);
>> + uint64_t iops = qdict_get_try_int(qdict, "iops", -1);
>> + uint64_t iops_rd = qdict_get_try_int(qdict, "iops_rd", -1);
>> + uint64_t iops_wr = qdict_get_try_int(qdict, "iops_wr", -1);
>> + BlockDriverState *bs;
>> +
>> + bs = bdrv_find(devname);
>> + if (!bs) {
>> + qerror_report(QERR_DEVICE_NOT_FOUND, devname);
>> + return -1;
>> + }
>> +
>> + if (devname && (bps == -1) && (bps_rd == -1) && (bps_wr == -1)
>> + && (iops == -1) && (iops_rd == -1) && (iops_wr == -1)) {
>
> No need for the devname check. It must be non-NULL since it is a
> mandatory argument, also bdrv_find() would have crashed since devname
> is used unchecked.
OK. i will remove devname.
>
>> + qerror_report(QERR_IO_THROTTLE_NO_PARAM);
>> + return -1;
>> + }
>> +
>> + if (((bps != -1) && ((bps_rd != -1) || (bps_wr != -1)))
>> + || ((iops != -1) && ((iops_rd != -1) || (iops_wr != -1)))) {
>> + qerror_report(QERR_IO_THROTTLE_PARAM_ERROR);
>> + return -1;
>> + }
>> +
>> + if (bps != -1) {
>> + bs->io_limits.bps[2] = bps;
>> + bs->io_limits.bps[0] = 0;
>> + bs->io_limits.bps[1] = 0;
>> + }
>> +
>> + if ((bps_rd != -1) || (bps_wr != -1)) {
>> + bs->io_limits.bps[0] = (bps_rd == -1) ? bs->io_limits.bps[0] : bps_rd;
>> + bs->io_limits.bps[1] = (bps_wr == -1) ? bs->io_limits.bps[1] : bps_wr;
>> + bs->io_limits.bps[2] = 0;
>> + }
>> +
>> + if (iops != -1) {
>> + bs->io_limits.iops[2] = iops;
>> + bs->io_limits.iops[0] = 0;
>> + bs->io_limits.iops[1] = 0;
>> + }
>> +
>> + if ((iops_rd != -1) || (iops_wr != -1)) {
>> + bs->io_limits.iops[0] =
>> + (iops_rd == -1) ? bs->io_limits.iops[0] : iops_rd;
>> + bs->io_limits.iops[1] =
>> + (iops_wr == -1) ? bs->io_limits.iops[1] : iops_wr;
>> + bs->io_limits.iops[2] = 0;
>> + }
>> +
>> + if (bdrv_io_limits_enabled(bs)) {
>> + if (!bs->io_limits_enabled) {
>> + bdrv_io_limits_res_init(bs);
>> + }
>> + } else {
>> + if (bs->io_limits_enabled) {
>> + BlockQueue *queue = bs->block_queue;
>> + if (!QTAILQ_EMPTY(&queue->requests)) {
>> + bs->io_unlimit = true;
>> + bs->io_limits_enabled = false;
>> + } else {
>> + bdrv_io_limits_res_deinit(bs);
>> + }
>> + }
>> + }
>
> bdrv_io_limits_res_init() and bdrv_io_limits_res_deinit() are
> basically enable/disable functions. I would change this to:
>
> if (!bs->io_limits_enabled && bdrv_io_limits_enabled(bs)) {
> bdrv_io_limits_enable(bs);
> } else if (bs->io_limits_enabled && !bdrv_io_limits_enabled(bs)) {
> bdrv_io_limits_disable(bs);
> }
Good point.
>
> And then remove the bs->io_unlimit field by immediately stopping I/O throttling:
>
> void bdrv_io_limits_disable(BlockDriverState *bs)
> {
> bs->io_limits_enabled = false;
> bs->req_from_queue = false;
>
> if (bs->block_queue) {
> qemu_block_queue_flush(bs->block_queue); /* <--- dispatch
> queued requests */
> qemu_del_block_queue(bs->block_queue);
> }
>
> if (bs->block_timer) {
> qemu_del_timer(bs->block_timer);
> qemu_free_timer(bs->block_timer);
> }
>
> bs->slice_start[0] = 0;
> bs->slice_start[1] = 0;
>
> bs->slice_end[0] = 0;
> bs->slice_end[1] = 0;
> }
>
> The queue flushing code from bdrv_block_timer() could be moved into
> qemu_block_queue_flush(). bdrv_block_timer() would call that function
> instead of looping over the queue itself.
OK.
>
>> +
>> + return 0;
>> +}
>> +
>> int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>> {
>> const char *id = qdict_get_str(qdict, "id");
>> diff --git a/blockdev.h b/blockdev.h
>> index 0a5144c..d0d0d77 100644
>> --- a/blockdev.h
>> +++ b/blockdev.h
>> @@ -63,6 +63,8 @@ int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
>> int do_change_block(Monitor *mon, const char *device,
>> const char *filename, const char *fmt);
>> int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>> +int do_block_set_io_throttle(Monitor *mon,
>> + const QDict *qdict, QObject **ret_data);
>> int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
>> int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index aceba74..3ca496d 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1210,6 +1210,21 @@ ETEXI
>> },
>>
>> STEXI
>> +@item block_set_io_throttle @var{device} @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
>> +@findex block_set_io_throttle
>> +Change I/O throttle limits for a block drive to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
>> +ETEXI
>> +
>> + {
>> + .name = "block_set_io_throttle",
>> + .args_type = "device:B,bps:i?,bps_rd:i?,bps_wr:i?,iops:i?,iops_rd:i?,iops_wr:i?",
>> + .params = "device [bps] [bps_rd] [bps_wr] [iops] [iops_rd] [iops_wr]",
>> + .help = "change I/O throttle limts for a block drive",
>> + .user_print = monitor_user_noop,
>> + .mhandler.cmd_new = do_block_set_io_throttle,
>> + },
>> +
>> +STEXI
>> @item block_passwd @var{device} @var{password}
>> @findex block_passwd
>> Set the encrypted device @var{device} password to @var{password}
>> diff --git a/qerror.c b/qerror.c
>> index d7fcd93..db04df7 100644
>> --- a/qerror.c
>> +++ b/qerror.c
>> @@ -213,6 +213,14 @@ static const QErrorStringTable qerror_table[] = {
>> .error_fmt = QERR_VNC_SERVER_FAILED,
>> .desc = "Could not start VNC server on %(target)",
>> },
>> + {
>> + .error_fmt = QERR_IO_THROTTLE_PARAM_ERROR,
>> + .desc = "Some params for io throttling can not coexist",
>> + },
>> + {
>> + .error_fmt = QERR_IO_THROTTLE_NO_PARAM,
>> + .desc = "No params for io throttling are specified",
>> + },
>> {}
>> };
>>
>> diff --git a/qerror.h b/qerror.h
>> index 16c830d..892abbb 100644
>> --- a/qerror.h
>> +++ b/qerror.h
>> @@ -181,4 +181,10 @@ QError *qobject_to_qerror(const QObject *obj);
>> #define QERR_FEATURE_DISABLED \
>> "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
>>
>> +#define QERR_IO_THROTTLE_PARAM_ERROR \
>> + "{ 'class': 'ParamsCoexistNotPermitted', 'data': {} }"
>
> This error is not specific to I/O throttling. Please add a generic error:
>
> #define QERR_INVALID_PARAMETER_COMBINATION \
> "{ 'class': 'InvalidParameterCombination', 'data': { 'param1': %s,
> 'param2': %s } }"
>
>> +#define QERR_IO_THROTTLE_NO_PARAM \
>> + "{ 'class': 'NoParamsSpecified', 'data': {} }"
>
> QERR_MISSING_PARAMETER already does this.
>
>> +
>> #endif /* QERROR_H */
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 92c5c3a..0aaeae1 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -828,6 +828,44 @@ Example:
>> EQMP
>>
>> {
>> + .name = "block_set_io_throttle",
>> + .args_type = "device:B,bps:i?,bps_rd:i?,bps_wr:i?,iops:i?,iops_rd:i?,iops_wr:i?",
>> + .params = "device [bps[ [bps_rd] [bps_wr] [iops] [iops_rd] [iops_wr]",
>
> [bps]
Good catch.
>
>> + .help = "change I/O throttle limts for a block drive",
>> + .user_print = monitor_user_noop,
>> + .mhandler.cmd_new = do_block_set_io_throttle,
>> + },
>> +
>> +SQMP
>> +block_set_io_throttle
>> +------------
>> +
>> +Change I/O throttle limts for a block drive.
>> +
>> +Arguments:
>> +
>> +- "device": device name (json-string)
>> +- "bps": total throughput value per second(json-int, optional)
>
> Please document the units of bps ("bps" can mean bits per second or
> bytes per second):
>
> "total throughput limit in bytes per second (json-int, optional)"
OK.
>
>> +- "bps_rd": read throughput value per second(json-int, optional)
>> +- "bps_wr": read throughput value per second(json-int, optional)
>
> Stefan
>
Stefan, thanks for your good comments and spent time.
--
Regards,
Zhi Yong Wu
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-08-05 2:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-04 4:34 [PATCH v1] qmp/hmp: add block_set_io_throttle and enhance query_block Zhi Yong Wu
2011-08-04 4:34 ` [Qemu-devel] " Zhi Yong Wu
2011-08-04 13:07 ` Stefan Hajnoczi
2011-08-04 13:07 ` Stefan Hajnoczi
2011-08-05 2:03 ` Zhi Yong Wu
2011-08-05 2:03 ` Zhi Yong Wu
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.