Re: [PATCH v2 1/3] core.fsyncmethod: add writeout-only mode

[Neeraj Singh <nksingh85@gmail.com>]

On Tue, Dec 7, 2021 at 3:45 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Dec 07, 2021 at 02:46:49AM +0000, Neeraj Singh via GitGitGadget wrote:
> > From: Neeraj Singh <neerajsi@microsoft.com>
> [snip]
> > --- a/compat/mingw.h
> > +++ b/compat/mingw.h
> > @@ -329,6 +329,9 @@ int mingw_getpagesize(void);
> >  #define getpagesize mingw_getpagesize
> >  #endif
> >
> > +int win32_fsync_no_flush(int fd);
> > +#define fsync_no_flush win32_fsync_no_flush
> > +
> >  struct rlimit {
> >       unsigned int rlim_cur;
> >  };
> > 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"
> > +#include <winternl.h>
> > +#include "lazyload.h"
> > +
> > +int win32_fsync_no_flush(int fd)
> > +{
> > +       IO_STATUS_BLOCK io_status;
> > +
> > +#define FLUSH_FLAGS_FILE_DATA_ONLY 1
> > +
> > +       DECLARE_PROC_ADDR(ntdll.dll, NTSTATUS, NtFlushBuffersFileEx,
> > +                      HANDLE FileHandle, ULONG Flags, PVOID Parameters, ULONG ParameterSize,
> > +                      PIO_STATUS_BLOCK IoStatusBlock);
> > +
> > +       if (!INIT_PROC_ADDR(NtFlushBuffersFileEx)) {
> > +             errno = ENOSYS;
> > +             return -1;
> > +       }
>
> I'm wondering whether it would make sense to fall back to fsync(3P) in
> case we cannot use writeout-only, but I see that were doing essentially
> that in `fsync_or_die()`. There is no indicator to the user though that
> writeout-only doesn't work -- do we want to print a one-time warning?
>

I wanted to leave the fallback to the caller so that the algorithm can
be adjusted in some way based on whether writeout-only succeeded.  For
batched fsync object files, we refrain from doing the last fsync if we
were doing real fsyncs all along.

I didn't want to issue a warning, since this writeout-only codepath
might be invoked from multiple subprocesses, which would each
potentially issue their one warning.  The consequence of failing
writeout only is worse performance, but should not be compromised
safety, so I'm not sure the user gets enough from the warning to
justify something that's potentially spammy.  In practice, when batch
mode is adopted on Windows (by default), some older pre-Win8 systems
will experience fsyncs and equivalent performance to what they're
seeing today. I don't want these users to have a warning too.

> > +       memset(&io_status, 0, sizeof(io_status));
> > +       if (NtFlushBuffersFileEx((HANDLE)_get_osfhandle(fd), FLUSH_FLAGS_FILE_DATA_ONLY,
> > +                             NULL, 0, &io_status)) {
> > +             errno = EINVAL;
> > +             return -1;
> > +       }
> > +
> > +       return 0;
> > +}
>
> [snip]
> > diff --git a/wrapper.c b/wrapper.c
> > index 36e12119d76..1c5f2c87791 100644
> > --- a/wrapper.c
> > +++ b/wrapper.c
> > @@ -546,6 +546,62 @@ int xmkstemp_mode(char *filename_template, int mode)
> >       return fd;
> >  }
> >
> > +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);
>
> Below we're looping around `EINTR` -- are Apple systems never returning
> it?
>

The EINTR check was added due to a test failure on HP NonStop.  I
don't believe any other platform has actually complained about that.

Thanks again for the code review!
-Neeraj