diff mbox series

[4/7] reftable/stack: stop using `fsync_component()` directly

Message ID 86269fc4fcad9a97709aa0d080c4c077a85ca667.1729677003.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series reftable: stop using Git subsystems | expand

Commit Message

Patrick Steinhardt Oct. 23, 2024, 9:56 a.m. UTC 
We're executing `fsync_component()` directly in the reftable library so
that we can fsync data to disk depending on "core.fsync". But as we're
in the process of converting the reftable library to become standalone
we cannot use that function in the library anymore.

Refactor the code such that users of the library can inject a custom
fsync function via the write options. This allows us to get rid of the
dependency on "write-or-die.h".

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c    |  7 ++++++
 reftable/reftable-writer.h |  6 +++++
 reftable/stack.c           | 49 +++++++++++++++++++++++++-------------
 3 files changed, 45 insertions(+), 17 deletions(-)

Comments

Justin Tobler Nov. 8, 2024, 2:09 a.m. UTC | #1 
On 24/10/23 11:56AM, Patrick Steinhardt wrote:
[snip]
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 9ae716ff375..df4f3237007 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -8,7 +8,6 @@ license that can be found in the LICENSE file or at
>  
>  #include "stack.h"
>  
> -#include "../write-or-die.h"
>  #include "system.h"
>  #include "constants.h"
>  #include "merged.h"
> @@ -43,17 +42,28 @@ static int stack_filename(struct reftable_buf *dest, struct reftable_stack *st,
>  	return 0;
>  }
>  
> -static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz)
> +static int stack_fsync(struct reftable_stack *st, int fd)
>  {
> -	int *fdp = (int *)arg;
> -	return write_in_full(*fdp, data, sz);
> +	if (st->opts.fsync)
> +		return st->opts.fsync(fd);
> +	return fsync(fd);
>  }
>  
> -static int reftable_fd_flush(void *arg)
> +struct fd_writer {
> +	struct reftable_stack *stack;

Out of curiousity, from the stack I think we only need the callback in
the options. Any reason we provide the whole stack here?

> +	int fd;
> +};
> +
> +static ssize_t fd_writer_write(void *arg, const void *data, size_t sz)
>  {
> -	int *fdp = (int *)arg;
> +	struct fd_writer *writer = arg;
> +	return write_in_full(writer->fd, data, sz);
> +}
>  
> -	return fsync_component(FSYNC_COMPONENT_REFERENCE, *fdp);

Previously when the writer was flushed it would invoke
`fsync_component()`. Now that a callback can be configured in the stack
options, the callback needs to also be propagated to
`fd_writer_write()` in addition to the file descriptor being synced.
This explains why `fd_writer` is now used.

The rest of the patch updates writer configuration and fsync call sites.
Looks good.

-Justin
Patrick Steinhardt Nov. 8, 2024, 6:17 a.m. UTC | #2 
On Thu, Nov 07, 2024 at 08:09:21PM -0600, Justin Tobler wrote:
> On 24/10/23 11:56AM, Patrick Steinhardt wrote:
> [snip]
> > diff --git a/reftable/stack.c b/reftable/stack.c
> > index 9ae716ff375..df4f3237007 100644
> > --- a/reftable/stack.c
> > +++ b/reftable/stack.c
> > @@ -43,17 +42,28 @@ static int stack_filename(struct reftable_buf *dest, struct reftable_stack *st,
> >  	return 0;
> >  }
> >  
> > -static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz)
> > +static int stack_fsync(struct reftable_stack *st, int fd)
> >  {
> > -	int *fdp = (int *)arg;
> > -	return write_in_full(*fdp, data, sz);
> > +	if (st->opts.fsync)
> > +		return st->opts.fsync(fd);
> > +	return fsync(fd);
> >  }
> >  
> > -static int reftable_fd_flush(void *arg)
> > +struct fd_writer {
> > +	struct reftable_stack *stack;
> 
> Out of curiousity, from the stack I think we only need the callback in
> the options. Any reason we provide the whole stack here?

I just think that passing around function pointers doesn't make for a
good calling convention here, as it hides the fact that it is possible
to call this without a callback. But there isn't a reason to pass in the
whole stack, it would also be fine to instead pass in e.g. the write
options.

I think I'll do that instead.

Patrick
diff mbox series

Patch

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 7d86d920970..2e774176eda 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -24,6 +24,7 @@ 
 #include "../setup.h"
 #include "../strmap.h"
 #include "../trace2.h"
+#include "../write-or-die.h"
 #include "parse.h"
 #include "refs-internal.h"
 
@@ -273,6 +274,11 @@  static int reftable_be_config(const char *var, const char *value,
 	return 0;
 }
 
+static int reftable_be_fsync(int fd)
+{
+	return fsync_component(FSYNC_COMPONENT_REFERENCE, fd);
+}
+
 static struct ref_store *reftable_be_init(struct repository *repo,
 					  const char *gitdir,
 					  unsigned int store_flags)
@@ -304,6 +310,7 @@  static struct ref_store *reftable_be_init(struct repository *repo,
 	refs->write_options.disable_auto_compact =
 		!git_env_bool("GIT_TEST_REFTABLE_AUTOCOMPACTION", 1);
 	refs->write_options.lock_timeout_ms = 100;
+	refs->write_options.fsync = reftable_be_fsync;
 
 	git_config(reftable_be_config, &refs->write_options);
 
diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
index 211860d08a4..c85ef5a5bd1 100644
--- a/reftable/reftable-writer.h
+++ b/reftable/reftable-writer.h
@@ -62,6 +62,12 @@  struct reftable_write_options {
 	 * negative value will cause us to block indefinitely.
 	 */
 	long lock_timeout_ms;
+
+	/*
+	 * Optional callback used to fsync files to disk. Falls back to using
+	 * fsync(3P) when unset.
+	 */
+	int (*fsync)(int fd);
 };
 
 /* reftable_block_stats holds statistics for a single block type */
diff --git a/reftable/stack.c b/reftable/stack.c
index 9ae716ff375..df4f3237007 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -8,7 +8,6 @@  license that can be found in the LICENSE file or at
 
 #include "stack.h"
 
-#include "../write-or-die.h"
 #include "system.h"
 #include "constants.h"
 #include "merged.h"
@@ -43,17 +42,28 @@  static int stack_filename(struct reftable_buf *dest, struct reftable_stack *st,
 	return 0;
 }
 
-static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz)
+static int stack_fsync(struct reftable_stack *st, int fd)
 {
-	int *fdp = (int *)arg;
-	return write_in_full(*fdp, data, sz);
+	if (st->opts.fsync)
+		return st->opts.fsync(fd);
+	return fsync(fd);
 }
 
-static int reftable_fd_flush(void *arg)
+struct fd_writer {
+	struct reftable_stack *stack;
+	int fd;
+};
+
+static ssize_t fd_writer_write(void *arg, const void *data, size_t sz)
 {
-	int *fdp = (int *)arg;
+	struct fd_writer *writer = arg;
+	return write_in_full(writer->fd, data, sz);
+}
 
-	return fsync_component(FSYNC_COMPONENT_REFERENCE, *fdp);
+static int fd_writer_flush(void *arg)
+{
+	struct fd_writer *writer = arg;
+	return stack_fsync(writer->stack, writer->fd);
 }
 
 int reftable_new_stack(struct reftable_stack **dest, const char *dir,
@@ -765,7 +775,7 @@  int reftable_addition_commit(struct reftable_addition *add)
 		goto done;
 	}
 
-	err = fsync_component(FSYNC_COMPONENT_REFERENCE, lock_file_fd);
+	err = stack_fsync(add->stack, lock_file_fd);
 	if (err < 0) {
 		err = REFTABLE_IO_ERROR;
 		goto done;
@@ -858,8 +868,10 @@  int reftable_addition_add(struct reftable_addition *add,
 	struct reftable_buf next_name = REFTABLE_BUF_INIT;
 	struct reftable_writer *wr = NULL;
 	struct tempfile *tab_file = NULL;
+	struct fd_writer writer = {
+		.stack = add->stack,
+	};
 	int err = 0;
-	int tab_fd;
 
 	reftable_buf_reset(&next_name);
 
@@ -887,10 +899,10 @@  int reftable_addition_add(struct reftable_addition *add,
 			goto done;
 		}
 	}
-	tab_fd = get_tempfile_fd(tab_file);
 
-	err = reftable_writer_new(&wr, reftable_fd_write, reftable_fd_flush,
-				  &tab_fd, &add->stack->opts);
+	writer.fd = get_tempfile_fd(tab_file);
+	err = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,
+				  &writer, &add->stack->opts);
 	if (err < 0)
 		goto done;
 
@@ -973,8 +985,11 @@  static int stack_compact_locked(struct reftable_stack *st,
 	struct reftable_buf next_name = REFTABLE_BUF_INIT;
 	struct reftable_buf tab_file_path = REFTABLE_BUF_INIT;
 	struct reftable_writer *wr = NULL;
+	struct fd_writer writer=  {
+		.stack = st,
+	};
 	struct tempfile *tab_file;
-	int tab_fd, err = 0;
+	int err = 0;
 
 	err = format_name(&next_name, reftable_reader_min_update_index(st->readers[first]),
 			  reftable_reader_max_update_index(st->readers[last]));
@@ -994,7 +1009,6 @@  static int stack_compact_locked(struct reftable_stack *st,
 		err = REFTABLE_IO_ERROR;
 		goto done;
 	}
-	tab_fd = get_tempfile_fd(tab_file);
 
 	if (st->opts.default_permissions &&
 	    chmod(get_tempfile_path(tab_file), st->opts.default_permissions) < 0) {
@@ -1002,8 +1016,9 @@  static int stack_compact_locked(struct reftable_stack *st,
 		goto done;
 	}
 
-	err = reftable_writer_new(&wr, reftable_fd_write, reftable_fd_flush,
-				  &tab_fd, &st->opts);
+	writer.fd = get_tempfile_fd(tab_file);
+	err = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,
+				  &writer, &st->opts);
 	if (err < 0)
 		goto done;
 
@@ -1460,7 +1475,7 @@  static int stack_compact_range(struct reftable_stack *st,
 		goto done;
 	}
 
-	err = fsync_component(FSYNC_COMPONENT_REFERENCE, get_lock_file_fd(&tables_list_lock));
+	err = stack_fsync(st, get_lock_file_fd(&tables_list_lock));
 	if (err < 0) {
 		err = REFTABLE_IO_ERROR;
 		unlink(new_table_path.buf);