[v11,2/6] qmp: Use ThrottleLimits structure

Message ID 1505385610-35529-3-git-send-email-pradeep.jagadeesh@huawei.com
State New
Headers show
Series
  • fsdev: qmp interface for io throttling
Related show

Commit Message

Pradeep Jagadeesh Sept. 14, 2017, 10:40 a.m.
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(-)

Comments

Greg Kurz Sept. 14, 2017, 11:19 a.m. | #1
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',
Pradeep Jagadeesh Sept. 14, 2017, 11:23 a.m. | #2
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',
>
Manos Pitsidianakis Sept. 18, 2017, 4:04 p.m. | #3
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.
Manos Pitsidianakis Sept. 18, 2017, 4:25 p.m. | #4
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.
Eric Blake Sept. 18, 2017, 5:10 p.m. | #5
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.
Pradeep Jagadeesh Sept. 19, 2017, 8:07 a.m. | #6
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
Pradeep Jagadeesh Sept. 19, 2017, 10:06 a.m. | #7
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
Eric Blake Sept. 19, 2017, 1:10 p.m. | #8
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.
Greg Kurz Sept. 19, 2017, 1:32 p.m. | #9
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
>
Pradeep Jagadeesh Sept. 20, 2017, 8:46 a.m. | #10
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

Patch

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',