| Message ID | 1463784024-17242-9-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat 21 May 2016 12:40:17 AM CEST, Eric Blake wrote: > Now that we can support boxed commands, use it to greatly > reduce the number of parameters (and likelihood of getting > out of sync) when adjusting throttle parameters. > > Signed-off-by: Eric Blake <eblake@redhat.com> It looks much better now, thanks! Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
Eric Blake <eblake@redhat.com> writes: > Now that we can support boxed commands, use it to greatly > reduce the number of parameters (and likelihood of getting > out of sync) when adjusting throttle parameters. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v7: new patch > --- > qapi/block-core.json | 20 ++++++++-- > blockdev.c | 111 +++++++++++++++++++-------------------------------- > hmp.c | 45 +++++---------------- > 3 files changed, 66 insertions(+), 110 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 98a20d2..26f7c0e 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1312,6 +1312,21 @@ > # the device will be removed from its group and the rest of its > # members will not be affected. The 'group' parameter is ignored. > # > +# See BlockIOThrottle for parameter descriptions. This comment will be trivial as soon as we got used to the 'box' feature :) > +# > +# Returns: Nothing on success > +# If @device is not a valid block device, DeviceNotFound > +# > +# Since: 1.1 > +## > +{ 'command': 'block_set_io_throttle', 'box': true, > + 'data': 'BlockIOThrottle' } > + > +## > +# BlockIOThrottle > +# > +# The parameters for the block_set_io_throttle command. This comment is prone to go stale. > +# > # @device: The name of the device > # > # @bps: total throughput limit in bytes per second > @@ -1378,12 +1393,9 @@ > # > # @group: #optional throttle group name (Since 2.4) > # > -# Returns: Nothing on success > -# If @device is not a valid block device, DeviceNotFound > -# > # Since: 1.1 > ## > -{ 'command': 'block_set_io_throttle', > +{ 'struct': 'BlockIOThrottle', > 'data': { 'device': 'str', 'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int', > 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int', > '*bps_max': 'int', '*bps_rd_max': 'int', > diff --git a/blockdev.c b/blockdev.c > index cf5afa3..b8db496 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2625,49 +2625,17 @@ fail: > } > > /* throttling disk I/O limits */ > -void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd, > - int64_t bps_wr, > - int64_t iops, > - int64_t iops_rd, > - int64_t iops_wr, > - bool has_bps_max, > - int64_t bps_max, > - bool has_bps_rd_max, > - int64_t bps_rd_max, > - bool has_bps_wr_max, > - int64_t bps_wr_max, > - bool has_iops_max, > - int64_t iops_max, > - bool has_iops_rd_max, > - int64_t iops_rd_max, > - bool has_iops_wr_max, > - int64_t iops_wr_max, > - bool has_bps_max_length, > - int64_t bps_max_length, > - bool has_bps_rd_max_length, > - int64_t bps_rd_max_length, > - bool has_bps_wr_max_length, > - int64_t bps_wr_max_length, > - bool has_iops_max_length, > - int64_t iops_max_length, > - bool has_iops_rd_max_length, > - int64_t iops_rd_max_length, > - bool has_iops_wr_max_length, > - int64_t iops_wr_max_length, > - bool has_iops_size, > - int64_t iops_size, > - bool has_group, > - const char *group, Error **errp) > +void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) > { This hunk together with the last one are worth a good chunk of the work you had to do to get here :) > ThrottleConfig cfg; > BlockDriverState *bs; > BlockBackend *blk; > AioContext *aio_context; > > - blk = blk_by_name(device); > + blk = blk_by_name(arg->device); > if (!blk) { > error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > - "Device '%s' not found", device); > + "Device '%s' not found", arg->device); > return; > } > > @@ -2676,59 +2644,59 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd, > > bs = blk_bs(blk); > if (!bs) { > - error_setg(errp, "Device '%s' has no medium", device); > + error_setg(errp, "Device '%s' has no medium", arg->device); > goto out; > } > > throttle_config_init(&cfg); > - cfg.buckets[THROTTLE_BPS_TOTAL].avg = bps; > - cfg.buckets[THROTTLE_BPS_READ].avg = bps_rd; > - cfg.buckets[THROTTLE_BPS_WRITE].avg = bps_wr; > + 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 = iops; > - cfg.buckets[THROTTLE_OPS_READ].avg = iops_rd; > - cfg.buckets[THROTTLE_OPS_WRITE].avg = iops_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 (has_bps_max) { > - cfg.buckets[THROTTLE_BPS_TOTAL].max = bps_max; > + if (arg->has_bps_max) { > + cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max; > } > - if (has_bps_rd_max) { > - cfg.buckets[THROTTLE_BPS_READ].max = bps_rd_max; > + if (arg->has_bps_rd_max) { > + cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max; > } > - if (has_bps_wr_max) { > - cfg.buckets[THROTTLE_BPS_WRITE].max = bps_wr_max; > + if (arg->has_bps_wr_max) { > + cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max; > } > - if (has_iops_max) { > - cfg.buckets[THROTTLE_OPS_TOTAL].max = iops_max; > + if (arg->has_iops_max) { > + cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max; > } > - if (has_iops_rd_max) { > - cfg.buckets[THROTTLE_OPS_READ].max = iops_rd_max; > + if (arg->has_iops_rd_max) { > + cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max; > } > - if (has_iops_wr_max) { > - cfg.buckets[THROTTLE_OPS_WRITE].max = iops_wr_max; > + if (arg->has_iops_wr_max) { > + cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max; > } > > - if (has_bps_max_length) { > - cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = bps_max_length; > + if (arg->has_bps_max_length) { > + cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length; > } > - if (has_bps_rd_max_length) { > - cfg.buckets[THROTTLE_BPS_READ].burst_length = bps_rd_max_length; > + if (arg->has_bps_rd_max_length) { > + cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length; > } > - if (has_bps_wr_max_length) { > - cfg.buckets[THROTTLE_BPS_WRITE].burst_length = bps_wr_max_length; > + if (arg->has_bps_wr_max_length) { > + cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length; > } > - if (has_iops_max_length) { > - cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = iops_max_length; > + if (arg->has_iops_max_length) { > + cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length; > } > - if (has_iops_rd_max_length) { > - cfg.buckets[THROTTLE_OPS_READ].burst_length = iops_rd_max_length; > + if (arg->has_iops_rd_max_length) { > + cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length; > } > - if (has_iops_wr_max_length) { > - cfg.buckets[THROTTLE_OPS_WRITE].burst_length = iops_wr_max_length; > + if (arg->has_iops_wr_max_length) { > + cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length; > } > > - if (has_iops_size) { > - cfg.op_size = iops_size; > + if (arg->has_iops_size) { > + cfg.op_size = arg->iops_size; > } > > if (!throttle_is_valid(&cfg, errp)) { > @@ -2739,9 +2707,10 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd, > /* Enable I/O limits if they're not enabled yet, otherwise > * just update the throttling group. */ > if (!blk_get_public(blk)->throttle_state) { > - blk_io_limits_enable(blk, has_group ? group : device); > - } else if (has_group) { > - blk_io_limits_update_group(blk, group); > + blk_io_limits_enable(blk, > + arg->has_group ? arg->group : arg->device); > + } else if (arg->has_group) { > + blk_io_limits_update_group(blk, arg->group); > } > /* Set the new throttling configuration */ > blk_set_io_limits(blk, &cfg); > diff --git a/hmp.c b/hmp.c > index 9836227..a0c3f4e 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1395,42 +1395,17 @@ void hmp_change(Monitor *mon, const QDict *qdict) > void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict) > { > Error *err = NULL; > + BlockIOThrottle 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"), > + }; > > - qmp_block_set_io_throttle(qdict_get_str(qdict, "device"), > - qdict_get_int(qdict, "bps"), > - qdict_get_int(qdict, "bps_rd"), > - qdict_get_int(qdict, "bps_wr"), > - qdict_get_int(qdict, "iops"), > - qdict_get_int(qdict, "iops_rd"), > - qdict_get_int(qdict, "iops_wr"), > - false, /* no burst max via HMP */ > - 0, > - false, > - 0, > - false, > - 0, > - false, > - 0, > - false, > - 0, > - false, > - 0, > - false, /* no burst length via HMP */ > - 0, > - false, > - 0, > - false, > - 0, > - false, > - 0, > - false, > - 0, > - false, > - 0, > - false, /* No default I/O size */ > - 0, > - false, > - NULL, &err); > + qmp_block_set_io_throttle(&throttle, &err); > hmp_handle_error(mon, &err); > }
diff --git a/qapi/block-core.json b/qapi/block-core.json index 98a20d2..26f7c0e 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1312,6 +1312,21 @@ # the device will be removed from its group and the rest of its # members will not be affected. The 'group' parameter is ignored. # +# See BlockIOThrottle for parameter descriptions. +# +# Returns: Nothing on success +# If @device is not a valid block device, DeviceNotFound +# +# Since: 1.1 +## +{ 'command': 'block_set_io_throttle', 'box': true, + 'data': 'BlockIOThrottle' } + +## +# BlockIOThrottle +# +# The parameters for the block_set_io_throttle command. +# # @device: The name of the device # # @bps: total throughput limit in bytes per second @@ -1378,12 +1393,9 @@ # # @group: #optional throttle group name (Since 2.4) # -# Returns: Nothing on success -# If @device is not a valid block device, DeviceNotFound -# # Since: 1.1 ## -{ 'command': 'block_set_io_throttle', +{ 'struct': 'BlockIOThrottle', 'data': { 'device': 'str', 'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int', '*bps_max': 'int', '*bps_rd_max': 'int', diff --git a/blockdev.c b/blockdev.c index cf5afa3..b8db496 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2625,49 +2625,17 @@ fail: } /* throttling disk I/O limits */ -void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd, - int64_t bps_wr, - int64_t iops, - int64_t iops_rd, - int64_t iops_wr, - bool has_bps_max, - int64_t bps_max, - bool has_bps_rd_max, - int64_t bps_rd_max, - bool has_bps_wr_max, - int64_t bps_wr_max, - bool has_iops_max, - int64_t iops_max, - bool has_iops_rd_max, - int64_t iops_rd_max, - bool has_iops_wr_max, - int64_t iops_wr_max, - bool has_bps_max_length, - int64_t bps_max_length, - bool has_bps_rd_max_length, - int64_t bps_rd_max_length, - bool has_bps_wr_max_length, - int64_t bps_wr_max_length, - bool has_iops_max_length, - int64_t iops_max_length, - bool has_iops_rd_max_length, - int64_t iops_rd_max_length, - bool has_iops_wr_max_length, - int64_t iops_wr_max_length, - bool has_iops_size, - int64_t iops_size, - bool has_group, - const char *group, Error **errp) +void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) { ThrottleConfig cfg; BlockDriverState *bs; BlockBackend *blk; AioContext *aio_context; - blk = blk_by_name(device); + blk = blk_by_name(arg->device); if (!blk) { error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); + "Device '%s' not found", arg->device); return; } @@ -2676,59 +2644,59 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd, bs = blk_bs(blk); if (!bs) { - error_setg(errp, "Device '%s' has no medium", device); + error_setg(errp, "Device '%s' has no medium", arg->device); goto out; } throttle_config_init(&cfg); - cfg.buckets[THROTTLE_BPS_TOTAL].avg = bps; - cfg.buckets[THROTTLE_BPS_READ].avg = bps_rd; - cfg.buckets[THROTTLE_BPS_WRITE].avg = bps_wr; + 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 = iops; - cfg.buckets[THROTTLE_OPS_READ].avg = iops_rd; - cfg.buckets[THROTTLE_OPS_WRITE].avg = iops_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 (has_bps_max) { - cfg.buckets[THROTTLE_BPS_TOTAL].max = bps_max; + if (arg->has_bps_max) { + cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max; } - if (has_bps_rd_max) { - cfg.buckets[THROTTLE_BPS_READ].max = bps_rd_max; + if (arg->has_bps_rd_max) { + cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max; } - if (has_bps_wr_max) { - cfg.buckets[THROTTLE_BPS_WRITE].max = bps_wr_max; + if (arg->has_bps_wr_max) { + cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max; } - if (has_iops_max) { - cfg.buckets[THROTTLE_OPS_TOTAL].max = iops_max; + if (arg->has_iops_max) { + cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max; } - if (has_iops_rd_max) { - cfg.buckets[THROTTLE_OPS_READ].max = iops_rd_max; + if (arg->has_iops_rd_max) { + cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max; } - if (has_iops_wr_max) { - cfg.buckets[THROTTLE_OPS_WRITE].max = iops_wr_max; + if (arg->has_iops_wr_max) { + cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max; } - if (has_bps_max_length) { - cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = bps_max_length; + if (arg->has_bps_max_length) { + cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length; } - if (has_bps_rd_max_length) { - cfg.buckets[THROTTLE_BPS_READ].burst_length = bps_rd_max_length; + if (arg->has_bps_rd_max_length) { + cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length; } - if (has_bps_wr_max_length) { - cfg.buckets[THROTTLE_BPS_WRITE].burst_length = bps_wr_max_length; + if (arg->has_bps_wr_max_length) { + cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length; } - if (has_iops_max_length) { - cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = iops_max_length; + if (arg->has_iops_max_length) { + cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length; } - if (has_iops_rd_max_length) { - cfg.buckets[THROTTLE_OPS_READ].burst_length = iops_rd_max_length; + if (arg->has_iops_rd_max_length) { + cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length; } - if (has_iops_wr_max_length) { - cfg.buckets[THROTTLE_OPS_WRITE].burst_length = iops_wr_max_length; + if (arg->has_iops_wr_max_length) { + cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length; } - if (has_iops_size) { - cfg.op_size = iops_size; + if (arg->has_iops_size) { + cfg.op_size = arg->iops_size; } if (!throttle_is_valid(&cfg, errp)) { @@ -2739,9 +2707,10 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd, /* Enable I/O limits if they're not enabled yet, otherwise * just update the throttling group. */ if (!blk_get_public(blk)->throttle_state) { - blk_io_limits_enable(blk, has_group ? group : device); - } else if (has_group) { - blk_io_limits_update_group(blk, group); + blk_io_limits_enable(blk, + arg->has_group ? arg->group : arg->device); + } else if (arg->has_group) { + blk_io_limits_update_group(blk, arg->group); } /* Set the new throttling configuration */ blk_set_io_limits(blk, &cfg); diff --git a/hmp.c b/hmp.c index 9836227..a0c3f4e 100644 --- a/hmp.c +++ b/hmp.c @@ -1395,42 +1395,17 @@ void hmp_change(Monitor *mon, const QDict *qdict) void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict) { Error *err = NULL; + BlockIOThrottle 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"), + }; - qmp_block_set_io_throttle(qdict_get_str(qdict, "device"), - qdict_get_int(qdict, "bps"), - qdict_get_int(qdict, "bps_rd"), - qdict_get_int(qdict, "bps_wr"), - qdict_get_int(qdict, "iops"), - qdict_get_int(qdict, "iops_rd"), - qdict_get_int(qdict, "iops_wr"), - false, /* no burst max via HMP */ - 0, - false, - 0, - false, - 0, - false, - 0, - false, - 0, - false, - 0, - false, /* no burst length via HMP */ - 0, - false, - 0, - false, - 0, - false, - 0, - false, - 0, - false, - 0, - false, /* No default I/O size */ - 0, - false, - NULL, &err); + qmp_block_set_io_throttle(&throttle, &err); hmp_handle_error(mon, &err); }
Now that we can support boxed commands, use it to greatly reduce the number of parameters (and likelihood of getting out of sync) when adjusting throttle parameters. Signed-off-by: Eric Blake <eblake@redhat.com> --- v7: new patch --- qapi/block-core.json | 20 ++++++++-- blockdev.c | 111 +++++++++++++++++++-------------------------------- hmp.c | 45 +++++---------------- 3 files changed, 66 insertions(+), 110 deletions(-)