git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2] Makefile: add OPEN_RETURNS_EINTR knob

From
Jeff King <peff@peff.net>
Date
Feb 26, 2021, 06:14 UTC
Message-ID
<YDiRywyld/0OTT5U@coredump.intra.peff.net>
In-Reply-To
<YDaY/M3Rw+6xwZlf@coredump.intra.peff.net>
On Wed, Feb 24, 2021 at 01:20:44PM -0500, Jeff King wrote:
Show 13 quoted lines
> > Hmph, just like other workarounds, shouldn't this be "if your
> > platform screws this up, define this knob to work it around"?  That
> > would make it fit better with the rest of compat/.
> 
> I actually started that way, but then I got stuck deciding which
> platforms in config.mak.uname to enable it for. We've seen it on Linux
> and on macOS. I don't know how prevalent it is on those platforms (or
> whether it is indeed a bug that may even get fixed). Since the check is
> basically zero-cost at run-time, it seemed like it was worth just being
> on guard for it so that nobody ever had to worry about it again.
> 
> But I don't feel that strongly. I'd be happy to do it with a Makefile
> knob if you prefer.
So here's a v2 that turns it into a Makefile knob.
This was motivated by two things:
  - it must not be enabled on Windows, where it overrides the existing
    mingw_open()
  - I couldn't replicate it on a Big Sur machine, using the same
    sequence that Aleksey did (basically just cloning and checking out
    dotnet/runtime.git from scratch).
    So it's not entirely clear to me how widespread the problem is. A
    conservative approach (at least in terms of change from the status
    quo) is to introduce this knob but not enable it automatically
    (which this patch does). And then if somebody runs into it, they can
    try turning the knob.
    It's less conservative in the sense that we won't be preemptively
    protecting people. I dunno. Once we have the knob, it would be
    pretty easy to just flip the default.
-- >8 --
Subject: [PATCH] Makefile: add OPEN_RETURNS_EINTR knob

On some platforms, open() reportedly returns EINTR when opening regular files and we receive a signal (usually SIGALRM from our progress meter). This shouldn't happen, as open() should be a restartable syscall, and we specify SA_RESTART when setting up the alarm handler. So it may actually be a kernel or libc bug for this to happen. But it has been reported on at least one version of Linux (on a network filesystem):

  https://lore.kernel.org/git/c8061cce-71e4-17bd-a56a-a5fed93804da@neanderfunk.de/
as well as on macOS starting with Big Sur even on a regular filesystem.

We can work around it by retrying open() calls that get EINTR, just as we do for read(), etc. Since we don't ever _want_ to interrupt an open() call, we can get away with just redefining open, rather than insisting all callsites use xopen().

We actually do have an xopen() wrapper already (and it even does this retry, though there's no indication of it being an observed problem back then; it seems simply to have been lifted from xread(), etc). But it is used hardly anywhere, and isn't suitable for general use because it will die() on error. In theory we could combine the two, but it's awkward to do so because of the variable-args interface of open().

This patch adds a Makefile knob for enabling the workaround. It's not enabled by default for any platforms in config.mak.uname yet, as we don't have enough data to decide how common this is (I have not been able to reproduce on either Linux or Big Sur myself). It may be worth enabling preemptively anyway, since the cost is pretty low (if we don't see an EINTR, it's just an extra conditional).

However, note that we must not enable this on Windows. It doesn't do anything there, and the macro overrides the existing mingw_open() redirection. I've added a preemptive #undef here in the mingw header (which is processed first) to just quietly disable it (we could also make it an #error, but there is little point in being so aggressive).

Reported-by: Aleksey Kliger <alklig@microsoft.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile          |  7 +++++++
 compat/mingw.h    |  1 +
 compat/open.c     | 25 +++++++++++++++++++++++++
 git-compat-util.h |  6 ++++++
 4 files changed, 39 insertions(+)
 create mode 100644 compat/open.c
diff --git a/Makefile b/Makefile
index 9b1bde2e0e..c4872ca16e 100644
--- a/Makefile
+++ b/Makefile
@@ -22,6 +22,9 @@ all::
 # when attempting to read from an fopen'ed directory (or even to fopen
 # it at all).
 #
+# Define OPEN_RETURNS_EINTR if your open() system call may return EINTR
+# when a signal is received (as opposed to restarting).
+#
 # Define NO_OPENSSL environment variable if you do not have OpenSSL.
 #
 # Define USE_LIBPCRE if you have and want to use libpcre. Various
@@ -1538,6 +1541,10 @@ ifdef FREAD_READS_DIRECTORIES
 	COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
 	COMPAT_OBJS += compat/fopen.o
 endif
+ifdef OPEN_RETURNS_EINTR
+	COMPAT_CFLAGS += -DOPEN_RETURNS_EINTR
+	COMPAT_OBJS += compat/open.o
+endif
 ifdef NO_SYMLINK_HEAD
 	BASIC_CFLAGS += -DNO_SYMLINK_HEAD
 endif
diff --git a/compat/mingw.h b/compat/mingw.h
index af8eddd73e..c9a52ad64a 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -227,6 +227,7 @@ int mingw_rmdir(const char *path);
 
 int mingw_open (const char *filename, int oflags, ...);
 #define open mingw_open
+#undef OPEN_RETURNS_EINTR
 
 int mingw_fgetc(FILE *stream);
 #define fgetc mingw_fgetc
diff --git a/compat/open.c b/compat/open.c
new file mode 100644
index 0000000000..eb3754a23b
--- /dev/null
+++ b/compat/open.c
@@ -0,0 +1,25 @@
+#include "git-compat-util.h"
+
+#undef open
+int git_open_with_retry(const char *path, int flags, ...)
+{
+	mode_t mode = 0;
+	int ret;
+
+	/*
+	 * Also O_TMPFILE would take a mode, but it isn't defined everywhere.
+	 * And anyway, we don't use it in our code base.
+	 */
+	if (flags & O_CREAT) {
+		va_list ap;
+		va_start(ap, flags);
+		mode = va_arg(ap, int);
+		va_end(ap);
+	}
+
+	do {
+		ret = open(path, flags, mode);
+	} while (ret < 0 && errno == EINTR);
+
+	return ret;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 838246289c..551cc9f22f 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -788,6 +788,12 @@ int git_vsnprintf(char *str, size_t maxsize,
 		  const char *format, va_list ap);
 #endif
 
+#ifdef OPEN_RETURNS_EINTR
+#undef open
+#define open git_open_with_retry
+int git_open_with_retry(const char *path, int flag, ...);
+#endif
+
 #ifdef __GLIBC_PREREQ
 #if __GLIBC_PREREQ(2, 1)
 #define HAVE_STRCHRNUL

-- 
2.31.0.rc0.520.g3ffb8d01f4
Previous: Jeff KingNext: Junio C Hamano
Message 5 of 15 in “wrapper: add workaround for open() returning EINTR”
  1. wrapper: add workaround for open() returning EINTRJeff King, Feb 24, 2021
  2. Jeff KingFeb 24, 2021
  3. Junio C HamanoFeb 24, 2021
  4. Jeff KingFeb 24, 2021
  5. Makefile: add OPEN_RETURNS_EINTR knobJeff King, Feb 26, 2021
  6. Junio C HamanoFeb 26, 2021
  7. config.mak.uname: enable OPEN_RETURNS_EINTR for macOS Big SurJeff King, Mar 1, 2021
  8. Junio C HamanoMar 1, 2021
  9. Junio C HamanoMar 1, 2021
  10. Jeff KingMar 3, 2021
  11. Junio C HamanoMar 4, 2021
  12. Taylor BlauFeb 24, 2021
  13. Johannes SixtFeb 24, 2021
  14. Jeff KingFeb 24, 2021
  15. Jeff KingFeb 26, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.