From nobody Wed Nov 27 12:28:36 2024 Delivered-To: importer@patchew.org Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=pass(p=none dis=none) header.from=redhat.com ARC-Seal: i=1; a=rsa-sha256; t=1699382943; cv=none; d=zohomail.com; s=zohoarc; b=QNcdRt+K2nJFZ7XftAeZzzc1gGs1Suv/ZXFhDu+wmOoq3cciWUB+e69TUFTbpyVNjt6FRKtfkhn9qqEqPv7ulIGe+7EnwpSLtw9vdbnQmutqkikqurrWby13fW31B18QbJqQbxNkZnEwPj1R73F+jEd9btIhFXqcb2wpgHnSbIc= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1699382943; h=Content-Transfer-Encoding:Cc:Cc:Date:Date:From:From:In-Reply-To:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:References:Sender:Subject:Subject:To:To:Message-Id:Reply-To; bh=w0Pw14MMEFDUie5Z7nWMO05syP8WwrvNVlewbsAACAU=; b=DipXZIsifcG6F4OnHZSu0lcNZBAhRclQavR50PyqEqG6VNHXdwmwCESFyrVlqeVHW+b5kqffcBdbuwfp1nkw7I5gkN/zIbiuXCSJ+Ma6D/PrwV1pjPw46a+wmjBaHn/haH41ggSUXXQtgQnwEGBDvSbM5pC+ccrxz0xWic7gYv4= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=pass header.from= (p=none dis=none) Return-Path: Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mx.zohomail.com with SMTPS id 1699382943491651.1995119618113; Tue, 7 Nov 2023 10:49:03 -0800 (PST) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1r0R6E-0004Vb-My; Tue, 07 Nov 2023 13:46:54 -0500 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 1r0R5n-00041W-Cq for qemu-devel@nongnu.org; Tue, 07 Nov 2023 13:46:28 -0500 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 1r0R5j-0001Vy-TL for qemu-devel@nongnu.org; Tue, 07 Nov 2023 13:46:27 -0500 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-63-Xfj5dljJPVGg9KoO0Z4whA-1; Tue, 07 Nov 2023 13:46:20 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id F2B36101AA75 for ; Tue, 7 Nov 2023 18:46:19 +0000 (UTC) Received: from merkur.fritz.box (unknown [10.39.194.197]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3E811C1596F; Tue, 7 Nov 2023 18:46:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1699382781; 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=w0Pw14MMEFDUie5Z7nWMO05syP8WwrvNVlewbsAACAU=; b=XdAVZLsA8i8COSV93H9gxKucgqQc7jckitF5muSbmHYF+mREk5kwhQlaH0xapMcLPO/eMt 3vcR0Qz/7OZeDyDTT0/0CuEUiU2eUa5/QqgazfcXgLOm6HdrCM/nt4E8ikXc5KmBADBid4 G7GnQAag6KDh9T1mzro9hbYzLyfglRA= X-MC-Unique: Xfj5dljJPVGg9KoO0Z4whA-1 From: Kevin Wolf To: qemu-devel@nongnu.org Cc: kwolf@redhat.com Subject: [PULL 16/25] block: Mark bdrv_replace_node() GRAPH_WRLOCK Date: Tue, 7 Nov 2023 19:45:56 +0100 Message-ID: <20231107184605.236540-17-kwolf@redhat.com> In-Reply-To: <20231107184605.236540-1-kwolf@redhat.com> References: <20231107184605.236540-1-kwolf@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 Received-SPF: pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; 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.001, 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, T_SCC_BODY_TEXT_LINE=-0.01 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+importer=patchew.org@nongnu.org Sender: qemu-devel-bounces+importer=patchew.org@nongnu.org X-ZohoMail-DKIM: pass (identity @redhat.com) X-ZM-MESSAGEID: 1699382945518100015 Content-Type: text/plain; charset="utf-8" Instead of taking the writer lock internally, require callers to already hold it when calling bdrv_replace_node(). Its callers may already want to hold the graph lock and so wouldn't be able to call functions that take it internally. Signed-off-by: Kevin Wolf Message-ID: <20231027155333.420094-17-kwolf@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- include/block/block-global-state.h | 6 ++++-- block.c | 30 +++++++++++------------------- block/commit.c | 13 +++++++++++-- block/mirror.c | 26 ++++++++++++++++---------- blockdev.c | 5 +++++ tests/unit/test-bdrv-drain.c | 6 ++++++ tests/unit/test-bdrv-graph-mod.c | 13 +++++++++++-- 7 files changed, 64 insertions(+), 35 deletions(-) diff --git a/include/block/block-global-state.h b/include/block/block-globa= l-state.h index a1fd70ec97..9e0ccc1c32 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -71,8 +71,10 @@ bdrv_co_create_file(const char *filename, QemuOpts *opts= , Error **errp); BlockDriverState *bdrv_new(void); int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, Error **errp); -int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, - Error **errp); + +int GRAPH_WRLOCK +bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, Error **er= rp); + int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs, Error **errp); BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_optio= ns, diff --git a/block.c b/block.c index c7409cf658..cac517ab8b 100644 --- a/block.c +++ b/block.c @@ -5484,25 +5484,7 @@ out: int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, Error **errp) { - int ret; - - GLOBAL_STATE_CODE(); - - /* Make sure that @from doesn't go away until we have successfully att= ached - * all of its parents to @to. */ - bdrv_ref(from); - bdrv_drained_begin(from); - bdrv_drained_begin(to); - bdrv_graph_wrlock(to); - - ret =3D bdrv_replace_node_common(from, to, true, false, errp); - - bdrv_graph_wrunlock(); - bdrv_drained_end(to); - bdrv_drained_end(from); - bdrv_unref(from); - - return ret; + return bdrv_replace_node_common(from, to, true, false, errp); } =20 int bdrv_drop_filter(BlockDriverState *bs, Error **errp) @@ -5717,9 +5699,19 @@ BlockDriverState *bdrv_insert_node(BlockDriverState = *bs, QDict *options, goto fail; } =20 + /* + * Make sure that @bs doesn't go away until we have successfully attac= hed + * all of its parents to @new_node_bs and undrained it again. + */ + bdrv_ref(bs); bdrv_drained_begin(bs); + bdrv_drained_begin(new_node_bs); + bdrv_graph_wrlock(new_node_bs); ret =3D bdrv_replace_node(bs, new_node_bs, errp); + bdrv_graph_wrunlock(); + bdrv_drained_end(new_node_bs); bdrv_drained_end(bs); + bdrv_unref(bs); =20 if (ret < 0) { error_prepend(errp, "Could not replace node: "); diff --git a/block/commit.c b/block/commit.c index d92af02ead..30bc082edc 100644 --- a/block/commit.c +++ b/block/commit.c @@ -68,6 +68,7 @@ static void commit_abort(Job *job) { CommitBlockJob *s =3D container_of(job, CommitBlockJob, common.job); BlockDriverState *top_bs =3D blk_bs(s->top); + BlockDriverState *commit_top_backing_bs; =20 if (s->chain_frozen) { bdrv_graph_rdlock_main_loop(); @@ -94,8 +95,12 @@ static void commit_abort(Job *job) * XXX Can (or should) we somehow keep 'consistent read' blocked even * after the failed/cancelled commit job is gone? If we already wrote * something to base, the intermediate images aren't valid any more. */ - bdrv_replace_node(s->commit_top_bs, s->commit_top_bs->backing->bs, - &error_abort); + commit_top_backing_bs =3D s->commit_top_bs->backing->bs; + bdrv_drained_begin(commit_top_backing_bs); + bdrv_graph_wrlock(commit_top_backing_bs); + bdrv_replace_node(s->commit_top_bs, commit_top_backing_bs, &error_abor= t); + bdrv_graph_wrunlock(); + bdrv_drained_end(commit_top_backing_bs); =20 bdrv_unref(s->commit_top_bs); bdrv_unref(top_bs); @@ -425,7 +430,11 @@ fail: /* commit_top_bs has to be replaced after deleting the block job, * otherwise this would fail because of lack of permissions. */ if (commit_top_bs) { + bdrv_drained_begin(top); + bdrv_graph_wrlock(top); bdrv_replace_node(commit_top_bs, top, &error_abort); + bdrv_graph_wrunlock(); + bdrv_drained_end(top); } } =20 diff --git a/block/mirror.c b/block/mirror.c index f8e439371c..dfc1c416e8 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -712,6 +712,7 @@ static int mirror_exit_common(Job *job) * these permissions any more means that we can't allow any new reques= ts on * mirror_top_bs from now on, so keep it drained. */ bdrv_drained_begin(mirror_top_bs); + bdrv_drained_begin(target_bs); bs_opaque->stop =3D true; =20 bdrv_graph_rdlock_main_loop(); @@ -757,15 +758,13 @@ static int mirror_exit_common(Job *job) /* The mirror job has no requests in flight any more, but we need = to * drain potential other users of the BDS before changing the grap= h. */ assert(s->in_drain); - bdrv_drained_begin(target_bs); + bdrv_drained_begin(to_replace); /* * Cannot use check_to_replace_node() here, because that would * check for an op blocker on @to_replace, and we have our own * there. - * - * TODO Pull out the writer lock from bdrv_replace_node() to here */ - bdrv_graph_rdlock_main_loop(); + bdrv_graph_wrlock(target_bs); if (bdrv_recurse_can_replace(src, to_replace)) { bdrv_replace_node(to_replace, target_bs, &local_err); } else { @@ -774,8 +773,8 @@ static int mirror_exit_common(Job *job) "would not lead to an abrupt change of visible data= ", to_replace->node_name, target_bs->node_name); } - bdrv_graph_rdunlock_main_loop(); - bdrv_drained_end(target_bs); + bdrv_graph_wrunlock(); + bdrv_drained_end(to_replace); if (local_err) { error_report_err(local_err); ret =3D -EPERM; @@ -790,7 +789,6 @@ static int mirror_exit_common(Job *job) aio_context_release(replace_aio_context); } g_free(s->replaces); - bdrv_unref(target_bs); =20 /* * Remove the mirror filter driver from the graph. Before this, get ri= d of @@ -798,7 +796,12 @@ static int mirror_exit_common(Job *job) * valid. */ block_job_remove_all_bdrv(bjob); + bdrv_graph_wrlock(mirror_top_bs); bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs, &error_ab= ort); + bdrv_graph_wrunlock(); + + bdrv_drained_end(target_bs); + bdrv_unref(target_bs); =20 bs_opaque->job =3D NULL; =20 @@ -1987,11 +1990,14 @@ fail: } =20 bs_opaque->stop =3D true; - bdrv_graph_rdlock_main_loop(); + bdrv_drained_begin(bs); + bdrv_graph_wrlock(bs); + assert(mirror_top_bs->backing->bs =3D=3D bs); bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing, &error_abort); - bdrv_graph_rdunlock_main_loop(); - bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs, &error_ab= ort); + bdrv_replace_node(mirror_top_bs, bs, &error_abort); + bdrv_graph_wrunlock(); + bdrv_drained_end(bs); =20 bdrv_unref(mirror_top_bs); =20 diff --git a/blockdev.c b/blockdev.c index f04faf6373..5bc921236c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1610,7 +1610,12 @@ static void external_snapshot_abort(void *opaque) aio_context_acquire(aio_context); } =20 + bdrv_drained_begin(state->new_bs); + bdrv_graph_wrlock(state->old_bs); bdrv_replace_node(state->new_bs, state->old_bs, &error_abort); + bdrv_graph_wrunlock(); + bdrv_drained_end(state->new_bs); + bdrv_unref(state->old_bs); /* bdrv_replace_node() ref'ed old_b= s */ =20 aio_context_release(aio_context); diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index 40d17b4c5a..b16f831c23 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -2000,7 +2000,13 @@ static void do_test_replace_child_mid_drain(int old_= drain_count, parent_s->was_undrained =3D false; =20 g_assert(parent_bs->quiesce_counter =3D=3D old_drain_count); + bdrv_drained_begin(old_child_bs); + bdrv_drained_begin(new_child_bs); + bdrv_graph_wrlock(NULL); bdrv_replace_node(old_child_bs, new_child_bs, &error_abort); + bdrv_graph_wrunlock(); + bdrv_drained_end(new_child_bs); + bdrv_drained_end(old_child_bs); g_assert(parent_bs->quiesce_counter =3D=3D new_drain_count); =20 if (!old_drain_count && !new_drain_count) { diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-= mod.c index 8609f7f42b..22d4cd83f6 100644 --- a/tests/unit/test-bdrv-graph-mod.c +++ b/tests/unit/test-bdrv-graph-mod.c @@ -234,11 +234,16 @@ static void test_parallel_exclusive_write(void) BlockDriverState *fl1 =3D pass_through_node("fl1"); BlockDriverState *fl2 =3D pass_through_node("fl2"); =20 + bdrv_drained_begin(fl1); + bdrv_drained_begin(fl2); + /* * bdrv_attach_child() eats child bs reference, so we need two @base - * references for two filters: + * references for two filters. We also need an additional @fl1 referen= ce so + * that it still exists when we want to undrain it. */ bdrv_ref(base); + bdrv_ref(fl1); =20 bdrv_graph_wrlock(NULL); bdrv_attach_child(top, fl1, "backing", &child_of_bds, @@ -250,10 +255,14 @@ static void test_parallel_exclusive_write(void) bdrv_attach_child(fl2, base, "backing", &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, &error_abort); - bdrv_graph_wrunlock(); =20 bdrv_replace_node(fl1, fl2, &error_abort); + bdrv_graph_wrunlock(); + + bdrv_drained_end(fl2); + bdrv_drained_end(fl1); =20 + bdrv_unref(fl1); bdrv_unref(fl2); bdrv_unref(top); } --=20 2.41.0