Re: [PATCH 6/8] core.fsync: add `fsync_component()` wrapper which doesn't die
On Thu, Mar 10, 2022 at 9:34 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 41 quoted lines
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > We have a `fsync_component_or_die()` helper function which only syncs
> > changes to disk in case the corresponding config is enabled by the user.
> > This wrapper will always die on an error though, which makes it
> > insufficient for new callsites we are about to add.
>
> You can replace "which makes it ..." part with a bit more concrete
> description to save suspense from the readers.
>
> fsync_component_or_die() that dies upon an error is not useful
> for callers with their own error handling or recovery logic,
> like ref transaction API.
>
> Split fsync_component() out that returns an error to help them.
>
> > -void fsync_or_die(int fd, const char *);
> > +int maybe_fsync(int fd);
> > ...
> > +static inline int fsync_component(enum fsync_component component, int fd)
> > +{
> > + if (fsync_components & component)
> > + return maybe_fsync(fd);
> > + return 0;
> > +}
> >
> > static inline void fsync_component_or_die(enum fsync_component component, int fd, const char *msg)
> > {
> > - if (fsync_components & component)
> > - fsync_or_die(fd, msg);
> > + if (fsync_component(component, fd) < 0)
> > + die_errno("fsync error on '%s'", msg);
> > }
>
> I think in the eventuall reroll, these "static inline" functions on
> the I/O code path will become real functions in write-or-die.c but
> other than that this reorganization looks sensible.
>
> Thanks.
>Yes, that will be part of v6 of the base changeset.
Show 30 quoted lines
> > diff --git a/write-or-die.c b/write-or-die.c
> > index 9faa5f9f56..4a5455ce46 100644
> > --- a/write-or-die.c
> > +++ b/write-or-die.c
> > @@ -56,19 +56,21 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)
> > }
> > }
> >
> > -void fsync_or_die(int fd, const char *msg)
> > +int maybe_fsync(int fd)
> > {
> > if (use_fsync < 0)
> > use_fsync = git_env_bool("GIT_TEST_FSYNC", 1);
> > if (!use_fsync)
> > - return;
> > + return 0;
> >
> > if (fsync_method == FSYNC_METHOD_WRITEOUT_ONLY &&
> > git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0)
> > - return;
> > + return 0;
> >
> > if (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0)
> > - die_errno("fsync error on '%s'", msg);
> > + return -1;
> > +
> > + return 0;
> > }
> >
> > void write_or_die(int fd, const void *buf, size_t count)