From patchwork Thu Aug 17 12:50:20 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Wolf X-Patchwork-Id: 13356398 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A8872C2FC20 for ; Thu, 17 Aug 2023 12:54:03 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qWcUe-0002jK-JL; Thu, 17 Aug 2023 08:52:54 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qWcTH-0008M9-P2 for qemu-devel@nongnu.org; Thu, 17 Aug 2023 08:51:28 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qWcTF-0002qP-5z for qemu-devel@nongnu.org; Thu, 17 Aug 2023 08:51:27 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1692276684; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=MDGrjG3c1lkKCik8iN1rhb4C2PmepOJMoYB87aaP5Go=; b=EAv2e2fGiR6klNdWBrh1PKVROQUHoIOTzUOgsO8wjo2iZyj956ScfZ/31amNQDFA5hoDFl bOCr2Z1eH/QM9YVoyuDcjF/ESYViiP8lJPgunBmPC23jQpAK3jtRSVw51yd9+aw69wtbYE mgiRB8F/OZN92FzF8ajP6oe1simlT8U= Received: from mimecast-mx02.redhat.com (66.187.233.73 [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-561-P6k4D1u8Pq6mO-jY2Xt9LA-1; Thu, 17 Aug 2023 08:51:23 -0400 X-MC-Unique: P6k4D1u8Pq6mO-jY2Xt9LA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B91453C0E450; Thu, 17 Aug 2023 12:51:22 +0000 (UTC) Received: from merkur.redhat.com (unknown [10.39.194.106]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8DCC41121314; Thu, 17 Aug 2023 12:51:21 +0000 (UTC) From: Kevin Wolf To: qemu-block@nongnu.org Cc: kwolf@redhat.com, stefanha@redhat.com, eblake@redhat.com, eesposit@redhat.com, pbonzini@redhat.com, vsementsov@yandex-team.ru, qemu-devel@nongnu.org Subject: [PATCH 21/21] block: Mark bdrv_add/del_child() and caller GRAPH_WRLOCK Date: Thu, 17 Aug 2023 14:50:20 +0200 Message-ID: <20230817125020.208339-22-kwolf@redhat.com> In-Reply-To: <20230817125020.208339-1-kwolf@redhat.com> References: <20230817125020.208339-1-kwolf@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 Received-SPF: pass client-ip=170.10.129.124; envelope-from=kwolf@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.021, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H4=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org The functions read the parents list in the generic block layer, so we need to hold the graph lock already there. The BlockDriver implementations actually modify the graph, so it has to be a writer lock. Signed-off-by: Kevin Wolf Reviewed-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- include/block/block-global-state.h | 8 +++++--- include/block/block_int-common.h | 9 +++++---- block/quorum.c | 23 ++++++----------------- blockdev.c | 17 +++++++++++------ 4 files changed, 27 insertions(+), 30 deletions(-) diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index 0f6df8f1a2..f31660c7b1 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -276,9 +276,11 @@ int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx, int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz); int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo); -void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, - Error **errp); -void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp); +void GRAPH_WRLOCK +bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, Error **errp); + +void GRAPH_WRLOCK +bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp); /** * diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 9d5002df5a..277168b973 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -393,10 +393,11 @@ struct BlockDriver { */ int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo); - void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child, - Error **errp); - void (*bdrv_del_child)(BlockDriverState *parent, BdrvChild *child, - Error **errp); + void GRAPH_WRLOCK_PTR (*bdrv_add_child)( + BlockDriverState *parent, BlockDriverState *child, Error **errp); + + void GRAPH_WRLOCK_PTR (*bdrv_del_child)( + BlockDriverState *parent, BdrvChild *child, Error **errp); /** * Informs the block driver that a permission change is intended. The diff --git a/block/quorum.c b/block/quorum.c index 620a50ba2c..05220cab7f 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -1066,8 +1066,8 @@ static void quorum_close(BlockDriverState *bs) g_free(s->children); } -static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs, - Error **errp) +static void GRAPH_WRLOCK +quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs, Error **errp) { BDRVQuorumState *s = bs->opaque; BdrvChild *child; @@ -1093,29 +1093,22 @@ static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs, } s->next_child_index++; - bdrv_drained_begin(bs); - /* We can safely add the child now */ bdrv_ref(child_bs); - bdrv_graph_wrlock(child_bs); child = bdrv_attach_child(bs, child_bs, indexstr, &child_of_bds, BDRV_CHILD_DATA, errp); - bdrv_graph_wrunlock(); if (child == NULL) { s->next_child_index--; - goto out; + return; } s->children = g_renew(BdrvChild *, s->children, s->num_children + 1); s->children[s->num_children++] = child; quorum_refresh_flags(bs); - -out: - bdrv_drained_end(bs); } -static void quorum_del_child(BlockDriverState *bs, BdrvChild *child, - Error **errp) +static void GRAPH_WRLOCK +quorum_del_child(BlockDriverState *bs, BdrvChild *child, Error **errp) { BDRVQuorumState *s = bs->opaque; char indexstr[INDEXSTR_LEN]; @@ -1145,18 +1138,14 @@ static void quorum_del_child(BlockDriverState *bs, BdrvChild *child, s->next_child_index--; } - bdrv_drained_begin(bs); - /* We can safely remove this child now */ memmove(&s->children[i], &s->children[i + 1], (s->num_children - i - 1) * sizeof(BdrvChild *)); s->children = g_renew(BdrvChild *, s->children, --s->num_children); - bdrv_graph_wrlock(NULL); + bdrv_unref_child(bs, child); - bdrv_graph_wrunlock(); quorum_refresh_flags(bs); - bdrv_drained_end(bs); } static void quorum_gather_child_options(BlockDriverState *bs, QDict *target, diff --git a/blockdev.c b/blockdev.c index 372eaf198c..325b7a3bef 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3545,8 +3545,8 @@ out: aio_context_release(aio_context); } -static BdrvChild *bdrv_find_child(BlockDriverState *parent_bs, - const char *child_name) +static BdrvChild * GRAPH_RDLOCK +bdrv_find_child(BlockDriverState *parent_bs, const char *child_name) { BdrvChild *child; @@ -3565,9 +3565,11 @@ void qmp_x_blockdev_change(const char *parent, const char *child, BlockDriverState *parent_bs, *new_bs = NULL; BdrvChild *p_child; + bdrv_graph_wrlock(NULL); + parent_bs = bdrv_lookup_bs(parent, parent, errp); if (!parent_bs) { - return; + goto out; } if (!child == !node) { @@ -3576,7 +3578,7 @@ void qmp_x_blockdev_change(const char *parent, const char *child, } else { error_setg(errp, "Either child or node must be specified"); } - return; + goto out; } if (child) { @@ -3584,7 +3586,7 @@ void qmp_x_blockdev_change(const char *parent, const char *child, if (!p_child) { error_setg(errp, "Node '%s' does not have child '%s'", parent, child); - return; + goto out; } bdrv_del_child(parent_bs, p_child, errp); } @@ -3593,10 +3595,13 @@ void qmp_x_blockdev_change(const char *parent, const char *child, new_bs = bdrv_find_node(node); if (!new_bs) { error_setg(errp, "Node '%s' not found", node); - return; + goto out; } bdrv_add_child(parent_bs, new_bs, errp); } + +out: + bdrv_graph_wrunlock(); } BlockJobInfoList *qmp_query_block_jobs(Error **errp)