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

On June 3, 2021 4:22 PM, Bryan Turner wrote:

On Wed, Jun 2, 2021 at 1:14 PM Jeff King [off-list ref] wrote:

quoted

On Wed, Jun 02, 2021 at 08:11:50PM +0000, Eric Wong wrote:

quoted

Jeff King [off-list ref] wrote:

quoted

And so when he gets this error:

  fatal: fsync error on '.git/objects/pack/tmp_pack_NkPgqN':
Interrupted system call

presumably we were in fsync() when the signal arrived, and unlike
most other platforms, the call needs to be restarted manually
(even though we set up the signal with SA_RESTART). I'm not sure
if this violates POSIX or not (I couldn't find a definitive answer
to the set of interruptible functions in the standard). But either
way, the workaround is probably something like:

"man 3posix fsync" says EINTR is allowed ("manpages-posix-dev"
package in Debian non-free).

Ah, thanks. Linux's fsync(3) doesn't mention it, and nor does it
appear in the discussion of interruptible calls in signals(7). So I
was looking for a POSIX equivalent of that signals manpage but
couldn't find one. :)

quoted

quoted

  #ifdef FSYNC_NEEDS_RESTART

The wrapper should apply to all platforms.  NFS (and presumably
other network FSes) can be mounted with interrupts enabled.

I don't mind that, as the wrapper is pretty low-cost (and one less
Makefile knob is nice). If it's widespread, though, I find it curious
that nobody has run into it before now.

I was dealing with a similar issue[1] recently, albeit not in the Git codebase but rather with Java. My issue was with epoll_wait, rather
than fsync, which is documented on signal(7) as not restartable even with SA_RESTART. That led me to this[2] little bit of code inside the
JVM:
#define RESTARTABLE(_cmd, _result) do { \
 do { \
   _result = _cmd; \
 } while((_result == -1) && (errno == EINTR)); \ } while(0)

which they use like this[3]:
RESTARTABLE(epoll_wait(epfd, events, numfds, -1), res);

Not sure what the Git maintainers' view on macros is, but if there wasn't going to be a Makefile knob perhaps something similar might
make sense as a reusable construct. Of course, it's unclear how often Git might _need_ such a thing; given this doesn't seem to come up
much, perhaps that's a sign such a macro would end up a waste of effort. Anyway, just thought I'd share because I was looking at
something similar.

[1] https://github.com/brettwooldridge/NuProcess/issues/124
[2]
https://github.com/JetBrains/jdk8u_jdk/blob/94318f9185757cc33d2b8d527d36be26ac6b7582/src/solaris/native/sun/nio/ch/nio_util.h#L33
-L37
[3]
https://github.com/JetBrains/jdk8u_jdk/blob/94318f9185757cc33d2b8d527d36be26ac6b7582/src/solaris/native/sun/nio/ch/EPoll.c#L92

I can only suggest, from other OpenSource projects I'm on, that make extensive use of macros, that they are very difficult to debug and sometimes harder to integrate with platform-specific compatibility layers.  I would rather have something explicit in compat.c that gdb could make sense of during a debug session if necessary. Trying to debug macros is harsh, rather like debugging -O2 without -g.

I used to be a big fan of macros, but grew out of it 😉. Just my take on it.

-Randall