From 1df18a1c9a50b58afb19ef218f6517f347606800 Mon Sep 17 00:00:00 2001 From: John Cai Date: Tue, 23 Jan 2024 18:51:10 +0000 Subject: reftable: honor core.fsync While the reffiles backend honors configured fsync settings, the reftable backend does not. Address this by fsyncing reftable files using the write-or-die api's fsync_component() in two places: when we add additional entries into the table, and when we close the reftable writer. This commits adds a flush function pointer as a new member of reftable_writer because we are not sure that the first argument to the *write function pointer always contains a file descriptor. In the case of strbuf_add_void, the first argument is a buffer. This way, we can pass in a corresponding flush function that knows how to flush depending on which writer is being used. This patch does not contain tests as they will need to wait for another patch to start to exercise the reftable backend. At that point, the tests will be added to observe that fsyncs are happening when the reftable is in use. Signed-off-by: John Cai Signed-off-by: Junio C Hamano --- reftable/merged_test.c | 6 +++--- reftable/readwrite_test.c | 24 ++++++++++++------------ reftable/refname_test.c | 2 +- reftable/reftable-writer.h | 1 + reftable/stack.c | 16 +++++++++++++--- reftable/test_framework.c | 5 +++++ reftable/test_framework.h | 2 ++ reftable/writer.c | 8 ++++++++ reftable/writer.h | 1 + 9 files changed, 46 insertions(+), 19 deletions(-) diff --git a/reftable/merged_test.c b/reftable/merged_test.c index 46908f738f..bf090b474e 100644 --- a/reftable/merged_test.c +++ b/reftable/merged_test.c @@ -42,7 +42,7 @@ static void write_test_table(struct strbuf *buf, } } - w = reftable_new_writer(&strbuf_add_void, buf, &opts); + w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts); reftable_writer_set_limits(w, min, max); for (i = 0; i < n; i++) { @@ -70,7 +70,7 @@ static void write_test_log_table(struct strbuf *buf, .exact_log_message = 1, }; struct reftable_writer *w = NULL; - w = reftable_new_writer(&strbuf_add_void, buf, &opts); + w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts); reftable_writer_set_limits(w, update_index, update_index); for (i = 0; i < n; i++) { @@ -412,7 +412,7 @@ static void test_default_write_opts(void) struct reftable_write_options opts = { 0 }; struct strbuf buf = STRBUF_INIT; struct reftable_writer *w = - reftable_new_writer(&strbuf_add_void, &buf, &opts); + reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); struct reftable_ref_record rec = { .refname = "master", diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c index b8a3224016..6b99daeaf2 100644 --- a/reftable/readwrite_test.c +++ b/reftable/readwrite_test.c @@ -51,7 +51,7 @@ static void write_table(char ***names, struct strbuf *buf, int N, .hash_id = hash_id, }; struct reftable_writer *w = - reftable_new_writer(&strbuf_add_void, buf, &opts); + reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts); struct reftable_ref_record ref = { NULL }; int i = 0, n; struct reftable_log_record log = { NULL }; @@ -130,7 +130,7 @@ static void test_log_buffer_size(void) .message = "commit: 9\n", } } }; struct reftable_writer *w = - reftable_new_writer(&strbuf_add_void, &buf, &opts); + reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); /* This tests buffer extension for log compression. Must use a random hash, to ensure that the compressed part is larger than the original. @@ -171,7 +171,7 @@ static void test_log_overflow(void) .message = msg, } } }; struct reftable_writer *w = - reftable_new_writer(&strbuf_add_void, &buf, &opts); + reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); uint8_t hash1[GIT_SHA1_RAWSZ] = {1}, hash2[GIT_SHA1_RAWSZ] = { 2 }; @@ -202,7 +202,7 @@ static void test_log_write_read(void) struct reftable_block_source source = { NULL }; struct strbuf buf = STRBUF_INIT; struct reftable_writer *w = - reftable_new_writer(&strbuf_add_void, &buf, &opts); + reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); const struct reftable_stats *stats = NULL; reftable_writer_set_limits(w, 0, N); for (i = 0; i < N; i++) { @@ -294,7 +294,7 @@ static void test_log_zlib_corruption(void) struct reftable_block_source source = { 0 }; struct strbuf buf = STRBUF_INIT; struct reftable_writer *w = - reftable_new_writer(&strbuf_add_void, &buf, &opts); + reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); const struct reftable_stats *stats = NULL; uint8_t hash1[GIT_SHA1_RAWSZ] = { 1 }; uint8_t hash2[GIT_SHA1_RAWSZ] = { 2 }; @@ -535,7 +535,7 @@ static void test_table_refs_for(int indexed) struct strbuf buf = STRBUF_INIT; struct reftable_writer *w = - reftable_new_writer(&strbuf_add_void, &buf, &opts); + reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); struct reftable_iterator it = { NULL }; int j; @@ -628,7 +628,7 @@ static void test_write_empty_table(void) struct reftable_write_options opts = { 0 }; struct strbuf buf = STRBUF_INIT; struct reftable_writer *w = - reftable_new_writer(&strbuf_add_void, &buf, &opts); + reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); struct reftable_block_source source = { NULL }; struct reftable_reader *rd = NULL; struct reftable_ref_record rec = { NULL }; @@ -666,7 +666,7 @@ static void test_write_object_id_min_length(void) }; struct strbuf buf = STRBUF_INIT; struct reftable_writer *w = - reftable_new_writer(&strbuf_add_void, &buf, &opts); + reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); struct reftable_ref_record ref = { .update_index = 1, .value_type = REFTABLE_REF_VAL1, @@ -701,7 +701,7 @@ static void test_write_object_id_length(void) }; struct strbuf buf = STRBUF_INIT; struct reftable_writer *w = - reftable_new_writer(&strbuf_add_void, &buf, &opts); + reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); struct reftable_ref_record ref = { .update_index = 1, .value_type = REFTABLE_REF_VAL1, @@ -735,7 +735,7 @@ static void test_write_empty_key(void) struct reftable_write_options opts = { 0 }; struct strbuf buf = STRBUF_INIT; struct reftable_writer *w = - reftable_new_writer(&strbuf_add_void, &buf, &opts); + reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); struct reftable_ref_record ref = { .refname = "", .update_index = 1, @@ -758,7 +758,7 @@ static void test_write_key_order(void) struct reftable_write_options opts = { 0 }; struct strbuf buf = STRBUF_INIT; struct reftable_writer *w = - reftable_new_writer(&strbuf_add_void, &buf, &opts); + reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); struct reftable_ref_record refs[2] = { { .refname = "b", @@ -801,7 +801,7 @@ static void test_write_multiple_indices(void) struct reftable_reader *reader; int err, i; - writer = reftable_new_writer(&strbuf_add_void, &writer_buf, &opts); + writer = reftable_new_writer(&strbuf_add_void, &noop_flush, &writer_buf, &opts); reftable_writer_set_limits(writer, 1, 1); for (i = 0; i < 100; i++) { struct reftable_ref_record ref = { diff --git a/reftable/refname_test.c b/reftable/refname_test.c index 699e1aea41..b9cc62554e 100644 --- a/reftable/refname_test.c +++ b/reftable/refname_test.c @@ -30,7 +30,7 @@ static void test_conflict(void) struct reftable_write_options opts = { 0 }; struct strbuf buf = STRBUF_INIT; struct reftable_writer *w = - reftable_new_writer(&strbuf_add_void, &buf, &opts); + reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); struct reftable_ref_record rec = { .refname = "a/b", .value_type = REFTABLE_REF_SYMREF, diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h index db8de197f6..7c7cae5f99 100644 --- a/reftable/reftable-writer.h +++ b/reftable/reftable-writer.h @@ -88,6 +88,7 @@ struct reftable_stats { /* reftable_new_writer creates a new writer */ struct reftable_writer * reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t), + int (*flush_func)(void *), void *writer_arg, struct reftable_write_options *opts); /* Set the range of update indices for the records we will add. When writing a diff --git a/reftable/stack.c b/reftable/stack.c index 7ffeb3ee10..ab295341cc 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -8,6 +8,7 @@ https://developers.google.com/open-source/licenses/bsd #include "stack.h" +#include "../write-or-die.h" #include "system.h" #include "merged.h" #include "reader.h" @@ -16,7 +17,6 @@ https://developers.google.com/open-source/licenses/bsd #include "reftable-record.h" #include "reftable-merged.h" #include "writer.h" - #include "tempfile.h" static int stack_try_add(struct reftable_stack *st, @@ -47,6 +47,13 @@ static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz) return write_in_full(*fdp, data, sz); } +static int reftable_fd_flush(void *arg) +{ + int *fdp = (int *)arg; + + return fsync_component(FSYNC_COMPONENT_REFERENCE, *fdp); +} + int reftable_new_stack(struct reftable_stack **dest, const char *dir, struct reftable_write_options config) { @@ -545,6 +552,9 @@ int reftable_addition_commit(struct reftable_addition *add) goto done; } + fsync_component_or_die(FSYNC_COMPONENT_REFERENCE, lock_file_fd, + get_tempfile_path(add->lock_file)); + err = rename_tempfile(&add->lock_file, add->stack->list_file); if (err < 0) { err = REFTABLE_IO_ERROR; @@ -639,7 +649,7 @@ int reftable_addition_add(struct reftable_addition *add, goto done; } } - wr = reftable_new_writer(reftable_fd_write, &tab_fd, + wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd, &add->stack->config); err = write_table(wr, arg); if (err < 0) @@ -731,7 +741,7 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last, strbuf_addstr(temp_tab, ".temp.XXXXXX"); tab_fd = mkstemp(temp_tab->buf); - wr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config); + wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd, &st->config); err = stack_write_compact(st, wr, first, last, config); if (err < 0) diff --git a/reftable/test_framework.c b/reftable/test_framework.c index 04044fc1a0..4066924eee 100644 --- a/reftable/test_framework.c +++ b/reftable/test_framework.c @@ -20,3 +20,8 @@ ssize_t strbuf_add_void(void *b, const void *data, size_t sz) strbuf_add(b, data, sz); return sz; } + +int noop_flush(void *arg) +{ + return 0; +} diff --git a/reftable/test_framework.h b/reftable/test_framework.h index ee44f735ae..687390f9c2 100644 --- a/reftable/test_framework.h +++ b/reftable/test_framework.h @@ -56,4 +56,6 @@ void set_test_hash(uint8_t *p, int i); */ ssize_t strbuf_add_void(void *b, const void *data, size_t sz); +int noop_flush(void *); + #endif diff --git a/reftable/writer.c b/reftable/writer.c index ee4590e20f..92935baa70 100644 --- a/reftable/writer.c +++ b/reftable/writer.c @@ -121,6 +121,7 @@ static struct strbuf reftable_empty_strbuf = STRBUF_INIT; struct reftable_writer * reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t), + int (*flush_func)(void *), void *writer_arg, struct reftable_write_options *opts) { struct reftable_writer *wp = @@ -136,6 +137,7 @@ reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t), wp->write = writer_func; wp->write_arg = writer_arg; wp->opts = *opts; + wp->flush = flush_func; writer_reinit_block_writer(wp, BLOCK_TYPE_REF); return wp; @@ -603,6 +605,12 @@ int reftable_writer_close(struct reftable_writer *w) put_be32(p, crc32(0, footer, p - footer)); p += 4; + err = w->flush(w->write_arg); + if (err < 0) { + err = REFTABLE_IO_ERROR; + goto done; + } + err = padded_write(w, footer, footer_size(writer_version(w)), 0); if (err < 0) goto done; diff --git a/reftable/writer.h b/reftable/writer.h index 09b88673d9..8d0df9cc52 100644 --- a/reftable/writer.h +++ b/reftable/writer.h @@ -16,6 +16,7 @@ https://developers.google.com/open-source/licenses/bsd struct reftable_writer { ssize_t (*write)(void *, const void *, size_t); + int (*flush)(void *); void *write_arg; int pending_padding; struct strbuf last_key; -- cgit 1.2.3-korg From 7fa52fdad5065382f2b27b14cc0ecd225ea0ce4d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 30 Jan 2024 06:22:47 +0100 Subject: reftable/stack: fsync "tables.list" during compaction In 1df18a1c9a (reftable: honor core.fsync, 2024-01-23), we have added code to fsync both newly written reftables as well as "tables.list" to disk. But there are two code paths where "tables.list" is being written: - When appending a new table due to a normal ref update. - When compacting a range of tables during compaction. We have only addressed the former code path, but do not yet sync the new "tables.list" file in the latter. Fix this omission. Note that we are not yet adding any tests. These tests will be added once the "reftable" backend has been upstreamed. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- reftable/stack.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/reftable/stack.c b/reftable/stack.c index ab295341cc..01d05933f6 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -1018,6 +1018,14 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last, unlink(new_table_path.buf); goto done; } + + err = fsync_component(FSYNC_COMPONENT_REFERENCE, lock_file_fd); + if (err < 0) { + err = REFTABLE_IO_ERROR; + unlink(new_table_path.buf); + goto done; + } + err = close(lock_file_fd); lock_file_fd = -1; if (err < 0) { -- cgit 1.2.3-korg