{"thread":{"id":"60784","subject":"[PATCH] reftable: honor core.fsync","startedAt":"2024-01-23T18:51:14Z","lastAt":"2024-01-29T17:15:14Z","messageCount":10,"participants":["John Cai via GitGitGadget","Junio C Hamano","Kristoffer Haugsbakk","John Cai","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"487279","messageId":"pull.1654.git.git.1706035870956.gitgitgadget@gmail.com","threadId":"60784","inReplyTo":null,"subject":"[PATCH] reftable: honor core.fsync","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-23T18:51:10Z","receivedAt":"2024-01-23T18:51:14Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nWhile the reffiles backend honors configured fsync settings, the\nreftable backend does not. Address this by fsyncing reftable files using\nthe write-or-die api's fsync_component() in two places: when we\nadd additional entries into the table, and when we close the reftable\nwriter.\n\nThis commits adds a flush function pointer as a new member of\nreftable_writer because we are not sure that the first argument to the\n*write function pointer always contains a file descriptor. In the case of\nstrbuf_add_void, the first argument is a buffer. This way, we can pass\nin a corresponding flush function that knows how to flush depending on\nwhich writer is being used.\n\nThis patch does not contain tests as they will need to wait for another\npatch to start to exercise the reftable backend. At that point, the\ntests will be added to observe that fsyncs are happening when the\nreftable is in use.\n\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n    reftable: honor core.fsync\n    \n    While the reffiles backend honors configured fsync settings, the\n    reftable backend does not. Address this by fsyncing reftable files using\n    the write-or-die api's fsync_component() in two places: when we add\n    additional entries into the table, and when we close the reftable\n    writer.\n    \n    This commits adds a flush function pointer as a new member of\n    reftable_writer because we are not sure that the first argument to the\n    *write function pointer always contains a file descriptor. In the case\n    of strbuf_add_void, the first argument is a buffer. This way, we can\n    pass in a corresponding flush function that knows how to flush depending\n    on which writer is being used.\n    \n    This patch does not contain tests as they will need to wait for another\n    patch to exercise the reftable backend in test. At that point, the tests\n    will be added to observe that fsyncs are happening when the reftable is\n    in use.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1654%2Fjohn-cai%2Fjc%2Ffsync-reftable-write-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1654/john-cai/jc/fsync-reftable-write-v1\nPull-Request: https://github.com/git/git/pull/1654\n\n reftable/merged_test.c     |  6 +++---\n reftable/readwrite_test.c  | 24 ++++++++++++------------\n reftable/refname_test.c    |  2 +-\n reftable/reftable-writer.h |  1 +\n reftable/stack.c           | 16 +++++++++++++---\n reftable/test_framework.c  |  5 +++++\n reftable/test_framework.h  |  2 ++\n reftable/writer.c          |  8 ++++++++\n reftable/writer.h          |  1 +\n 9 files changed, 46 insertions(+), 19 deletions(-)\n\ndiff --git a/reftable/merged_test.c b/reftable/merged_test.c\nindex 46908f738f7..bf090b474ed 100644\n--- a/reftable/merged_test.c\n+++ b/reftable/merged_test.c\n@@ -42,7 +42,7 @@ static void write_test_table(struct strbuf *buf,\n \t\t}\n \t}\n \n-\tw = reftable_new_writer(&strbuf_add_void, buf, &opts);\n+\tw = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);\n \treftable_writer_set_limits(w, min, max);\n \n \tfor (i = 0; i < n; i++) {\n@@ -70,7 +70,7 @@ static void write_test_log_table(struct strbuf *buf,\n \t\t.exact_log_message = 1,\n \t};\n \tstruct reftable_writer *w = NULL;\n-\tw = reftable_new_writer(&strbuf_add_void, buf, &opts);\n+\tw = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);\n \treftable_writer_set_limits(w, update_index, update_index);\n \n \tfor (i = 0; i < n; i++) {\n@@ -412,7 +412,7 @@ static void test_default_write_opts(void)\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &buf, &opts);\n+\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \n \tstruct reftable_ref_record rec = {\n \t\t.refname = \"master\",\ndiff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c\nindex b8a32240164..6b99daeaf2a 100644\n--- a/reftable/readwrite_test.c\n+++ b/reftable/readwrite_test.c\n@@ -51,7 +51,7 @@ static void write_table(char ***names, struct strbuf *buf, int N,\n \t\t.hash_id = hash_id,\n \t};\n \tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, buf, &opts);\n+\t\treftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);\n \tstruct reftable_ref_record ref = { NULL };\n \tint i = 0, n;\n \tstruct reftable_log_record log = { NULL };\n@@ -130,7 +130,7 @@ static void test_log_buffer_size(void)\n \t\t\t\t\t   .message = \"commit: 9\\n\",\n \t\t\t\t   } } };\n \tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &buf, &opts);\n+\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \n \t/* This tests buffer extension for log compression. Must use a random\n \t   hash, to ensure that the compressed part is larger than the original.\n@@ -171,7 +171,7 @@ static void test_log_overflow(void)\n \t\t\t\t\t   .message = msg,\n \t\t\t\t   } } };\n \tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &buf, &opts);\n+\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \n \tuint8_t hash1[GIT_SHA1_RAWSZ]  = {1}, hash2[GIT_SHA1_RAWSZ] = { 2 };\n \n@@ -202,7 +202,7 @@ static void test_log_write_read(void)\n \tstruct reftable_block_source source = { NULL };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &buf, &opts);\n+\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \tconst struct reftable_stats *stats = NULL;\n \treftable_writer_set_limits(w, 0, N);\n \tfor (i = 0; i < N; i++) {\n@@ -294,7 +294,7 @@ static void test_log_zlib_corruption(void)\n \tstruct reftable_block_source source = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &buf, &opts);\n+\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \tconst struct reftable_stats *stats = NULL;\n \tuint8_t hash1[GIT_SHA1_RAWSZ] = { 1 };\n \tuint8_t hash2[GIT_SHA1_RAWSZ] = { 2 };\n@@ -535,7 +535,7 @@ static void test_table_refs_for(int indexed)\n \n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &buf, &opts);\n+\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \n \tstruct reftable_iterator it = { NULL };\n \tint j;\n@@ -628,7 +628,7 @@ static void test_write_empty_table(void)\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &buf, &opts);\n+\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \tstruct reftable_block_source source = { NULL };\n \tstruct reftable_reader *rd = NULL;\n \tstruct reftable_ref_record rec = { NULL };\n@@ -666,7 +666,7 @@ static void test_write_object_id_min_length(void)\n \t};\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &buf, &opts);\n+\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \tstruct reftable_ref_record ref = {\n \t\t.update_index = 1,\n \t\t.value_type = REFTABLE_REF_VAL1,\n@@ -701,7 +701,7 @@ static void test_write_object_id_length(void)\n \t};\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &buf, &opts);\n+\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \tstruct reftable_ref_record ref = {\n \t\t.update_index = 1,\n \t\t.value_type = REFTABLE_REF_VAL1,\n@@ -735,7 +735,7 @@ static void test_write_empty_key(void)\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &buf, &opts);\n+\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \tstruct reftable_ref_record ref = {\n \t\t.refname = \"\",\n \t\t.update_index = 1,\n@@ -758,7 +758,7 @@ static void test_write_key_order(void)\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &buf, &opts);\n+\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \tstruct reftable_ref_record refs[2] = {\n \t\t{\n \t\t\t.refname = \"b\",\n@@ -801,7 +801,7 @@ static void test_write_multiple_indices(void)\n \tstruct reftable_reader *reader;\n \tint err, i;\n \n-\twriter = reftable_new_writer(&strbuf_add_void, &writer_buf, &opts);\n+\twriter = reftable_new_writer(&strbuf_add_void, &noop_flush, &writer_buf, &opts);\n \treftable_writer_set_limits(writer, 1, 1);\n \tfor (i = 0; i < 100; i++) {\n \t\tstruct reftable_ref_record ref = {\ndiff --git a/reftable/refname_test.c b/reftable/refname_test.c\nindex 699e1aea412..b9cc62554ea 100644\n--- a/reftable/refname_test.c\n+++ b/reftable/refname_test.c\n@@ -30,7 +30,7 @@ static void test_conflict(void)\n \tstruct reftable_write_options opts = { 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &buf, &opts);\n+\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \tstruct reftable_ref_record rec = {\n \t\t.refname = \"a/b\",\n \t\t.value_type = REFTABLE_REF_SYMREF,\ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex db8de197f6c..7c7cae5f99b 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -88,6 +88,7 @@ struct reftable_stats {\n /* reftable_new_writer creates a new writer */\n struct reftable_writer *\n reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),\n+\t\t    int (*flush_func)(void *),\n \t\t    void *writer_arg, struct reftable_write_options *opts);\n \n /* Set the range of update indices for the records we will add. When writing a\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 7ffeb3ee107..ab295341cc4 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -8,6 +8,7 @@ license that can be found in the LICENSE file or at\n \n #include \"stack.h\"\n \n+#include \"../write-or-die.h\"\n #include \"system.h\"\n #include \"merged.h\"\n #include \"reader.h\"\n@@ -16,7 +17,6 @@ license that can be found in the LICENSE file or at\n #include \"reftable-record.h\"\n #include \"reftable-merged.h\"\n #include \"writer.h\"\n-\n #include \"tempfile.h\"\n \n static int stack_try_add(struct reftable_stack *st,\n@@ -47,6 +47,13 @@ static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz)\n \treturn write_in_full(*fdp, data, sz);\n }\n \n+static int reftable_fd_flush(void *arg)\n+{\n+\tint *fdp = (int *)arg;\n+\n+\treturn fsync_component(FSYNC_COMPONENT_REFERENCE, *fdp);\n+}\n+\n int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n \t\t       struct reftable_write_options config)\n {\n@@ -545,6 +552,9 @@ int reftable_addition_commit(struct reftable_addition *add)\n \t\tgoto done;\n \t}\n \n+\tfsync_component_or_die(FSYNC_COMPONENT_REFERENCE, lock_file_fd,\n+\t\t\t       get_tempfile_path(add->lock_file));\n+\n \terr = rename_tempfile(&add->lock_file, add->stack->list_file);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n@@ -639,7 +649,7 @@ int reftable_addition_add(struct reftable_addition *add,\n \t\t\tgoto done;\n \t\t}\n \t}\n-\twr = reftable_new_writer(reftable_fd_write, &tab_fd,\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 \tif (err < 0)\n@@ -731,7 +741,7 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last,\n \tstrbuf_addstr(temp_tab, \".temp.XXXXXX\");\n \n \ttab_fd = mkstemp(temp_tab->buf);\n-\twr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config);\n+\twr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd, &st->config);\n \n \terr = stack_write_compact(st, wr, first, last, config);\n \tif (err < 0)\ndiff --git a/reftable/test_framework.c b/reftable/test_framework.c\nindex 04044fc1a0f..4066924eee4 100644\n--- a/reftable/test_framework.c\n+++ b/reftable/test_framework.c\n@@ -20,3 +20,8 @@ ssize_t strbuf_add_void(void *b, const void *data, size_t sz)\n \tstrbuf_add(b, data, sz);\n \treturn sz;\n }\n+\n+int noop_flush(void *arg)\n+{\n+\treturn 0;\n+}\ndiff --git a/reftable/test_framework.h b/reftable/test_framework.h\nindex ee44f735aea..687390f9c23 100644\n--- a/reftable/test_framework.h\n+++ b/reftable/test_framework.h\n@@ -56,4 +56,6 @@ void set_test_hash(uint8_t *p, int i);\n  */\n ssize_t strbuf_add_void(void *b, const void *data, size_t sz);\n \n+int noop_flush(void *);\n+\n #endif\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex ee4590e20f8..92935baa703 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -121,6 +121,7 @@ static struct strbuf reftable_empty_strbuf = STRBUF_INIT;\n \n struct reftable_writer *\n reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),\n+\t\t    int (*flush_func)(void *),\n \t\t    void *writer_arg, struct reftable_write_options *opts)\n {\n \tstruct reftable_writer *wp =\n@@ -136,6 +137,7 @@ reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),\n \twp->write = writer_func;\n \twp->write_arg = writer_arg;\n \twp->opts = *opts;\n+\twp->flush = flush_func;\n \twriter_reinit_block_writer(wp, BLOCK_TYPE_REF);\n \n \treturn wp;\n@@ -603,6 +605,12 @@ int reftable_writer_close(struct reftable_writer *w)\n \tput_be32(p, crc32(0, footer, p - footer));\n \tp += 4;\n \n+\terr = w->flush(w->write_arg);\n+\tif (err < 0) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tgoto done;\n+\t}\n+\n \terr = padded_write(w, footer, footer_size(writer_version(w)), 0);\n \tif (err < 0)\n \t\tgoto done;\ndiff --git a/reftable/writer.h b/reftable/writer.h\nindex 09b88673d97..8d0df9cc528 100644\n--- a/reftable/writer.h\n+++ b/reftable/writer.h\n@@ -16,6 +16,7 @@ license that can be found in the LICENSE file or at\n \n struct reftable_writer {\n \tssize_t (*write)(void *, const void *, size_t);\n+\tint (*flush)(void *);\n \tvoid *write_arg;\n \tint pending_padding;\n \tstruct strbuf last_key;\n\nbase-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c\n-- \ngitgitgadget\n"},{"id":"487280","messageId":"xmqq34unn8x4.fsf@gitster.g","threadId":"60784","inReplyTo":"pull.1654.git.git.1706035870956.gitgitgadget@gmail.com","subject":"Re: [PATCH] reftable: honor core.fsync","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-23T19:31:35Z","receivedAt":"2024-01-23T19:31:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> This commits adds a flush function pointer as a new member of\n> reftable_writer because we are not sure that the first argument to the\n> *write function pointer always contains a file descriptor. In the case of\n> strbuf_add_void, the first argument is a buffer. This way, we can pass\n> in a corresponding flush function that knows how to flush depending on\n> which writer is being used.\n\nA comment and a half.\n\n * Can't the new \"how to flush\" go to the write-option structure?\n   If you represent \"no flush\" as a NULL pointer in the flush member,\n   most of the changes to the _test files can go, no?\n\n * For a function\n\n\tint func(int ac, char **av);\n\n   a literal pointer to it can legally be written as either\n\n\tint (*funcp)(int, char **) = &func;\n\tint (*funcp)(int, char **) = func;\n\n   but it is my understanding that this codebase prefers the latter,\n   a tradition which goes back to 2005 when Linus was still writing\n   a lot of code, i.e. the identifier that is the name of the\n   function, without & in front.\n\n"},{"id":"487281","messageId":"200cff64-cf53-4f91-bdf4-5afae2d2a127@app.fastmail.com","threadId":"60784","inReplyTo":"pull.1654.git.git.1706035870956.gitgitgadget@gmail.com","subject":"Re: [PATCH] reftable: honor core.fsync","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-23T21:06:18Z","receivedAt":"2024-01-23T21:07:22Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Tue, Jan 23, 2024, at 19:51, John Cai via GitGitGadget wrote:\n> This commits adds a flush function pointer as a new member of\n\nI guess you meant singular “This commit”?\n\n> This commits adds a flush function pointer as a new member of\n> […]\n> This patch does not contain tests as they will need to wait for another\n\nOut of these two “This commit” is more true for the future `git log`.\n\n-- \nKristoffer Haugsbakk\n"},{"id":"487285","messageId":"51A9851B-418E-47AC-A6FE-A411F39D4D92@gmail.com","threadId":"60784","inReplyTo":"200cff64-cf53-4f91-bdf4-5afae2d2a127@app.fastmail.com","subject":"Re: [PATCH] reftable: honor core.fsync","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2024-01-23T21:38:32Z","receivedAt":"2024-01-23T21:38:34Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Kristoffer,\n\nOn 23 Jan 2024, at 16:06, Kristoffer Haugsbakk wrote:\n\n> On Tue, Jan 23, 2024, at 19:51, John Cai via GitGitGadget wrote:\n>> This commits adds a flush function pointer as a new member of\n>\n> I guess you meant singular “This commit”?\n>\n>> This commits adds a flush function pointer as a new member of\n>> […]\n>> This patch does not contain tests as they will need to wait for another\n>\n> Out of these two “This commit” is more true for the future `git log`.\n\nyes this was a typo, thanks for catching that!\n\n>\n> -- \n> Kristoffer Haugsbakk\n\nthanks\nJohn\n"},{"id":"487286","messageId":"0F4C94D3-B81E-4E88-B5F7-7A3746A6DB79@gmail.com","threadId":"60784","inReplyTo":"xmqq34unn8x4.fsf@gitster.g","subject":"Re: [PATCH] reftable: honor core.fsync","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2024-01-23T21:42:15Z","receivedAt":"2024-01-23T21:42:17Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 23 Jan 2024, at 14:31, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n>> This commits adds a flush function pointer as a new member of\n>> reftable_writer because we are not sure that the first argument to the\n>> *write function pointer always contains a file descriptor. In the case of\n>> strbuf_add_void, the first argument is a buffer. This way, we can pass\n>> in a corresponding flush function that knows how to flush depending on\n>> which writer is being used.\n>\n> A comment and a half.\n>\n>  * Can't the new \"how to flush\" go to the write-option structure?\n>    If you represent \"no flush\" as a NULL pointer in the flush member,\n>    most of the changes to the _test files can go, no?\n\nThat's a good option and cuts down on code changes. Thanks for the suggestion.\n\n>\n>  * For a function\n>\n> \tint func(int ac, char **av);\n>\n>    a literal pointer to it can legally be written as either\n>\n> \tint (*funcp)(int, char **) = &func;\n> \tint (*funcp)(int, char **) = func;\n>\n>    but it is my understanding that this codebase prefers the latter,\n>    a tradition which goes back to 2005 when Linus was still writing\n>    a lot of code, i.e. the identifier that is the name of the\n>    function, without & in front.\n\ngood to know, thanks\n\nJohn\n"},{"id":"487287","messageId":"xmqqsf2nlnxv.fsf@gitster.g","threadId":"60784","inReplyTo":"xmqq34unn8x4.fsf@gitster.g","subject":"Re: [PATCH] reftable: honor core.fsync","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-23T21:50:04Z","receivedAt":"2024-01-23T21:50:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> A comment and a half.\n>\n>  * Can't the new \"how to flush\" go to the write-option structure?\n>    If you represent \"no flush\" as a NULL pointer in the flush member,\n>    most of the changes to the _test files can go, no?\n\nNah, that was a stupid comment.  These are used to populate the\nmembers of the reftable_writer instance being created, and it does\nmake sense to have flush_func immediately next to writer_func.\n\nThe part about using NULL as the value to say \"do not use any flusher\"\nstill stands, though.  You do not have to expose noop_flush into the\nglobal namespace that way.\n\nThanks.\n"},{"id":"487310","messageId":"ZbDNVouHgr-J2ptC@tanuki","threadId":"60784","inReplyTo":"xmqqsf2nlnxv.fsf@gitster.g","subject":"Re: [PATCH] reftable: honor core.fsync","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-24T08:41:58Z","receivedAt":"2024-01-24T08:42:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 23, 2024 at 01:50:04PM -0800, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > A comment and a half.\n> >\n> >  * Can't the new \"how to flush\" go to the write-option structure?\n> >    If you represent \"no flush\" as a NULL pointer in the flush member,\n> >    most of the changes to the _test files can go, no?\n> \n> Nah, that was a stupid comment.  These are used to populate the\n> members of the reftable_writer instance being created, and it does\n> make sense to have flush_func immediately next to writer_func.\n\nAgreed (not on the \"stupid\" part, on having it next to `writer_func`).\n\n> The part about using NULL as the value to say \"do not use any flusher\"\n> still stands, though.  You do not have to expose noop_flush into the\n> global namespace that way.\n\nOne benefit of explicitly using the `noop_flush()` function is that we\nmake sure that all callsites that should provide a proper flushing\nfunction indeed do. A `noop_flush` in production code may raise some\neyebrows, whereas a `NULL` value could easily be overlooked.\n\nWhether that is a good enough reason for the additional churn might be a\ndifferent question. I don't think it's particularly bad though.\n\nPatrick\n"},{"id":"487339","messageId":"xmqq4jf2hcif.fsf@gitster.g","threadId":"60784","inReplyTo":"ZbDNVouHgr-J2ptC@tanuki","subject":"Re: [PATCH] reftable: honor core.fsync","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-24T17:22:48Z","receivedAt":"2024-01-24T17:22:54Z","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>> The part about using NULL as the value to say \"do not use any flusher\"\n>> still stands, though.  You do not have to expose noop_flush into the\n>> global namespace that way.\n>\n> One benefit of explicitly using the `noop_flush()` function is that we\n> make sure that all callsites that should provide a proper flushing\n> function indeed do. A `noop_flush` in production code may raise some\n> eyebrows, whereas a `NULL` value could easily be overlooked.\n\nVery true.  Another benefit is that at runtime we do not need any\nconditional deep inside the logic that calls the .flush method of\nthe writer object.\n"},{"id":"487495","messageId":"Zbd0i9nOeWWNQ2EW@tanuki","threadId":"60784","inReplyTo":"pull.1654.git.git.1706035870956.gitgitgadget@gmail.com","subject":"Re: [PATCH] reftable: honor core.fsync","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-29T09:48:59Z","receivedAt":"2024-01-29T09:49:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 23, 2024 at 06:51:10PM +0000, John Cai via GitGitGadget wrote:\n> From: John Cai <johncai86@gmail.com>\n> \n> While the reffiles backend honors configured fsync settings, the\n> reftable backend does not. Address this by fsyncing reftable files using\n> the write-or-die api's fsync_component() in two places: when we\n> add additional entries into the table, and when we close the reftable\n> writer.\n> \n> This commits adds a flush function pointer as a new member of\n> reftable_writer because we are not sure that the first argument to the\n> *write function pointer always contains a file descriptor. In the case of\n> strbuf_add_void, the first argument is a buffer. This way, we can pass\n> in a corresponding flush function that knows how to flush depending on\n> which writer is being used.\n> \n> This patch does not contain tests as they will need to wait for another\n> patch to start to exercise the reftable backend. At that point, the\n> tests will be added to observe that fsyncs are happening when the\n> reftable is in use.\n> \n> Signed-off-by: John Cai <johncai86@gmail.com>\n\nI noticed that we missed syncing the \"tables.list\" file when performing\nauto-compaction. The below patch is needed on top of what we already\nhave.\n\nThe topic is currently in `next`, but not yet in `master`, so we might\nstill squash it in. Junio, please let me know whether you want to do so\nor whether I shall send this fix-up as a new patch. Thanks!\n\nPatrick\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ab295341cc..b17cfb9516 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -1018,6 +1018,10 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n \t\tunlink(new_table_path.buf);\n \t\tgoto done;\n \t}\n+\n+\tfsync_component_or_die(FSYNC_COMPONENT_REFERENCE, lock_file_fd,\n+\t\t\t       lock_file_name.buf);\n+\n \terr = close(lock_file_fd);\n \tlock_file_fd = -1;\n \tif (err < 0) {\n"},{"id":"487527","messageId":"xmqqttmwjc2x.fsf@gitster.g","threadId":"60784","inReplyTo":"Zbd0i9nOeWWNQ2EW@tanuki","subject":"Re: [PATCH] reftable: honor core.fsync","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-29T17:15:02Z","receivedAt":"2024-01-29T17:15:14Z","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> The topic is currently in `next`, but not yet in `master`, so we might\n> still squash it in. Junio, please let me know whether you want to do so\n> or whether I shall send this fix-up as a new patch. Thanks!\n\nAny commit in 'next' gets improved only by piling incremental\nupdates on top with explanation (the idea is: if all of us thought\nit has been seen enough eyeballs and yet we later find there was\nsomething we all missed, that is worth a separate explanation---the\nprimary motivation of the change still was good, but for such and\nsuch reasons we missed this case), unless it turns out that the\napproach was fundamentally wrong and such an incremental update\nboils down to almost reverting the earlier and replacing with the\nnewer (in which case, we do revert the earlier and replace it with\nthe newer, in 'next').\n\nThanks.\n"}]}