| Message ID | 50e39f698a7c0cc06d3bc060e6dbc539ea693241.1646905589.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | A design for future-proofing fsync() configuration | expand |
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. > 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)
On Thu, Mar 10, 2022 at 9:34 AM Junio C Hamano <gitster@pobox.com> wrote: > > 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. > > 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)
diff --git a/cache.h b/cache.h index f307e89516..63a95d1977 100644 --- a/cache.h +++ b/cache.h @@ -1745,12 +1745,19 @@ int copy_file(const char *dst, const char *src, int mode); int copy_file_with_time(const char *dst, const char *src, int mode); void write_or_die(int fd, const void *buf, size_t count); -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); } ssize_t read_in_full(int fd, void *buf, size_t count); 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)
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. Add a new `fsync_component()` wrapper which returns an error code instead of dying. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- cache.h | 13 ++++++++++--- write-or-die.c | 10 ++++++---- 2 files changed, 16 insertions(+), 7 deletions(-)