Jeff King [off-list ref] writes:
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.
Yeah, I think all of the above makes sense.
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.
Yes, any caller who cares about the result of fsync() is willing to
wait, and on a well benaved system, we would never see EINTR so the
loop won't iterate. Having to carry just a handful of extra bytes
in ICache in a codepath that is not performance critical is probably
an acceptable cost for simpler code.
On June 4, 2021 1:12 AM, Junio C Hamano wrote:
To: Jeff King <redacted>
Cc: Taylor Blau <redacted>; Randall S. Becker <redacted>; git@vger.kernel.org
Subject: Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86
Jeff King [off-list ref] writes:
quoted
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.
Yeah, I think all of the above makes sense.
quoted
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.
Yes, any caller who cares about the result of fsync() is willing to wait, and on a well benaved system, we would never see EINTR so
the
loop won't iterate. Having to carry just a handful of extra bytes in ICache in a codepath that is not performance critical is
probably an
acceptable cost for simpler code.
Just something to note: I was running git gc --aggressive on OpenSSL today and got the same error as in this case. I'm assuming that
the fix will apply to that too. In this situation, the gc ran for over 2 hours until failing. This may be a more widely available
test condition for fsync() EINTR, possibly.
-Randall
On Sun, Jun 06, 2021 at 03:06:31PM -0400, Randall S. Becker wrote:
Just something to note: I was running git gc --aggressive on OpenSSL
today and got the same error as in this case. I'm assuming that the
fix will apply to that too. In this situation, the gc ran for over 2
hours until failing. This may be a more widely available test
condition for fsync() EINTR, possibly.
Thanks. One of my original questions was that we'd expect to see this in
the wild, and not just the test suite. But it sounds like now we have. :)
So I feel better that we have a full understanding of the issue.
-Peff