From patchwork Mon Mar 30 14:16:17 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alberto Garcia X-Patchwork-Id: 456192 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 047B7140140 for ; Tue, 31 Mar 2015 01:17:53 +1100 (AEDT) Received: from localhost ([::1]:34206 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YcaVi-0004Xi-Pm for incoming@patchwork.ozlabs.org; Mon, 30 Mar 2015 10:17:50 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57008) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YcaV7-0003dV-DW for qemu-devel@nongnu.org; Mon, 30 Mar 2015 10:17:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YcaV2-0003T4-I3 for qemu-devel@nongnu.org; Mon, 30 Mar 2015 10:17:13 -0400 Received: from smtp3.mundo-r.com ([212.51.32.191]:24355 helo=smtp4.mundo-r.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YcaV2-0003P6-6y for qemu-devel@nongnu.org; Mon, 30 Mar 2015 10:17:08 -0400 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AisFADFaGVVbdWOb/2dsb2JhbABcgwaBLrIZBQF7gwWVEQKBNEwBAQEBAQF9hBUBBSdSEFE8GxmIMwHKTQEBCCKGD4kuZAeELQWLFo86izuIeiKBfwMcgVM7MYECJIEdAQEB X-IPAS-Result: AisFADFaGVVbdWOb/2dsb2JhbABcgwaBLrIZBQF7gwWVEQKBNEwBAQEBAQF9hBUBBSdSEFE8GxmIMwHKTQEBCCKGD4kuZAeELQWLFo86izuIeiKBfwMcgVM7MYECJIEdAQEB X-IronPort-AV: E=Sophos;i="5.11,494,1422918000"; d="scan'208";a="343829534" Received: from fanzine.igalia.com ([91.117.99.155]) by smtp4.mundo-r.com with ESMTP; 30 Mar 2015 16:16:31 +0200 Received: from maestria.local.igalia.com ([192.168.10.14] helo=mail.igalia.com) by fanzine.igalia.com with esmtps (Cipher TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim) id 1YcaUR-0007Ka-LS; Mon, 30 Mar 2015 16:16:31 +0200 Received: from fanzine.local.igalia.com ([192.168.10.13] helo=perseus.local) by mail.igalia.com with esmtps (Cipher TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim) id 1YcaUH-0004Cl-5n; Mon, 30 Mar 2015 16:16:21 +0200 Received: from berto by perseus.local with local (Exim 4.84) (envelope-from ) id 1YcaUF-0005YI-M2; Mon, 30 Mar 2015 17:16:19 +0300 From: Alberto Garcia To: qemu-devel@nongnu.org Date: Mon, 30 Mar 2015 17:16:17 +0300 Message-Id: X-Mailer: git-send-email 2.1.4 In-Reply-To: References: In-Reply-To: References: X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 212.51.32.191 Cc: Kevin Wolf , Alberto Garcia , Stefan Hajnoczi Subject: [Qemu-devel] [PATCH 5/7] throttle: acquire the ThrottleGroup lock in bdrv_swap() X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 --- block.c | 15 +++++++++++++++ block/throttle-groups.c | 31 ++++++++++++++++++++++++++++++- include/block/throttle-groups.h | 3 +++ 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index 5ca972f..5643082 100644 --- a/block.c +++ b/block.c @@ -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); diff --git a/block/throttle-groups.c b/block/throttle-groups.c index f909f50..20a510a 100644 --- a/block/throttle-groups.c +++ b/block/throttle-groups.c @@ -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); diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h index 322139a..fab113f 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