Re: [Qemu-devel] [PATCH V3 for-1.6 3/5] block: Add support for throttling burst threshold in QMP and the command line.

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2726 bytes --]

On 07/23/2013 10:29 AM, Benoît Canet wrote:
> The thresholds of the leaky bucket algorithm can be used to allow some
> burstiness.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block/qapi.c     |   24 +++++++++++++
>  blockdev.c       |  105 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  hmp.c            |   32 +++++++++++++++--
>  qapi-schema.json |   34 ++++++++++++++++--
>  qemu-options.hx  |    2 +-
>  qmp-commands.hx  |   30 ++++++++++++++--
>  6 files changed, 205 insertions(+), 22 deletions(-)
> 

> @@ -1916,6 +1971,18 @@ QemuOptsList qemu_common_drive_opts = {
>              .type = QEMU_OPT_NUMBER,
>              .help = "limit write operations per second",
>          },{
> +            .name = "iops_threshold",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "total I/O operations threshold",
> +        },{

Kevin's series renamed these to have a dash in the name, and also moved
all the throttling parameters into a sub-struct.  Does it make more
sense to have just '*throttling' with that sub-struct containing 12
parameters, 6 for limits and 6 for thresholds, or would it be better to
have '*throttling' with 6 members for limits, as well as
'*throttling-threshold' with the other 6 members?  Naming-wise,
throttling.bps-total and throttling-threshold.bps-total convey as much
information as throttling.bps-total and throttling.bps-total-threshold.

> +++ b/qapi-schema.json
> @@ -769,6 +769,18 @@
>  #
>  # @image: the info of image used (since: 1.6)
>  #
> +# @bps_threshold: #optional total threshold in bytes (Since 1.6)

As others have stated, it would be worth explaining at the high level
the difference between limit and threshold (ie. why I need two different
parameters), as well as what the threshold defaults to if omitted (which
WILL be the case when driven by older management tools).

> +++ b/qemu-options.hx
> @@ -409,7 +409,7 @@ 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][,iops_wr=w][,bps_threshold=bt]|[[,bps_rd_threshold=rt][,bps_wr_threshold=wt]]][[,iops_threshold=it]|[[,iops_rd_threshold=rt][,iops_wr_threshold=wt]]\n"

Is it worth line-wrapping this, so the help text doesn't pass 80 columns?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]