Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 2, 2021, 20:11 UTC
Message-ID: <YLfl4jkuwSCiNrrS@nand.local>
In-Reply-To: <YLfgy94sbmStC0mR@coredump.intra.peff.net>

On Wed, Jun 02, 2021 at 03:49:31PM -0400, Jeff King wrote:

Show 7 quoted lines

> And so when he gets this error:
>
>   fatal: fsync error on '.git/objects/pack/tmp_pack_NkPgqN': Interrupted system call
>
> presumably we were in fsync() when the signal arrived, and unlike most
> other platforms, the call needs to be restarted manually (even though we
> set up the signal with SA_RESTART).

Aha, thanks for explaining. That makes sense to me.

> I'm not sure if this violates POSIX or not (I couldn't find a
> definitive answer to the set of interruptible functions in the
> standard).

I couldn't find anything either, but I believe you that what you outlined is the right solution.

> But either way, the workaround is probably something like:

Seems very reasonable. Here's a patch:

--- >8 ---

Subject: [PATCH] compat: introduce git_fsync_with_restart()

Some platforms, like NonStop do not automatically restart fsync() when interrupted by a signal, even when that signal is setup with SA_RESTART.

This can lead to test breakage, e.g., where "--progress" is used, thus SIGALRM is sent often, and can interrupt an fsync() syscall.

Add a Makefile knob FSYNC_NEEDS_RESTART to replace fsync() with one that gracefully handles getting EINTR.

Reported-by: Randall S. Becker <randall.becker@nexbridge.ca>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Makefile          |  8 ++++++++
 compat/fsync.c    | 10 ++++++++++
 git-compat-util.h |  6 ++++++
 3 files changed, 24 insertions(+)
 create mode 100644 compat/fsync.c

diff --git a/Makefile b/Makefile
index c3565fc0f8..ebea693a2c 100644
--- a/Makefile
+++ b/Makefile
@@ -423,6 +423,9 @@ all::
 # Define NEED_ACCESS_ROOT_HANDLER if access() under root may success for X_OK
 # even if execution permission isn't granted for any user.
 #
+# Define FSYNC_NEEDS_RESTART if your platform's fsync() needs to be restarted
+# manually when interrupted by a signal.
+#
 # Define PAGER_ENV to a SP separated VAR=VAL pairs to define
 # default environment variables to be passed when a pager is spawned, e.g.
 #
@@ -1926,6 +1929,11 @@ ifdef NEED_ACCESS_ROOT_HANDLER
 	COMPAT_OBJS += compat/access.o
 endif

+ifdef FSYNC_NEEDS_RESTART
+	COMPAT_CFLAGS += -DFSYNC_NEEDS_RESTART
+	COMPAT_OBJS += compat/fsync.o
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif
diff --git a/compat/fsync.c b/compat/fsync.c
new file mode 100644
index 0000000000..f340fc628b
--- /dev/null
+++ b/compat/fsync.c
@@ -0,0 +1,10 @@
+#include "git-compat-util.h"
+
+#undef fsync
+int git_fsync_with_restart(int fd)
+{
+	int ret;
+	while ((ret = fsync(fd)) < 0 && errno == EINTR)
+		; /* try again */
+	return ret;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index a508dbe5a3..972fdb609f 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -833,6 +833,12 @@ const char *inet_ntop(int af, const void *src, char *dst, size_t size);
 int git_atexit(void (*handler)(void));
 #endif

+#ifdef FSYNC_NEEDS_RESTART
+#undef fsync
+#define fsync git_fsync_with_restart
+int git_fsync_with_restart(int fd);
+#endif
+
 static inline size_t st_add(size_t a, size_t b)
 {
 	if (unsigned_add_overflows(a, b))
--
2.31.1.163.ga65ce7f831