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@xxxxxxxxx>
---
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