reftable: honor core.fsync

PATCH In review John Cai via GitGitGadget 5 participants 10 messages Jan 23, 2024 – Jan 29, 2024

lore

Share & tools

lore

John Cai via GitGitGadget

author

2024-01-23 18:51 UTC #0 lore raw

PATCH 1/1 reftable: honor core.fsync

From: John Cai <[email protected]>

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 <[email protected]>

---
reftable: honor core.fsync

This patch does not contain tests as they will need to wait for another patch to exercise the reftable backend in test. At that point, the tests will be added to observe that fsyncs are happening when the reftable is in use.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1654%2Fjohn-cai%2Fjc%2Ffsync-reftable-write-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1654/john-cai/jc/fsync-reftable-write-v1
Pull-Request: https://github.com/git/git/pull/1654

 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(-)

reftable/merged_test.c +3 -3

index 46908f738f7..bf090b474ed 100644
@@ -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",

reftable/readwrite_test.c +12 ~~-12~~

index b8a32240164..6b99daeaf2a 100644
@@ -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 = {

reftable/refname_test.c +1 -1

index 699e1aea412..b9cc62554ea 100644
@@ -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,

reftable/reftable-writer.h +1 -0

index db8de197f6c..7c7cae5f99b 100644
@@ -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

reftable/stack.c +13 -3

index 7ffeb3ee107..ab295341cc4 100644
@@ -8,6 +8,7 @@ license that can be found in the LICENSE file or at
 
 #include "stack.h"
 
+#include "../write-or-die.h"
 #include "system.h"
 #include "merged.h"
 #include "reader.h"

@@ -16,7 +17,6 @@ license that can be found in the LICENSE file or at
 #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)

reftable/test_framework.c +5 -0

index 04044fc1a0f..4066924eee4 100644
@@ -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;
+}

reftable/test_framework.h +2 -0

index ee44f735aea..687390f9c23 100644
@@ -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

reftable/writer.c +8 -0

index ee4590e20f8..92935baa703 100644
@@ -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;

reftable/writer.h +1 -0

index 09b88673d97..8d0df9cc528 100644
@@ -16,6 +16,7 @@ license that can be found in the LICENSE file or at
 
 struct reftable_writer {
 	ssize_t (*write)(void *, const void *, size_t);
+	int (*flush)(void *);
 	void *write_arg;
 	int pending_padding;
 	struct strbuf last_key;

base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c

Signature · 1 line

gitgitgadget

Junio C Hamano

2024-01-23 19:31 UTC #1 lore raw

Quoted 6 lines · John Cai via GitGitGadget

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.

A comment and a half.

* Can't the new "how to flush" go to the write-option structure? If you represent "no flush" as a NULL pointer in the flush member, most of the changes to the _test files can go, no?

* For a function

	int func(int ac, char **av);

a literal pointer to it can legally be written as either

	int (*funcp)(int, char **) = &func;
	int (*funcp)(int, char **) = func;

but it is my understanding that this codebase prefers the latter, a tradition which goes back to 2005 when Linus was still writing a lot of code, i.e. the identifier that is the name of the function, without & in front.

Kristoffer Haugsbakk

2024-01-23 21:07 UTC #6 lore raw

In reply to John Cai via GitGitGadget

Quoted 1 line · John Cai via GitGitGadget

This commits adds a flush function pointer as a new member of

I guess you meant singular “This commit”?

Quoted 3 lines

This commits adds a flush function pointer as a new member of […]
This patch does not contain tests as they will need to wait for another

Out of these two “This commit” is more true for the future git log.

Signature · 1 line

Kristoffer Haugsbakk

John Cai

2024-01-23 21:38 UTC #7 lore raw

Hi Kristoffer,

Quoted 10 lines · Kristoffer Haugsbakk

Quoted 1 line · John Cai via GitGitGadget
This commits adds a flush function pointer as a new member of
I guess you meant singular “This commit”?
Quoted 3 lines
This commits adds a flush function pointer as a new member of […]
This patch does not contain tests as they will need to wait for another
Out of these two “This commit” is more true for the future git log.

yes this was a typo, thanks for catching that!

Quoted 3 lines

Signature · 2 lines
-- 
Kristoffer Haugsbakk

thanks
John

John Cai

2024-01-23 21:42 UTC #2 lore raw

In reply to Junio C Hamano

Hi Junio,

Quoted 9 lines · Junio C Hamano

Quoted 6 lines · John Cai via GitGitGadget
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.

Quoted 5 lines

A comment and a half.
* Can't the new "how to flush" go to the write-option structure? If you represent "no flush" as a NULL pointer in the flush member, most of the changes to the _test files can go, no?

That's a good option and cuts down on code changes. Thanks for the suggestion.

Quoted 7 lines

* For a function
	int func(int ac, char **av);
a literal pointer to it can legally be written as either

Quoted 7 lines

	int (*funcp)(int, char **) = &func;
	int (*funcp)(int, char **) = func;
but it is my understanding that this codebase prefers the latter, a tradition which goes back to 2005 when Linus was still writing a lot of code, i.e. the identifier that is the name of the function, without & in front.

good to know, thanks

John

Junio C Hamano

2024-01-23 21:50 UTC #3 lore raw

In reply to Junio C Hamano

Quoted 5 lines · Junio C Hamano

A comment and a half.
* Can't the new "how to flush" go to the write-option structure? If you represent "no flush" as a NULL pointer in the flush member, most of the changes to the _test files can go, no?

Nah, that was a stupid comment. These are used to populate the members of the reftable_writer instance being created, and it does make sense to have flush_func immediately next to writer_func.

The part about using NULL as the value to say "do not use any flusher" still stands, though. You do not have to expose noop_flush into the global namespace that way.

Thanks.

Patrick Steinhardt

2024-01-24 08:42 UTC #4 lore raw

Quoted 11 lines · Junio C Hamano

Quoted 5 lines · Junio C Hamano
A comment and a half.
* Can't the new "how to flush" go to the write-option structure? If you represent "no flush" as a NULL pointer in the flush member, most of the changes to the _test files can go, no?
Nah, that was a stupid comment. These are used to populate the members of the reftable_writer instance being created, and it does make sense to have flush_func immediately next to writer_func.

Agreed (not on the "stupid" part, on having it next to writer_func).

Quoted 3 lines

The part about using NULL as the value to say "do not use any flusher" still stands, though. You do not have to expose noop_flush into the global namespace that way.

One benefit of explicitly using the noop_flush() function is that we make sure that all callsites that should provide a proper flushing function indeed do. A noop_flush in production code may raise some eyebrows, whereas a NULL value could easily be overlooked.

Whether that is a good enough reason for the additional churn might be a different question. I don't think it's particularly bad though.

Patrick

Junio C Hamano

2024-01-24 17:22 UTC #5 lore raw

Quoted 8 lines · Patrick Steinhardt

Quoted 3 lines
The part about using NULL as the value to say "do not use any flusher" still stands, though. You do not have to expose noop_flush into the global namespace that way.
One benefit of explicitly using the noop_flush() function is that we make sure that all callsites that should provide a proper flushing function indeed do. A noop_flush in production code may raise some eyebrows, whereas a NULL value could easily be overlooked.

Very true. Another benefit is that at runtime we do not need any conditional deep inside the logic that calls the .flush method of the writer object.

Patrick Steinhardt

2024-01-29 09:49 UTC #8 lore raw

In reply to John Cai via GitGitGadget

Quoted 15 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>
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.

Quoted 6 lines

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 <[email protected]>

I noticed that we missed syncing the "tables.list" file when performing auto-compaction. The below patch is needed on top of what we already have.

The topic is currently in next, but not yet in master, so we might still squash it in. Junio, please let me know whether you want to do so or whether I shall send this fix-up as a new patch. Thanks!

Patrick

reftable/stack.c +4 -0

index ab295341cc..b17cfb9516 100644
@@ -1018,6 +1018,10 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,
 		unlink(new_table_path.buf);
 		goto done;
 	}
+
+	fsync_component_or_die(FSYNC_COMPONENT_REFERENCE, lock_file_fd,
+			       lock_file_name.buf);
+
 	err = close(lock_file_fd);
 	lock_file_fd = -1;
 	if (err < 0) {

Junio C Hamano

2024-01-29 17:15 UTC #9 lore raw

Quoted 3 lines · Patrick Steinhardt

The topic is currently in next, but not yet in master, so we might still squash it in. Junio, please let me know whether you want to do so or whether I shall send this fix-up as a new patch. Thanks!

Any commit in 'next' gets improved only by piling incremental updates on top with explanation (the idea is: if all of us thought it has been seen enough eyeballs and yet we later find there was something we all missed, that is worth a separate explanation---the primary motivation of the change still was good, but for such and such reasons we missed this case), unless it turns out that the approach was fundamentally wrong and such an incremental update boils down to almost reverting the earlier and replacing with the newer (in which case, we do revert the earlier and replace it with the newer, in 'next').

Thanks.