Re: [Qemu-devel] [PATCH 1/2 v1] fsdev: QMP interface for throttling

[Pradeep Jagadeesh]

On 3/23/2017 3:32 PM, Eric Blake wrote:
> On 03/23/2017 07:20 AM, Pradeep Jagadeesh wrote:
> > This patchset enables qmp interfaces for the 9pfs
> > devices (fsdev). This provides two interfaces one
>
> s/interfaces/interfaces,/
>
> Also, I just noticed you are using trailing spaces in your commit
> message (not fatal, but unusual).
Will fix.
> > for querying all the 9pfs devices info. The second one
> > to set the IO limits for the required 9pfs device.
>
> When sending a multi-patch series, please remember to include the 0/2
> cover letter ('git config format.coverletter auto' can help here).
OK, I will have a look.
> > Signed-off-by: Pradeep Jagadeesh <address@hidden>
> > ---
> >  Makefile                    |  5 ++-
> >  fsdev/qemu-fsdev-throttle.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
> >  fsdev/qemu-fsdev-throttle.h | 11 ++++++
> >  fsdev/qemu-fsdev.c          | 38 +++++++++++++++++-
> >  fsdev/qemu-fsdev.h          |  2 +-
> >  hmp-commands-info.hx        | 18 +++++++++
> >  hmp-commands.hx             | 18 +++++++++
> >  hmp.c                       | 74 ++++++++++++++++++++++++++++++++++
> >  hmp.h                       |  5 +++
> >  qapi-schema.json            |  6 +++
> >  qapi/fsdev.json             | 87 ++++++++++++++++++++++++++++++++++++++++
> >  qapi/iothrottle.json        | 93 +++++++++++++++++++++++++++++++++++++++++++
> >  qmp.c                       | 14 +++++++
> >  13 files changed, 464 insertions(+), 3 deletions(-)
>
> That's a big patch to review in one go. Does it make sense to break it
> down into smaller chunks?
>
> >  create mode 100644 qapi/fsdev.json
> >  create mode 100644 qapi/iothrottle.json
> >
> > v0 -> v1:
> >
> >    Addressed the comments by Erik Blake, Greg Kurz and Daniel P.Berrange
>
> It's Eric, but you're not the first (and probably not the last) to get
> it wrong.
:) will remember.
> >    Mainly renaming the functions and removing the redundant code
> >
> > diff --git a/Makefile b/Makefile
> > index 73e0c12..c33b46d 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -413,7 +413,10 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json 
> > $(SRC_PATH)/qapi/common.json \
> >                 $(SRC_PATH)/qapi/block.json $(SRC_PATH)/qapi/block-core.json \
> >                 $(SRC_PATH)/qapi/event.json $(SRC_PATH)/qapi/introspect.json \
> >                 $(SRC_PATH)/qapi/crypto.json $(SRC_PATH)/qapi/rocker.json \
> > -               $(SRC_PATH)/qapi/trace.json
> > +               $(SRC_PATH)/qapi/trace.json  $(SRC_PATH)/qapi/iothrottle.json
>
> Why two spaces?
Will fix it.
> > +++ b/qapi-schema.json
> > @@ -78,9 +78,15 @@
> >  # QAPI crypto definitions
> >  { 'include': 'qapi/crypto.json' }
> >
> > +# QAPI IOThrottle definitions
> > +{ 'include': 'qapi/iothrottle.json' }
> > +
> >  # QAPI block definitions
> >  { 'include': 'qapi/block.json' }
> >
> > +# QAPI fsdev definitions
> > +{ 'include': 'qapi/fsdev.json' }
> > +
>
> Adding two new files definitely feels like something that could be
> split. Also, is it possible that existing code could take advantage of
> qapi/iothrottle.json, rather than it just being additional code?
I will check and remove unnecessary includes.
> >  # QAPI event definitions
> >  { 'include': 'qapi/event.json' }
> >
> > diff --git a/qapi/fsdev.json b/qapi/fsdev.json
> > new file mode 100644
> > index 0000000..7fb3c25
> > --- /dev/null
> > +++ b/qapi/fsdev.json
> > @@ -0,0 +1,87 @@
> > +# -*- Mode: Python -*-
> > +
> > +##
> > +# == QAPI fsdev definitions
> > +##
> > +
> > +# QAPI common definitions
> > +{ 'include': 'common.json' }
> > +{ 'include': 'iothrottle.json' }
>
> Is this really necessary? Given that the top level already included
> these, I think it serves more as a documentation purpose than something
> you actually need.
Not really, will fix this.
> > +
> > +##
> > +# @fsdev_set_io_throttle:
> > +#
> > +# Change I/O limits for a 9p/fsdev device.
> > +#
> > +# I/O limits can be enabled by setting throttle vaue to non-zero numbers.
>
> s/vaue/value/
ok
> > +#
> > +# I/O limits can be disabled by setting all of them to 0.
> > +#
> > +# Returns: Nothing on success
> > +#          If @device is not a valid fsdev device, DeviceNotFound
> > +#
> > +# Since: 2.10
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "fsdev_set_io_throttle",
>
> Please name this fsdev-set-io-throttle.  New commands should favor the
> use of '-' rather than '_'.
ok
> > +##
> > +# @query-fsdev-io-throttle:
> > +#
> > +# Return a list of information about fsdev device
> > +#
> > +# Returns: @FsdevIOIOThrottle
>
> Typo in the type name, should be merely IOThrottle
ok
> > +#
> > +# Since: 2.10
> > +#
> > +# Example:
> > +#
> > +# -> { "Execute": "query-fsdev-io-throttle" }
>
> > +#
> > +##
> > +{ 'command': 'query-fsdev-io-throttle', 'returns': [ 'IOThrottle' ] }
> > +
> > diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
> > new file mode 100644
> > index 0000000..f7b615d
> > --- /dev/null
> > +++ b/qapi/iothrottle.json
> > @@ -0,0 +1,93 @@
> > +# -*- Mode: Python -*-
> > +
> > +##
> > +# == QAPI IOThrottle definitions
> > +##
> > +
> > +# QAPI common definitions
> > +{ 'include': 'common.json' }
>
> Again, is this necessary?
Removed.
> > +# @iops_size: an I/O size in bytes (Since 1.7)
> > +#
> > +#
> > +# Since: 2.10
>
> It's weird to state that this struct is since 2.10, but its members are
> since 1.7.  Either you should list a 'since' for when the members of the
> struct were first useful (0.14.0), or eliminate all the (since ...) tags
> on the members and keep the overall struct at 2.10.  I'd lean towards
> the former, at least if BlockDeviceInfo is properly refactored to
> inherit from this type.
Some members of this structure have Since, not all of them. So, shall I 
remove since tag from all?

As of now I used as it is from BlockIOThrottle. Not changed any since tags.

OK, I will try to refactor the BlockDeviceInfo using this structure.
> > +##
> > +{ 'struct': 'IOThrottle',
> > +  'data': { '*device': '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' } }
>
> So yeah, you definitely need to split this patch.  Your FIRST patch
> should be the creation of IOThrottle, and fixing BlockDeviceInfo to
> inherit from it; then a later patch would be the introduction of the
> fsdev throttle commands that are additional clients of the new
> IOThrottle type.
OK
>