[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] virtio-9p: qmp interface to set/query io th
From: |
xiezhide |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] virtio-9p: qmp interface to set/query io throttle for fsdev devices |
Date: |
Tue, 13 Nov 2018 10:22:43 +0000 |
-----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