Re: [Qemu-devel] [PATCH 1/2] virtio-9p: qmp interface to set/query io throttle for fsdev devices

[xiezhide]

-----Original Message-----
From: Eric Blake [mailto:address@hidden 
Sent: 2018年11月13日 0:27
To: xiezhide <address@hidden>; address@hidden
Cc: address@hidden; address@hidden; address@hidden; address@hidden; 
address@hidden
Subject: Re: [Qemu-devel] [PATCH 1/2] virtio-9p: qmp interface to set/query io 
throttle for fsdev devices

On 11/11/18 7:12 PM, xiezhide wrote:
> This patch provide qmp interface to set/query io throttle for fsdev devices.

This patch is titled 1/2, but not threaded to a 0/2 or 2/2 patch. 
Remember that proper threading aids reviewers.

> 
> This patch include following work:
> 1. port Pradeep Jagadeesh's patches, details please review
>   http://lists.gnu.org/archive/html/qemu-devel/2017-10/msg00173.html
> 2. fix two issue:
> (1). qmp set io throttle code dump when qemu start CLI not include io 
> throttle params (2). issue Berto comment at: 
> http://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03150.html
> 3. resolve back-compat issue Eric comment at: 
> http://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03149.html
> 
> Signed-off-by: x00390961 
> <address@hidden<mailto:address@hidden>>
> ---
> Makefile                                             |  20 +++-
> Makefile.objs                                    |   8 ++
> block/throttle.c                                |   6 +-

Indentation is really weird here. Are you using 'git send-email' 
directly, or did you do some copy-and-paste that corrupted whitespace?

---- with outlook and copy and paste,  fix it in v2

> util/throttle.c                                      | 224 
> ++++++++++++++++++++++++++--------------
> 22 files changed, 640 insertions(+), 354 deletions(-) create mode 
> 100644 qapi/fsdev.json create mode 100644 qapi/tlimits.json

That's a rather big patch to review - you're refactoring existing code AND 
adding new code in the same patch. Can you break it into smaller pieces, to aid 
reviewers?

> +++ b/Makefile
> @@ -94,6 +94,7 @@ GENERATED_FILES += qapi/qapi-types-block-core.h 
> qapi/qapi-types-block-core.c GENERATED_FILES += 
> qapi/qapi-types-block.h qapi/qapi-types-block.c GENERATED_FILES += 
> qapi/qapi-types-char.h qapi/qapi-types-char.c GENERATED_FILES += 
> qapi/qapi-types-common.h qapi/qapi-types-common.c
> +GENERATED_FILES += qapi/qapi-types-tlimits.h 
> +qapi/qapi-types-tlimits.c

Corrupt patch - where's the leading blank on unchanged context lines?


> +++ b/qapi/block-core.json
> @@ -8,6 +8,7 @@
> { 'include': 'crypto.json' }
> { 'include': 'job.json' }
> { 'include': 'sockets.json' }
> +{ 'include': 'tlimits.json' }
> 
>   ##
> # @SnapshotInfo:
> @@ -2150,130 +2151,13 @@
> #
> # @id: The name or QOM path of the guest device (since: 2.8) #

> -# @iops_size: an I/O size in bytes (Since 1.7) -# # @group: throttle 
> group name (Since 2.4) # # Since: 1.1 ## { 'struct': 
> 'BlockIOThrottle',
> -  'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int',
> -            'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 
> 'int',
> -            '*bps_max': 'int', '*bps_rd_max': 'int',
> -            '*bps_wr_max': 'int', '*iops_max': 'int',
> -            '*iops_rd_max': 'int', '*iops_wr_max': 'int',
> -            '*bps_max_length': 'int', '*bps_rd_max_length': 'int',
> -            '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
> -            '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
> -            '*iops_size': 'int', '*group': 'str' } }
> -
> -##
> -# @ThrottleLimits:

> -#
> -# Since: 2.11
> -##
> -{ 'struct': 'ThrottleLimits',
> -  'data': { '*iops-total' : 'int', '*iops-total-max' : 'int',
> -            '*iops-total-max-length' : 'int', '*iops-read' : 'int',
> -            '*iops-read-max' : 'int', '*iops-read-max-length' : 'int',
> -            '*iops-write' : 'int', '*iops-write-max' : 'int',
> -            '*iops-write-max-length' : 'int', '*bps-total' : 'int',
> -            '*bps-total-max' : 'int', '*bps-total-max-length' : 'int',
> -            '*bps-read' : 'int', '*bps-read-max' : 'int',
> -            '*bps-read-max-length' : 'int', '*bps-write' : 'int',
> -            '*bps-write-max' : 'int', '*bps-write-max-length' : 'int',
> -            '*iops-size' : 'int' } }
> +  'base': 'ThrottleLimits',
> +  'data': { '*device': 'str', '*id': 'str', '*group': 'str' } }

Okay, so it looks like you are moving throttle limits to a new file?

-----Yes, so that it can be reused by fsdev

> 
>   ##
> # @block-stream:
> diff --git a/qapi/fsdev.json b/qapi/fsdev.json new file mode 100644 
> index 0000000..2df8ec1
> --- /dev/null
> +++ b/qapi/fsdev.json
> @@ -0,0 +1,96 @@
> +# -*- Mode: Python -*-
> +
> +##
> +# == QAPI fsdev definitions
> +##
> +
> +{ 'include': 'tlimits.json' }
> +
> +##
> +# @FsdevIOThrottle:
> +#
> +# A set of parameters describing fsdev throttling.
> +#
> +# @id: device id
> +#
> +# Since: 2.11

Is 2.11 the correct release? Perhaps so, if this struct is being reused by 
existing code, and you just refactored to move it to a new file.


> +##
> +{ 'struct': 'FsdevIOThrottle',
> +  'base': 'ThrottleLimits',
> +  'data': { '*id': 'str' } }
> +
> +##
> +# @fsdev-set-io-throttle:
> +#
> +# Change I/O limits for a 9p/fsdev device.
> +#
> +# I/O limits can be enabled by setting throttle value to non-zero number.
> +#
> +# I/O limits can be disabled by setting all throttle values to 0.
> +#
> +# Returns: Nothing on success
> +#          If @device is not a valid fsdev device, GenericError
> +#
> +# Since: 2.11

However, isn't this a new command?  If that's the case, you definitely 
don't want 2.11 here; you've missed 3.1, and the next release will be 
4.0 (although there are still threads mentioning 3.2).

-----it's a new file and change version as you suggestion 4.0

> +++ b/qapi/tlimits.json
> @@ -0,0 +1,89 @@
> +# -*- Mode: Python -*-
> +
> +##
> +# == Throttle limits (VM unrelated)

What does 'VM unrelated' mean?
-----it was moved from block-core.json, will remove it in v2

Thanks
Kidd