| Message ID | 1506954812-6552-3-git-send-email-pradeep.jagadeesh@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | fsdev: qmp interface for io throttling | expand |
On Mon 02 Oct 2017 04:33:28 PM CEST, Pradeep Jagadeesh wrote: > This patch factors out code to use the ThrottleLimits > structure. > { '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' } } > + 'base': 'ThrottleLimits', > + 'data': { '*device': 'str', '*id': 'str', '*group': 'str' } } So BlockIOThrottle used to have parameters named bps_rd and iops_wr, and after this patch they become bps-read and iops-write. This breaks the API completely, as you can see if you run e.g. iotest 129: AssertionError: failed path traversal for "return" in "{u'error': {u'class': u'GenericError', u'desc': u"Parameter 'iops_rd' is unexpected"}}" I just checked previous versions of the series and I see that Manos already warned you of this in v11: https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg04698.html Berto
[adding Markus, and block list] On 10/13/2017 09:16 AM, Alberto Garcia wrote: > On Mon 02 Oct 2017 04:33:28 PM CEST, Pradeep Jagadeesh wrote: >> This patch factors out code to use the ThrottleLimits >> structure. > >> { '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' } } >> + 'base': 'ThrottleLimits', >> + 'data': { '*device': 'str', '*id': 'str', '*group': 'str' } } > > So BlockIOThrottle used to have parameters named bps_rd and iops_wr, and > after this patch they become bps-read and iops-write. This breaks the > API completely, as you can see if you run e.g. iotest 129: > > AssertionError: failed path traversal for "return" in "{u'error': {u'class': u'GenericError', u'desc': u"Parameter 'iops_rd' is unexpected"}}" > > I just checked previous versions of the series and I see that Manos > already warned you of this in v11: > > https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg04698.html On the bright side, ThrottleLimits is marked 'since 2.11' (added in commit 432d889e), meaning it has not yet been released, so we CAN fix the naming in ThrottleLimits to be compatible with BlockIOThrottle if we want to share the type, as long as we get it done before the 2.11 release. It does mean tweaking Manos' code to use compatible names everywhere, but that may be a wise course of action (we tend to favor '-' in new API names unless there is a strong reason to use '_'; but sharing code for maximum back-compat would be a reason to use '_').
On 10/13/2017 4:26 PM, Eric Blake wrote: > [adding Markus, and block list] > > On 10/13/2017 09:16 AM, Alberto Garcia wrote: >> On Mon 02 Oct 2017 04:33:28 PM CEST, Pradeep Jagadeesh wrote: >>> This patch factors out code to use the ThrottleLimits >>> structure. >> >>> { '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' } } >>> + 'base': 'ThrottleLimits', >>> + 'data': { '*device': 'str', '*id': 'str', '*group': 'str' } } >> >> So BlockIOThrottle used to have parameters named bps_rd and iops_wr, and >> after this patch they become bps-read and iops-write. This breaks the >> API completely, as you can see if you run e.g. iotest 129: >> >> AssertionError: failed path traversal for "return" in "{u'error': {u'class': u'GenericError', u'desc': u"Parameter 'iops_rd' is unexpected"}}" >> >> I just checked previous versions of the series and I see that Manos >> already warned you of this in v11: >> >> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg04698.html > > On the bright side, ThrottleLimits is marked 'since 2.11' (added in > commit 432d889e), meaning it has not yet been released, so we CAN fix > the naming in ThrottleLimits to be compatible with BlockIOThrottle if we > want to share the type, as long as we get it done before the 2.11 > release. It does mean tweaking Manos' code to use compatible names > everywhere, but that may be a wise course of action (we tend to favor > '-' in new API names unless there is a strong reason to use '_'; but > sharing code for maximum back-compat would be a reason to use '_'). Sorry for the late reply. I was out of office. I am ready to change them to "_" instead of "-". Need to ask Manos. @Manos, what do you say about the above comment. That makes sense. It will help to reuse lot of code. Shall we rename the parameters with "_" instead of "-"? Regards, Pradeep >
On Fri, Oct 13, 2017 at 09:26:17AM -0500, Eric Blake wrote: >[adding Markus, and block list] > >On 10/13/2017 09:16 AM, Alberto Garcia wrote: >> On Mon 02 Oct 2017 04:33:28 PM CEST, Pradeep Jagadeesh wrote: >>> This patch factors out code to use the ThrottleLimits >>> structure. >> >>> { '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' } } >>> + 'base': 'ThrottleLimits', >>> + 'data': { '*device': 'str', '*id': 'str', '*group': 'str' } } >> >> So BlockIOThrottle used to have parameters named bps_rd and iops_wr, and >> after this patch they become bps-read and iops-write. This breaks the >> API completely, as you can see if you run e.g. iotest 129: >> >> AssertionError: failed path traversal for "return" in "{u'error': {u'class': u'GenericError', u'desc': u"Parameter 'iops_rd' is unexpected"}}" >> >> I just checked previous versions of the series and I see that Manos >> already warned you of this in v11: >> >> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg04698.html > >On the bright side, ThrottleLimits is marked 'since 2.11' (added in >commit 432d889e), meaning it has not yet been released, so we CAN fix >the naming in ThrottleLimits to be compatible with BlockIOThrottle if we >want to share the type, as long as we get it done before the 2.11 >release. We decided to keep BlockIOThrottle separate from ThrottleLimits because that would break the old I/O throttling API, just like is done in this patch series. BlockIOThrottle is the one using old naming conventions so I think it should be the one to go, if that has to be done. But this all boils down to whether the legacy throttling API has to break in 2.11 or not, which probably is the maintainer's decision. >It does mean tweaking Manos' code to use compatible names >everywhere, but that may be a wise course of action (we tend to favor >'-' in new API names unless there is a strong reason to use '_'; but >sharing code for maximum back-compat would be a reason to use '_'). > >-- >Eric Blake, Principal Software Engineer >Red Hat, Inc. +1-919-301-3266 >Virtualization: qemu.org | libvirt.org >
On 11/6/2017 10:35 AM, Manos Pitsidianakis wrote: > On Fri, Oct 13, 2017 at 09:26:17AM -0500, Eric Blake wrote: >> [adding Markus, and block list] >> >> On 10/13/2017 09:16 AM, Alberto Garcia wrote: >>> On Mon 02 Oct 2017 04:33:28 PM CEST, Pradeep Jagadeesh wrote: >>>> This patch factors out code to use the ThrottleLimits >>>> structure. >>> >>>> { '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' } } >>>> + 'base': 'ThrottleLimits', >>>> + 'data': { '*device': 'str', '*id': 'str', '*group': 'str' } } >>> >>> So BlockIOThrottle used to have parameters named bps_rd and iops_wr, and >>> after this patch they become bps-read and iops-write. This breaks the >>> API completely, as you can see if you run e.g. iotest 129: >>> >>> AssertionError: failed path traversal for "return" in "{u'error': >>> {u'class': u'GenericError', u'desc': u"Parameter 'iops_rd' is >>> unexpected"}}" >>> >>> I just checked previous versions of the series and I see that Manos >>> already warned you of this in v11: >>> >>> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg04698.html >> >> On the bright side, ThrottleLimits is marked 'since 2.11' (added in >> commit 432d889e), meaning it has not yet been released, so we CAN fix >> the naming in ThrottleLimits to be compatible with BlockIOThrottle if we >> want to share the type, as long as we get it done before the 2.11 >> release. > > We decided to keep BlockIOThrottle separate from ThrottleLimits because > that would break the old I/O throttling API, just like is done in this > patch series. BlockIOThrottle is the one using old naming conventions > so I think it should be the one to go, if that has to be done. > > But this all boils down to whether the legacy throttling API has to > break in 2.11 or not, which probably is the maintainer's decision. > > >> It does mean tweaking Manos' code to use compatible names >> everywhere, but that may be a wise course of action (we tend to favor >> '-' in new API names unless there is a strong reason to use '_'; but >> sharing code for maximum back-compat would be a reason to use '_'). >> Thanks for your reply Manos. @Eric, So shall I go ahead and revert my patches to as before. I mean using iothrottle structure?. What is your suggestion? -Pradeep >> -- >> Eric Blake, Principal Software Engineer >> Red Hat, Inc. +1-919-301-3266 >> Virtualization: qemu.org | libvirt.org >> > > >
On 10/13/2017 4:26 PM, Eric Blake wrote: > [adding Markus, and block list] > > On 10/13/2017 09:16 AM, Alberto Garcia wrote: >> On Mon 02 Oct 2017 04:33:28 PM CEST, Pradeep Jagadeesh wrote: >>> This patch factors out code to use the ThrottleLimits >>> structure. >> >>> { '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' } } >>> + 'base': 'ThrottleLimits', >>> + 'data': { '*device': 'str', '*id': 'str', '*group': 'str' } } >> >> So BlockIOThrottle used to have parameters named bps_rd and iops_wr, and >> after this patch they become bps-read and iops-write. This breaks the >> API completely, as you can see if you run e.g. iotest 129: >> >> AssertionError: failed path traversal for "return" in "{u'error': {u'class': u'GenericError', u'desc': u"Parameter 'iops_rd' is unexpected"}}" >> >> I just checked previous versions of the series and I see that Manos >> already warned you of this in v11: >> >> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg04698.html > > On the bright side, ThrottleLimits is marked 'since 2.11' (added in > commit 432d889e), meaning it has not yet been released, so we CAN fix > the naming in ThrottleLimits to be compatible with BlockIOThrottle if we > want to share the type, as long as we get it done before the 2.11 > release. It does mean tweaking Manos' code to use compatible names > everywhere, but that may be a wise course of action (we tend to favor > '-' in new API names unless there is a strong reason to use '_'; but > sharing code for maximum back-compat would be a reason to use '_'). Could you please have a look at Manos reply and my reply also. Please let me know what you think. -Pradeep >
diff --git a/qapi/block-core.json b/qapi/block-core.json index 750bb0c..f1d2c13 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1824,86 +1824,17 @@ # # A set of parameters describing block throttling. # -# @device: Block device name (deprecated, use @id instead) -# # @id: The name or QOM path of the guest device (since: 2.8) # -# @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) -# -# @bps_rd_max: read throughput limit during bursts, -# in bytes (Since 1.7) -# -# @bps_wr_max: write throughput limit during bursts, -# in bytes (Since 1.7) -# -# @iops_max: total I/O operations per second during bursts, -# in bytes (Since 1.7) -# -# @iops_rd_max: read I/O operations per second during bursts, -# in bytes (Since 1.7) -# -# @iops_wr_max: write I/O operations per second during bursts, -# in bytes (Since 1.7) -# -# @bps_max_length: maximum length of the @bps_max burst -# period, in seconds. It must only -# be set if @bps_max is set as well. -# Defaults to 1. (Since 2.6) -# -# @bps_rd_max_length: maximum length of the @bps_rd_max -# burst period, in seconds. It must only -# be set if @bps_rd_max is set as well. -# Defaults to 1. (Since 2.6) -# -# @bps_wr_max_length: maximum length of the @bps_wr_max -# burst period, in seconds. It must only -# be set if @bps_wr_max is set as well. -# Defaults to 1. (Since 2.6) -# -# @iops_max_length: maximum length of the @iops burst -# period, in seconds. It must only -# be set if @iops_max is set as well. -# Defaults to 1. (Since 2.6) -# -# @iops_rd_max_length: maximum length of the @iops_rd_max -# burst period, in seconds. It must only -# be set if @iops_rd_max is set as well. -# Defaults to 1. (Since 2.6) -# -# @iops_wr_max_length: maximum length of the @iops_wr_max -# burst period, in seconds. It must only -# be set if @iops_wr_max is set as well. -# Defaults to 1. (Since 2.6) -# -# @iops_size: an I/O size in bytes (Since 1.7) +# @device: Block device name (deprecated, use @id instead) # # @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' } } + 'base': 'ThrottleLimits', + 'data': { '*device': 'str', '*id': 'str', '*group': 'str' } } ## # @ThrottleLimits:
This patch factors out code to use the ThrottleLimits structure. Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> --- qapi/block-core.json | 75 +++------------------------------------------------- 1 file changed, 3 insertions(+), 72 deletions(-)