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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6402 bytes --]

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).

> 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).

> 
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> ---
>  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.

>    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?


> +++ 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?

>  # 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.

> +
> +##
> +# @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/

> +#
> +# 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 '_'.


> +##
> +# @query-fsdev-io-throttle:
> +#
> +# Return a list of information about fsdev device
> +#
> +# Returns: @FsdevIOIOThrottle

Typo in the type name, should be merely IOThrottle

> +#
> +# 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?


> +# @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.

> +##
> +{ '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.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]