From patchwork Tue Sep 24 05:33:02 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Steinhardt X-Patchwork-Id: 13810116 Received: from fout-a8-smtp.messagingengine.com (fout-a8-smtp.messagingengine.com [103.168.172.151]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 64C072E859 for ; Tue, 24 Sep 2024 05:33:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=103.168.172.151 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727155991; cv=none; b=oSadLuwVAoYeS6VvmzsRJHmgM1TV6S1GOy74zsUcPB3hY168ICuW7gWZHrYgR99zNTpcdDFdbNceqT/PtvLVxU+pqbUFNvs413DAI0aw7CpgX29gHk2+yrjTXqAcxpyyzr42lMn9ctBiKZO6PAYX26DvaJSXthIwJLpRkE82az4= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727155991; c=relaxed/simple; bh=OYHprMcU3KfL1NT+BK3QDx+jtqRsldwhCV6VXqAEDm4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Td3myJd2TRZqW5+hwdlLVMVshkyCpgw7FTRPXJVWas1JG6Evt8o53pQItkc0sbz+xCplxZP3Bp6J3KyTUtcyOJWtujTNpwRzM+7G7RRBG2wkYkfoS3XFi0mRfmfOHesSxmNGxP5FrW7DlKLG01SpSNLp9r5+axdKdBlYgF7BiR8= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im; spf=pass smtp.mailfrom=pks.im; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b=NIZiWXvF; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=V4jtvswl; arc=none smtp.client-ip=103.168.172.151 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=pks.im Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b="NIZiWXvF"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="V4jtvswl" Received: from phl-compute-02.internal (phl-compute-02.phl.internal [10.202.2.42]) by mailfout.phl.internal (Postfix) with ESMTP id 158C8138019F; Tue, 24 Sep 2024 01:33:07 -0400 (EDT) Received: from phl-mailfrontend-01 ([10.202.2.162]) by phl-compute-02.internal (MEProxy); Tue, 24 Sep 2024 01:33:07 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=cc:cc :content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm2; t=1727155987; x=1727242387; bh=OIiz8siCq3 svQraTGa8A6pzjhnpaCX0GstoY9rk1/RI=; b=NIZiWXvFxTh551Y0OpmBnriYZt 1YEbkbB2AV718Css8b8PtXhbRZGEdT2zGz40nD2w3nAcM3Z+/K8eLVQ0Wl7K9Xxw Y2HRPEFKmwfuGsyybmpU+L5v4xAu6uvUScBtwPTHr4IQ/NAveC0Q6rscVK6TZC7I 2nDqtIufR0JgP3EvGx8MVfskHTRDwWa38IB1p0DqODNbK3Qy0gOBQMU7Bf6hj/Vr N86D3lIR8ZoLRID84sPhiTIWnfms0Hu4X8ePiH3/ccUOu9p9FzMHbFAFFeXR7yLC VdjbApuZ8+pys+Tyw9WBdZe1BC13ejacFLT5x/6buJivdWOMLTXA3AoU1YSA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm2; t=1727155987; x=1727242387; bh=OIiz8siCq3svQraTGa8A6pzjhnpa CX0GstoY9rk1/RI=; b=V4jtvswlPhqjfhlVXEIkijmZvSIuuX5/54eUOskAO164 pdOXqGnzaJhLxrhomi/Y+gU6QAMVyRtnx6RD4sdMhErlheES7IxdA3VD224GLhmE MaFKubX+Km9IMJZGMWTQYGKTRAe+rVI8qS1BRiyR3Tj+fQmB1N9qqztoR9V88nB1 2HWWsHNDuIjuN4UXIgITGXXPooFgXtPls350ZDWoN8L+uTynYU7wbvEzReynaCis N3B4Jeg5OfSODbUtxRalkexG/DaMJyBnUYHSEg6kU517AaYaQjXRCPX1DpAUQB86 sdHigMCm7Drj0KjW9ZJI9Xhp/XmDyB9KfeevSpe8Dg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeftddrvddtuddgieehucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdggtfgfnhhsuhgsshgtrhhisggvpdfu rfetoffkrfgpnffqhgenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnh htshculddquddttddmnecujfgurhepfffhvfevuffkfhggtggujgesthdtredttddtvden ucfhrhhomheprfgrthhrihgtkhcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimh eqnecuggftrfgrthhtvghrnhepveekkeffhfeitdeludeigfejtdetvdelvdduhefgueeg udfghfeukefhjedvkedtnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrg hilhhfrhhomhepphhssehpkhhsrdhimhdpnhgspghrtghpthhtohephedpmhhouggvpehs mhhtphhouhhtpdhrtghpthhtohepshhunhhshhhinhgvsehsuhhnshhhihhnvggtohdrtg homhdprhgtphhtthhopehgihhtshhtvghrsehpohgsohigrdgtohhmpdhrtghpthhtohep ghhithesvhhgvghrrdhkvghrnhgvlhdrohhrghdprhgtphhtthhopehjrghmvghssehjrg hmvghslhhiuhdrihhopdhrtghpthhtohepkhgrrhhthhhikhdrudekkeesghhmrghilhdr tghomh X-ME-Proxy: Feedback-ID: i197146af:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 24 Sep 2024 01:33:05 -0400 (EDT) Received: by vm-mail (OpenSMTPD) with ESMTPSA id f2cfab99 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Tue, 24 Sep 2024 05:32:32 +0000 (UTC) Date: Tue, 24 Sep 2024 07:33:02 +0200 From: Patrick Steinhardt To: git@vger.kernel.org Cc: karthik nayak , Eric Sunshine , James Liu , Junio C Hamano Subject: [PATCH v4 1/3] refs/reftable: introduce "reftable.lockTimeout" Message-ID: <77cffd3b1eb638e05c031e2949fdc9374d599e05.1727155858.git.ps@pks.im> References: Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: When multiple concurrent processes try to update references in a repository they may try to lock the same lockfiles. This can happen even when the updates are non-conflicting and can both be applied, so it doesn't always make sense to abort the transaction immediately. Both the "loose" and "packed" backends thus have a grace period that they wait for the lock to be released that can be controlled via the config values "core.filesRefLockTimeout" and "core.packedRefsTimeout", respectively. The reftable backend doesn't have such a setting yet and instead fails immediately when it sees such a lock. But the exact same concepts apply here as they do apply to the other backends. Introduce a new "reftable.lockTimeout" config that controls how long we may wait for a "tables.list" lock to be released. The default value of this config is 100ms, which is the same default as we have it for the "loose" backend. Note that even though we also lock individual tables, this config really only applies to the "tables.list" file. This is because individual tables are only ever locked when we already hold the "tables.list" lock during compaction. When we observe such a lock we in fact do not want to compact the table at all because it is already in the process of being compacted by a concurrent process. So applying the same timeout here would not make any sense and only delay progress. Signed-off-by: Patrick Steinhardt --- Documentation/config/reftable.txt | 8 ++++++++ refs/reftable-backend.c | 8 ++++++++ reftable/reftable-writer.h | 11 +++++++++++ reftable/stack.c | 18 ++++++++++++------ t/t0610-reftable-basics.sh | 27 +++++++++++++++++++++++++++ 5 files changed, 66 insertions(+), 6 deletions(-) diff --git a/Documentation/config/reftable.txt b/Documentation/config/reftable.txt index 05157279778..57087803a54 100644 --- a/Documentation/config/reftable.txt +++ b/Documentation/config/reftable.txt @@ -46,3 +46,11 @@ reftable.geometricFactor:: By default, the geometric sequence uses a factor of 2, meaning that for any table, the next-biggest table must at least be twice as big. A maximum factor of 256 is supported. + +reftable.lockTimeout:: + Whenever the reftable backend appends a new table to the stack, it has + to lock the central "tables.list" file before updating it. This config + controls how long the process will wait to acquire the lock in case + another process has already acquired it. Value 0 means not to retry at + all; -1 means to try indefinitely. Default is 100 (i.e., retry for + 100ms). diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 1c4b19e737f..ca281e39a29 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -256,6 +256,13 @@ static int reftable_be_config(const char *var, const char *value, if (factor > UINT8_MAX) die("reftable geometric factor cannot exceed %u", (unsigned)UINT8_MAX); opts->auto_compaction_factor = factor; + } else if (!strcmp(var, "reftable.locktimeout")) { + int64_t lock_timeout = git_config_int64(var, value, ctx->kvi); + if (lock_timeout > LONG_MAX) + die("reftable lock timeout cannot exceed %"PRIdMAX, (intmax_t)LONG_MAX); + if (lock_timeout < 0 && lock_timeout != -1) + die("reftable lock timeout does not support negative values other than -1"); + opts->lock_timeout_ms = lock_timeout; } return 0; @@ -281,6 +288,7 @@ static struct ref_store *reftable_be_init(struct repository *repo, refs->write_options.default_permissions = calc_shared_perm(0666 & ~mask); refs->write_options.disable_auto_compact = !git_env_bool("GIT_TEST_REFTABLE_AUTOCOMPACTION", 1); + refs->write_options.lock_timeout_ms = 100; git_config(reftable_be_config, &refs->write_options); diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h index 189b1f4144f..f5e25cfda16 100644 --- a/reftable/reftable-writer.h +++ b/reftable/reftable-writer.h @@ -51,6 +51,17 @@ struct reftable_write_options { * tables to compact. Defaults to 2 if unset. */ uint8_t auto_compaction_factor; + + /* + * The number of milliseconds to wait when trying to lock "tables.list". + * Note that this does not apply to locking individual tables, as these + * should only ever be locked when already holding the "tables.list" + * lock. + * + * Passing 0 will fail immediately when the file is locked, passing a + * negative value will cause us to block indefinitely. + */ + long lock_timeout_ms; }; /* reftable_block_stats holds statistics for a single block type */ diff --git a/reftable/stack.c b/reftable/stack.c index ce0a35216ba..5ccad2cff31 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -603,8 +603,10 @@ static int reftable_stack_init_addition(struct reftable_addition *add, add->stack = st; - err = hold_lock_file_for_update(&add->tables_list_lock, st->list_file, - LOCK_NO_DEREF); + err = hold_lock_file_for_update_timeout(&add->tables_list_lock, + st->list_file, + LOCK_NO_DEREF, + st->opts.lock_timeout_ms); if (err < 0) { if (errno == EEXIST) { err = REFTABLE_LOCK_ERROR; @@ -1056,8 +1058,10 @@ static int stack_compact_range(struct reftable_stack *st, * Hold the lock so that we can read "tables.list" and lock all tables * which are part of the user-specified range. */ - err = hold_lock_file_for_update(&tables_list_lock, st->list_file, - LOCK_NO_DEREF); + err = hold_lock_file_for_update_timeout(&tables_list_lock, + st->list_file, + LOCK_NO_DEREF, + st->opts.lock_timeout_ms); if (err < 0) { if (errno == EEXIST) err = REFTABLE_LOCK_ERROR; @@ -1156,8 +1160,10 @@ static int stack_compact_range(struct reftable_stack *st, * "tables.list". We'll then replace the compacted range of tables with * the new table. */ - err = hold_lock_file_for_update(&tables_list_lock, st->list_file, - LOCK_NO_DEREF); + err = hold_lock_file_for_update_timeout(&tables_list_lock, + st->list_file, + LOCK_NO_DEREF, + st->opts.lock_timeout_ms); if (err < 0) { if (errno == EEXIST) err = REFTABLE_LOCK_ERROR; diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index 37510c2b2a5..62da3e37823 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -423,6 +423,33 @@ test_expect_success 'ref transaction: fails gracefully when auto compaction fail ) ' +test_expect_success 'ref transaction: timeout acquiring tables.list lock' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit initial && + >.git/reftable/tables.list.lock && + test_must_fail git update-ref refs/heads/branch HEAD 2>err && + test_grep "cannot lock references" err + ) +' + +test_expect_success 'ref transaction: retry acquiring tables.list lock' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit initial && + LOCK=.git/reftable/tables.list.lock && + >$LOCK && + { + ( sleep 1 && rm -f $LOCK ) & + } && + git -c reftable.lockTimeout=5000 update-ref refs/heads/branch HEAD + ) +' + test_expect_success 'pack-refs: compacts tables' ' test_when_finished "rm -rf repo" && git init repo && From patchwork Tue Sep 24 05:33:05 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Steinhardt X-Patchwork-Id: 13810117 Received: from fout-a8-smtp.messagingengine.com (fout-a8-smtp.messagingengine.com [103.168.172.151]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 89BA544C68 for ; Tue, 24 Sep 2024 05:33:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=103.168.172.151 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727155993; cv=none; b=a9kW3w8UHJ0PHJFOm5Bk+LxojNeSDjQfmCBzISFUmK3MmGCpigrjRn3W3lvvbXET67OtiQ9Endq89HQndDK9smkpN51mDSFkCdBxKrlA7cpt71t8f3Bwqh6mZStsAG2mGOkGcumQvE5iRH9/XdO+U5t6RnTcFlYD4TzP1cJzyxM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727155993; c=relaxed/simple; bh=W1Lo7MaVPazKghuou8iJkwWKPsVnPlV2WWBwpHSASXg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=gIzNeMBlFoGUlQahxApW3TqfLhZsBjcZNaQyz2EtuB1FEtwDUJYNBJVJi8HzByGiB6i8zs6CpYXIGVK1HVSMPMtBI/41jENP22iL/Zbg+FVvaG5ptvZuMLIKyWahn8tcb6F/1JQ+w/IZPMb5+0m2KJKUCzySuemLQ8P1W8hcXPk= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im; spf=pass smtp.mailfrom=pks.im; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b=JWnpZhca; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=J/KE534A; arc=none smtp.client-ip=103.168.172.151 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=pks.im Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b="JWnpZhca"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="J/KE534A" Received: from phl-compute-06.internal (phl-compute-06.phl.internal [10.202.2.46]) by mailfout.phl.internal (Postfix) with ESMTP id 8B3C213802F2; Tue, 24 Sep 2024 01:33:10 -0400 (EDT) Received: from phl-mailfrontend-01 ([10.202.2.162]) by phl-compute-06.internal (MEProxy); Tue, 24 Sep 2024 01:33:10 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=cc:cc :content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm2; t=1727155990; x=1727242390; bh=ok6m0JBIS6 XCs7P8j6gwzg3Gz3Q0Mq6D9+b4UpU6c1c=; b=JWnpZhcaDcIFgrfk63PxP1OiE6 hsEtkRpvGlamzlI5yLMkHjAgUNXWO5AbVT6PQNS2MfeOTqbmBOBkxmcp1wMNGVQH OD4DNbZ/JFrQCzw8e0MLDoMqjngVEEXVGV4YCq8qnI15JflXCnyT11nBlHPG4/iQ 90k2c0uJDU03Qfxe3cGroOrhbCoVoEB+BVYyN6LCk9BHC/a3CHBz8X1vFA4CtF46 5Eq9C5m6pY0gq6/E1kwEgKZRuvD5kboAsz+PrQjWetGQfia65uAuJiRSvfqlenFp FW92vl2NHW4V2ERO4Oug25HD/Bxs94ZrPL42Egn6wAgQY9wyn18ULB5IgaPw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm2; t=1727155990; x=1727242390; bh=ok6m0JBIS6XCs7P8j6gwzg3Gz3Q0 Mq6D9+b4UpU6c1c=; b=J/KE534A/gW9nSswAtvaxlIurgy6HlPRqQ5iy+1OP8TI EqxS76MUJ8rE7W51oA2SsQOl3UPEl4j4/KccqM1cXTrfs0tGi9XG+E7ouB1nicTP gwoPE4YISvUXrFCrmEwbD/fQqWXNyY395wI/S8Q6LL0BeJq6dn8YVoAMWuFjsAkL +mh9ghkb3DGbS60jKsnFXpQgn5MgCp7w3iOaH3oZxUe6Ge+/2HNAz9G9EPZ8140w O3XpU0FiCaJ9ACBmGl2eprpuJeeVDDuQ5ZF20aK6Tisgu/IJPyjNB8eF5TPnzXWy VoQOeI4MR/flbIlqqE8vE9kLfTGVN5AE0FAxWYfoow== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeftddrvddtuddgieehucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdggtfgfnhhsuhgsshgtrhhisggvpdfu rfetoffkrfgpnffqhgenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnh htshculddquddttddmnecujfgurhepfffhvfevuffkfhggtggujgesthdtredttddtvden ucfhrhhomheprfgrthhrihgtkhcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimh eqnecuggftrfgrthhtvghrnhepveekkeffhfeitdeludeigfejtdetvdelvdduhefgueeg udfghfeukefhjedvkedtnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrg hilhhfrhhomhepphhssehpkhhsrdhimhdpnhgspghrtghpthhtohephedpmhhouggvpehs mhhtphhouhhtpdhrtghpthhtohepghhithesvhhgvghrrdhkvghrnhgvlhdrohhrghdprh gtphhtthhopehgihhtshhtvghrsehpohgsohigrdgtohhmpdhrtghpthhtohepshhunhhs hhhinhgvsehsuhhnshhhihhnvggtohdrtghomhdprhgtphhtthhopehkrghrthhhihhkrd dukeeksehgmhgrihhlrdgtohhmpdhrtghpthhtohepjhgrmhgvshesjhgrmhgvshhlihhu rdhioh X-ME-Proxy: Feedback-ID: i197146af:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 24 Sep 2024 01:33:09 -0400 (EDT) Received: by vm-mail (OpenSMTPD) with ESMTPSA id c2161ab3 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Tue, 24 Sep 2024 05:32:35 +0000 (UTC) Date: Tue, 24 Sep 2024 07:33:05 +0200 From: Patrick Steinhardt To: git@vger.kernel.org Cc: karthik nayak , Eric Sunshine , James Liu , Junio C Hamano Subject: [PATCH v4 2/3] reftable/stack: allow locking of outdated stacks Message-ID: <81a836062e95135b4700ed2de93b6f047592624e.1727155858.git.ps@pks.im> References: Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: In `reftable_stack_new_addition()` we first lock the stack and then check whether it is still up-to-date. If it is not we return an error to the caller indicating that the stack is outdated. This is overly restrictive in our ref transaction interface though: we lock the stack right before we start to verify the transaction, so we do not really care whether it is outdated or not. What we really want is that the stack is up-to-date after it has been locked so that we can verify queued updates against its current state while we know that it is locked for concurrent modification. Introduce a new flag `REFTABLE_STACK_NEW_ADDITION_RELOAD` that alters the behaviour of `reftable_stack_init_addition()` in this case: when we notice that it is out-of-date we reload it instead of returning an error to the caller. This logic will be wired up in the reftable backend in the next commit. Signed-off-by: Patrick Steinhardt --- refs/reftable-backend.c | 4 +- reftable/reftable-stack.h | 13 ++++++- reftable/stack.c | 20 ++++++---- t/unit-tests/t-reftable-stack.c | 67 ++++++++++++++++++++++++++++++++- 4 files changed, 91 insertions(+), 13 deletions(-) diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index ca281e39a29..6ca00627dd7 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -770,7 +770,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out, if (ret) return ret; - ret = reftable_stack_new_addition(&addition, stack); + ret = reftable_stack_new_addition(&addition, stack, 0); if (ret) { if (ret == REFTABLE_LOCK_ERROR) strbuf_addstr(err, "cannot lock references"); @@ -2207,7 +2207,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, if (ret < 0) goto done; - ret = reftable_stack_new_addition(&add, stack); + ret = reftable_stack_new_addition(&add, stack, 0); if (ret < 0) goto done; diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h index f4f8cabc7fb..6370fe45ddf 100644 --- a/reftable/reftable-stack.h +++ b/reftable/reftable-stack.h @@ -37,12 +37,21 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st); /* holds a transaction to add tables at the top of a stack. */ struct reftable_addition; +enum { + /* + * Reload the stack when the stack is out-of-date after locking it. + */ + REFTABLE_STACK_NEW_ADDITION_RELOAD = (1 << 0), +}; + /* * returns a new transaction to add reftables to the given stack. As a side - * effect, the ref database is locked. + * effect, the ref database is locked. Accepts REFTABLE_STACK_NEW_ADDITION_* + * flags. */ int reftable_stack_new_addition(struct reftable_addition **dest, - struct reftable_stack *st); + struct reftable_stack *st, + unsigned int flags); /* Adds a reftable to transaction. */ int reftable_addition_add(struct reftable_addition *add, diff --git a/reftable/stack.c b/reftable/stack.c index 5ccad2cff31..84cf37a2ad0 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -596,7 +596,8 @@ struct reftable_addition { #define REFTABLE_ADDITION_INIT {0} static int reftable_stack_init_addition(struct reftable_addition *add, - struct reftable_stack *st) + struct reftable_stack *st, + unsigned int flags) { struct strbuf lock_file_name = STRBUF_INIT; int err; @@ -626,6 +627,11 @@ static int reftable_stack_init_addition(struct reftable_addition *add, err = stack_uptodate(st); if (err < 0) goto done; + if (err > 0 && flags & REFTABLE_STACK_NEW_ADDITION_RELOAD) { + err = reftable_stack_reload_maybe_reuse(add->stack, 1); + if (err) + goto done; + } if (err > 0) { err = REFTABLE_OUTDATED_ERROR; goto done; @@ -633,9 +639,8 @@ static int reftable_stack_init_addition(struct reftable_addition *add, add->next_update_index = reftable_stack_next_update_index(st); done: - if (err) { + if (err) reftable_addition_close(add); - } strbuf_release(&lock_file_name); return err; } @@ -739,13 +744,14 @@ int reftable_addition_commit(struct reftable_addition *add) } int reftable_stack_new_addition(struct reftable_addition **dest, - struct reftable_stack *st) + struct reftable_stack *st, + unsigned int flags) { int err = 0; struct reftable_addition empty = REFTABLE_ADDITION_INIT; REFTABLE_CALLOC_ARRAY(*dest, 1); **dest = empty; - err = reftable_stack_init_addition(*dest, st); + err = reftable_stack_init_addition(*dest, st, flags); if (err) { reftable_free(*dest); *dest = NULL; @@ -759,7 +765,7 @@ static int stack_try_add(struct reftable_stack *st, void *arg) { struct reftable_addition add = REFTABLE_ADDITION_INIT; - int err = reftable_stack_init_addition(&add, st); + int err = reftable_stack_init_addition(&add, st, 0); if (err < 0) goto done; @@ -1608,7 +1614,7 @@ static int reftable_stack_clean_locked(struct reftable_stack *st) int reftable_stack_clean(struct reftable_stack *st) { struct reftable_addition *add = NULL; - int err = reftable_stack_new_addition(&add, st); + int err = reftable_stack_new_addition(&add, st, 0); if (err < 0) { goto done; } diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c index d62a9c1bed5..a37cc698d87 100644 --- a/t/unit-tests/t-reftable-stack.c +++ b/t/unit-tests/t-reftable-stack.c @@ -271,7 +271,7 @@ static void t_reftable_stack_transaction_api(void) reftable_addition_destroy(add); - err = reftable_stack_new_addition(&add, st); + err = reftable_stack_new_addition(&add, st, 0); check(!err); err = reftable_addition_add(add, write_test_ref, &ref); @@ -292,6 +292,68 @@ static void t_reftable_stack_transaction_api(void) clear_dir(dir); } +static void t_reftable_stack_transaction_with_reload(void) +{ + char *dir = get_tmp_dir(__LINE__); + struct reftable_stack *st1 = NULL, *st2 = NULL; + int err; + struct reftable_addition *add = NULL; + struct reftable_ref_record refs[2] = { + { + .refname = (char *) "refs/heads/a", + .update_index = 1, + .value_type = REFTABLE_REF_VAL1, + .value.val1 = { '1' }, + }, + { + .refname = (char *) "refs/heads/b", + .update_index = 2, + .value_type = REFTABLE_REF_VAL1, + .value.val1 = { '1' }, + }, + }; + struct reftable_ref_record ref = { 0 }; + + err = reftable_new_stack(&st1, dir, NULL); + check(!err); + err = reftable_new_stack(&st2, dir, NULL); + check(!err); + + err = reftable_stack_new_addition(&add, st1, 0); + check(!err); + err = reftable_addition_add(add, write_test_ref, &refs[0]); + check(!err); + err = reftable_addition_commit(add); + check(!err); + reftable_addition_destroy(add); + + /* + * The second stack is now outdated, which we should notice. We do not + * create the addition and lock the stack by default, but allow the + * reload to happen when REFTABLE_STACK_NEW_ADDITION_RELOAD is set. + */ + err = reftable_stack_new_addition(&add, st2, 0); + check_int(err, ==, REFTABLE_OUTDATED_ERROR); + err = reftable_stack_new_addition(&add, st2, REFTABLE_STACK_NEW_ADDITION_RELOAD); + check(!err); + err = reftable_addition_add(add, write_test_ref, &refs[1]); + check(!err); + err = reftable_addition_commit(add); + check(!err); + reftable_addition_destroy(add); + + for (size_t i = 0; i < ARRAY_SIZE(refs); i++) { + err = reftable_stack_read_ref(st2, refs[i].refname, &ref); + check(!err); + check(reftable_ref_record_equal(&refs[i], &ref, GIT_SHA1_RAWSZ)); + } + + reftable_ref_record_release(&ref); + reftable_stack_destroy(st1); + reftable_stack_destroy(st2); + clear_dir(dir); +} + static void t_reftable_stack_transaction_api_performs_auto_compaction(void) { char *dir = get_tmp_dir(__LINE__); @@ -322,7 +384,7 @@ static void t_reftable_stack_transaction_api_performs_auto_compaction(void) */ st->opts.disable_auto_compact = i != n; - err = reftable_stack_new_addition(&add, st); + err = reftable_stack_new_addition(&add, st, 0); check(!err); err = reftable_addition_add(add, write_test_ref, &ref); @@ -1314,6 +1376,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED) TEST(t_reftable_stack_reload_with_missing_table(), "stack iteration with garbage tables"); TEST(t_reftable_stack_tombstone(), "'tombstone' refs in stack"); TEST(t_reftable_stack_transaction_api(), "update transaction to stack"); + TEST(t_reftable_stack_transaction_with_reload(), "transaction with reload"); TEST(t_reftable_stack_transaction_api_performs_auto_compaction(), "update transaction triggers auto-compaction"); TEST(t_reftable_stack_update_index_check(), "update transactions with equal update indices"); TEST(t_reftable_stack_uptodate(), "stack must be reloaded before ref update"); From patchwork Tue Sep 24 05:33:08 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Steinhardt X-Patchwork-Id: 13810118 Received: from fout-a8-smtp.messagingengine.com (fout-a8-smtp.messagingengine.com [103.168.172.151]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D7E022E859 for ; Tue, 24 Sep 2024 05:33:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=103.168.172.151 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727155995; cv=none; b=DDA7pus70t4H1yudf4m0useu9QpyjFnF/fJ1tQDYErIfXWlW6+UaNiRMPvAHYj7VJ0rZTh3jcEcRJS+6OKbaXji6vnZ68kNOSqRzYTyu6ZMg9uV1E7+73mWbgUWQBdZfh4OYqQYZj5WniztqUoLabaok0qRe5GCcwd6nKQwZkD4= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727155995; c=relaxed/simple; bh=rX69GNu/VuXRnl2fD04QhRWxMnOE/m98dmtZ8/6xaxU=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=c9Ph0UwKLdUsjjkpKTsnK90RXa/1fvH0E1JRIT/uWnPVBrSHm1elF0y4kpJ2aj2SDp8h5d+juSew/TsJONFSrVxv9PeAV39Zl37yyjGw3EbtaE+g6u5zKDNOUo0YkPzPINDpwpRqMcJ5S0F468uCKPDjv4rbqhAET3avnlUpxws= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im; spf=pass smtp.mailfrom=pks.im; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b=mA8C7AQc; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=ebtaf1r+; arc=none smtp.client-ip=103.168.172.151 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=pks.im Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b="mA8C7AQc"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="ebtaf1r+" Received: from phl-compute-05.internal (phl-compute-05.phl.internal [10.202.2.45]) by mailfout.phl.internal (Postfix) with ESMTP id 025E213802E6; Tue, 24 Sep 2024 01:33:13 -0400 (EDT) Received: from phl-mailfrontend-01 ([10.202.2.162]) by phl-compute-05.internal (MEProxy); Tue, 24 Sep 2024 01:33:13 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=cc:cc :content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm2; t=1727155992; x=1727242392; bh=Inh0ixxUlX AJDUcsFWk1eb51WK+4Yjh7AnLuc0SH6dw=; b=mA8C7AQcAyUXhg4bBzsfdI5DKo nhW1Nl399Rzjk7EvTKdobUqkM1mz6JHYyL9nXkf2zebUUeBF0RpMAGetilaUgb/U waPUL9K8w5BXVAxAJYYBNGrrAUytIOCTNx+AqQ3JDunSbfRo/8BR3GspbRB5+HMu S0Vp5oLEfDfXzVTBPIzGLS1C7ZWauQP2Gi3z+5a+auXl/ceFSrUh8sT2+mpxdYov TabkcJo/eY6zgQh+vLrdfQnfXElVAdNkiiOmnUiomJg337x/iK4trX5Van9NaLZY 3ynoY70jH5QQL0cQ5Om/zXajdC1OFa37heFRpnTy2JeUISkOEs4D3YYVy1hQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm2; t=1727155992; x=1727242392; bh=Inh0ixxUlXAJDUcsFWk1eb51WK+4 Yjh7AnLuc0SH6dw=; b=ebtaf1r+wCQMmtcJlveTLNDmrDODDfsPelx6+ULUJf0k RM7zQ2vZDWZQkNWCzt91EgzjNKtbKyO7l/PNdSGn3VShDes5LO09N00N2S7sVcKO xE1R6XS+SfeUa5bgL6H6k1FC5wZFBWHPDQGKb0mDZDx9sfz0TPuToftK9gfgn/wj OJFCffTPscW1E4D3j46ZoAe9rh4SZsRocA+ZpHrDfDba+Ykm8/3dTeDOGi3wa8rB gU2fraX1coguSx4VHomcImGuvHi5YrkzNMmW7FogCfPWs0jTHGzz7RJ46+75zUJP QWcunvNNep0BMyo5e0BAyR7yljuG+ZhPCMt2yKs8CQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeftddrvddtuddgieehucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdggtfgfnhhsuhgsshgtrhhisggvpdfu rfetoffkrfgpnffqhgenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnh htshculddquddttddmnecujfgurhepfffhvfevuffkfhggtggujgesthdtredttddtvden ucfhrhhomheprfgrthhrihgtkhcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimh eqnecuggftrfgrthhtvghrnhepveekkeffhfeitdeludeigfejtdetvdelvdduhefgueeg udfghfeukefhjedvkedtnecuvehluhhsthgvrhfuihiivgepudenucfrrghrrghmpehmrg hilhhfrhhomhepphhssehpkhhsrdhimhdpnhgspghrtghpthhtohephedpmhhouggvpehs mhhtphhouhhtpdhrtghpthhtohepghhithhsthgvrhesphhosghogidrtghomhdprhgtph htthhopehsuhhnshhhihhnvgesshhunhhshhhinhgvtghordgtohhmpdhrtghpthhtohep ghhithesvhhgvghrrdhkvghrnhgvlhdrohhrghdprhgtphhtthhopehjrghmvghssehjrg hmvghslhhiuhdrihhopdhrtghpthhtohepkhgrrhhthhhikhdrudekkeesghhmrghilhdr tghomh X-ME-Proxy: Feedback-ID: i197146af:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 24 Sep 2024 01:33:11 -0400 (EDT) Received: by vm-mail (OpenSMTPD) with ESMTPSA id 4c150322 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Tue, 24 Sep 2024 05:32:38 +0000 (UTC) Date: Tue, 24 Sep 2024 07:33:08 +0200 From: Patrick Steinhardt To: git@vger.kernel.org Cc: karthik nayak , Eric Sunshine , James Liu , Junio C Hamano Subject: [PATCH v4 3/3] refs/reftable: reload locked stack when preparing transaction Message-ID: <9ce2d18dff2a655365b609dd86ea484a489c717a.1727155858.git.ps@pks.im> References: Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: When starting a reftable transaction we lock all stacks we are about to modify. While it may happen that the stack is out-of-date at this point in time we don't really care: transactional updates encode the expected state of a certain reference, so all that we really want to verify is that the _current_ value matches that expected state. Pass `REFTABLE_STACK_NEW_ADDITION_RELOAD` when locking the stack such that an out-of-date stack will be reloaded after having been locked. This change is safe because all verifications of the expected state happen after this step anyway. Add a testcase that verifies that many writers are now able to write to the stack concurrently without failures and with a deterministic end result. Signed-off-by: Patrick Steinhardt --- refs/reftable-backend.c | 3 ++- t/t0610-reftable-basics.sh | 31 +++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 6ca00627dd7..efe2b0258ca 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -770,7 +770,8 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out, if (ret) return ret; - ret = reftable_stack_new_addition(&addition, stack, 0); + ret = reftable_stack_new_addition(&addition, stack, + REFTABLE_STACK_NEW_ADDITION_RELOAD); if (ret) { if (ret == REFTABLE_LOCK_ERROR) strbuf_addstr(err, "cannot lock references"); diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index 62da3e37823..2d951c8ceb6 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -450,6 +450,37 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' ' ) ' +test_expect_success 'ref transaction: many concurrent writers' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + # Set a high timeout such that a busy CI machine will not abort + # early. 10 seconds should hopefully be ample of time to make + # this non-flaky. + git config set reftable.lockTimeout 10000 && + test_commit --no-tag initial && + + head=$(git rev-parse HEAD) && + for i in $(test_seq 100) + do + printf "%s commit\trefs/heads/branch-%s\n" "$head" "$i" || + return 1 + done >expect && + printf "%s commit\trefs/heads/main\n" "$head" >>expect && + + for i in $(test_seq 100) + do + { git update-ref refs/heads/branch-$i HEAD& } || + return 1 + done && + + wait && + git for-each-ref --sort=v:refname >actual && + test_cmp expect actual + ) +' + test_expect_success 'pack-refs: compacts tables' ' test_when_finished "rm -rf repo" && git init repo &&