| Message ID | 9703246156152e1c8c92b4237c8dc3a9ef59901a.1722435214.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reftable: improvements and fixes for compaction | expand |
On 24/07/31 04:15PM, Patrick Steinhardt wrote: > When modifying "tables.list", we need to lock the list before updating > it to ensure that no concurrent writers modify the list at the same > point in time. While we do this via the `lock_file` subsystem when > compacting the stack, we manually handle the lock when adding a new > table to it. While not wrong, it is at least inconsistent. Indeed, much more consistent to use the lockfile API here. :) > > Refactor the code to consistenly lock "tables.list" via the `lock_file` s/consistenly/consistently/ > subsytem. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > reftable/stack.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/reftable/stack.c b/reftable/stack.c > index 9ca549294f..9cc91a262c 100644 > --- a/reftable/stack.c > +++ b/reftable/stack.c > @@ -567,7 +567,7 @@ static void format_name(struct strbuf *dest, uint64_t min, uint64_t max) > } > > struct reftable_addition { > - struct tempfile *lock_file; > + struct lock_file tables_list_lock; > struct reftable_stack *stack; > > char **new_tables; > @@ -581,13 +581,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add, > struct reftable_stack *st) > { > struct strbuf lock_file_name = STRBUF_INIT; > - int err = 0; > - add->stack = st; > + int err; > > - strbuf_addf(&lock_file_name, "%s.lock", st->list_file); > + add->stack = st; > > - add->lock_file = create_tempfile(lock_file_name.buf); > - if (!add->lock_file) { > + err = hold_lock_file_for_update(&add->tables_list_lock, st->list_file, > + LOCK_NO_DEREF); > + if (err < 0) { > if (errno == EEXIST) { > err = REFTABLE_LOCK_ERROR; > } else { > @@ -596,7 +596,8 @@ static int reftable_stack_init_addition(struct reftable_addition *add, > goto done; > } > if (st->opts.default_permissions) { > - if (chmod(add->lock_file->filename.buf, st->opts.default_permissions) < 0) { > + if (chmod(get_lock_file_path(&add->tables_list_lock), > + st->opts.default_permissions) < 0) { > err = REFTABLE_IO_ERROR; > goto done; > } > @@ -635,7 +636,7 @@ static void reftable_addition_close(struct reftable_addition *add) > add->new_tables_len = 0; > add->new_tables_cap = 0; > > - delete_tempfile(&add->lock_file); > + rollback_lock_file(&add->tables_list_lock); > strbuf_release(&nm); > } > > @@ -651,7 +652,7 @@ void reftable_addition_destroy(struct reftable_addition *add) > int reftable_addition_commit(struct reftable_addition *add) > { > struct strbuf table_list = STRBUF_INIT; > - int lock_file_fd = get_tempfile_fd(add->lock_file); > + int lock_file_fd = get_lock_file_fd(&add->tables_list_lock); > int err = 0; > size_t i; > > @@ -674,13 +675,14 @@ int reftable_addition_commit(struct reftable_addition *add) > goto done; > } > > - err = fsync_component(FSYNC_COMPONENT_REFERENCE, lock_file_fd); > + err = fsync_component(FSYNC_COMPONENT_REFERENCE, > + get_lock_file_fd(&add->tables_list_lock)); I might be missing something, but is there a reason we have to get the lock file fd again instead of just using `lock_file_fd`? > if (err < 0) { > err = REFTABLE_IO_ERROR; > goto done; > } > > - err = rename_tempfile(&add->lock_file, add->stack->list_file); > + err = commit_lock_file(&add->tables_list_lock); > if (err < 0) { > err = REFTABLE_IO_ERROR; > goto done; > -- > 2.46.0.dirty >
On Wed, Jul 31, 2024 at 06:02:41PM -0500, Justin Tobler wrote: > On 24/07/31 04:15PM, Patrick Steinhardt wrote: > > @@ -674,13 +675,14 @@ int reftable_addition_commit(struct reftable_addition *add) > > goto done; > > } > > > > - err = fsync_component(FSYNC_COMPONENT_REFERENCE, lock_file_fd); > > + err = fsync_component(FSYNC_COMPONENT_REFERENCE, > > + get_lock_file_fd(&add->tables_list_lock)); > > I might be missing something, but is there a reason we have to get the > lock file fd again instead of just using `lock_file_fd`? Not really. No idea what I was thinking here, will fix. Patrick
diff --git a/reftable/stack.c b/reftable/stack.c index 9ca549294f..9cc91a262c 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -567,7 +567,7 @@ static void format_name(struct strbuf *dest, uint64_t min, uint64_t max) } struct reftable_addition { - struct tempfile *lock_file; + struct lock_file tables_list_lock; struct reftable_stack *stack; char **new_tables; @@ -581,13 +581,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add, struct reftable_stack *st) { struct strbuf lock_file_name = STRBUF_INIT; - int err = 0; - add->stack = st; + int err; - strbuf_addf(&lock_file_name, "%s.lock", st->list_file); + add->stack = st; - add->lock_file = create_tempfile(lock_file_name.buf); - if (!add->lock_file) { + err = hold_lock_file_for_update(&add->tables_list_lock, st->list_file, + LOCK_NO_DEREF); + if (err < 0) { if (errno == EEXIST) { err = REFTABLE_LOCK_ERROR; } else { @@ -596,7 +596,8 @@ static int reftable_stack_init_addition(struct reftable_addition *add, goto done; } if (st->opts.default_permissions) { - if (chmod(add->lock_file->filename.buf, st->opts.default_permissions) < 0) { + if (chmod(get_lock_file_path(&add->tables_list_lock), + st->opts.default_permissions) < 0) { err = REFTABLE_IO_ERROR; goto done; } @@ -635,7 +636,7 @@ static void reftable_addition_close(struct reftable_addition *add) add->new_tables_len = 0; add->new_tables_cap = 0; - delete_tempfile(&add->lock_file); + rollback_lock_file(&add->tables_list_lock); strbuf_release(&nm); } @@ -651,7 +652,7 @@ void reftable_addition_destroy(struct reftable_addition *add) int reftable_addition_commit(struct reftable_addition *add) { struct strbuf table_list = STRBUF_INIT; - int lock_file_fd = get_tempfile_fd(add->lock_file); + int lock_file_fd = get_lock_file_fd(&add->tables_list_lock); int err = 0; size_t i; @@ -674,13 +675,14 @@ int reftable_addition_commit(struct reftable_addition *add) goto done; } - err = fsync_component(FSYNC_COMPONENT_REFERENCE, lock_file_fd); + err = fsync_component(FSYNC_COMPONENT_REFERENCE, + get_lock_file_fd(&add->tables_list_lock)); if (err < 0) { err = REFTABLE_IO_ERROR; goto done; } - err = rename_tempfile(&add->lock_file, add->stack->list_file); + err = commit_lock_file(&add->tables_list_lock); if (err < 0) { err = REFTABLE_IO_ERROR; goto done;
When modifying "tables.list", we need to lock the list before updating it to ensure that no concurrent writers modify the list at the same point in time. While we do this via the `lock_file` subsystem when compacting the stack, we manually handle the lock when adding a new table to it. While not wrong, it is at least inconsistent. Refactor the code to consistenly lock "tables.list" via the `lock_file` subsytem. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/stack.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)