On Tue, 21 Mar 2017 10:44:32 +0100 Pradeep Jagadeesh <pradeep.jagade...@huawei.com> wrote:
> Hi Eric,
>
> Thanks for having a look at the patch. My answers are inline.
>
> > 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 '.'
> OK
> >
> >> 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.jagade...@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 meant that, the qemu cli io throttle facility for 9p/fsdev is enabled
> in 2.9.But the qmp interfaces are from 2.10.
QMP users don't care about the cli API. The only important thing is:
Since: 2.10
The curious will find out about the background in git log, no need to
mention this in the *code*.
> >
> >> +#
> >> +# 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).
> OK, mentioned above.
> >
> >> +#
> >> +# 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.
> OK, I will change.
>
> >> +##
> >> +# @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.
> Hmm..copied the block devices code, I will correct it.
> I thought of reusing the code, but the whole struct from block devices
> can not be used, as there is one member called "group" that is not used
> in case of 9p. Also this needs lot of changes even in case of block
> devices. Because I may need to rename the structure as IOThrottle or
> something like that.
> Shall I reuse the code and avoid setting the group member in case of 9p?
> What do you think?
The code factoring would affect:
- hmp_9pfs_set_io_throttle() which looks identical to the existing
hmp_block_set_io_throttle() function
- a bunch of lines to handle the throttle arguments in fsdev_set_io_throttle()
which are the same as in qmp_block_set_io_throttle(). And BTW, a similar
refactoring seems doable in the cli API between fsdev_throttle_parse_opts()
and extract_common_blockdev_options().
This being said, I don't know what this would mean with json files.
> >
> >
> >> +#
> >> +# 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?
> >
> Explained in above comment.
> >> +
> >> +##
> >> +# @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.
> I will fix this.
> >
> >> +# "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.
> Will fix
> >
> >> +# ]
> >> +# }
> >> +#
> >> +##
> >> +{ 'command': 'query-9pfs-io-throttle', 'returns': [ 'FS9PIOThrottle' ] }
> >> +
.git/rebase-apply/patch:631: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
> >>
> >
>
pgp2EwNo2llCl.pgp
Description: OpenPGP digital signature
