Minor throttling updates
cover Minor throttling updates
Here's a couple of patches for the throttling code. I think the commit messages are clear enough, but if you have any comments or questions I'll be glad to hear them.
Berto
Alberto Garcia (2):
throttle: Check for pending requests in throttle_group_unregister_bs()
throttle: Use bs->throttle_state instead of bs->io_limits_enabled
block.c | 6 +++--- block/qapi.c | 2 +- block/throttle-groups.c | 7 +++++++ blockdev.c | 4 ++-- include/block/block_int.h | 5 ++++- 5 files changed, 17 insertions(+), 7 deletions(-)
Signature · 1 line
2.6.2
PATCH 1/2 throttle: Check for pending requests in throttle_group_unregister_bs()
throttle_group_unregister_bs() removes a BlockDriverState from its throttling group and destroys the timers. This means that there must be no pending throttled requests at that point (because it would be impossible to complete them), so the caller has to drain them first.
At the moment throttle_group_unregister_bs() is only called from bdrv_io_limits_disable(), which already takes care of draining the requests, so there's nothing to worry about, but this patch makes this invariant explicit in the documentation and adds the relevant assertions.
--- block/throttle-groups.c | 7 +++++++ 1 file changed, 7 insertions(+)
block/throttle-groups.c +7 -0
@@ -437,6 +437,9 @@ void throttle_group_register_bs(BlockDriverState *bs, const char *groupname)* list, destroying the timers and setting the throttle_state pointer * to NULL. * +* The BlockDriverState must not have pending throttled requests, so +* the caller has to drain them first. +* * The group will be destroyed if it's empty after this operation. * * @bs: the BlockDriverState to remove
@@ -446,6 +449,10 @@ void throttle_group_unregister_bs(BlockDriverState *bs)ThrottleGroup *tg =container_of (bs->throttle_state, ThrottleGroup, ts);int i; +assert (bs->pending_reqs[0 ] ==0 && bs->pending_reqs[1 ] ==0 ); +assert (qemu_co_queue_empty (&bs->throttled_reqs[0 ])); +assert (qemu_co_queue_empty (&bs->throttled_reqs[1 ])); +qemu_mutex_lock (&tg->lock);for (i =0 ; i <2 ; i++) {if (tg->tokens[i] == bs) {
Signature · 1 line
2.6.2
PATCH 2/2 throttle: Use bs->throttle_state instead of bs->io_limits_enabled
In reply to Alberto GarciaThere are two ways to check for I/O limits in a BlockDriverState:
- bs->throttle_state: if this pointer is not NULL, it means that this BDS is member of a throttling group, its ThrottleTimers structure has been initialized and its I/O limits are ready to be applied.
- bs->io_limits_enabled: if true it means that the throttle_state pointer is valid _and_ the limits are currently enabled.
The latter is used in several places to check whether a BDS has I/O limits configured, but what it really checks is whether requests are being throttled or not. For example, io_limits_enabled can be temporarily set to false in cases like bdrv_read_unthrottled() without otherwise touching the throtting configuration of that BDS.
This patch replaces bs->io_limits_enabled with bs->throttle_state in all cases where what we really want to check is the existence of I/O limits, not whether they are currently enabled or not.
--- block.c | 6 +++--- block/qapi.c | 2 +- blockdev.c | 4 ++-- include/block/block_int.h | 5 ++++- 4 files changed, 10 insertions(+), 7 deletions(-)
block.c +3 -3
@@ -1901,7 +1901,7 @@ void bdrv_close(BlockDriverState *bs) }/* Disable I/O limits and drain all pending throttled requests */ -if (bs->io_limits_enabled) { +if (bs->throttle_state) {bdrv_io_limits_disable (bs); }
@@ -3706,7 +3706,7 @@ void bdrv_detach_aio_context(BlockDriverState *bs) baf->detach_aio_context (baf->opaque); } -if (bs->io_limits_enabled) { +if (bs->throttle_state) {throttle_timers_detach_aio_context (&bs->throttle_timers); }if (bs->drv->bdrv_detach_aio_context) {
@@ -3742,7 +3742,7 @@ void bdrv_attach_aio_context(BlockDriverState *bs,if (bs->drv->bdrv_attach_aio_context) { bs->drv->bdrv_attach_aio_context (bs, new_context); } -if (bs->io_limits_enabled) { +if (bs->throttle_state) {throttle_timers_attach_aio_context (&bs->throttle_timers, new_context); }
block/qapi.c +1 -1
@@ -64,7 +64,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs, Error **errp) info->backing_file_depth =bdrv_get_backing_file_depth (bs); info->detect_zeroes = bs->detect_zeroes; -if (bs->io_limits_enabled) { +if (bs->throttle_state) {ThrottleConfig cfg;throttle_group_get_config (bs, &cfg);
blockdev.c +2 -2
@@ -2161,14 +2161,14 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,if (throttle_enabled (&cfg)) {/* Enable I/O limits if they're not enabled yet, otherwise * just update the throttling group. */ -if (!bs->io_limits_enabled) { +if (!bs->throttle_state) {bdrv_io_limits_enable (bs, has_group ? group : device); }else if (has_group) {bdrv_io_limits_update_group (bs, group); }/* Set the new throttling configuration */ bdrv_set_io_limits (bs, &cfg); - }else if (bs->io_limits_enabled) { + }else if (bs->throttle_state) {/* If all throttling settings are set to 0, disable I/O limits */ bdrv_io_limits_disable (bs); }
include/block/block_int.h +4 -1
@@ -390,7 +390,10 @@ struct BlockDriverState {/* number of in-flight serialising requests */ unsigned int serialising_in_flight; -/* I/O throttling */ +/* I/O throttling. +* throttle_state tells us if this BDS has I/O limits configured. +* io_limits_enabled tells us if they are currently being +* enforced, but it can be temporarily set to false */ CoQueue throttled_reqs[2 ];bool io_limits_enabled;/* The following fields are protected by the ThrottleGroup lock.
Signature · 1 line
2.6.2
Kevin Wolf
AppliedAm 04.11.2015 um 14:15 hat Alberto Garcia geschrieben:
Quoted 3 lines
Here's a couple of patches for the throttling code. I think the commit messages are clear enough, but if you have any comments or questions I'll be glad to hear them.
Thanks, applied to the block branch.
Kevin