Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support

[Fam Zheng]

On Mon, 03/30 19:19, Alberto Garcia wrote:
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7873084..d8211b7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -990,6 +990,8 @@
>  #
>  # @iops_size: #optional an I/O size in bytes (Since 1.7)
>  #
> +# @group: #optional throttle group name (Since 2.3)

We should probably elaborate (here, and at other places of @group appearances):
@device is used as group name. This is useful since other devices could use
this device name as the group name, for exmple the following -drive definition:

...
-drive file=foo,id=foo,bps=100 \
-drive file=bar,id=foo,bps=200,group=foo \
...

will put both devices into the same group.

Also, as the two bps values don't match, I assume the last one is used? Is
there a warning when ignoring a config from previous definitions?

Thinking about this, I'd slightly prefer a canonical throttle group definition
rather than patching the existing parameters:

-object throttle-group,id=tg0,bps=100,iops=200,iops-max=1000 \
-drive file=foo,id=foo,throttle-group=tg0 \
-drive file=bar,id=bar,throttle-group=tg0 \

and error out if "bps=" etc are specified together with "throttle-group=" in
-drive.

And in QMP, add block_set_io_throttle_group which works together with
object-add.

What do you think?

> +#
>  # Returns: Nothing on success
>  #          If @device is not a valid block device, DeviceNotFound
>  #
> @@ -1001,7 +1003,7 @@
>              '*bps_max': 'int', '*bps_rd_max': 'int',
>              '*bps_wr_max': 'int', '*iops_max': 'int',
>              '*iops_rd_max': 'int', '*iops_wr_max': 'int',
> -            '*iops_size': 'int' } }
> +            '*iops_size': 'int', '*group': 'str' } }
>  
>  ##
>  # @block-stream:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 319d971..7d6b2ec 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -464,6 +464,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
>      "       [[,bps_max=bm]|[[,bps_rd_max=rm][,bps_wr_max=wm]]]\n"
>      "       [[,iops_max=im]|[[,iops_rd_max=irm][,iops_wr_max=iwm]]]\n"
>      "       [[,iops_size=is]]\n"
> +    "       [[,group=g]]\n"
>      "                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 3a42ad0..ce897ff 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1730,7 +1730,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,bps_max:l?,bps_rd_max:l?,bps_wr_max:l?,iops_max:l?,iops_rd_max:l?,iops_wr_max:l?,iops_size: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?,iops_size:l?,group:s?",
>          .mhandler.cmd_new = qmp_marshal_input_block_set_io_throttle,
>      },
>  
> @@ -1756,6 +1756,7 @@ Arguments:
>  - "iops_rd_max":  read I/O operations max (json-int)
>  - "iops_wr_max":  write I/O operations max (json-int)
>  - "iops_size":  I/O size in bytes when limiting (json-int)

Why removing these three?

> +- "group": throttle group name (json-string)
>  
>  Example:
>  
> -- 
> 2.1.4
> 
>