From db6283385cb708b9d589e5b57e96eab4afd0269e Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Mon, 8 Jun 2015 18:17:45 +0200 Subject: [PATCH] throttle: acquire the ThrottleGroup lock in bdrv_swap() bdrv_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. Signed-off-by: Alberto Garcia Reviewed-by: Stefan Hajnoczi Message-id: d92dc40d7c4f1fc5cda5cbbf4ffb7a4670b79d17.1433779731.git.berto@igalia.com Signed-off-by: Stefan Hajnoczi --- block.c | 16 ++++++++++++++++ block/throttle-groups.c | 31 ++++++++++++++++++++++++++++++- include/block/throttle-groups.h | 3 +++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index 1bc0e7394b..54d789a8e9 100644 --- a/block.c +++ b/block.c @@ -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 @@ -1887,11 +1888,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; @@ -1909,8 +1919,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); diff --git a/block/throttle-groups.c b/block/throttle-groups.c index da8c70c4a6..efc462fbc5 100644 --- a/block/throttle-groups.c +++ b/block/throttle-groups.c @@ -33,7 +33,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. @@ -459,6 +460,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); diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h index 322139a7a2..fab113f6d1 100644 --- a/include/block/throttle-groups.h +++ b/include/block/throttle-groups.h @@ -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 -- 2.11.4.GIT