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

From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Taylor Blau <me@ttaylorr.com>,
	"Randall S. Becker" <rsbecker@nexbridge.com>,
	git@vger.kernel.org
Subject: Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86
Date: Thu, 3 Jun 2021 23:55:15 -0400	[thread overview]
Message-ID: <YLmkI4a4J60KFY2W@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqk0na2yyc.fsf@gitster.g>

On Fri, Jun 04, 2021 at 10:36:11AM +0900, Junio C Hamano wrote:

> Thanks.  I was hoping somebody in the thread would tie the loose
> ends, but upon inspection of the output from
> 
>     $ git grep -e fsync\( maint seen -- \*.[ch]
> 
> it turns out that fsync_or_die() is the only place that calls
> fsync(), so perhaps doing it in a way that is quite different from
> what has been discussed may be even a better alternative.
> 
> If any new callers care about the return value of fsync(), I'd
> expect that they would be calling this wrapper, and the "best
> effort" callers that do not check the returned value by definition
> do not care if fsync() does not complete due to an interrupt, so I
> am hoping that the current "we only call it from this wrapper" is
> not just "the code currently happens to be this way", but it is
> sensible that the code will stay that way in the future.
> 
> Obviously I appreciate reviews and possibly tests, but sanity
> checking my observation that fsync() is called only from here is a
> good thing to have.

Thanks for digging further. I didn't even think to look at how many
calls there were, but I agree there is only the one. And moreover, I
agree that it is unlikely we'd ever have more than one, for the reasons
you listed. So I think your patch is a nice and simple solution, and we
don't need worry about magic macro wrappers at all.

One brief aside: I'm still not entirely convinced that NonStop isn't
violating POSIX. Yes, as Eric noted, fsync() is allowed to return EINTR.
But should it do so when the signal it got was set up with SA_RESTART?

The sigaction(3posix) page says:

     SA_RESTART   This flag affects the behavior of interruptible functions;
		  that is, those specified to fail with errno set to
		  [EINTR]. If set, and a function specified as
		  interruptible is interrupted by this signal, the
		  function shall restart and shall not fail with [EINTR]
		  unless otherwise specified. [...]

and I could not find anywhere that it is "otherwise specified" for
fsync(). Of course, whatever POSIX says, if NonStop needs this
workaround, we should provide it. But this may explain why we never saw
it on other systems.

It also means it's less important for this workaround to kick in
everywhere. But given how low-cost it is, I'm just as happy to avoid
having a separate knob to enable it.

> -- >8 --
> Subject: fsync(): be prepared to see EINTR

The patch itself looks good to me.

-Peff