diff mbox

[2/2,v1] throttle: factor out the redundant code

Message ID 1490271635-19049-2-git-send-email-pradeep.jagadeesh@huawei.com
State New
Headers show

Commit Message

Pradeep Jagadeesh March 23, 2017, 12:20 p.m. UTC 
This patchset removes the throttle redundant code that was present
in block and fsdev files.

Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
---
 blockdev.c                      | 106 ++++-----------------------------------
 fsdev/Makefile.objs             |   1 +
 fsdev/qemu-fsdev-throttle.c     |  94 +---------------------------------
 fsdev/qemu-fsdev-throttle.h     |   3 +-
 hmp.c                           |  39 +++++++--------
 include/qemu/throttle-options.h |   5 ++
 qapi/block-core.json            |  76 +---------------------------
 qapi/iothrottle.json            |   4 +-
 util/Makefile.objs              |   1 +
 util/throttle-options.c         | 108 ++++++++++++++++++++++++++++++++++++++++
 10 files changed, 151 insertions(+), 286 deletions(-)
 create mode 100644 util/throttle-options.c

Comments

Eric Blake March 23, 2017, 2:46 p.m. UTC | #1 
On 03/23/2017 07:20 AM, Pradeep Jagadeesh wrote:
> This patchset removes the throttle redundant code that was present
> in block and fsdev files.
> 
> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
> ---

I think you want portions of this patch to come first; you want to
refactor out the IOThrottle portion of existing types, and then extend
IOThrottle to be used in more places.

>  blockdev.c                      | 106 ++++-----------------------------------
>  fsdev/Makefile.objs             |   1 +
>  fsdev/qemu-fsdev-throttle.c     |  94 +---------------------------------
>  fsdev/qemu-fsdev-throttle.h     |   3 +-
>  hmp.c                           |  39 +++++++--------
>  include/qemu/throttle-options.h |   5 ++
>  qapi/block-core.json            |  76 +---------------------------
>  qapi/iothrottle.json            |   4 +-
>  util/Makefile.objs              |   1 +
>  util/throttle-options.c         | 108 ++++++++++++++++++++++++++++++++++++++++
>  10 files changed, 151 insertions(+), 286 deletions(-)
>  create mode 100644 util/throttle-options.c
> 

> +++ b/fsdev/qemu-fsdev-throttle.c
> @@ -33,56 +33,7 @@ void fsdev_set_io_throttle(IOThrottle *arg, FsThrottle *fst, Error **errp)
>  {
>      ThrottleConfig cfg;
>  
> -    throttle_config_init(&cfg);
> -    cfg.buckets[THROTTLE_BPS_TOTAL].avg = arg->bps;
> -    cfg.buckets[THROTTLE_BPS_READ].avg  = arg->bps_rd;
> -    cfg.buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr;
> -

This code was added in the last patch - which makes for a lot of
unnecessary churn.  Again, the best series does refactoring of existing
code first, then adds the new code that takes advantage of the
refactored entry points, so that the new code isn't churning.


> +++ b/hmp.c
> @@ -1554,20 +1554,25 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &err);
>  }
>  
> +static void hmp_initialize_io_throttle(IOThrottle *iot, const QDict *qdict)
> +{
> +    iot->has_device = true;
> +    iot->device = (char *) qdict_get_str(qdict, "device");

Is casting away const going to result in a double free of memory later
on?  Or put another way, should this be a g_strdup'd copy (which you
then have to make sure is not leaked)?

> +    iot->bps = qdict_get_int(qdict, "bps");
> +    iot->bps_rd = qdict_get_int(qdict, "bps_rd");
> +    iot->bps_wr = qdict_get_int(qdict, "bps_wr");
> +    iot->iops = qdict_get_int(qdict, "iops");
> +    iot->iops_rd = qdict_get_int(qdict, "iops_rd");
> +    iot->iops_wr = qdict_get_int(qdict, "iops_wr");
> +}
> +
>  void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> -    BlockIOThrottle throttle = {
> -        .has_device = true,
> -        .device = (char *) qdict_get_str(qdict, "device"),

At any rate, I see it is just code motion.  You are refactoring too much
in one patch here; I'd consider splitting it along the lines of one
patch that adds hmp_initialize_io_throttle; and another patch that adds
parse_io_throttle_options(), so that it's easier to see just one piece
of code motion at a time, rather than trying to track multiple motions
in the same large patch.

> +++ b/qapi/block-core.json
> @@ -1758,86 +1758,14 @@
>  #
>  # A set of parameters describing block throttling.
>  #

> -# @iops_size: an I/O size in bytes (Since 1.7)
> +# @iothrottle: throttle configuration (Since 1.7)

NACK. This is an incompatible change; you are breaking the QMP wire
structure (that is, where I used to pass "arguments":{"device":"foo",
"bps_rd":1, "group":"bar" }, your change would now require me to pass
"arguments":{"iothrottle":{"device":"foo", "bps_rd":1}, "group":"bar"};
the extra nesting is what breaks things).

>  #
>  # @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' } }
> +  'data': { '*iothrottle': 'IOThrottle', '*group': 'str' } }

What you REALLY want is a compatible change, namely:

{ 'struct': 'BlockIOThrottle', 'base': 'IOThrottle',
  'data': { '*group': 'str' } }

which says that all fields of IOThrottle are inherited at the same level
as the additional field 'group'.

For that matter, are you sure that 'group' is the only field that should
not be directly in IOThrottle, or should 'device' also be specific to
BlockIOThrottle (I'm not sure whether you were actually using 'device'
in the fsdev case, or if 'id' was sufficient).

>  
>  ##
>  # @block-stream:
> diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
> index f7b615d..58f4520 100644
> --- a/qapi/iothrottle.json
> +++ b/qapi/iothrottle.json
> @@ -14,6 +14,8 @@
>  #
>  # @device: Block device name
>  #
> +# @id: The name or QOM path of the guest device (since: 2.8)
> +#

You've missed 2.8 by a long shot.  Oh - this is an argument that it is
'id' (and not 'device') that does not belong here, but should be placed
alongside 'group' in the BlockIOThrottle subtype.

>  # @bps: total throughput limit in bytes per second
>  #
>  # @bps_rd: read throughput limit in bytes per second
> @@ -80,7 +82,7 @@
>  # Since: 2.10
>  ##
>  { 'struct': 'IOThrottle',
> -  'data': { '*device': 'str', 'bps': 'int', 'bps_rd': 'int',
> +  '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',
Eric Blake March 23, 2017, 6:14 p.m. UTC | #2 
On 03/23/2017 09:46 AM, Eric Blake wrote:
> On 03/23/2017 07:20 AM, Pradeep Jagadeesh wrote:
>> This patchset removes the throttle redundant code that was present
>> in block and fsdev files.
>>
>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>> ---
> 
> I think you want portions of this patch to come first; you want to
> refactor out the IOThrottle portion of existing types, and then extend
> IOThrottle to be used in more places.

For the record, here's a nice example of how to factor out common fields
into a new base class:

https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg04649.html
Pradeep Jagadeesh March 24, 2017, 2:09 p.m. UTC | #3 
On 3/23/2017 7:14 PM, Eric Blake wrote:
> On 03/23/2017 09:46 AM, Eric Blake wrote:
>> On 03/23/2017 07:20 AM, Pradeep Jagadeesh wrote:
>>> This patchset removes the throttle redundant code that was present
>>> in block and fsdev files.
>>>
>>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>>> ---
>>
>> I think you want portions of this patch to come first; you want to
>> refactor out the IOThrottle portion of existing types, and then extend
>> IOThrottle to be used in more places.
>
> For the record, here's a nice example of how to factor out common fields
> into a new base class:
>
> https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg04649.html

Nice one, but If I do this way, I can not remove some duplicate code 
from initialization code. For example below code.

void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
  {
      Error *err = NULL;
-    BlockIOThrottle throttle = {
-        .has_device = true,
-        .device = (char *) qdict_get_str(qdict, "device"),
-        .bps = qdict_get_int(qdict, "bps"),
-        .bps_rd = qdict_get_int(qdict, "bps_rd"),
-        .bps_wr = qdict_get_int(qdict, "bps_wr"),
-        .iops = qdict_get_int(qdict, "iops"),
-        .iops_rd = qdict_get_int(qdict, "iops_rd"),
-        .iops_wr = qdict_get_int(qdict, "iops_wr"),
-    };
>
Also
  void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict)
  {
      Error *err = NULL;
-    IOThrottle throttle = {
-        .device = (char *) qdict_get_str(qdict, "device"),
-        .bps = qdict_get_int(qdict, "bps"),
-        .bps_rd = qdict_get_int(qdict, "bps_rd"),
-        .bps_wr = qdict_get_int(qdict, "bps_wr"),
-        .iops = qdict_get_int(qdict, "iops"),
-        .iops_rd = qdict_get_int(qdict, "iops_rd"),
-        .iops_wr = qdict_get_int(qdict, "iops_wr"),
-    };
-
Regards,
Pradeep
Pradeep Jagadeesh March 28, 2017, 11:11 a.m. UTC | #4 
On 3/23/2017 3:46 PM, Eric Blake wrote:
> On 03/23/2017 07:20 AM, Pradeep Jagadeesh wrote:
>> This patchset removes the throttle redundant code that was present
>> in block and fsdev files.
>>
>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>
>> ---
>
> I think you want portions of this patch to come first; you want to
> refactor out the IOThrottle portion of existing types, and then extend
> IOThrottle to be used in more places.
OK, I will do that.
>
>>  blockdev.c                      | 106 ++++-----------------------------------
>>  fsdev/Makefile.objs             |   1 +
>>  fsdev/qemu-fsdev-throttle.c     |  94 +---------------------------------
>>  fsdev/qemu-fsdev-throttle.h     |   3 +-
>>  hmp.c                           |  39 +++++++--------
>>  include/qemu/throttle-options.h |   5 ++
>>  qapi/block-core.json            |  76 +---------------------------
>>  qapi/iothrottle.json            |   4 +-
>>  util/Makefile.objs              |   1 +
>>  util/throttle-options.c         | 108 ++++++++++++++++++++++++++++++++++++++++
>>  10 files changed, 151 insertions(+), 286 deletions(-)
>>  create mode 100644 util/throttle-options.c
>>
>
>> +++ b/fsdev/qemu-fsdev-throttle.c
>> @@ -33,56 +33,7 @@ void fsdev_set_io_throttle(IOThrottle *arg, FsThrottle *fst, Error **errp)
>>  {
>>      ThrottleConfig cfg;
>>
>> -    throttle_config_init(&cfg);
>> -    cfg.buckets[THROTTLE_BPS_TOTAL].avg = arg->bps;
>> -    cfg.buckets[THROTTLE_BPS_READ].avg  = arg->bps_rd;
>> -    cfg.buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr;
>> -
>
> This code was added in the last patch - which makes for a lot of
> unnecessary churn.  Again, the best series does refactoring of existing
> code first, then adds the new code that takes advantage of the
> refactored entry points, so that the new code isn't churning.
>
OK
>
>> +++ b/hmp.c
>> @@ -1554,20 +1554,25 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>>      hmp_handle_error(mon, &err);
>>  }
>>
>> +static void hmp_initialize_io_throttle(IOThrottle *iot, const QDict *qdict)
>> +{
>> +    iot->has_device = true;
>> +    iot->device = (char *) qdict_get_str(qdict, "device");
>
> Is casting away const going to result in a double free of memory later
> on?  Or put another way, should this be a g_strdup'd copy (which you
> then have to make sure is not leaked)?
This code was there in block_set_io_throttle(). I have just moved to new 
function. But I will think about it.
>
>> +    iot->bps = qdict_get_int(qdict, "bps");
>> +    iot->bps_rd = qdict_get_int(qdict, "bps_rd");
>> +    iot->bps_wr = qdict_get_int(qdict, "bps_wr");
>> +    iot->iops = qdict_get_int(qdict, "iops");
>> +    iot->iops_rd = qdict_get_int(qdict, "iops_rd");
>> +    iot->iops_wr = qdict_get_int(qdict, "iops_wr");
>> +}
>> +
>>  void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
>>  {
>>      Error *err = NULL;
>> -    BlockIOThrottle throttle = {
>> -        .has_device = true,
>> -        .device = (char *) qdict_get_str(qdict, "device"),
>
> At any rate, I see it is just code motion.  You are refactoring too much
> in one patch here; I'd consider splitting it along the lines of one
> patch that adds hmp_initialize_io_throttle; and another patch that adds
> parse_io_throttle_options(), so that it's easier to see just one piece
> of code motion at a time, rather than trying to track multiple motions
> in the same large patch.
I will split the code into different patches.
>
>> +++ b/qapi/block-core.json
>> @@ -1758,86 +1758,14 @@
>>  #
>>  # A set of parameters describing block throttling.
>>  #
>
>> -# @iops_size: an I/O size in bytes (Since 1.7)
>> +# @iothrottle: throttle configuration (Since 1.7)
>
> NACK. This is an incompatible change; you are breaking the QMP wire
> structure (that is, where I used to pass "arguments":{"device":"foo",
> "bps_rd":1, "group":"bar" }, your change would now require me to pass
> "arguments":{"iothrottle":{"device":"foo", "bps_rd":1}, "group":"bar"};
> the extra nesting is what breaks things).
>
>>  #
>>  # @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' } }
>> +  'data': { '*iothrottle': 'IOThrottle', '*group': 'str' } }
>
> What you REALLY want is a compatible change, namely:
>
> { 'struct': 'BlockIOThrottle', 'base': 'IOThrottle',
>   'data': { '*group': 'str' } }
>
> which says that all fields of IOThrottle are inherited at the same level
> as the additional field 'group'.
>
> For that matter, are you sure that 'group' is the only field that should
> not be directly in IOThrottle, or should 'device' also be specific to
> BlockIOThrottle (I'm not sure whether you were actually using 'device'
> in the fsdev case, or if 'id' was sufficient).

Yes, I just need id in case of fsdev. I will fix that. But if I do this 
way, some of the code refactoring is not possible.Because they will have 
different structures for fsdev and block.
>
>>
>>  ##
>>  # @block-stream:
>> diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
>> index f7b615d..58f4520 100644
>> --- a/qapi/iothrottle.json
>> +++ b/qapi/iothrottle.json
>> @@ -14,6 +14,8 @@
>>  #
>>  # @device: Block device name
>>  #
>> +# @id: The name or QOM path of the guest device (since: 2.8)
>> +#
>
> You've missed 2.8 by a long shot.  Oh - this is an argument that it is
> 'id' (and not 'device') that does not belong here, but should be placed
> alongside 'group' in the BlockIOThrottle subtype.
id is required for the fsdev/block both. This was part of the 
BlockIOThrottle. So I just took it and kept the description as it is.
What should I changed to? 2.10?
>
>>  # @bps: total throughput limit in bytes per second
>>  #
>>  # @bps_rd: read throughput limit in bytes per second
>> @@ -80,7 +82,7 @@
>>  # Since: 2.10
>>  ##
>>  { 'struct': 'IOThrottle',
>> -  'data': { '*device': 'str', 'bps': 'int', 'bps_rd': 'int',
>> +  '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',
>
>
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index c5b2c2c..2f4ec3a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -386,49 +386,7 @@  static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
     }
 
     if (throttle_cfg) {
-        throttle_config_init(throttle_cfg);
-        throttle_cfg->buckets[THROTTLE_BPS_TOTAL].avg =
-            qemu_opt_get_number(opts, "throttling.bps-total", 0);
-        throttle_cfg->buckets[THROTTLE_BPS_READ].avg  =
-            qemu_opt_get_number(opts, "throttling.bps-read", 0);
-        throttle_cfg->buckets[THROTTLE_BPS_WRITE].avg =
-            qemu_opt_get_number(opts, "throttling.bps-write", 0);
-        throttle_cfg->buckets[THROTTLE_OPS_TOTAL].avg =
-            qemu_opt_get_number(opts, "throttling.iops-total", 0);
-        throttle_cfg->buckets[THROTTLE_OPS_READ].avg =
-            qemu_opt_get_number(opts, "throttling.iops-read", 0);
-        throttle_cfg->buckets[THROTTLE_OPS_WRITE].avg =
-            qemu_opt_get_number(opts, "throttling.iops-write", 0);
-
-        throttle_cfg->buckets[THROTTLE_BPS_TOTAL].max =
-            qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
-        throttle_cfg->buckets[THROTTLE_BPS_READ].max  =
-            qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
-        throttle_cfg->buckets[THROTTLE_BPS_WRITE].max =
-            qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
-        throttle_cfg->buckets[THROTTLE_OPS_TOTAL].max =
-            qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
-        throttle_cfg->buckets[THROTTLE_OPS_READ].max =
-            qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
-        throttle_cfg->buckets[THROTTLE_OPS_WRITE].max =
-            qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
-
-        throttle_cfg->buckets[THROTTLE_BPS_TOTAL].burst_length =
-            qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
-        throttle_cfg->buckets[THROTTLE_BPS_READ].burst_length  =
-            qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
-        throttle_cfg->buckets[THROTTLE_BPS_WRITE].burst_length =
-            qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
-        throttle_cfg->buckets[THROTTLE_OPS_TOTAL].burst_length =
-            qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
-        throttle_cfg->buckets[THROTTLE_OPS_READ].burst_length =
-            qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
-        throttle_cfg->buckets[THROTTLE_OPS_WRITE].burst_length =
-            qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
-
-        throttle_cfg->op_size =
-            qemu_opt_get_number(opts, "throttling.iops-size", 0);
-
+        parse_io_throttle_options(throttle_cfg, opts);
         if (!throttle_is_valid(throttle_cfg, errp)) {
             return;
         }
@@ -2635,9 +2593,12 @@  void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
     BlockDriverState *bs;
     BlockBackend *blk;
     AioContext *aio_context;
+    IOThrottle *iothrottle;
+
+    iothrottle = arg->iothrottle;
 
-    blk = qmp_get_blk(arg->has_device ? arg->device : NULL,
-                      arg->has_id ? arg->id : NULL,
+    blk = qmp_get_blk(iothrottle->has_device ? iothrottle->device : NULL,
+                      iothrottle->has_id ? iothrottle->id : NULL,
                       errp);
     if (!blk) {
         return;
@@ -2652,56 +2613,7 @@  void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
         goto out;
     }
 
-    throttle_config_init(&cfg);
-    cfg.buckets[THROTTLE_BPS_TOTAL].avg = arg->bps;
-    cfg.buckets[THROTTLE_BPS_READ].avg  = arg->bps_rd;
-    cfg.buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr;
-
-    cfg.buckets[THROTTLE_OPS_TOTAL].avg = arg->iops;
-    cfg.buckets[THROTTLE_OPS_READ].avg  = arg->iops_rd;
-    cfg.buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr;
-
-    if (arg->has_bps_max) {
-        cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max;
-    }
-    if (arg->has_bps_rd_max) {
-        cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max;
-    }
-    if (arg->has_bps_wr_max) {
-        cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max;
-    }
-    if (arg->has_iops_max) {
-        cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max;
-    }
-    if (arg->has_iops_rd_max) {
-        cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max;
-    }
-    if (arg->has_iops_wr_max) {
-        cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max;
-    }
-
-    if (arg->has_bps_max_length) {
-        cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length;
-    }
-    if (arg->has_bps_rd_max_length) {
-        cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length;
-    }
-    if (arg->has_bps_wr_max_length) {
-        cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length;
-    }
-    if (arg->has_iops_max_length) {
-        cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length;
-    }
-    if (arg->has_iops_rd_max_length) {
-        cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length;
-    }
-    if (arg->has_iops_wr_max_length) {
-        cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length;
-    }
-
-    if (arg->has_iops_size) {
-        cfg.op_size = arg->iops_size;
-    }
+    qmp_set_io_throttle(&cfg, iothrottle);
 
     if (!throttle_is_valid(&cfg, errp)) {
         goto out;
@@ -2713,8 +2625,8 @@  void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp)
         if (!blk_get_public(blk)->throttle_state) {
             blk_io_limits_enable(blk,
                                  arg->has_group ? arg->group :
-                                 arg->has_device ? arg->device :
-                                 arg->id);
+                                 iothrottle->has_device ? iothrottle->device :
+                                 iothrottle->id);
         } else if (arg->has_group) {
             blk_io_limits_update_group(blk, arg->group);
         }
diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs
index 659df6e..31eed64 100644
--- a/fsdev/Makefile.objs
+++ b/fsdev/Makefile.objs
@@ -5,6 +5,7 @@  common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o
 else
 common-obj-y = qemu-fsdev-dummy.o
 endif
+#common-obj-y += qemu-fsdev-opts.o prin.o qemu-fsdev-throttle.o qmp-throttle.o
 common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o
 
 # Toplevel always builds this; targets without virtio will put it in
diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
index e3ea095..275a565 100644
--- a/fsdev/qemu-fsdev-throttle.c
+++ b/fsdev/qemu-fsdev-throttle.c
@@ -33,56 +33,7 @@  void fsdev_set_io_throttle(IOThrottle *arg, FsThrottle *fst, Error **errp)
 {
     ThrottleConfig cfg;
 
-    throttle_config_init(&cfg);
-    cfg.buckets[THROTTLE_BPS_TOTAL].avg = arg->bps;
-    cfg.buckets[THROTTLE_BPS_READ].avg  = arg->bps_rd;
-    cfg.buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr;
-
-    cfg.buckets[THROTTLE_OPS_TOTAL].avg = arg->iops;
-    cfg.buckets[THROTTLE_OPS_READ].avg  = arg->iops_rd;
-    cfg.buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr;
-
-    if (arg->has_bps_max) {
-        cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max;
-    }
-    if (arg->has_bps_rd_max) {
-        cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max;
-    }
-    if (arg->has_bps_wr_max) {
-        cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max;
-    }
-    if (arg->has_iops_max) {
-        cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max;
-    }
-    if (arg->has_iops_rd_max) {
-        cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max;
-    }
-    if (arg->has_iops_wr_max) {
-        cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max;
-    }
-
-    if (arg->has_bps_max_length) {
-        cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length;
-    }
-    if (arg->has_bps_rd_max_length) {
-        cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length;
-    }
-    if (arg->has_bps_wr_max_length) {
-        cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length;
-    }
-    if (arg->has_iops_max_length) {
-        cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length;
-    }
-    if (arg->has_iops_rd_max_length) {
-        cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length;
-    }
-    if (arg->has_iops_wr_max_length) {
-        cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length;
-    }
-
-    if (arg->has_iops_size) {
-        cfg.op_size = arg->iops_size;
-    }
+    qmp_set_io_throttle(&cfg, arg);
 
     if (throttle_is_valid(&cfg, errp)) {
         fst->cfg = cfg;
@@ -127,48 +78,7 @@  void fsdev_get_io_throttle(FsThrottle *fst, IOThrottle **fs9pcfg,
 
 void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp)
 {
-    throttle_config_init(&fst->cfg);
-    fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg =
-        qemu_opt_get_number(opts, "throttling.bps-total", 0);
-    fst->cfg.buckets[THROTTLE_BPS_READ].avg  =
-        qemu_opt_get_number(opts, "throttling.bps-read", 0);
-    fst->cfg.buckets[THROTTLE_BPS_WRITE].avg =
-        qemu_opt_get_number(opts, "throttling.bps-write", 0);
-    fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg =
-        qemu_opt_get_number(opts, "throttling.iops-total", 0);
-    fst->cfg.buckets[THROTTLE_OPS_READ].avg =
-        qemu_opt_get_number(opts, "throttling.iops-read", 0);
-    fst->cfg.buckets[THROTTLE_OPS_WRITE].avg =
-        qemu_opt_get_number(opts, "throttling.iops-write", 0);
-
-    fst->cfg.buckets[THROTTLE_BPS_TOTAL].max =
-        qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
-    fst->cfg.buckets[THROTTLE_BPS_READ].max  =
-        qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
-    fst->cfg.buckets[THROTTLE_BPS_WRITE].max =
-        qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
-    fst->cfg.buckets[THROTTLE_OPS_TOTAL].max =
-        qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
-    fst->cfg.buckets[THROTTLE_OPS_READ].max =
-        qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
-    fst->cfg.buckets[THROTTLE_OPS_WRITE].max =
-        qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
-
-    fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length =
-        qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
-    fst->cfg.buckets[THROTTLE_BPS_READ].burst_length  =
-        qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
-    fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length =
-        qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
-    fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length =
-        qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
-    fst->cfg.buckets[THROTTLE_OPS_READ].burst_length =
-        qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
-    fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length =
-        qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
-    fst->cfg.op_size =
-        qemu_opt_get_number(opts, "throttling.iops-size", 0);
-
+    parse_io_throttle_options(&fst->cfg, opts);
     throttle_is_valid(&fst->cfg, errp);
 }
 
diff --git a/fsdev/qemu-fsdev-throttle.h b/fsdev/qemu-fsdev-throttle.h
index f6b4d86..14cd183 100644
--- a/fsdev/qemu-fsdev-throttle.h
+++ b/fsdev/qemu-fsdev-throttle.h
@@ -19,13 +19,14 @@ 
 #include "qemu/main-loop.h"
 #include "qemu/coroutine.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
 #include "qemu/throttle.h"
 #include "qapi/qmp/types.h"
 #include "qapi-visit.h"
-#include "qapi/qmp/qerror.h"
 #include "qapi/qobject-output-visitor.h"
 #include "qapi/util.h"
 #include "qmp-commands.h"
+#include "qemu/throttle-options.h"
 
 typedef struct FsThrottle {
     ThrottleState ts;
diff --git a/hmp.c b/hmp.c
index 008ff3b..f489d49 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1554,20 +1554,25 @@  void hmp_change(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
+static void hmp_initialize_io_throttle(IOThrottle *iot, const QDict *qdict)
+{
+    iot->has_device = true;
+    iot->device = (char *) qdict_get_str(qdict, "device");
+    iot->bps = qdict_get_int(qdict, "bps");
+    iot->bps_rd = qdict_get_int(qdict, "bps_rd");
+    iot->bps_wr = qdict_get_int(qdict, "bps_wr");
+    iot->iops = qdict_get_int(qdict, "iops");
+    iot->iops_rd = qdict_get_int(qdict, "iops_rd");
+    iot->iops_wr = qdict_get_int(qdict, "iops_wr");
+}
+
 void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
-    BlockIOThrottle throttle = {
-        .has_device = true,
-        .device = (char *) qdict_get_str(qdict, "device"),
-        .bps = qdict_get_int(qdict, "bps"),
-        .bps_rd = qdict_get_int(qdict, "bps_rd"),
-        .bps_wr = qdict_get_int(qdict, "bps_wr"),
-        .iops = qdict_get_int(qdict, "iops"),
-        .iops_rd = qdict_get_int(qdict, "iops_rd"),
-        .iops_wr = qdict_get_int(qdict, "iops_wr"),
-    };
-
+    BlockIOThrottle throttle;
+    IOThrottle iothrottle;
+    hmp_initialize_io_throttle(&iothrottle, qdict);
+    throttle.iothrottle = &iothrottle;
     qmp_block_set_io_throttle(&throttle, &err);
     hmp_handle_error(mon, &err);
 }
@@ -1577,16 +1582,8 @@  void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
 void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
-    IOThrottle throttle = {
-        .device = (char *) qdict_get_str(qdict, "device"),
-        .bps = qdict_get_int(qdict, "bps"),
-        .bps_rd = qdict_get_int(qdict, "bps_rd"),
-        .bps_wr = qdict_get_int(qdict, "bps_wr"),
-        .iops = qdict_get_int(qdict, "iops"),
-        .iops_rd = qdict_get_int(qdict, "iops_rd"),
-        .iops_wr = qdict_get_int(qdict, "iops_wr"),
-    };
-
+    IOThrottle throttle;
+    hmp_initialize_io_throttle(&throttle, qdict);
     qmp_fsdev_set_io_throttle(&throttle, &err);
     hmp_handle_error(mon, &err);
 }
diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
index 3133d1c..59a575e 100644
--- a/include/qemu/throttle-options.h
+++ b/include/qemu/throttle-options.h
@@ -10,6 +10,9 @@ 
 #ifndef THROTTLE_OPTIONS_H
 #define THROTTLE_OPTIONS_H
 
+#include "qemu/throttle.h"
+#include "qmp-commands.h"
+
 #define THROTTLE_OPTS \
           { \
             .name = "throttling.iops-total",\
@@ -89,4 +92,6 @@ 
             .help = "when limiting by iops max size of an I/O in bytes",\
         }
 
+void qmp_set_io_throttle(ThrottleConfig *, IOThrottle *);
+void parse_io_throttle_options(ThrottleConfig *, QemuOpts *);
 #endif
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0f132fc..3d0a3fb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1758,86 +1758,14 @@ 
 #
 # 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)
+# @iothrottle: throttle configuration (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' } }
+  'data': { '*iothrottle': 'IOThrottle', '*group': 'str' } }
 
 ##
 # @block-stream:
diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json
index f7b615d..58f4520 100644
--- a/qapi/iothrottle.json
+++ b/qapi/iothrottle.json
@@ -14,6 +14,8 @@ 
 #
 # @device: Block device name
 #
+# @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
@@ -80,7 +82,7 @@ 
 # Since: 2.10
 ##
 { 'struct': 'IOThrottle',
-  'data': { '*device': 'str', 'bps': 'int', 'bps_rd': 'int',
+  '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',
diff --git a/util/Makefile.objs b/util/Makefile.objs
index 06366b5..401eaf5 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -42,3 +42,4 @@  util-obj-y += log.o
 util-obj-y += qdist.o
 util-obj-y += qht.o
 util-obj-y += range.o
+util-obj-y += throttle-options.o
diff --git a/util/throttle-options.c b/util/throttle-options.c
new file mode 100644
index 0000000..e428938
--- /dev/null
+++ b/util/throttle-options.c
@@ -0,0 +1,108 @@ 
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "qemu/throttle-options.h"
+#include "qemu/iov.h"
+
+void qmp_set_io_throttle(ThrottleConfig *cfg, IOThrottle *arg)
+{
+
+    throttle_config_init(cfg);
+    cfg->buckets[THROTTLE_BPS_TOTAL].avg = arg->bps;
+    cfg->buckets[THROTTLE_BPS_READ].avg  = arg->bps_rd;
+    cfg->buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr;
+
+    cfg->buckets[THROTTLE_OPS_TOTAL].avg = arg->iops;
+    cfg->buckets[THROTTLE_OPS_READ].avg  = arg->iops_rd;
+    cfg->buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr;
+
+    if (arg->has_bps_max) {
+        cfg->buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max;
+    }
+    if (arg->has_bps_rd_max) {
+        cfg->buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max;
+    }
+    if (arg->has_bps_wr_max) {
+        cfg->buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max;
+    }
+    if (arg->has_iops_max) {
+        cfg->buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max;
+    }
+    if (arg->has_iops_rd_max) {
+        cfg->buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max;
+    }
+    if (arg->has_iops_wr_max) {
+        cfg->buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max;
+    }
+
+    if (arg->has_bps_max_length) {
+        cfg->buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length;
+    }
+    if (arg->has_bps_rd_max_length) {
+        cfg->buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length;
+    }
+    if (arg->has_bps_wr_max_length) {
+        cfg->buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length;
+    }
+    if (arg->has_iops_max_length) {
+        cfg->buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length;
+    }
+    if (arg->has_iops_rd_max_length) {
+        cfg->buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length;
+    }
+    if (arg->has_iops_wr_max_length) {
+        cfg->buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length;
+    }
+
+    if (arg->has_iops_size) {
+        cfg->op_size = arg->iops_size;
+    }
+}
+
+void parse_io_throttle_options(ThrottleConfig *throttle_cfg, QemuOpts *opts)
+{
+    throttle_config_init(throttle_cfg);
+    throttle_cfg->buckets[THROTTLE_BPS_TOTAL].avg =
+        qemu_opt_get_number(opts, "throttling.bps-total", 0);
+    throttle_cfg->buckets[THROTTLE_BPS_READ].avg  =
+        qemu_opt_get_number(opts, "throttling.bps-read", 0);
+    throttle_cfg->buckets[THROTTLE_BPS_WRITE].avg =
+        qemu_opt_get_number(opts, "throttling.bps-write", 0);
+    throttle_cfg->buckets[THROTTLE_OPS_TOTAL].avg =
+        qemu_opt_get_number(opts, "throttling.iops-total", 0);
+    throttle_cfg->buckets[THROTTLE_OPS_READ].avg =
+        qemu_opt_get_number(opts, "throttling.iops-read", 0);
+    throttle_cfg->buckets[THROTTLE_OPS_WRITE].avg =
+        qemu_opt_get_number(opts, "throttling.iops-write", 0);
+
+    throttle_cfg->buckets[THROTTLE_BPS_TOTAL].max =
+        qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
+    throttle_cfg->buckets[THROTTLE_BPS_READ].max  =
+        qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
+    throttle_cfg->buckets[THROTTLE_BPS_WRITE].max =
+        qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
+    throttle_cfg->buckets[THROTTLE_OPS_TOTAL].max =
+        qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
+    throttle_cfg->buckets[THROTTLE_OPS_READ].max =
+        qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
+    throttle_cfg->buckets[THROTTLE_OPS_WRITE].max =
+        qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
+
+    throttle_cfg->buckets[THROTTLE_BPS_TOTAL].burst_length =
+        qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1);
+    throttle_cfg->buckets[THROTTLE_BPS_READ].burst_length  =
+        qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1);
+    throttle_cfg->buckets[THROTTLE_BPS_WRITE].burst_length =
+        qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1);
+    throttle_cfg->buckets[THROTTLE_OPS_TOTAL].burst_length =
+        qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1);
+    throttle_cfg->buckets[THROTTLE_OPS_READ].burst_length =
+        qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1);
+    throttle_cfg->buckets[THROTTLE_OPS_WRITE].burst_length =
+        qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1);
+
+    throttle_cfg->op_size =
+        qemu_opt_get_number(opts, "throttling.iops-size", 0);
+
+}
+
+