{"thread":{"id":"61046","subject":"[PATCH 0/4] reftable/stack: register temporary files","startedAt":"2024-03-04T11:10:47Z","lastAt":"2024-03-07T21:06:13Z","messageCount":26,"participants":["Patrick Steinhardt","Justin Tobler","Junio C Hamano","Toon claes"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"489885","messageId":"cover.1709549619.git.ps@pks.im","threadId":"61046","inReplyTo":null,"subject":"[PATCH 0/4] reftable/stack: register temporary files","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T11:10:41Z","receivedAt":"2024-03-04T11:10:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nwhen writing to a reftable stack there are several kinds of temporary\nfiles that we have to write. For one this is the new table, second the\nnew \"tables.list.lock\" file. We do not register all of these with the\ntempfiles subsystem though, which can have the consequence that\ninterrupted writes may leave those files behind.\n\nThis patch series addresses those cases for three cases:\n\n  - Newly written tables.\n\n  - Newly compacted tables.\n\n  - Pending \"tables.list.lock\" during compaction.\n\nThe end result should be more robust regarding interruption and leave\nleft cruft behind.\n\nPatrick\n\nPatrick Steinhardt (4):\n  lockfile: report when rollback fails\n  reftable/stack: register new tables as tempfiles\n  reftable/stack: register lockfiles during compaction\n  reftable/stack: register compacted tables as tempfiles\n\n lockfile.h        |   4 +-\n reftable/stack.c  | 329 ++++++++++++++++++++++------------------------\n reftable/system.h |   2 +\n tempfile.c        |  21 +--\n tempfile.h        |   2 +-\n 5 files changed, 177 insertions(+), 181 deletions(-)\n\n-- \n2.44.0\n\n"},{"id":"489886","messageId":"1acaa9ca1a53e3d8e875fed7ce961d0b5426688f.1709549619.git.ps@pks.im","threadId":"61046","inReplyTo":"cover.1709549619.git.ps@pks.im","subject":"[PATCH 1/4] lockfile: report when rollback fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T11:10:45Z","receivedAt":"2024-03-04T11:10:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not report to the caller when rolling back a lockfile fails, which\nwill be needed by the reftable compaction logic in a subsequent commit.\nIt also cannot really report on all errors because the function calls\n`delete_tempfile()`, which doesn't return an error either.\n\nRefactor the code so that both `delete_tempfile()` and\n`rollback_lock_file()` return an error code.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n lockfile.h |  4 ++--\n tempfile.c | 21 +++++++++++++--------\n tempfile.h |  2 +-\n 3 files changed, 16 insertions(+), 11 deletions(-)\n\ndiff --git a/lockfile.h b/lockfile.h\nindex 90af4e66b2..4ed570d3f7 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -323,9 +323,9 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path)\n  * for a `lock_file` object that has already been committed or rolled\n  * back.\n  */\n-static inline void rollback_lock_file(struct lock_file *lk)\n+static inline int rollback_lock_file(struct lock_file *lk)\n {\n-\tdelete_tempfile(&lk->tempfile);\n+\treturn delete_tempfile(&lk->tempfile);\n }\n \n #endif /* LOCKFILE_H */\ndiff --git a/tempfile.c b/tempfile.c\nindex ecdebf1afb..ed88cf8431 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -50,15 +50,17 @@\n \n static VOLATILE_LIST_HEAD(tempfile_list);\n \n-static void remove_template_directory(struct tempfile *tempfile,\n+static int remove_template_directory(struct tempfile *tempfile,\n \t\t\t\t      int in_signal_handler)\n {\n \tif (tempfile->directory) {\n \t\tif (in_signal_handler)\n-\t\t\trmdir(tempfile->directory);\n+\t\t\treturn rmdir(tempfile->directory);\n \t\telse\n-\t\t\trmdir_or_warn(tempfile->directory);\n+\t\t\treturn rmdir_or_warn(tempfile->directory);\n \t}\n+\n+\treturn 0;\n }\n \n static void remove_tempfiles(int in_signal_handler)\n@@ -353,16 +355,19 @@ int rename_tempfile(struct tempfile **tempfile_p, const char *path)\n \treturn 0;\n }\n \n-void delete_tempfile(struct tempfile **tempfile_p)\n+int delete_tempfile(struct tempfile **tempfile_p)\n {\n \tstruct tempfile *tempfile = *tempfile_p;\n+\tint err = 0;\n \n \tif (!is_tempfile_active(tempfile))\n-\t\treturn;\n+\t\treturn 0;\n \n-\tclose_tempfile_gently(tempfile);\n-\tunlink_or_warn(tempfile->filename.buf);\n-\tremove_template_directory(tempfile, 0);\n+\terr |= close_tempfile_gently(tempfile);\n+\terr |= unlink_or_warn(tempfile->filename.buf);\n+\terr |= remove_template_directory(tempfile, 0);\n \tdeactivate_tempfile(tempfile);\n \t*tempfile_p = NULL;\n+\n+\treturn err ? -1 : 0;\n }\ndiff --git a/tempfile.h b/tempfile.h\nindex d0413af733..2d2ae5b657 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -269,7 +269,7 @@ int reopen_tempfile(struct tempfile *tempfile);\n  * `delete_tempfile()` for a `tempfile` object that has already been\n  * deleted or renamed.\n  */\n-void delete_tempfile(struct tempfile **tempfile_p);\n+int delete_tempfile(struct tempfile **tempfile_p);\n \n /*\n  * Close the file descriptor and/or file pointer if they are still\n-- \n2.44.0\n\n"},{"id":"489887","messageId":"02bf41d419efd00e510a89a405e1b046b166ba20.1709549619.git.ps@pks.im","threadId":"61046","inReplyTo":"cover.1709549619.git.ps@pks.im","subject":"[PATCH 2/4] reftable/stack: register new tables as tempfiles","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T11:10:51Z","receivedAt":"2024-03-04T11:10:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not register new tables which we're about to add to the stack with\nthe tempfile API. Those tables will thus not be deleted in case Git gets\nkilled.\n\nRefactor the code to register tables as tempfiles.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 29 ++++++++++++-----------------\n 1 file changed, 12 insertions(+), 17 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex b64e55648a..81544fbfa0 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -737,8 +737,9 @@ int reftable_addition_add(struct reftable_addition *add,\n \tstruct strbuf tab_file_name = STRBUF_INIT;\n \tstruct strbuf next_name = STRBUF_INIT;\n \tstruct reftable_writer *wr = NULL;\n+\tstruct tempfile *tab_file = NULL;\n \tint err = 0;\n-\tint tab_fd = 0;\n+\tint tab_fd;\n \n \tstrbuf_reset(&next_name);\n \tformat_name(&next_name, add->next_update_index, add->next_update_index);\n@@ -746,17 +747,20 @@ int reftable_addition_add(struct reftable_addition *add,\n \tstack_filename(&temp_tab_file_name, add->stack, next_name.buf);\n \tstrbuf_addstr(&temp_tab_file_name, \".temp.XXXXXX\");\n \n-\ttab_fd = mkstemp(temp_tab_file_name.buf);\n-\tif (tab_fd < 0) {\n+\ttab_file = mks_tempfile(temp_tab_file_name.buf);\n+\tif (!tab_file) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n \tif (add->stack->config.default_permissions) {\n-\t\tif (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {\n+\t\tif (chmod(get_tempfile_path(tab_file),\n+\t\t\t  add->stack->config.default_permissions)) {\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n \t\t}\n \t}\n+\ttab_fd = get_tempfile_fd(tab_file);\n+\n \twr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd,\n \t\t\t\t &add->stack->config);\n \terr = write_table(wr, arg);\n@@ -771,14 +775,13 @@ int reftable_addition_add(struct reftable_addition *add,\n \tif (err < 0)\n \t\tgoto done;\n \n-\terr = close(tab_fd);\n-\ttab_fd = 0;\n+\terr = close_tempfile_gently(tab_file);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n \n-\terr = stack_check_addition(add->stack, temp_tab_file_name.buf);\n+\terr = stack_check_addition(add->stack, get_tempfile_path(tab_file));\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -789,14 +792,13 @@ int reftable_addition_add(struct reftable_addition *add,\n \n \tformat_name(&next_name, wr->min_update_index, wr->max_update_index);\n \tstrbuf_addstr(&next_name, \".ref\");\n-\n \tstack_filename(&tab_file_name, add->stack, next_name.buf);\n \n \t/*\n \t  On windows, this relies on rand() picking a unique destination name.\n \t  Maybe we should do retry loop as well?\n \t */\n-\terr = rename(temp_tab_file_name.buf, tab_file_name.buf);\n+\terr = rename_tempfile(&tab_file, tab_file_name.buf);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n@@ -806,14 +808,7 @@ int reftable_addition_add(struct reftable_addition *add,\n \t\t\t    add->new_tables_cap);\n \tadd->new_tables[add->new_tables_len++] = strbuf_detach(&next_name, NULL);\n done:\n-\tif (tab_fd > 0) {\n-\t\tclose(tab_fd);\n-\t\ttab_fd = 0;\n-\t}\n-\tif (temp_tab_file_name.len > 0) {\n-\t\tunlink(temp_tab_file_name.buf);\n-\t}\n-\n+\tdelete_tempfile(&tab_file);\n \tstrbuf_release(&temp_tab_file_name);\n \tstrbuf_release(&tab_file_name);\n \tstrbuf_release(&next_name);\n-- \n2.44.0\n\n"},{"id":"489888","messageId":"45b5c3167fbfd64d8d1e14ed55bae94cb9cba28b.1709549619.git.ps@pks.im","threadId":"61046","inReplyTo":"cover.1709549619.git.ps@pks.im","subject":"[PATCH 3/4] reftable/stack: register lockfiles during compaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T11:10:55Z","receivedAt":"2024-03-04T11:10:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not register any of the locks we acquire when compacting the\nreftable stack via our lockfiles interfaces. These locks will thus not\nbe released when Git gets killed.\n\nRefactor the code to register locks as lockfiles.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c  | 255 ++++++++++++++++++++++------------------------\n reftable/system.h |   2 +\n 2 files changed, 123 insertions(+), 134 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 81544fbfa0..977336b7d5 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -978,212 +978,199 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t       size_t first, size_t last,\n \t\t\t       struct reftable_log_expiry_config *expiry)\n {\n-\tchar **delete_on_success = NULL, **subtable_locks = NULL, **listp = NULL;\n-\tstruct strbuf temp_tab_file_name = STRBUF_INIT;\n+\tstruct strbuf tables_list_buf = STRBUF_INIT;\n+\tstruct strbuf new_table_temp_path = STRBUF_INIT;\n \tstruct strbuf new_table_name = STRBUF_INIT;\n-\tstruct strbuf lock_file_name = STRBUF_INIT;\n-\tstruct strbuf ref_list_contents = STRBUF_INIT;\n \tstruct strbuf new_table_path = STRBUF_INIT;\n-\tsize_t i, j, compact_count;\n-\tint err = 0;\n-\tint have_lock = 0;\n-\tint lock_file_fd = -1;\n-\tint is_empty_table = 0;\n+\tstruct strbuf table_name = STRBUF_INIT;\n+\tstruct lock_file tables_list_lock = LOCK_INIT;\n+\tstruct lock_file *table_locks = NULL;\n+\tint is_empty_table = 0, err = 0;\n+\tsize_t i;\n \n \tif (first > last || (!expiry && first == last)) {\n \t\terr = 0;\n \t\tgoto done;\n \t}\n \n-\tcompact_count = last - first + 1;\n-\tREFTABLE_CALLOC_ARRAY(delete_on_success, compact_count + 1);\n-\tREFTABLE_CALLOC_ARRAY(subtable_locks, compact_count + 1);\n-\n \tst->stats.attempts++;\n \n-\tstrbuf_reset(&lock_file_name);\n-\tstrbuf_addstr(&lock_file_name, st->list_file);\n-\tstrbuf_addstr(&lock_file_name, \".lock\");\n-\n-\tlock_file_fd =\n-\t\topen(lock_file_name.buf, O_EXCL | O_CREAT | O_WRONLY, 0666);\n-\tif (lock_file_fd < 0) {\n-\t\tif (errno == EEXIST) {\n+\t/*\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+\tif (err < 0) {\n+\t\tif (errno == EEXIST)\n \t\t\terr = 1;\n-\t\t} else {\n+\t\telse\n \t\t\terr = REFTABLE_IO_ERROR;\n-\t\t}\n \t\tgoto done;\n \t}\n-\t/* Don't want to write to the lock for now.  */\n-\tclose(lock_file_fd);\n-\tlock_file_fd = -1;\n \n-\thave_lock = 1;\n \terr = stack_uptodate(st);\n-\tif (err != 0)\n+\tif (err)\n \t\tgoto done;\n \n-\tfor (i = first, j = 0; i <= last; i++) {\n-\t\tstruct strbuf subtab_file_name = STRBUF_INIT;\n-\t\tstruct strbuf subtab_lock = STRBUF_INIT;\n-\t\tint sublock_file_fd = -1;\n-\n-\t\tstack_filename(&subtab_file_name, st,\n-\t\t\t       reader_name(st->readers[i]));\n-\n-\t\tstrbuf_reset(&subtab_lock);\n-\t\tstrbuf_addbuf(&subtab_lock, &subtab_file_name);\n-\t\tstrbuf_addstr(&subtab_lock, \".lock\");\n+\t/*\n+\t * Lock all tables in the user-provided range. This is the slice of our\n+\t * stack which we'll compact.\n+\t */\n+\tREFTABLE_CALLOC_ARRAY(table_locks, last - first + 1);\n+\tfor (i = first; i <= last; i++) {\n+\t\tstack_filename(&table_name, st, reader_name(st->readers[i]));\n \n-\t\tsublock_file_fd = open(subtab_lock.buf,\n-\t\t\t\t       O_EXCL | O_CREAT | O_WRONLY, 0666);\n-\t\tif (sublock_file_fd >= 0) {\n-\t\t\tclose(sublock_file_fd);\n-\t\t} else if (sublock_file_fd < 0) {\n-\t\t\tif (errno == EEXIST) {\n+\t\terr = hold_lock_file_for_update(&table_locks[i - first],\n+\t\t\t\t\t\ttable_name.buf, LOCK_NO_DEREF);\n+\t\tif (err < 0) {\n+\t\t\tif (errno == EEXIST)\n \t\t\t\terr = 1;\n-\t\t\t} else {\n+\t\t\telse\n \t\t\t\terr = REFTABLE_IO_ERROR;\n-\t\t\t}\n+\t\t\tgoto done;\n \t\t}\n \n-\t\tsubtable_locks[j] = subtab_lock.buf;\n-\t\tdelete_on_success[j] = subtab_file_name.buf;\n-\t\tj++;\n-\n-\t\tif (err != 0)\n+\t\t/*\n+\t\t * We need to close the lockfiles as we might otherwise easily\n+\t\t * run into file descriptor exhaustion when we compress a lot\n+\t\t * of tables.\n+\t\t */\n+\t\terr = close_lock_file_gently(&table_locks[i - first]);\n+\t\tif (err < 0) {\n+\t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n+\t\t}\n \t}\n \n-\terr = unlink(lock_file_name.buf);\n-\tif (err < 0)\n+\t/*\n+\t * We have locked all tables in our range and can thus release the\n+\t * \"tables.list\" lock while compacting the locked tables. This allows\n+\t * concurrent updates to the stack to proceed.\n+\t */\n+\terr = rollback_lock_file(&tables_list_lock);\n+\tif (err < 0) {\n+\t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n-\thave_lock = 0;\n-\n-\terr = stack_compact_locked(st, first, last, &temp_tab_file_name,\n-\t\t\t\t   expiry);\n-\t/* Compaction + tombstones can create an empty table out of non-empty\n-\t * tables. */\n-\tis_empty_table = (err == REFTABLE_EMPTY_TABLE_ERROR);\n-\tif (is_empty_table) {\n-\t\terr = 0;\n \t}\n-\tif (err < 0)\n-\t\tgoto done;\n \n-\tlock_file_fd =\n-\t\topen(lock_file_name.buf, O_EXCL | O_CREAT | O_WRONLY, 0666);\n-\tif (lock_file_fd < 0) {\n-\t\tif (errno == EEXIST) {\n+\t/*\n+\t * Compact the now-locked tables into a new table. Note that compacting\n+\t * these tables may end up with an empty new table in case tombstones\n+\t * end up cancelling out all refs in that range.\n+\t */\n+\terr = stack_compact_locked(st, first, last, &new_table_temp_path, expiry);\n+\tif (err < 0) {\n+\t\tif (err != REFTABLE_EMPTY_TABLE_ERROR)\n+\t\t\tgoto done;\n+\t\tis_empty_table = 1;\n+\t}\n+\n+\t/*\n+\t * Now that we have written the new, compacted table we need to re-lock\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+\tif (err < 0) {\n+\t\tif (errno == EEXIST)\n \t\t\terr = 1;\n-\t\t} else {\n+\t\telse\n \t\t\terr = REFTABLE_IO_ERROR;\n-\t\t}\n \t\tgoto done;\n \t}\n-\thave_lock = 1;\n+\n \tif (st->config.default_permissions) {\n-\t\tif (chmod(lock_file_name.buf, st->config.default_permissions) < 0) {\n+\t\tif (chmod(get_lock_file_path(&tables_list_lock),\n+\t\t\t  st->config.default_permissions) < 0) {\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n \t\t}\n \t}\n \n-\tformat_name(&new_table_name, st->readers[first]->min_update_index,\n-\t\t    st->readers[last]->max_update_index);\n-\tstrbuf_addstr(&new_table_name, \".ref\");\n-\n-\tstack_filename(&new_table_path, st, new_table_name.buf);\n-\n+\t/*\n+\t * If the resulting compacted table is not empty, then we need to move\n+\t * it into place now.\n+\t */\n \tif (!is_empty_table) {\n-\t\t/* retry? */\n-\t\terr = rename(temp_tab_file_name.buf, new_table_path.buf);\n+\t\tformat_name(&new_table_name, st->readers[first]->min_update_index,\n+\t\t\t    st->readers[last]->max_update_index);\n+\t\tstrbuf_addstr(&new_table_name, \".ref\");\n+\t\tstack_filename(&new_table_path, st, new_table_name.buf);\n+\n+\t\terr = rename(new_table_temp_path.buf, new_table_path.buf);\n \t\tif (err < 0) {\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n \t\t}\n \t}\n \n-\tfor (i = 0; i < first; i++) {\n-\t\tstrbuf_addstr(&ref_list_contents, st->readers[i]->name);\n-\t\tstrbuf_addstr(&ref_list_contents, \"\\n\");\n-\t}\n-\tif (!is_empty_table) {\n-\t\tstrbuf_addbuf(&ref_list_contents, &new_table_name);\n-\t\tstrbuf_addstr(&ref_list_contents, \"\\n\");\n-\t}\n-\tfor (i = last + 1; i < st->merged->stack_len; i++) {\n-\t\tstrbuf_addstr(&ref_list_contents, st->readers[i]->name);\n-\t\tstrbuf_addstr(&ref_list_contents, \"\\n\");\n-\t}\n-\n-\terr = write_in_full(lock_file_fd, ref_list_contents.buf, ref_list_contents.len);\n-\tif (err < 0) {\n-\t\terr = REFTABLE_IO_ERROR;\n-\t\tunlink(new_table_path.buf);\n-\t\tgoto done;\n-\t}\n-\n-\terr = fsync_component(FSYNC_COMPONENT_REFERENCE, lock_file_fd);\n+\t/*\n+\t * Write the new \"tables.list\" contents with the compacted table we\n+\t * have just written. In case the compacted table became empty we\n+\t * simply skip writing it.\n+\t */\n+\tfor (i = 0; i < first; i++)\n+\t\tstrbuf_addf(&tables_list_buf, \"%s\\n\", st->readers[i]->name);\n+\tif (!is_empty_table)\n+\t\tstrbuf_addf(&tables_list_buf, \"%s\\n\", new_table_name.buf);\n+\tfor (i = last + 1; i < st->merged->stack_len; i++)\n+\t\tstrbuf_addf(&tables_list_buf, \"%s\\n\", st->readers[i]->name);\n+\n+\terr = write_in_full(get_lock_file_fd(&tables_list_lock),\n+\t\t\t    tables_list_buf.buf, tables_list_buf.len);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tunlink(new_table_path.buf);\n \t\tgoto done;\n \t}\n \n-\terr = close(lock_file_fd);\n-\tlock_file_fd = -1;\n+\terr = fsync_component(FSYNC_COMPONENT_REFERENCE, get_lock_file_fd(&tables_list_lock));\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tunlink(new_table_path.buf);\n \t\tgoto done;\n \t}\n \n-\terr = rename(lock_file_name.buf, st->list_file);\n+\terr = commit_lock_file(&tables_list_lock);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tunlink(new_table_path.buf);\n \t\tgoto done;\n \t}\n-\thave_lock = 0;\n \n-\t/* Reload the stack before deleting. On windows, we can only delete the\n-\t   files after we closed them.\n-\t*/\n+\t/*\n+\t * Reload the stack before deleting the compacted tables. We can only\n+\t * delete the files after we closed them on Windows, so this needs to\n+\t * happen first.\n+\t */\n \terr = reftable_stack_reload_maybe_reuse(st, first < last);\n+\tif (err < 0)\n+\t\tgoto done;\n \n-\tlistp = delete_on_success;\n-\twhile (*listp) {\n-\t\tif (strcmp(*listp, new_table_path.buf)) {\n-\t\t\tunlink(*listp);\n-\t\t}\n-\t\tlistp++;\n+\t/*\n+\t * Delete the old tables. They may still be in use by concurrent\n+\t * readers, so it is expected that unlinking tables may fail.\n+\t */\n+\tfor (i = first; i <= last; i++) {\n+\t\tstruct lock_file *table_lock = &table_locks[i - first];\n+\t\tchar *table_path = get_locked_file_path(table_lock);\n+\t\tunlink(table_path);\n+\t\tfree(table_path);\n \t}\n \n done:\n-\tfree_names(delete_on_success);\n+\trollback_lock_file(&tables_list_lock);\n+\tfor (i = first; table_locks && i <= last; i++)\n+\t\trollback_lock_file(&table_locks[i - first]);\n+\treftable_free(table_locks);\n \n-\tif (subtable_locks) {\n-\t\tlistp = subtable_locks;\n-\t\twhile (*listp) {\n-\t\t\tunlink(*listp);\n-\t\t\tlistp++;\n-\t\t}\n-\t\tfree_names(subtable_locks);\n-\t}\n-\tif (lock_file_fd >= 0) {\n-\t\tclose(lock_file_fd);\n-\t\tlock_file_fd = -1;\n-\t}\n-\tif (have_lock) {\n-\t\tunlink(lock_file_name.buf);\n-\t}\n \tstrbuf_release(&new_table_name);\n \tstrbuf_release(&new_table_path);\n-\tstrbuf_release(&ref_list_contents);\n-\tstrbuf_release(&temp_tab_file_name);\n-\tstrbuf_release(&lock_file_name);\n+\tstrbuf_release(&new_table_temp_path);\n+\tstrbuf_release(&tables_list_buf);\n+\tstrbuf_release(&table_name);\n \treturn err;\n }\n \ndiff --git a/reftable/system.h b/reftable/system.h\nindex 6b74a81514..5d8b6dede5 100644\n--- a/reftable/system.h\n+++ b/reftable/system.h\n@@ -12,7 +12,9 @@ license that can be found in the LICENSE file or at\n /* This header glues the reftable library to the rest of Git */\n \n #include \"git-compat-util.h\"\n+#include \"lockfile.h\"\n #include \"strbuf.h\"\n+#include \"tempfile.h\"\n #include \"hash-ll.h\" /* hash ID, sizes.*/\n #include \"dir.h\" /* remove_dir_recursively, for tests.*/\n \n-- \n2.44.0\n\n"},{"id":"489889","messageId":"b952d54a05e1c0cf47371f78e3901cfb2119e246.1709549619.git.ps@pks.im","threadId":"61046","inReplyTo":"cover.1709549619.git.ps@pks.im","subject":"[PATCH 4/4] reftable/stack: register compacted tables as tempfiles","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T11:10:59Z","receivedAt":"2024-03-04T11:11:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not register tables resulting from stack compaction with the\ntempfile API. Those tables will thus not be deleted in case Git gets\nkilled.\n\nRefactor the code to register compacted tables as tempfiles.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 53 +++++++++++++++++++++++++++---------------------\n 1 file changed, 30 insertions(+), 23 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 977336b7d5..40129da16c 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -827,51 +827,57 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)\n \n static int stack_compact_locked(struct reftable_stack *st,\n \t\t\t\tsize_t first, size_t last,\n-\t\t\t\tstruct strbuf *temp_tab,\n-\t\t\t\tstruct reftable_log_expiry_config *config)\n+\t\t\t\tstruct reftable_log_expiry_config *config,\n+\t\t\t\tstruct tempfile **temp_table_out)\n {\n \tstruct strbuf next_name = STRBUF_INIT;\n-\tint tab_fd = -1;\n+\tstruct strbuf table_path = STRBUF_INIT;\n \tstruct reftable_writer *wr = NULL;\n+\tstruct tempfile *temp_table;\n+\tint temp_table_fd;\n \tint err = 0;\n \n \tformat_name(&next_name,\n \t\t    reftable_reader_min_update_index(st->readers[first]),\n \t\t    reftable_reader_max_update_index(st->readers[last]));\n+\tstack_filename(&table_path, st, next_name.buf);\n+\tstrbuf_addstr(&table_path, \".temp.XXXXXX\");\n \n-\tstack_filename(temp_tab, st, next_name.buf);\n-\tstrbuf_addstr(temp_tab, \".temp.XXXXXX\");\n+\ttemp_table = mks_tempfile(table_path.buf);\n+\tif (!temp_table) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n+\t}\n+\ttemp_table_fd = get_tempfile_fd(temp_table);\n \n-\ttab_fd = mkstemp(temp_tab->buf);\n \tif (st->config.default_permissions &&\n-\t    chmod(temp_tab->buf, st->config.default_permissions) < 0) {\n+\t    chmod(get_tempfile_path(temp_table), st->config.default_permissions) < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n \n-\twr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd, &st->config);\n-\n+\twr = reftable_new_writer(reftable_fd_write, reftable_fd_flush,\n+\t\t\t\t &temp_table_fd, &st->config);\n \terr = stack_write_compact(st, wr, first, last, config);\n \tif (err < 0)\n \t\tgoto done;\n+\n \terr = reftable_writer_close(wr);\n \tif (err < 0)\n \t\tgoto done;\n \n-\terr = close(tab_fd);\n-\ttab_fd = 0;\n+\terr = close_tempfile_gently(temp_table);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\t*temp_table_out = temp_table;\n+\ttemp_table = NULL;\n \n done:\n+\tdelete_tempfile(&temp_table);\n \treftable_writer_free(wr);\n-\tif (tab_fd > 0) {\n-\t\tclose(tab_fd);\n-\t\ttab_fd = 0;\n-\t}\n-\tif (err != 0 && temp_tab->len > 0) {\n-\t\tunlink(temp_tab->buf);\n-\t\tstrbuf_release(temp_tab);\n-\t}\n \tstrbuf_release(&next_name);\n+\tstrbuf_release(&table_path);\n \treturn err;\n }\n \n@@ -979,12 +985,12 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t       struct reftable_log_expiry_config *expiry)\n {\n \tstruct strbuf tables_list_buf = STRBUF_INIT;\n-\tstruct strbuf new_table_temp_path = STRBUF_INIT;\n \tstruct strbuf new_table_name = STRBUF_INIT;\n \tstruct strbuf new_table_path = STRBUF_INIT;\n \tstruct strbuf table_name = STRBUF_INIT;\n \tstruct lock_file tables_list_lock = LOCK_INIT;\n \tstruct lock_file *table_locks = NULL;\n+\tstruct tempfile *new_table = NULL;\n \tint is_empty_table = 0, err = 0;\n \tsize_t i;\n \n@@ -1059,7 +1065,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * these tables may end up with an empty new table in case tombstones\n \t * end up cancelling out all refs in that range.\n \t */\n-\terr = stack_compact_locked(st, first, last, &new_table_temp_path, expiry);\n+\terr = stack_compact_locked(st, first, last, expiry, &new_table);\n \tif (err < 0) {\n \t\tif (err != REFTABLE_EMPTY_TABLE_ERROR)\n \t\t\tgoto done;\n@@ -1099,7 +1105,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\tstrbuf_addstr(&new_table_name, \".ref\");\n \t\tstack_filename(&new_table_path, st, new_table_name.buf);\n \n-\t\terr = rename(new_table_temp_path.buf, new_table_path.buf);\n+\t\terr = rename_tempfile(&new_table, new_table_path.buf);\n \t\tif (err < 0) {\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n@@ -1166,9 +1172,10 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\trollback_lock_file(&table_locks[i - first]);\n \treftable_free(table_locks);\n \n+\tdelete_tempfile(&new_table);\n \tstrbuf_release(&new_table_name);\n \tstrbuf_release(&new_table_path);\n-\tstrbuf_release(&new_table_temp_path);\n+\n \tstrbuf_release(&tables_list_buf);\n \tstrbuf_release(&table_name);\n \treturn err;\n-- \n2.44.0\n\n"},{"id":"490025","messageId":"lyarkcrkcb7ww542lc6y7t534yxnpahvrwephottvcbhr5dkn2@wwd5wk5bzlw7","threadId":"61046","inReplyTo":"1acaa9ca1a53e3d8e875fed7ce961d0b5426688f.1709549619.git.ps@pks.im","subject":"Re: [PATCH 1/4] lockfile: report when rollback fails","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-03-05T22:09:07Z","receivedAt":"2024-03-05T22:09:51Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/03/04 12:10PM, Patrick Steinhardt wrote:\n> We do not report to the caller when rolling back a lockfile fails, which\n> will be needed by the reftable compaction logic in a subsequent commit.\n> It also cannot really report on all errors because the function calls\n> `delete_tempfile()`, which doesn't return an error either.\n> \n> Refactor the code so that both `delete_tempfile()` and\n> `rollback_lock_file()` return an error code.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  lockfile.h |  4 ++--\n>  tempfile.c | 21 +++++++++++++--------\n>  tempfile.h |  2 +-\n>  3 files changed, 16 insertions(+), 11 deletions(-)\n> \n> diff --git a/lockfile.h b/lockfile.h\n> index 90af4e66b2..4ed570d3f7 100644\n> --- a/lockfile.h\n> +++ b/lockfile.h\n> @@ -323,9 +323,9 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path)\n>   * for a `lock_file` object that has already been committed or rolled\n>   * back.\n>   */\n> -static inline void rollback_lock_file(struct lock_file *lk)\n> +static inline int rollback_lock_file(struct lock_file *lk)\n>  {\n> -\tdelete_tempfile(&lk->tempfile);\n> +\treturn delete_tempfile(&lk->tempfile);\n>  }\n\nquestion: For a lockfile that is already committed or rolled back, is\nthe underlying tempfile still active? I'm trying to figure out if it\npossible for an error to be returned in this scenerio. If not, it might\nbe nice to clarify in the comment above that, in addition to being a\nNOOP, no error is returned.\n\n-Justin\n"},{"id":"490031","messageId":"6cw6d3ubo2kbogzdbniyoznij2zfoh5t3htwb4oaghaltcgeqg@kkrw4g6atr2k","threadId":"61046","inReplyTo":"02bf41d419efd00e510a89a405e1b046b166ba20.1709549619.git.ps@pks.im","subject":"Re: [PATCH 2/4] reftable/stack: register new tables as tempfiles","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-03-05T22:30:18Z","receivedAt":"2024-03-05T22:31:02Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/03/04 12:10PM, Patrick Steinhardt wrote:\n> We do not register new tables which we're about to add to the stack with\n> the tempfile API. Those tables will thus not be deleted in case Git gets\n> killed.\n> \n> Refactor the code to register tables as tempfiles.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  reftable/stack.c | 29 ++++++++++++-----------------\n>  1 file changed, 12 insertions(+), 17 deletions(-)\n> \n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index b64e55648a..81544fbfa0 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -737,8 +737,9 @@ int reftable_addition_add(struct reftable_addition *add,\n>  \tstruct strbuf tab_file_name = STRBUF_INIT;\n>  \tstruct strbuf next_name = STRBUF_INIT;\n>  \tstruct reftable_writer *wr = NULL;\n> +\tstruct tempfile *tab_file = NULL;\n>  \tint err = 0;\n> -\tint tab_fd = 0;\n> +\tint tab_fd;\n>  \n>  \tstrbuf_reset(&next_name);\n>  \tformat_name(&next_name, add->next_update_index, add->next_update_index);\n> @@ -746,17 +747,20 @@ int reftable_addition_add(struct reftable_addition *add,\n>  \tstack_filename(&temp_tab_file_name, add->stack, next_name.buf);\n>  \tstrbuf_addstr(&temp_tab_file_name, \".temp.XXXXXX\");\n>  \n> -\ttab_fd = mkstemp(temp_tab_file_name.buf);\n> -\tif (tab_fd < 0) {\n> +\ttab_file = mks_tempfile(temp_tab_file_name.buf);\n> +\tif (!tab_file) {\n>  \t\terr = REFTABLE_IO_ERROR;\n>  \t\tgoto done;\n>  \t}\n>  \tif (add->stack->config.default_permissions) {\n> -\t\tif (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {\n> +\t\tif (chmod(get_tempfile_path(tab_file),\n> +\t\t\t  add->stack->config.default_permissions)) {\n>  \t\t\terr = REFTABLE_IO_ERROR;\n>  \t\t\tgoto done;\n>  \t\t}\n>  \t}\n\nSince the tempfile is now being created through the tempfile API, I\nthink the file mode can be set directly through `mks_tempfile_m()`\ninstead of creating the tempfile and then using chmod. Just something I\nthought to mention.\n\n> +\ttab_fd = get_tempfile_fd(tab_file);\n\n-Justin\n"},{"id":"490037","messageId":"mwhby7dxpiyrvknqe2uoli4ulygjy6hbxcxpqt3alw3dthwntr@4o24tp5jp6h7","threadId":"61046","inReplyTo":"45b5c3167fbfd64d8d1e14ed55bae94cb9cba28b.1709549619.git.ps@pks.im","subject":"Re: [PATCH 3/4] reftable/stack: register lockfiles during compaction","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-03-05T23:30:48Z","receivedAt":"2024-03-05T23:31:32Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/03/04 12:10PM, Patrick Steinhardt wrote:\n> We do not register any of the locks we acquire when compacting the\n> reftable stack via our lockfiles interfaces. These locks will thus not\n> be released when Git gets killed.\n> \n> Refactor the code to register locks as lockfiles.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n> ...\n> +\t/*\n> +\t * Write the new \"tables.list\" contents with the compacted table we\n> +\t * have just written. In case the compacted table became empty we\n> +\t * simply skip writing it.\n> +\t */\n> +\tfor (i = 0; i < first; i++)\n> +\t\tstrbuf_addf(&tables_list_buf, \"%s\\n\", st->readers[i]->name);\n> +\tif (!is_empty_table)\n> +\t\tstrbuf_addf(&tables_list_buf, \"%s\\n\", new_table_name.buf);\n\nSomething not really related to this patch, but I noticed and had a\nquestion about.\n\nIf I'm understanding this correctly, when a newly compacted table is\nempty, it becomes possible for a range of indexes to no longer exist \nwithin the stack. If this occurs in the middle of the stack, future\ncompaction will likely combine the tables on either side and restore the\nmissing index range. If the empty table was at the end of the stack,\nwould this effectly reset the max index to something lower for future\ntables written to the stack? If so, could this lead to issues with\nseparate concurrent table writes?\n\n> ...  \n> diff --git a/reftable/system.h b/reftable/system.h\n> index 6b74a81514..5d8b6dede5 100644\n> --- a/reftable/system.h\n> +++ b/reftable/system.h\n> @@ -12,7 +12,9 @@ license that can be found in the LICENSE file or at\n>  /* This header glues the reftable library to the rest of Git */\n>  \n>  #include \"git-compat-util.h\"\n> +#include \"lockfile.h\"\n>  #include \"strbuf.h\"\n> +#include \"tempfile.h\"\n>  #include \"hash-ll.h\" /* hash ID, sizes.*/\n>  #include \"dir.h\" /* remove_dir_recursively, for tests.*/\n\nNaive question, why do we include the headers in `system.h`? I assume\nthis is because they are common? Are there other benefits to this\nindirection?\n\n-Justin\n"},{"id":"490077","messageId":"ZehauDgP4L40QKcH@tanuki","threadId":"61046","inReplyTo":"mwhby7dxpiyrvknqe2uoli4ulygjy6hbxcxpqt3alw3dthwntr@4o24tp5jp6h7","subject":"Re: [PATCH 3/4] reftable/stack: register lockfiles during compaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-06T11:59:52Z","receivedAt":"2024-03-06T11:59:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 05, 2024 at 05:30:48PM -0600, Justin Tobler wrote:\n> On 24/03/04 12:10PM, Patrick Steinhardt wrote:\n> > We do not register any of the locks we acquire when compacting the\n> > reftable stack via our lockfiles interfaces. These locks will thus not\n> > be released when Git gets killed.\n> > \n> > Refactor the code to register locks as lockfiles.\n> > \n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> > ...\n> > +\t/*\n> > +\t * Write the new \"tables.list\" contents with the compacted table we\n> > +\t * have just written. In case the compacted table became empty we\n> > +\t * simply skip writing it.\n> > +\t */\n> > +\tfor (i = 0; i < first; i++)\n> > +\t\tstrbuf_addf(&tables_list_buf, \"%s\\n\", st->readers[i]->name);\n> > +\tif (!is_empty_table)\n> > +\t\tstrbuf_addf(&tables_list_buf, \"%s\\n\", new_table_name.buf);\n> \n> Something not really related to this patch, but I noticed and had a\n> question about.\n> \n> If I'm understanding this correctly, when a newly compacted table is\n> empty, it becomes possible for a range of indexes to no longer exist \n> within the stack. If this occurs in the middle of the stack, future\n> compaction will likely combine the tables on either side and restore the\n> missing index range. If the empty table was at the end of the stack,\n> would this effectly reset the max index to something lower for future\n> tables written to the stack? If so, could this lead to issues with\n> separate concurrent table writes?\n\nVery good question indeed, but I think we should be fine here. This is\nmostly because concurrent writers will notice when \"tables.list\" has\nchanged, and, if so, abort the transaction with an out-of-date error.\n\nA few scenarios with concurrent processes, one process which compacts\nthe stack (C) and one which modifies it (M):\n\n  - M acquires the lock before C compacts: M sees the whole stack and\n    uses the latest update index to update it, resulting in a newly\n    written table. When C then locks afterwards, it may decide to\n    compact and drop some tables in the middle of the stack. This may\n    lead to a gap in update indices, but this is fine.\n\n  - M acquires the lock while C compacts: M sees the whole stack and\n    uses the latest update index to update the stack. C then acquires\n    the lock to write the merged tables, notices that its compacted\n    tables still exist and are in the same order, and thus removes them.\n    We now have a gap in update indices, but this is totally fine.\n\n  - M acquires the lock after C compacts: M will refresh \"tables.list\"\n    after it has acquired the lock itself. Thus, it won't ever see the\n    now-dropped empty table.\n\nM cannot write its table when C has the \"tables.list\" lock, so this\nscenario cannot happen. In the same spirit, two Ms cannot race with each\nother either as only one can have the \"tables.list\" lock, and the other\none would abort with an out-of-date error when it has subsequently\nacquired the lock and found the \"tables.list\" contents to have been\nupdated concurrently.\n\n> > ...  \n> > diff --git a/reftable/system.h b/reftable/system.h\n> > index 6b74a81514..5d8b6dede5 100644\n> > --- a/reftable/system.h\n> > +++ b/reftable/system.h\n> > @@ -12,7 +12,9 @@ license that can be found in the LICENSE file or at\n> >  /* This header glues the reftable library to the rest of Git */\n> >  \n> >  #include \"git-compat-util.h\"\n> > +#include \"lockfile.h\"\n> >  #include \"strbuf.h\"\n> > +#include \"tempfile.h\"\n> >  #include \"hash-ll.h\" /* hash ID, sizes.*/\n> >  #include \"dir.h\" /* remove_dir_recursively, for tests.*/\n> \n> Naive question, why do we include the headers in `system.h`? I assume\n> this is because they are common? Are there other benefits to this\n> indirection?\n\nWell, \"system.h\" is supposedly the glue between the common Git codebase\nand the reftable library, so all Git-specific headers should be added\nhere instead of being added individually to the respective files in the\nlibrary. Whether that is ultimately a sensible thing and whether it\nreally helps us all that much is a different question though.\n\nPatrick\n"},{"id":"490078","messageId":"Zehav4V_8GGZG94Q@tanuki","threadId":"61046","inReplyTo":"6cw6d3ubo2kbogzdbniyoznij2zfoh5t3htwb4oaghaltcgeqg@kkrw4g6atr2k","subject":"Re: [PATCH 2/4] reftable/stack: register new tables as tempfiles","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-06T11:59:59Z","receivedAt":"2024-03-06T12:00:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 05, 2024 at 04:30:18PM -0600, Justin Tobler wrote:\n> On 24/03/04 12:10PM, Patrick Steinhardt wrote:\n> > We do not register new tables which we're about to add to the stack with\n> > the tempfile API. Those tables will thus not be deleted in case Git gets\n> > killed.\n> > \n> > Refactor the code to register tables as tempfiles.\n> > \n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  reftable/stack.c | 29 ++++++++++++-----------------\n> >  1 file changed, 12 insertions(+), 17 deletions(-)\n> > \n> > diff --git a/reftable/stack.c b/reftable/stack.c\n> > index b64e55648a..81544fbfa0 100644\n> > --- a/reftable/stack.c\n> > +++ b/reftable/stack.c\n> > @@ -737,8 +737,9 @@ int reftable_addition_add(struct reftable_addition *add,\n> >  \tstruct strbuf tab_file_name = STRBUF_INIT;\n> >  \tstruct strbuf next_name = STRBUF_INIT;\n> >  \tstruct reftable_writer *wr = NULL;\n> > +\tstruct tempfile *tab_file = NULL;\n> >  \tint err = 0;\n> > -\tint tab_fd = 0;\n> > +\tint tab_fd;\n> >  \n> >  \tstrbuf_reset(&next_name);\n> >  \tformat_name(&next_name, add->next_update_index, add->next_update_index);\n> > @@ -746,17 +747,20 @@ int reftable_addition_add(struct reftable_addition *add,\n> >  \tstack_filename(&temp_tab_file_name, add->stack, next_name.buf);\n> >  \tstrbuf_addstr(&temp_tab_file_name, \".temp.XXXXXX\");\n> >  \n> > -\ttab_fd = mkstemp(temp_tab_file_name.buf);\n> > -\tif (tab_fd < 0) {\n> > +\ttab_file = mks_tempfile(temp_tab_file_name.buf);\n> > +\tif (!tab_file) {\n> >  \t\terr = REFTABLE_IO_ERROR;\n> >  \t\tgoto done;\n> >  \t}\n> >  \tif (add->stack->config.default_permissions) {\n> > -\t\tif (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {\n> > +\t\tif (chmod(get_tempfile_path(tab_file),\n> > +\t\t\t  add->stack->config.default_permissions)) {\n> >  \t\t\terr = REFTABLE_IO_ERROR;\n> >  \t\t\tgoto done;\n> >  \t\t}\n> >  \t}\n> \n> Since the tempfile is now being created through the tempfile API, I\n> think the file mode can be set directly through `mks_tempfile_m()`\n> instead of creating the tempfile and then using chmod. Just something I\n> thought to mention.\n\nUnfortunately not. The problem is that `mks_tempfile_m()` will munge\npassed-in permissions via \"core.sharedRepository\", but we already pre\ncalculated the target mode in `config.default_permissions`. Thus, the\nresult would have wrong permissions if we used `mks_tempfile_m()`.\n\nPatrick\n"},{"id":"490079","messageId":"Zeha21vz6EJD8cnk@tanuki","threadId":"61046","inReplyTo":"lyarkcrkcb7ww542lc6y7t534yxnpahvrwephottvcbhr5dkn2@wwd5wk5bzlw7","subject":"Re: [PATCH 1/4] lockfile: report when rollback fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-06T12:00:27Z","receivedAt":"2024-03-06T12:00:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 05, 2024 at 04:09:07PM -0600, Justin Tobler wrote:\n> On 24/03/04 12:10PM, Patrick Steinhardt wrote:\n> > We do not report to the caller when rolling back a lockfile fails, which\n> > will be needed by the reftable compaction logic in a subsequent commit.\n> > It also cannot really report on all errors because the function calls\n> > `delete_tempfile()`, which doesn't return an error either.\n> > \n> > Refactor the code so that both `delete_tempfile()` and\n> > `rollback_lock_file()` return an error code.\n> > \n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  lockfile.h |  4 ++--\n> >  tempfile.c | 21 +++++++++++++--------\n> >  tempfile.h |  2 +-\n> >  3 files changed, 16 insertions(+), 11 deletions(-)\n> > \n> > diff --git a/lockfile.h b/lockfile.h\n> > index 90af4e66b2..4ed570d3f7 100644\n> > --- a/lockfile.h\n> > +++ b/lockfile.h\n> > @@ -323,9 +323,9 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path)\n> >   * for a `lock_file` object that has already been committed or rolled\n> >   * back.\n> >   */\n> > -static inline void rollback_lock_file(struct lock_file *lk)\n> > +static inline int rollback_lock_file(struct lock_file *lk)\n> >  {\n> > -\tdelete_tempfile(&lk->tempfile);\n> > +\treturn delete_tempfile(&lk->tempfile);\n> >  }\n> \n> question: For a lockfile that is already committed or rolled back, is\n> the underlying tempfile still active? I'm trying to figure out if it\n> possible for an error to be returned in this scenerio. If not, it might\n> be nice to clarify in the comment above that, in addition to being a\n> NOOP, no error is returned.\n\nNo, it's not. I thought that it being a no-op should be clear enough,\nbut on the other hand it doesn't hurt either to clarify even further.\n\nI'll refrain from sending a v2 only to add this comment as there haven't\nbeen any other things that need to be addressed yet.\n\nPatrick\n"},{"id":"490095","messageId":"wz2wdic24l6ssynni2vhdhbhgaljxhdk2afe2ooi42hj5bw4pi@yfxlsa4excu5","threadId":"61046","inReplyTo":"Zehav4V_8GGZG94Q@tanuki","subject":"Re: [PATCH 2/4] reftable/stack: register new tables as tempfiles","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-03-06T16:34:39Z","receivedAt":"2024-03-06T16:35:23Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/03/06 12:59PM, Patrick Steinhardt wrote:\n> On Tue, Mar 05, 2024 at 04:30:18PM -0600, Justin Tobler wrote:\n> > On 24/03/04 12:10PM, Patrick Steinhardt wrote:\n> > > We do not register new tables which we're about to add to the stack with\n> > > the tempfile API. Those tables will thus not be deleted in case Git gets\n> > > killed.\n> > > \n> > > Refactor the code to register tables as tempfiles.\n> > > \n> > > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > > ---\n> > >  reftable/stack.c | 29 ++++++++++++-----------------\n> > >  1 file changed, 12 insertions(+), 17 deletions(-)\n> > > \n> > > diff --git a/reftable/stack.c b/reftable/stack.c\n> > > index b64e55648a..81544fbfa0 100644\n> > > --- a/reftable/stack.c\n> > > +++ b/reftable/stack.c\n> > > @@ -737,8 +737,9 @@ int reftable_addition_add(struct reftable_addition *add,\n> > >  \tstruct strbuf tab_file_name = STRBUF_INIT;\n> > >  \tstruct strbuf next_name = STRBUF_INIT;\n> > >  \tstruct reftable_writer *wr = NULL;\n> > > +\tstruct tempfile *tab_file = NULL;\n> > >  \tint err = 0;\n> > > -\tint tab_fd = 0;\n> > > +\tint tab_fd;\n> > >  \n> > >  \tstrbuf_reset(&next_name);\n> > >  \tformat_name(&next_name, add->next_update_index, add->next_update_index);\n> > > @@ -746,17 +747,20 @@ int reftable_addition_add(struct reftable_addition *add,\n> > >  \tstack_filename(&temp_tab_file_name, add->stack, next_name.buf);\n> > >  \tstrbuf_addstr(&temp_tab_file_name, \".temp.XXXXXX\");\n> > >  \n> > > -\ttab_fd = mkstemp(temp_tab_file_name.buf);\n> > > -\tif (tab_fd < 0) {\n> > > +\ttab_file = mks_tempfile(temp_tab_file_name.buf);\n> > > +\tif (!tab_file) {\n> > >  \t\terr = REFTABLE_IO_ERROR;\n> > >  \t\tgoto done;\n> > >  \t}\n> > >  \tif (add->stack->config.default_permissions) {\n> > > -\t\tif (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {\n> > > +\t\tif (chmod(get_tempfile_path(tab_file),\n> > > +\t\t\t  add->stack->config.default_permissions)) {\n> > >  \t\t\terr = REFTABLE_IO_ERROR;\n> > >  \t\t\tgoto done;\n> > >  \t\t}\n> > >  \t}\n> > \n> > Since the tempfile is now being created through the tempfile API, I\n> > think the file mode can be set directly through `mks_tempfile_m()`\n> > instead of creating the tempfile and then using chmod. Just something I\n> > thought to mention.\n> \n> Unfortunately not. The problem is that `mks_tempfile_m()` will munge\n> passed-in permissions via \"core.sharedRepository\", but we already pre\n> calculated the target mode in `config.default_permissions`. Thus, the\n> result would have wrong permissions if we used `mks_tempfile_m()`.\n\nAh that makes sense. Thanks for the explanation!\n\n-Justin\n"},{"id":"490096","messageId":"xmqqedcntj06.fsf@gitster.g","threadId":"61046","inReplyTo":"Zehav4V_8GGZG94Q@tanuki","subject":"Re: [PATCH 2/4] reftable/stack: register new tables as tempfiles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-06T16:36:25Z","receivedAt":"2024-03-06T16:36:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> Since the tempfile is now being created through the tempfile API, I\n>> think the file mode can be set directly through `mks_tempfile_m()`\n>> instead of creating the tempfile and then using chmod. Just something I\n>> thought to mention.\n>\n> Unfortunately not. The problem is that `mks_tempfile_m()` will munge\n> passed-in permissions via \"core.sharedRepository\", but we already pre\n> calculated the target mode in `config.default_permissions`. Thus, the\n> result would have wrong permissions if we used `mks_tempfile_m()`.\n\nI somehow found that default_permissions thing always disturbing.\n\nEven if we keep a separate mechanism for determining the file\npermission (perhaps in order to give ourselves a better separation\nas \"an independent library\" from the rest of Git), shouldn't the\npermission setting that is computed by the mechanism and stored in\nconfig.default_permissions be consistent with the permission the\nrest of git computes based on core.sharedRepository?\n"},{"id":"490097","messageId":"xmqq7ciftiuw.fsf@gitster.g","threadId":"61046","inReplyTo":"ZehauDgP4L40QKcH@tanuki","subject":"Re: [PATCH 3/4] reftable/stack: register lockfiles during compaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-06T16:39:35Z","receivedAt":"2024-03-06T16:39:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> Naive question, why do we include the headers in `system.h`? I assume\n>> this is because they are common? Are there other benefits to this\n>> indirection?\n>\n> Well, \"system.h\" is supposedly the glue between the common Git codebase\n> and the reftable library, so all Git-specific headers should be added\n> here instead of being added individually to the respective files in the\n> library. Whether that is ultimately a sensible thing and whether it\n> really helps us all that much is a different question though.\n\nThat matches my understanding of what have been done in reftable/\ndirectory.  If a project other than Git wants to use the reftable\ncode, they only need to prepare a shim and write their own\n\"system.h\" to provide services equivalent to what Git supplies.\n\nThanks.\n"},{"id":"490110","messageId":"tqr5merpvso5g7b53idhtfhy7wjipkbujxv6bmdij2absrtmmk@2lbfweqvldc4","threadId":"61046","inReplyTo":"ZehauDgP4L40QKcH@tanuki","subject":"Re: [PATCH 3/4] reftable/stack: register lockfiles during compaction","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-03-06T19:57:23Z","receivedAt":"2024-03-06T19:58:07Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/03/06 12:59PM, Patrick Steinhardt wrote:\n\n> > Something not really related to this patch, but I noticed and had a\n> > question about.\n> > \n> > If I'm understanding this correctly, when a newly compacted table is\n> > empty, it becomes possible for a range of indexes to no longer exist \n> > within the stack. If this occurs in the middle of the stack, future\n> > compaction will likely combine the tables on either side and restore the\n> > missing index range. If the empty table was at the end of the stack,\n> > would this effectly reset the max index to something lower for future\n> > tables written to the stack? If so, could this lead to issues with\n> > separate concurrent table writes?\n> \n> Very good question indeed, but I think we should be fine here. This is\n> mostly because concurrent writers will notice when \"tables.list\" has\n> changed, and, if so, abort the transaction with an out-of-date error.\n> \n> A few scenarios with concurrent processes, one process which compacts\n> the stack (C) and one which modifies it (M):\n> \n>   - M acquires the lock before C compacts: M sees the whole stack and\n>     uses the latest update index to update it, resulting in a newly\n>     written table. When C then locks afterwards, it may decide to\n>     compact and drop some tables in the middle of the stack. This may\n>     lead to a gap in update indices, but this is fine.\n> \n>   - M acquires the lock while C compacts: M sees the whole stack and\n>     uses the latest update index to update the stack. C then acquires\n>     the lock to write the merged tables, notices that its compacted\n>     tables still exist and are in the same order, and thus removes them.\n>     We now have a gap in update indices, but this is totally fine.\n> \n>   - M acquires the lock after C compacts: M will refresh \"tables.list\"\n>     after it has acquired the lock itself. Thus, it won't ever see the\n>     now-dropped empty table.\n> \n> M cannot write its table when C has the \"tables.list\" lock, so this\n> scenario cannot happen. In the same spirit, two Ms cannot race with each\n> other either as only one can have the \"tables.list\" lock, and the other\n> one would abort with an out-of-date error when it has subsequently\n> acquired the lock and found the \"tables.list\" contents to have been\n> updated concurrently.\n\nThanks Patrick for the great explanation! Digging into this a bit\nfurther, I see that we return `REFTABLE_LOCK_ERROR` when the list file\nlock already exists or has changed when attempting to add a new table to\nthe list. \n\nWhen performing compaction in `stack_compact_range()`, after initially\nacquiring the table list lock, we also check if the stack is up-to-date\nwith `stack_uptodate()`. I noticed that this check is not performed\nagain after the table list is locked for the second time. At first I\nthought this could be problematic, but I realized that this would only\nbe an issue for concurrent compactions and because the tables are locked\nit should not matter.\n\n-Justin\n"},{"id":"490135","messageId":"Zelb8ldHh4Lnlh7Z@tanuki","threadId":"61046","inReplyTo":"xmqqedcntj06.fsf@gitster.g","subject":"Re: [PATCH 2/4] reftable/stack: register new tables as tempfiles","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-07T06:17:22Z","receivedAt":"2024-03-07T06:17:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 06, 2024 at 08:36:25AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> >> Since the tempfile is now being created through the tempfile API, I\n> >> think the file mode can be set directly through `mks_tempfile_m()`\n> >> instead of creating the tempfile and then using chmod. Just something I\n> >> thought to mention.\n> >\n> > Unfortunately not. The problem is that `mks_tempfile_m()` will munge\n> > passed-in permissions via \"core.sharedRepository\", but we already pre\n> > calculated the target mode in `config.default_permissions`. Thus, the\n> > result would have wrong permissions if we used `mks_tempfile_m()`.\n> \n> I somehow found that default_permissions thing always disturbing.\n> \n> Even if we keep a separate mechanism for determining the file\n> permission (perhaps in order to give ourselves a better separation\n> as \"an independent library\" from the rest of Git), shouldn't the\n> permission setting that is computed by the mechanism and stored in\n> config.default_permissions be consistent with the permission the\n> rest of git computes based on core.sharedRepository?\n\nIt is consistent. The problem is rather that `mks_tempfile_m()` takes a\nmode as input, but still ends up applying the umask to that mode. Thus,\nusing that function without a subsequent call to chmod(3P) would end up\nmishandling \"core.sharedRepository\".\n\nPatrick\n"},{"id":"490172","messageId":"87sf12fc7z.fsf@to1.studio","threadId":"61046","inReplyTo":"b952d54a05e1c0cf47371f78e3901cfb2119e246.1709549619.git.ps@pks.im","subject":"Re: [PATCH 4/4] reftable/stack: register compacted tables as tempfiles","fromName":"Toon claes","fromEmail":"toon@iotcl.com","sentAt":"2024-03-07T12:38:56Z","receivedAt":"2024-03-07T12:39:10Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"> diff --git a/reftable/stack.c b/reftable/stack.c\n> index 977336b7d5..40129da16c 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -827,51 +827,57 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)\n>  \n>  static int stack_compact_locked(struct reftable_stack *st,\n>  \t\t\t\tsize_t first, size_t last,\n> -\t\t\t\tstruct strbuf *temp_tab,\n> -\t\t\t\tstruct reftable_log_expiry_config *config)\n> +\t\t\t\tstruct reftable_log_expiry_config *config,\n> +\t\t\t\tstruct tempfile **temp_table_out)\n>  {\n>  \tstruct strbuf next_name = STRBUF_INIT;\n> -\tint tab_fd = -1;\n> +\tstruct strbuf table_path = STRBUF_INIT;\n>  \tstruct reftable_writer *wr = NULL;\n> +\tstruct tempfile *temp_table;\n> +\tint temp_table_fd;\n\nJust one small nit, if you don't mind? In PATCH 2/4 you use\n`struct tempfile *tab_file` and `int tab_fd`. I would like to see\nconsistency and use similar names. Personally I don't like table being\nshortened to \"tab\", and I think you feel the same as you've renamed the\nparameter from this function.\n\n\n-- \nToon\n"},{"id":"490174","messageId":"Zem5_Z171kzCkRK5@tanuki","threadId":"61046","inReplyTo":"87sf12fc7z.fsf@to1.studio","subject":"Re: [PATCH 4/4] reftable/stack: register compacted tables as tempfiles","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-07T12:58:37Z","receivedAt":"2024-03-07T12:58:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 07, 2024 at 01:38:56PM +0100, Toon claes wrote:\n> > diff --git a/reftable/stack.c b/reftable/stack.c\n> > index 977336b7d5..40129da16c 100644\n> > --- a/reftable/stack.c\n> > +++ b/reftable/stack.c\n> > @@ -827,51 +827,57 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)\n> >  \n> >  static int stack_compact_locked(struct reftable_stack *st,\n> >  \t\t\t\tsize_t first, size_t last,\n> > -\t\t\t\tstruct strbuf *temp_tab,\n> > -\t\t\t\tstruct reftable_log_expiry_config *config)\n> > +\t\t\t\tstruct reftable_log_expiry_config *config,\n> > +\t\t\t\tstruct tempfile **temp_table_out)\n> >  {\n> >  \tstruct strbuf next_name = STRBUF_INIT;\n> > -\tint tab_fd = -1;\n> > +\tstruct strbuf table_path = STRBUF_INIT;\n> >  \tstruct reftable_writer *wr = NULL;\n> > +\tstruct tempfile *temp_table;\n> > +\tint temp_table_fd;\n> \n> Just one small nit, if you don't mind? In PATCH 2/4 you use\n> `struct tempfile *tab_file` and `int tab_fd`. I would like to see\n> consistency and use similar names. Personally I don't like table being\n> shortened to \"tab\", and I think you feel the same as you've renamed the\n> parameter from this function.\n\nYeah, I'm always a proponent of descriptive, unabbreviated names and\nwould thus rather want to use \"temp_table\" or something like that. But\nin 2/4 I decided to stick with \"tab_file\" because there is a bunch of\nalready existing variables in that function which are called similar --\n\"temp_tab_file_name\", \"tab_file_name\" and \"tab_fd\". But renaming all of\nthem to ensure function-level consistency would create a lot of churn.\n\nI'll thus instead rename variables over here to their abbreviated\nversions.\n\nPatrick\n"},{"id":"490175","messageId":"cover.1709816483.git.ps@pks.im","threadId":"61046","inReplyTo":"cover.1709549619.git.ps@pks.im","subject":"[PATCH v2 0/4] reftable/stack: register temporary files","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-07T13:10:27Z","receivedAt":"2024-03-07T13:10:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that registers temporary\nfiles written by the reftable library with the tempfile framework. This\nensures that those files get cleaned up even when Git dies or otherwise\ngets signalled.\n\nChanges compared to v1:\n\n  - Patch 1: Clarify that rolling back a deactivated lockfile will not\n    result in an error.\n\n  - Patch 4: Rename some local variables for improved consistency with\n    other code.\n\nPatrick\n\nPatrick Steinhardt (4):\n  lockfile: report when rollback fails\n  reftable/stack: register new tables as tempfiles\n  reftable/stack: register lockfiles during compaction\n  reftable/stack: register compacted tables as tempfiles\n\n lockfile.h        |   6 +-\n reftable/stack.c  | 330 ++++++++++++++++++++++------------------------\n reftable/system.h |   2 +\n tempfile.c        |  21 +--\n tempfile.h        |   2 +-\n 5 files changed, 178 insertions(+), 183 deletions(-)\n\nRange-diff against v1:\n1:  1acaa9ca1a ! 1:  782e96a678 lockfile: report when rollback fails\n    @@ Commit message\n     \n      ## lockfile.h ##\n     @@ lockfile.h: static inline int commit_lock_file_to(struct lock_file *lk, const char *path)\n    +  * Roll back `lk`: close the file descriptor and/or file pointer and\n    +  * remove the lockfile. It is a NOOP to call `rollback_lock_file()`\n       * for a `lock_file` object that has already been committed or rolled\n    -  * back.\n    +- * back.\n    ++ * back. No error will be returned in this case.\n       */\n     -static inline void rollback_lock_file(struct lock_file *lk)\n     +static inline int rollback_lock_file(struct lock_file *lk)\n2:  02bf41d419 = 2:  5dbc93d5be reftable/stack: register new tables as tempfiles\n3:  45b5c3167f = 3:  c88c85443e reftable/stack: register lockfiles during compaction\n4:  b952d54a05 ! 4:  4023d78f08 reftable/stack: register compacted tables as tempfiles\n    @@ reftable/stack.c: uint64_t reftable_stack_next_update_index(struct reftable_stac\n     -\t\t\t\tstruct strbuf *temp_tab,\n     -\t\t\t\tstruct reftable_log_expiry_config *config)\n     +\t\t\t\tstruct reftable_log_expiry_config *config,\n    -+\t\t\t\tstruct tempfile **temp_table_out)\n    ++\t\t\t\tstruct tempfile **tab_file_out)\n      {\n      \tstruct strbuf next_name = STRBUF_INIT;\n     -\tint tab_fd = -1;\n    -+\tstruct strbuf table_path = STRBUF_INIT;\n    ++\tstruct strbuf tab_file_path = STRBUF_INIT;\n      \tstruct reftable_writer *wr = NULL;\n    -+\tstruct tempfile *temp_table;\n    -+\tint temp_table_fd;\n    - \tint err = 0;\n    +-\tint err = 0;\n    ++\tstruct tempfile *tab_file;\n    ++\tint tab_fd, err = 0;\n      \n      \tformat_name(&next_name,\n      \t\t    reftable_reader_min_update_index(st->readers[first]),\n      \t\t    reftable_reader_max_update_index(st->readers[last]));\n    -+\tstack_filename(&table_path, st, next_name.buf);\n    -+\tstrbuf_addstr(&table_path, \".temp.XXXXXX\");\n    ++\tstack_filename(&tab_file_path, st, next_name.buf);\n    ++\tstrbuf_addstr(&tab_file_path, \".temp.XXXXXX\");\n      \n     -\tstack_filename(temp_tab, st, next_name.buf);\n     -\tstrbuf_addstr(temp_tab, \".temp.XXXXXX\");\n    -+\ttemp_table = mks_tempfile(table_path.buf);\n    -+\tif (!temp_table) {\n    ++\ttab_file = mks_tempfile(tab_file_path.buf);\n    ++\tif (!tab_file) {\n     +\t\terr = REFTABLE_IO_ERROR;\n     +\t\tgoto done;\n     +\t}\n    -+\ttemp_table_fd = get_tempfile_fd(temp_table);\n    ++\ttab_fd = get_tempfile_fd(tab_file);\n      \n     -\ttab_fd = mkstemp(temp_tab->buf);\n      \tif (st->config.default_permissions &&\n     -\t    chmod(temp_tab->buf, st->config.default_permissions) < 0) {\n    -+\t    chmod(get_tempfile_path(temp_table), st->config.default_permissions) < 0) {\n    ++\t    chmod(get_tempfile_path(tab_file), st->config.default_permissions) < 0) {\n      \t\terr = REFTABLE_IO_ERROR;\n      \t\tgoto done;\n      \t}\n    @@ reftable/stack.c: uint64_t reftable_stack_next_update_index(struct reftable_stac\n     -\twr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd, &st->config);\n     -\n     +\twr = reftable_new_writer(reftable_fd_write, reftable_fd_flush,\n    -+\t\t\t\t &temp_table_fd, &st->config);\n    ++\t\t\t\t &tab_fd, &st->config);\n      \terr = stack_write_compact(st, wr, first, last, config);\n      \tif (err < 0)\n      \t\tgoto done;\n    @@ reftable/stack.c: uint64_t reftable_stack_next_update_index(struct reftable_stac\n      \n     -\terr = close(tab_fd);\n     -\ttab_fd = 0;\n    -+\terr = close_tempfile_gently(temp_table);\n    ++\terr = close_tempfile_gently(tab_file);\n     +\tif (err < 0)\n     +\t\tgoto done;\n     +\n    -+\t*temp_table_out = temp_table;\n    -+\ttemp_table = NULL;\n    ++\t*tab_file_out = tab_file;\n    ++\ttab_file = NULL;\n      \n      done:\n    -+\tdelete_tempfile(&temp_table);\n    ++\tdelete_tempfile(&tab_file);\n      \treftable_writer_free(wr);\n     -\tif (tab_fd > 0) {\n     -\t\tclose(tab_fd);\n    @@ reftable/stack.c: uint64_t reftable_stack_next_update_index(struct reftable_stac\n     -\t\tstrbuf_release(temp_tab);\n     -\t}\n      \tstrbuf_release(&next_name);\n    -+\tstrbuf_release(&table_path);\n    ++\tstrbuf_release(&tab_file_path);\n      \treturn err;\n      }\n      \n-- \n2.44.0\n\n"},{"id":"490176","messageId":"782e96a678346bbdfca51ebcb4514307de84b882.1709816483.git.ps@pks.im","threadId":"61046","inReplyTo":"cover.1709816483.git.ps@pks.im","subject":"[PATCH v2 1/4] lockfile: report when rollback fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-07T13:10:31Z","receivedAt":"2024-03-07T13:10:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not report to the caller when rolling back a lockfile fails, which\nwill be needed by the reftable compaction logic in a subsequent commit.\nIt also cannot really report on all errors because the function calls\n`delete_tempfile()`, which doesn't return an error either.\n\nRefactor the code so that both `delete_tempfile()` and\n`rollback_lock_file()` return an error code.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n lockfile.h |  6 +++---\n tempfile.c | 21 +++++++++++++--------\n tempfile.h |  2 +-\n 3 files changed, 17 insertions(+), 12 deletions(-)\n\ndiff --git a/lockfile.h b/lockfile.h\nindex 90af4e66b2..1bb9926497 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -321,11 +321,11 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path)\n  * Roll back `lk`: close the file descriptor and/or file pointer and\n  * remove the lockfile. It is a NOOP to call `rollback_lock_file()`\n  * for a `lock_file` object that has already been committed or rolled\n- * back.\n+ * back. No error will be returned in this case.\n  */\n-static inline void rollback_lock_file(struct lock_file *lk)\n+static inline int rollback_lock_file(struct lock_file *lk)\n {\n-\tdelete_tempfile(&lk->tempfile);\n+\treturn delete_tempfile(&lk->tempfile);\n }\n \n #endif /* LOCKFILE_H */\ndiff --git a/tempfile.c b/tempfile.c\nindex ecdebf1afb..ed88cf8431 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -50,15 +50,17 @@\n \n static VOLATILE_LIST_HEAD(tempfile_list);\n \n-static void remove_template_directory(struct tempfile *tempfile,\n+static int remove_template_directory(struct tempfile *tempfile,\n \t\t\t\t      int in_signal_handler)\n {\n \tif (tempfile->directory) {\n \t\tif (in_signal_handler)\n-\t\t\trmdir(tempfile->directory);\n+\t\t\treturn rmdir(tempfile->directory);\n \t\telse\n-\t\t\trmdir_or_warn(tempfile->directory);\n+\t\t\treturn rmdir_or_warn(tempfile->directory);\n \t}\n+\n+\treturn 0;\n }\n \n static void remove_tempfiles(int in_signal_handler)\n@@ -353,16 +355,19 @@ int rename_tempfile(struct tempfile **tempfile_p, const char *path)\n \treturn 0;\n }\n \n-void delete_tempfile(struct tempfile **tempfile_p)\n+int delete_tempfile(struct tempfile **tempfile_p)\n {\n \tstruct tempfile *tempfile = *tempfile_p;\n+\tint err = 0;\n \n \tif (!is_tempfile_active(tempfile))\n-\t\treturn;\n+\t\treturn 0;\n \n-\tclose_tempfile_gently(tempfile);\n-\tunlink_or_warn(tempfile->filename.buf);\n-\tremove_template_directory(tempfile, 0);\n+\terr |= close_tempfile_gently(tempfile);\n+\terr |= unlink_or_warn(tempfile->filename.buf);\n+\terr |= remove_template_directory(tempfile, 0);\n \tdeactivate_tempfile(tempfile);\n \t*tempfile_p = NULL;\n+\n+\treturn err ? -1 : 0;\n }\ndiff --git a/tempfile.h b/tempfile.h\nindex d0413af733..2d2ae5b657 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -269,7 +269,7 @@ int reopen_tempfile(struct tempfile *tempfile);\n  * `delete_tempfile()` for a `tempfile` object that has already been\n  * deleted or renamed.\n  */\n-void delete_tempfile(struct tempfile **tempfile_p);\n+int delete_tempfile(struct tempfile **tempfile_p);\n \n /*\n  * Close the file descriptor and/or file pointer if they are still\n-- \n2.44.0\n\n"},{"id":"490177","messageId":"5dbc93d5be38552dd62d8d7d2566c73fc2dca162.1709816483.git.ps@pks.im","threadId":"61046","inReplyTo":"cover.1709816483.git.ps@pks.im","subject":"[PATCH v2 2/4] reftable/stack: register new tables as tempfiles","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-07T13:10:35Z","receivedAt":"2024-03-07T13:10:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not register new tables which we're about to add to the stack with\nthe tempfile API. Those tables will thus not be deleted in case Git gets\nkilled.\n\nRefactor the code to register tables as tempfiles.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 29 ++++++++++++-----------------\n 1 file changed, 12 insertions(+), 17 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex b64e55648a..81544fbfa0 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -737,8 +737,9 @@ int reftable_addition_add(struct reftable_addition *add,\n \tstruct strbuf tab_file_name = STRBUF_INIT;\n \tstruct strbuf next_name = STRBUF_INIT;\n \tstruct reftable_writer *wr = NULL;\n+\tstruct tempfile *tab_file = NULL;\n \tint err = 0;\n-\tint tab_fd = 0;\n+\tint tab_fd;\n \n \tstrbuf_reset(&next_name);\n \tformat_name(&next_name, add->next_update_index, add->next_update_index);\n@@ -746,17 +747,20 @@ int reftable_addition_add(struct reftable_addition *add,\n \tstack_filename(&temp_tab_file_name, add->stack, next_name.buf);\n \tstrbuf_addstr(&temp_tab_file_name, \".temp.XXXXXX\");\n \n-\ttab_fd = mkstemp(temp_tab_file_name.buf);\n-\tif (tab_fd < 0) {\n+\ttab_file = mks_tempfile(temp_tab_file_name.buf);\n+\tif (!tab_file) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n \tif (add->stack->config.default_permissions) {\n-\t\tif (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {\n+\t\tif (chmod(get_tempfile_path(tab_file),\n+\t\t\t  add->stack->config.default_permissions)) {\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n \t\t}\n \t}\n+\ttab_fd = get_tempfile_fd(tab_file);\n+\n \twr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd,\n \t\t\t\t &add->stack->config);\n \terr = write_table(wr, arg);\n@@ -771,14 +775,13 @@ int reftable_addition_add(struct reftable_addition *add,\n \tif (err < 0)\n \t\tgoto done;\n \n-\terr = close(tab_fd);\n-\ttab_fd = 0;\n+\terr = close_tempfile_gently(tab_file);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n \n-\terr = stack_check_addition(add->stack, temp_tab_file_name.buf);\n+\terr = stack_check_addition(add->stack, get_tempfile_path(tab_file));\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -789,14 +792,13 @@ int reftable_addition_add(struct reftable_addition *add,\n \n \tformat_name(&next_name, wr->min_update_index, wr->max_update_index);\n \tstrbuf_addstr(&next_name, \".ref\");\n-\n \tstack_filename(&tab_file_name, add->stack, next_name.buf);\n \n \t/*\n \t  On windows, this relies on rand() picking a unique destination name.\n \t  Maybe we should do retry loop as well?\n \t */\n-\terr = rename(temp_tab_file_name.buf, tab_file_name.buf);\n+\terr = rename_tempfile(&tab_file, tab_file_name.buf);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n@@ -806,14 +808,7 @@ int reftable_addition_add(struct reftable_addition *add,\n \t\t\t    add->new_tables_cap);\n \tadd->new_tables[add->new_tables_len++] = strbuf_detach(&next_name, NULL);\n done:\n-\tif (tab_fd > 0) {\n-\t\tclose(tab_fd);\n-\t\ttab_fd = 0;\n-\t}\n-\tif (temp_tab_file_name.len > 0) {\n-\t\tunlink(temp_tab_file_name.buf);\n-\t}\n-\n+\tdelete_tempfile(&tab_file);\n \tstrbuf_release(&temp_tab_file_name);\n \tstrbuf_release(&tab_file_name);\n \tstrbuf_release(&next_name);\n-- \n2.44.0\n\n"},{"id":"490178","messageId":"c88c85443e8aeb299327402572c08bcac7d3f13a.1709816483.git.ps@pks.im","threadId":"61046","inReplyTo":"cover.1709816483.git.ps@pks.im","subject":"[PATCH v2 3/4] reftable/stack: register lockfiles during compaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-07T13:10:39Z","receivedAt":"2024-03-07T13:10:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not register any of the locks we acquire when compacting the\nreftable stack via our lockfiles interfaces. These locks will thus not\nbe released when Git gets killed.\n\nRefactor the code to register locks as lockfiles.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c  | 255 ++++++++++++++++++++++------------------------\n reftable/system.h |   2 +\n 2 files changed, 123 insertions(+), 134 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 81544fbfa0..977336b7d5 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -978,212 +978,199 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t       size_t first, size_t last,\n \t\t\t       struct reftable_log_expiry_config *expiry)\n {\n-\tchar **delete_on_success = NULL, **subtable_locks = NULL, **listp = NULL;\n-\tstruct strbuf temp_tab_file_name = STRBUF_INIT;\n+\tstruct strbuf tables_list_buf = STRBUF_INIT;\n+\tstruct strbuf new_table_temp_path = STRBUF_INIT;\n \tstruct strbuf new_table_name = STRBUF_INIT;\n-\tstruct strbuf lock_file_name = STRBUF_INIT;\n-\tstruct strbuf ref_list_contents = STRBUF_INIT;\n \tstruct strbuf new_table_path = STRBUF_INIT;\n-\tsize_t i, j, compact_count;\n-\tint err = 0;\n-\tint have_lock = 0;\n-\tint lock_file_fd = -1;\n-\tint is_empty_table = 0;\n+\tstruct strbuf table_name = STRBUF_INIT;\n+\tstruct lock_file tables_list_lock = LOCK_INIT;\n+\tstruct lock_file *table_locks = NULL;\n+\tint is_empty_table = 0, err = 0;\n+\tsize_t i;\n \n \tif (first > last || (!expiry && first == last)) {\n \t\terr = 0;\n \t\tgoto done;\n \t}\n \n-\tcompact_count = last - first + 1;\n-\tREFTABLE_CALLOC_ARRAY(delete_on_success, compact_count + 1);\n-\tREFTABLE_CALLOC_ARRAY(subtable_locks, compact_count + 1);\n-\n \tst->stats.attempts++;\n \n-\tstrbuf_reset(&lock_file_name);\n-\tstrbuf_addstr(&lock_file_name, st->list_file);\n-\tstrbuf_addstr(&lock_file_name, \".lock\");\n-\n-\tlock_file_fd =\n-\t\topen(lock_file_name.buf, O_EXCL | O_CREAT | O_WRONLY, 0666);\n-\tif (lock_file_fd < 0) {\n-\t\tif (errno == EEXIST) {\n+\t/*\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+\tif (err < 0) {\n+\t\tif (errno == EEXIST)\n \t\t\terr = 1;\n-\t\t} else {\n+\t\telse\n \t\t\terr = REFTABLE_IO_ERROR;\n-\t\t}\n \t\tgoto done;\n \t}\n-\t/* Don't want to write to the lock for now.  */\n-\tclose(lock_file_fd);\n-\tlock_file_fd = -1;\n \n-\thave_lock = 1;\n \terr = stack_uptodate(st);\n-\tif (err != 0)\n+\tif (err)\n \t\tgoto done;\n \n-\tfor (i = first, j = 0; i <= last; i++) {\n-\t\tstruct strbuf subtab_file_name = STRBUF_INIT;\n-\t\tstruct strbuf subtab_lock = STRBUF_INIT;\n-\t\tint sublock_file_fd = -1;\n-\n-\t\tstack_filename(&subtab_file_name, st,\n-\t\t\t       reader_name(st->readers[i]));\n-\n-\t\tstrbuf_reset(&subtab_lock);\n-\t\tstrbuf_addbuf(&subtab_lock, &subtab_file_name);\n-\t\tstrbuf_addstr(&subtab_lock, \".lock\");\n+\t/*\n+\t * Lock all tables in the user-provided range. This is the slice of our\n+\t * stack which we'll compact.\n+\t */\n+\tREFTABLE_CALLOC_ARRAY(table_locks, last - first + 1);\n+\tfor (i = first; i <= last; i++) {\n+\t\tstack_filename(&table_name, st, reader_name(st->readers[i]));\n \n-\t\tsublock_file_fd = open(subtab_lock.buf,\n-\t\t\t\t       O_EXCL | O_CREAT | O_WRONLY, 0666);\n-\t\tif (sublock_file_fd >= 0) {\n-\t\t\tclose(sublock_file_fd);\n-\t\t} else if (sublock_file_fd < 0) {\n-\t\t\tif (errno == EEXIST) {\n+\t\terr = hold_lock_file_for_update(&table_locks[i - first],\n+\t\t\t\t\t\ttable_name.buf, LOCK_NO_DEREF);\n+\t\tif (err < 0) {\n+\t\t\tif (errno == EEXIST)\n \t\t\t\terr = 1;\n-\t\t\t} else {\n+\t\t\telse\n \t\t\t\terr = REFTABLE_IO_ERROR;\n-\t\t\t}\n+\t\t\tgoto done;\n \t\t}\n \n-\t\tsubtable_locks[j] = subtab_lock.buf;\n-\t\tdelete_on_success[j] = subtab_file_name.buf;\n-\t\tj++;\n-\n-\t\tif (err != 0)\n+\t\t/*\n+\t\t * We need to close the lockfiles as we might otherwise easily\n+\t\t * run into file descriptor exhaustion when we compress a lot\n+\t\t * of tables.\n+\t\t */\n+\t\terr = close_lock_file_gently(&table_locks[i - first]);\n+\t\tif (err < 0) {\n+\t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n+\t\t}\n \t}\n \n-\terr = unlink(lock_file_name.buf);\n-\tif (err < 0)\n+\t/*\n+\t * We have locked all tables in our range and can thus release the\n+\t * \"tables.list\" lock while compacting the locked tables. This allows\n+\t * concurrent updates to the stack to proceed.\n+\t */\n+\terr = rollback_lock_file(&tables_list_lock);\n+\tif (err < 0) {\n+\t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n-\thave_lock = 0;\n-\n-\terr = stack_compact_locked(st, first, last, &temp_tab_file_name,\n-\t\t\t\t   expiry);\n-\t/* Compaction + tombstones can create an empty table out of non-empty\n-\t * tables. */\n-\tis_empty_table = (err == REFTABLE_EMPTY_TABLE_ERROR);\n-\tif (is_empty_table) {\n-\t\terr = 0;\n \t}\n-\tif (err < 0)\n-\t\tgoto done;\n \n-\tlock_file_fd =\n-\t\topen(lock_file_name.buf, O_EXCL | O_CREAT | O_WRONLY, 0666);\n-\tif (lock_file_fd < 0) {\n-\t\tif (errno == EEXIST) {\n+\t/*\n+\t * Compact the now-locked tables into a new table. Note that compacting\n+\t * these tables may end up with an empty new table in case tombstones\n+\t * end up cancelling out all refs in that range.\n+\t */\n+\terr = stack_compact_locked(st, first, last, &new_table_temp_path, expiry);\n+\tif (err < 0) {\n+\t\tif (err != REFTABLE_EMPTY_TABLE_ERROR)\n+\t\t\tgoto done;\n+\t\tis_empty_table = 1;\n+\t}\n+\n+\t/*\n+\t * Now that we have written the new, compacted table we need to re-lock\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+\tif (err < 0) {\n+\t\tif (errno == EEXIST)\n \t\t\terr = 1;\n-\t\t} else {\n+\t\telse\n \t\t\terr = REFTABLE_IO_ERROR;\n-\t\t}\n \t\tgoto done;\n \t}\n-\thave_lock = 1;\n+\n \tif (st->config.default_permissions) {\n-\t\tif (chmod(lock_file_name.buf, st->config.default_permissions) < 0) {\n+\t\tif (chmod(get_lock_file_path(&tables_list_lock),\n+\t\t\t  st->config.default_permissions) < 0) {\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n \t\t}\n \t}\n \n-\tformat_name(&new_table_name, st->readers[first]->min_update_index,\n-\t\t    st->readers[last]->max_update_index);\n-\tstrbuf_addstr(&new_table_name, \".ref\");\n-\n-\tstack_filename(&new_table_path, st, new_table_name.buf);\n-\n+\t/*\n+\t * If the resulting compacted table is not empty, then we need to move\n+\t * it into place now.\n+\t */\n \tif (!is_empty_table) {\n-\t\t/* retry? */\n-\t\terr = rename(temp_tab_file_name.buf, new_table_path.buf);\n+\t\tformat_name(&new_table_name, st->readers[first]->min_update_index,\n+\t\t\t    st->readers[last]->max_update_index);\n+\t\tstrbuf_addstr(&new_table_name, \".ref\");\n+\t\tstack_filename(&new_table_path, st, new_table_name.buf);\n+\n+\t\terr = rename(new_table_temp_path.buf, new_table_path.buf);\n \t\tif (err < 0) {\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n \t\t}\n \t}\n \n-\tfor (i = 0; i < first; i++) {\n-\t\tstrbuf_addstr(&ref_list_contents, st->readers[i]->name);\n-\t\tstrbuf_addstr(&ref_list_contents, \"\\n\");\n-\t}\n-\tif (!is_empty_table) {\n-\t\tstrbuf_addbuf(&ref_list_contents, &new_table_name);\n-\t\tstrbuf_addstr(&ref_list_contents, \"\\n\");\n-\t}\n-\tfor (i = last + 1; i < st->merged->stack_len; i++) {\n-\t\tstrbuf_addstr(&ref_list_contents, st->readers[i]->name);\n-\t\tstrbuf_addstr(&ref_list_contents, \"\\n\");\n-\t}\n-\n-\terr = write_in_full(lock_file_fd, ref_list_contents.buf, ref_list_contents.len);\n-\tif (err < 0) {\n-\t\terr = REFTABLE_IO_ERROR;\n-\t\tunlink(new_table_path.buf);\n-\t\tgoto done;\n-\t}\n-\n-\terr = fsync_component(FSYNC_COMPONENT_REFERENCE, lock_file_fd);\n+\t/*\n+\t * Write the new \"tables.list\" contents with the compacted table we\n+\t * have just written. In case the compacted table became empty we\n+\t * simply skip writing it.\n+\t */\n+\tfor (i = 0; i < first; i++)\n+\t\tstrbuf_addf(&tables_list_buf, \"%s\\n\", st->readers[i]->name);\n+\tif (!is_empty_table)\n+\t\tstrbuf_addf(&tables_list_buf, \"%s\\n\", new_table_name.buf);\n+\tfor (i = last + 1; i < st->merged->stack_len; i++)\n+\t\tstrbuf_addf(&tables_list_buf, \"%s\\n\", st->readers[i]->name);\n+\n+\terr = write_in_full(get_lock_file_fd(&tables_list_lock),\n+\t\t\t    tables_list_buf.buf, tables_list_buf.len);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tunlink(new_table_path.buf);\n \t\tgoto done;\n \t}\n \n-\terr = close(lock_file_fd);\n-\tlock_file_fd = -1;\n+\terr = fsync_component(FSYNC_COMPONENT_REFERENCE, get_lock_file_fd(&tables_list_lock));\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tunlink(new_table_path.buf);\n \t\tgoto done;\n \t}\n \n-\terr = rename(lock_file_name.buf, st->list_file);\n+\terr = commit_lock_file(&tables_list_lock);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tunlink(new_table_path.buf);\n \t\tgoto done;\n \t}\n-\thave_lock = 0;\n \n-\t/* Reload the stack before deleting. On windows, we can only delete the\n-\t   files after we closed them.\n-\t*/\n+\t/*\n+\t * Reload the stack before deleting the compacted tables. We can only\n+\t * delete the files after we closed them on Windows, so this needs to\n+\t * happen first.\n+\t */\n \terr = reftable_stack_reload_maybe_reuse(st, first < last);\n+\tif (err < 0)\n+\t\tgoto done;\n \n-\tlistp = delete_on_success;\n-\twhile (*listp) {\n-\t\tif (strcmp(*listp, new_table_path.buf)) {\n-\t\t\tunlink(*listp);\n-\t\t}\n-\t\tlistp++;\n+\t/*\n+\t * Delete the old tables. They may still be in use by concurrent\n+\t * readers, so it is expected that unlinking tables may fail.\n+\t */\n+\tfor (i = first; i <= last; i++) {\n+\t\tstruct lock_file *table_lock = &table_locks[i - first];\n+\t\tchar *table_path = get_locked_file_path(table_lock);\n+\t\tunlink(table_path);\n+\t\tfree(table_path);\n \t}\n \n done:\n-\tfree_names(delete_on_success);\n+\trollback_lock_file(&tables_list_lock);\n+\tfor (i = first; table_locks && i <= last; i++)\n+\t\trollback_lock_file(&table_locks[i - first]);\n+\treftable_free(table_locks);\n \n-\tif (subtable_locks) {\n-\t\tlistp = subtable_locks;\n-\t\twhile (*listp) {\n-\t\t\tunlink(*listp);\n-\t\t\tlistp++;\n-\t\t}\n-\t\tfree_names(subtable_locks);\n-\t}\n-\tif (lock_file_fd >= 0) {\n-\t\tclose(lock_file_fd);\n-\t\tlock_file_fd = -1;\n-\t}\n-\tif (have_lock) {\n-\t\tunlink(lock_file_name.buf);\n-\t}\n \tstrbuf_release(&new_table_name);\n \tstrbuf_release(&new_table_path);\n-\tstrbuf_release(&ref_list_contents);\n-\tstrbuf_release(&temp_tab_file_name);\n-\tstrbuf_release(&lock_file_name);\n+\tstrbuf_release(&new_table_temp_path);\n+\tstrbuf_release(&tables_list_buf);\n+\tstrbuf_release(&table_name);\n \treturn err;\n }\n \ndiff --git a/reftable/system.h b/reftable/system.h\nindex 6b74a81514..5d8b6dede5 100644\n--- a/reftable/system.h\n+++ b/reftable/system.h\n@@ -12,7 +12,9 @@ license that can be found in the LICENSE file or at\n /* This header glues the reftable library to the rest of Git */\n \n #include \"git-compat-util.h\"\n+#include \"lockfile.h\"\n #include \"strbuf.h\"\n+#include \"tempfile.h\"\n #include \"hash-ll.h\" /* hash ID, sizes.*/\n #include \"dir.h\" /* remove_dir_recursively, for tests.*/\n \n-- \n2.44.0\n\n"},{"id":"490179","messageId":"4023d78f084c8df7ffd6c5ed68b8d0eadc713fd0.1709816483.git.ps@pks.im","threadId":"61046","inReplyTo":"cover.1709816483.git.ps@pks.im","subject":"[PATCH v2 4/4] reftable/stack: register compacted tables as tempfiles","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-07T13:10:43Z","receivedAt":"2024-03-07T13:10:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We do not register tables resulting from stack compaction with the\ntempfile API. Those tables will thus not be deleted in case Git gets\nkilled.\n\nRefactor the code to register compacted tables as tempfiles.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 54 +++++++++++++++++++++++++++---------------------\n 1 file changed, 30 insertions(+), 24 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 977336b7d5..1ecf1b9751 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -827,51 +827,56 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)\n \n static int stack_compact_locked(struct reftable_stack *st,\n \t\t\t\tsize_t first, size_t last,\n-\t\t\t\tstruct strbuf *temp_tab,\n-\t\t\t\tstruct reftable_log_expiry_config *config)\n+\t\t\t\tstruct reftable_log_expiry_config *config,\n+\t\t\t\tstruct tempfile **tab_file_out)\n {\n \tstruct strbuf next_name = STRBUF_INIT;\n-\tint tab_fd = -1;\n+\tstruct strbuf tab_file_path = STRBUF_INIT;\n \tstruct reftable_writer *wr = NULL;\n-\tint err = 0;\n+\tstruct tempfile *tab_file;\n+\tint tab_fd, err = 0;\n \n \tformat_name(&next_name,\n \t\t    reftable_reader_min_update_index(st->readers[first]),\n \t\t    reftable_reader_max_update_index(st->readers[last]));\n+\tstack_filename(&tab_file_path, st, next_name.buf);\n+\tstrbuf_addstr(&tab_file_path, \".temp.XXXXXX\");\n \n-\tstack_filename(temp_tab, st, next_name.buf);\n-\tstrbuf_addstr(temp_tab, \".temp.XXXXXX\");\n+\ttab_file = mks_tempfile(tab_file_path.buf);\n+\tif (!tab_file) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n+\t}\n+\ttab_fd = get_tempfile_fd(tab_file);\n \n-\ttab_fd = mkstemp(temp_tab->buf);\n \tif (st->config.default_permissions &&\n-\t    chmod(temp_tab->buf, st->config.default_permissions) < 0) {\n+\t    chmod(get_tempfile_path(tab_file), st->config.default_permissions) < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n \n-\twr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd, &st->config);\n-\n+\twr = reftable_new_writer(reftable_fd_write, reftable_fd_flush,\n+\t\t\t\t &tab_fd, &st->config);\n \terr = stack_write_compact(st, wr, first, last, config);\n \tif (err < 0)\n \t\tgoto done;\n+\n \terr = reftable_writer_close(wr);\n \tif (err < 0)\n \t\tgoto done;\n \n-\terr = close(tab_fd);\n-\ttab_fd = 0;\n+\terr = close_tempfile_gently(tab_file);\n+\tif (err < 0)\n+\t\tgoto done;\n+\n+\t*tab_file_out = tab_file;\n+\ttab_file = NULL;\n \n done:\n+\tdelete_tempfile(&tab_file);\n \treftable_writer_free(wr);\n-\tif (tab_fd > 0) {\n-\t\tclose(tab_fd);\n-\t\ttab_fd = 0;\n-\t}\n-\tif (err != 0 && temp_tab->len > 0) {\n-\t\tunlink(temp_tab->buf);\n-\t\tstrbuf_release(temp_tab);\n-\t}\n \tstrbuf_release(&next_name);\n+\tstrbuf_release(&tab_file_path);\n \treturn err;\n }\n \n@@ -979,12 +984,12 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\t\t       struct reftable_log_expiry_config *expiry)\n {\n \tstruct strbuf tables_list_buf = STRBUF_INIT;\n-\tstruct strbuf new_table_temp_path = STRBUF_INIT;\n \tstruct strbuf new_table_name = STRBUF_INIT;\n \tstruct strbuf new_table_path = STRBUF_INIT;\n \tstruct strbuf table_name = STRBUF_INIT;\n \tstruct lock_file tables_list_lock = LOCK_INIT;\n \tstruct lock_file *table_locks = NULL;\n+\tstruct tempfile *new_table = NULL;\n \tint is_empty_table = 0, err = 0;\n \tsize_t i;\n \n@@ -1059,7 +1064,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * these tables may end up with an empty new table in case tombstones\n \t * end up cancelling out all refs in that range.\n \t */\n-\terr = stack_compact_locked(st, first, last, &new_table_temp_path, expiry);\n+\terr = stack_compact_locked(st, first, last, expiry, &new_table);\n \tif (err < 0) {\n \t\tif (err != REFTABLE_EMPTY_TABLE_ERROR)\n \t\t\tgoto done;\n@@ -1099,7 +1104,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\tstrbuf_addstr(&new_table_name, \".ref\");\n \t\tstack_filename(&new_table_path, st, new_table_name.buf);\n \n-\t\terr = rename(new_table_temp_path.buf, new_table_path.buf);\n+\t\terr = rename_tempfile(&new_table, new_table_path.buf);\n \t\tif (err < 0) {\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n@@ -1166,9 +1171,10 @@ static int stack_compact_range(struct reftable_stack *st,\n \t\trollback_lock_file(&table_locks[i - first]);\n \treftable_free(table_locks);\n \n+\tdelete_tempfile(&new_table);\n \tstrbuf_release(&new_table_name);\n \tstrbuf_release(&new_table_path);\n-\tstrbuf_release(&new_table_temp_path);\n+\n \tstrbuf_release(&tables_list_buf);\n \tstrbuf_release(&table_name);\n \treturn err;\n-- \n2.44.0\n\n"},{"id":"490196","messageId":"xmqqo7bpncrt.fsf@gitster.g","threadId":"61046","inReplyTo":"Zelb8ldHh4Lnlh7Z@tanuki","subject":"Re: [PATCH 2/4] reftable/stack: register new tables as tempfiles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-07T17:59:50Z","receivedAt":"2024-03-07T17:59:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> It is consistent. The problem is rather that `mks_tempfile_m()` takes a\n> mode as input, but still ends up applying the umask to that mode.\n\nAh, OK.  \n\n> Thus,\n> using that function without a subsequent call to chmod(3P) would end up\n> mishandling \"core.sharedRepository\".\n\nIt is understandable because this is meant for tempfile; you want to\ncreate it, expect to use it yourself and not by others, before you\nare done with it.  There is no place in this sequence where you need\nto open access up to those who share access to the repository.\n\nThe rest of the message is primarily for my own education.\n\nI got a bit curious how other parts of the system does this.  For\nexample, writing an loose object calls git_mkstemp_mode() with 0444\nand then finalize_object_file() calls adjust_shared_perm() after it\nrenames the file to its final name at the end.  A tight umask like\n0700 may demote the original 0444 to 0400, but adjust_shared_perm()\nexpands the 0400 appropriately.\n\nThe index file is more interesting.  We bypass the whole mkstemp\nthing (as we know the final filename and add .lock after it), and\nthe call chain looks like this:\n\n    repo_refresh_and_write_index() ->\n      repo_hold_locked_index() ->\n        hold_lock_file_for_update() ->\n          lock_file() ->\n            create_tempfile_mode() ->\n              open()\n              activate_tempfile()\n              adjust_shared_perm()\n      refresh_index()\n      write_locked_index() ->\n        commit_locked_index() ->\n          commit_lock_file_to() ->\n            rename_tempfile().\n\nAfter we create the file with open() and before returning to the\ncaller, hold_lock_file_for_update() already sets the permission\nbits correctly, so the \"committing\" phase to rename the written\nlockfile into its final place becomes merely a rename without any\nfutzing with permission bits.\n\nSo in short, for a file that we intend to create, write, and then\ncommit to the final name, we use at least two approaches:\n\n - Let mkstemp_mode() do its thing, and fix the permission bits\n   later with adjust_shared_perm().\n\n - Let hold_lock_file_for_update() take care of the permission bits.\n\nI sense there might be some clean-up opportunities around here.\nAfter all, lockfile is (or at least pretends to be) built on top of\ntempfile, and it is for more permanent (as opposed to temporary)\nfiles, but it somehow wasn't a good fit to wrap new tables in this\nseries?\n\nThanks.\n"},{"id":"490219","messageId":"Zeopa57o1fMxPoZg@framework","threadId":"61046","inReplyTo":"xmqqo7bpncrt.fsf@gitster.g","subject":"Re: [PATCH 2/4] reftable/stack: register new tables as tempfiles","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-07T20:54:03Z","receivedAt":"2024-03-07T20:54:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 07, 2024 at 09:59:50AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n[snip]\n> I sense there might be some clean-up opportunities around here.\n> After all, lockfile is (or at least pretends to be) built on top of\n> tempfile, and it is for more permanent (as opposed to temporary)\n> files, but it somehow wasn't a good fit to wrap new tables in this\n> series?\n\nWell, I didn't think lockfiles are a good fit here. I did convert\n\"tables.list\" to use a lock because it's a natural fit given that we\nwant to use \"table.list.lock\". But newly written tables aren't written\nto a file with \".lock\" suffix, but instead to a file ending with\n\".temp.XXXXXX\". This is intentionally so that two processes can write\nnew tables at the same point in time, even though two concurrent writes\nwill end up being mutually exclusive.\n\nAs lockfiles to me are rather about mutually exclusive locking I think\nthat using tempfiles directly is preferable. As far as I can see there\nis also no real benefit with lockfiles in our context, except for the\nmode handling. But given that we have \"default_permissions\" I'd say it\nis preferable to consistently use these for now via chmod(3P).\n\nI ain't got any strong opinions on this though, I'm rather being\npragmatic. So if there is a good reason to use lockfiles that I missed\nthen I wouldn't mind converting the code.\n\nPatrick\n"},{"id":"490220","messageId":"xmqqwmqdkb0d.fsf@gitster.g","threadId":"61046","inReplyTo":"Zeopa57o1fMxPoZg@framework","subject":"Re: [PATCH 2/4] reftable/stack: register new tables as tempfiles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-07T21:06:10Z","receivedAt":"2024-03-07T21:06:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Thu, Mar 07, 2024 at 09:59:50AM -0800, Junio C Hamano wrote:\n>> Patrick Steinhardt <ps@pks.im> writes:\n> [snip]\n>> I sense there might be some clean-up opportunities around here.\n>> After all, lockfile is (or at least pretends to be) built on top of\n>> tempfile, and it is for more permanent (as opposed to temporary)\n>> files, but it somehow wasn't a good fit to wrap new tables in this\n>> series?\n> ...\n> As lockfiles to me are rather about mutually exclusive locking I think\n> that using tempfiles directly is preferable. As far as I can see there\n> is also no real benefit with lockfiles in our context, except for the\n> mode handling. But given that we have \"default_permissions\" I'd say it\n> is preferable to consistently use these for now via chmod(3P).\n\nI wasn't talking about the use of temp or lock API in _this_ series,\nbut was talking about the different permission bit handling between\nthe two API.  The loose object codepath that uses the tempfile API\nis closer to what you are doing, which suffers from the same \"at\ncreation tempfile API does not bother with permission bits because\nit may be removed at the end\".  The index codepath that uses the\nhold_lock_file_for_update() does not have to care, as it gets the\npermission right from the start.\n\nBecause of these differences, the loose object codepath has to do\nthe adjust_perm_bits() itself, and similarly, you have to fix the\npermission the same.\n\nThese callers may become simpler if we give an option to the\ngit_mkstemps_mode() to return a file whose permission bits are\nalready correct from the start.\n\n"}]}