-----Original Message----- From: Eric Blake [mailto:ebl...@redhat.com] Sent: 2018年11月13日 0:27 To: xiezhide <xiezh...@huawei.com>; qemu-devel@nongnu.org Cc: arm...@redhat.com; gr...@kaod.org; dgilb...@redhat.com; be...@igalia.com; aneesh.ku...@linux.vnet.ibm.com 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 > <xiezh...@huawei.com<mailto:xiezh...@huawei.com>> > --- > 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
