[PATCH v2 3/4] reftable/stack: move list lock to `struct reftable_stack`

[Karthik Nayak <karthik.188@gmail.com>]

The struct `reftable_addition` is used to modify a given stack, as such,
it also includes a `struct reftable_flock` used to obtain the lock to
the list file. While the scope of the field lies within this struct, it
doesn't allow for optimizations to be made on `struct reftable_stack`
itself.

Move the field to `struct reftable_stack`, allowing us to make a simple
optimization around avoiding a stack reload when we have already
obtained a lock. While this is currently possible in the write path, the
write path also contains multiple branches to reads which only work
on top of `struct reftable_stack`, and we would miss the optimization in
such paths.

Since the lock is now shared across all additions on the same stack, a
second `reftable_addition` that fails to acquire the already held lock
would still call `reftable_addition_close()`, which will release the
`stack->list_lock` which is still held by the first addition. To avoid
this, add a new bit field `locked` to `reftable_addition` that tracks
whether a particular addition is the one holding the lock, and only
release it in that case. Add a unit test to validate this behavior.

While here, remove an unused header file from 'reftable/stack.h'.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 reftable/stack.c                | 26 +++++++++++++++++++-------
 reftable/stack.h                |  7 ++++++-
 t/unit-tests/u-reftable-stack.c | 28 ++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index 703548417c..c3d4deff29 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -536,6 +536,8 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,
 		goto out;
 	}
 
+	p->list_lock = REFTABLE_FLOCK_INIT;
+
 	err = reftable_stack_reload_maybe_reuse(p, 1);
 	if (err < 0)
 		goto out;
@@ -628,10 +630,16 @@ int reftable_stack_reload(struct reftable_stack *st)
 }
 
 struct reftable_addition {
-	struct reftable_flock tables_list_lock;
 	struct reftable_stack *stack;
 	struct reftable_write_options opts;
 
+	/*
+	 * While the list lock is acquired on the stack, we need to distinguish
+	 * which 'reftable_addition' is responsible for the lock. This avoids
+	 * clearing the lock of another 'reftable_addition'.
+	 */
+	unsigned int locked : 1;
+
 	char **new_tables;
 	size_t new_tables_len, new_tables_cap;
 	uint64_t next_update_index;
@@ -653,7 +661,9 @@ static void reftable_addition_close(struct reftable_addition *add)
 	add->new_tables_len = 0;
 	add->new_tables_cap = 0;
 
-	flock_release(&add->tables_list_lock);
+	if (add->locked)
+		flock_release(&add->stack->list_lock);
+	add->locked = 0;
 	reftable_buf_release(&nm);
 }
 
@@ -669,13 +679,14 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 	if (opts)
 		add->opts = *opts;
 
-	err = flock_acquire(&add->tables_list_lock, st->list_file,
+	err = flock_acquire(&add->stack->list_lock, st->list_file,
 			    add->opts.lock_timeout_ms);
 	if (err < 0)
 		goto done;
+	add->locked = 1;
 
 	if (add->opts.default_permissions) {
-		if (chmod(add->tables_list_lock.path,
+		if (chmod(add->stack->list_lock.path,
 			  add->opts.default_permissions) < 0) {
 			err = REFTABLE_IO_ERROR;
 			goto done;
@@ -774,7 +785,7 @@ int reftable_addition_commit(struct reftable_addition *add)
 			goto done;
 	}
 
-	err = reftable_write_data(add->tables_list_lock.fd,
+	err = reftable_write_data(add->stack->list_lock.fd,
 				  table_list.buf, table_list.len);
 	reftable_buf_release(&table_list);
 	if (err < 0) {
@@ -782,17 +793,18 @@ int reftable_addition_commit(struct reftable_addition *add)
 		goto done;
 	}
 
-	err = fsync(add->tables_list_lock.fd);
+	err = fsync(add->stack->list_lock.fd);
 	if (err < 0) {
 		err = REFTABLE_IO_ERROR;
 		goto done;
 	}
 
-	err = flock_commit(&add->tables_list_lock);
+	err = flock_commit(&add->stack->list_lock);
 	if (err < 0) {
 		err = REFTABLE_IO_ERROR;
 		goto done;
 	}
+	add->locked = 0;
 
 	/* success, no more state to clean up. */
 	for (i = 0; i < add->new_tables_len; i++)
diff --git a/reftable/stack.h b/reftable/stack.h
index f7901e6c6f..52e07ad551 100644
--- a/reftable/stack.h
+++ b/reftable/stack.h
@@ -10,7 +10,6 @@
 #define STACK_H
 
 #include "system.h"
-#include "reftable-writer.h"
 #include "reftable-stack.h"
 
 struct reftable_stack {
@@ -18,6 +17,12 @@ struct reftable_stack {
 	char *list_file;
 	int list_fd;
 
+	/*
+	 * Set while an addition holds the stack locked. Used by
+	 * stack_uptodate() to skip reload checks while locked.
+	 */
+	struct reftable_flock list_lock;
+
 	char *reftable_dir;
 
 	struct reftable_stack_options opts;
diff --git a/t/unit-tests/u-reftable-stack.c b/t/unit-tests/u-reftable-stack.c
index 04927113c2..b6f1c6cc52 100644
--- a/t/unit-tests/u-reftable-stack.c
+++ b/t/unit-tests/u-reftable-stack.c
@@ -1310,3 +1310,31 @@ void test_reftable_stack__invalid_limit_updates(void)
 	reftable_stack_destroy(st);
 	clear_dir(dir);
 }
+
+void test_reftable_stack__two_additions(void)
+{
+	struct reftable_stack *st = NULL;
+	char *dir = get_tmp_dir(__LINE__);
+	struct reftable_addition *add1 = NULL;
+	struct reftable_addition *add2 = NULL;
+
+	struct reftable_ref_record ref = {
+		.refname = (char *) "HEAD",
+		.update_index = 1,
+		.value_type = REFTABLE_REF_SYMREF,
+		.value.symref = (char *) "master",
+	};
+
+	cl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0);
+
+	cl_assert_equal_i(reftable_stack_addition_new(&add1, st, NULL), 0);
+	cl_assert_equal_i(reftable_stack_addition_new(&add2, st, NULL), REFTABLE_LOCK_ERROR);
+
+	cl_assert_equal_i(reftable_addition_add(add1, write_test_ref, &ref), 0);
+
+	cl_assert_equal_i(reftable_addition_commit(add1), 0);
+
+	reftable_addition_destroy(add1);
+	reftable_stack_destroy(st);
+	clear_dir(dir);
+}

-- 
2.55.GIT