From: Fam Zheng <famz@redhat.com>
To: "Benoît Canet" <benoit@irqsave.net>
Cc: kwolf@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org,
stefanha@redhat.com, pbonzini@redhat.com
Subject: Re: [Qemu-devel] [PATCH V11 4/5] block: Add support for throttling burst max in QMP and the command line.
Date: Mon, 2 Sep 2013 10:50:32 +0800 [thread overview]
Message-ID: <20130902025032.GA9089@T430s.nay.redhat.com> (raw)
In-Reply-To: <1378053587-12121-5-git-send-email-benoit@irqsave.net>
On Sun, 09/01 18:39, Benoît Canet wrote:
> The max parameter of the leaky bucket throttling algorithm can be used to
> allow the guest to do bursts.
> The max value is a pool of I/O that the guest can use without being throttled
> at all. Throttling is triggered once this pool is empty.
It would be good to putting this description in comment and/or document strings
(e.g. help text) to make it easier for both users and developers to understand
how is this "max" used, because it's hard to fully understand from "total max
in bytes" or "total max in bytes".
>
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> block/qapi.c | 26 ++++++++++++
> blockdev.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++--------
> hmp.c | 32 +++++++++++++--
> qapi-schema.json | 34 +++++++++++++++-
> qemu-options.hx | 4 +-
> qmp-commands.hx | 28 ++++++++++++-
> 6 files changed, 218 insertions(+), 24 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index cac3919..b1edc66 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -232,6 +232,32 @@ void bdrv_query_info(BlockDriverState *bs,
> info->inserted->iops = cfg.buckets[THROTTLE_OPS_TOTAL].avg;
> info->inserted->iops_rd = cfg.buckets[THROTTLE_OPS_READ].avg;
> info->inserted->iops_wr = cfg.buckets[THROTTLE_OPS_WRITE].avg;
> +
> + info->inserted->has_bps_max =
> + cfg.buckets[THROTTLE_BPS_TOTAL].max;
> + info->inserted->bps_max =
> + cfg.buckets[THROTTLE_BPS_TOTAL].max;
> + info->inserted->has_bps_rd_max =
> + cfg.buckets[THROTTLE_BPS_READ].max;
> + info->inserted->bps_rd_max =
> + cfg.buckets[THROTTLE_BPS_READ].max;
> + info->inserted->has_bps_wr_max =
> + cfg.buckets[THROTTLE_BPS_WRITE].max;
> + info->inserted->bps_wr_max =
> + cfg.buckets[THROTTLE_BPS_WRITE].max;
> +
> + info->inserted->has_iops_max =
> + cfg.buckets[THROTTLE_OPS_TOTAL].max;
> + info->inserted->iops_max =
> + cfg.buckets[THROTTLE_OPS_TOTAL].max;
> + info->inserted->has_iops_rd_max =
> + cfg.buckets[THROTTLE_OPS_READ].max;
> + info->inserted->iops_rd_max =
> + cfg.buckets[THROTTLE_OPS_READ].max;
> + info->inserted->has_iops_wr_max =
> + cfg.buckets[THROTTLE_OPS_WRITE].max;
> + info->inserted->iops_wr_max =
> + cfg.buckets[THROTTLE_OPS_WRITE].max;
> }
>
> bs0 = bs;
> diff --git a/blockdev.c b/blockdev.c
> index 66fce9f..dc0637d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -495,13 +495,18 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
> cfg.buckets[THROTTLE_OPS_WRITE].avg =
> qemu_opt_get_number(opts, "throttling.iops-write", 0);
>
> - cfg.buckets[THROTTLE_BPS_TOTAL].max = 0;
> - cfg.buckets[THROTTLE_BPS_READ].max = 0;
> - cfg.buckets[THROTTLE_BPS_WRITE].max = 0;
> -
> - cfg.buckets[THROTTLE_OPS_TOTAL].max = 0;
> - cfg.buckets[THROTTLE_OPS_READ].max = 0;
> - cfg.buckets[THROTTLE_OPS_WRITE].max = 0;
> + cfg.buckets[THROTTLE_BPS_TOTAL].max =
> + qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
> + cfg.buckets[THROTTLE_BPS_READ].max =
> + qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
> + cfg.buckets[THROTTLE_BPS_WRITE].max =
> + qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
> + cfg.buckets[THROTTLE_OPS_TOTAL].max =
> + qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
> + cfg.buckets[THROTTLE_OPS_READ].max =
> + qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
> + cfg.buckets[THROTTLE_OPS_WRITE].max =
> + qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
>
> cfg.op_size = 0;
>
> @@ -782,6 +787,14 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> qemu_opt_rename(all_opts, "bps_rd", "throttling.bps-read");
> qemu_opt_rename(all_opts, "bps_wr", "throttling.bps-write");
>
> + qemu_opt_rename(all_opts, "iops_max", "throttling.iops-total-max");
> + qemu_opt_rename(all_opts, "iops_rd_max", "throttling.iops-read-max");
> + qemu_opt_rename(all_opts, "iops_wr_max", "throttling.iops-write-max");
> +
> + qemu_opt_rename(all_opts, "bps_max", "throttling.bps-total-max");
> + qemu_opt_rename(all_opts, "bps_rd_max", "throttling.bps-read-max");
> + qemu_opt_rename(all_opts, "bps_wr_max", "throttling.bps-write-max");
> +
> qemu_opt_rename(all_opts, "readonly", "read-only");
>
> value = qemu_opt_get(all_opts, "cache");
> @@ -1266,8 +1279,22 @@ void qmp_change_blockdev(const char *device, const char *filename,
>
> /* throttling disk I/O limits */
> void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
> - int64_t bps_wr, int64_t iops, int64_t iops_rd,
> - int64_t iops_wr, Error **errp)
> + int64_t bps_wr,
> + int64_t iops,
> + int64_t iops_rd,
> + int64_t iops_wr,
> + bool has_bps_max,
> + int64_t bps_max,
> + bool has_bps_rd_max,
> + int64_t bps_rd_max,
> + bool has_bps_wr_max,
> + int64_t bps_wr_max,
> + bool has_iops_max,
> + int64_t iops_max,
> + bool has_iops_rd_max,
> + int64_t iops_rd_max,
> + bool has_iops_wr_max,
> + int64_t iops_wr_max, Error **errp)
> {
> ThrottleConfig cfg;
> BlockDriverState *bs;
> @@ -1287,13 +1314,24 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
> cfg.buckets[THROTTLE_OPS_READ].avg = iops_rd;
> cfg.buckets[THROTTLE_OPS_WRITE].avg = iops_wr;
>
> - cfg.buckets[THROTTLE_BPS_TOTAL].max = 0;
> - cfg.buckets[THROTTLE_BPS_READ].max = 0;
> - cfg.buckets[THROTTLE_BPS_WRITE].max = 0;
> -
> - cfg.buckets[THROTTLE_OPS_TOTAL].max = 0;
> - cfg.buckets[THROTTLE_OPS_READ].max = 0;
> - cfg.buckets[THROTTLE_OPS_WRITE].max = 0;
> + if (has_bps_max) {
> + cfg.buckets[THROTTLE_BPS_TOTAL].max = bps_max;
> + }
> + if (has_bps_rd_max) {
> + cfg.buckets[THROTTLE_BPS_READ].max = bps_rd_max;
> + }
> + if (has_bps_wr_max) {
> + cfg.buckets[THROTTLE_BPS_WRITE].max = bps_wr_max;
> + }
> + if (has_iops_max) {
> + cfg.buckets[THROTTLE_OPS_TOTAL].max = iops_max;
> + }
> + if (has_iops_rd_max) {
> + cfg.buckets[THROTTLE_OPS_READ].max = iops_rd_max;
> + }
> + if (has_iops_wr_max) {
> + cfg.buckets[THROTTLE_OPS_WRITE].max = iops_wr_max;
> + }
>
> cfg.op_size = 0;
>
> @@ -1997,6 +2035,30 @@ QemuOptsList qemu_common_drive_opts = {
> .type = QEMU_OPT_NUMBER,
> .help = "limit write bytes per second",
> },{
> + .name = "throttling.iops-total-max",
> + .type = QEMU_OPT_NUMBER,
> + .help = "I/O operations burst",
> + },{
> + .name = "throttling.iops-read-max",
> + .type = QEMU_OPT_NUMBER,
> + .help = "I/O operations read burst",
> + },{
> + .name = "throttling.iops-write-max",
> + .type = QEMU_OPT_NUMBER,
> + .help = "I/O operations write burst",
> + },{
> + .name = "throttling.bps-total-max",
> + .type = QEMU_OPT_NUMBER,
> + .help = "total bytes burst",
> + },{
> + .name = "throttling.bps-read-max",
> + .type = QEMU_OPT_NUMBER,
> + .help = "total bytes read burst",
> + },{
> + .name = "throttling.bps-write-max",
> + .type = QEMU_OPT_NUMBER,
> + .help = "total bytes write burst",
> + },{
> .name = "copy-on-read",
> .type = QEMU_OPT_BOOL,
> .help = "copy read data from backing file into image file",
> @@ -2119,6 +2181,30 @@ QemuOptsList qemu_old_drive_opts = {
> .type = QEMU_OPT_NUMBER,
> .help = "limit write bytes per second",
> },{
> + .name = "iops_max",
> + .type = QEMU_OPT_NUMBER,
> + .help = "I/O operations burst",
> + },{
> + .name = "iops_rd_max",
> + .type = QEMU_OPT_NUMBER,
> + .help = "I/O operations read burst",
> + },{
> + .name = "iops_wr_max",
> + .type = QEMU_OPT_NUMBER,
> + .help = "I/O operations write burst",
> + },{
> + .name = "bps_max",
> + .type = QEMU_OPT_NUMBER,
> + .help = "total bytes burst",
> + },{
> + .name = "bps_rd_max",
> + .type = QEMU_OPT_NUMBER,
> + .help = "total bytes read burst",
> + },{
> + .name = "bps_wr_max",
> + .type = QEMU_OPT_NUMBER,
> + .help = "total bytes write burst",
> + },{
> .name = "copy-on-read",
> .type = QEMU_OPT_BOOL,
> .help = "copy read data from backing file into image file",
> diff --git a/hmp.c b/hmp.c
> index fcca6ae..85a6c16 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -344,14 +344,28 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
> {
> monitor_printf(mon, " I/O throttling: bps=%" PRId64
> " bps_rd=%" PRId64 " bps_wr=%" PRId64
> + " bps_max=%" PRId64
> + " bps_rd_max=%" PRId64
> + " bps_wr_max=%" PRId64
> " iops=%" PRId64 " iops_rd=%" PRId64
> - " iops_wr=%" PRId64 "\n",
> + " iops_wr=%" PRId64
> + " iops_max=%" PRId64
> + " iops_rd_max=%" PRId64
> + " iops_wr_max=%" PRId64 "\n",
> info->value->inserted->bps,
> info->value->inserted->bps_rd,
> info->value->inserted->bps_wr,
> + info->value->inserted->bps_max,
> + info->value->inserted->bps_rd_max,
> + info->value->inserted->bps_wr_max,
> info->value->inserted->iops,
> info->value->inserted->iops_rd,
> - info->value->inserted->iops_wr);
> + info->value->inserted->iops_wr,
> + info->value->inserted->iops_max,
> + info->value->inserted->iops_rd_max,
> + info->value->inserted->iops_wr_max);
> + } else {
> + monitor_printf(mon, " [not inserted]");
> }
>
> if (verbose) {
> @@ -1098,7 +1112,19 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
> qdict_get_int(qdict, "bps_wr"),
> qdict_get_int(qdict, "iops"),
> qdict_get_int(qdict, "iops_rd"),
> - qdict_get_int(qdict, "iops_wr"), &err);
> + qdict_get_int(qdict, "iops_wr"),
> + false, /* no burst max via HMP */
> + 0,
> + false,
> + 0,
> + false,
> + 0,
> + false,
> + 0,
> + false,
> + 0,
> + false,
> + 0, &err);
> hmp_handle_error(mon, &err);
> }
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a51f7d2..5e5461e 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -785,6 +785,18 @@
> #
> # @image: the info of image used (since: 1.6)
> #
> +# @bps_max: #optional total max in bytes (Since 1.7)
> +#
> +# @bps_rd_max: #optional read max in bytes (Since 1.7)
> +#
> +# @bps_wr_max: #optional write max in bytes (Since 1.7)
> +#
> +# @iops_max: #optional total I/O operations max (Since 1.7)
> +#
> +# @iops_rd_max: #optional read I/O operations max (Since 1.7)
> +#
> +# @iops_wr_max: #optional write I/O operations max (Since 1.7)
> +#
> # Since: 0.14.0
> #
> # Notes: This interface is only found in @BlockInfo.
> @@ -795,7 +807,10 @@
> 'encrypted': 'bool', 'encryption_key_missing': 'bool',
> 'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
> 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
> - 'image': 'ImageInfo' } }
> + 'image': 'ImageInfo',
> + '*bps_max': 'int', '*bps_rd_max': 'int',
> + '*bps_wr_max': 'int', '*iops_max': 'int',
> + '*iops_rd_max': 'int', '*iops_wr_max': 'int' }}
Please be consistent with brace spaceing: s/}}/} }/
>
> ##
> # @BlockDeviceIoStatus:
> @@ -2174,6 +2189,18 @@
> #
> # @iops_wr: write I/O operations per second
> #
> +# @bps_max: #optional total max in bytes (Since 1.7)
> +#
> +# @bps_rd_max: #optional read max in bytes (Since 1.7)
> +#
> +# @bps_wr_max: #optional write max in bytes (Since 1.7)
> +#
> +# @iops_max: #optional total I/O operations max (Since 1.7)
> +#
> +# @iops_rd_max: #optional read I/O operations max (Since 1.7)
> +#
> +# @iops_wr_max: #optional write I/O operations max (Since 1.7)
> +#
> # Returns: Nothing on success
> # If @device is not a valid block device, DeviceNotFound
> #
> @@ -2181,7 +2208,10 @@
> ##
> { 'command': 'block_set_io_throttle',
> 'data': { 'device': 'str', 'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
> - 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int' } }
> + 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
> + '*bps_max': 'int', '*bps_rd_max': 'int',
> + '*bps_wr_max': 'int', '*iops_max': 'int',
> + '*iops_rd_max': 'int', '*iops_wr_max': 'int' }}
Please be consistent with brace spaceing: s/}}/} }/
>
> ##
> # @block-stream:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index d15338e..8df7f1f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -409,7 +409,9 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
> " [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
> " [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
> " [,readonly=on|off][,copy-on-read=on|off]\n"
> - " [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]][[,iops=i]|[[,iops_rd=r][,iops_wr=w]]\n"
> + " [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]][[,iops=i]|[[,iops_rd=r]\n"
> + " [,iops_wr=w][,bps_max=bm]|[[,bps_rd_max=rm][,bps_wr_max=wm]]]\n"
> + " [[,iops_max=im]|[[,iops_rd_max=irm][,iops_wr_max=iwm]]\n"
The brackets don't match here, let's expand and indent it to see
what's here:
[
[,bps=b]
|
[
[,bps_rd=r]
[,bps_wr=w]
]
]
[
[,iops=i]
|
[
[,iops_rd=r]
[,iops_wr=w]
[,bps_max=bm]
|
[
[,bps_rd_max=rm]
[,bps_wr_max=wm]
]
]
[
[,iops_max=im]
|
[
[,iops_rd_max=irm]
[,iops_wr_max=iwm]
]\n"
Mismatch here, right? And I think you wanted to move bps_max* group to outmost.
> " use 'file' as a drive image\n", QEMU_ARCH_ALL)
> STEXI
> @item -drive @var{option}[,@var{option}[,@var{option}[,...]]]
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index cf93d9b..a10d82b 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1389,7 +1389,7 @@ EQMP
>
> {
> .name = "block_set_io_throttle",
> - .args_type = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l",
> + .args_type = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l,bps_max:l?,bps_rd_max:l?,bps_wr_max:l?,iops_max:l?,iops_rd_max:l?,iops_wr_max:l?",
Too long, wrap this line?
> .mhandler.cmd_new = qmp_marshal_input_block_set_io_throttle,
> },
>
> @@ -1408,6 +1408,12 @@ Arguments:
> - "iops": total I/O operations per second (json-int)
> - "iops_rd": read I/O operations per second (json-int)
> - "iops_wr": write I/O operations per second (json-int)
> +- "bps_max": total max in bytes (json-int)
> +- "bps_rd_max": read max in bytes (json-int)
> +- "bps_wr_max": write max in bytes (json-int)
> +- "iops_max": total I/O operations max (json-int)
> +- "iops_rd_max": read I/O operations max (json-int)
> +- "iops_wr_max": write I/O operations max (json-int)
>
> Example:
>
> @@ -1417,7 +1423,13 @@ Example:
> "bps_wr": 0,
> "iops": 0,
> "iops_rd": 0,
> - "iops_wr": 0 } }
> + "iops_wr": 0,
> + "bps_max": 8000000,
> + "bps_rd_max": 0,
> + "bps_wr_max": 0,
> + "iops_max": 0,
> + "iops_rd_max": 0,
> + "iops_wr_max": 0 } }
OK, good brace spacing.
> <- { "return": {} }
>
> EQMP
> @@ -1758,6 +1770,12 @@ Each json-object contain the following:
> - "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)
> + - "bps_max": total max in bytes (json-int)
> + - "bps_rd_max": read max in bytes (json-int)
> + - "bps_wr_max": write max in bytes (json-int)
> + - "iops_max": total I/O operations max (json-int)
> + - "iops_rd_max": read I/O operations max (json-int)
> + - "iops_wr_max": write I/O operations max (json-int)
> - "image": the detail of the image, it is a json-object containing
> the following:
> - "filename": image file name (json-string)
> @@ -1827,6 +1845,12 @@ Example:
> "iops":1000000,
> "iops_rd":0,
> "iops_wr":0,
> + "bps_max": 8000000,
> + "bps_rd_max": 0,
> + "bps_wr_max": 0,
> + "iops_max": 0,
> + "iops_rd_max": 0,
> + "iops_wr_max": 0,
> "image":{
> "filename":"disks/test.qcow2",
> "format":"qcow2",
> --
> 1.7.10.4
>
>
next prev parent reply other threads:[~2013-09-02 2:50 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-01 16:39 [Qemu-devel] [PATCH V11 0/5] Continuous Leaky Bucket Throttling Benoît Canet
2013-09-01 16:39 ` [Qemu-devel] [PATCH V11 1/5] throttle: Add a new throttling API implementing continuous leaky bucket Benoît Canet
2013-09-02 3:08 ` Fam Zheng
2013-09-02 10:16 ` Benoît Canet
2013-09-02 15:34 ` Paolo Bonzini
2013-09-02 16:06 ` Benoît Canet
2013-09-01 16:39 ` [Qemu-devel] [PATCH V11 2/5] throttle: Add units tests Benoît Canet
2013-09-01 16:39 ` [Qemu-devel] [PATCH V11 3/5] block: Enable the new throttling code in the block layer Benoît Canet
2013-09-02 2:24 ` Fam Zheng
2013-09-02 10:19 ` Benoît Canet
2013-09-01 16:39 ` [Qemu-devel] [PATCH V11 4/5] block: Add support for throttling burst max in QMP and the command line Benoît Canet
2013-09-02 2:50 ` Fam Zheng [this message]
2013-09-02 11:49 ` Benoît Canet
2013-09-01 16:39 ` [Qemu-devel] [PATCH V11 5/5] block: Add iops_size to do the iops accounting for a given io size Benoît Canet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130902025032.GA9089@T430s.nay.redhat.com \
--to=famz@redhat.com \
--cc=benoit@irqsave.net \
--cc=kwolf@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@gmail.com \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).