| Message ID | 1505385610-35529-3-git-send-email-pradeep.jagadeesh@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | fsdev: qmp interface for io throttling | expand |
On Thu, 14 Sep 2017 06:40:06 -0400 Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote: > This patch factors out code to use the ThrottleLimits > strurcture. > > Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> > Reviewed-by: Greg Kurz <groug@kaod.org> > Reviewed-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> > Reviewed-by: Markus Armbruster <armbru@redhat.com> > --- > qapi/block-core.json | 78 +++------------------------------------------------- Uhhh... what happened here ? Where did the lines go ? I've never reviewed this patch before... > 1 file changed, 4 insertions(+), 74 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index bb11815..d0ccfda 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1826,84 +1826,13 @@ > # > # @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) > -# > # @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', '*group': 'str' } } > > ## > # @ThrottleLimits: > @@ -1913,6 +1842,7 @@ > # transaction. All fields are optional. When setting limits, if a field is > # missing the current value is not changed. > # > +# @id: device id > # @iops-total: limit total I/O operations per second > # @iops-total-max: I/O operations burst > # @iops-total-max-length: length of the iops-total-max burst period, in seconds > @@ -1942,7 +1872,7 @@ > # Since: 2.11 > ## > { 'struct': 'ThrottleLimits', > - 'data': { '*iops-total' : 'int', '*iops-total-max' : 'int', > + 'data': { '*id' : 'str', '*iops-total' : 'int', '*iops-total-max' : 'int', > '*iops-total-max-length' : 'int', '*iops-read' : 'int', > '*iops-read-max' : 'int', '*iops-read-max-length' : 'int', > '*iops-write' : 'int', '*iops-write-max' : 'int',
On 9/14/2017 1:19 PM, Greg Kurz wrote: > On Thu, 14 Sep 2017 06:40:06 -0400 > Pradeep Jagadeesh <pradeepkiruvale@gmail.com> wrote: > >> This patch factors out code to use the ThrottleLimits >> strurcture. >> >> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> >> Reviewed-by: Greg Kurz <groug@kaod.org> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> Reviewed-by: Alberto Garcia <berto@igalia.com> >> Reviewed-by: Markus Armbruster <armbru@redhat.com> >> --- >> qapi/block-core.json | 78 +++------------------------------------------------- > > Uhhh... what happened here ? Where did the lines go ? I've never reviewed this > patch before... I did rebase the code on 2.10 Now I am using the ThrottleLimits structure that was introduced IN 2.10. This is same as the IOThrottle structure. So, many things got changed in this version. -Pradeep > >> 1 file changed, 4 insertions(+), 74 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index bb11815..d0ccfda 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -1826,84 +1826,13 @@ >> # >> # @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) >> -# >> # @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', '*group': 'str' } } >> >> ## >> # @ThrottleLimits: >> @@ -1913,6 +1842,7 @@ >> # transaction. All fields are optional. When setting limits, if a field is >> # missing the current value is not changed. >> # >> +# @id: device id >> # @iops-total: limit total I/O operations per second >> # @iops-total-max: I/O operations burst >> # @iops-total-max-length: length of the iops-total-max burst period, in seconds >> @@ -1942,7 +1872,7 @@ >> # Since: 2.11 >> ## >> { 'struct': 'ThrottleLimits', >> - 'data': { '*iops-total' : 'int', '*iops-total-max' : 'int', >> + 'data': { '*id' : 'str', '*iops-total' : 'int', '*iops-total-max' : 'int', >> '*iops-total-max-length' : 'int', '*iops-read' : 'int', >> '*iops-read-max' : 'int', '*iops-read-max-length' : 'int', >> '*iops-write' : 'int', '*iops-write-max' : 'int', >
On Thu, Sep 14, 2017 at 06:40:06AM -0400, Pradeep Jagadeesh wrote: >This patch factors out code to use the ThrottleLimits >strurcture. > >Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> >Reviewed-by: Greg Kurz <groug@kaod.org> >Reviewed-by: Eric Blake <eblake@redhat.com> >Reviewed-by: Alberto Garcia <berto@igalia.com> >Reviewed-by: Markus Armbruster <armbru@redhat.com> >--- > qapi/block-core.json | 78 +++------------------------------------------------- > 1 file changed, 4 insertions(+), 74 deletions(-) > >diff --git a/qapi/block-core.json b/qapi/block-core.json >index bb11815..d0ccfda 100644 >--- a/qapi/block-core.json >+++ b/qapi/block-core.json >@@ -1826,84 +1826,13 @@ > # > # @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) >-# > # @group: throttle group name (Since 2.4) BlockIOThrottle and ThrottleLimits are not directly compatible, for example here we have iops_rd_max_length as a name, > # > # 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', '*group': 'str' } } > > ## > # @ThrottleLimits: >@@ -1913,6 +1842,7 @@ > # transaction. All fields are optional. When setting limits, if a field is > # missing the current value is not changed. > # >+# @id: device id > # @iops-total: limit total I/O operations per second > # @iops-total-max: I/O operations burst > # @iops-total-max-length: length of the iops-total-max burst period, in seconds >@@ -1942,7 +1872,7 @@ > # Since: 2.11 > ## > { 'struct': 'ThrottleLimits', >- 'data': { '*iops-total' : 'int', '*iops-total-max' : 'int', >+ 'data': { '*id' : 'str', '*iops-total' : 'int', '*iops-total-max' : 'int', > '*iops-total-max-length' : 'int', '*iops-read' : 'int', > '*iops-read-max' : 'int', '*iops-read-max-length' : 'int', > '*iops-write' : 'int', '*iops-write-max' : 'int', And here it's iops-read-max-length. This breaks the block layer's old throttling API. Do we want the old API replaced for 2.11? I don't know which maintainer is responsible for this, so I CC'd armbru.
On Thu, Sep 14, 2017 at 06:40:06AM -0400, Pradeep Jagadeesh wrote: >This patch factors out code to use the ThrottleLimits >strurcture. > >Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> >Reviewed-by: Greg Kurz <groug@kaod.org> >Reviewed-by: Eric Blake <eblake@redhat.com> >Reviewed-by: Alberto Garcia <berto@igalia.com> >Reviewed-by: Markus Armbruster <armbru@redhat.com> >--- > qapi/block-core.json | 78 +++------------------------------------------------- > 1 file changed, 4 insertions(+), 74 deletions(-) > >diff --git a/qapi/block-core.json b/qapi/block-core.json >index bb11815..d0ccfda 100644 >--- a/qapi/block-core.json >+++ b/qapi/block-core.json >@@ -1826,84 +1826,13 @@ > # > # @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) >-# > # @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', '*group': 'str' } } > > ## > # @ThrottleLimits: >@@ -1913,6 +1842,7 @@ > # transaction. All fields are optional. When setting limits, if a field is > # missing the current value is not changed. > # >+# @id: device id > # @iops-total: limit total I/O operations per second > # @iops-total-max: I/O operations burst > # @iops-total-max-length: length of the iops-total-max burst period, in seconds >@@ -1942,7 +1872,7 @@ > # Since: 2.11 > ## > { 'struct': 'ThrottleLimits', >- 'data': { '*iops-total' : 'int', '*iops-total-max' : 'int', >+ 'data': { '*id' : 'str', '*iops-total' : 'int', '*iops-total-max' : 'int', > '*iops-total-max-length' : 'int', '*iops-read' : 'int', > '*iops-read-max' : 'int', '*iops-read-max-length' : 'int', > '*iops-write' : 'int', '*iops-write-max' : 'int', >-- >1.8.3.1 > Why is id needed? ThrottleLimits is not per group, not per device. And as I mentioned in another reply, this changes the block_set_io_throttle command which is not simple refactoring.
On 09/14/2017 05:40 AM, Pradeep Jagadeesh wrote: > This patch factors out code to use the ThrottleLimits > strurcture. s/strurcture/structure/ > > Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> > Reviewed-by: Greg Kurz <groug@kaod.org> > Reviewed-by: Eric Blake <eblake@redhat.com> Echoing the sentiments made elsewhere - this is a significant rewrite from the earlier version, so you WANT reviewers to look at it fresh rather than assuming that previous reviews still apply.
On 9/18/2017 6:25 PM, Manos Pitsidianakis wrote: > On Thu, Sep 14, 2017 at 06:40:06AM -0400, Pradeep Jagadeesh wrote: >> This patch factors out code to use the ThrottleLimits >> strurcture. >> >> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> >> Reviewed-by: Greg Kurz <groug@kaod.org> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> Reviewed-by: Alberto Garcia <berto@igalia.com> >> Reviewed-by: Markus Armbruster <armbru@redhat.com> >> --- >> qapi/block-core.json | 78 >> +++------------------------------------------------- >> 1 file changed, 4 insertions(+), 74 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index bb11815..d0ccfda 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -1826,84 +1826,13 @@ >> # >> # @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) >> -# >> # @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', '*group': 'str' } } >> >> ## >> # @ThrottleLimits: >> @@ -1913,6 +1842,7 @@ >> # transaction. All fields are optional. When setting limits, if a >> field is >> # missing the current value is not changed. >> # >> +# @id: device id >> # @iops-total: limit total I/O operations per second >> # @iops-total-max: I/O operations burst >> # @iops-total-max-length: length of the iops-total-max burst period, >> in seconds >> @@ -1942,7 +1872,7 @@ >> # Since: 2.11 >> ## >> { 'struct': 'ThrottleLimits', >> - 'data': { '*iops-total' : 'int', '*iops-total-max' : 'int', >> + 'data': { '*id' : 'str', '*iops-total' : 'int', '*iops-total-max' : >> 'int', >> '*iops-total-max-length' : 'int', '*iops-read' : 'int', >> '*iops-read-max' : 'int', '*iops-read-max-length' : 'int', >> '*iops-write' : 'int', '*iops-write-max' : 'int', >> -- >> 1.8.3.1 >> > > Why is id needed? ThrottleLimits is not per group, not per device. And > as I mentioned in another reply, this changes the block_set_io_throttle > command which is not simple refactoring. When you use qmp/hmp interface the id is used to set/get throttle configuration. So, I thought if id is part of the throttle config, it will be useful. Now I will try to move it to just for fsdeviothrottle. -Pradeep
On 9/18/2017 7:10 PM, Eric Blake wrote: > On 09/14/2017 05:40 AM, Pradeep Jagadeesh wrote: >> This patch factors out code to use the ThrottleLimits >> strurcture. > > s/strurcture/structure/ > >> >> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> >> Reviewed-by: Greg Kurz <groug@kaod.org> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > Echoing the sentiments made elsewhere - this is a significant rewrite > from the earlier version, so you WANT reviewers to look at it fresh > rather than assuming that previous reviews still apply. > OK, but shall I send them as V12 or V01 again? -Pradeep
On 09/19/2017 05:06 AM, Pradeep Jagadeesh wrote: > On 9/18/2017 7:10 PM, Eric Blake wrote: >> On 09/14/2017 05:40 AM, Pradeep Jagadeesh wrote: >>> This patch factors out code to use the ThrottleLimits >>> strurcture. >> >> s/strurcture/structure/ >> >>> >>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> >>> Reviewed-by: Greg Kurz <groug@kaod.org> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >> >> Echoing the sentiments made elsewhere - this is a significant rewrite >> from the earlier version, so you WANT reviewers to look at it fresh >> rather than assuming that previous reviews still apply. >> > OK, but shall I send them as V12 or V01 again? v12 is fine - you really are revising the patches a twelfth time.
On Tue, 19 Sep 2017 12:06:26 +0200 Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> wrote: > On 9/18/2017 7:10 PM, Eric Blake wrote: > > On 09/14/2017 05:40 AM, Pradeep Jagadeesh wrote: > >> This patch factors out code to use the ThrottleLimits > >> strurcture. > > > > s/strurcture/structure/ > > > >> > >> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> > >> Reviewed-by: Greg Kurz <groug@kaod.org> > >> Reviewed-by: Eric Blake <eblake@redhat.com> > > > > Echoing the sentiments made elsewhere - this is a significant rewrite > > from the earlier version, so you WANT reviewers to look at it fresh > > rather than assuming that previous reviews still apply. > > > OK, but shall I send them as V12 or V01 again? > The version is rather tied to the whole patchset than to each individual patch. The next round will be the 12th, so all patches should have v12 in the Subject. > -Pradeep >
On 9/18/2017 6:04 PM, Manos Pitsidianakis wrote: > On Thu, Sep 14, 2017 at 06:40:06AM -0400, Pradeep Jagadeesh wrote: >> This patch factors out code to use the ThrottleLimits >> strurcture. >> >> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> >> Reviewed-by: Greg Kurz <groug@kaod.org> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> Reviewed-by: Alberto Garcia <berto@igalia.com> >> Reviewed-by: Markus Armbruster <armbru@redhat.com> >> --- >> qapi/block-core.json | 78 >> +++------------------------------------------------- >> 1 file changed, 4 insertions(+), 74 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index bb11815..d0ccfda 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -1826,84 +1826,13 @@ >> # >> # @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) >> -# >> # @group: throttle group name (Since 2.4) > > BlockIOThrottle and ThrottleLimits are not directly compatible, for > example here we have iops_rd_max_length as a name, > >> # >> # 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', '*group': 'str' } } >> >> ## >> # @ThrottleLimits: >> @@ -1913,6 +1842,7 @@ >> # transaction. All fields are optional. When setting limits, if a >> field is >> # missing the current value is not changed. >> # >> +# @id: device id >> # @iops-total: limit total I/O operations per second >> # @iops-total-max: I/O operations burst >> # @iops-total-max-length: length of the iops-total-max burst period, >> in seconds >> @@ -1942,7 +1872,7 @@ >> # Since: 2.11 >> ## >> { 'struct': 'ThrottleLimits', >> - 'data': { '*iops-total' : 'int', '*iops-total-max' : 'int', >> + 'data': { '*id' : 'str', '*iops-total' : 'int', '*iops-total-max' : >> 'int', >> '*iops-total-max-length' : 'int', '*iops-read' : 'int', >> '*iops-read-max' : 'int', '*iops-read-max-length' : 'int', >> '*iops-write' : 'int', '*iops-write-max' : 'int', > > And here it's iops-read-max-length. This breaks the block layer's old > throttling API. Do we want the old API replaced for 2.11? I don't know > which maintainer is responsible for this, so I CC'd armbru. I do not think its going to break. It just exist we neither try to set that or to query. -Pradeep
diff --git a/qapi/block-core.json b/qapi/block-core.json index bb11815..d0ccfda 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1826,84 +1826,13 @@ # # @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) -# # @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', '*group': 'str' } } ## # @ThrottleLimits: @@ -1913,6 +1842,7 @@ # transaction. All fields are optional. When setting limits, if a field is # missing the current value is not changed. # +# @id: device id # @iops-total: limit total I/O operations per second # @iops-total-max: I/O operations burst # @iops-total-max-length: length of the iops-total-max burst period, in seconds @@ -1942,7 +1872,7 @@ # Since: 2.11 ## { 'struct': 'ThrottleLimits', - 'data': { '*iops-total' : 'int', '*iops-total-max' : 'int', + 'data': { '*id' : 'str', '*iops-total' : 'int', '*iops-total-max' : 'int', '*iops-total-max-length' : 'int', '*iops-read' : 'int', '*iops-read-max' : 'int', '*iops-read-max-length' : 'int', '*iops-write' : 'int', '*iops-write-max' : 'int',