threads / patch / 49646

patch, 3 partsUse nanosecond-precision file times on Windows

Subject: [PATCH 0/3] Use nanosecond-precision file times on Windows

## tl;dr

8 messages between Oct 23, 2018 and Oct 25, 2018. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Johannes Schindelin via GitGitGadget· Oct 23, 2018, 10:23 UTC · lore

This is yet another patch series in the slow wave of patches coming over from Git for Windows.

With this change, we now use preciser timestamps to determine e.g. whether the Git index is out of date. This change made it into Git for Windows already in version 2.6.0, i.e. for a little over three years.

Please note that this change originally caused a lot of trouble, as e.g. libgit2 was unaware of our plans and used second-precision file times. So if you used Git for Windows as well as a libgit2-based program to, say, update the Git index, there would be a back-and-forth between index updates with and without the fractional second parts, causing quite a bit of bad performance.

These issues have been ironed out long ago, though, so it is high time to contribute these patches to core Git.

Johannes Schindelin (1):
  mingw: factor out code to set stat() data
Karsten Blees (2):
  mingw: replace MSVCRT's fstat() with a Win32-based implementation
  mingw: implement nanosecond-precision file times
 compat/mingw.c   | 76 +++++++++++++++++++++++++++++++-----------------
 compat/mingw.h   | 36 ++++++++++++++++-------
 config.mak.uname |  2 --
 3 files changed, 76 insertions(+), 38 deletions(-)
base-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-53%2Fdscho%2Fnanosecond-file-times-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-53/dscho/nanosecond-file-times-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/53
-- 
gitgitgadget
Johannes Schindelin via GitGitGadget· Oct 23, 2018, 10:23 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH 1/3] mingw: factor out code to set stat() data

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In our fstat() emulation, we convert the file metadata from Win32 data structures to an emulated POSIX structure.

To structure the code better, let's factor that part out into its own function.

Note: it would be tempting to try to unify this code with the part of
do_lstat() that does the same thing, but they operate on different data
structures: BY_HANDLE_FILE_INFORMATION vs WIN32_FILE_ATTRIBUTE_DATA. So
unfortunately, they cannot be unified.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)
Show changes to compat/mingw.c +25 −14
diff --git a/compat/mingw.c b/compat/mingw.c
index 18caf2196..d2e7d86db 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -736,6 +736,29 @@ static int do_stat_internal(int follow, const char *file_name, struct stat *buf)
 	return do_lstat(follow, alt_name, buf);
 }
 
+static int get_file_info_by_handle(HANDLE hnd, struct stat *buf)
+{
+	BY_HANDLE_FILE_INFORMATION fdata;
+
+	if (!GetFileInformationByHandle(hnd, &fdata)) {
+		errno = err_win_to_posix(GetLastError());
+		return -1;
+	}
+
+	buf->st_ino = 0;
+	buf->st_gid = 0;
+	buf->st_uid = 0;
+	buf->st_nlink = 1;
+	buf->st_mode = file_attr_to_st_mode(fdata.dwFileAttributes);
+	buf->st_size = fdata.nFileSizeLow |
+		(((off_t)fdata.nFileSizeHigh)<<32);
+	buf->st_dev = buf->st_rdev = 0; /* not used by Git */
+	buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
+	buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
+	buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
+	return 0;
+}
+
 int mingw_lstat(const char *file_name, struct stat *buf)
 {
 	return do_stat_internal(0, file_name, buf);
@@ -748,7 +771,6 @@ int mingw_stat(const char *file_name, struct stat *buf)
 int mingw_fstat(int fd, struct stat *buf)
 {
 	HANDLE fh = (HANDLE)_get_osfhandle(fd);
-	BY_HANDLE_FILE_INFORMATION fdata;
 
 	if (fh == INVALID_HANDLE_VALUE) {
 		errno = EBADF;
@@ -758,20 +780,9 @@ int mingw_fstat(int fd, struct stat *buf)
 	if (GetFileType(fh) != FILE_TYPE_DISK)
 		return _fstati64(fd, buf);
 
-	if (GetFileInformationByHandle(fh, &fdata)) {
-		buf->st_ino = 0;
-		buf->st_gid = 0;
-		buf->st_uid = 0;
-		buf->st_nlink = 1;
-		buf->st_mode = file_attr_to_st_mode(fdata.dwFileAttributes);
-		buf->st_size = fdata.nFileSizeLow |
-			(((off_t)fdata.nFileSizeHigh)<<32);
-		buf->st_dev = buf->st_rdev = 0; /* not used by Git */
-		buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
-		buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
-		buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
+	if (!get_file_info_by_handle(fh, buf))
 		return 0;
-	}
+
 	errno = EBADF;
 	return -1;
 }
-- 
gitgitgadget
Karsten Blees via GitGitGadget· Oct 23, 2018, 10:23 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation

From: Karsten Blees <blees@dcon.de>

fstat() is the only stat-related CRT function for which we don't have a full replacement yet (and thus the only reason to stick with MSVCRT's 'struct stat' definition).

Fully implement fstat(), in preparation of implementing a POSIX 2013 compatible 'struct stat' with nanosecond-precision file times.

This allows us also to implement some clever code to handle pipes and character devices in our own way.

Signed-off-by: Karsten Blees <blees@dcon.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)
Show changes to compat/mingw.c +21 −10
diff --git a/compat/mingw.c b/compat/mingw.c
index d2e7d86db..07fc0b79a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -771,20 +771,31 @@ int mingw_stat(const char *file_name, struct stat *buf)
 int mingw_fstat(int fd, struct stat *buf)
 {
 	HANDLE fh = (HANDLE)_get_osfhandle(fd);
+	DWORD avail, type = GetFileType(fh) & ~FILE_TYPE_REMOTE;
 
-	if (fh == INVALID_HANDLE_VALUE) {
-		errno = EBADF;
-		return -1;
-	}
-	/* direct non-file handles to MS's fstat() */
-	if (GetFileType(fh) != FILE_TYPE_DISK)
-		return _fstati64(fd, buf);
+	switch (type) {
+	case FILE_TYPE_DISK:
+		return get_file_info_by_handle(fh, buf);
 
-	if (!get_file_info_by_handle(fh, buf))
+	case FILE_TYPE_CHAR:
+	case FILE_TYPE_PIPE:
+		/* initialize stat fields */
+		memset(buf, 0, sizeof(*buf));
+		buf->st_nlink = 1;
+
+		if (type == FILE_TYPE_CHAR) {
+			buf->st_mode = _S_IFCHR;
+		} else {
+			buf->st_mode = _S_IFIFO;
+			if (PeekNamedPipe(fh, NULL, 0, NULL, &avail, NULL))
+				buf->st_size = avail;
+		}
 		return 0;
 
-	errno = EBADF;
-	return -1;
+	default:
+		errno = EBADF;
+		return -1;
+	}
 }
 
 static inline void time_t_to_filetime(time_t t, FILETIME *ft)
-- 
gitgitgadget
brian m. carlson· Oct 24, 2018, 02:20 UTC · re: Karsten Blees via GitGitGadget · lore

Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation

On Tue, Oct 23, 2018 at 03:23:21AM -0700, Karsten Blees via GitGitGadget wrote:
Show 13 quoted lines
> -	if (!get_file_info_by_handle(fh, buf))
> +	case FILE_TYPE_CHAR:
> +	case FILE_TYPE_PIPE:
> +		/* initialize stat fields */
> +		memset(buf, 0, sizeof(*buf));
> +		buf->st_nlink = 1;
> +
> +		if (type == FILE_TYPE_CHAR) {
> +			buf->st_mode = _S_IFCHR;
> +		} else {
> +			buf->st_mode = _S_IFIFO;
> +			if (PeekNamedPipe(fh, NULL, 0, NULL, &avail, NULL))
> +				buf->st_size = avail;

These lines strike me as a bit odd. As far as I'm aware, Unix systems don't return anything useful in this field when calling fstat on a pipe. Is there a reason we fill this in on Windows? If so, could the commit message explain what that is?

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Johannes Schindelin· Oct 24, 2018, 07:37 UTC · re: brian m. carlson · lore

Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation

Hi brian,
On Wed, 24 Oct 2018, brian m. carlson wrote:
Show 19 quoted lines
> On Tue, Oct 23, 2018 at 03:23:21AM -0700, Karsten Blees via GitGitGadget wrote:
> > -	if (!get_file_info_by_handle(fh, buf))
> > +	case FILE_TYPE_CHAR:
> > +	case FILE_TYPE_PIPE:
> > +		/* initialize stat fields */
> > +		memset(buf, 0, sizeof(*buf));
> > +		buf->st_nlink = 1;
> > +
> > +		if (type == FILE_TYPE_CHAR) {
> > +			buf->st_mode = _S_IFCHR;
> > +		} else {
> > +			buf->st_mode = _S_IFIFO;
> > +			if (PeekNamedPipe(fh, NULL, 0, NULL, &avail, NULL))
> > +				buf->st_size = avail;
> 
> These lines strike me as a bit odd.  As far as I'm aware, Unix systems
> don't return anything useful in this field when calling fstat on a pipe.
> Is there a reason we fill this in on Windows?  If so, could the commit
> message explain what that is?
AFAICT the idea was to imitate MSVCRT's fstat() in these cases.

But a quick web search suggests that you are right: https://bugzilla.redhat.com/show_bug.cgi?id=58768#c4 (I could not find any official documentation talking about fstat() and pipes, but I trust Alan to know their stuff).

Do note, please, that according to the issue described in that link, at least *some* glibc/Linux combinations behave in exactly the way this patch implements it.

At this point, I am wary of changing this, too, as the code in question has been in production (read: tested thoroughly) in the current form for *years*, and I am really loathe to introduce a bug where even Windows-specific code in compat/ might rely on this behavior. (And no, I do not trust our test suite to find all of those use cases.)

Ciao, Dscho

brian m. carlson· Oct 24, 2018, 22:40 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation

On Wed, Oct 24, 2018 at 09:37:43AM +0200, Johannes Schindelin wrote:
Show 14 quoted lines
> Hi brian,
> 
> On Wed, 24 Oct 2018, brian m. carlson wrote:
> > These lines strike me as a bit odd.  As far as I'm aware, Unix systems
> > don't return anything useful in this field when calling fstat on a pipe.
> > Is there a reason we fill this in on Windows?  If so, could the commit
> > message explain what that is?
> 
> AFAICT the idea was to imitate MSVCRT's fstat() in these cases.
> 
> But a quick web search suggests that you are right:
> https://bugzilla.redhat.com/show_bug.cgi?id=58768#c4 (I could not find any
> official documentation talking about fstat() and pipes, but I trust Alan
> to know their stuff).

Yeah, that behavior is quite old. I'm surprised that Linux ever did that.

Show 9 quoted lines
> Do note, please, that according to the issue described in that link, at
> least *some* glibc/Linux combinations behave in exactly the way this patch
> implements it.
> 
> At this point, I am wary of changing this, too, as the code in question
> has been in production (read: tested thoroughly) in the current form for
> *years*, and I am really loathe to introduce a bug where even
> Windows-specific code in compat/ might rely on this behavior. (And no, I
> do not trust our test suite to find all of those use cases.)

I don't feel strongly either way. I feel confident the rest of Git doesn't use that field, so I don't see any downsides to keeping it other than the slight overhead of populating it. I just thought I'd ask in case there was something important I was missing.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Junio C Hamano· Oct 25, 2018, 09:35 UTC · re: brian m. carlson · lore

Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 7 quoted lines
> Yeah, that behavior is quite old.  I'm surprised that Linux ever did
> that.
> ...
> I don't feel strongly either way.  I feel confident the rest of Git
> doesn't use that field, so I don't see any downsides to keeping it other
> than the slight overhead of populating it.  I just thought I'd ask in
> case there was something important I was missing.

OK, I'd consider that this part of the review settled for taking the patch as-is. Let's mark the topic for merging to 'next' soonish in the what's cooking report.

Thanks.
Karsten Blees via GitGitGadget· Oct 23, 2018, 10:23 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH 3/3] mingw: implement nanosecond-precision file times

From: Karsten Blees <blees@dcon.de>

We no longer use any of MSVCRT's stat-functions, so there's no need to stick to a CRT-compatible 'struct stat' either.

Define and use our own POSIX-2013-compatible 'struct stat' with nanosecond- precision file times.

Note: This can cause performance issues when using Git variants with
different file time resolutions, as the timestamps are stored in the Git
index: after updating the index with a Git variant that uses
second-precision file times, a nanosecond-aware Git will think that
pretty much every single file listed in the index is out of date.
Signed-off-by: Karsten Blees <blees@dcon.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c   | 18 ++++++++++--------
 compat/mingw.h   | 36 ++++++++++++++++++++++++++----------
 config.mak.uname |  2 --
 3 files changed, 36 insertions(+), 20 deletions(-)
Show changes to 3 files +36 −20

compat/mingw.c, compat/mingw.h, config.mak.uname

diff --git a/compat/mingw.c b/compat/mingw.c
index 07fc0b79a..26016d02e 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -592,9 +592,11 @@ static inline long long filetime_to_hnsec(const FILETIME *ft)
 	return winTime - 116444736000000000LL;
 }
 
-static inline time_t filetime_to_time_t(const FILETIME *ft)
+static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)
 {
-	return (time_t)(filetime_to_hnsec(ft) / 10000000);
+	long long hnsec = filetime_to_hnsec(ft);
+	ts->tv_sec = (time_t)(hnsec / 10000000);
+	ts->tv_nsec = (hnsec % 10000000) * 100;
 }
 
 /**
@@ -653,9 +655,9 @@ static int do_lstat(int follow, const char *file_name, struct stat *buf)
 		buf->st_size = fdata.nFileSizeLow |
 			(((off_t)fdata.nFileSizeHigh)<<32);
 		buf->st_dev = buf->st_rdev = 0; /* not used by Git */
-		buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
-		buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
-		buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
+		filetime_to_timespec(&(fdata.ftLastAccessTime), &(buf->st_atim));
+		filetime_to_timespec(&(fdata.ftLastWriteTime), &(buf->st_mtim));
+		filetime_to_timespec(&(fdata.ftCreationTime), &(buf->st_ctim));
 		if (fdata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) {
 			WIN32_FIND_DATAW findbuf;
 			HANDLE handle = FindFirstFileW(wfilename, &findbuf);
@@ -753,9 +755,9 @@ static int get_file_info_by_handle(HANDLE hnd, struct stat *buf)
 	buf->st_size = fdata.nFileSizeLow |
 		(((off_t)fdata.nFileSizeHigh)<<32);
 	buf->st_dev = buf->st_rdev = 0; /* not used by Git */
-	buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
-	buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
-	buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
+	filetime_to_timespec(&(fdata.ftLastAccessTime), &(buf->st_atim));
+	filetime_to_timespec(&(fdata.ftLastWriteTime), &(buf->st_mtim));
+	filetime_to_timespec(&(fdata.ftCreationTime), &(buf->st_ctim));
 	return 0;
 }
 
diff --git a/compat/mingw.h b/compat/mingw.h
index 571019d0b..9419b27e1 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -327,18 +327,41 @@ static inline int getrlimit(int resource, struct rlimit *rlp)
 }
 
 /*
- * Use mingw specific stat()/lstat()/fstat() implementations on Windows.
+ * Use mingw specific stat()/lstat()/fstat() implementations on Windows,
+ * including our own struct stat with 64 bit st_size and nanosecond-precision
+ * file times.
  */
 #ifndef __MINGW64_VERSION_MAJOR
 #define off_t off64_t
 #define lseek _lseeki64
+struct timespec {
+	time_t tv_sec;
+	long tv_nsec;
+};
 #endif
 
-/* use struct stat with 64 bit st_size */
+struct mingw_stat {
+    _dev_t st_dev;
+    _ino_t st_ino;
+    _mode_t st_mode;
+    short st_nlink;
+    short st_uid;
+    short st_gid;
+    _dev_t st_rdev;
+    off64_t st_size;
+    struct timespec st_atim;
+    struct timespec st_mtim;
+    struct timespec st_ctim;
+};
+
+#define st_atime st_atim.tv_sec
+#define st_mtime st_mtim.tv_sec
+#define st_ctime st_ctim.tv_sec
+
 #ifdef stat
 #undef stat
 #endif
-#define stat _stati64
+#define stat mingw_stat
 int mingw_lstat(const char *file_name, struct stat *buf);
 int mingw_stat(const char *file_name, struct stat *buf);
 int mingw_fstat(int fd, struct stat *buf);
@@ -351,13 +374,6 @@ int mingw_fstat(int fd, struct stat *buf);
 #endif
 #define lstat mingw_lstat
 
-#ifndef _stati64
-# define _stati64(x,y) mingw_stat(x,y)
-#elif defined (_USE_32BIT_TIME_T)
-# define _stat32i64(x,y) mingw_stat(x,y)
-#else
-# define _stat64(x,y) mingw_stat(x,y)
-#endif
 
 int mingw_utime(const char *file_name, const struct utimbuf *times);
 #define utime mingw_utime
diff --git a/config.mak.uname b/config.mak.uname
index 8acdeb71f..f179d7a1d 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -370,7 +370,6 @@ ifeq ($(uname_S),Windows)
 	RUNTIME_PREFIX = YesPlease
 	HAVE_WPGMPTR = YesWeDo
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
-	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
 	MMAP_PREVENTS_DELETE = UnfortunatelyYes
 	# USE_NED_ALLOCATOR = YesPlease
@@ -518,7 +517,6 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	RUNTIME_PREFIX = YesPlease
 	HAVE_WPGMPTR = YesWeDo
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
-	NO_NSEC = YesPlease
 	USE_WIN32_MMAP = YesPlease
 	MMAP_PREVENTS_DELETE = UnfortunatelyYes
 	USE_NED_ALLOCATOR = YesPlease
-- 
gitgitgadget

← back to recent threads