Re: [PATCH v2 1/3] core.fsyncmethod: add writeout-only mode
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Dec 7, 2021, 12:18 UTC
- Message-ID
- <211207.861r2opplg.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <e79522cbdd4feb45b062b75225475f34039d1866.1638845211.git.gitgitgadget@gmail.com>
On Tue, Dec 07 2021, Neeraj Singh via GitGitGadget wrote:
> From: Neeraj Singh <neerajsi@microsoft.com> > > This commit introduces the `core.fsyncmethod` configuration
Just a commit msg nit: core.fsyncMethod (I see the docs etc. are using it camelCased, good..
Show 7 quoted lines
> 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).
Show 8 quoted lines
> + 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;
> + elseAs 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...)
Show 53 quoted lines
> +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.
> + } > +} > + > 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... :)