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

Jeff King [off-list ref] writes:

This looks as I'd expect. But after seeing Eric's response, we perhaps
want to do away with the knob entirely.

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.

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

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.

Make sure we deal with such a case by retrying the syscall
ourselves.  Luckily, we call fsync() fron a single wrapper,
fsync_or_die(), so the fix is fairly isolated.
    
Reported-by: Randall S. Becker <redacted>
Helped-by: Jeff King [off-list ref]
Helped-by: Taylor Blau [off-list ref]
[jc: the above two did most of the work---I just tied the loose end]
Signed-off-by: Junio C Hamano <redacted>
---

 write-or-die.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git i/write-or-die.c w/write-or-die.c
index eab8c8d0b9..534b2f5cba 100644
--- i/write-or-die.c
+++ w/write-or-die.c

@@ -57,7 +57,11 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)
 
 void fsync_or_die(int fd, const char *msg)
 {
-	if (fsync(fd) < 0) {
+	int status;
+
+	while ((status = fsync(fd)) < 0 && errno == EINTR)
+		; /* try again */
+	if (status < 0) {
 		die_errno("fsync error on '%s'", msg);
 	}
 }

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

Jeff King [off-list ref] writes:

quoted

This looks as I'd expect. But after seeing Eric's response, we perhaps
want to do away with the knob entirely.

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.

That makes total sense to me; I can't imagine a scenario where you
would want to call fsync() over fsync_or_die(). But if you did, then you
probably don't care about whether or not fsync() was interrupted, or can
check the return value yourself.

If it became more common, then I wouldn't mind #undef-ing fsync() and
replacing it with our own hacked up version.

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.

Your patch looks good to me (and Randall already tested my version with
the additional knob on NonStop and reported it working as expected[1]),
so this has my:

  Reviewed-by: Taylor Blau [off-list ref]

Thanks,
Taylor

[1]: https://lore.kernel.org/git/009901d758b4$12016d80$36044880$@nexbridge.com/

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

Am 04.06.21 um 03:36 schrieb Junio C Hamano:

The function body can be written like this:

	while (fsync(fd) < 0) {
		if (errno != EINTR)
			die_errno("fsync error on '%s'", msg);
	}

I find it easier to read because it has less syntax elements
and is shorter.  Bikeshedding, of course. :-/

René