diff mbox series

[v6,4/8] core.fsyncobjectfiles: add windows support for batch mode

Message ID bdb99822f8c45a8b2855ee2ab38c4460e4b5e22e.1632527609.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Implement a batched fsync option for core.fsyncObjectFiles | expand

Commit Message

Neeraj Singh (WINDOWS-SFS) Sept. 24, 2021, 11:53 p.m. UTC 
From: Neeraj Singh <neerajsi@microsoft.com>

This commit adds a win32 implementation for fsync_no_flush that is
called git_fsync. The 'NtFlushBuffersFileEx' function being called is
available since Windows 8. If the function is not available, we
return -1 and Git falls back to doing a full fsync.

The operating system is told to flush data only without a hardware
flush primitive. A later full fsync will cause the metadata log
to be flushed and then the disk cache to be flushed on NTFS and
ReFS. Other filesystems will treat this as a full flush operation.

I added a new file here for this system call so as not to conflict with
downstream changes in the git-for-windows repository related to fscache.

Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
---
 compat/mingw.h                      |  3 +++
 compat/win32/flush.c                | 28 ++++++++++++++++++++++++++++
 config.mak.uname                    |  2 ++
 contrib/buildsystems/CMakeLists.txt |  3 ++-
 wrapper.c                           |  4 ++++
 5 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 compat/win32/flush.c

Comments

Junio C Hamano Sept. 27, 2021, 8:07 p.m. UTC | #1 
"Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/compat/mingw.h b/compat/mingw.h
> index c9a52ad64a6..6074a3d3ced 100644
> --- 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

...

> diff --git a/wrapper.c b/wrapper.c
> index bb4f9f043ce..1a1e2fba9c9 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -567,6 +567,10 @@ int git_fsync(int fd, enum fsync_action action)
>  						 SYNC_FILE_RANGE_WAIT_AFTER);
>  #endif
>  
> +#ifdef fsync_no_flush
> +		return fsync_no_flush(fd);
> +#endif
> +
>  		errno = ENOSYS;
>  		return -1;

This almost makes me wonder if we want to have a fallback
implementation of fsync_no_flush() that does

   int fsync_no_flush(int unused)
   {
	errno = ENOSYS;
	return -1;
   }

when nobody (like Windows) define their own fsync_no_flush().  That
way, this codepath does not have to have #ifdef/#endif here.

This function is already #ifdef ridden anyway, so reducing just one
instance may not make much difference, but since I noticed it ...

Thanks.
Neeraj Singh Sept. 27, 2021, 8:55 p.m. UTC | #2 
On Mon, Sep 27, 2021 at 1:07 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > diff --git a/compat/mingw.h b/compat/mingw.h
> > index c9a52ad64a6..6074a3d3ced 100644
> > --- 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
>
> ...
>
> > diff --git a/wrapper.c b/wrapper.c
> > index bb4f9f043ce..1a1e2fba9c9 100644
> > --- a/wrapper.c
> > +++ b/wrapper.c
> > @@ -567,6 +567,10 @@ int git_fsync(int fd, enum fsync_action action)
> >                                                SYNC_FILE_RANGE_WAIT_AFTER);
> >  #endif
> >
> > +#ifdef fsync_no_flush
> > +             return fsync_no_flush(fd);
> > +#endif
> > +
> >               errno = ENOSYS;
> >               return -1;
>
> This almost makes me wonder if we want to have a fallback
> implementation of fsync_no_flush() that does
>
>    int fsync_no_flush(int unused)
>    {
>         errno = ENOSYS;
>         return -1;
>    }
>
> when nobody (like Windows) define their own fsync_no_flush().  That
> way, this codepath does not have to have #ifdef/#endif here.
>
> This function is already #ifdef ridden anyway, so reducing just one
> instance may not make much difference, but since I noticed it ...
>
> Thanks.

I'll make your suggested change on Github so that it will be available if
we do another re-roll.

Thanks,
Nereaj
Neeraj Singh Sept. 27, 2021, 9:03 p.m. UTC | #3 
On Mon, Sep 27, 2021 at 1:55 PM Neeraj Singh <nksingh85@gmail.com> wrote:
>
> On Mon, Sep 27, 2021 at 1:07 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > "Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > > diff --git a/compat/mingw.h b/compat/mingw.h
> > > index c9a52ad64a6..6074a3d3ced 100644
> > > --- 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
> >
> > ...
> >
> > > diff --git a/wrapper.c b/wrapper.c
> > > index bb4f9f043ce..1a1e2fba9c9 100644
> > > --- a/wrapper.c
> > > +++ b/wrapper.c
> > > @@ -567,6 +567,10 @@ int git_fsync(int fd, enum fsync_action action)
> > >                                                SYNC_FILE_RANGE_WAIT_AFTER);
> > >  #endif
> > >
> > > +#ifdef fsync_no_flush
> > > +             return fsync_no_flush(fd);
> > > +#endif
> > > +
> > >               errno = ENOSYS;
> > >               return -1;
> >
> > This almost makes me wonder if we want to have a fallback
> > implementation of fsync_no_flush() that does
> >
> >    int fsync_no_flush(int unused)
> >    {
> >         errno = ENOSYS;
> >         return -1;
> >    }
> >
> > when nobody (like Windows) define their own fsync_no_flush().  That
> > way, this codepath does not have to have #ifdef/#endif here.
> >
> > This function is already #ifdef ridden anyway, so reducing just one
> > instance may not make much difference, but since I noticed it ...
> >
> > Thanks.
>
> I'll make your suggested change on Github so that it will be available if
> we do another re-roll.
>
> Thanks,
> Nereaj

Actually, while trying your suggestion, my conclusion is that we'd
either have the
inverse ifdef around the fsync_no_flush fallback or an #undef, or some
other confusing
state.  The current ifdeffery is unpleasant to read but not too long
and also pretty direct.
Win32 has an extra level of indirection, but the unix platforms
syscalls are directly written
in one place.

Thanks,
Neeraj
Junio C Hamano Sept. 27, 2021, 11:53 p.m. UTC | #4 
Neeraj Singh <nksingh85@gmail.com> writes:

> ....  The current ifdeffery is unpleasant to read but not too long
> and also pretty direct.
> Win32 has an extra level of indirection, but the unix platforms
> syscalls are directly written
> in one place.

Yes, that is exactly why I concluded that reducing just one instance
would not make that much difference ;-)

Thanks.
diff mbox series

Patch

diff --git a/compat/mingw.h b/compat/mingw.h
index c9a52ad64a6..6074a3d3ced 100644
--- 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;
+       }
+
+       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;
+}
diff --git a/config.mak.uname b/config.mak.uname
index e6d482fbcc6..34c93314a50 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -451,6 +451,7 @@  endif
 	CFLAGS =
 	BASIC_CFLAGS = -nologo -I. -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
 	COMPAT_OBJS = compat/msvc.o compat/winansi.o \
+		compat/win32/flush.o \
 		compat/win32/path-utils.o \
 		compat/win32/pthread.o compat/win32/syslog.o \
 		compat/win32/trace2_win32_process_info.o \
@@ -626,6 +627,7 @@  ifneq (,$(findstring MINGW,$(uname_S)))
 	COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
 	COMPAT_OBJS += compat/mingw.o compat/winansi.o \
 		compat/win32/trace2_win32_process_info.o \
+		compat/win32/flush.o \
 		compat/win32/path-utils.o \
 		compat/win32/pthread.o compat/win32/syslog.o \
 		compat/win32/dirent.o
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 171b4124afe..b573a5ee122 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -261,7 +261,8 @@  if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
 				NOGDI OBJECT_CREATION_MODE=1 __USE_MINGW_ANSI_STDIO=0
 				USE_NED_ALLOCATOR OVERRIDE_STRDUP MMAP_PREVENTS_DELETE USE_WIN32_MMAP
 				UNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET)
-	list(APPEND compat_SOURCES compat/mingw.c compat/winansi.c compat/win32/path-utils.c
+	list(APPEND compat_SOURCES compat/mingw.c compat/winansi.c
+		compat/win32/flush.c compat/win32/path-utils.c
 		compat/win32/pthread.c compat/win32mmap.c compat/win32/syslog.c
 		compat/win32/trace2_win32_process_info.c compat/win32/dirent.c
 		compat/nedmalloc/nedmalloc.c compat/strdup.c)
diff --git a/wrapper.c b/wrapper.c
index bb4f9f043ce..1a1e2fba9c9 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -567,6 +567,10 @@  int git_fsync(int fd, enum fsync_action action)
 						 SYNC_FILE_RANGE_WAIT_AFTER);
 #endif
 
+#ifdef fsync_no_flush
+		return fsync_no_flush(fd);
+#endif
+
 		errno = ENOSYS;
 		return -1;