Block Throttle Group Support
v3 cover Block Throttle Group Support
Hello,
here's the new version of the block throttling patches. After the discussions from the previous thread, this one is significantly rewritten from the original code.
The main change is in the group throttling API, which is -I believe- much simpler now. The code in block.c and throttle.c also needs far fewer changes.
Here's what's new:
- The following calls are no longer part of the API, and are now handled internally:
throttle_group_incref()
throttle_group_unref()
throttle_group_set_token()
throttle_group_token()
throttle_group_lock()
throttle_group_unlock()
- New throttle_group_config() and throttle_group_get_config() calls, that can be called concurrently by the members of the same group.
- bdrv_throttle_group_add/remove() have been merged into throttle_group_register/unregister_bs() and moved to throttle-groups.c.
The result of this is that in the sequence throttle_group_incref() + bdrv_throttle_group_add() + throttle_group_register_bs() only this last calls remains and takes care of everything else.
- The algorithm in bdrv_io_limits_intercept() and friends has been simplified and moved to throttle-groups.c.
- The any_timer_armed array no longer exists. The group throttling code checks the timers directly to see if any of them is pending.
- The tests have been adapted to the new API.
- The code is hopefully better documented now.
There's still the question of what to do with the QMP API to get all throttling groups, but since there is no consensus yet I haven't included it in this series.
And I think that's all.
As usual, comments and feedback are welcome.
Berto
Alberto Garcia (5):
throttle: Add throttle group infrastructure
throttle: Add throttle group infrastructure tests
throttle: Add throttle group support
throttle: add the name of the ThrottleGroup to BlockDeviceInfo
throttle: Update throttle infrastructure copyright
Benoît Canet (1):
throttle: Extract timers from ThrottleState into a separate structure
block.c | 82 ++++----- block/Makefile.objs | 1 + block/qapi.c | 8 +- block/throttle-groups.c | 371 ++++++++++++++++++++++++++++++++++++++++ blockdev.c | 19 +- hmp.c | 10 +- include/block/block.h | 3 +- include/block/block_int.h | 9 +- include/block/throttle-groups.h | 45 +++++ include/qemu/throttle.h | 46 +++-- qapi/block-core.json | 8 +- qemu-options.hx | 1 + qmp-commands.hx | 3 +- tests/test-throttle.c | 119 +++++++++---- util/throttle.c | 81 +++++---- 15 files changed, 665 insertions(+), 141 deletions(-) create mode 100644 block/throttle-groups.c create mode 100644 include/block/throttle-groups.h
Signature · 1 line
2.1.4
PATCH v3 1/6 throttle: Extract timers from ThrottleState into a separate structure
From: Benoît Canet <[email protected]>
Group throttling will share ThrottleState between multiple bs. As a consequence the ThrottleState will be accessed by multiple aio context.
Timers are tied to their aio context so they must go out of the ThrottleState structure.
This commit paves the way for each bs of a common ThrottleState to have its own timer.
--- block.c | 35 ++++++++++++-------- include/block/block_int.h | 1 + include/qemu/throttle.h | 38 ++++++++++++++-------- tests/test-throttle.c | 82 ++++++++++++++++++++++++++--------------------- util/throttle.c | 73 ++++++++++++++++++++++++----------------- 5 files changed, 135 insertions(+), 94 deletions(-)
block.c +21 -14
@@ -130,7 +130,7 @@ void bdrv_set_io_limits(BlockDriverState *bs, {int i; -throttle_config (&bs->throttle_state, cfg); +throttle_config (&bs->throttle_state, &bs->throttle_timers, cfg);for (i =0 ; i <2 ; i++) {qemu_co_enter_next (&bs->throttled_reqs[i]);
@@ -163,7 +163,7 @@ void bdrv_io_limits_disable(BlockDriverState *bs)bdrv_start_throttled_reqs (bs); -throttle_destroy (&bs->throttle_state); +throttle_timers_destroy (&bs->throttle_timers); }static void bdrv_throttle_read_timer_cb (void *opaque)
@@ -182,12 +182,13 @@ static void bdrv_throttle_write_timer_cb(void *opaque)void bdrv_io_limits_enable (BlockDriverState *bs) {assert (!bs->io_limits_enabled); -throttle_init (&bs->throttle_state, -bdrv_get_aio_context (bs), -QEMU_CLOCK_VIRTUAL , - bdrv_throttle_read_timer_cb, - bdrv_throttle_write_timer_cb, - bs); +throttle_init (&bs->throttle_state); +throttle_timers_init (&bs->throttle_timers, +bdrv_get_aio_context (bs), +QEMU_CLOCK_VIRTUAL , + bdrv_throttle_read_timer_cb, + bdrv_throttle_write_timer_cb, + bs); bs->io_limits_enabled = true; }
@@ -201,7 +202,9 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs, bool is_write) {/* does this io must wait */ -bool must_wait =throttle_schedule_timer (&bs->throttle_state, is_write); +bool must_wait =throttle_schedule_timer (&bs->throttle_state, + &bs->throttle_timers, + is_write);/* if must wait or any request of this type throttled queue the IO */ if (must_wait ||
@@ -214,7 +217,8 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,/* if the next request must wait -> do nothing */ -if (throttle_schedule_timer (&bs->throttle_state, is_write)) { +if (throttle_schedule_timer (&bs->throttle_state, &bs->throttle_timers, + is_write)) {return ; }
@@ -2049,6 +2053,9 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,memcpy (&bs_dest->throttle_state, &bs_src->throttle_state,sizeof (ThrottleState)); +memcpy (&bs_dest->throttle_timers, + &bs_src->throttle_timers, +sizeof (ThrottleTimers)); bs_dest->throttled_reqs[0 ] = bs_src->throttled_reqs[0 ]; bs_dest->throttled_reqs[1 ] = bs_src->throttled_reqs[1 ]; bs_dest->io_limits_enabled = bs_src->io_limits_enabled;
@@ -2110,7 +2117,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)assert (QLIST_EMPTY (&bs_new->dirty_bitmaps));assert (bs_new->job ==NULL );assert (bs_new->io_limits_enabled == false); -assert (!throttle_have_timer (&bs_new->throttle_state)); +assert (!throttle_timers_are_initialized (&bs_new->throttle_timers)); tmp = *bs_new; *bs_new = *bs_old;
@@ -2127,7 +2134,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)/* Check a few fields that should remain attached to the device */ assert (bs_new->job ==NULL );assert (bs_new->io_limits_enabled == false); -assert (!throttle_have_timer (&bs_new->throttle_state)); +assert (!throttle_timers_are_initialized (&bs_new->throttle_timers));/* insert the nodes back into the graph node list if needed */ if (bs_new->node_name[0 ] !='\0' ) {
@@ -5793,7 +5800,7 @@ void bdrv_detach_aio_context(BlockDriverState *bs) }if (bs->io_limits_enabled) { -throttle_detach_aio_context (&bs->throttle_state); +throttle_timers_detach_aio_context (&bs->throttle_timers); }if (bs->drv->bdrv_detach_aio_context) { bs->drv->bdrv_detach_aio_context (bs);
@@ -5829,7 +5836,7 @@ void bdrv_attach_aio_context(BlockDriverState *bs, bs->drv->bdrv_attach_aio_context (bs, new_context); }if (bs->io_limits_enabled) { -throttle_attach_aio_context (&bs->throttle_state, new_context); +throttle_timers_attach_aio_context (&bs->throttle_timers, new_context); } QLIST_FOREACH(ban, &bs->aio_notifiers, list) {
include/block/block_int.h +1 -0
@@ -360,6 +360,7 @@ struct BlockDriverState {/* I/O throttling */ ThrottleState throttle_state; +ThrottleTimers throttle_timers;CoQueue throttled_reqs[2 ];bool io_limits_enabled;
include/qemu/throttle.h +24 -14
@@ -65,14 +65,17 @@ typedef struct ThrottleConfig {typedef struct ThrottleState {ThrottleConfig cfg;/* configuration */ int64_t previous_leak;/* timestamp of the last leak done */ -QEMUTimer * timers[2 ];/* timers used to do the throttling */ +}ThrottleState ; + +typedef struct ThrottleTimers { +QEMUTimer *timers[2 ];/* timers used to do the throttling */ QEMUClockType clock_type;/* the clock used */ /* Callbacks */ QEMUTimerCB *read_timer_cb;QEMUTimerCB *write_timer_cb;void *timer_opaque; -}ThrottleState ; +}ThrottleTimers ;/* operations on single leaky buckets */ void throttle_leak_bucket (LeakyBucket *bkt,int64_t delta);
@@ -86,20 +89,23 @@ bool throttle_compute_timer(ThrottleState *ts, int64_t *next_timestamp);/* init/destroy cycle */ -void throttle_init (ThrottleState *ts, -AioContext *aio_context, -QEMUClockType clock_type, -void (read_timer )(void *), -void (write_timer )(void *), -void *timer_opaque); +void throttle_init (ThrottleState *ts); + +void throttle_timers_init (ThrottleTimers *tt, +AioContext *aio_context, +QEMUClockType clock_type, +QEMUTimerCB *read_timer_cb, +QEMUTimerCB *write_timer_cb, +void *timer_opaque); -void throttle_destroy (ThrottleState *ts); +void throttle_timers_destroy (ThrottleTimers *tt); -void throttle_detach_aio_context (ThrottleState *ts); +void throttle_timers_detach_aio_context (ThrottleTimers *tt); -void throttle_attach_aio_context (ThrottleState *ts,AioContext *new_context); +void throttle_timers_attach_aio_context (ThrottleTimers *tt, +AioContext *new_context); -bool throttle_have_timer (ThrottleState *ts); +bool throttle_timers_are_initialized (ThrottleTimers *tt);/* configuration */ bool throttle_enabled (ThrottleConfig *cfg);
@@ -108,12 +114,16 @@ bool throttle_conflicting(ThrottleConfig *cfg);bool throttle_is_valid (ThrottleConfig *cfg); -void throttle_config (ThrottleState *ts,ThrottleConfig *cfg); +void throttle_config (ThrottleState *ts, +ThrottleTimers *tt, +ThrottleConfig *cfg);void throttle_get_config (ThrottleState *ts,ThrottleConfig *cfg);/* usage */ -bool throttle_schedule_timer (ThrottleState *ts,bool is_write); +bool throttle_schedule_timer (ThrottleState *ts, +ThrottleTimers *tt, +bool is_write);void throttle_account (ThrottleState *ts,bool is_write,uint64_t size);
tests/test-throttle.c +46 -36
163 lines not on this page · raw message
util/throttle.c +43 -30
140 lines not on this page · raw message
Signature · 1 line
2.1.4
Re: [Qemu-devel] [PATCH 1/6] throttle: Extract timers from ThrottleState into a separate structure
Quoted 14 lines · Alberto Garcia
From: Benoît Canet <[email protected]>
Group throttling will share ThrottleState between multiple bs. As a consequence the ThrottleState will be accessed by multiple aio context.
Timers are tied to their aio context so they must go out of the ThrottleState structure.
This commit paves the way for each bs of a common ThrottleState to have its own timer.
Signed-off-by Benoit Canet <[email protected]>Signed-off-by Alberto Garcia <[email protected]>
Quoted 7 lines
--- block.c | 35 ++++++++++++-------- include/block/block_int.h | 1 + include/qemu/throttle.h | 38 ++++++++++++++-------- tests/test-throttle.c | 82 ++++++++++++++++++++++++++--------------------- util/throttle.c | 73 ++++++++++++++++++++++++----------------- 5 files changed, 135 insertions(+), 94 deletions(-)
PATCH v3 2/6 throttle: Add throttle group infrastructure
In reply to Alberto Garcia--- block/Makefile.objs | 1 + block/throttle-groups.c | 244 ++++++++++++++++++++++++++++++++++++++++ include/block/block_int.h | 1 + include/block/throttle-groups.h | 41 +++++++ 4 files changed, 287 insertions(+) create mode 100644 block/throttle-groups.c create mode 100644 include/block/throttle-groups.h
block/Makefile.objs +1 -0
@@ -10,6 +10,7 @@ block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o block-obj-$(CONFIG_POSIX) += raw-posix.o block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o block-obj-y += null.o mirror.o +block-obj-y += throttle-groups.o block-obj-y += nbd.o nbd-client.o sheepdog.o block-obj-$(CONFIG_LIBISCSI) += iscsi.o
block/throttle-groups.c new file +244 -0
244 lines not on this page · raw message
include/block/block_int.h +1 -0
@@ -363,6 +363,7 @@ struct BlockDriverState {ThrottleTimers throttle_timers;CoQueue throttled_reqs[2 ];bool io_limits_enabled; +QLIST_ENTRY (BlockDriverState) round_robin;/* I/O stats (display with "info blockstats"). */ BlockAcctStats stats;
include/block/throttle-groups.h new file +41 -0
41 lines not on this page · raw message
Signature · 1 line
2.1.4
Re: [Qemu-devel] [PATCH 2/6] throttle: Add throttle group infrastructure
block/throttle-groups.c line 39
Quoted 2 lines · Alberto Garcia
block/throttle-groups.c new file +2
-0+typedef struct ThrottleGroup { + char *name; /* This is constant during the lifetime of the group */
Is this also protected by throttle_groups_lock?
I guess throttle_groups_lock must be held in order to read this field - otherwise there is a risk that the object is freed unless you have already incremented the refcount.
Review
Quoted 2 lines
(quoted diff) +2
-0+ + QemuMutex lock;
What does this lock protect? I guess 'ts', 'head', and 'tokens' fields. Please document it.
Quoted 3 lines
block/throttle-groups.c new file +3
-0+ThrottleState ts; + QLIST_HEAD(, BlockDriverState) head; +BlockDriverState *tokens[2 ];
Re: [Qemu-devel] [PATCH 2/6] throttle: Add throttle group infrastructure
Quoted 8 lines · Stefan Hajnoczi
block/throttle-groups.c line 39
Quoted 2 lines
block/throttle-groups.c new file +2
-0+typedef struct ThrottleGroup { + char *name; /* This is constant during the lifetime of the group */Is this also protected by
throttle_groups_lock?I guess
throttle_groups_lockmust be held in order to read this field - otherwise there is a risk that the object is freed unless you have already incremented the refcount.
The creation and destruction of ThrottleGroup objects are protected by throttle_groups_lock. That includes handling the memory for that field and its contents.
Once the ThrottleGroup is created the 'name' field doesn't need any additional locking since it remains constant during the lifetime of the group.
The only way to read it from the outside is throttle_group_get_name() and that's safe (until you release the reference to the group, that is).
Quoted 4 lines
Quoted 1 line
+ QemuMutex lock;
What does this lock protect? I guess 'ts', 'head', and 'tokens' fields. Please document it.
I thought it was obvious in the code but since you ask I guess it's not :) I can add an additional comment.
Berto
Re: [Qemu-devel] [PATCH 2/6] throttle: Add throttle group infrastructure
Quoted 15 lines · Alberto Garcia
Quoted 8 lines · Stefan Hajnoczi
block/throttle-groups.c line 39
Quoted 2 lines
block/throttle-groups.c new file +2
-0+typedef struct ThrottleGroup { + char *name; /* This is constant during the lifetime of the group */Is this also protected by
throttle_groups_lock?I guess
throttle_groups_lockmust be held in order to read this field - otherwise there is a risk that the object is freed unless you have already incremented the refcount.The creation and destruction of ThrottleGroup objects are protected by
throttle_groups_lock. That includes handling the memory for that field and its contents.
Quoted 7 lines
Once the ThrottleGroup is created the 'name' field doesn't need any additional locking since it remains constant during the lifetime of the group.
The only way to read it from the outside is
throttle_group_get_name()and that's safe (until you release the reference to the group, that is).
Right, the race condition is when the group is released.
Looking at this again, the assumption isn't that throttle_groups_lock is held. The AioContext lock is held by throttle_group_get_name() users and that's why there is no race when releasing the reference.
If someone adds a throttle_group_get_name() caller in the future without holding AioContext, then we'd be in trouble. That is why documenting the locking constraints is useful.
Stefan
Re: [Qemu-devel] [PATCH 2/6] throttle: Add throttle group infrastructure
Quoted 24 lines
Quoted 14 lines
Quoted 9 lines
block/throttle-groups.c line 39
Quoted 3 lines
block/throttle-groups.c new file +2
-0+typedef struct ThrottleGroup { + char *name; /* This is constant during the lifetime of the group */Is this also protected by
throttle_groups_lock?I guess
throttle_groups_lockmust be held in order to read this field - otherwise there is a risk that the object is freed unless you have already incremented the refcount.The creation and destruction of ThrottleGroup objects are protected by
throttle_groups_lock. That includes handling the memory for that field and its contents.Quoted 7 lines
Once the ThrottleGroup is created the 'name' field doesn't need any additional locking since it remains constant during the lifetime of the group.
The only way to read it from the outside is
throttle_group_get_name()and that's safe (until you release the reference to the group, that is).Right, the race condition is when the group is released.
Quoted 8 lines
Looking at this again, the assumption isn't that
throttle_groups_lockis held. The AioContext lock is held bythrottle_group_get_name()users and that's why there is no race when releasing the reference.If someone adds a
throttle_group_get_name()caller in the future without holding AioContext, then we'd be in trouble. That is why documenting the locking constraints is useful.
But that would only happen if you access bs->throttle_state without holding bs's AioContext. I understand that it's implicit that you should hold the context there.
Maybe I can update the throttle_group_* API to use BlockDriverState in all cases instead of ThrottleState, it's probably more clear like that.
Berto
PATCH v3 3/6 throttle: Add throttle group infrastructure tests
In reply to Alberto Garcia--- tests/test-throttle.c | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-)
tests/test-throttle.c +35 -2
@@ -1,10 +1,12 @@/* * Throttle infrastructure tests * -* Copyright Nodalink, SARL. 2013 +* Copyright Nodalink, EURL. 2013-2014 +* Copyright Igalia, S.L. 2015 * * Authors: -* Benoît Canet <[email protected]> +* Benoît Canet <[email protected]> +* Alberto Garcia <[email protected]> * * This work is licensed under the terms of the GNU LGPL, version 2 or later. * See the COPYING.LIB file in the top-level directory.
@@ -15,6 +17,7 @@#include "block/aio.h" #include "qemu/throttle.h" #include "qemu/error-report.h" +#include "block/throttle-groups.h" static AioContext *ctx;static LeakyBucket bkt;
@@ -500,6 +503,35 @@ static void test_accounting(void) (64.0 / 13))); } +static void test_groups (void ) +{ +BlockDriverState *bdrv1, *bdrv2, *bdrv3; + + bdrv1 =bdrv_new (); + bdrv2 =bdrv_new (); + bdrv3 =bdrv_new (); + +g_assert (bdrv1->throttle_state ==NULL ); +g_assert (bdrv2->throttle_state ==NULL ); +g_assert (bdrv3->throttle_state ==NULL ); + +throttle_group_register_bs (bdrv1,"bar" ); +throttle_group_register_bs (bdrv2,"foo" ); +throttle_group_register_bs (bdrv3,"bar" ); + +g_assert (!strcmp (throttle_group_get_name (bdrv1->throttle_state),"bar" )); +g_assert (!strcmp (throttle_group_get_name (bdrv2->throttle_state),"foo" )); +g_assert (bdrv1->throttle_state == bdrv3->throttle_state); + +throttle_group_unregister_bs (bdrv1); +throttle_group_unregister_bs (bdrv2); +throttle_group_unregister_bs (bdrv3); + +g_assert (bdrv1->throttle_state ==NULL ); +g_assert (bdrv2->throttle_state ==NULL ); +g_assert (bdrv3->throttle_state ==NULL ); +} +int main (int argc,char **argv) {GSource *src;
@@ -533,6 +565,7 @@ int main(int argc, char **argv)g_test_add_func ("/throttle/config/is_valid" , test_is_valid);g_test_add_func ("/throttle/config_functions" , test_config_functions);g_test_add_func ("/throttle/accounting" , test_accounting); +g_test_add_func ("/throttle/groups" , test_groups);return g_test_run (); }
Signature · 1 line
2.1.4
Re: [Qemu-devel] [PATCH 3/6] throttle: Add throttle group infrastructure tests
Quoted 4 lines · Alberto Garcia
Signed-off-by Alberto Garcia <[email protected]>--- tests/test-throttle.c | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-)
PATCH v3 4/6 throttle: Add throttle group support
In reply to Alberto GarciaThe throttle group support use a cooperative round robin scheduling algorithm.
The principles of the algorithm are simple:
- Each BDS of the group is used as a token in a circular way.
- The active BDS computes if a wait must be done and arms the right timer.
- If a wait must be done the token timer will be armed so the token will become the next active BDS.
--- block.c | 65 ++++++++++---------- block/qapi.c | 5 +- block/throttle-groups.c | 127 ++++++++++++++++++++++++++++++++++++++++ blockdev.c | 19 +++++- hmp.c | 4 +- include/block/block.h | 3 +- include/block/block_int.h | 9 +-- include/block/throttle-groups.h | 4 ++ qapi/block-core.json | 4 +- qemu-options.hx | 1 + qmp-commands.hx | 3 +- 11 files changed, 198 insertions(+), 46 deletions(-)
block.c +31 -34
@@ -36,6 +36,7 @@#include "qmp-commands.h" #include "qemu/timer.h" #include "qapi-event.h" +#include "block/throttle-groups.h" #ifdef CONFIG_BSD #include <sys/types.h>
@@ -130,7 +131,7 @@ void bdrv_set_io_limits(BlockDriverState *bs, {int i; -throttle_config (&bs->throttle_state, &bs->throttle_timers, cfg); +throttle_group_config (bs->throttle_state, &bs->throttle_timers, cfg);for (i =0 ; i <2 ; i++) {qemu_co_enter_next (&bs->throttled_reqs[i]);
@@ -160,9 +161,8 @@ static bool bdrv_start_throttled_reqs(BlockDriverState *bs)void bdrv_io_limits_disable (BlockDriverState *bs) { bs->io_limits_enabled = false; -bdrv_start_throttled_reqs (bs); - +throttle_group_unregister_bs (bs);throttle_timers_destroy (&bs->throttle_timers); }
@@ -179,10 +179,11 @@ static void bdrv_throttle_write_timer_cb(void *opaque) }/* should be called before bdrv_set_io_limits if a limit is set */ -void bdrv_io_limits_enable (BlockDriverState *bs) +void bdrv_io_limits_enable (BlockDriverState *bs,const char *group) {assert (!bs->io_limits_enabled); -throttle_init (&bs->throttle_state); + +throttle_group_register_bs (bs, group ? group :bdrv_get_device_name (bs)); throttle_timers_init(&bs->throttle_timers, bdrv_get_aio_context(bs), QEMU_CLOCK_VIRTUAL,
@@ -192,6 +193,23 @@ void bdrv_io_limits_enable(BlockDriverState *bs) bs->io_limits_enabled = true; } +void bdrv_io_limits_update_group (BlockDriverState *bs,const char *group) +{ +/* this bs is not part of any group */ +if (!bs->throttle_state) { +return ; + } + +/* this bs is a part of the same group than the one we want */ +if (!g_strcmp0 (throttle_group_get_name (bs->throttle_state), group)) { +return ; + } + +/* need to change the group this bs belong to */ +bdrv_io_limits_disable (bs); +bdrv_io_limits_enable (bs, group); +} +/* This function makes an IO wait if needed * * @nb_sectors: the number of sectors of the IO
@@ -201,29 +219,7 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,unsigned int bytes, bool is_write) { -/* does this io must wait */ -bool must_wait =throttle_schedule_timer (&bs->throttle_state, - &bs->throttle_timers, - is_write); - -/* if must wait or any request of this type throttled queue the IO */ -if (must_wait || - !qemu_co_queue_empty (&bs->throttled_reqs[is_write])) { -qemu_co_queue_wait (&bs->throttled_reqs[is_write]); - } - -/* the IO will be executed, do the accounting */ -throttle_account (&bs->throttle_state, is_write, bytes); - - -/* if the next request must wait -> do nothing */ -if (throttle_schedule_timer (&bs->throttle_state, &bs->throttle_timers, - is_write)) { -return ; - } - -/* else queue next request for execution */ -qemu_co_queue_next (&bs->throttled_reqs[is_write]); +throttle_group_io_limits_intercept (bs ,bytes ,is_write ); }size_t bdrv_opt_mem_align (BlockDriverState *bs)
@@ -2050,15 +2046,16 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, bs_dest->enable_write_cache = bs_src->enable_write_cache;/* i/o throttled req */ -memcpy (&bs_dest->throttle_state, - &bs_src->throttle_state, -sizeof (ThrottleState)); + bs_dest->throttle_state = bs_src->throttle_state, + bs_dest->io_limits_enabled = bs_src->io_limits_enabled; + bs_dest->throttled_reqs[0 ] = bs_src->throttled_reqs[0 ]; + bs_dest->throttled_reqs[1 ] = bs_src->throttled_reqs[1 ]; +memcpy (&bs_dest->round_robin, + &bs_src->round_robin, +sizeof (bs_dest->round_robin));memcpy (&bs_dest->throttle_timers, &bs_src->throttle_timers,sizeof (ThrottleTimers)); - bs_dest->throttled_reqs[0 ] = bs_src->throttled_reqs[0 ]; - bs_dest->throttled_reqs[1 ] = bs_src->throttled_reqs[1 ]; - bs_dest->io_limits_enabled = bs_src->io_limits_enabled;/* r/w error */ bs_dest->on_read_error = bs_src->on_read_error;
block/qapi.c +4 -1
@@ -24,6 +24,7 @@#include "block/qapi.h" #include "block/block_int.h" +#include "block/throttle-groups.h" #include "block/write-threshold.h" #include "qmp-commands.h" #include "qapi-visit.h"
@@ -63,7 +64,9 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)if (bs->io_limits_enabled) {ThrottleConfig cfg; -throttle_get_config (&bs->throttle_state, &cfg); + +throttle_group_get_config (bs->throttle_state, &cfg); + info->bps = cfg.buckets[THROTTLE_BPS_TOTAL ].avg; info->bps_rd = cfg.buckets[THROTTLE_BPS_READ ].avg; info->bps_wr = cfg.buckets[THROTTLE_BPS_WRITE ].avg;
block/throttle-groups.c +127 -0
139 lines not on this page · raw message
blockdev.c +16 -3
@@ -357,6 +357,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,const char *id;bool has_driver_specific_opts;BlockdevDetectZeroesOptions detect_zeroes; +const char *throttling_group;/* Check common options by copying from bs_opts to opts, all other options * stay in bs_opts for processing by bdrv_open(). */
@@ -459,6 +460,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, cfg.op_size =qemu_opt_get_number (opts,"throttling.iops-size" ,0 ); + throttling_group =qemu_opt_get (opts,"throttling.group" ); +if (!check_throttle_config (&cfg, &error)) {error_propagate (errp, error); goto early_err;
@@ -547,7 +550,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,/* disk I/O throttling */ if (throttle_enabled (&cfg)) { -bdrv_io_limits_enable (bs); +bdrv_io_limits_enable (bs, throttling_group);bdrv_set_io_limits (bs, &cfg); }
@@ -711,6 +714,8 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) { "iops_size", "throttling.iops-size" }, + { "group", "throttling.group" }, + { "readonly", "read-only" }, };
@@ -1877,7 +1882,9 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,bool has_iops_wr_max, int64_t iops_wr_max, bool has_iops_size, - int64_t iops_size, Error **errp) + int64_t iops_size, + bool has_group, + const char *group, Error **errp) { ThrottleConfig cfg;BlockDriverState *bs;
@@ -1929,9 +1936,11 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,aio_context_acquire (aio_context);if (!bs->io_limits_enabled &&throttle_enabled (&cfg)) { -bdrv_io_limits_enable (bs); +bdrv_io_limits_enable (bs, has_group ? group :NULL ); }else if (bs->io_limits_enabled && !throttle_enabled (&cfg)) {bdrv_io_limits_disable (bs); + }else if (bs->io_limits_enabled &&throttle_enabled (&cfg)) { +bdrv_io_limits_update_group (bs, has_group ? group :NULL ); }if (bs->io_limits_enabled) {
@@ -2991,6 +3000,10 @@ QemuOptsList qemu_common_drive_opts = { .type = QEMU_OPT_NUMBER, .help = "when limiting by iops max size of an I/O in bytes", },{ + .name = "throttling.group", + .type = QEMU_OPT_STRING, + .help = "name of the block throttling group", + },{ .name = "copy-on-read", .type = QEMU_OPT_BOOL, .help = "copy read data from backing file into image file",
hmp.c +3 -1
@@ -1261,7 +1261,9 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict) false,0 , false,/* No default I/O size */ - 0, &err); +0 , + false, + NULL, &err);hmp_handle_error (mon, &err); }
include/block/block.h +2 -1
@@ -162,8 +162,9 @@ void bdrv_stats_print(Monitor *mon, const QObject *data);void bdrv_info_stats (Monitor *mon,QObject **ret_data);/* disk I/O throttling */ -void bdrv_io_limits_enable (BlockDriverState *bs); +void bdrv_io_limits_enable (BlockDriverState *bs,const char *group);void bdrv_io_limits_disable (BlockDriverState *bs); +void bdrv_io_limits_update_group (BlockDriverState *bs,const char *group);void bdrv_init (void );void bdrv_init_with_whitelist (void );
include/block/block_int.h +5 -4
@@ -358,12 +358,13 @@ struct BlockDriverState {/* number of in-flight serialising requests */ unsigned int serialising_in_flight; -/* I/O throttling */ -ThrottleState throttle_state; -ThrottleTimers throttle_timers; -CoQueue throttled_reqs[2 ]; +/* I/O throttling - following elements protected by ThrottleGroup lock */ +ThrottleState *throttle_state;bool io_limits_enabled; +CoQueue throttled_reqs[2 ];QLIST_ENTRY (BlockDriverState) round_robin; +/* timers have their own locking */ +ThrottleTimers throttle_timers;/* I/O stats (display with "info blockstats"). */ BlockAcctStats stats;
include/block/throttle-groups.h +4 -0
@@ -38,4 +38,8 @@ void throttle_group_get_config(ThrottleState *ts, ThrottleConfig *cfg);void throttle_group_register_bs (BlockDriverState *bs,const char *groupname);void throttle_group_unregister_bs (BlockDriverState *bs); +void throttle_group_io_limits_intercept (BlockDriverState *bs, +unsigned int bytes, +bool is_write); +#endif
qapi/block-core.json +3 -1
@@ -989,6 +989,8 @@ # # @iops_size: #optional an I/O size in bytes (Since 1.7) # +# @group: #optional throttle group name (Since 2.3) +# # Returns: Nothing on success # If @device is not a valid block device, DeviceNotFound #
@@ -1000,7 +1002,7 @@ '*bps_max': 'int', '*bps_rd_max': 'int', '*bps_wr_max': 'int', '*iops_max': 'int', '*iops_rd_max': 'int', '*iops_wr_max': 'int', - '*iops_size': 'int' } } + '*iops_size': 'int', '*group': 'str' } } ## # @block-stream:
qemu-options.hx +1 -0
@@ -454,6 +454,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive, " [[,bps_max=bm]|[[,bps_rd_max=rm][,bps_wr_max=wm]]]\n" " [[,iops_max=im]|[[,iops_rd_max=irm][,iops_wr_max=iwm]]]\n" " [[,iops_size=is]]\n" + " [[,group=g]]\n" " use 'file' as a drive image\n", QEMU_ARCH_ALL) STEXI @item -drive @var{option}[,@var{option}[,@var{option}[,...]]]
qmp-commands.hx +2 -1
@@ -1704,7 +1704,7 @@ EQMP { .name = "block_set_io_throttle", - .args_type = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l,bps_max:l?,bps_rd_max:l?,bps_wr_max:l?,iops_max:l?,iops_rd_max:l?,iops_wr_max:l?,iops_size:l?", + .args_type = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l,bps_max:l?,bps_rd_max:l?,bps_wr_max:l?,iops_max:l?,iops_rd_max:l?,iops_wr_max:l?,iops_size:l?,group:s?", .mhandler.cmd_new = qmp_marshal_input_block_set_io_throttle, },
@@ -1730,6 +1730,7 @@ Arguments: - "iops_rd_max": read I/O operations max (json-int) - "iops_wr_max": write I/O operations max (json-int) - "iops_size": I/O size in bytes when limiting (json-int) +- "group": throttle group name (json-string) Example:
Signature · 1 line
2.1.4
Re: [Qemu-devel] [PATCH 4/6] throttle: Add throttle group support
block.c static void bdrv_throttle_write_timer_cb(void *opaque) · line 186
Quoted 11 lines · Alberto Garcia
block.c +3
-2@@ -179,10 +179,11 @@ static void bdrv_throttle_write_timer_cb(void *opaque) }/* should be called before bdrv_set_io_limits if a limit is set */ -void bdrv_io_limits_enable (BlockDriverState *bs) +void bdrv_io_limits_enable (BlockDriverState *bs,const char *group) {assert (!bs->io_limits_enabled); -throttle_init (&bs->throttle_state); + +throttle_group_register_bs (bs, group ? group :bdrv_get_device_name (bs));
Please don't allow a NULL group argument. blockdev_init() should pick the group name instead of requiring bdrv_io_limits_enable() to generate a unique name.
This way we avoid silently adding all BDS without a BlockBackend to a "" throttling group. I think that's a bug waiting to happen :).
Quoted 19 lines
block.c +0
-15@@ -201,29 +219,7 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,unsigned int bytes, bool is_write) { -/* does this io must wait */ -bool must_wait =throttle_schedule_timer (&bs->throttle_state, - &bs->throttle_timers, - is_write); - -/* if must wait or any request of this type throttled queue the IO */ -if (must_wait || - !qemu_co_queue_empty (&bs->throttled_reqs[is_write])) { -qemu_co_queue_wait (&bs->throttled_reqs[is_write]); - } - -/* the IO will be executed, do the accounting */ -throttle_account (&bs->throttle_state, is_write, bytes); - -
block.c static void bdrv_io_limits_intercept(BlockDriverState *bs, · line 223
Quoted 10 lines
block.c +1
-8-/* if the next request must wait -> do nothing */ -if (throttle_schedule_timer (&bs->throttle_state, &bs->throttle_timers, - is_write)) { -return ; - } - -/* else queue next request for execution */ -qemu_co_queue_next (&bs->throttled_reqs[is_write]); +throttle_group_io_limits_intercept (bs, bytes, is_write); }
bdrv_io_limits_intercept() is no longer useful. Can you replace bdrv_io_limits_intercept() calls with throttle_group_io_limits_intercept() to eliminate this indirection?
Quoted 6 lines
block.c +0
-0size_t bdrv_opt_mem_align (BlockDriverState *bs)@@ -2050,15 +2046,16 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, bs_dest->enable_write_cache = bs_src->enable_write_cache;/* i/o throttled req */
block.c static void bdrv_move_feature_fields(BlockDriverState *bs_dest, · line 2055
Quoted 10 lines
block.c +7
-3-memcpy (&bs_dest->throttle_state, - &bs_src->throttle_state, -sizeof (ThrottleState)); + bs_dest->throttle_state = bs_src->throttle_state, + bs_dest->io_limits_enabled = bs_src->io_limits_enabled; + bs_dest->throttled_reqs[0 ] = bs_src->throttled_reqs[0 ]; + bs_dest->throttled_reqs[1 ] = bs_src->throttled_reqs[1 ]; +memcpy (&bs_dest->round_robin, + &bs_src->round_robin, +sizeof (bs_dest->round_robin));
The bdrv_swap() code is tricky to think about...I think this is okay because bs_dest and bs_src linked list pointers will be unchanged (although they are now pointing to a different block driver instance).
Just highlighting this in case anyone else spots a problem with the pointer swapping.
block/throttle-groups.c static BlockDriverState *throttle_group_next_bs(BlockDriverState *bs) · line 156
Quoted 11 lines
block/throttle-groups.c +7
-0@@ -145,6 +147,131 @@ static BlockDriverState *throttle_group_next_bs(BlockDriverState *bs)return next; } +/* Return the next BlockDriverState in the round-robin sequence with +* pending I/O requests. +* +* @bs: the current BlockDriverState +* @is_write: the type of operation (read/write) +* @ret: the next BlockDriverState with pending requests, or bs +* if there is none.
Assumes tg->lock is held. It's helpful to document this.
Re: [Qemu-devel] [PATCH 4/6] throttle: Add throttle group support
In reply to Alberto GarciaQuoted 10 lines · Alberto Garcia
block/throttle-groups.c +10
-0+/* Return the next BlockDriverState in the round-robin sequence with +* pending I/O requests. +* +* @bs: the current BlockDriverState +* @is_write: the type of operation (read/write) +* @ret: the next BlockDriverState with pending requests, or bs +* if there is none. +*/ +static BlockDriverState *next_throttle_token (BlockDriverState *bs, +bool is_write)
block/throttle-groups.c static BlockDriverState *throttle_group_next_bs(BlockDriverState *bs) · line 169
Quoted 10 lines
block/throttle-groups.c +10
-0+{ +ThrottleGroup *tg =container_of (bs->throttle_state, ThrottleGroup, ts); +BlockDriverState *token, *start; + + start = token = tg->tokens[is_write]; + +/* get next bs round in round robin style */ + token =throttle_group_next_bs (token); +while (token != start && +qemu_co_queue_empty (&token->throttled_reqs[is_write])) {
It's not safe to access bs->throttled_reqs[]. There are many of other places that access bs->throttled_reqs[] without holding tg->lock (e.g. block.c).
throttled_reqs needs to be protected by tg->lock in order for this to work.
block/throttle-groups.c static BlockDriverState *throttle_group_next_bs(BlockDriverState *bs) · line 229
Quoted 11 lines
block/throttle-groups.c +11
-0+/* Check if an I/O request needs to be throttled, wait and set a timer +* if necessary, and schedule the next request using a round robin +* algorithm. +* +* @bs: the current BlockDriverState +* @bytes: the number of bytes for this I/O +* @is_write: the type of operation (read/write) +*/ +void throttle_group_io_limits_intercept (BlockDriverState *bs, +unsigned int bytes, +bool is_write)
This function must be called from coroutine context because it uses qemu_co_queue_wait() and may yield. Please mark the function with coroutine_fn (see include/block/coroutine.h). This serves as documentation.
Quoted 5 lines
block/throttle-groups.c +5
-0+{ +bool must_wait; +BlockDriverState *token; + +ThrottleGroup *tg =container_of (bs->throttle_state, ThrottleGroup, ts);
block/throttle-groups.c static BlockDriverState *throttle_group_next_bs(BlockDriverState *bs) · line 244
Quoted 10 lines
block/throttle-groups.c +10
-0+qemu_mutex_lock (&tg->lock); + +/* First we check if this I/O has to be throttled. */ + token =next_throttle_token (bs, is_write); + must_wait =throttle_group_schedule_timer (token, is_write); + +/* Wait if there's a timer set or queued requests of this type */ +if (must_wait || !qemu_co_queue_empty (&bs->throttled_reqs[is_write])) { +qemu_mutex_unlock (&tg->lock); +qemu_co_queue_wait (&bs->throttled_reqs[is_write]);
Here bs->throttled_reqs[] is used without holding tg->lock. It can race with token->throttled_reqs[] users.
Re: [Qemu-devel] [PATCH 4/6] throttle: Add throttle group support
Quoted 11 lines · Stefan Hajnoczi
block/throttle-groups.c static BlockDriverState *throttle_group_next_bs(BlockDriverState *bs) · line 169
Quoted 4 lines
block/throttle-groups.c +4
-0+/* get next bs round in round robin style */ + token =throttle_group_next_bs (token); +while (token != start && +qemu_co_queue_empty (&token->throttled_reqs[is_write])) {It's not safe to access bs->throttled_reqs[]. There are many of other places that access bs->throttled_reqs[] without holding
tg->lock(e.g.block.c).
throttled_reqsneeds to be protected bytg->lockin order for this to work.
Good catch, but I don't think that's the solution. throttled_reqs cannot be protected by that lock because it must release it before calling qemu_co_queue_wait(&bs->throttled_reqs[is_write]). Otherwise it will cause a deadlock.
My assumption was that throttled_reqs would be protected by the BDS's AioContext lock, but I overlooked the fact that this part of the algorithm needs to access other BDSs' queues so we indeed have a problem.
I will give it a thought to see what I can come up with. Since we only need to check whether other BDSs have pending requests maybe I can implement this with a flag.
Thanks!
Berto
Re: [Qemu-devel] [PATCH 4/6] throttle: Add throttle group support
Quoted 19 lines · Alberto Garcia
Quoted 11 lines · Stefan Hajnoczi
block/throttle-groups.c static BlockDriverState *throttle_group_next_bs(BlockDriverState *bs) · line 169
Quoted 4 lines
block/throttle-groups.c +4
-0+/* get next bs round in round robin style */ + token =throttle_group_next_bs (token); +while (token != start && +qemu_co_queue_empty (&token->throttled_reqs[is_write])) {It's not safe to access bs->throttled_reqs[]. There are many of other places that access bs->throttled_reqs[] without holding
tg->lock(e.g.block.c).
throttled_reqsneeds to be protected bytg->lockin order for this to work.Good catch, but I don't think that's the solution.
throttled_reqscannot be protected by that lock because it must release it before callingqemu_co_queue_wait(&bs->throttled_reqs[is_write]). Otherwise it will cause a deadlock.
Quoted 8 lines
My assumption was that
throttled_reqswould be protected by the BDS's AioContext lock, but I overlooked the fact that this part of the algorithm needs to access other BDSs' queues so we indeed have a problem.I will give it a thought to see what I can come up with. Since we only need to check whether other BDSs have pending requests maybe I can implement this with a flag.
A flag is a good solution since it is not possible to acquire other BDS' AioContext lock from arbitrary block layer code (it's only allowed when the QEMU global mutex is held).
Stefan
PATCH v3 5/6 throttle: add the name of the ThrottleGroup to BlockDeviceInfo
In reply to Alberto Garcia--- block/qapi.c | 3 +++ hmp.c | 6 ++++-- qapi/block-core.json | 4 +++- 3 files changed, 10 insertions(+), 3 deletions(-)
block/qapi.c +3 -0
@@ -91,6 +91,9 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs) info->has_iops_size = cfg.op_size; info->iops_size = cfg.op_size; + + info->has_group = true; + info->group =g_strdup (throttle_group_get_name (bs->throttle_state)); } info->write_threshold =bdrv_write_threshold_get (bs);
hmp.c +4 -2
@@ -374,7 +374,8 @@ static void print_block_info(Monitor *mon, BlockInfo *info," iops_max=%" PRId64" iops_rd_max=%" PRId64" iops_wr_max=%" PRId64 -" iops_size=%" PRId64"\n" , +" iops_size=%" PRId64 +" group=%s\n" , inserted->bps, inserted->bps_rd, inserted->bps_wr,
@@ -387,7 +388,8 @@ static void print_block_info(Monitor *mon, BlockInfo *info, inserted->iops_max, inserted->iops_rd_max, inserted->iops_wr_max, - inserted->iops_size); + inserted->iops_size, + inserted->group); }if (verbose) {
qapi/block-core.json +3 -1
@@ -255,6 +255,8 @@ # # @iops_size: #optional an I/O size in bytes (Since 1.7) # +# @group: #optional throttle group name (Since 2.3) +# # @cache: the cache mode used for the block device (since: 2.3) # # @write_threshold: configured write threshold for the device.
@@ -274,7 +276,7 @@ '*bps_max': 'int', '*bps_rd_max': 'int', '*bps_wr_max': 'int', '*iops_max': 'int', '*iops_rd_max': 'int', '*iops_wr_max': 'int', - '*iops_size': 'int', 'cache': 'BlockdevCacheInfo', + '*iops_size': 'int', '*group': 'str', 'cache': 'BlockdevCacheInfo', 'write_threshold': 'int' } } ##
Signature · 1 line
2.1.4
PATCH v3 6/6 throttle: Update throttle infrastructure copyright
In reply to Alberto Garcia--- include/qemu/throttle.h | 8 +++++--- util/throttle.c | 8 +++++--- 2 files changed, 10 insertions(+), 6 deletions(-)
include/qemu/throttle.h +5 -3
@@ -1,10 +1,12 @@/* * QEMU throttling infrastructure * -* Copyright (C) Nodalink, SARL. 2013 +* Copyright (C) Nodalink, EURL. 2013-2014 +* Copyright (C) Igalia, S.L. 2015 * -* Author: -* Benoît Canet <[email protected]> +* Authors: +* Benoît Canet <[email protected]> +* Alberto Garcia <[email protected]> * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License as
util/throttle.c +5 -3
@@ -1,10 +1,12 @@/* * QEMU throttling infrastructure * -* Copyright (C) Nodalink, SARL. 2013 +* Copyright (C) Nodalink, EURL. 2013-2014 +* Copyright (C) Igalia, S.L. 2015 * -* Author: -* Benoît Canet <[email protected]> +* Authors: +* Benoît Canet <[email protected]> +* Alberto Garcia <[email protected]> * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License as
Signature · 1 line
2.1.4
Quoted 1 line · Alberto Garcia
Here's what's new:
Thanks for the cleanup! The locking isn't safe yet but the series is in pretty good shape overall. Please see comments on individual patches for details.
Stefan