On Tue, Dec 7, 2021 at 4:27 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
>
>
> On Tue, Dec 07 2021, Neeraj Singh via GitGitGadget wrote:
>
> > From: Neeraj Singh <neerajsi@xxxxxxxxxxxxx>
> >
> > This commit introduces the `core.fsyncmethod` configuration
>
> Just a commit msg nit: core.fsyncMethod (I see the docs etc. are using
> it camelCased, good..
Will fix.
> > diff --git a/compat/win32/flush.c b/compat/win32/flush.c
> > new file mode 100644
> > index 00000000000..75324c24ee7
> > --- /dev/null
> > +++ b/compat/win32/flush.c
> > @@ -0,0 +1,28 @@
> > +#include "../../git-compat-util.h"
>
> nit: Just FWIW I think the better thing is '#include
> "git-compat-util.h"', i.e. we're compiling at the top-level and have
> added it to -I.
>
> (I know a lot of compat/ and contrib/ and even main-tree stuff does
> that, but just FWIW it's not needed).
>
Will fix.
> > + if (!strcmp(var, "core.fsyncmethod")) {
> > + if (!value)
> > + return config_error_nonbool(var);
> > + if (!strcmp(value, "fsync"))
> > + fsync_method = FSYNC_METHOD_FSYNC;
> > + else if (!strcmp(value, "writeout-only"))
> > + fsync_method = FSYNC_METHOD_WRITEOUT_ONLY;
> > + else
>
> As a non-nit comment I think this config schema looks great so far.
>
> > + warning(_("unknown %s value '%s'"), var, value);
>
> Just a suggestion maybe something slightly scarier like:
>
> "unknown core.fsyncMethod value '%s'; config from future git version? ignoring requested fsync strategy"
>
> Also using the nicer camelCased version instead of "var" (also helps
> translators with context...)
>
Will fix. The motivation for this scheme was to 'factor' the messages
so there would be less to translate. But I see now that the message
doesn't have enough context to translate reasonably.
> > +int git_fsync(int fd, enum fsync_action action)
> > +{
> > + switch (action) {
> > + case FSYNC_WRITEOUT_ONLY:
> > +
> > +#ifdef __APPLE__
> > + /*
> > + * on macOS, fsync just causes filesystem cache writeback but does not
> > + * flush hardware caches.
> > + */
> > + return fsync(fd);
> > +#endif
> > +
> > +#ifdef HAVE_SYNC_FILE_RANGE
> > + /*
> > + * On linux 2.6.17 and above, sync_file_range is the way to issue
> > + * a writeback without a hardware flush. An offset of 0 and size of 0
> > + * indicates writeout of the entire file and the wait flags ensure that all
> > + * dirty data is written to the disk (potentially in a disk-side cache)
> > + * before we continue.
> > + */
> > +
> > + return sync_file_range(fd, 0, 0, SYNC_FILE_RANGE_WAIT_BEFORE |
> > + SYNC_FILE_RANGE_WRITE |
> > + SYNC_FILE_RANGE_WAIT_AFTER);
> > +#endif
> > +
> > +#ifdef fsync_no_flush
> > + return fsync_no_flush(fd);
> > +#endif
> > +
> > + errno = ENOSYS;
> > + return -1;
> > +
> > + case FSYNC_HARDWARE_FLUSH:
> > + /*
> > + * On some platforms fsync may return EINTR. Try again in this
> > + * case, since callers asking for a hardware flush may die if
> > + * this function returns an error.
> > + */
> > + for (;;) {
> > + int err;
> > +#ifdef __APPLE__
> > + err = fcntl(fd, F_FULLFSYNC);
> > +#else
> > + err = fsync(fd);
> > +#endif
> > + if (err >= 0 || errno != EINTR)
> > + return err;
> > + }
> > +
> > + default:
> > + BUG("unexpected git_fsync(%d) call", action);
>
> Don't include such "default" cases, you have an exhaustive "enum", if
> you skip it the compiler will check this for you.
>
Junio gave the feedback to include this "default:" case in the switch
[1]. Removing the default leads to the "error: control reaches end of
non-void function" on gcc. Fixing that error and adding a trial option
does give the exhaustiveness error that you're talking about. I'd
rather just leave this as-is since the BUG() obviates the need for an
in-practice-unreachable return statement.
[1] https://lore.kernel.org/git/xmqqfsu70x58.fsf@gitster.g/
> > + }
> > +}
> > +
> > static int warn_if_unremovable(const char *op, const char *file, int rc)
>
> Just a code nit: I think it's very much preferred if possible to have as
> much of code like this compile on all platforms. See the series at
> 4002e87cb25 (grep: remove #ifdef NO_PTHREADS, 2018-11-03) is part of for
> a good example.
>
> Maybe not worth it in this case since they're not nested ifdef's.
>
> I'm basically thinking of something (also re Patrick's comment on the
> 2nd patch) where we have a platform_fsync() whose return
> value/arguments/whatever capture this "I want to return now" or "you
> should be looping" and takes the enum_fsync_action" strategy.
>
> Then the git_fsync() would be the platform-independent looping etc., and
> another funciton would do the "one fsync at a time, maybe call me
> again".
>
> Maybe it would suck more, just food for thought... :)
I'm going to introduce a new static function called fsync_loop which
does the looping and I'll call it from git_fsync. That appears to be
the cleanest to me to address your and Patrick's feedback.
Thanks,
Neeraj
