{"thread":{"id":"62129","subject":"[PATCH 0/3] reftable: graceful concurrent writes","startedAt":"2024-09-17T13:43:22Z","lastAt":"2024-10-02T10:58:31Z","messageCount":42,"participants":["Patrick Steinhardt","karthik nayak","Eric Sunshine","James Liu","Junio C Hamano","Jeff King","Josh Steadmon"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"502952","messageId":"cover.1726578382.git.ps@pks.im","threadId":"62129","inReplyTo":null,"subject":"[PATCH 0/3] reftable: graceful concurrent writes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T13:43:17Z","receivedAt":"2024-09-17T13:43:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthe reftable backend cannot properly handle concurrent writes due to two\nreasons:\n\n - It will bail out immediately when it sees a locked \"tables.list\"\n file. This is being addressed by introducing a configurable locking\n timeout, similar to how we have it for both loose and packed refs.\n\n - It will bail out when it notices that its stack is out-of-date after\n having locked the \"tables.list\" file. This is addressed by reloading\n the stack as requested after locking it, which is fine because our\n transactional API would verify queued ref updates against their\n expected state after the lock was acquired anyway.\n\nSo with this patch series we can now spawn concurrent writers and they\nare expected to succeed, which is demonstrated by the test added by the\nlast patch.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (3):\n refs/reftable: introduce \"reftable.lockTimeout\"\n reftable/stack: allow locking of outdated stacks\n refs/reftable: reload locked stack when preparing transaction\n\n Documentation/config/reftable.txt | 7 ++++\n refs/reftable-backend.c | 9 ++++-\n reftable/reftable-stack.h | 13 +++++-\n reftable/reftable-writer.h | 8 ++++\n reftable/stack.c | 38 ++++++++++++------\n t/t0610-reftable-basics.sh | 58 ++++++++++++++++++++++++++\n t/unit-tests/t-reftable-stack.c | 67 ++++++++++++++++++++++++++++++-\n 7 files changed, 181 insertions(+), 19 deletions(-)\n\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502953","messageId":"ca3eab99f7ef86d1b7a5b4d4bdb8d2b0a55566e1.1726578382.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1726578382.git.ps@pks.im","subject":"[PATCH 1/3] refs/reftable: introduce \"reftable.lockTimeout\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T13:43:20Z","receivedAt":"2024-09-17T13:43:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When multiple concurrent processes try to update references in a\nrepository they may try to lock the same lockfiles. This can happen even\nwhen the updates are non-conflicting and can both be applied, so it\ndoesn't always make sense to abort the transaction immediately. Both the\n\"loose\" and \"packed\" backends thus have a grace period that they wait\nfor the lock to be released that can be controlled via the config values\n\"core.filesRefLockTimeout\" and \"core.packedRefsTimeout\", respectively.\n\nThe reftable backend doesn't have such a setting yet and instead fails\nimmediately when it sees such a lock. But the exact same concepts apply\nhere as they do apply to the other backends.\n\nIntroduce a new \"reftable.lockTimeout\" config that controls how long we\nmay wait for a \"tables.list\" lock to be released. The default value of\nthis config is 100ms, which is the same default as we have it for the\n\"loose\" backend.\n\nNote that even though we also lock individual tables, this config really\nonly applies to the \"tables.list\" file. This is because individual\ntables are only ever locked when we already hold the \"tables.list\" lock\nduring compaction. When we observe such a lock we in fact do not want to\ncompact the table at all because it is already in the process of being\ncompacted by a concurrent process. So applying the same timeout here\nwould not make any sense and only delay progress.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/config/reftable.txt | 7 +++++++\n refs/reftable-backend.c | 4 ++++\n reftable/reftable-writer.h | 8 ++++++++\n reftable/stack.c | 18 ++++++++++++------\n t/t0610-reftable-basics.sh | 27 +++++++++++++++++++++++++++\n 5 files changed, 58 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config/reftable.txt b/Documentation/config/reftable.txt\nindex 05157279778..6c9449d0232 100644\n--- a/Documentation/config/reftable.txt\n+++ b/Documentation/config/reftable.txt\n@@ -46,3 +46,10 @@ reftable.geometricFactor::\n By default, the geometric sequence uses a factor of 2, meaning that for any\n table, the next-biggest table must at least be twice as big. A maximum factor\n of 256 is supported.\n+\n+reftable.lockTimeout::\n+\tWhenever the reftable backend appends a new table to the stack, it has\n+\tto lock the central \"tables.list\" file before updating it. This config\n+\tcontrols how long the process will wait to acquire the lock in case\n+\tanother process has already acquired it. Default is 1000 (i.e., retry\n+\tfor 1 second).\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 1c4b19e737f..e90ddfb98dd 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -256,6 +256,9 @@ static int reftable_be_config(const char *var, const char *value,\n \t\tif (factor > UINT8_MAX)\n \t\t\tdie(\"reftable geometric factor cannot exceed %u\", (unsigned)UINT8_MAX);\n \t\topts->auto_compaction_factor = factor;\n+\t} else if (!strcmp(var, \"reftable.locktimeout\")) {\n+\t\tunsigned long lock_timeout = git_config_ulong(var, value, ctx->kvi);\n+\t\topts->lock_timeout_ms = lock_timeout;\n \t}\n \n \treturn 0;\n@@ -281,6 +284,7 @@ static struct ref_store *reftable_be_init(struct repository *repo,\n \trefs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);\n \trefs->write_options.disable_auto_compact =\n \t\t!git_env_bool(\"GIT_TEST_REFTABLE_AUTOCOMPACTION\", 1);\n+\trefs->write_options.lock_timeout_ms = 100;\n \n \tgit_config(reftable_be_config, &refs->write_options);\n \ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex 189b1f4144f..d5f2446220b 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -51,6 +51,14 @@ struct reftable_write_options {\n \t * tables to compact. Defaults to 2 if unset.\n \t */\n \tuint8_t auto_compaction_factor;\n+\n+\t/*\n+\t * The number of milliseconds to wait when trying to lock \"tables.list\".\n+\t * Note that this does not apply to locking individual tables, as these\n+\t * should only ever be locked when already holding the \"tables.list\"\n+\t * lock.\n+\t */\n+\tunsigned lock_timeout_ms;\n };\n \n /* reftable_block_stats holds statistics for a single block type */\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ce0a35216ba..5ccad2cff31 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -603,8 +603,10 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \n \tadd->stack = st;\n \n-\terr = hold_lock_file_for_update(&add->tables_list_lock, st->list_file,\n-\t\t\t\t\tLOCK_NO_DEREF);\n+\terr = hold_lock_file_for_update_timeout(&add->tables_list_lock,\n+\t\t\t\t\t\tst->list_file,\n+\t\t\t\t\t\tLOCK_NO_DEREF,\n+\t\t\t\t\t\tst->opts.lock_timeout_ms);\n \tif (err < 0) {\n \t\tif (errno == EEXIST) {\n \t\t\terr = REFTABLE_LOCK_ERROR;\n@@ -1056,8 +1058,10 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * Hold the lock so that we can read \"tables.list\" and lock all tables\n \t * which are part of the user-specified range.\n \t */\n-\terr = hold_lock_file_for_update(&tables_list_lock, st->list_file,\n-\t\t\t\t\tLOCK_NO_DEREF);\n+\terr = hold_lock_file_for_update_timeout(&tables_list_lock,\n+\t\t\t\t\t\tst->list_file,\n+\t\t\t\t\t\tLOCK_NO_DEREF,\n+\t\t\t\t\t\tst->opts.lock_timeout_ms);\n \tif (err < 0) {\n \t\tif (errno == EEXIST)\n \t\t\terr = REFTABLE_LOCK_ERROR;\n@@ -1156,8 +1160,10 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * \"tables.list\". We'll then replace the compacted range of tables with\n \t * the new table.\n \t */\n-\terr = hold_lock_file_for_update(&tables_list_lock, st->list_file,\n-\t\t\t\t\tLOCK_NO_DEREF);\n+\terr = hold_lock_file_for_update_timeout(&tables_list_lock,\n+\t\t\t\t\t\tst->list_file,\n+\t\t\t\t\t\tLOCK_NO_DEREF,\n+\t\t\t\t\t\tst->opts.lock_timeout_ms);\n \tif (err < 0) {\n \t\tif (errno == EEXIST)\n \t\t\terr = REFTABLE_LOCK_ERROR;\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 37510c2b2a5..62da3e37823 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -423,6 +423,33 @@ test_expect_success 'ref transaction: fails gracefully when auto compaction fail\n \t)\n '\n \n+test_expect_success 'ref transaction: timeout acquiring tables.list lock' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit initial &&\n+\t\t>.git/reftable/tables.list.lock &&\n+\t\ttest_must_fail git update-ref refs/heads/branch HEAD 2>err &&\n+\t\ttest_grep \"cannot lock references\" err\n+\t)\n+'\n+\n+test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit initial &&\n+\t\tLOCK=.git/reftable/tables.list.lock &&\n+\t\t>$LOCK &&\n+\t\t{\n+\t\t\t( sleep 1 && rm -f $LOCK ) &\n+\t\t} &&\n+\t\tgit -c reftable.lockTimeout=5000 update-ref refs/heads/branch HEAD\n+\t)\n+'\n+\n test_expect_success 'pack-refs: compacts tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502955","messageId":"cd65e6d57b06fb7f74baf3882da9353fdb8d86bc.1726578382.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1726578382.git.ps@pks.im","subject":"[PATCH 2/3] reftable/stack: allow locking of outdated stacks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T13:43:22Z","receivedAt":"2024-09-17T13:43:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `reftable_stack_new_addition()` we first lock the stack and then\ncheck whether it is still up-to-date. If it is not we return an error to\nthe caller indicating that the stack is outdated.\n\nThis is overly restrictive in our ref transaction interface though: we\nlock the stack right before we start to verify the transaction, so we do\nnot really care whether it is outdated or not. What we really want is\nthat the stack is up-to-date after it has been locked so that we can\nverify queued updates against its current state while we know that it is\nlocked for concurrent modification.\n\nIntroduce a new flag `REFTABLE_STACK_NEW_ADDITION_RELOAD` that alters\nthe behaviour of `reftable_stack_init_addition()` in this case: when we\nnotice that it is out-of-date we reload it instead of returning an error\nto the caller.\n\nThis logic will be wired up in the reftable backend in the next commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 4 +-\n reftable/reftable-stack.h | 13 ++++++-\n reftable/stack.c | 20 ++++++----\n t/unit-tests/t-reftable-stack.c | 67 ++++++++++++++++++++++++++++++++-\n 4 files changed, 91 insertions(+), 13 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex e90ddfb98dd..c500fb820a7 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -766,7 +766,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n \t\tif (ret)\n \t\t\treturn ret;\n \n-\t\tret = reftable_stack_new_addition(&addition, stack);\n+\t\tret = reftable_stack_new_addition(&addition, stack, 0);\n \t\tif (ret) {\n \t\t\tif (ret == REFTABLE_LOCK_ERROR)\n \t\t\t\tstrbuf_addstr(err, \"cannot lock references\");\n@@ -2203,7 +2203,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,\n \tif (ret < 0)\n \t\tgoto done;\n \n-\tret = reftable_stack_new_addition(&add, stack);\n+\tret = reftable_stack_new_addition(&add, stack, 0);\n \tif (ret < 0)\n \t\tgoto done;\n \ndiff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\nindex f4f8cabc7fb..67316b215ed 100644\n--- a/reftable/reftable-stack.h\n+++ b/reftable/reftable-stack.h\n@@ -37,12 +37,21 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st);\n /* holds a transaction to add tables at the top of a stack. */\n struct reftable_addition;\n \n+enum {\n+\t/*\n+\t * Reload the stack when the stack is out-of-date after locking it.\n+\t */\n+\tREFTABLE_STACK_NEW_ADDITION_RELOAD = (1 << 0),\n+};\n+\n /*\n * returns a new transaction to add reftables to the given stack. As a side\n- * effect, the ref database is locked.\n+ * effect, the ref database is locked. Accepts REFTABLE_STACK_NEW_ADDITION_*\n+ * flags.\n */\n int reftable_stack_new_addition(struct reftable_addition **dest,\n-\t\t\t\tstruct reftable_stack *st);\n+\t\t\t\tstruct reftable_stack *st,\n+\t\t\t\tint flags);\n \n /* Adds a reftable to transaction. */\n int reftable_addition_add(struct reftable_addition *add,\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 5ccad2cff31..f9c95d5fa62 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -596,7 +596,8 @@ struct reftable_addition {\n #define REFTABLE_ADDITION_INIT {0}\n \n static int reftable_stack_init_addition(struct reftable_addition *add,\n-\t\t\t\t\tstruct reftable_stack *st)\n+\t\t\t\t\tstruct reftable_stack *st,\n+\t\t\t\t\tint flags)\n {\n \tstruct strbuf lock_file_name = STRBUF_INIT;\n \tint err;\n@@ -626,6 +627,11 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \terr = stack_uptodate(st);\n \tif (err < 0)\n \t\tgoto done;\n+\tif (err > 0 && flags & REFTABLE_STACK_NEW_ADDITION_RELOAD) {\n+\t\terr = reftable_stack_reload_maybe_reuse(add->stack, 1);\n+\t\tif (err)\n+\t\t\tgoto done;\n+\t}\n \tif (err > 0) {\n \t\terr = REFTABLE_OUTDATED_ERROR;\n \t\tgoto done;\n@@ -633,9 +639,8 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \n \tadd->next_update_index = reftable_stack_next_update_index(st);\n done:\n-\tif (err) {\n+\tif (err)\n \t\treftable_addition_close(add);\n-\t}\n \tstrbuf_release(&lock_file_name);\n \treturn err;\n }\n@@ -739,13 +744,14 @@ int reftable_addition_commit(struct reftable_addition *add)\n }\n \n int reftable_stack_new_addition(struct reftable_addition **dest,\n-\t\t\t\tstruct reftable_stack *st)\n+\t\t\t\tstruct reftable_stack *st,\n+\t\t\t\tint flags)\n {\n \tint err = 0;\n \tstruct reftable_addition empty = REFTABLE_ADDITION_INIT;\n \tREFTABLE_CALLOC_ARRAY(*dest, 1);\n \t**dest = empty;\n-\terr = reftable_stack_init_addition(*dest, st);\n+\terr = reftable_stack_init_addition(*dest, st, flags);\n \tif (err) {\n \t\treftable_free(*dest);\n \t\t*dest = NULL;\n@@ -759,7 +765,7 @@ static int stack_try_add(struct reftable_stack *st,\n \t\t\t void *arg)\n {\n \tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n-\tint err = reftable_stack_init_addition(&add, st);\n+\tint err = reftable_stack_init_addition(&add, st, 0);\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -1608,7 +1614,7 @@ static int reftable_stack_clean_locked(struct reftable_stack *st)\n int reftable_stack_clean(struct reftable_stack *st)\n {\n \tstruct reftable_addition *add = NULL;\n-\tint err = reftable_stack_new_addition(&add, st);\n+\tint err = reftable_stack_new_addition(&add, st, 0);\n \tif (err < 0) {\n \t\tgoto done;\n \t}\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex d62a9c1bed5..a37cc698d87 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -271,7 +271,7 @@ static void t_reftable_stack_transaction_api(void)\n \n \treftable_addition_destroy(add);\n \n-\terr = reftable_stack_new_addition(&add, st);\n+\terr = reftable_stack_new_addition(&add, st, 0);\n \tcheck(!err);\n \n \terr = reftable_addition_add(add, write_test_ref, &ref);\n@@ -292,6 +292,68 @@ static void t_reftable_stack_transaction_api(void)\n \tclear_dir(dir);\n }\n \n+static void t_reftable_stack_transaction_with_reload(void)\n+{\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tstruct reftable_stack *st1 = NULL, *st2 = NULL;\n+\tint err;\n+\tstruct reftable_addition *add = NULL;\n+\tstruct reftable_ref_record refs[2] = {\n+\t\t{\n+\t\t\t.refname = (char *) \"refs/heads/a\",\n+\t\t\t.update_index = 1,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { '1' },\n+\t\t},\n+\t\t{\n+\t\t\t.refname = (char *) \"refs/heads/b\",\n+\t\t\t.update_index = 2,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { '1' },\n+\t\t},\n+\t};\n+\tstruct reftable_ref_record ref = { 0 };\n+\n+\terr = reftable_new_stack(&st1, dir, NULL);\n+\tcheck(!err);\n+\terr = reftable_new_stack(&st2, dir, NULL);\n+\tcheck(!err);\n+\n+\terr = reftable_stack_new_addition(&add, st1, 0);\n+\tcheck(!err);\n+\terr = reftable_addition_add(add, write_test_ref, &refs[0]);\n+\tcheck(!err);\n+\terr = reftable_addition_commit(add);\n+\tcheck(!err);\n+\treftable_addition_destroy(add);\n+\n+\t/*\n+\t * The second stack is now outdated, which we should notice. We do not\n+\t * create the addition and lock the stack by default, but allow the\n+\t * reload to happen when REFTABLE_STACK_NEW_ADDITION_RELOAD is set.\n+\t */\n+\terr = reftable_stack_new_addition(&add, st2, 0);\n+\tcheck_int(err, ==, REFTABLE_OUTDATED_ERROR);\n+\terr = reftable_stack_new_addition(&add, st2, REFTABLE_STACK_NEW_ADDITION_RELOAD);\n+\tcheck(!err);\n+\terr = reftable_addition_add(add, write_test_ref, &refs[1]);\n+\tcheck(!err);\n+\terr = reftable_addition_commit(add);\n+\tcheck(!err);\n+\treftable_addition_destroy(add);\n+\n+\tfor (size_t i = 0; i < ARRAY_SIZE(refs); i++) {\n+\t\terr = reftable_stack_read_ref(st2, refs[i].refname, &ref);\n+\t\tcheck(!err);\n+\t\tcheck(reftable_ref_record_equal(&refs[i], &ref, GIT_SHA1_RAWSZ));\n+\t}\n+\n+\treftable_ref_record_release(&ref);\n+\treftable_stack_destroy(st1);\n+\treftable_stack_destroy(st2);\n+\tclear_dir(dir);\n+}\n+\n static void t_reftable_stack_transaction_api_performs_auto_compaction(void)\n {\n \tchar *dir = get_tmp_dir(__LINE__);\n@@ -322,7 +384,7 @@ static void t_reftable_stack_transaction_api_performs_auto_compaction(void)\n \t\t */\n \t\tst->opts.disable_auto_compact = i != n;\n \n-\t\terr = reftable_stack_new_addition(&add, st);\n+\t\terr = reftable_stack_new_addition(&add, st, 0);\n \t\tcheck(!err);\n \n \t\terr = reftable_addition_add(add, write_test_ref, &ref);\n@@ -1314,6 +1376,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \tTEST(t_reftable_stack_reload_with_missing_table(), \"stack iteration with garbage tables\");\n \tTEST(t_reftable_stack_tombstone(), \"'tombstone' refs in stack\");\n \tTEST(t_reftable_stack_transaction_api(), \"update transaction to stack\");\n+\tTEST(t_reftable_stack_transaction_with_reload(), \"transaction with reload\");\n \tTEST(t_reftable_stack_transaction_api_performs_auto_compaction(), \"update transaction triggers auto-compaction\");\n \tTEST(t_reftable_stack_update_index_check(), \"update transactions with equal update indices\");\n \tTEST(t_reftable_stack_uptodate(), \"stack must be reloaded before ref update\");\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502954","messageId":"37ec3acbff00564ecf3b63d1635aad5da0c54b19.1726578382.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1726578382.git.ps@pks.im","subject":"[PATCH 3/3] refs/reftable: reload locked stack when preparing transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-17T13:43:25Z","receivedAt":"2024-09-17T13:43:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When starting a reftable transaction we lock all stacks we are about to\nmodify. While it may happen that the stack is out-of-date at this point\nin time we don't really care: transactional updates encode the expected\nstate of a certain reference, so all that we really want to verify is\nthat the _current_ value matches that expected state.\n\nPass `REFTABLE_STACK_NEW_ADDITION_RELOAD` when locking the stack such\nthat an out-of-date stack will be reloaded after having been locked.\nThis change is safe because all verifications of the expected state\nhappen after this step anyway.\n\nAdd a testcase that verifies that many writers are now able to write to\nthe stack concurrently without failures and with a deterministic end\nresult.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 3 ++-\n t/t0610-reftable-basics.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex c500fb820a7..d4b383ca179 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -766,7 +766,8 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n \t\tif (ret)\n \t\t\treturn ret;\n \n-\t\tret = reftable_stack_new_addition(&addition, stack, 0);\n+\t\tret = reftable_stack_new_addition(&addition, stack,\n+\t\t\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \t\tif (ret) {\n \t\t\tif (ret == REFTABLE_LOCK_ERROR)\n \t\t\t\tstrbuf_addstr(err, \"cannot lock references\");\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 62da3e37823..2d951c8ceb6 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -450,6 +450,37 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n \t)\n '\n \n+test_expect_success 'ref transaction: many concurrent writers' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\t# Set a high timeout such that a busy CI machine will not abort\n+\t\t# early. 10 seconds should hopefully be ample of time to make\n+\t\t# this non-flaky.\n+\t\tgit config set reftable.lockTimeout 10000 &&\n+\t\ttest_commit --no-tag initial &&\n+\n+\t\thead=$(git rev-parse HEAD) &&\n+\t\tfor i in $(test_seq 100)\n+\t\tdo\n+\t\t\tprintf \"%s commit\\trefs/heads/branch-%s\\n\" \"$head\" \"$i\" ||\n+\t\t\treturn 1\n+\t\tdone >expect &&\n+\t\tprintf \"%s commit\\trefs/heads/main\\n\" \"$head\" >>expect &&\n+\n+\t\tfor i in $(test_seq 100)\n+\t\tdo\n+\t\t\t{ git update-ref refs/heads/branch-$i HEAD& } ||\n+\t\t\treturn 1\n+\t\tdone &&\n+\n+\t\twait &&\n+\t\tgit for-each-ref --sort=v:refname >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'pack-refs: compacts tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502962","messageId":"CAOLa=ZTja-nFmKZ8iyyp0szuaAWAnPncy0E6rM5=WWgnr=01uA@mail.gmail.com","threadId":"62129","inReplyTo":"ca3eab99f7ef86d1b7a5b4d4bdb8d2b0a55566e1.1726578382.git.ps@pks.im","subject":"Re: [PATCH 1/3] refs/reftable: introduce \"reftable.lockTimeout\"","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-09-17T17:46:39Z","receivedAt":"2024-09-17T17:46:42Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n[snip]\n\n> +reftable.lockTimeout::\n> +\tWhenever the reftable backend appends a new table to the stack, it has\n> +\tto lock the central \"tables.list\" file before updating it. This config\n> +\tcontrols how long the process will wait to acquire the lock in case\n> +\tanother process has already acquired it. Default is 1000 (i.e., retry\n> +\tfor 1 second).\n\nIsn't the default 100ms? As that was what was mentioned in the commit\nmessage\n\n[snip]\n\n> +test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit initial &&\n> +\t\tLOCK=.git/reftable/tables.list.lock &&\n> +\t\t>$LOCK &&\n> +\t\t{\n> +\t\t\t( sleep 1 && rm -f $LOCK ) &\n> +\t\t} &&\n> +\t\tgit -c reftable.lockTimeout=5000 update-ref refs/heads/branch HEAD\n> +\t)\n> +'\n\nNit: This does stall the test for 1s. Which is slightly annoying when\nrunning single tests locally. Couldn't we achieve this by doing `sleep\n0.1`?\n"},{"id":"502963","messageId":"CAPig+cQ_+C65V6Rj9uCBKJy-rdymRd93-0S_w-bfSSJj82Zsrw@mail.gmail.com","threadId":"62129","inReplyTo":"CAOLa=ZTja-nFmKZ8iyyp0szuaAWAnPncy0E6rM5=WWgnr=01uA@mail.gmail.com","subject":"Re: [PATCH 1/3] refs/reftable: introduce \"reftable.lockTimeout\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-09-17T17:50:27Z","receivedAt":"2024-09-17T17:50:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 17, 2024 at 1:46 PM karthik nayak wrote:\n> Patrick Steinhardt writes:\n> > +test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n> > + test_when_finished \"rm -rf repo\" &&\n> > + git init repo &&\n> > + (\n> > + cd repo &&\n> > + test_commit initial &&\n> > + LOCK=.git/reftable/tables.list.lock &&\n> > + >$LOCK &&\n> > + {\n> > + ( sleep 1 && rm -f $LOCK ) &\n> > + } &&\n> > + git -c reftable.lockTimeout=5000 update-ref refs/heads/branch HEAD\n> > + )\n> > +'\n>\n> Nit: This does stall the test for 1s. Which is slightly annoying when\n> running single tests locally. Couldn't we achieve this by doing `sleep\n> 0.1`?\n\n`sleep 0.1` is neither POSIX nor portable.\n"},{"id":"502965","messageId":"CAOLa=ZRwLYLba4_mze5cDRZqyG+5+-F+sQmuzf4Lt1LKVo5_CQ@mail.gmail.com","threadId":"62129","inReplyTo":"cover.1726578382.git.ps@pks.im","subject":"Re: [PATCH 0/3] reftable: graceful concurrent writes","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-09-17T18:26:58Z","receivedAt":"2024-09-17T18:27:03Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> Hi,\n>\n> the reftable backend cannot properly handle concurrent writes due to two\n> reasons:\n>\n> - It will bail out immediately when it sees a locked \"tables.list\"\n> file. This is being addressed by introducing a configurable locking\n> timeout, similar to how we have it for both loose and packed refs.\n>\n> - It will bail out when it notices that its stack is out-of-date after\n> having locked the \"tables.list\" file. This is addressed by reloading\n> the stack as requested after locking it, which is fine because our\n> transactional API would verify queued ref updates against their\n> expected state after the lock was acquired anyway.\n>\n> So with this patch series we can now spawn concurrent writers and they\n> are expected to succeed, which is demonstrated by the test added by the\n> last patch.\n>\n\nI only had a comment in the first commit. The rest two look good to me.\nI do wonder if we really need a flag? But that's just a nit.\n\nThanks\n"},{"id":"502975","messageId":"ZupXlaLP3QcP8SPb@pks.im","threadId":"62129","inReplyTo":"CAPig+cQ_+C65V6Rj9uCBKJy-rdymRd93-0S_w-bfSSJj82Zsrw@mail.gmail.com","subject":"Re: [PATCH 1/3] refs/reftable: introduce \"reftable.lockTimeout\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-18T04:31:17Z","receivedAt":"2024-09-18T04:31:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 17, 2024 at 01:50:27PM -0400, Eric Sunshine wrote:\n> On Tue, Sep 17, 2024 at 1:46 PM karthik nayak wrote:\n> > Patrick Steinhardt writes:\n> > > +test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n> > > + test_when_finished \"rm -rf repo\" &&\n> > > + git init repo &&\n> > > + (\n> > > + cd repo &&\n> > > + test_commit initial &&\n> > > + LOCK=.git/reftable/tables.list.lock &&\n> > > + >$LOCK &&\n> > > + {\n> > > + ( sleep 1 && rm -f $LOCK ) &\n> > > + } &&\n> > > + git -c reftable.lockTimeout=5000 update-ref refs/heads/branch HEAD\n> > > + )\n> > > +'\n> >\n> > Nit: This does stall the test for 1s. Which is slightly annoying when\n> > running single tests locally. Couldn't we achieve this by doing `sleep\n> > 0.1`?\n> \n> `sleep 0.1` is neither POSIX nor portable.\n\nThe above test also verifies that the timeout can in fact be overridden.\nIf we only had `sleep 0.1` we wouldn't be able to reliably verify that,\nas the default is a timeout of 100ms.\n\nWe also have essentially the same tests in t0601 for packed-refs, I\nmostly just copied the logic over from there. It's not exactly pretty,\nand I'm not a huge fan of introducing timing sensitive tests, but in the\nend I guess it's okayish.\n\nPatrick\n"},{"id":"502976","messageId":"ZupXmXtFzw6GuGGT@pks.im","threadId":"62129","inReplyTo":"CAOLa=ZTja-nFmKZ8iyyp0szuaAWAnPncy0E6rM5=WWgnr=01uA@mail.gmail.com","subject":"Re: [PATCH 1/3] refs/reftable: introduce \"reftable.lockTimeout\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-18T04:31:21Z","receivedAt":"2024-09-18T04:31:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 17, 2024 at 01:46:39PM -0400, karthik nayak wrote:\n> Patrick Steinhardt writes:\n> \n> [snip]\n> \n> > +reftable.lockTimeout::\n> > +\tWhenever the reftable backend appends a new table to the stack, it has\n> > +\tto lock the central \"tables.list\" file before updating it. This config\n> > +\tcontrols how long the process will wait to acquire the lock in case\n> > +\tanother process has already acquired it. Default is 1000 (i.e., retry\n> > +\tfor 1 second).\n> \n> Isn't the default 100ms? As that was what was mentioned in the commit\n> message\n\nOh, yeah. I ended up changing it. 1 second is the default timeout used\nby packed-refs, 100ms is the default timeout used by loose refs. The\nreason why I ultimately picked 100ms over 1s is that our usecase is\ncloser to loose refs, because it is our \"normal\" code path that we hit\nwhenever we write reftables.\n\nWill fix.\n\nPatrick\n"},{"id":"502977","messageId":"ZupXncLHidkSW3j8@pks.im","threadId":"62129","inReplyTo":"CAOLa=ZRwLYLba4_mze5cDRZqyG+5+-F+sQmuzf4Lt1LKVo5_CQ@mail.gmail.com","subject":"Re: [PATCH 0/3] reftable: graceful concurrent writes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-18T04:31:25Z","receivedAt":"2024-09-18T04:31:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 17, 2024 at 11:26:58AM -0700, karthik nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > Hi,\n> >\n> > the reftable backend cannot properly handle concurrent writes due to two\n> > reasons:\n> >\n> > - It will bail out immediately when it sees a locked \"tables.list\"\n> > file. This is being addressed by introducing a configurable locking\n> > timeout, similar to how we have it for both loose and packed refs.\n> >\n> > - It will bail out when it notices that its stack is out-of-date after\n> > having locked the \"tables.list\" file. This is addressed by reloading\n> > the stack as requested after locking it, which is fine because our\n> > transactional API would verify queued ref updates against their\n> > expected state after the lock was acquired anyway.\n> >\n> > So with this patch series we can now spawn concurrent writers and they\n> > are expected to succeed, which is demonstrated by the test added by the\n> > last patch.\n> >\n> \n> I only had a comment in the first commit. The rest two look good to me.\n> I do wonder if we really need a flag? But that's just a nit.\n\nI didn't carefully vet all locations where we could pass that flag, so\nright now we only pass it in a single location. Also, we need to keep in\nmind that this is library code: even if we had converted all callsites\nto pass the new flag I still it's a sensibe thing to wish for to disable\nthe automatic refresh and abort instead.\n\nThanks for your review!\n\nPatrick\n"},{"id":"502978","messageId":"cover.1726633812.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1726578382.git.ps@pks.im","subject":"[PATCH v2 0/3] reftable: graceful concurrent writes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-18T04:32:21Z","receivedAt":"2024-09-18T04:32:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second patch series that makes graceful concurrent writes\nwork with the reftable backend.\n\nThere is only a single change compared to v1, namely a fix to the docs\nto mention the right default timeout value.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (3):\n refs/reftable: introduce \"reftable.lockTimeout\"\n reftable/stack: allow locking of outdated stacks\n refs/reftable: reload locked stack when preparing transaction\n\n Documentation/config/reftable.txt | 7 ++++\n refs/reftable-backend.c | 9 ++++-\n reftable/reftable-stack.h | 13 +++++-\n reftable/reftable-writer.h | 8 ++++\n reftable/stack.c | 38 ++++++++++++------\n t/t0610-reftable-basics.sh | 58 ++++++++++++++++++++++++++\n t/unit-tests/t-reftable-stack.c | 67 ++++++++++++++++++++++++++++++-\n 7 files changed, 181 insertions(+), 19 deletions(-)\n\nRange-diff against v1:\n1: ca3eab99f7e ! 1: 700a35df125 refs/reftable: introduce \"reftable.lockTimeout\"\n @@ Documentation/config/reftable.txt: reftable.geometricFactor::\n +\tWhenever the reftable backend appends a new table to the stack, it has\n +\tto lock the central \"tables.list\" file before updating it. This config\n +\tcontrols how long the process will wait to acquire the lock in case\n -+\tanother process has already acquired it. Default is 1000 (i.e., retry\n -+\tfor 1 second).\n ++\tanother process has already acquired it. Default is 100 (i.e., retry\n ++\tfor 100ms).\n \n ## refs/reftable-backend.c ##\n @@ refs/reftable-backend.c: static int reftable_be_config(const char *var, const char *value,\n2: cd65e6d57b0 = 2: f4be0966e17 reftable/stack: allow locking of outdated stacks\n3: 37ec3acbff0 = 3: 111b497ef17 refs/reftable: reload locked stack when preparing transaction\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502979","messageId":"700a35df125eca6639d08d30720332dbc1ed15a1.1726633812.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1726633812.git.ps@pks.im","subject":"[PATCH v2 1/3] refs/reftable: introduce \"reftable.lockTimeout\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-18T04:32:24Z","receivedAt":"2024-09-18T04:32:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When multiple concurrent processes try to update references in a\nrepository they may try to lock the same lockfiles. This can happen even\nwhen the updates are non-conflicting and can both be applied, so it\ndoesn't always make sense to abort the transaction immediately. Both the\n\"loose\" and \"packed\" backends thus have a grace period that they wait\nfor the lock to be released that can be controlled via the config values\n\"core.filesRefLockTimeout\" and \"core.packedRefsTimeout\", respectively.\n\nThe reftable backend doesn't have such a setting yet and instead fails\nimmediately when it sees such a lock. But the exact same concepts apply\nhere as they do apply to the other backends.\n\nIntroduce a new \"reftable.lockTimeout\" config that controls how long we\nmay wait for a \"tables.list\" lock to be released. The default value of\nthis config is 100ms, which is the same default as we have it for the\n\"loose\" backend.\n\nNote that even though we also lock individual tables, this config really\nonly applies to the \"tables.list\" file. This is because individual\ntables are only ever locked when we already hold the \"tables.list\" lock\nduring compaction. When we observe such a lock we in fact do not want to\ncompact the table at all because it is already in the process of being\ncompacted by a concurrent process. So applying the same timeout here\nwould not make any sense and only delay progress.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/config/reftable.txt | 7 +++++++\n refs/reftable-backend.c | 4 ++++\n reftable/reftable-writer.h | 8 ++++++++\n reftable/stack.c | 18 ++++++++++++------\n t/t0610-reftable-basics.sh | 27 +++++++++++++++++++++++++++\n 5 files changed, 58 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config/reftable.txt b/Documentation/config/reftable.txt\nindex 05157279778..abb793bcbb0 100644\n--- a/Documentation/config/reftable.txt\n+++ b/Documentation/config/reftable.txt\n@@ -46,3 +46,10 @@ reftable.geometricFactor::\n By default, the geometric sequence uses a factor of 2, meaning that for any\n table, the next-biggest table must at least be twice as big. A maximum factor\n of 256 is supported.\n+\n+reftable.lockTimeout::\n+\tWhenever the reftable backend appends a new table to the stack, it has\n+\tto lock the central \"tables.list\" file before updating it. This config\n+\tcontrols how long the process will wait to acquire the lock in case\n+\tanother process has already acquired it. Default is 100 (i.e., retry\n+\tfor 100ms).\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 1c4b19e737f..e90ddfb98dd 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -256,6 +256,9 @@ static int reftable_be_config(const char *var, const char *value,\n \t\tif (factor > UINT8_MAX)\n \t\t\tdie(\"reftable geometric factor cannot exceed %u\", (unsigned)UINT8_MAX);\n \t\topts->auto_compaction_factor = factor;\n+\t} else if (!strcmp(var, \"reftable.locktimeout\")) {\n+\t\tunsigned long lock_timeout = git_config_ulong(var, value, ctx->kvi);\n+\t\topts->lock_timeout_ms = lock_timeout;\n \t}\n \n \treturn 0;\n@@ -281,6 +284,7 @@ static struct ref_store *reftable_be_init(struct repository *repo,\n \trefs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);\n \trefs->write_options.disable_auto_compact =\n \t\t!git_env_bool(\"GIT_TEST_REFTABLE_AUTOCOMPACTION\", 1);\n+\trefs->write_options.lock_timeout_ms = 100;\n \n \tgit_config(reftable_be_config, &refs->write_options);\n \ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex 189b1f4144f..d5f2446220b 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -51,6 +51,14 @@ struct reftable_write_options {\n \t * tables to compact. Defaults to 2 if unset.\n \t */\n \tuint8_t auto_compaction_factor;\n+\n+\t/*\n+\t * The number of milliseconds to wait when trying to lock \"tables.list\".\n+\t * Note that this does not apply to locking individual tables, as these\n+\t * should only ever be locked when already holding the \"tables.list\"\n+\t * lock.\n+\t */\n+\tunsigned lock_timeout_ms;\n };\n \n /* reftable_block_stats holds statistics for a single block type */\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ce0a35216ba..5ccad2cff31 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -603,8 +603,10 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \n \tadd->stack = st;\n \n-\terr = hold_lock_file_for_update(&add->tables_list_lock, st->list_file,\n-\t\t\t\t\tLOCK_NO_DEREF);\n+\terr = hold_lock_file_for_update_timeout(&add->tables_list_lock,\n+\t\t\t\t\t\tst->list_file,\n+\t\t\t\t\t\tLOCK_NO_DEREF,\n+\t\t\t\t\t\tst->opts.lock_timeout_ms);\n \tif (err < 0) {\n \t\tif (errno == EEXIST) {\n \t\t\terr = REFTABLE_LOCK_ERROR;\n@@ -1056,8 +1058,10 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * Hold the lock so that we can read \"tables.list\" and lock all tables\n \t * which are part of the user-specified range.\n \t */\n-\terr = hold_lock_file_for_update(&tables_list_lock, st->list_file,\n-\t\t\t\t\tLOCK_NO_DEREF);\n+\terr = hold_lock_file_for_update_timeout(&tables_list_lock,\n+\t\t\t\t\t\tst->list_file,\n+\t\t\t\t\t\tLOCK_NO_DEREF,\n+\t\t\t\t\t\tst->opts.lock_timeout_ms);\n \tif (err < 0) {\n \t\tif (errno == EEXIST)\n \t\t\terr = REFTABLE_LOCK_ERROR;\n@@ -1156,8 +1160,10 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * \"tables.list\". We'll then replace the compacted range of tables with\n \t * the new table.\n \t */\n-\terr = hold_lock_file_for_update(&tables_list_lock, st->list_file,\n-\t\t\t\t\tLOCK_NO_DEREF);\n+\terr = hold_lock_file_for_update_timeout(&tables_list_lock,\n+\t\t\t\t\t\tst->list_file,\n+\t\t\t\t\t\tLOCK_NO_DEREF,\n+\t\t\t\t\t\tst->opts.lock_timeout_ms);\n \tif (err < 0) {\n \t\tif (errno == EEXIST)\n \t\t\terr = REFTABLE_LOCK_ERROR;\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 37510c2b2a5..62da3e37823 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -423,6 +423,33 @@ test_expect_success 'ref transaction: fails gracefully when auto compaction fail\n \t)\n '\n \n+test_expect_success 'ref transaction: timeout acquiring tables.list lock' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit initial &&\n+\t\t>.git/reftable/tables.list.lock &&\n+\t\ttest_must_fail git update-ref refs/heads/branch HEAD 2>err &&\n+\t\ttest_grep \"cannot lock references\" err\n+\t)\n+'\n+\n+test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit initial &&\n+\t\tLOCK=.git/reftable/tables.list.lock &&\n+\t\t>$LOCK &&\n+\t\t{\n+\t\t\t( sleep 1 && rm -f $LOCK ) &\n+\t\t} &&\n+\t\tgit -c reftable.lockTimeout=5000 update-ref refs/heads/branch HEAD\n+\t)\n+'\n+\n test_expect_success 'pack-refs: compacts tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502980","messageId":"f4be0966e17600602b1057a6ae219711994df128.1726633812.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1726633812.git.ps@pks.im","subject":"[PATCH v2 2/3] reftable/stack: allow locking of outdated stacks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-18T04:32:27Z","receivedAt":"2024-09-18T04:32:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `reftable_stack_new_addition()` we first lock the stack and then\ncheck whether it is still up-to-date. If it is not we return an error to\nthe caller indicating that the stack is outdated.\n\nThis is overly restrictive in our ref transaction interface though: we\nlock the stack right before we start to verify the transaction, so we do\nnot really care whether it is outdated or not. What we really want is\nthat the stack is up-to-date after it has been locked so that we can\nverify queued updates against its current state while we know that it is\nlocked for concurrent modification.\n\nIntroduce a new flag `REFTABLE_STACK_NEW_ADDITION_RELOAD` that alters\nthe behaviour of `reftable_stack_init_addition()` in this case: when we\nnotice that it is out-of-date we reload it instead of returning an error\nto the caller.\n\nThis logic will be wired up in the reftable backend in the next commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 4 +-\n reftable/reftable-stack.h | 13 ++++++-\n reftable/stack.c | 20 ++++++----\n t/unit-tests/t-reftable-stack.c | 67 ++++++++++++++++++++++++++++++++-\n 4 files changed, 91 insertions(+), 13 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex e90ddfb98dd..c500fb820a7 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -766,7 +766,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n \t\tif (ret)\n \t\t\treturn ret;\n \n-\t\tret = reftable_stack_new_addition(&addition, stack);\n+\t\tret = reftable_stack_new_addition(&addition, stack, 0);\n \t\tif (ret) {\n \t\t\tif (ret == REFTABLE_LOCK_ERROR)\n \t\t\t\tstrbuf_addstr(err, \"cannot lock references\");\n@@ -2203,7 +2203,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,\n \tif (ret < 0)\n \t\tgoto done;\n \n-\tret = reftable_stack_new_addition(&add, stack);\n+\tret = reftable_stack_new_addition(&add, stack, 0);\n \tif (ret < 0)\n \t\tgoto done;\n \ndiff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\nindex f4f8cabc7fb..67316b215ed 100644\n--- a/reftable/reftable-stack.h\n+++ b/reftable/reftable-stack.h\n@@ -37,12 +37,21 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st);\n /* holds a transaction to add tables at the top of a stack. */\n struct reftable_addition;\n \n+enum {\n+\t/*\n+\t * Reload the stack when the stack is out-of-date after locking it.\n+\t */\n+\tREFTABLE_STACK_NEW_ADDITION_RELOAD = (1 << 0),\n+};\n+\n /*\n * returns a new transaction to add reftables to the given stack. As a side\n- * effect, the ref database is locked.\n+ * effect, the ref database is locked. Accepts REFTABLE_STACK_NEW_ADDITION_*\n+ * flags.\n */\n int reftable_stack_new_addition(struct reftable_addition **dest,\n-\t\t\t\tstruct reftable_stack *st);\n+\t\t\t\tstruct reftable_stack *st,\n+\t\t\t\tint flags);\n \n /* Adds a reftable to transaction. */\n int reftable_addition_add(struct reftable_addition *add,\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 5ccad2cff31..f9c95d5fa62 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -596,7 +596,8 @@ struct reftable_addition {\n #define REFTABLE_ADDITION_INIT {0}\n \n static int reftable_stack_init_addition(struct reftable_addition *add,\n-\t\t\t\t\tstruct reftable_stack *st)\n+\t\t\t\t\tstruct reftable_stack *st,\n+\t\t\t\t\tint flags)\n {\n \tstruct strbuf lock_file_name = STRBUF_INIT;\n \tint err;\n@@ -626,6 +627,11 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \terr = stack_uptodate(st);\n \tif (err < 0)\n \t\tgoto done;\n+\tif (err > 0 && flags & REFTABLE_STACK_NEW_ADDITION_RELOAD) {\n+\t\terr = reftable_stack_reload_maybe_reuse(add->stack, 1);\n+\t\tif (err)\n+\t\t\tgoto done;\n+\t}\n \tif (err > 0) {\n \t\terr = REFTABLE_OUTDATED_ERROR;\n \t\tgoto done;\n@@ -633,9 +639,8 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \n \tadd->next_update_index = reftable_stack_next_update_index(st);\n done:\n-\tif (err) {\n+\tif (err)\n \t\treftable_addition_close(add);\n-\t}\n \tstrbuf_release(&lock_file_name);\n \treturn err;\n }\n@@ -739,13 +744,14 @@ int reftable_addition_commit(struct reftable_addition *add)\n }\n \n int reftable_stack_new_addition(struct reftable_addition **dest,\n-\t\t\t\tstruct reftable_stack *st)\n+\t\t\t\tstruct reftable_stack *st,\n+\t\t\t\tint flags)\n {\n \tint err = 0;\n \tstruct reftable_addition empty = REFTABLE_ADDITION_INIT;\n \tREFTABLE_CALLOC_ARRAY(*dest, 1);\n \t**dest = empty;\n-\terr = reftable_stack_init_addition(*dest, st);\n+\terr = reftable_stack_init_addition(*dest, st, flags);\n \tif (err) {\n \t\treftable_free(*dest);\n \t\t*dest = NULL;\n@@ -759,7 +765,7 @@ static int stack_try_add(struct reftable_stack *st,\n \t\t\t void *arg)\n {\n \tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n-\tint err = reftable_stack_init_addition(&add, st);\n+\tint err = reftable_stack_init_addition(&add, st, 0);\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -1608,7 +1614,7 @@ static int reftable_stack_clean_locked(struct reftable_stack *st)\n int reftable_stack_clean(struct reftable_stack *st)\n {\n \tstruct reftable_addition *add = NULL;\n-\tint err = reftable_stack_new_addition(&add, st);\n+\tint err = reftable_stack_new_addition(&add, st, 0);\n \tif (err < 0) {\n \t\tgoto done;\n \t}\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex d62a9c1bed5..a37cc698d87 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -271,7 +271,7 @@ static void t_reftable_stack_transaction_api(void)\n \n \treftable_addition_destroy(add);\n \n-\terr = reftable_stack_new_addition(&add, st);\n+\terr = reftable_stack_new_addition(&add, st, 0);\n \tcheck(!err);\n \n \terr = reftable_addition_add(add, write_test_ref, &ref);\n@@ -292,6 +292,68 @@ static void t_reftable_stack_transaction_api(void)\n \tclear_dir(dir);\n }\n \n+static void t_reftable_stack_transaction_with_reload(void)\n+{\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tstruct reftable_stack *st1 = NULL, *st2 = NULL;\n+\tint err;\n+\tstruct reftable_addition *add = NULL;\n+\tstruct reftable_ref_record refs[2] = {\n+\t\t{\n+\t\t\t.refname = (char *) \"refs/heads/a\",\n+\t\t\t.update_index = 1,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { '1' },\n+\t\t},\n+\t\t{\n+\t\t\t.refname = (char *) \"refs/heads/b\",\n+\t\t\t.update_index = 2,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { '1' },\n+\t\t},\n+\t};\n+\tstruct reftable_ref_record ref = { 0 };\n+\n+\terr = reftable_new_stack(&st1, dir, NULL);\n+\tcheck(!err);\n+\terr = reftable_new_stack(&st2, dir, NULL);\n+\tcheck(!err);\n+\n+\terr = reftable_stack_new_addition(&add, st1, 0);\n+\tcheck(!err);\n+\terr = reftable_addition_add(add, write_test_ref, &refs[0]);\n+\tcheck(!err);\n+\terr = reftable_addition_commit(add);\n+\tcheck(!err);\n+\treftable_addition_destroy(add);\n+\n+\t/*\n+\t * The second stack is now outdated, which we should notice. We do not\n+\t * create the addition and lock the stack by default, but allow the\n+\t * reload to happen when REFTABLE_STACK_NEW_ADDITION_RELOAD is set.\n+\t */\n+\terr = reftable_stack_new_addition(&add, st2, 0);\n+\tcheck_int(err, ==, REFTABLE_OUTDATED_ERROR);\n+\terr = reftable_stack_new_addition(&add, st2, REFTABLE_STACK_NEW_ADDITION_RELOAD);\n+\tcheck(!err);\n+\terr = reftable_addition_add(add, write_test_ref, &refs[1]);\n+\tcheck(!err);\n+\terr = reftable_addition_commit(add);\n+\tcheck(!err);\n+\treftable_addition_destroy(add);\n+\n+\tfor (size_t i = 0; i < ARRAY_SIZE(refs); i++) {\n+\t\terr = reftable_stack_read_ref(st2, refs[i].refname, &ref);\n+\t\tcheck(!err);\n+\t\tcheck(reftable_ref_record_equal(&refs[i], &ref, GIT_SHA1_RAWSZ));\n+\t}\n+\n+\treftable_ref_record_release(&ref);\n+\treftable_stack_destroy(st1);\n+\treftable_stack_destroy(st2);\n+\tclear_dir(dir);\n+}\n+\n static void t_reftable_stack_transaction_api_performs_auto_compaction(void)\n {\n \tchar *dir = get_tmp_dir(__LINE__);\n@@ -322,7 +384,7 @@ static void t_reftable_stack_transaction_api_performs_auto_compaction(void)\n \t\t */\n \t\tst->opts.disable_auto_compact = i != n;\n \n-\t\terr = reftable_stack_new_addition(&add, st);\n+\t\terr = reftable_stack_new_addition(&add, st, 0);\n \t\tcheck(!err);\n \n \t\terr = reftable_addition_add(add, write_test_ref, &ref);\n@@ -1314,6 +1376,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \tTEST(t_reftable_stack_reload_with_missing_table(), \"stack iteration with garbage tables\");\n \tTEST(t_reftable_stack_tombstone(), \"'tombstone' refs in stack\");\n \tTEST(t_reftable_stack_transaction_api(), \"update transaction to stack\");\n+\tTEST(t_reftable_stack_transaction_with_reload(), \"transaction with reload\");\n \tTEST(t_reftable_stack_transaction_api_performs_auto_compaction(), \"update transaction triggers auto-compaction\");\n \tTEST(t_reftable_stack_update_index_check(), \"update transactions with equal update indices\");\n \tTEST(t_reftable_stack_uptodate(), \"stack must be reloaded before ref update\");\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502981","messageId":"111b497ef17340c71f6c009a6c4228f45c5f1322.1726633812.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1726633812.git.ps@pks.im","subject":"[PATCH v2 3/3] refs/reftable: reload locked stack when preparing transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-18T04:32:30Z","receivedAt":"2024-09-18T04:32:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When starting a reftable transaction we lock all stacks we are about to\nmodify. While it may happen that the stack is out-of-date at this point\nin time we don't really care: transactional updates encode the expected\nstate of a certain reference, so all that we really want to verify is\nthat the _current_ value matches that expected state.\n\nPass `REFTABLE_STACK_NEW_ADDITION_RELOAD` when locking the stack such\nthat an out-of-date stack will be reloaded after having been locked.\nThis change is safe because all verifications of the expected state\nhappen after this step anyway.\n\nAdd a testcase that verifies that many writers are now able to write to\nthe stack concurrently without failures and with a deterministic end\nresult.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 3 ++-\n t/t0610-reftable-basics.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex c500fb820a7..d4b383ca179 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -766,7 +766,8 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n \t\tif (ret)\n \t\t\treturn ret;\n \n-\t\tret = reftable_stack_new_addition(&addition, stack, 0);\n+\t\tret = reftable_stack_new_addition(&addition, stack,\n+\t\t\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \t\tif (ret) {\n \t\t\tif (ret == REFTABLE_LOCK_ERROR)\n \t\t\t\tstrbuf_addstr(err, \"cannot lock references\");\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 62da3e37823..2d951c8ceb6 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -450,6 +450,37 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n \t)\n '\n \n+test_expect_success 'ref transaction: many concurrent writers' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\t# Set a high timeout such that a busy CI machine will not abort\n+\t\t# early. 10 seconds should hopefully be ample of time to make\n+\t\t# this non-flaky.\n+\t\tgit config set reftable.lockTimeout 10000 &&\n+\t\ttest_commit --no-tag initial &&\n+\n+\t\thead=$(git rev-parse HEAD) &&\n+\t\tfor i in $(test_seq 100)\n+\t\tdo\n+\t\t\tprintf \"%s commit\\trefs/heads/branch-%s\\n\" \"$head\" \"$i\" ||\n+\t\t\treturn 1\n+\t\tdone >expect &&\n+\t\tprintf \"%s commit\\trefs/heads/main\\n\" \"$head\" >>expect &&\n+\n+\t\tfor i in $(test_seq 100)\n+\t\tdo\n+\t\t\t{ git update-ref refs/heads/branch-$i HEAD& } ||\n+\t\t\treturn 1\n+\t\tdone &&\n+\n+\t\twait &&\n+\t\tgit for-each-ref --sort=v:refname >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'pack-refs: compacts tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502984","messageId":"D49ATJ4EIEG0.3A1RCYNFQG0XR@jamesliu.io","threadId":"62129","inReplyTo":"700a35df125eca6639d08d30720332dbc1ed15a1.1726633812.git.ps@pks.im","subject":"Re: [PATCH v2 1/3] refs/reftable: introduce \"reftable.lockTimeout\"","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-09-18T09:22:28Z","receivedAt":"2024-09-18T09:22:34Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"On Wed Sep 18, 2024 at 2:32 PM AEST, Patrick Steinhardt wrote:\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index 1c4b19e737f..e90ddfb98dd 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -256,6 +256,9 @@ static int reftable_be_config(const char *var, const char *value,\n> \t\tif (factor > UINT8_MAX)\n> \t\t\tdie(\"reftable geometric factor cannot exceed %u\", (unsigned)UINT8_MAX);\n> \t\topts->auto_compaction_factor = factor;\n> +\t} else if (!strcmp(var, \"reftable.locktimeout\")) {\n> +\t\tunsigned long lock_timeout = git_config_ulong(var, value, ctx->kvi);\n> +\t\topts->lock_timeout_ms = lock_timeout;\n> \t}\n\nDo we need to support the `0` and `-1` values that are possible for\nthe \"core.filesRefLockTimeout\" and \"core.packedRefsTimeout\" timeouts\nhere as well?\n\n"},{"id":"502985","messageId":"D49AWUZ2NCNC.11D23I38TRE0B@jamesliu.io","threadId":"62129","inReplyTo":"f4be0966e17600602b1057a6ae219711994df128.1726633812.git.ps@pks.im","subject":"Re: [PATCH v2 2/3] reftable/stack: allow locking of outdated stacks","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-09-18T09:26:49Z","receivedAt":"2024-09-18T09:26:55Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"I just want to check my understanding of this test, since I think it's\nthe first time I've reviewed anything using this test harness:\n\nOn Wed Sep 18, 2024 at 2:32 PM AEST, Patrick Steinhardt wrote:\n> diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\n> index d62a9c1bed5..a37cc698d87 100644\n> --- a/t/unit-tests/t-reftable-stack.c\n> +++ b/t/unit-tests/t-reftable-stack.c\n> @@ -271,7 +271,7 @@ static void t_reftable_stack_transaction_api(void)\n> \n> \treftable_addition_destroy(add);\n> \n> -\terr = reftable_stack_new_addition(&add, st);\n> +\terr = reftable_stack_new_addition(&add, st, 0);\n> \tcheck(!err);\n> \n> \terr = reftable_addition_add(add, write_test_ref, &ref);\n> @@ -292,6 +292,68 @@ static void t_reftable_stack_transaction_api(void)\n> \tclear_dir(dir);\n> }\n> \n> +static void t_reftable_stack_transaction_with_reload(void)\n> +{\n> +\tchar *dir = get_tmp_dir(__LINE__);\n> +\tstruct reftable_stack *st1 = NULL, *st2 = NULL;\n> +\tint err;\n> +\tstruct reftable_addition *add = NULL;\n> +\tstruct reftable_ref_record refs[2] = {\n> +\t\t{\n> +\t\t\t.refname = (char *) \"refs/heads/a\",\n> +\t\t\t.update_index = 1,\n> +\t\t\t.value_type = REFTABLE_REF_VAL1,\n> +\t\t\t.value.val1 = { '1' },\n> +\t\t},\n> +\t\t{\n> +\t\t\t.refname = (char *) \"refs/heads/b\",\n> +\t\t\t.update_index = 2,\n> +\t\t\t.value_type = REFTABLE_REF_VAL1,\n> +\t\t\t.value.val1 = { '1' },\n> +\t\t},\n> +\t};\n> +\tstruct reftable_ref_record ref = { 0 };\n> +\n\nCreate two reftable stacks that provide a view into the reftable tables\ninside \"dir\".\n\n> +\terr = reftable_new_stack(&st1, dir, NULL);\n> +\tcheck(!err);\n> +\terr = reftable_new_stack(&st2, dir, NULL);\n> +\tcheck(!err);\n> +\n\nSuccessfully add refs[0] to the first stack using the transactional API.\n\n> +\terr = reftable_stack_new_addition(&add, st1, 0);\n> +\tcheck(!err);\n> +\terr = reftable_addition_add(add, write_test_ref, &refs[0]);\n> +\tcheck(!err);\n> +\terr = reftable_addition_commit(add);\n> +\tcheck(!err);\n> +\treftable_addition_destroy(add);\n> +\n> +\t/*\n> +\t * The second stack is now outdated, which we should notice. We do not\n> +\t * create the addition and lock the stack by default, but allow the\n> +\t * reload to happen when REFTABLE_STACK_NEW_ADDITION_RELOAD is set.\n> +\t */\n\nWe try to open a transaction via the second reftable stack, but the\nthis stack is outdated because we've written to \"dir\" when the previous\nstack addition was committed.\n\n> +\terr = reftable_stack_new_addition(&add, st2, 0);\n> +\tcheck_int(err, ==, REFTABLE_OUTDATED_ERROR);\n\nTry again, but supply the flag so it performs a reload internally. Write\nrefs[1] to \"dir\" by committing the transaction. \n\n> +\terr = reftable_stack_new_addition(&add, st2, REFTABLE_STACK_NEW_ADDITION_RELOAD);\n> +\tcheck(!err);\n> +\terr = reftable_addition_add(add, write_test_ref, &refs[1]);\n> +\tcheck(!err);\n> +\terr = reftable_addition_commit(add);\n> +\tcheck(!err);\n> +\treftable_addition_destroy(add);\n> +\n\nAsserts.\n\n> +\tfor (size_t i = 0; i < ARRAY_SIZE(refs); i++) {\n> +\t\terr = reftable_stack_read_ref(st2, refs[i].refname, &ref);\n> +\t\tcheck(!err);\n> +\t\tcheck(reftable_ref_record_equal(&refs[i], &ref, GIT_SHA1_RAWSZ));\n> +\t}\n> +\n> +\treftable_ref_record_release(&ref);\n> +\treftable_stack_destroy(st1);\n> +\treftable_stack_destroy(st2);\n> +\tclear_dir(dir);\n> +}\n> +\n\n"},{"id":"502986","messageId":"D49B1PC1DJX4.3SMBLL1G9PRMD@jamesliu.io","threadId":"62129","inReplyTo":"cover.1726633812.git.ps@pks.im","subject":"Re: [PATCH v2 0/3] reftable: graceful concurrent writes","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-09-18T09:33:09Z","receivedAt":"2024-09-18T09:33:13Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"Thanks Patrick! I've just left two comments.\n\nCheers,\nJames\n"},{"id":"502987","messageId":"ZuqfufrL_de7Oaq1@pks.im","threadId":"62129","inReplyTo":"D49ATJ4EIEG0.3A1RCYNFQG0XR@jamesliu.io","subject":"Re: [PATCH v2 1/3] refs/reftable: introduce \"reftable.lockTimeout\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-18T09:39:05Z","receivedAt":"2024-09-18T09:39:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 18, 2024 at 07:22:28PM +1000, James Liu wrote:\n> On Wed Sep 18, 2024 at 2:32 PM AEST, Patrick Steinhardt wrote:\n> > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> > index 1c4b19e737f..e90ddfb98dd 100644\n> > --- a/refs/reftable-backend.c\n> > +++ b/refs/reftable-backend.c\n> > @@ -256,6 +256,9 @@ static int reftable_be_config(const char *var, const char *value,\n> > \t\tif (factor > UINT8_MAX)\n> > \t\t\tdie(\"reftable geometric factor cannot exceed %u\", (unsigned)UINT8_MAX);\n> > \t\topts->auto_compaction_factor = factor;\n> > +\t} else if (!strcmp(var, \"reftable.locktimeout\")) {\n> > +\t\tunsigned long lock_timeout = git_config_ulong(var, value, ctx->kvi);\n> > +\t\topts->lock_timeout_ms = lock_timeout;\n> > \t}\n> \n> Do we need to support the `0` and `-1` values that are possible for\n> the \"core.filesRefLockTimeout\" and \"core.packedRefsTimeout\" timeouts\n> here as well?\n\nWe already handle `0`, which is provided by the underlying lockfile\ninterface. But we don't handle `-1` yet. I guess wiring it up does make\nsense indeed, even if it is just to be consistent with the other\ntimeouts.\n\nPatrick\n"},{"id":"502988","messageId":"Zuqfvp_vaALkuERP@pks.im","threadId":"62129","inReplyTo":"D49AWUZ2NCNC.11D23I38TRE0B@jamesliu.io","subject":"Re: [PATCH v2 2/3] reftable/stack: allow locking of outdated stacks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-18T09:39:10Z","receivedAt":"2024-09-18T09:39:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 18, 2024 at 07:26:49PM +1000, James Liu wrote:\n> I just want to check my understanding of this test, since I think it's\n> the first time I've reviewed anything using this test harness:\n> \n> On Wed Sep 18, 2024 at 2:32 PM AEST, Patrick Steinhardt wrote:\n> > diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\n> > index d62a9c1bed5..a37cc698d87 100644\n> > --- a/t/unit-tests/t-reftable-stack.c\n> > +++ b/t/unit-tests/t-reftable-stack.c\n> > @@ -271,7 +271,7 @@ static void t_reftable_stack_transaction_api(void)\n> > \n> > \treftable_addition_destroy(add);\n> > \n> > -\terr = reftable_stack_new_addition(&add, st);\n> > +\terr = reftable_stack_new_addition(&add, st, 0);\n> > \tcheck(!err);\n> > \n> > \terr = reftable_addition_add(add, write_test_ref, &ref);\n> > @@ -292,6 +292,68 @@ static void t_reftable_stack_transaction_api(void)\n> > \tclear_dir(dir);\n> > }\n> > \n> > +static void t_reftable_stack_transaction_with_reload(void)\n> > +{\n> > +\tchar *dir = get_tmp_dir(__LINE__);\n> > +\tstruct reftable_stack *st1 = NULL, *st2 = NULL;\n> > +\tint err;\n> > +\tstruct reftable_addition *add = NULL;\n> > +\tstruct reftable_ref_record refs[2] = {\n> > +\t\t{\n> > +\t\t\t.refname = (char *) \"refs/heads/a\",\n> > +\t\t\t.update_index = 1,\n> > +\t\t\t.value_type = REFTABLE_REF_VAL1,\n> > +\t\t\t.value.val1 = { '1' },\n> > +\t\t},\n> > +\t\t{\n> > +\t\t\t.refname = (char *) \"refs/heads/b\",\n> > +\t\t\t.update_index = 2,\n> > +\t\t\t.value_type = REFTABLE_REF_VAL1,\n> > +\t\t\t.value.val1 = { '1' },\n> > +\t\t},\n> > +\t};\n> > +\tstruct reftable_ref_record ref = { 0 };\n> > +\n> \n> Create two reftable stacks that provide a view into the reftable tables\n> inside \"dir\".\n\nYup.\n\n> > +\terr = reftable_new_stack(&st1, dir, NULL);\n> > +\tcheck(!err);\n> > +\terr = reftable_new_stack(&st2, dir, NULL);\n> > +\tcheck(!err);\n> > +\n> \n> Successfully add refs[0] to the first stack using the transactional API.\n\nHere we only open the stacks without doing anything with them yet. This\nis preparation for being able to read/write them.\n\n> > +\terr = reftable_stack_new_addition(&add, st1, 0);\n> > +\tcheck(!err);\n> > +\terr = reftable_addition_add(add, write_test_ref, &refs[0]);\n> > +\tcheck(!err);\n> > +\terr = reftable_addition_commit(add);\n> > +\tcheck(!err);\n> > +\treftable_addition_destroy(add);\n> > +\n> > +\t/*\n> > +\t * The second stack is now outdated, which we should notice. We do not\n> > +\t * create the addition and lock the stack by default, but allow the\n> > +\t * reload to happen when REFTABLE_STACK_NEW_ADDITION_RELOAD is set.\n> > +\t */\n> \n> We try to open a transaction via the second reftable stack, but the\n> this stack is outdated because we've written to \"dir\" when the previous\n> stack addition was committed.\n\nYup.\n\n> > +\terr = reftable_stack_new_addition(&add, st2, 0);\n> > +\tcheck_int(err, ==, REFTABLE_OUTDATED_ERROR);\n> \n> Try again, but supply the flag so it performs a reload internally. Write\n> refs[1] to \"dir\" by committing the transaction. \n\nYup.\n\n> > +\terr = reftable_stack_new_addition(&add, st2, REFTABLE_STACK_NEW_ADDITION_RELOAD);\n> > +\tcheck(!err);\n> > +\terr = reftable_addition_add(add, write_test_ref, &refs[1]);\n> > +\tcheck(!err);\n> > +\terr = reftable_addition_commit(add);\n> > +\tcheck(!err);\n> > +\treftable_addition_destroy(add);\n> > +\n> \n> Asserts.\n\nExactly.\n\nPatrick\n"},{"id":"502989","messageId":"cover.1726653185.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1726578382.git.ps@pks.im","subject":"[PATCH v3 0/3] reftable: graceful concurrent writes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-18T09:59:24Z","receivedAt":"2024-09-18T09:59:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the third version of my patch series that implements support for\ngraceful concurrent writes with the reftable backend.\n\nThere is only a single change compared to v2, namely that we handle `0`\nand `-1` for the lock timeout config. With `0` we fail immediately, with\n`-1` we lock indefinitely. This matches semantics of loose and packed\nref locking.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (3):\n refs/reftable: introduce \"reftable.lockTimeout\"\n reftable/stack: allow locking of outdated stacks\n refs/reftable: reload locked stack when preparing transaction\n\n Documentation/config/reftable.txt | 8 ++++\n refs/reftable-backend.c | 13 +++++-\n reftable/reftable-stack.h | 13 +++++-\n reftable/reftable-writer.h | 11 +++++\n reftable/stack.c | 38 ++++++++++++------\n t/t0610-reftable-basics.sh | 58 ++++++++++++++++++++++++++\n t/unit-tests/t-reftable-stack.c | 67 ++++++++++++++++++++++++++++++-\n 7 files changed, 189 insertions(+), 19 deletions(-)\n\nRange-diff against v2:\n1: 700a35df125 ! 1: 77cffd3b1eb refs/reftable: introduce \"reftable.lockTimeout\"\n @@ Documentation/config/reftable.txt: reftable.geometricFactor::\n +\tWhenever the reftable backend appends a new table to the stack, it has\n +\tto lock the central \"tables.list\" file before updating it. This config\n +\tcontrols how long the process will wait to acquire the lock in case\n -+\tanother process has already acquired it. Default is 100 (i.e., retry\n -+\tfor 100ms).\n ++\tanother process has already acquired it. Value 0 means not to retry at\n ++\tall; -1 means to try indefinitely. Default is 100 (i.e., retry for\n ++\t100ms).\n \n ## refs/reftable-backend.c ##\n @@ refs/reftable-backend.c: static int reftable_be_config(const char *var, const char *value,\n @@ refs/reftable-backend.c: static int reftable_be_config(const char *var, const ch\n \t\t\tdie(\"reftable geometric factor cannot exceed %u\", (unsigned)UINT8_MAX);\n \t\topts->auto_compaction_factor = factor;\n +\t} else if (!strcmp(var, \"reftable.locktimeout\")) {\n -+\t\tunsigned long lock_timeout = git_config_ulong(var, value, ctx->kvi);\n ++\t\tint64_t lock_timeout = git_config_int64(var, value, ctx->kvi);\n ++\t\tif (lock_timeout > LONG_MAX)\n ++\t\t\tdie(\"reftable lock timeout cannot exceed %\"PRIdMAX, (intmax_t)LONG_MAX);\n ++\t\tif (lock_timeout < 0 && lock_timeout != -1)\n ++\t\t\tdie(\"reftable lock timeout does not support negative values other than -1\");\n +\t\topts->lock_timeout_ms = lock_timeout;\n \t}\n \n @@ reftable/reftable-writer.h: struct reftable_write_options {\n +\t * Note that this does not apply to locking individual tables, as these\n +\t * should only ever be locked when already holding the \"tables.list\"\n +\t * lock.\n ++\t *\n ++\t * Passing 0 will fail immediately when the file is locked, passing a\n ++\t * negative value will cause us to block indefinitely.\n +\t */\n -+\tunsigned lock_timeout_ms;\n ++\tlong lock_timeout_ms;\n };\n \n /* reftable_block_stats holds statistics for a single block type */\n2: f4be0966e17 = 2: 6130565498e reftable/stack: allow locking of outdated stacks\n3: 111b497ef17 = 3: 25d4e513a36 refs/reftable: reload locked stack when preparing transaction\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502990","messageId":"77cffd3b1eb638e05c031e2949fdc9374d599e05.1726653185.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1726653185.git.ps@pks.im","subject":"[PATCH v3 1/3] refs/reftable: introduce \"reftable.lockTimeout\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-18T09:59:27Z","receivedAt":"2024-09-18T09:59:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When multiple concurrent processes try to update references in a\nrepository they may try to lock the same lockfiles. This can happen even\nwhen the updates are non-conflicting and can both be applied, so it\ndoesn't always make sense to abort the transaction immediately. Both the\n\"loose\" and \"packed\" backends thus have a grace period that they wait\nfor the lock to be released that can be controlled via the config values\n\"core.filesRefLockTimeout\" and \"core.packedRefsTimeout\", respectively.\n\nThe reftable backend doesn't have such a setting yet and instead fails\nimmediately when it sees such a lock. But the exact same concepts apply\nhere as they do apply to the other backends.\n\nIntroduce a new \"reftable.lockTimeout\" config that controls how long we\nmay wait for a \"tables.list\" lock to be released. The default value of\nthis config is 100ms, which is the same default as we have it for the\n\"loose\" backend.\n\nNote that even though we also lock individual tables, this config really\nonly applies to the \"tables.list\" file. This is because individual\ntables are only ever locked when we already hold the \"tables.list\" lock\nduring compaction. When we observe such a lock we in fact do not want to\ncompact the table at all because it is already in the process of being\ncompacted by a concurrent process. So applying the same timeout here\nwould not make any sense and only delay progress.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/config/reftable.txt | 8 ++++++++\n refs/reftable-backend.c | 8 ++++++++\n reftable/reftable-writer.h | 11 +++++++++++\n reftable/stack.c | 18 ++++++++++++------\n t/t0610-reftable-basics.sh | 27 +++++++++++++++++++++++++++\n 5 files changed, 66 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config/reftable.txt b/Documentation/config/reftable.txt\nindex 05157279778..57087803a54 100644\n--- a/Documentation/config/reftable.txt\n+++ b/Documentation/config/reftable.txt\n@@ -46,3 +46,11 @@ reftable.geometricFactor::\n By default, the geometric sequence uses a factor of 2, meaning that for any\n table, the next-biggest table must at least be twice as big. A maximum factor\n of 256 is supported.\n+\n+reftable.lockTimeout::\n+\tWhenever the reftable backend appends a new table to the stack, it has\n+\tto lock the central \"tables.list\" file before updating it. This config\n+\tcontrols how long the process will wait to acquire the lock in case\n+\tanother process has already acquired it. Value 0 means not to retry at\n+\tall; -1 means to try indefinitely. Default is 100 (i.e., retry for\n+\t100ms).\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 1c4b19e737f..ca281e39a29 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -256,6 +256,13 @@ static int reftable_be_config(const char *var, const char *value,\n \t\tif (factor > UINT8_MAX)\n \t\t\tdie(\"reftable geometric factor cannot exceed %u\", (unsigned)UINT8_MAX);\n \t\topts->auto_compaction_factor = factor;\n+\t} else if (!strcmp(var, \"reftable.locktimeout\")) {\n+\t\tint64_t lock_timeout = git_config_int64(var, value, ctx->kvi);\n+\t\tif (lock_timeout > LONG_MAX)\n+\t\t\tdie(\"reftable lock timeout cannot exceed %\"PRIdMAX, (intmax_t)LONG_MAX);\n+\t\tif (lock_timeout < 0 && lock_timeout != -1)\n+\t\t\tdie(\"reftable lock timeout does not support negative values other than -1\");\n+\t\topts->lock_timeout_ms = lock_timeout;\n \t}\n \n \treturn 0;\n@@ -281,6 +288,7 @@ static struct ref_store *reftable_be_init(struct repository *repo,\n \trefs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);\n \trefs->write_options.disable_auto_compact =\n \t\t!git_env_bool(\"GIT_TEST_REFTABLE_AUTOCOMPACTION\", 1);\n+\trefs->write_options.lock_timeout_ms = 100;\n \n \tgit_config(reftable_be_config, &refs->write_options);\n \ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex 189b1f4144f..f5e25cfda16 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -51,6 +51,17 @@ struct reftable_write_options {\n \t * tables to compact. Defaults to 2 if unset.\n \t */\n \tuint8_t auto_compaction_factor;\n+\n+\t/*\n+\t * The number of milliseconds to wait when trying to lock \"tables.list\".\n+\t * Note that this does not apply to locking individual tables, as these\n+\t * should only ever be locked when already holding the \"tables.list\"\n+\t * lock.\n+\t *\n+\t * Passing 0 will fail immediately when the file is locked, passing a\n+\t * negative value will cause us to block indefinitely.\n+\t */\n+\tlong lock_timeout_ms;\n };\n \n /* reftable_block_stats holds statistics for a single block type */\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ce0a35216ba..5ccad2cff31 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -603,8 +603,10 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \n \tadd->stack = st;\n \n-\terr = hold_lock_file_for_update(&add->tables_list_lock, st->list_file,\n-\t\t\t\t\tLOCK_NO_DEREF);\n+\terr = hold_lock_file_for_update_timeout(&add->tables_list_lock,\n+\t\t\t\t\t\tst->list_file,\n+\t\t\t\t\t\tLOCK_NO_DEREF,\n+\t\t\t\t\t\tst->opts.lock_timeout_ms);\n \tif (err < 0) {\n \t\tif (errno == EEXIST) {\n \t\t\terr = REFTABLE_LOCK_ERROR;\n@@ -1056,8 +1058,10 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * Hold the lock so that we can read \"tables.list\" and lock all tables\n \t * which are part of the user-specified range.\n \t */\n-\terr = hold_lock_file_for_update(&tables_list_lock, st->list_file,\n-\t\t\t\t\tLOCK_NO_DEREF);\n+\terr = hold_lock_file_for_update_timeout(&tables_list_lock,\n+\t\t\t\t\t\tst->list_file,\n+\t\t\t\t\t\tLOCK_NO_DEREF,\n+\t\t\t\t\t\tst->opts.lock_timeout_ms);\n \tif (err < 0) {\n \t\tif (errno == EEXIST)\n \t\t\terr = REFTABLE_LOCK_ERROR;\n@@ -1156,8 +1160,10 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * \"tables.list\". We'll then replace the compacted range of tables with\n \t * the new table.\n \t */\n-\terr = hold_lock_file_for_update(&tables_list_lock, st->list_file,\n-\t\t\t\t\tLOCK_NO_DEREF);\n+\terr = hold_lock_file_for_update_timeout(&tables_list_lock,\n+\t\t\t\t\t\tst->list_file,\n+\t\t\t\t\t\tLOCK_NO_DEREF,\n+\t\t\t\t\t\tst->opts.lock_timeout_ms);\n \tif (err < 0) {\n \t\tif (errno == EEXIST)\n \t\t\terr = REFTABLE_LOCK_ERROR;\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 37510c2b2a5..62da3e37823 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -423,6 +423,33 @@ test_expect_success 'ref transaction: fails gracefully when auto compaction fail\n \t)\n '\n \n+test_expect_success 'ref transaction: timeout acquiring tables.list lock' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit initial &&\n+\t\t>.git/reftable/tables.list.lock &&\n+\t\ttest_must_fail git update-ref refs/heads/branch HEAD 2>err &&\n+\t\ttest_grep \"cannot lock references\" err\n+\t)\n+'\n+\n+test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit initial &&\n+\t\tLOCK=.git/reftable/tables.list.lock &&\n+\t\t>$LOCK &&\n+\t\t{\n+\t\t\t( sleep 1 && rm -f $LOCK ) &\n+\t\t} &&\n+\t\tgit -c reftable.lockTimeout=5000 update-ref refs/heads/branch HEAD\n+\t)\n+'\n+\n test_expect_success 'pack-refs: compacts tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502991","messageId":"6130565498e51ce3228d6fe5bc5f0b83c0db9182.1726653185.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1726653185.git.ps@pks.im","subject":"[PATCH v3 2/3] reftable/stack: allow locking of outdated stacks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-18T09:59:31Z","receivedAt":"2024-09-18T09:59:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `reftable_stack_new_addition()` we first lock the stack and then\ncheck whether it is still up-to-date. If it is not we return an error to\nthe caller indicating that the stack is outdated.\n\nThis is overly restrictive in our ref transaction interface though: we\nlock the stack right before we start to verify the transaction, so we do\nnot really care whether it is outdated or not. What we really want is\nthat the stack is up-to-date after it has been locked so that we can\nverify queued updates against its current state while we know that it is\nlocked for concurrent modification.\n\nIntroduce a new flag `REFTABLE_STACK_NEW_ADDITION_RELOAD` that alters\nthe behaviour of `reftable_stack_init_addition()` in this case: when we\nnotice that it is out-of-date we reload it instead of returning an error\nto the caller.\n\nThis logic will be wired up in the reftable backend in the next commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 4 +-\n reftable/reftable-stack.h | 13 ++++++-\n reftable/stack.c | 20 ++++++----\n t/unit-tests/t-reftable-stack.c | 67 ++++++++++++++++++++++++++++++++-\n 4 files changed, 91 insertions(+), 13 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex ca281e39a29..6ca00627dd7 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -770,7 +770,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n \t\tif (ret)\n \t\t\treturn ret;\n \n-\t\tret = reftable_stack_new_addition(&addition, stack);\n+\t\tret = reftable_stack_new_addition(&addition, stack, 0);\n \t\tif (ret) {\n \t\t\tif (ret == REFTABLE_LOCK_ERROR)\n \t\t\t\tstrbuf_addstr(err, \"cannot lock references\");\n@@ -2207,7 +2207,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,\n \tif (ret < 0)\n \t\tgoto done;\n \n-\tret = reftable_stack_new_addition(&add, stack);\n+\tret = reftable_stack_new_addition(&add, stack, 0);\n \tif (ret < 0)\n \t\tgoto done;\n \ndiff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\nindex f4f8cabc7fb..67316b215ed 100644\n--- a/reftable/reftable-stack.h\n+++ b/reftable/reftable-stack.h\n@@ -37,12 +37,21 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st);\n /* holds a transaction to add tables at the top of a stack. */\n struct reftable_addition;\n \n+enum {\n+\t/*\n+\t * Reload the stack when the stack is out-of-date after locking it.\n+\t */\n+\tREFTABLE_STACK_NEW_ADDITION_RELOAD = (1 << 0),\n+};\n+\n /*\n * returns a new transaction to add reftables to the given stack. As a side\n- * effect, the ref database is locked.\n+ * effect, the ref database is locked. Accepts REFTABLE_STACK_NEW_ADDITION_*\n+ * flags.\n */\n int reftable_stack_new_addition(struct reftable_addition **dest,\n-\t\t\t\tstruct reftable_stack *st);\n+\t\t\t\tstruct reftable_stack *st,\n+\t\t\t\tint flags);\n \n /* Adds a reftable to transaction. */\n int reftable_addition_add(struct reftable_addition *add,\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 5ccad2cff31..f9c95d5fa62 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -596,7 +596,8 @@ struct reftable_addition {\n #define REFTABLE_ADDITION_INIT {0}\n \n static int reftable_stack_init_addition(struct reftable_addition *add,\n-\t\t\t\t\tstruct reftable_stack *st)\n+\t\t\t\t\tstruct reftable_stack *st,\n+\t\t\t\t\tint flags)\n {\n \tstruct strbuf lock_file_name = STRBUF_INIT;\n \tint err;\n@@ -626,6 +627,11 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \terr = stack_uptodate(st);\n \tif (err < 0)\n \t\tgoto done;\n+\tif (err > 0 && flags & REFTABLE_STACK_NEW_ADDITION_RELOAD) {\n+\t\terr = reftable_stack_reload_maybe_reuse(add->stack, 1);\n+\t\tif (err)\n+\t\t\tgoto done;\n+\t}\n \tif (err > 0) {\n \t\terr = REFTABLE_OUTDATED_ERROR;\n \t\tgoto done;\n@@ -633,9 +639,8 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \n \tadd->next_update_index = reftable_stack_next_update_index(st);\n done:\n-\tif (err) {\n+\tif (err)\n \t\treftable_addition_close(add);\n-\t}\n \tstrbuf_release(&lock_file_name);\n \treturn err;\n }\n@@ -739,13 +744,14 @@ int reftable_addition_commit(struct reftable_addition *add)\n }\n \n int reftable_stack_new_addition(struct reftable_addition **dest,\n-\t\t\t\tstruct reftable_stack *st)\n+\t\t\t\tstruct reftable_stack *st,\n+\t\t\t\tint flags)\n {\n \tint err = 0;\n \tstruct reftable_addition empty = REFTABLE_ADDITION_INIT;\n \tREFTABLE_CALLOC_ARRAY(*dest, 1);\n \t**dest = empty;\n-\terr = reftable_stack_init_addition(*dest, st);\n+\terr = reftable_stack_init_addition(*dest, st, flags);\n \tif (err) {\n \t\treftable_free(*dest);\n \t\t*dest = NULL;\n@@ -759,7 +765,7 @@ static int stack_try_add(struct reftable_stack *st,\n \t\t\t void *arg)\n {\n \tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n-\tint err = reftable_stack_init_addition(&add, st);\n+\tint err = reftable_stack_init_addition(&add, st, 0);\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -1608,7 +1614,7 @@ static int reftable_stack_clean_locked(struct reftable_stack *st)\n int reftable_stack_clean(struct reftable_stack *st)\n {\n \tstruct reftable_addition *add = NULL;\n-\tint err = reftable_stack_new_addition(&add, st);\n+\tint err = reftable_stack_new_addition(&add, st, 0);\n \tif (err < 0) {\n \t\tgoto done;\n \t}\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex d62a9c1bed5..a37cc698d87 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -271,7 +271,7 @@ static void t_reftable_stack_transaction_api(void)\n \n \treftable_addition_destroy(add);\n \n-\terr = reftable_stack_new_addition(&add, st);\n+\terr = reftable_stack_new_addition(&add, st, 0);\n \tcheck(!err);\n \n \terr = reftable_addition_add(add, write_test_ref, &ref);\n@@ -292,6 +292,68 @@ static void t_reftable_stack_transaction_api(void)\n \tclear_dir(dir);\n }\n \n+static void t_reftable_stack_transaction_with_reload(void)\n+{\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tstruct reftable_stack *st1 = NULL, *st2 = NULL;\n+\tint err;\n+\tstruct reftable_addition *add = NULL;\n+\tstruct reftable_ref_record refs[2] = {\n+\t\t{\n+\t\t\t.refname = (char *) \"refs/heads/a\",\n+\t\t\t.update_index = 1,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { '1' },\n+\t\t},\n+\t\t{\n+\t\t\t.refname = (char *) \"refs/heads/b\",\n+\t\t\t.update_index = 2,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { '1' },\n+\t\t},\n+\t};\n+\tstruct reftable_ref_record ref = { 0 };\n+\n+\terr = reftable_new_stack(&st1, dir, NULL);\n+\tcheck(!err);\n+\terr = reftable_new_stack(&st2, dir, NULL);\n+\tcheck(!err);\n+\n+\terr = reftable_stack_new_addition(&add, st1, 0);\n+\tcheck(!err);\n+\terr = reftable_addition_add(add, write_test_ref, &refs[0]);\n+\tcheck(!err);\n+\terr = reftable_addition_commit(add);\n+\tcheck(!err);\n+\treftable_addition_destroy(add);\n+\n+\t/*\n+\t * The second stack is now outdated, which we should notice. We do not\n+\t * create the addition and lock the stack by default, but allow the\n+\t * reload to happen when REFTABLE_STACK_NEW_ADDITION_RELOAD is set.\n+\t */\n+\terr = reftable_stack_new_addition(&add, st2, 0);\n+\tcheck_int(err, ==, REFTABLE_OUTDATED_ERROR);\n+\terr = reftable_stack_new_addition(&add, st2, REFTABLE_STACK_NEW_ADDITION_RELOAD);\n+\tcheck(!err);\n+\terr = reftable_addition_add(add, write_test_ref, &refs[1]);\n+\tcheck(!err);\n+\terr = reftable_addition_commit(add);\n+\tcheck(!err);\n+\treftable_addition_destroy(add);\n+\n+\tfor (size_t i = 0; i < ARRAY_SIZE(refs); i++) {\n+\t\terr = reftable_stack_read_ref(st2, refs[i].refname, &ref);\n+\t\tcheck(!err);\n+\t\tcheck(reftable_ref_record_equal(&refs[i], &ref, GIT_SHA1_RAWSZ));\n+\t}\n+\n+\treftable_ref_record_release(&ref);\n+\treftable_stack_destroy(st1);\n+\treftable_stack_destroy(st2);\n+\tclear_dir(dir);\n+}\n+\n static void t_reftable_stack_transaction_api_performs_auto_compaction(void)\n {\n \tchar *dir = get_tmp_dir(__LINE__);\n@@ -322,7 +384,7 @@ static void t_reftable_stack_transaction_api_performs_auto_compaction(void)\n \t\t */\n \t\tst->opts.disable_auto_compact = i != n;\n \n-\t\terr = reftable_stack_new_addition(&add, st);\n+\t\terr = reftable_stack_new_addition(&add, st, 0);\n \t\tcheck(!err);\n \n \t\terr = reftable_addition_add(add, write_test_ref, &ref);\n@@ -1314,6 +1376,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \tTEST(t_reftable_stack_reload_with_missing_table(), \"stack iteration with garbage tables\");\n \tTEST(t_reftable_stack_tombstone(), \"'tombstone' refs in stack\");\n \tTEST(t_reftable_stack_transaction_api(), \"update transaction to stack\");\n+\tTEST(t_reftable_stack_transaction_with_reload(), \"transaction with reload\");\n \tTEST(t_reftable_stack_transaction_api_performs_auto_compaction(), \"update transaction triggers auto-compaction\");\n \tTEST(t_reftable_stack_update_index_check(), \"update transactions with equal update indices\");\n \tTEST(t_reftable_stack_uptodate(), \"stack must be reloaded before ref update\");\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"502992","messageId":"25d4e513a368a0787b6f78466f85a3155ca3a740.1726653185.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1726653185.git.ps@pks.im","subject":"[PATCH v3 3/3] refs/reftable: reload locked stack when preparing transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-18T09:59:35Z","receivedAt":"2024-09-18T09:59:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When starting a reftable transaction we lock all stacks we are about to\nmodify. While it may happen that the stack is out-of-date at this point\nin time we don't really care: transactional updates encode the expected\nstate of a certain reference, so all that we really want to verify is\nthat the _current_ value matches that expected state.\n\nPass `REFTABLE_STACK_NEW_ADDITION_RELOAD` when locking the stack such\nthat an out-of-date stack will be reloaded after having been locked.\nThis change is safe because all verifications of the expected state\nhappen after this step anyway.\n\nAdd a testcase that verifies that many writers are now able to write to\nthe stack concurrently without failures and with a deterministic end\nresult.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 3 ++-\n t/t0610-reftable-basics.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 6ca00627dd7..efe2b0258ca 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -770,7 +770,8 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n \t\tif (ret)\n \t\t\treturn ret;\n \n-\t\tret = reftable_stack_new_addition(&addition, stack, 0);\n+\t\tret = reftable_stack_new_addition(&addition, stack,\n+\t\t\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \t\tif (ret) {\n \t\t\tif (ret == REFTABLE_LOCK_ERROR)\n \t\t\t\tstrbuf_addstr(err, \"cannot lock references\");\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 62da3e37823..2d951c8ceb6 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -450,6 +450,37 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n \t)\n '\n \n+test_expect_success 'ref transaction: many concurrent writers' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\t# Set a high timeout such that a busy CI machine will not abort\n+\t\t# early. 10 seconds should hopefully be ample of time to make\n+\t\t# this non-flaky.\n+\t\tgit config set reftable.lockTimeout 10000 &&\n+\t\ttest_commit --no-tag initial &&\n+\n+\t\thead=$(git rev-parse HEAD) &&\n+\t\tfor i in $(test_seq 100)\n+\t\tdo\n+\t\t\tprintf \"%s commit\\trefs/heads/branch-%s\\n\" \"$head\" \"$i\" ||\n+\t\t\treturn 1\n+\t\tdone >expect &&\n+\t\tprintf \"%s commit\\trefs/heads/main\\n\" \"$head\" >>expect &&\n+\n+\t\tfor i in $(test_seq 100)\n+\t\tdo\n+\t\t\t{ git update-ref refs/heads/branch-$i HEAD& } ||\n+\t\t\treturn 1\n+\t\tdone &&\n+\n+\t\twait &&\n+\t\tgit for-each-ref --sort=v:refname >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'pack-refs: compacts tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"503035","messageId":"D49SPD0WS6X4.20XXOKHPNQNH6@jamesliu.io","threadId":"62129","inReplyTo":"cover.1726653185.git.ps@pks.im","subject":"Re: [PATCH v3 0/3] reftable: graceful concurrent writes","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-09-18T23:23:21Z","receivedAt":"2024-09-18T23:23:26Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"On Wed Sep 18, 2024 at 7:59 PM AEST, Patrick Steinhardt wrote:\n> Hi,\n>\n> this is the third version of my patch series that implements support for\n> graceful concurrent writes with the reftable backend.\n>\n> There is only a single change compared to v2, namely that we handle `0`\n> and `-1` for the lock timeout config. With `0` we fail immediately, with\n> `-1` we lock indefinitely. This matches semantics of loose and packed\n> ref locking.\n\nI guess you meant \"we retry indefinitely\" here :D\n\n>\n> Thanks!\n>\n> Patrick\n\nThanks Patrick, no more comments from me!\n\nCheers,\nJames\n"},{"id":"503098","messageId":"xmqqmsk35nvu.fsf@gitster.g","threadId":"62129","inReplyTo":"77cffd3b1eb638e05c031e2949fdc9374d599e05.1726653185.git.ps@pks.im","subject":"Re: [PATCH v3 1/3] refs/reftable: introduce \"reftable.lockTimeout\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-19T21:34:13Z","receivedAt":"2024-09-19T21:34:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> +reftable.lockTimeout::\n> +\tWhenever the reftable backend appends a new table to the stack, it has\n> +\tto lock the central \"tables.list\" file before updating it. This config\n> +\tcontrols how long the process will wait to acquire the lock in case\n> +\tanother process has already acquired it. Value 0 means not to retry at\n> +\tall; -1 means to try indefinitely. Default is 100 (i.e., retry for\n> +\t100ms).\n\nExisting timeout knobs are in a hierarchy that is too wide\n(i.e. core.*timeout) and this fixes the mistake by placing the name\nin a lot more appropriate name (i.e. reftable.*timeout). If I were\ndesigning the system from scratch, I would probably place all of\nthem in \"refs.*timeout\", but I do not think it is worth extra\nengineering effort to rename them and pay the transition cost.\n\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index 1c4b19e737f..ca281e39a29 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -256,6 +256,13 @@ static int reftable_be_config(const char *var, const char *value,\n> \t\tif (factor > UINT8_MAX)\n> \t\t\tdie(\"reftable geometric factor cannot exceed %u\", (unsigned)UINT8_MAX);\n> \t\topts->auto_compaction_factor = factor;\n> +\t} else if (!strcmp(var, \"reftable.locktimeout\")) {\n> +\t\tint64_t lock_timeout = git_config_int64(var, value, ctx->kvi);\n> +\t\tif (lock_timeout > LONG_MAX)\n> +\t\t\tdie(\"reftable lock timeout cannot exceed %\"PRIdMAX, (intmax_t)LONG_MAX);\n> +\t\tif (lock_timeout < 0 && lock_timeout != -1)\n> +\t\t\tdie(\"reftable lock timeout does not support negative values other than -1\");\n> +\t\topts->lock_timeout_ms = lock_timeout;\n\nExisting lock timeouts this models after seems to consider a\nplatform native \"int\" is good enough size to represent the timeout\nvalue in milliseconds, but the eventual user of this value in the\nlockfile API expects a long, so lock_timeout_ms being long is fine.\n\nPerhaps #leftoverbits to straighten out the types used for the other\ntwo timeout configuration variables.\n"},{"id":"503152","messageId":"xmqqzfo2gprt.fsf@gitster.g","threadId":"62129","inReplyTo":"6130565498e51ce3228d6fe5bc5f0b83c0db9182.1726653185.git.ps@pks.im","subject":"Re: [PATCH v3 2/3] reftable/stack: allow locking of outdated stacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-20T18:10:14Z","receivedAt":"2024-09-20T18:10:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> int reftable_stack_new_addition(struct reftable_addition **dest,\n> -\t\t\t\tstruct reftable_stack *st);\n> +\t\t\t\tstruct reftable_stack *st,\n> +\t\t\t\tint flags);\n\nWe usually use \"unsigned\" for a flag word, i.e., a collection of\nflag bits.\n\n> @@ -1314,6 +1376,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n> \tTEST(t_reftable_stack_reload_with_missing_table(), \"stack iteration with garbage tables\");\n> \tTEST(t_reftable_stack_tombstone(), \"'tombstone' refs in stack\");\n> \tTEST(t_reftable_stack_transaction_api(), \"update transaction to stack\");\n> +\tTEST(t_reftable_stack_transaction_with_reload(), \"transaction with reload\");\n> \tTEST(t_reftable_stack_transaction_api_performs_auto_compaction(), \"update transaction triggers auto-compaction\");\n> \tTEST(t_reftable_stack_update_index_check(), \"update transactions with equal update indices\");\n> \tTEST(t_reftable_stack_uptodate(), \"stack must be reloaded before ref update\");\n\nLooking good.\n"},{"id":"503298","messageId":"cover.1727155858.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1726578382.git.ps@pks.im","subject":"[PATCH v4 0/3] reftable: graceful concurrent writes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-24T05:32:59Z","receivedAt":"2024-09-24T05:33:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the fourth version of my patch series that implements for\ngraceful concurrent writes with the reftable backend.\n\nThere's only a single change, namely a fix to the type used by the newly\nintroduced `flags` field.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (3):\n refs/reftable: introduce \"reftable.lockTimeout\"\n reftable/stack: allow locking of outdated stacks\n refs/reftable: reload locked stack when preparing transaction\n\n Documentation/config/reftable.txt | 8 ++++\n refs/reftable-backend.c | 13 +++++-\n reftable/reftable-stack.h | 13 +++++-\n reftable/reftable-writer.h | 11 +++++\n reftable/stack.c | 38 ++++++++++++------\n t/t0610-reftable-basics.sh | 58 ++++++++++++++++++++++++++\n t/unit-tests/t-reftable-stack.c | 67 ++++++++++++++++++++++++++++++-\n 7 files changed, 189 insertions(+), 19 deletions(-)\n\nRange-diff against v3:\n1: 77cffd3b1eb = 1: 77cffd3b1eb refs/reftable: introduce \"reftable.lockTimeout\"\n2: 6130565498e ! 2: 81a836062e9 reftable/stack: allow locking of outdated stacks\n @@ reftable/reftable-stack.h: uint64_t reftable_stack_next_update_index(struct reft\n int reftable_stack_new_addition(struct reftable_addition **dest,\n -\t\t\t\tstruct reftable_stack *st);\n +\t\t\t\tstruct reftable_stack *st,\n -+\t\t\t\tint flags);\n ++\t\t\t\tunsigned int flags);\n \n /* Adds a reftable to transaction. */\n int reftable_addition_add(struct reftable_addition *add,\n @@ reftable/stack.c: struct reftable_addition {\n static int reftable_stack_init_addition(struct reftable_addition *add,\n -\t\t\t\t\tstruct reftable_stack *st)\n +\t\t\t\t\tstruct reftable_stack *st,\n -+\t\t\t\t\tint flags)\n ++\t\t\t\t\tunsigned int flags)\n {\n \tstruct strbuf lock_file_name = STRBUF_INIT;\n \tint err;\n @@ reftable/stack.c: int reftable_addition_commit(struct reftable_addition *add)\n int reftable_stack_new_addition(struct reftable_addition **dest,\n -\t\t\t\tstruct reftable_stack *st)\n +\t\t\t\tstruct reftable_stack *st,\n -+\t\t\t\tint flags)\n ++\t\t\t\tunsigned int flags)\n {\n \tint err = 0;\n \tstruct reftable_addition empty = REFTABLE_ADDITION_INIT;\n3: 25d4e513a36 = 3: 9ce2d18dff2 refs/reftable: reload locked stack when preparing transaction\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"503299","messageId":"77cffd3b1eb638e05c031e2949fdc9374d599e05.1727155858.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1727155858.git.ps@pks.im","subject":"[PATCH v4 1/3] refs/reftable: introduce \"reftable.lockTimeout\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-24T05:33:02Z","receivedAt":"2024-09-24T05:33:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When multiple concurrent processes try to update references in a\nrepository they may try to lock the same lockfiles. This can happen even\nwhen the updates are non-conflicting and can both be applied, so it\ndoesn't always make sense to abort the transaction immediately. Both the\n\"loose\" and \"packed\" backends thus have a grace period that they wait\nfor the lock to be released that can be controlled via the config values\n\"core.filesRefLockTimeout\" and \"core.packedRefsTimeout\", respectively.\n\nThe reftable backend doesn't have such a setting yet and instead fails\nimmediately when it sees such a lock. But the exact same concepts apply\nhere as they do apply to the other backends.\n\nIntroduce a new \"reftable.lockTimeout\" config that controls how long we\nmay wait for a \"tables.list\" lock to be released. The default value of\nthis config is 100ms, which is the same default as we have it for the\n\"loose\" backend.\n\nNote that even though we also lock individual tables, this config really\nonly applies to the \"tables.list\" file. This is because individual\ntables are only ever locked when we already hold the \"tables.list\" lock\nduring compaction. When we observe such a lock we in fact do not want to\ncompact the table at all because it is already in the process of being\ncompacted by a concurrent process. So applying the same timeout here\nwould not make any sense and only delay progress.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/config/reftable.txt | 8 ++++++++\n refs/reftable-backend.c | 8 ++++++++\n reftable/reftable-writer.h | 11 +++++++++++\n reftable/stack.c | 18 ++++++++++++------\n t/t0610-reftable-basics.sh | 27 +++++++++++++++++++++++++++\n 5 files changed, 66 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config/reftable.txt b/Documentation/config/reftable.txt\nindex 05157279778..57087803a54 100644\n--- a/Documentation/config/reftable.txt\n+++ b/Documentation/config/reftable.txt\n@@ -46,3 +46,11 @@ reftable.geometricFactor::\n By default, the geometric sequence uses a factor of 2, meaning that for any\n table, the next-biggest table must at least be twice as big. A maximum factor\n of 256 is supported.\n+\n+reftable.lockTimeout::\n+\tWhenever the reftable backend appends a new table to the stack, it has\n+\tto lock the central \"tables.list\" file before updating it. This config\n+\tcontrols how long the process will wait to acquire the lock in case\n+\tanother process has already acquired it. Value 0 means not to retry at\n+\tall; -1 means to try indefinitely. Default is 100 (i.e., retry for\n+\t100ms).\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 1c4b19e737f..ca281e39a29 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -256,6 +256,13 @@ static int reftable_be_config(const char *var, const char *value,\n \t\tif (factor > UINT8_MAX)\n \t\t\tdie(\"reftable geometric factor cannot exceed %u\", (unsigned)UINT8_MAX);\n \t\topts->auto_compaction_factor = factor;\n+\t} else if (!strcmp(var, \"reftable.locktimeout\")) {\n+\t\tint64_t lock_timeout = git_config_int64(var, value, ctx->kvi);\n+\t\tif (lock_timeout > LONG_MAX)\n+\t\t\tdie(\"reftable lock timeout cannot exceed %\"PRIdMAX, (intmax_t)LONG_MAX);\n+\t\tif (lock_timeout < 0 && lock_timeout != -1)\n+\t\t\tdie(\"reftable lock timeout does not support negative values other than -1\");\n+\t\topts->lock_timeout_ms = lock_timeout;\n \t}\n \n \treturn 0;\n@@ -281,6 +288,7 @@ static struct ref_store *reftable_be_init(struct repository *repo,\n \trefs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);\n \trefs->write_options.disable_auto_compact =\n \t\t!git_env_bool(\"GIT_TEST_REFTABLE_AUTOCOMPACTION\", 1);\n+\trefs->write_options.lock_timeout_ms = 100;\n \n \tgit_config(reftable_be_config, &refs->write_options);\n \ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex 189b1f4144f..f5e25cfda16 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -51,6 +51,17 @@ struct reftable_write_options {\n \t * tables to compact. Defaults to 2 if unset.\n \t */\n \tuint8_t auto_compaction_factor;\n+\n+\t/*\n+\t * The number of milliseconds to wait when trying to lock \"tables.list\".\n+\t * Note that this does not apply to locking individual tables, as these\n+\t * should only ever be locked when already holding the \"tables.list\"\n+\t * lock.\n+\t *\n+\t * Passing 0 will fail immediately when the file is locked, passing a\n+\t * negative value will cause us to block indefinitely.\n+\t */\n+\tlong lock_timeout_ms;\n };\n \n /* reftable_block_stats holds statistics for a single block type */\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ce0a35216ba..5ccad2cff31 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -603,8 +603,10 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \n \tadd->stack = st;\n \n-\terr = hold_lock_file_for_update(&add->tables_list_lock, st->list_file,\n-\t\t\t\t\tLOCK_NO_DEREF);\n+\terr = hold_lock_file_for_update_timeout(&add->tables_list_lock,\n+\t\t\t\t\t\tst->list_file,\n+\t\t\t\t\t\tLOCK_NO_DEREF,\n+\t\t\t\t\t\tst->opts.lock_timeout_ms);\n \tif (err < 0) {\n \t\tif (errno == EEXIST) {\n \t\t\terr = REFTABLE_LOCK_ERROR;\n@@ -1056,8 +1058,10 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * Hold the lock so that we can read \"tables.list\" and lock all tables\n \t * which are part of the user-specified range.\n \t */\n-\terr = hold_lock_file_for_update(&tables_list_lock, st->list_file,\n-\t\t\t\t\tLOCK_NO_DEREF);\n+\terr = hold_lock_file_for_update_timeout(&tables_list_lock,\n+\t\t\t\t\t\tst->list_file,\n+\t\t\t\t\t\tLOCK_NO_DEREF,\n+\t\t\t\t\t\tst->opts.lock_timeout_ms);\n \tif (err < 0) {\n \t\tif (errno == EEXIST)\n \t\t\terr = REFTABLE_LOCK_ERROR;\n@@ -1156,8 +1160,10 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * \"tables.list\". We'll then replace the compacted range of tables with\n \t * the new table.\n \t */\n-\terr = hold_lock_file_for_update(&tables_list_lock, st->list_file,\n-\t\t\t\t\tLOCK_NO_DEREF);\n+\terr = hold_lock_file_for_update_timeout(&tables_list_lock,\n+\t\t\t\t\t\tst->list_file,\n+\t\t\t\t\t\tLOCK_NO_DEREF,\n+\t\t\t\t\t\tst->opts.lock_timeout_ms);\n \tif (err < 0) {\n \t\tif (errno == EEXIST)\n \t\t\terr = REFTABLE_LOCK_ERROR;\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 37510c2b2a5..62da3e37823 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -423,6 +423,33 @@ test_expect_success 'ref transaction: fails gracefully when auto compaction fail\n \t)\n '\n \n+test_expect_success 'ref transaction: timeout acquiring tables.list lock' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit initial &&\n+\t\t>.git/reftable/tables.list.lock &&\n+\t\ttest_must_fail git update-ref refs/heads/branch HEAD 2>err &&\n+\t\ttest_grep \"cannot lock references\" err\n+\t)\n+'\n+\n+test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit initial &&\n+\t\tLOCK=.git/reftable/tables.list.lock &&\n+\t\t>$LOCK &&\n+\t\t{\n+\t\t\t( sleep 1 && rm -f $LOCK ) &\n+\t\t} &&\n+\t\tgit -c reftable.lockTimeout=5000 update-ref refs/heads/branch HEAD\n+\t)\n+'\n+\n test_expect_success 'pack-refs: compacts tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"503300","messageId":"81a836062e95135b4700ed2de93b6f047592624e.1727155858.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1727155858.git.ps@pks.im","subject":"[PATCH v4 2/3] reftable/stack: allow locking of outdated stacks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-24T05:33:05Z","receivedAt":"2024-09-24T05:33:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `reftable_stack_new_addition()` we first lock the stack and then\ncheck whether it is still up-to-date. If it is not we return an error to\nthe caller indicating that the stack is outdated.\n\nThis is overly restrictive in our ref transaction interface though: we\nlock the stack right before we start to verify the transaction, so we do\nnot really care whether it is outdated or not. What we really want is\nthat the stack is up-to-date after it has been locked so that we can\nverify queued updates against its current state while we know that it is\nlocked for concurrent modification.\n\nIntroduce a new flag `REFTABLE_STACK_NEW_ADDITION_RELOAD` that alters\nthe behaviour of `reftable_stack_init_addition()` in this case: when we\nnotice that it is out-of-date we reload it instead of returning an error\nto the caller.\n\nThis logic will be wired up in the reftable backend in the next commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 4 +-\n reftable/reftable-stack.h | 13 ++++++-\n reftable/stack.c | 20 ++++++----\n t/unit-tests/t-reftable-stack.c | 67 ++++++++++++++++++++++++++++++++-\n 4 files changed, 91 insertions(+), 13 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex ca281e39a29..6ca00627dd7 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -770,7 +770,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n \t\tif (ret)\n \t\t\treturn ret;\n \n-\t\tret = reftable_stack_new_addition(&addition, stack);\n+\t\tret = reftable_stack_new_addition(&addition, stack, 0);\n \t\tif (ret) {\n \t\t\tif (ret == REFTABLE_LOCK_ERROR)\n \t\t\t\tstrbuf_addstr(err, \"cannot lock references\");\n@@ -2207,7 +2207,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,\n \tif (ret < 0)\n \t\tgoto done;\n \n-\tret = reftable_stack_new_addition(&add, stack);\n+\tret = reftable_stack_new_addition(&add, stack, 0);\n \tif (ret < 0)\n \t\tgoto done;\n \ndiff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\nindex f4f8cabc7fb..6370fe45ddf 100644\n--- a/reftable/reftable-stack.h\n+++ b/reftable/reftable-stack.h\n@@ -37,12 +37,21 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st);\n /* holds a transaction to add tables at the top of a stack. */\n struct reftable_addition;\n \n+enum {\n+\t/*\n+\t * Reload the stack when the stack is out-of-date after locking it.\n+\t */\n+\tREFTABLE_STACK_NEW_ADDITION_RELOAD = (1 << 0),\n+};\n+\n /*\n * returns a new transaction to add reftables to the given stack. As a side\n- * effect, the ref database is locked.\n+ * effect, the ref database is locked. Accepts REFTABLE_STACK_NEW_ADDITION_*\n+ * flags.\n */\n int reftable_stack_new_addition(struct reftable_addition **dest,\n-\t\t\t\tstruct reftable_stack *st);\n+\t\t\t\tstruct reftable_stack *st,\n+\t\t\t\tunsigned int flags);\n \n /* Adds a reftable to transaction. */\n int reftable_addition_add(struct reftable_addition *add,\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 5ccad2cff31..84cf37a2ad0 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -596,7 +596,8 @@ struct reftable_addition {\n #define REFTABLE_ADDITION_INIT {0}\n \n static int reftable_stack_init_addition(struct reftable_addition *add,\n-\t\t\t\t\tstruct reftable_stack *st)\n+\t\t\t\t\tstruct reftable_stack *st,\n+\t\t\t\t\tunsigned int flags)\n {\n \tstruct strbuf lock_file_name = STRBUF_INIT;\n \tint err;\n@@ -626,6 +627,11 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \terr = stack_uptodate(st);\n \tif (err < 0)\n \t\tgoto done;\n+\tif (err > 0 && flags & REFTABLE_STACK_NEW_ADDITION_RELOAD) {\n+\t\terr = reftable_stack_reload_maybe_reuse(add->stack, 1);\n+\t\tif (err)\n+\t\t\tgoto done;\n+\t}\n \tif (err > 0) {\n \t\terr = REFTABLE_OUTDATED_ERROR;\n \t\tgoto done;\n@@ -633,9 +639,8 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \n \tadd->next_update_index = reftable_stack_next_update_index(st);\n done:\n-\tif (err) {\n+\tif (err)\n \t\treftable_addition_close(add);\n-\t}\n \tstrbuf_release(&lock_file_name);\n \treturn err;\n }\n@@ -739,13 +744,14 @@ int reftable_addition_commit(struct reftable_addition *add)\n }\n \n int reftable_stack_new_addition(struct reftable_addition **dest,\n-\t\t\t\tstruct reftable_stack *st)\n+\t\t\t\tstruct reftable_stack *st,\n+\t\t\t\tunsigned int flags)\n {\n \tint err = 0;\n \tstruct reftable_addition empty = REFTABLE_ADDITION_INIT;\n \tREFTABLE_CALLOC_ARRAY(*dest, 1);\n \t**dest = empty;\n-\terr = reftable_stack_init_addition(*dest, st);\n+\terr = reftable_stack_init_addition(*dest, st, flags);\n \tif (err) {\n \t\treftable_free(*dest);\n \t\t*dest = NULL;\n@@ -759,7 +765,7 @@ static int stack_try_add(struct reftable_stack *st,\n \t\t\t void *arg)\n {\n \tstruct reftable_addition add = REFTABLE_ADDITION_INIT;\n-\tint err = reftable_stack_init_addition(&add, st);\n+\tint err = reftable_stack_init_addition(&add, st, 0);\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -1608,7 +1614,7 @@ static int reftable_stack_clean_locked(struct reftable_stack *st)\n int reftable_stack_clean(struct reftable_stack *st)\n {\n \tstruct reftable_addition *add = NULL;\n-\tint err = reftable_stack_new_addition(&add, st);\n+\tint err = reftable_stack_new_addition(&add, st, 0);\n \tif (err < 0) {\n \t\tgoto done;\n \t}\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex d62a9c1bed5..a37cc698d87 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -271,7 +271,7 @@ static void t_reftable_stack_transaction_api(void)\n \n \treftable_addition_destroy(add);\n \n-\terr = reftable_stack_new_addition(&add, st);\n+\terr = reftable_stack_new_addition(&add, st, 0);\n \tcheck(!err);\n \n \terr = reftable_addition_add(add, write_test_ref, &ref);\n@@ -292,6 +292,68 @@ static void t_reftable_stack_transaction_api(void)\n \tclear_dir(dir);\n }\n \n+static void t_reftable_stack_transaction_with_reload(void)\n+{\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tstruct reftable_stack *st1 = NULL, *st2 = NULL;\n+\tint err;\n+\tstruct reftable_addition *add = NULL;\n+\tstruct reftable_ref_record refs[2] = {\n+\t\t{\n+\t\t\t.refname = (char *) \"refs/heads/a\",\n+\t\t\t.update_index = 1,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { '1' },\n+\t\t},\n+\t\t{\n+\t\t\t.refname = (char *) \"refs/heads/b\",\n+\t\t\t.update_index = 2,\n+\t\t\t.value_type = REFTABLE_REF_VAL1,\n+\t\t\t.value.val1 = { '1' },\n+\t\t},\n+\t};\n+\tstruct reftable_ref_record ref = { 0 };\n+\n+\terr = reftable_new_stack(&st1, dir, NULL);\n+\tcheck(!err);\n+\terr = reftable_new_stack(&st2, dir, NULL);\n+\tcheck(!err);\n+\n+\terr = reftable_stack_new_addition(&add, st1, 0);\n+\tcheck(!err);\n+\terr = reftable_addition_add(add, write_test_ref, &refs[0]);\n+\tcheck(!err);\n+\terr = reftable_addition_commit(add);\n+\tcheck(!err);\n+\treftable_addition_destroy(add);\n+\n+\t/*\n+\t * The second stack is now outdated, which we should notice. We do not\n+\t * create the addition and lock the stack by default, but allow the\n+\t * reload to happen when REFTABLE_STACK_NEW_ADDITION_RELOAD is set.\n+\t */\n+\terr = reftable_stack_new_addition(&add, st2, 0);\n+\tcheck_int(err, ==, REFTABLE_OUTDATED_ERROR);\n+\terr = reftable_stack_new_addition(&add, st2, REFTABLE_STACK_NEW_ADDITION_RELOAD);\n+\tcheck(!err);\n+\terr = reftable_addition_add(add, write_test_ref, &refs[1]);\n+\tcheck(!err);\n+\terr = reftable_addition_commit(add);\n+\tcheck(!err);\n+\treftable_addition_destroy(add);\n+\n+\tfor (size_t i = 0; i < ARRAY_SIZE(refs); i++) {\n+\t\terr = reftable_stack_read_ref(st2, refs[i].refname, &ref);\n+\t\tcheck(!err);\n+\t\tcheck(reftable_ref_record_equal(&refs[i], &ref, GIT_SHA1_RAWSZ));\n+\t}\n+\n+\treftable_ref_record_release(&ref);\n+\treftable_stack_destroy(st1);\n+\treftable_stack_destroy(st2);\n+\tclear_dir(dir);\n+}\n+\n static void t_reftable_stack_transaction_api_performs_auto_compaction(void)\n {\n \tchar *dir = get_tmp_dir(__LINE__);\n@@ -322,7 +384,7 @@ static void t_reftable_stack_transaction_api_performs_auto_compaction(void)\n \t\t */\n \t\tst->opts.disable_auto_compact = i != n;\n \n-\t\terr = reftable_stack_new_addition(&add, st);\n+\t\terr = reftable_stack_new_addition(&add, st, 0);\n \t\tcheck(!err);\n \n \t\terr = reftable_addition_add(add, write_test_ref, &ref);\n@@ -1314,6 +1376,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \tTEST(t_reftable_stack_reload_with_missing_table(), \"stack iteration with garbage tables\");\n \tTEST(t_reftable_stack_tombstone(), \"'tombstone' refs in stack\");\n \tTEST(t_reftable_stack_transaction_api(), \"update transaction to stack\");\n+\tTEST(t_reftable_stack_transaction_with_reload(), \"transaction with reload\");\n \tTEST(t_reftable_stack_transaction_api_performs_auto_compaction(), \"update transaction triggers auto-compaction\");\n \tTEST(t_reftable_stack_update_index_check(), \"update transactions with equal update indices\");\n \tTEST(t_reftable_stack_uptodate(), \"stack must be reloaded before ref update\");\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"503301","messageId":"9ce2d18dff2a655365b609dd86ea484a489c717a.1727155858.git.ps@pks.im","threadId":"62129","inReplyTo":"cover.1727155858.git.ps@pks.im","subject":"[PATCH v4 3/3] refs/reftable: reload locked stack when preparing transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-24T05:33:08Z","receivedAt":"2024-09-24T05:33:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When starting a reftable transaction we lock all stacks we are about to\nmodify. While it may happen that the stack is out-of-date at this point\nin time we don't really care: transactional updates encode the expected\nstate of a certain reference, so all that we really want to verify is\nthat the _current_ value matches that expected state.\n\nPass `REFTABLE_STACK_NEW_ADDITION_RELOAD` when locking the stack such\nthat an out-of-date stack will be reloaded after having been locked.\nThis change is safe because all verifications of the expected state\nhappen after this step anyway.\n\nAdd a testcase that verifies that many writers are now able to write to\nthe stack concurrently without failures and with a deterministic end\nresult.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 3 ++-\n t/t0610-reftable-basics.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 6ca00627dd7..efe2b0258ca 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -770,7 +770,8 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n \t\tif (ret)\n \t\t\treturn ret;\n \n-\t\tret = reftable_stack_new_addition(&addition, stack, 0);\n+\t\tret = reftable_stack_new_addition(&addition, stack,\n+\t\t\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n \t\tif (ret) {\n \t\t\tif (ret == REFTABLE_LOCK_ERROR)\n \t\t\t\tstrbuf_addstr(err, \"cannot lock references\");\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 62da3e37823..2d951c8ceb6 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -450,6 +450,37 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n \t)\n '\n \n+test_expect_success 'ref transaction: many concurrent writers' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\t# Set a high timeout such that a busy CI machine will not abort\n+\t\t# early. 10 seconds should hopefully be ample of time to make\n+\t\t# this non-flaky.\n+\t\tgit config set reftable.lockTimeout 10000 &&\n+\t\ttest_commit --no-tag initial &&\n+\n+\t\thead=$(git rev-parse HEAD) &&\n+\t\tfor i in $(test_seq 100)\n+\t\tdo\n+\t\t\tprintf \"%s commit\\trefs/heads/branch-%s\\n\" \"$head\" \"$i\" ||\n+\t\t\treturn 1\n+\t\tdone >expect &&\n+\t\tprintf \"%s commit\\trefs/heads/main\\n\" \"$head\" >>expect &&\n+\n+\t\tfor i in $(test_seq 100)\n+\t\tdo\n+\t\t\t{ git update-ref refs/heads/branch-$i HEAD& } ||\n+\t\t\treturn 1\n+\t\tdone &&\n+\n+\t\twait &&\n+\t\tgit for-each-ref --sort=v:refname >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'pack-refs: compacts tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.46.0.551.gc5ee8f2d1c.dirty\n\n"},{"id":"503302","messageId":"ZvJPLB-VitzqEMbY@pks.im","threadId":"62129","inReplyTo":"xmqqzfo2gprt.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] reftable/stack: allow locking of outdated stacks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-24T05:33:40Z","receivedAt":"2024-09-24T05:33:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Sep 20, 2024 at 11:10:14AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > int reftable_stack_new_addition(struct reftable_addition **dest,\n> > -\t\t\t\tstruct reftable_stack *st);\n> > +\t\t\t\tstruct reftable_stack *st,\n> > +\t\t\t\tint flags);\n> \n> We usually use \"unsigned\" for a flag word, i.e., a collection of\n> flag bits.\n\nOh, yeah, this was a mere oversight. The topic hasn't yet hit next, so\nlet me fix this in a final reroll.\n\nThanks!\n\nPatrick\n"},{"id":"503303","messageId":"ZvJPOJKx8tR3KdgQ@pks.im","threadId":"62129","inReplyTo":"D49SPD0WS6X4.20XXOKHPNQNH6@jamesliu.io","subject":"Re: [PATCH v3 0/3] reftable: graceful concurrent writes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-24T05:33:44Z","receivedAt":"2024-09-24T05:33:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Sep 19, 2024 at 09:23:21AM +1000, James Liu wrote:\n> On Wed Sep 18, 2024 at 7:59 PM AEST, Patrick Steinhardt wrote:\n> > Hi,\n> >\n> > this is the third version of my patch series that implements support for\n> > graceful concurrent writes with the reftable backend.\n> >\n> > There is only a single change compared to v2, namely that we handle `0`\n> > and `-1` for the lock timeout config. With `0` we fail immediately, with\n> > `-1` we lock indefinitely. This matches semantics of loose and packed\n> > ref locking.\n> \n> I guess you meant \"we retry indefinitely\" here :D\n\nOops, yes :)\n\n> >\n> > Thanks!\n> >\n> > Patrick\n> \n> Thanks Patrick, no more comments from me!\n\nThanks for your review!\n\nPatrick\n"},{"id":"503601","messageId":"20240927040752.GA567671@coredump.intra.peff.net","threadId":"62129","inReplyTo":"9ce2d18dff2a655365b609dd86ea484a489c717a.1727155858.git.ps@pks.im","subject":"Re: [PATCH v4 3/3] refs/reftable: reload locked stack when preparing transaction","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-27T04:07:52Z","receivedAt":"2024-09-27T04:07:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 24, 2024 at 07:33:08AM +0200, Patrick Steinhardt wrote:\n\n> +test_expect_success 'ref transaction: many concurrent writers' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\t# Set a high timeout such that a busy CI machine will not abort\n> +\t\t# early. 10 seconds should hopefully be ample of time to make\n> +\t\t# this non-flaky.\n> +\t\tgit config set reftable.lockTimeout 10000 &&\n\nI saw this test racily fail in the Windows CI build. The failure is as\nyou might imagine, a few of the background update-ref invocations\nfailed:\n\n fatal: update_ref failed for ref 'refs/heads/branch-21': reftable: transaction failure: I/O error\n\nbut of course we don't notice because they're backgrounded. And then the\nexpected output is missing the branch-21 entry (and in my case,\nbranch-64 suffered a similar fate).\n\nAt first I thought we probably needed to bump the timeout (and EIO was\njust our way of passing that up the stack). But looking at the\ntimestamps in the Actions log, the whole loop took less than 10ms to\nrun.\n\nSo could this be indicative of a real contention issue specific to\nWindows? I'm wondering if something like the old \"you can't delete a\nfile somebody else has open\" restriction is biting us somehow.\n\n-Peff\n"},{"id":"503668","messageId":"ZvpJ_zpAcpezXB89@pks.im","threadId":"62129","inReplyTo":"20240927040752.GA567671@coredump.intra.peff.net","subject":"Re: [PATCH v4 3/3] refs/reftable: reload locked stack when preparing transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-30T06:49:40Z","receivedAt":"2024-09-30T06:49:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Sep 27, 2024 at 12:07:52AM -0400, Jeff King wrote:\n> On Tue, Sep 24, 2024 at 07:33:08AM +0200, Patrick Steinhardt wrote:\n> \n> > +test_expect_success 'ref transaction: many concurrent writers' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\t\t# Set a high timeout such that a busy CI machine will not abort\n> > +\t\t# early. 10 seconds should hopefully be ample of time to make\n> > +\t\t# this non-flaky.\n> > +\t\tgit config set reftable.lockTimeout 10000 &&\n> \n> I saw this test racily fail in the Windows CI build. The failure is as\n> you might imagine, a few of the background update-ref invocations\n> failed:\n> \n> fatal: update_ref failed for ref 'refs/heads/branch-21': reftable: transaction failure: I/O error\n> \n> but of course we don't notice because they're backgrounded. And then the\n> expected output is missing the branch-21 entry (and in my case,\n> branch-64 suffered a similar fate).\n> \n> At first I thought we probably needed to bump the timeout (and EIO was\n> just our way of passing that up the stack). But looking at the\n> timestamps in the Actions log, the whole loop took less than 10ms to\n> run.\n> \n> So could this be indicative of a real contention issue specific to\n> Windows? I'm wondering if something like the old \"you can't delete a\n> file somebody else has open\" restriction is biting us somehow.\n\nThanks for letting me know!\n\nI very much expect that this is the scenario that you mention, where we\ntry to delete a file that is still held open by another process. We're\ntrying to be mindful about this restriction is the reftable library by\nnot failing when a call to unlink(3P) fails for any of the tables, and\nwe do have logic in place to unlink them at a later point in time when\nnot referenced by the \"tables.list\" file. So none of the calls to unlink\nare error-checked at all.\n\nBut there's one file that we _have_ to rewrite, and that is of course\nthe \"tables.list\" file itself. We never unlink the file though but only\nrename the new instance into place. I think I recently discovered that\nwe have some problems here, because Windows seemed to allow this in some\nscenarios but not in others.\n\nIn any case, I've already set up a Windows VM last week, so I'll\ninvestigate the issue later this week.\n\nPatrick\n"},{"id":"503782","messageId":"c4lz3begoplgde5iimvk4k7cufiyryntccqo46u3fy5qvqauv3@tta5wfg2ik5t","threadId":"62129","inReplyTo":"20240927040752.GA567671@coredump.intra.peff.net","subject":"Re: [PATCH v4 3/3] refs/reftable: reload locked stack when preparing transaction","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-09-30T22:19:04Z","receivedAt":"2024-09-30T22:19:11Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.09.27 00:07, Jeff King wrote:\n> On Tue, Sep 24, 2024 at 07:33:08AM +0200, Patrick Steinhardt wrote:\n> \n> > +test_expect_success 'ref transaction: many concurrent writers' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\t\t# Set a high timeout such that a busy CI machine will not abort\n> > +\t\t# early. 10 seconds should hopefully be ample of time to make\n> > +\t\t# this non-flaky.\n> > +\t\tgit config set reftable.lockTimeout 10000 &&\n> \n> I saw this test racily fail in the Windows CI build. The failure is as\n> you might imagine, a few of the background update-ref invocations\n> failed:\n> \n> fatal: update_ref failed for ref 'refs/heads/branch-21': reftable: transaction failure: I/O error\n> \n> but of course we don't notice because they're backgrounded. And then the\n> expected output is missing the branch-21 entry (and in my case,\n> branch-64 suffered a similar fate).\n> \n> At first I thought we probably needed to bump the timeout (and EIO was\n> just our way of passing that up the stack). But looking at the\n> timestamps in the Actions log, the whole loop took less than 10ms to\n> run.\n> \n> So could this be indicative of a real contention issue specific to\n> Windows? I'm wondering if something like the old \"you can't delete a\n> file somebody else has open\" restriction is biting us somehow.\n> \n> -Peff\n\nWe're seeing repeated failures from this test case with ASan enabled.\nUnfortunately, we've only been able to reproduce this on our\n$DAYJOB-specific build system. I haven't been able to get it to fail\nusing just the upstream Makefile so far. I'll keep trying to find a way\nto reproduce this.\n\nFWIW, we're not getting I/O errors, we see the following:\nfatal: update_ref failed for ref 'refs/heads/branch-20': cannot lock references\n\nWe tried increasing the timeout in the test to 2 minutes (up from 10s),\nbut it didn't fix the failures.\n"},{"id":"503797","messageId":"Zvt6LxWm8gtJGw9S@pks.im","threadId":"62129","inReplyTo":"c4lz3begoplgde5iimvk4k7cufiyryntccqo46u3fy5qvqauv3@tta5wfg2ik5t","subject":"Re: [PATCH v4 3/3] refs/reftable: reload locked stack when preparing transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-01T04:27:33Z","receivedAt":"2024-10-01T04:27:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 30, 2024 at 03:19:04PM -0700, Josh Steadmon wrote:\n> On 2024.09.27 00:07, Jeff King wrote:\n> > On Tue, Sep 24, 2024 at 07:33:08AM +0200, Patrick Steinhardt wrote:\n> > \n> > > +test_expect_success 'ref transaction: many concurrent writers' '\n> > > +\ttest_when_finished \"rm -rf repo\" &&\n> > > +\tgit init repo &&\n> > > +\t(\n> > > +\t\tcd repo &&\n> > > +\t\t# Set a high timeout such that a busy CI machine will not abort\n> > > +\t\t# early. 10 seconds should hopefully be ample of time to make\n> > > +\t\t# this non-flaky.\n> > > +\t\tgit config set reftable.lockTimeout 10000 &&\n> > \n> > I saw this test racily fail in the Windows CI build. The failure is as\n> > you might imagine, a few of the background update-ref invocations\n> > failed:\n> > \n> > fatal: update_ref failed for ref 'refs/heads/branch-21': reftable: transaction failure: I/O error\n> > \n> > but of course we don't notice because they're backgrounded. And then the\n> > expected output is missing the branch-21 entry (and in my case,\n> > branch-64 suffered a similar fate).\n> > \n> > At first I thought we probably needed to bump the timeout (and EIO was\n> > just our way of passing that up the stack). But looking at the\n> > timestamps in the Actions log, the whole loop took less than 10ms to\n> > run.\n> > \n> > So could this be indicative of a real contention issue specific to\n> > Windows? I'm wondering if something like the old \"you can't delete a\n> > file somebody else has open\" restriction is biting us somehow.\n> > \n> > -Peff\n> \n> We're seeing repeated failures from this test case with ASan enabled.\n> Unfortunately, we've only been able to reproduce this on our\n> $DAYJOB-specific build system. I haven't been able to get it to fail\n> using just the upstream Makefile so far. I'll keep trying to find a way\n> to reproduce this.\n> \n> FWIW, we're not getting I/O errors, we see the following:\n> fatal: update_ref failed for ref 'refs/heads/branch-20': cannot lock references\n> \n> We tried increasing the timeout in the test to 2 minutes (up from 10s),\n> but it didn't fix the failures.\n\nIf this is causing problems for folks I'd say we can do the below change\nfor now. It's of course only a stop-gap solution until I find the time\nto debug this, which should be later this week or early next week.\n\nPatrick\n\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 2d951c8ceb..ad7bb39b79 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -450,7 +450,7 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n \t)\n '\n \n-test_expect_success 'ref transaction: many concurrent writers' '\n+test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \t(\n"},{"id":"503802","messageId":"xmqqjzes2s5y.fsf@gitster.g","threadId":"62129","inReplyTo":"c4lz3begoplgde5iimvk4k7cufiyryntccqo46u3fy5qvqauv3@tta5wfg2ik5t","subject":"Re: [PATCH v4 3/3] refs/reftable: reload locked stack when preparing transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-01T07:34:01Z","receivedAt":"2024-10-01T07:34:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> On 2024.09.27 00:07, Jeff King wrote:\n>> On Tue, Sep 24, 2024 at 07:33:08AM +0200, Patrick Steinhardt wrote:\n>> \n>> > +test_expect_success 'ref transaction: many concurrent writers' '\n>> > +\ttest_when_finished \"rm -rf repo\" &&\n>> > +\tgit init repo &&\n>> > +\t(\n>> > +\t\tcd repo &&\n>> > +\t\t# Set a high timeout such that a busy CI machine will not abort\n>> > +\t\t# early. 10 seconds should hopefully be ample of time to make\n>> > +\t\t# this non-flaky.\n>> > +\t\tgit config set reftable.lockTimeout 10000 &&\n>> ...\n>\n> We're seeing repeated failures from this test case with ASan enabled.\n> Unfortunately, we've only been able to reproduce this on our\n> $DAYJOB-specific build system. I haven't been able to get it to fail\n> using just the upstream Makefile so far. I'll keep trying to find a way\n> to reproduce this.\n>\n> FWIW, we're not getting I/O errors, we see the following:\n> fatal: update_ref failed for ref 'refs/heads/branch-20': cannot lock references\n>\n> We tried increasing the timeout in the test to 2 minutes (up from 10s),\n> but it didn't fix the failures.\n\nThanks for a report, and please keep digging ;-).\n\nIs your build, like Peff's, for Windows, or your variant of Linux?\n\nThanks.\n"},{"id":"503859","messageId":"joxzwkbi4uqyb6d67e24ivfvb54ev5ciqgvuxgxh4nntonjwqa@az3xgiiqg4x7","threadId":"62129","inReplyTo":"xmqqjzes2s5y.fsf@gitster.g","subject":"Re: [PATCH v4 3/3] refs/reftable: reload locked stack when preparing transaction","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-10-01T18:53:54Z","receivedAt":"2024-10-01T18:54:01Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.10.01 00:34, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n> > On 2024.09.27 00:07, Jeff King wrote:\n> >> On Tue, Sep 24, 2024 at 07:33:08AM +0200, Patrick Steinhardt wrote:\n> >> \n> >> > +test_expect_success 'ref transaction: many concurrent writers' '\n> >> > +\ttest_when_finished \"rm -rf repo\" &&\n> >> > +\tgit init repo &&\n> >> > +\t(\n> >> > +\t\tcd repo &&\n> >> > +\t\t# Set a high timeout such that a busy CI machine will not abort\n> >> > +\t\t# early. 10 seconds should hopefully be ample of time to make\n> >> > +\t\t# this non-flaky.\n> >> > +\t\tgit config set reftable.lockTimeout 10000 &&\n> >> ...\n> >\n> > We're seeing repeated failures from this test case with ASan enabled.\n> > Unfortunately, we've only been able to reproduce this on our\n> > $DAYJOB-specific build system. I haven't been able to get it to fail\n> > using just the upstream Makefile so far. I'll keep trying to find a way\n> > to reproduce this.\n> >\n> > FWIW, we're not getting I/O errors, we see the following:\n> > fatal: update_ref failed for ref 'refs/heads/branch-20': cannot lock references\n> >\n> > We tried increasing the timeout in the test to 2 minutes (up from 10s),\n> > but it didn't fix the failures.\n> \n> Thanks for a report, and please keep digging ;-).\n> \n> Is your build, like Peff's, for Windows, or your variant of Linux?\n> \n> Thanks.\n\nIt's our internal Debian-based variant of Linux. Sorry for not\nspecifying earlier.\n"},{"id":"503861","messageId":"20241001190823.GA2194387@coredump.intra.peff.net","threadId":"62129","inReplyTo":"joxzwkbi4uqyb6d67e24ivfvb54ev5ciqgvuxgxh4nntonjwqa@az3xgiiqg4x7","subject":"Re: [PATCH v4 3/3] refs/reftable: reload locked stack when preparing transaction","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-01T19:08:23Z","receivedAt":"2024-10-01T19:08:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 01, 2024 at 11:53:54AM -0700, Josh Steadmon wrote:\n\n> > > We're seeing repeated failures from this test case with ASan enabled.\n> > > Unfortunately, we've only been able to reproduce this on our\n> > > $DAYJOB-specific build system. I haven't been able to get it to fail\n> > > using just the upstream Makefile so far. I'll keep trying to find a way\n> > > to reproduce this.\n> > >\n> > > FWIW, we're not getting I/O errors, we see the following:\n> > > fatal: update_ref failed for ref 'refs/heads/branch-20': cannot lock references\n> > >\n> > > We tried increasing the timeout in the test to 2 minutes (up from 10s),\n> > > but it didn't fix the failures.\n> > \n> > Thanks for a report, and please keep digging ;-).\n> > \n> > Is your build, like Peff's, for Windows, or your variant of Linux?\n> > \n> > Thanks.\n> \n> It's our internal Debian-based variant of Linux. Sorry for not\n> specifying earlier.\n\nI just tried doing (on 'next', since the new test is added there):\n\n make SANITIZE=address,undefined CFLAGS=-O0\n cd t\n ./t0610-reftable-basics.sh --run=1-47 --stress\n\nand got lots of failures like you describe. But in my case bumping the\ntimeout from 10s to 100s made that go away. So I think it was just the\nsanitizers making things _really_ slow and causing legit timeouts.\n\nHave you tried going beyond 2 minutes? That seems like a crazy amount of\ntime, but I guess it's possible on a really overloaded system. Of course\nit's also quite possible that there are multiple issues and you are\nseeing something else entirely. :)\n\n-Peff\n"},{"id":"503875","messageId":"20241001225425.GB2317071@coredump.intra.peff.net","threadId":"62129","inReplyTo":"Zvt6LxWm8gtJGw9S@pks.im","subject":"Re: [PATCH v4 3/3] refs/reftable: reload locked stack when preparing transaction","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-01T22:54:25Z","receivedAt":"2024-10-01T22:54:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 01, 2024 at 06:27:33AM +0200, Patrick Steinhardt wrote:\n\n> If this is causing problems for folks I'd say we can do the below change\n> for now. It's of course only a stop-gap solution until I find the time\n> to debug this, which should be later this week or early next week.\n> \n> Patrick\n> \n> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n> index 2d951c8ceb..ad7bb39b79 100755\n> --- a/t/t0610-reftable-basics.sh\n> +++ b/t/t0610-reftable-basics.sh\n> @@ -450,7 +450,7 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n> \t)\n> '\n> \n> -test_expect_success 'ref transaction: many concurrent writers' '\n> +test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '\n> \ttest_when_finished \"rm -rf repo\" &&\n> \tgit init repo &&\n> \t(\n\nIMHO we can live with a flaky test for a little while. It's not like\nit's the only one. ;) And hopefully your digging turns up a real\nsolution.\n\nIt also sounds from subsequent discussion that Josh's issue was on\nLinux, so it wouldn't help there.\n\n-Peff\n"},{"id":"503878","messageId":"xmqqttdvqud9.fsf@gitster.g","threadId":"62129","inReplyTo":"20241001225425.GB2317071@coredump.intra.peff.net","subject":"Re: [PATCH v4 3/3] refs/reftable: reload locked stack when preparing transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-01T23:24:50Z","receivedAt":"2024-10-01T23:24:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Oct 01, 2024 at 06:27:33AM +0200, Patrick Steinhardt wrote:\n>\n>> If this is causing problems for folks I'd say we can do the below change\n>> for now. It's of course only a stop-gap solution until I find the time\n>> to debug this, which should be later this week or early next week.\n>> \n>> Patrick\n>> \n>> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n>> index 2d951c8ceb..ad7bb39b79 100755\n>> --- a/t/t0610-reftable-basics.sh\n>> +++ b/t/t0610-reftable-basics.sh\n>> @@ -450,7 +450,7 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n>> \t)\n>> '\n>> \n>> -test_expect_success 'ref transaction: many concurrent writers' '\n>> +test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '\n>> \ttest_when_finished \"rm -rf repo\" &&\n>> \tgit init repo &&\n>> \t(\n>\n> IMHO we can live with a flaky test for a little while. It's not like\n> it's the only one. ;) And hopefully your digging turns up a real\n> solution.\n>\n> It also sounds from subsequent discussion that Josh's issue was on\n> Linux, so it wouldn't help there.\n\nThat's true. WINDOWS prereq would not help there, even though it\nwould hide the breakage from CI.\n\n\n\n"},{"id":"503927","messageId":"Zv0nTA2fAzy6FTY7@pks.im","threadId":"62129","inReplyTo":"xmqqttdvqud9.fsf@gitster.g","subject":"Re: [PATCH v4 3/3] refs/reftable: reload locked stack when preparing transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-02T10:58:26Z","receivedAt":"2024-10-02T10:58:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 01, 2024 at 04:24:50PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > On Tue, Oct 01, 2024 at 06:27:33AM +0200, Patrick Steinhardt wrote:\n> >\n> >> If this is causing problems for folks I'd say we can do the below change\n> >> for now. It's of course only a stop-gap solution until I find the time\n> >> to debug this, which should be later this week or early next week.\n> >> \n> >> Patrick\n> >> \n> >> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n> >> index 2d951c8ceb..ad7bb39b79 100755\n> >> --- a/t/t0610-reftable-basics.sh\n> >> +++ b/t/t0610-reftable-basics.sh\n> >> @@ -450,7 +450,7 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n> >> \t)\n> >> '\n> >> \n> >> -test_expect_success 'ref transaction: many concurrent writers' '\n> >> +test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '\n> >> \ttest_when_finished \"rm -rf repo\" &&\n> >> \tgit init repo &&\n> >> \t(\n> >\n> > IMHO we can live with a flaky test for a little while. It's not like\n> > it's the only one. ;) And hopefully your digging turns up a real\n> > solution.\n> >\n> > It also sounds from subsequent discussion that Josh's issue was on\n> > Linux, so it wouldn't help there.\n> \n> That's true. WINDOWS prereq would not help there, even though it\n> would hide the breakage from CI.\n\nOkay, fair enough, thanks. I should finally get to this issue either on\nFriday or early next week.\n\nPatrick\n"}]}