Re: [Qemu-devel] [PATCH v0] fsdev: QMP interface for throttling

[Eric Blake <eblake@redhat.com>]

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

On 03/20/2017 08:07 AM, Pradeep Jagadeesh wrote:
> This patchset enables qmp interfaces for the 9pfs 
> devices (fsdev).This provides two interfaces one 

Space between English sentences, after '.'

> for querying all the 9pfs devices info. The second one
> to set the IO limits for the required 9pfs device.
> 
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> ---

> +++ b/qapi-schema.json
> @@ -81,6 +81,9 @@
>  # QAPI block definitions
>  { 'include': 'qapi/block.json' }
>  
> +# QAPI 9pfs definitions
> +{ 'include': 'qapi/9pfs.json' }
> +
>  # QAPI event definitions
>  { 'include': 'qapi/event.json' }
>  
> diff --git a/qapi/9pfs.json b/qapi/9pfs.json
> new file mode 100644
> index 0000000..c068474
> --- /dev/null
> +++ b/qapi/9pfs.json
> @@ -0,0 +1,169 @@
> +# -*- Mode: Python -*-
> +
> +##
> +# == QAPI 9p definitions
> +##
> +
> +# QAPI common definitions
> +{ 'include': 'common.json' }
> +
> +##
> +# @fs9p_set_io_throttle:
> +#
> +# Change I/O limits for a 9p/fsdev device.
> +#
> +# Since QEMU 2.9, I/0 limits can be enabled on each  fsdev(9pfs) device

This says 2.9...

> +#
> +# I/O limits can be disabled by setting all of them to 0.
> +#
> +# Returns: Nothing on success
> +#          If @device is not a valid 9p device, DeviceNotFound
> +#
> +# Since: 2:10

...but this says 2:10 (typo, should be 2.10).  No need to mention the
version twice, especially if one of them is wrong (keep the Since: line).

> +#
> +# Example:
> +#
> +# -> { "execute": "fs9p_set_io_throttle",
> +#      "arguments": { "device": "ide0-1-0",
> +#                     "bps": 1000000,
> +#                     "bps_rd": 0,
> +#                     "bps_wr": 0,
> +#                     "iops": 0,
> +#                     "iops_rd": 0,
> +#                     "iops_wr": 0,
> +#                     "bps_max": 8000000,
> +#                     "bps_rd_max": 0,
> +#                     "bps_wr_max": 0,
> +#                     "iops_max": 0,
> +#                     "iops_rd_max": 0,
> +#                     "iops_wr_max": 0,
> +#                     "bps_max_length": 60,
> +#                     "iops_size": 0 } }
> +# <- { "returns": {} }
> +##
> +{ 'command': 'fs9p_set_io_throttle', 'boxed': true,
> +  'data': 'FS9PIOThrottle' }

New commands and members should be named with '-' rather than '_' as the
word separator, so this should be 'fs9p-set-io-throttle', 'bps-rd', etc.

> +##
> +# @FS9PIOThrottle:
> +#
> +# A set of parameters describing block
> +#
> +# @device: Block device name
> +#
> +# @bps: total throughput limit in bytes per second
> +#
> +# @bps_rd: read throughput limit in bytes per second
> +#
> +# @bps_wr: write throughput limit in bytes per second
> +#
> +# @iops: total I/O operations per second
> +#
> +# @iops_rd: read I/O operations per second
> +#
> +# @iops_wr: write I/O operations per second
> +#
> +# @bps_max: total throughput limit during bursts,
> +#                     in bytes (Since 1.7)

You're introducing this struct in 2.10, so this member is not since 1.7.
 Either that, or you're copying-and-pasting when you should be sharing
code and reusing an existing struct.


> +#
> +# Since: 2.10
> +##
> +{ 'struct': 'FS9PIOThrottle',
> +  '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' } }

If you reuse an existing struct that uses _ instead of -, then that
explains your naming.  But in that case, why do you need to declare a
new (copied) struct, instead of just reusing the existing one?

> +
> +##
> +# @query-9pfs-io-throttle:
> +#
> +# Return a list of information about each iothread
> +#
> +# Returns: @FS9PIOIOThrottle
> +#
> +# Since: 2.10
> +#
> +# Example:
> +#
> +# -> { "Execute": "query-9pfs-io-throttle" }
> +# <- { "returns" : [
> +#          {
> +#             "device": "ide0-hd0",
> +#              "bps":1000000,
> +#              "bps_rd":0,
> +#              "bps_wr":0,
> +#              "iops":1000000,
> +#              "iops_rd":0,
> +#              "iops_wr":0,
> +#              "bps_max": 8000000,
> +#              "bps_rd_max": 0,
> +#              "bps_wr_max": 0,

You are not consistent on whether to include a space after ':'.  The
easiest way to get this right is to paste actual output from pretty qmp
mode.

> +#              "iops_max": 0,
> +#              "iops_rd_max": 0,
> +#              "iops_wr_max": 0,
> +#              "bps_max_length": 0,
> +#              "bps_rd_max_length": 0,
> +#              "bps_wr_max_length": 0,
> +#              "iops_max_length": 0,
> +#              "iops_rd_max_length": 0,
> +#              "iops_wr_max_length": 0,
> +#              "iops_size": 0,
> +#            }

This is not valid JSON. No trailing commas.

> +#          ]
> +#      }
> +#
> +##
> +{ 'command': 'query-9pfs-io-throttle', 'returns': [ 'FS9PIOThrottle' ] }
> +
> 

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