config.mak.uname: enable OPEN_RETURNS_EINTR for macOS Big Sur

Commit Message

Jeff King March 1, 2021, 9:29 a.m. UTC

On Fri, Feb 26, 2021 at 02:17:53PM -0800, Junio C Hamano wrote:

> > +	/*
> > +	 * Also O_TMPFILE would take a mode, but it isn't defined everywhere.
> > +	 * And anyway, we don't use it in our code base.
> > +	 */
> 
> That is being extra careful---I like it very much.

I wondered what would happen if my "anyway" above is wrong. We at least
would not invoke undefined behavior (because we'd avoid looking at the
mode parameter even though it exists), but would pass a "0" mode to the
real open(). Presumably somebody would notice that. :)

> > +	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;
> > +}
> 
> Thanks.

I got another off-list report of the problem. I think we probably want
to do this on top:

-- >8 --
Subject: config.mak.uname: enable OPEN_RETURNS_EINTR for macOS Big Sur

We've had mixed reports on whether the latest release of macOS needs
this Makefile knob set. In most reported cases, there's antivirus
software running (which one might imagine could cause an open() call to
be delayed). However, one of the (off-list) reports I've gotten
indicated that it happened on an otherwise clean install of Big Sur.

Since the symptom is so bad (checkout randomly fails to write several
fails when the progress meter kicks in), and since the workaround is so
lightweight (if we don't see EINTR, it's just an extra conditional
check), let's just turn it on by default.

Signed-off-by: Jeff King <[e-mail]>
---
Apparently Big Sur jumped from macOS 10.x to 11.x. But our "uname -r"
check gives the "Darwin version", in which it is 20.x (following 19.x
for the previous version). At least according to some sources I found
online. :) So that is good, because otherwise all of our uname_R checks
here would have been broken. I don't have a Big Sur machine handy to
test with, but I believe this should do what we want.

 config.mak.uname | 3 +++
 1 file changed, 3 insertions(+)

Comments

Junio C Hamano March 1, 2021, 5:17 p.m. UTC | #1

Jeff King <[e-mail]> writes:

> I got another off-list report of the problem. I think we probably want
> to do this on top:
>
> -- >8 --
> Subject: config.mak.uname: enable OPEN_RETURNS_EINTR for macOS Big Sur
>
> We've had mixed reports on whether the latest release of macOS needs
> this Makefile knob set. In most reported cases, there's antivirus
> software running (which one might imagine could cause an open() call to
> be delayed). However, one of the (off-list) reports I've gotten
> indicated that it happened on an otherwise clean install of Big Sur.
>
> Since the symptom is so bad (checkout randomly fails to write several
> fails when the progress meter kicks in), and since the workaround is so
> lightweight (if we don't see EINTR, it's just an extra conditional
> check), let's just turn it on by default.
>
> Signed-off-by: Jeff King <[e-mail]>
> ---
> Apparently Big Sur jumped from macOS 10.x to 11.x. But our "uname -r"
> check gives the "Darwin version", in which it is 20.x (following 19.x
> for the previous version). At least according to some sources I found
> online. :) So that is good, because otherwise all of our uname_R checks
> here would have been broken. I don't have a Big Sur machine handy to
> test with, but I believe this should do what we want.

Thanks.

>  config.mak.uname | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index e22d4b6d67..d204c20a64 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -124,6 +124,9 @@ ifeq ($(uname_S),Darwin)
>  	ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 11 && echo 1),1)
>  		HAVE_GETDELIM = YesPlease
>  	endif
> +	ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 20 && echo 1),1)
> +		OPEN_RETURNS_EINTR = UnfortunatelyYes
> +	endif
>  	NO_MEMMEM = YesPlease
>  	USE_ST_TIMESPEC = YesPlease
>  	HAVE_DEV_TTY = YesPlease

Junio C Hamano March 1, 2021, 11:57 p.m. UTC | #2

Jeff King <[e-mail]> writes:

> I got another off-list report of the problem. I think we probably want
> to do this on top:

Queued and pushed out.

I wonder if these hits for SA_RESTART in config.mak.uname would be a
good way to guide us.

    [6c109904 (Port to HP NonStop, 2012-09-19)]
            # Not detected (nor checked for) by './configure'.
            # We don't have SA_RESTART on NonStop, unfortunalety.
            COMPAT_CFLAGS += -DSA_RESTART=0

    [40036bed (Port to QNX, 2012-12-18)]
    ifeq ($(uname_S),QNX)
            COMPAT_CFLAGS += -DSA_RESTART=0

One caveat is that we do not know if their system headers hide the
real implementation of open(2) behind a C preprocessor macro, in
whcih case OPEN_RETURNS_EINTR wrapper may not work correctly.

>  config.mak.uname | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index e22d4b6d67..d204c20a64 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -124,6 +124,9 @@ ifeq ($(uname_S),Darwin)
>  	ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 11 && echo 1),1)
>  		HAVE_GETDELIM = YesPlease
>  	endif
> +	ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 20 && echo 1),1)
> +		OPEN_RETURNS_EINTR = UnfortunatelyYes
> +	endif
>  	NO_MEMMEM = YesPlease
>  	USE_ST_TIMESPEC = YesPlease
>  	HAVE_DEV_TTY = YesPlease

Jeff King March 3, 2021, 1:41 p.m. UTC | #3

On Mon, Mar 01, 2021 at 03:57:35PM -0800, Junio C Hamano wrote:

> Jeff King <[e-mail]> writes:
> 
> > I got another off-list report of the problem. I think we probably want
> > to do this on top:
> 
> Queued and pushed out.

Thanks. I guess we're a bit late to make it into the upcoming release.
Certainly we have survived for many years without this particular
bugfix, so in that sense it is not urgent. But I do wonder if we will
see more reports as more people start using the new macOS release. So it
might be good to keep in mind for maint, if we cut a minor release.

Or alternatively, we could include _just_ the first patch. That's low
risk, since you have to enable to knob yourself, but it gives people an
option if they run into the symptom. But even that is probably not that
urgent. People can also cherry-pick the patch, after all (and a
distributor like homebrew can probably include the patch in their recipe
if need be).

> I wonder if these hits for SA_RESTART in config.mak.uname would be a
> good way to guide us.
> 
>     [6c109904 (Port to HP NonStop, 2012-09-19)]
>             # Not detected (nor checked for) by './configure'.
>             # We don't have SA_RESTART on NonStop, unfortunalety.
>             COMPAT_CFLAGS += -DSA_RESTART=0
> 
>     [40036bed (Port to QNX, 2012-12-18)]
>     ifeq ($(uname_S),QNX)
>             COMPAT_CFLAGS += -DSA_RESTART=0

I'm inclined to leave them alone until somebody with access to such a
system can look further into it. After all, if you do not have
SA_RESTART, you might not even have EINTR in the first place.

> One caveat is that we do not know if their system headers hide the
> real implementation of open(2) behind a C preprocessor macro, in
> whcih case OPEN_RETURNS_EINTR wrapper may not work correctly.

Yeah. I didn't think about that when I did the original "just do it
everywhere" patch. But that is exactly what caused the problem on
Windows (not a system macro, but in fact our own!). So I'm glad to have
backed it off to a Makefile knob.

-Peff

Junio C Hamano March 4, 2021, 12:47 a.m. UTC | #4

Jeff King <[e-mail]> writes:

> Thanks. I guess we're a bit late to make it into the upcoming release.
> Certainly we have survived for many years without this particular
> bugfix, so in that sense it is not urgent. But I do wonder if we will
> see more reports as more people start using the new macOS release. So it
> might be good to keep in mind for maint, if we cut a minor release.
>
> Or alternatively, we could include _just_ the first patch. That's low
> risk, since you have to enable to knob yourself, but it gives people an
> option if they run into the symptom. But even that is probably not that
> urgent. People can also cherry-pick the patch, after all (and a
> distributor like homebrew can probably include the patch in their recipe
> if need be).

True.  The topic is forked at somewhere mergeable to 'maint' so
distro folks who are interested can merge them down later.  I think
it is small and safe enough to merge them both down to the upcoming
release, though.

Patch

diff --git a/config.mak.uname b/config.mak.uname
index e22d4b6d67..d204c20a64 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -124,6 +124,9 @@  ifeq ($(uname_S),Darwin)
 	ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 11 && echo 1),1)
 		HAVE_GETDELIM = YesPlease
 	endif
+	ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 20 && echo 1),1)
+		OPEN_RETURNS_EINTR = UnfortunatelyYes
+	endif
 	NO_MEMMEM = YesPlease
 	USE_ST_TIMESPEC = YesPlease
 	HAVE_DEV_TTY = YesPlease