Block Throttle Group Support
v6 cover Block Throttle Group Support
This updates test-throttle.c following Stefan's advice to init the QEMU main loop.
V6:
- Use qemu_init_main_loop() in the throttling tests to make sure that all BDS have an AioContext attached.
V5: https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg05990.html
- Fixed and expanded the tests. There's a new one that checks that configuration changes affect the whole throttling group.
- Refactored the code a bit: new function schedule_next_request()
- any_timer_armed is now reset in throttle_group_config(), it was causing qemu-iotest 093 to fail.
- Documentation improvements
V4: https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg05558.html
- All functions from the ThrottleGroup API now receive a BlockDriverState.
- The creation/destruction of ThrottleTimers is now handled internally.
- bdrv_io_limits_enable() no longer accepts NULL as a valid group name.
- No member of a group can access someone else's throttled_reqs queues anymore.
- No member of a group can access someone else's timers unless they have queued requests.
- After a timer is fired make sure that there was actually a request in the queue, and schedule a new one otherwise.
- Protect bdrv_swap() by locking the throttling group.
- throttle_group_co_io_limits_intercept() is now marked as a coroutine function.
- Documentation updates.
Regards,
Berto
Alberto Garcia (6):
throttle: Add throttle group infrastructure
throttle: Add throttle group infrastructure tests
throttle: Add throttle group support
throttle: acquire the ThrottleGroup lock in bdrv_swap()
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 | 101 ++++----- block/Makefile.objs | 1 + block/qapi.c | 8 +- block/throttle-groups.c | 489 ++++++++++++++++++++++++++++++++++++++++ blockdev.c | 22 +- hmp.c | 10 +- include/block/block.h | 3 +- include/block/block_int.h | 7 +- include/block/throttle-groups.h | 46 ++++ include/qemu/throttle.h | 46 ++-- qapi/block-core.json | 8 +- qemu-options.hx | 1 + qmp-commands.hx | 3 +- tests/test-throttle.c | 161 +++++++++---- util/throttle.c | 81 ++++--- 15 files changed, 823 insertions(+), 164 deletions(-) create mode 100644 block/throttle-groups.c create mode 100644 include/block/throttle-groups.h
Signature · 1 line
2.1.4
PATCH v6 1/7 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 ; }
@@ -2090,6 +2094,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;
@@ -2151,7 +2158,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;
@@ -2168,7 +2175,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' ) {
@@ -5858,7 +5865,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);
@@ -5894,7 +5901,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
@@ -377,6 +377,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
PATCH v6 2/7 throttle: Add throttle group infrastructure
In reply to Alberto Garcia--- block/Makefile.objs | 1 + block/throttle-groups.c | 261 ++++++++++++++++++++++++++++++++++++++++ include/block/block_int.h | 1 + include/block/throttle-groups.h | 39 ++++++ 4 files changed, 302 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 +261 -0
261 lines not on this page · raw message
include/block/block_int.h +1 -0
@@ -380,6 +380,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 +39 -0
@@ -0,0 +1,39 @@ +/* +* QEMU block throttling group infrastructure +* +* Copyright (C) Nodalink, EURL. 2014 +* Copyright (C) Igalia, S.L. 2015 +* +* 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 +* published by the Free Software Foundation; either version 2 or +* (at your option) version 3 of the License. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, see <http://www.gnu.org/licenses/>. +*/ + +#ifndef THROTTLE_GROUPS_H +#define THROTTLE_GROUPS_H + +#include "qemu/throttle.h" +#include "block/block_int.h" + +const char *throttle_group_get_name (BlockDriverState *bs); + +void throttle_group_config (BlockDriverState *bs,ThrottleConfig *cfg); +void throttle_group_get_config (BlockDriverState *bs,ThrottleConfig *cfg); + +void throttle_group_register_bs (BlockDriverState *bs,const char *groupname); +void throttle_group_unregister_bs (BlockDriverState *bs); + +#endif
Signature · 1 line
2.1.4
Re: [Qemu-devel] [PATCH 2/7] throttle: Add throttle group infrastructure
Quoted 9 lines · Alberto Garcia
Signed-off-by Alberto Garcia <[email protected]>--- block/Makefile.objs | 1 + block/throttle-groups.c | 261 ++++++++++++++++++++++++++++++++++++++++ include/block/block_int.h | 1 + include/block/throttle-groups.h | 39 ++++++ 4 files changed, 302 insertions(+) create mode 100644 block/throttle-groups.c create mode 100644 include/block/throttle-groups.h
PATCH v6 3/7 throttle: Add throttle group infrastructure tests
In reply to Alberto Garcia--- tests/test-throttle.c | 79 ++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 69 insertions(+), 10 deletions(-)
tests/test-throttle.c +69 -10
@@ -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,23 +503,78 @@ static void test_accounting(void) (64.0 / 13))); } +static void test_groups (void ) +{ +ThrottleConfig cfg1, cfg2; +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 (bdrv1->throttle_state !=NULL ); +g_assert (bdrv2->throttle_state !=NULL ); +g_assert (bdrv3->throttle_state !=NULL ); + +g_assert (!strcmp (throttle_group_get_name (bdrv1),"bar" )); +g_assert (!strcmp (throttle_group_get_name (bdrv2),"foo" )); +g_assert (bdrv1->throttle_state == bdrv3->throttle_state); + +/* Setting the config of a group member affects the whole group */ +memset (&cfg1,0 ,sizeof (cfg1)); + cfg1.buckets[THROTTLE_BPS_READ ].avg =500000 ; + cfg1.buckets[THROTTLE_BPS_WRITE ].avg =285000 ; + cfg1.buckets[THROTTLE_OPS_READ ].avg =20000 ; + cfg1.buckets[THROTTLE_OPS_WRITE ].avg =12000 ; +throttle_group_config (bdrv1, &cfg1); + +throttle_group_get_config (bdrv1, &cfg1); +throttle_group_get_config (bdrv3, &cfg2); +g_assert (!memcmp (&cfg1, &cfg2,sizeof (cfg1))); + + cfg2.buckets[THROTTLE_BPS_READ ].avg =4547 ; + cfg2.buckets[THROTTLE_BPS_WRITE ].avg =1349 ; + cfg2.buckets[THROTTLE_OPS_READ ].avg =123 ; + cfg2.buckets[THROTTLE_OPS_WRITE ].avg =86 ; +throttle_group_config (bdrv3, &cfg1); + +throttle_group_get_config (bdrv1, &cfg1); +throttle_group_get_config (bdrv3, &cfg2); +g_assert (!memcmp (&cfg1, &cfg2,sizeof (cfg1))); + +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;Error *local_error =NULL ; -init_clocks (); +qemu_init_main_loop (&local_error); + ctx =qemu_get_aio_context (); - ctx =aio_context_new (&local_error);if (!ctx) {error_report ("Failed to create AIO Context: '%s'" , -error_get_pretty (local_error)); -error_free (local_error); + local_error ?error_get_pretty (local_error) : +"Failed to initialize the QEMU main loop" ); +if (local_error) { +error_free (local_error); + }exit (1 ); } - src =aio_get_g_source (ctx); -g_source_attach (src,NULL ); -g_source_unref (src);do {}while (g_main_context_iteration (NULL , false));
@@ -533,6 +591,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/7] throttle: Add throttle group infrastructure tests
Quoted 11 lines · Alberto Garcia
Signed-off-by Alberto Garcia <[email protected]>--- tests/test-throttle.c | 79 ++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 69 insertions(+), 10 deletions(-)tests/test-throttle.c +0
-0@@ -1,10 +1,12 @@/*
tests/test-throttle.c line 9
Quoted 10 lines
tests/test-throttle.c +4
-2* 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]>
Could be moved to patch 7.
Fam
Quoted 110 lines
tests/test-throttle.c +65
-8* * 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,23 +503,78 @@ static void test_accounting(void) (64.0 / 13))); } +static void test_groups (void ) +{ +ThrottleConfig cfg1, cfg2; +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 (bdrv1->throttle_state !=NULL ); +g_assert (bdrv2->throttle_state !=NULL ); +g_assert (bdrv3->throttle_state !=NULL ); + +g_assert (!strcmp (throttle_group_get_name (bdrv1),"bar" )); +g_assert (!strcmp (throttle_group_get_name (bdrv2),"foo" )); +g_assert (bdrv1->throttle_state == bdrv3->throttle_state); + +/* Setting the config of a group member affects the whole group */ +memset (&cfg1,0 ,sizeof (cfg1)); + cfg1.buckets[THROTTLE_BPS_READ ].avg =500000 ; + cfg1.buckets[THROTTLE_BPS_WRITE ].avg =285000 ; + cfg1.buckets[THROTTLE_OPS_READ ].avg =20000 ; + cfg1.buckets[THROTTLE_OPS_WRITE ].avg =12000 ; +throttle_group_config (bdrv1, &cfg1); + +throttle_group_get_config (bdrv1, &cfg1); +throttle_group_get_config (bdrv3, &cfg2); +g_assert (!memcmp (&cfg1, &cfg2,sizeof (cfg1))); + + cfg2.buckets[THROTTLE_BPS_READ ].avg =4547 ; + cfg2.buckets[THROTTLE_BPS_WRITE ].avg =1349 ; + cfg2.buckets[THROTTLE_OPS_READ ].avg =123 ; + cfg2.buckets[THROTTLE_OPS_WRITE ].avg =86 ; +throttle_group_config (bdrv3, &cfg1); + +throttle_group_get_config (bdrv1, &cfg1); +throttle_group_get_config (bdrv3, &cfg2); +g_assert (!memcmp (&cfg1, &cfg2,sizeof (cfg1))); + +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;Error *local_error =NULL ; -init_clocks (); +qemu_init_main_loop (&local_error); + ctx =qemu_get_aio_context (); - ctx =aio_context_new (&local_error);if (!ctx) {error_report ("Failed to create AIO Context: '%s'" , -error_get_pretty (local_error)); -error_free (local_error); + local_error ?error_get_pretty (local_error) : +"Failed to initialize the QEMU main loop" ); +if (local_error) { +error_free (local_error); + }exit (1 ); } - src =aio_get_g_source (ctx); -g_source_attach (src,NULL ); -g_source_unref (src);do {}while (g_main_context_iteration (NULL , false));@@ -533,6 +591,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/7] throttle: Add throttle group infrastructure tests
In reply to Alberto GarciaQuoted 4 lines · Alberto Garcia
Signed-off-by Alberto Garcia <[email protected]>--- tests/test-throttle.c | 79 ++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 69 insertions(+), 10 deletions(-)
PATCH v6 4/7 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 | 83 +++++----------- block/qapi.c | 5 +- block/throttle-groups.c | 207 +++++++++++++++++++++++++++++++++++++++- blockdev.c | 22 ++++- hmp.c | 4 +- include/block/block.h | 3 +- include/block/block_int.h | 7 +- include/block/throttle-groups.h | 4 + qapi/block-core.json | 4 +- qemu-options.hx | 1 + qmp-commands.hx | 3 +- 11 files changed, 271 insertions(+), 72 deletions(-)
block.c +25 -58
@@ -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, cfg);for (i =0 ; i <2 ; i++) {qemu_co_enter_next (&bs->throttled_reqs[i]);
@@ -160,70 +161,33 @@ 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_timers_destroy (&bs->throttle_timers); -} - -static void bdrv_throttle_read_timer_cb (void *opaque) -{ -BlockDriverState *bs = opaque; -qemu_co_enter_next (&bs->throttled_reqs[0 ]); -} - -static void bdrv_throttle_write_timer_cb (void *opaque) -{ -BlockDriverState *bs = opaque; -qemu_co_enter_next (&bs->throttled_reqs[1 ]); +throttle_group_unregister_bs (bs); }/* 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_timers_init (&bs->throttle_timers, -bdrv_get_aio_context (bs), -QEMU_CLOCK_VIRTUAL , - bdrv_throttle_read_timer_cb, - bdrv_throttle_write_timer_cb, - bs); +throttle_group_register_bs (bs, group); bs->io_limits_enabled = true; } -/* This function makes an IO wait if needed -* -* @nb_sectors: the number of sectors of the IO -* @is_write: is the IO a write -*/ -static void bdrv_io_limits_intercept (BlockDriverState *bs, -unsigned int bytes, -bool is_write) +void bdrv_io_limits_update_group (BlockDriverState *bs,const char *group) { -/* 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]); +/* this bs is not part of any group */ +if (!bs->throttle_state) { +return ; } -/* 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)) { +/* this bs is a part of the same group than the one we want */ +if (!g_strcmp0 (throttle_group_get_name (bs), group)) {return ; } -/* else queue next request for execution */ -qemu_co_queue_next (&bs->throttled_reqs[is_write]); +/* need to change the group this bs belong to */ +bdrv_io_limits_disable (bs); +bdrv_io_limits_enable (bs, group); }size_t bdrv_opt_mem_align (BlockDriverState *bs)
@@ -2091,15 +2055,18 @@ 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->pending_reqs[0 ] = bs_src->pending_reqs[0 ]; + bs_dest->pending_reqs[1 ] = bs_src->pending_reqs[1 ]; + 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;
@@ -3170,7 +3137,7 @@ static int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs,/* throttling disk I/O */ if (bs->io_limits_enabled) { -bdrv_io_limits_intercept (bs, bytes, false); +throttle_group_co_io_limits_intercept (bs, bytes, false); }/* Align read if necessary by padding qiov */
@@ -3411,7 +3378,7 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,/* throttling disk I/O */ if (bs->io_limits_enabled) { -bdrv_io_limits_intercept (bs, bytes, true); +throttle_group_co_io_limits_intercept (bs, bytes, true); }/*
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, &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 +203 -4
241 lines not on this page · raw message
blockdev.c +19 -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,10 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,/* disk I/O throttling */ if (throttle_enabled (&cfg)) { -bdrv_io_limits_enable (bs); +if (!throttling_group) { + throttling_group =blk_name (blk); + } +bdrv_io_limits_enable (bs, throttling_group);bdrv_set_io_limits (bs, &cfg); }
@@ -711,6 +717,8 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) { "iops_size", "throttling.iops-size" }, + { "group", "throttling.group" }, + { "readonly", "read-only" }, };
@@ -1887,7 +1895,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;
@@ -1941,9 +1951,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 : device); }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 : device); }if (bs->io_limits_enabled) {
@@ -3017,6 +3029,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
@@ -1272,7 +1272,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
@@ -173,8 +173,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 -2
@@ -376,10 +376,13 @@ struct BlockDriverState {unsigned int serialising_in_flight;/* I/O throttling */ -ThrottleState throttle_state; -ThrottleTimers throttle_timers;CoQueue throttled_reqs[2 ];bool io_limits_enabled; +/* The following fields are protected by the ThrottleGroup lock. +* See the ThrottleGroup documentation for details. */ +ThrottleState *throttle_state; +ThrottleTimers throttle_timers; +unsigned pending_reqs[2 ];QLIST_ENTRY (BlockDriverState) round_robin;/* I/O stats (display with "info blockstats"). */
include/block/throttle-groups.h +4 -0
@@ -36,4 +36,8 @@ void throttle_group_get_config(BlockDriverState *bs, ThrottleConfig *cfg);void throttle_group_register_bs (BlockDriverState *bs,const char *groupname);void throttle_group_unregister_bs (BlockDriverState *bs); +void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs, +unsigned int bytes, +bool is_write); +#endif
qapi/block-core.json +3 -1
@@ -990,6 +990,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 #
@@ -1001,7 +1003,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
@@ -464,6 +464,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
@@ -1730,7 +1730,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, },
@@ -1756,6 +1756,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/7] throttle: Add throttle group support
qapi/block-core.json line 993
Quoted 9 lines · Alberto Garcia
qapi/block-core.json +1
-0@@ -990,6 +990,8 @@ # # @iops_size: #optional an I/O size in bytes (Since 1.7) # +# @group: #optional throttle group name (Since 2.3)
We should probably elaborate (here, and at other places of @group appearances):
@device is used as group name. This is useful since other devices could use this device name as the group name, for exmple the following -drive definition:
...
-drive file=foo,id=foo,bps=100 \
-drive file=bar,id=foo,bps=200,group=foo \
...
will put both devices into the same group.
Also, as the two bps values don't match, I assume the last one is used? Is there a warning when ignoring a config from previous definitions?
Thinking about this, I'd slightly prefer a canonical throttle group definition rather than patching the existing parameters:
-object throttle-group,id=tg0,bps=100,iops=200,iops-max=1000 \
-drive file=foo,id=foo,throttle-group=tg0 \
-drive file=bar,id=bar,throttle-group=tg0 \
and error out if "bps=" etc are specified together with "throttle-group=" in -drive.
And in QMP, add block_set_io_throttle_group which works together with object-add.
What do you think?
Quoted 39 lines
qapi/block-core.json +2
-1+# # Returns: Nothing on success # If @device is not a valid block device, DeviceNotFound #@@ -1001,7 +1003,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@@ -464,6 +464,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 +1
-1@@ -1730,7 +1730,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, },
qmp-commands.hx Arguments: · line 1758
Quoted 3 lines
qmp-commands.hx +0
-0@@ -1756,6 +1756,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)
Why removing these three?
Quoted 8 lines
qmp-commands.hx +1
-0+- "group": throttle group name (json-string) Example:Signature · 1 line
2.1.4
Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
Quoted 12 lines · Fam Zheng
Quoted 1 line
+# @group: #optional throttle group name (Since 2.3)
We should probably elaborate (here, and at other places of @group appearances): @device is used as group name. This is useful since other devices could use this device name as the group name, for exmple the following -drive definition:
...
(quoted diff) +0
-2-drive file=foo,id=foo,bps=100 \ -drive file=bar,id=foo,bps=200,group=foo \
Quoted 1 line
will put both devices into the same group.
That's right, thanks for pointing it out. I can update the documentation. Alternatively we could use some other mechanism for auto-generating group names, rather than using the name of the device, but either way I think it deserves being documented.
Quoted 3 lines
Also, as the two bps values don't match, I assume the last one is used? Is there a warning when ignoring a config from previous definitions?
Everytime a new device is added to a group it sets new values for the whole group, because values are not attached to any particular device. For the same reason if one device is removed the rest will continue working with the existing configuration.
Quoted 7 lines
Thinking about this, I'd slightly prefer a canonical throttle group definition rather than patching the existing parameters:
(quoted diff) +0
-3-object throttle-group,id=tg0,bps=100,iops=200,iops-max=1000 \ -drive file=foo,id=foo,throttle-group=tg0 \ -drive file=bar,id=bar,throttle-group=tg0 \
Quoted 5 lines
and error out if "bps=" etc are specified together with "throttle-group=" in -drive.
And in QMP, add
block_set_io_throttle_groupwhich works together with object-add.
And then you would create a group automatically for drives that have bps= specified? What would happen if all members of the group are removed? And what would happen if it's a group created with -object?
My first impression is that the idea is feasible, but I'm unsure at the moment of its complexity or possible side effects.
Quoted 6 lines
qmp-commands.hx Arguments: · line 1758
Quoted 4 lines
qmp-commands.hx +0
-0@@ -1756,6 +1756,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)Why removing these three?
There's nothing removed, that's just a list of bullets, note the column where the dashes appear :)
Berto
Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
Quoted 37 lines · Alberto Garcia
Quoted 12 lines · Fam Zheng
Quoted 1 line
+# @group: #optional throttle group name (Since 2.3)
We should probably elaborate (here, and at other places of @group appearances): @device is used as group name. This is useful since other devices could use this device name as the group name, for exmple the following -drive definition:
...
(quoted diff) +0
-2-drive file=foo,id=foo,bps=100 \ -drive file=bar,id=foo,bps=200,group=foo \Quoted 1 line
will put both devices into the same group.
That's right, thanks for pointing it out. I can update the documentation. Alternatively we could use some other mechanism for auto-generating group names, rather than using the name of the device, but either way I think it deserves being documented.
Quoted 3 lines
Also, as the two bps values don't match, I assume the last one is used? Is there a warning when ignoring a config from previous definitions?
Everytime a new device is added to a group it sets new values for the whole group, because values are not attached to any particular device. For the same reason if one device is removed the rest will continue working with the existing configuration.
Quoted 7 lines
Thinking about this, I'd slightly prefer a canonical throttle group definition rather than patching the existing parameters:
(quoted diff) +0
-3-object throttle-group,id=tg0,bps=100,iops=200,iops-max=1000 \ -drive file=foo,id=foo,throttle-group=tg0 \ -drive file=bar,id=bar,throttle-group=tg0 \
Quoted 8 lines
Quoted 5 lines
and error out if "bps=" etc are specified together with "throttle-group=" in -drive.
And in QMP, add
block_set_io_throttle_groupwhich works together with object-add.And then you would create a group automatically for drives that have bps= specified?
Yes.
Quoted 1 line
What would happen if all members of the group are removed?
The automatic group created from "bps=" should be anonymous, which is attached by the only one BDS. If it is removed, refcount should release the group automatically.
Quoted 1 line
And what would happen if it's a group created with -object?
The -object has a refcount, so it should stay even when all members are removed, unless "(QMP) object-del".
Quoted 3 lines
My first impression is that the idea is feasible, but I'm unsure at the moment of its complexity or possible side effects.
I think it is a cleaner model and easier to understand. What complexity are you referring to?
Quoted 10 lines
Quoted 7 lines
qmp-commands.hx Arguments: · line 1758
Quoted 5 lines
qmp-commands.hx +0
-0@@ -1756,6 +1756,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)Why removing these three?
There's nothing removed, that's just a list of bullets, note the column where the dashes appear :)
:D
Fam
Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
Quoted 10 lines · Fam Zheng
Quoted 10 lines
Quoted 10 lines
Thinking about this, I'd slightly prefer a canonical throttle group definition rather than patching the existing parameters:
(quoted diff) +0
-3-object throttle-group,id=tg0,bps=100,iops=200,iops-max=1000 \ -drive file=foo,id=foo,throttle-group=tg0 \ -drive file=bar,id=bar,throttle-group=tg0 \and error out if "bps=" etc are specified together with "throttle-group=" in -drive.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
And in QMP, add
block_set_io_throttle_groupwhich works together with object-add.My first impression is that the idea is feasible, but I'm unsure at the moment of its complexity or possible side effects.
I think it is a cleaner model and easier to understand. What complexity are you referring to?
The changes on the code. At first sight it seems that it should not be too difficult to convert the current implementation (and API) into the one you are proposing, but I'll wait for a few days to hear other opinions before attempting to rewrite it :)
Berto
Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
In reply to Alberto Garciablockdev.c void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd, · line 1961
Quoted 13 lines · Alberto Garcia
blockdev.c +3
-1@@ -1941,9 +1951,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 : device); }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 : device); }if (bs->io_limits_enabled) {
The semantics are inconsistent:
1. Create drive0 with throttle group "mygroup".
2. Issue block-set-io-throttle device="drive0"
The result is that a new throttle group called "drive0" is created. I expected to modify the throttling configuration for drive0 (i.e. "mygroup").
Now let's disable the throttle group:
3. Issue block-set-io-throttle with 0 values for device="drive0"
The result is that "mygroup" is changed to all 0s.
Enable should behave like disable and operate on the device's throttle group.
Stefan
Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
Quoted 16 lines · Stefan Hajnoczi
blockdev.c void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd, · line 1961
Quoted 13 lines
blockdev.c +3
-1@@ -1941,9 +1951,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 : device); }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 : device); }if (bs->io_limits_enabled) {The semantics are inconsistent:
Quoted 6 lines
1. Create drive0 with throttle group "mygroup".
2. Issue block-set-io-throttle device="drive0"The result is that a new throttle group called "drive0" is created. I expected to modify the throttling configuration for drive0 (i.e. "mygroup").
That's right. What we can do is choose the group to update using the following criteria, in order of precedence:
1) The 'group' parameter from block-set-io-throttle.
2) The group the device is already member of.
3) A new group ("drive0" in this example).
Currently we're not doing 2).
Quoted 5 lines
Now let's disable the throttle group:
3. Issue block-set-io-throttle with 0 values for device="drive0"
The result is that "mygroup" is changed to all 0s.
No, that simply removes the device from the group without touching its configuration:
Quoted 2 lines
Quoted 2 lines
} else if (bs->io_limits_enabled && !throttle_enabled(&cfg)) { bdrv_io_limits_disable(bs);
The group name passed by the user is actually not used in this case.
Berto
Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
Quoted 25 lines · Alberto Garcia
Quoted 16 lines · Stefan Hajnoczi
blockdev.c void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd, · line 1961
Quoted 13 lines
blockdev.c +3
-1@@ -1941,9 +1951,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 : device); }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 : device); }if (bs->io_limits_enabled) {The semantics are inconsistent:
Quoted 6 lines
1. Create drive0 with throttle group "mygroup".
2. Issue block-set-io-throttle device="drive0"The result is that a new throttle group called "drive0" is created. I expected to modify the throttling configuration for drive0 (i.e. "mygroup").
Quoted 8 lines
That's right. What we can do is choose the group to update using the following criteria, in order of precedence:
1) The 'group' parameter from block-set-io-throttle.
2) The group the device is already member of.
3) A new group ("drive0" in this example).Currently we're not doing 2).
Great, it makes sense to add 2).
Quoted 6 lines
Quoted 5 lines
Now let's disable the throttle group:
3. Issue block-set-io-throttle with 0 values for device="drive0"
The result is that "mygroup" is changed to all 0s.
Quoted 7 lines
No, that simply removes the device from the group without touching its configuration:
Quoted 2 lines
Quoted 2 lines
} else if (bs->io_limits_enabled && !throttle_enabled(&cfg)) { bdrv_io_limits_disable(bs);The group name passed by the user is actually not used in this case.
You are right.
Stefan
Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
Quoted 10 lines · Stefan Hajnoczi
Quoted 8 lines
That's right. What we can do is choose the group to update using the following criteria, in order of precedence:
1) The 'group' parameter from block-set-io-throttle.
2) The group the device is already member of.
3) A new group ("drive0" in this example).Currently we're not doing 2).
Great, it makes sense to add 2).
Ok, I'll do that and explain the whole thing in the API doc.
Berto
PATCH v6 5/7 throttle: acquire the ThrottleGroup lock in bdrv_swap()
In reply to Alberto Garciabdrv_swap() touches the fields of a BlockDriverState that are protected by the ThrottleGroup lock. Although those fields end up in their original place, they are temporarily swapped in the process, so there's a chance that an operation on a member of the same group happening on a different thread can try to use them.
--- block.c | 15 +++++++++++++++ block/throttle-groups.c | 31 ++++++++++++++++++++++++++++++- include/block/throttle-groups.h | 3 +++ 3 files changed, 48 insertions(+), 1 deletion(-)
block.c +15 -0
@@ -2120,11 +2120,20 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)QTAILQ_REMOVE (&graph_bdrv_states, bs_old, node_list); } +/* If the BlockDriverState is part of a throttling group acquire +* its lock since we're going to mess with the protected fields. +* Otherwise there's no need to worry since no one else can touch +* them. */ +if (bs_old->throttle_state) { +throttle_group_lock (bs_old); + } +/* bs_new must be unattached and shouldn't have anything fancy enabled */ assert (!bs_new->blk);assert (QLIST_EMPTY (&bs_new->dirty_bitmaps));assert (bs_new->job ==NULL );assert (bs_new->io_limits_enabled == false); +assert (bs_new->throttle_state ==NULL );assert (!throttle_timers_are_initialized (&bs_new->throttle_timers)); tmp = *bs_new;
@@ -2142,8 +2151,14 @@ 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 (bs_new->throttle_state ==NULL );assert (!throttle_timers_are_initialized (&bs_new->throttle_timers)); +/* Release the ThrottleGroup lock */ +if (bs_old->throttle_state) { +throttle_group_unlock (bs_old); + } +/* insert the nodes back into the graph node list if needed */ if (bs_new->node_name[0 ] !='\0' ) {QTAILQ_INSERT_TAIL (&graph_bdrv_states, bs_new, node_list);
block/throttle-groups.c +30 -1
@@ -32,7 +32,8 @@* its own locking. * * This locking is however handled internally in this file, so it's -* transparent to outside users. +* mostly transparent to outside users (but see the documentation in +* throttle_groups_lock()). * * The whole ThrottleGroup structure is private and invisible to * outside users, that only use it through its ThrottleState.
@@ -452,6 +453,34 @@ void throttle_group_unregister_bs(BlockDriverState *bs) bs->throttle_state =NULL ; } +/* Acquire the lock of this throttling group. +* +* You won't normally need to use this. None of the functions from the +* ThrottleGroup API require you to acquire the lock since all of them +* deal with it internally. +* +* This should only be used in exceptional cases when you want to +* access the protected fields of a BlockDriverState directly +* (e.g. bdrv_swap()). +* +* @bs: a BlockDriverState that is member of the group +*/ +void throttle_group_lock (BlockDriverState *bs) +{ +ThrottleGroup *tg =container_of (bs->throttle_state, ThrottleGroup, ts); +qemu_mutex_lock (&tg->lock); +} + +/* Release the lock of this throttling group. +* +* See the comments in throttle_group_lock(). +*/ +void throttle_group_unlock (BlockDriverState *bs) +{ +ThrottleGroup *tg =container_of (bs->throttle_state, ThrottleGroup, ts); +qemu_mutex_unlock (&tg->lock); +} +static void throttle_groups_init (void ) {qemu_mutex_init (&throttle_groups_lock);
include/block/throttle-groups.h +3 -0
@@ -40,4 +40,7 @@ void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs,unsigned int bytes, bool is_write); +void throttle_group_lock (BlockDriverState *bs); +void throttle_group_unlock (BlockDriverState *bs); +#endif
Signature · 1 line
2.1.4
PATCH v6 6/7 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)); } info->write_threshold =bdrv_write_threshold_get (bs);
hmp.c +4 -2
@@ -375,7 +375,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,
@@ -388,7 +389,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
@@ -259,6 +259,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.
@@ -278,7 +280,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
Re: [Qemu-devel] [PATCH 6/7] throttle: add the name of the ThrottleGroup to BlockDeviceInfo
Quoted 12 lines · Alberto Garcia
Signed-off-by Alberto Garcia <[email protected]>--- block/qapi.c | 3 +++ hmp.c | 6 ++++-- qapi/block-core.json | 4 +++- 3 files changed, 10 insertions(+), 3 deletions(-)block/qapi.c +0
-0
block/qapi.c BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs) · line 96
Quoted 6 lines
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));
True even if it's unset?
Quoted 32 lines
block/qapi.c +0
-0} info->write_threshold =bdrv_write_threshold_get (bs);hmp.c +4
-2@@ -375,7 +375,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,@@ -388,7 +389,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 +0
-0
qapi/block-core.json line 262
Quoted 4 lines
qapi/block-core.json +1
-0@@ -259,6 +259,8 @@ # # @iops_size: #optional an I/O size in bytes (Since 1.7) # +# @group: #optional throttle group name (Since 2.3)
Will probably be 2.4.
Fam
Quoted 17 lines
qapi/block-core.json +2
-1+# # @cache: the cache mode used for the block device (since: 2.3) # # @write_threshold: configured write threshold for the device.@@ -278,7 +280,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
Re: [Qemu-devel] [PATCH 6/7] throttle: add the name of the ThrottleGroup to BlockDeviceInfo
Quoted 7 lines · Fam Zheng
block/qapi.c BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs) · line 96
Quoted 5 lines
info->has_iops_size = cfg.op_size; info->iops_size = cfg.op_size;block/qapi.c +3
-0+ + info->has_group = true; + info->group =g_strdup (throttle_group_get_name (bs));True even if it's unset?
Note that that's inside an "if (bs->io_limits_enabled)" block.
io_limits_enabled is set immediately after adding a BDS to a throttling group, and that requires a name.
Quoted 3 lines
Quoted 1 line
+# @group: #optional throttle group name (Since 2.3)
Will probably be 2.4.
Right.
Berto
Re: [Qemu-devel] [PATCH 6/7] throttle: add the name of the ThrottleGroup to BlockDeviceInfo
Quoted 11 lines · Alberto Garcia
Quoted 7 lines · Fam Zheng
block/qapi.c BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs) · line 96
Quoted 5 lines
info->has_iops_size = cfg.op_size; info->iops_size = cfg.op_size;block/qapi.c +3
-0+ + info->has_group = true; + info->group =g_strdup (throttle_group_get_name (bs));True even if it's unset?
Note that that's inside an "if (
bs->io_limits_enabled)" block.
Then this is OK :)
Fam
Quoted 11 lines
io_limits_enabledis set immediately after adding a BDS to a throttling group, and that requires a name.Quoted 3 lines
Quoted 1 line
+# @group: #optional throttle group name (Since 2.3)
Will probably be 2.4.
Right.
Berto
PATCH v6 7/7 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