{"thread":{"id":"55833","subject":"RE: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","startedAt":"2021-06-02T17:52:54Z","lastAt":"2021-06-08T06:40:21Z","messageCount":22,"participants":["Randall S. Becker","Taylor Blau","Jeff King","Eric Wong","Bryan Turner","Junio C Hamano","RenĂ© Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"426244","messageId":"002701d757d8$1a8d9dc0$4fa8d940$@nexbridge.com","threadId":"55833","inReplyTo":null,"subject":"RE: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-06-02T17:52:44Z","receivedAt":"2021-06-02T17:52:54Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On June 2, 2021 4:30 AM, Junio C Hamano wrote:\n>Subject: [ANNOUNCE] Git v2.32.0-rc3\n>\n>A release candidate Git v2.32.0-rc3 is now available for testing at\n>the usual places. It is comprised of 589 non-merge commits since\n>v2.31.0, contributed by 84 people, 31 of which are new faces [*].\n\nHere is the current -x output from t5300. That last known working version was 2.31.1.\n\n\nok 1 - setup\n\nexpecting success of 5300.2 'pack without delta':\n packname_1=$(git pack-objects --progress --window=0 test-1 \\\n stderr) &&\n check_deltas stderr = 0\n\n+ + git pack-objects --progress --window=0 test-1\n+ 0< obj-list 2> stderr\npackname_1=\nerror: last command exited with $?=128\nnot ok 2 - pack without delta\n#\n# packname_1=$(git pack-objects --progress --window=0 test-1 \\\n# stderr) &&\n# check_deltas stderr = 0\n#\n\nWorking area is:\n-rw-rw-r-- 1 JENKINS.JENKINS JENKINS 4096 Jun 2 12:47 a\n-rw-rw-r-- 1 JENKINS.JENKINS JENKINS 2097152 Jun 2 12:47 a_big\n-rw-rw-r-- 1 JENKINS.JENKINS JENKINS 4096 Jun 2 12:47 b\n-rw-rw-r-- 1 JENKINS.JENKINS JENKINS 2097152 Jun 2 12:47 b_big\n-rw-rw-r-- 1 JENKINS.JENKINS JENKINS 4096 Jun 2 12:47 c\n-rw-rw-r-- 1 JENKINS.JENKINS JENKINS 4100 Jun 2 12:47 d\n-rw-rw-r-- 1 JENKINS.JENKINS JENKINS 4228179 Jun 2 12:48 expect\n-rw-rw-r-- 1 JENKINS.JENKINS JENKINS 328 Jun 2 12:48 obj-list\n-rw-rw-r-- 1 JENKINS.JENKINS JENKINS 605 Jun 2 12:48 stderr\n\nStderr contains:\nEnumerating objects: 8, done.\nCounting objects: 100% (8/8), done.\nfatal: fsync error on '.git/objects/pack/tmp_pack_NkPgqN': Interrupted system call\n\nThe maximum platform I/O size is 56Kb.\n\nI'm happy to help figure this out but need some direction. I don't know the pack-object code.\n\nSincerely,\nRandall\n\n"},{"id":"426248","messageId":"YLfc2+Te7Y3UY+Sm@nand.local","threadId":"55833","inReplyTo":"002701d757d8$1a8d9dc0$4fa8d940$@nexbridge.com","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-02T19:32:43Z","receivedAt":"2021-06-02T19:32:48Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 02, 2021 at 01:52:44PM -0400, Randall S. Becker wrote:\n> I'm happy to help figure this out but need some direction. I don't\n> know the pack-object code.\n\nIs the failure consistent, i.e., that it occurs every time you run the\ntest? Not knowing much about your platform, it would be helpful to have\na bisection showing where this breakage first occurs.\n\nThanks,\nTaylor\n"},{"id":"426252","messageId":"YLfgy94sbmStC0mR@coredump.intra.peff.net","threadId":"55833","inReplyTo":"YLfc2+Te7Y3UY+Sm@nand.local","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-02T19:49:31Z","receivedAt":"2021-06-02T19:49:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 02, 2021 at 03:32:43PM -0400, Taylor Blau wrote:\n\n> On Wed, Jun 02, 2021 at 01:52:44PM -0400, Randall S. Becker wrote:\n> > I'm happy to help figure this out but need some direction. I don't\n> > know the pack-object code.\n> \n> Is the failure consistent, i.e., that it occurs every time you run the\n> test? Not knowing much about your platform, it would be helpful to have\n> a bisection showing where this breakage first occurs.\n\nI suspect the symptom comes from the test Randall noted, but that the\nactual issue has been there all along. The test uses \"--progress\"\nexplicitly, so we'll be sending SIGALRM (whereas most tests will disable\nthe progress mechanism because their output isn't going to a tty).\n\nAnd so when he gets this error:\n\n fatal: fsync error on '.git/objects/pack/tmp_pack_NkPgqN': Interrupted system call\n\npresumably we were in fsync() when the signal arrived, and unlike most\nother platforms, the call needs to be restarted manually (even though we\nset up the signal with SA_RESTART). I'm not sure if this violates POSIX\nor not (I couldn't find a definitive answer to the set of interruptible\nfunctions in the standard). But either way, the workaround is probably\nsomething like:\n\n\n #ifdef FSYNC_NEEDS_RESTART\n #undef fsync /* we'd define to git_fsync() in a header file */\n static int git_fsync(int fd)\n {\n\tint ret;\n\twhile ((ret = fsync(fd)) < 0 && errno == EINTR)\n\t\t; /* try again */\n\treturn ret;\n }\n #endif\n\n-Peff\n"},{"id":"426256","messageId":"YLfl4jkuwSCiNrrS@nand.local","threadId":"55833","inReplyTo":"YLfgy94sbmStC0mR@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-02T20:11:14Z","receivedAt":"2021-06-02T20:11:19Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 02, 2021 at 03:49:31PM -0400, Jeff King wrote:\n> And so when he gets this error:\n>\n> fatal: fsync error on '.git/objects/pack/tmp_pack_NkPgqN': Interrupted system call\n>\n> presumably we were in fsync() when the signal arrived, and unlike most\n> other platforms, the call needs to be restarted manually (even though we\n> set up the signal with SA_RESTART).\n\nAha, thanks for explaining. That makes sense to me.\n\n> I'm not sure if this violates POSIX or not (I couldn't find a\n> definitive answer to the set of interruptible functions in the\n> standard).\n\nI couldn't find anything either, but I believe you that what you\noutlined is the right solution.\n\n> But either way, the workaround is probably something like:\n\nSeems very reasonable. Here's a patch:\n\n--- >8 ---\n\nSubject: [PATCH] compat: introduce git_fsync_with_restart()\n\nSome platforms, like NonStop do not automatically restart fsync() when\ninterrupted by a signal, even when that signal is setup with SA_RESTART.\n\nThis can lead to test breakage, e.g., where \"--progress\" is used, thus\nSIGALRM is sent often, and can interrupt an fsync() syscall.\n\nAdd a Makefile knob FSYNC_NEEDS_RESTART to replace fsync() with one that\ngracefully handles getting EINTR.\n\nReported-by: Randall S. Becker \nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n Makefile | 8 ++++++++\n compat/fsync.c | 10 ++++++++++\n git-compat-util.h | 6 ++++++\n 3 files changed, 24 insertions(+)\n create mode 100644 compat/fsync.c\n\ndiff --git a/Makefile b/Makefile\nindex c3565fc0f8..ebea693a2c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -423,6 +423,9 @@ all::\n # Define NEED_ACCESS_ROOT_HANDLER if access() under root may success for X_OK\n # even if execution permission isn't granted for any user.\n #\n+# Define FSYNC_NEEDS_RESTART if your platform's fsync() needs to be restarted\n+# manually when interrupted by a signal.\n+#\n # Define PAGER_ENV to a SP separated VAR=VAL pairs to define\n # default environment variables to be passed when a pager is spawned, e.g.\n #\n@@ -1926,6 +1929,11 @@ ifdef NEED_ACCESS_ROOT_HANDLER\n \tCOMPAT_OBJS += compat/access.o\n endif\n\n+ifdef FSYNC_NEEDS_RESTART\n+\tCOMPAT_CFLAGS += -DFSYNC_NEEDS_RESTART\n+\tCOMPAT_OBJS += compat/fsync.o\n+endif\n+\n ifeq ($(TCLTK_PATH),)\n NO_TCLTK = NoThanks\n endif\ndiff --git a/compat/fsync.c b/compat/fsync.c\nnew file mode 100644\nindex 0000000000..f340fc628b\n--- /dev/null\n+++ b/compat/fsync.c\n@@ -0,0 +1,10 @@\n+#include \"git-compat-util.h\"\n+\n+#undef fsync\n+int git_fsync_with_restart(int fd)\n+{\n+\tint ret;\n+\twhile ((ret = fsync(fd)) < 0 && errno == EINTR)\n+\t\t; /* try again */\n+\treturn ret;\n+}\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex a508dbe5a3..972fdb609f 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -833,6 +833,12 @@ const char *inet_ntop(int af, const void *src, char *dst, size_t size);\n int git_atexit(void (*handler)(void));\n #endif\n\n+#ifdef FSYNC_NEEDS_RESTART\n+#undef fsync\n+#define fsync git_fsync_with_restart\n+int git_fsync_with_restart(int fd);\n+#endif\n+\n static inline size_t st_add(size_t a, size_t b)\n {\n \tif (unsigned_add_overflows(a, b))\n--\n2.31.1.163.ga65ce7f831\n\n"},{"id":"426257","messageId":"20210602201150.GA29388@dcvr","threadId":"55833","inReplyTo":"YLfgy94sbmStC0mR@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-06-02T20:11:50Z","receivedAt":"2021-06-02T20:11:52Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> And so when he gets this error:\n> \n> fatal: fsync error on '.git/objects/pack/tmp_pack_NkPgqN': Interrupted system call\n> \n> presumably we were in fsync() when the signal arrived, and unlike most\n> other platforms, the call needs to be restarted manually (even though we\n> set up the signal with SA_RESTART). I'm not sure if this violates POSIX\n> or not (I couldn't find a definitive answer to the set of interruptible\n> functions in the standard). But either way, the workaround is probably\n> something like:\n\n\"man 3posix fsync\" says EINTR is allowed (\"manpages-posix-dev\"\npackage in Debian non-free).\n\n> #ifdef FSYNC_NEEDS_RESTART\n\nThe wrapper should apply to all platforms. NFS (and presumably\nother network FSes) can be mounted with interrupts enabled.\n\n> #undef fsync /* we'd define to git_fsync() in a header file */\n> static int git_fsync(int fd)\n> {\n> \tint ret;\n> \twhile ((ret = fsync(fd)) < 0 && errno == EINTR)\n> \t\t; /* try again */\n> \treturn ret;\n> }\n\nLooks fine.\n"},{"id":"426259","messageId":"YLfmo8kl0URnGgp5@coredump.intra.peff.net","threadId":"55833","inReplyTo":"20210602201150.GA29388@dcvr","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-02T20:14:27Z","receivedAt":"2021-06-02T20:14:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 02, 2021 at 08:11:50PM +0000, Eric Wong wrote:\n\n> Jeff King wrote:\n> > And so when he gets this error:\n> > \n> > fatal: fsync error on '.git/objects/pack/tmp_pack_NkPgqN': Interrupted system call\n> > \n> > presumably we were in fsync() when the signal arrived, and unlike most\n> > other platforms, the call needs to be restarted manually (even though we\n> > set up the signal with SA_RESTART). I'm not sure if this violates POSIX\n> > or not (I couldn't find a definitive answer to the set of interruptible\n> > functions in the standard). But either way, the workaround is probably\n> > something like:\n> \n> \"man 3posix fsync\" says EINTR is allowed (\"manpages-posix-dev\"\n> package in Debian non-free).\n\nAh, thanks. Linux's fsync(3) doesn't mention it, and nor does it appear\nin the discussion of interruptible calls in signals(7). So I was looking\nfor a POSIX equivalent of that signals manpage but couldn't find one. :)\n\n> > #ifdef FSYNC_NEEDS_RESTART\n> \n> The wrapper should apply to all platforms. NFS (and presumably\n> other network FSes) can be mounted with interrupts enabled.\n\nI don't mind that, as the wrapper is pretty low-cost (and one less\nMakefile knob is nice). If it's widespread, though, I find it curious\nthat nobody has run into it before now.\n\n-Peff\n"},{"id":"426260","messageId":"YLfm8cqY6EjQuhcO@coredump.intra.peff.net","threadId":"55833","inReplyTo":"YLfl4jkuwSCiNrrS@nand.local","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-02T20:15:45Z","receivedAt":"2021-06-02T20:15:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 02, 2021 at 04:11:14PM -0400, Taylor Blau wrote:\n\n> Subject: [PATCH] compat: introduce git_fsync_with_restart()\n> \n> Some platforms, like NonStop do not automatically restart fsync() when\n> interrupted by a signal, even when that signal is setup with SA_RESTART.\n> \n> This can lead to test breakage, e.g., where \"--progress\" is used, thus\n> SIGALRM is sent often, and can interrupt an fsync() syscall.\n> \n> Add a Makefile knob FSYNC_NEEDS_RESTART to replace fsync() with one that\n> gracefully handles getting EINTR.\n> \n> Reported-by: Randall S. Becker \n> Signed-off-by: Jeff King \n\nProbably Helped-by might be more appropriate. But regardless, I\ndefinitely give my S-o-B for anything I contributed.\n\n> Makefile | 8 ++++++++\n> compat/fsync.c | 10 ++++++++++\n> git-compat-util.h | 6 ++++++\n> 3 files changed, 24 insertions(+)\n> create mode 100644 compat/fsync.c\n\nThis looks as I'd expect. But after seeing Eric's response, we perhaps\nwant to do away with the knob entirely.\n\n-Peff\n"},{"id":"426261","messageId":"YLfnnkl3UfuU6Yo/@nand.local","threadId":"55833","inReplyTo":"YLfmo8kl0URnGgp5@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-02T20:18:38Z","receivedAt":"2021-06-02T20:19:52Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 02, 2021 at 04:14:27PM -0400, Jeff King wrote:\n> > The wrapper should apply to all platforms. NFS (and presumably\n> > other network FSes) can be mounted with interrupts enabled.\n>\n> I don't mind that, as the wrapper is pretty low-cost (and one less\n> Makefile knob is nice). If it's widespread, though, I find it curious\n> that nobody has run into it before now.\n\nYeah, I find it curious as well. I agree that the wrapper is relatively\ncheap (especially relative to fsync), but it seems unnecessary despite\nwhat's in POSIX's fsync(3). So the more conservative choice to me would\nbe to hide it behind a Makefile knob, but I don't mind much either way.\n\nThanks,\nTaylor\n"},{"id":"426263","messageId":"003c01d757ee$c0664600$4132d200$@nexbridge.com","threadId":"55833","inReplyTo":"YLfmo8kl0URnGgp5@coredump.intra.peff.net","subject":"RE: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-06-02T20:34:51Z","receivedAt":"2021-06-02T20:35:10Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On June 2, 2021 4:14 PM, Peff wrote:\n>Subject: Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86\n>On Wed, Jun 02, 2021 at 08:11:50PM +0000, Eric Wong wrote:\n>\n>> Jeff King wrote:\n>> > And so when he gets this error:\n>> >\n>> > fatal: fsync error on '.git/objects/pack/tmp_pack_NkPgqN':\n>> > Interrupted system call\n>> >\n>> > presumably we were in fsync() when the signal arrived, and unlike\n>> > most other platforms, the call needs to be restarted manually (even\n>> > though we set up the signal with SA_RESTART). I'm not sure if this\n>> > violates POSIX or not (I couldn't find a definitive answer to the\n>> > set of interruptible functions in the standard). But either way, the\n>> > workaround is probably something like:\n>>\n>> \"man 3posix fsync\" says EINTR is allowed (\"manpages-posix-dev\"\n>> package in Debian non-free).\n>\n>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\n>for a POSIX equivalent of that signals manpage but couldn't find one. :)\n>\n>> > #ifdef FSYNC_NEEDS_RESTART\n>>\n>> The wrapper should apply to all platforms. NFS (and presumably other\n>> network FSes) can be mounted with interrupts enabled.\n>\n>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\n>nobody has run into it before now.\n\nI suspect this is because of the way the file system on NonStop behaves. It is a multi-processor platform, with multi-cores, so anything can happen. If the file system is delayed for any reason, like a signal coming from a different core (EINTR has high priority), then fsync() will be interrupted. EINTR is allowed on NonStop for fsync(). So it would be really great if the patch included a modification to config.mak.uname to include that. This would be a timing-only issue on most other systems, probably something that would hit NFS.\n\nThe patch for the config is:\ndiff --git a/config.mak.uname b/config.mak.uname\nindex cb443b4e02..ac3e3ca2c5 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -566,6 +566,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n NO_REGEX = NeedsStartEnd\n NO_PTHREADS = UnfortunatelyYes\n FREAD_READS_DIRECTORIES = UnfortunatelyYes\n+ FSYNC_NEEDS_RESTART = YesPlease\n\n # Not detected (nor checked for) by './configure'.\n\n\n\n"},{"id":"426264","messageId":"003d01d757ee$ebe93080$c3bb9180$@nexbridge.com","threadId":"55833","inReplyTo":"YLfm8cqY6EjQuhcO@coredump.intra.peff.net","subject":"RE: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-06-02T20:36:04Z","receivedAt":"2021-06-02T20:36:18Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On June 2, 2021 4:16 PM, Peff wrote:\n>To: Taylor Blau \n>Cc: Randall S. Becker ; 'Junio C Hamano' ; git@vger.kernel.org\n>Subject: Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86\n>\n>On Wed, Jun 02, 2021 at 04:11:14PM -0400, Taylor Blau wrote:\n>\n>> Subject: [PATCH] compat: introduce git_fsync_with_restart()\n>>\n>> Some platforms, like NonStop do not automatically restart fsync() when\n>> interrupted by a signal, even when that signal is setup with SA_RESTART.\n>>\n>> This can lead to test breakage, e.g., where \"--progress\" is used, thus\n>> SIGALRM is sent often, and can interrupt an fsync() syscall.\n>>\n>> Add a Makefile knob FSYNC_NEEDS_RESTART to replace fsync() with one\n>> that gracefully handles getting EINTR.\n>>\n>> Reported-by: Randall S. Becker \n>> Signed-off-by: Jeff King \n>\n>Probably Helped-by might be more appropriate. But regardless, I definitely give my S-o-B for anything I contributed.\n>\n>> Makefile | 8 ++++++++\n>> compat/fsync.c | 10 ++++++++++\n>> git-compat-util.h | 6 ++++++\n>> 3 files changed, 24 insertions(+)\n>> create mode 100644 compat/fsync.c\n>\n>This looks as I'd expect. But after seeing Eric's response, we perhaps want to do away with the knob entirely.\n\nI'm for doing away with the knob altogether, providing you don't mean the platform itself đŸ˜‰\n\nRegards,\nRandall\n\n"},{"id":"426351","messageId":"YLkt+w9Lxyy8iLS5@coredump.intra.peff.net","threadId":"55833","inReplyTo":"003c01d757ee$c0664600$4132d200$@nexbridge.com","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-03T19:31:07Z","receivedAt":"2021-06-03T19:31:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 02, 2021 at 04:34:51PM -0400, Randall S. Becker wrote:\n\n> >> The wrapper should apply to all platforms. NFS (and presumably other\n> >> network FSes) can be mounted with interrupts enabled.\n> >\n> >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\n> >nobody has run into it before now.\n> \n> I suspect this is because of the way the file system on NonStop behaves. It is a multi-processor platform, with multi-cores, so anything can happen. If the file system is delayed for any reason, like a signal coming from a different core (EINTR has high priority), then fsync() will be interrupted. EINTR is allowed on NonStop for fsync(). So it would be really great if the patch included a modification to config.mak.uname to include that. This would be a timing-only issue on most other systems, probably something that would hit NFS.\n> \n> The patch for the config is:\n> diff --git a/config.mak.uname b/config.mak.uname\n> index cb443b4e02..ac3e3ca2c5 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -566,6 +566,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n> NO_REGEX = NeedsStartEnd\n> NO_PTHREADS = UnfortunatelyYes\n> FREAD_READS_DIRECTORIES = UnfortunatelyYes\n> + FSYNC_NEEDS_RESTART = YesPlease\n> \n> # Not detected (nor checked for) by './configure'.\n\nYeah, if we don't make it unconditional, then this is the obvious next\nstep. But the more important question is: did you test this out and did\nit fix the test breakage you saw on NonStop?\n\n-Peff\n"},{"id":"426354","messageId":"009901d758b4$12016d80$36044880$@nexbridge.com","threadId":"55833","inReplyTo":"YLkt+w9Lxyy8iLS5@coredump.intra.peff.net","subject":"RE: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-06-03T20:07:19Z","receivedAt":"2021-06-03T20:07:39Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On June 3, 2021 3:31 PM, Peff wrote:\n>On Wed, Jun 02, 2021 at 04:34:51PM -0400, Randall S. Becker wrote:\n>\n>> >> The wrapper should apply to all platforms. NFS (and presumably\n>> >> other network FSes) can be mounted with interrupts enabled.\n>> >\n>> >I don't mind that, as the wrapper is pretty low-cost (and one less\n>> >Makefile knob is nice). If it's widespread, though, I find it curious that nobody has run into it before now.\n>>\n>> I suspect this is because of the way the file system on NonStop behaves. It is a multi-processor platform, with multi-cores, so anything\n>can happen. If the file system is delayed for any reason, like a signal coming from a different core (EINTR has high priority), then fsync()\n>will be interrupted. EINTR is allowed on NonStop for fsync(). So it would be really great if the patch included a modification to\n>config.mak.uname to include that. This would be a timing-only issue on most other systems, probably something that would hit NFS.\n>>\n>> The patch for the config is:\n>> diff --git a/config.mak.uname b/config.mak.uname index\n>> cb443b4e02..ac3e3ca2c5 100644\n>> --- a/config.mak.uname\n>> +++ b/config.mak.uname\n>> @@ -566,6 +566,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n>> NO_REGEX = NeedsStartEnd\n>> NO_PTHREADS = UnfortunatelyYes\n>> FREAD_READS_DIRECTORIES = UnfortunatelyYes\n>> + FSYNC_NEEDS_RESTART = YesPlease\n>>\n>> # Not detected (nor checked for) by './configure'.\n>\n>Yeah, if we don't make it unconditional, then this is the obvious next step. But the more important question is: did you test this out and did\n>it fix the test breakage you saw on NonStop?\n\nThe fix works for me and t5300 passes. I tested it without the conditional approach. While the test was running, I noticed this:\n\n+ mkdir -p /home/git/git/t/trash directory.t5300-pack-object/prereq-test-dir-FAIL_PREREQS\n+ cd /home/git/git/t/trash directory.t5300-pack-object/prereq-test-dir-FAIL_PREREQS\n+ test_bool_env GIT_TEST_FAIL_PREREQS false\nerror: last command exited with $?=1\nprerequisite FAIL_PREREQS not satisfied\nexpecting success of 5300.32 'index-pack --threads=N or pack.threads=N warns when no pthreads':\n\nThis may be intended, but the error line showed in red.\n\nRegards,\nRandall\n\n\n"},{"id":"426356","messageId":"CAGyf7-G=B+S6m9mifjOCFKGfEx69zzdOoCr03ckK7fJZrNEGtg@mail.gmail.com","threadId":"55833","inReplyTo":"YLfmo8kl0URnGgp5@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2021-06-03T20:21:52Z","receivedAt":"2021-06-03T20:23:06Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Wed, Jun 2, 2021 at 1:14 PM Jeff King wrote:\n>\n> On Wed, Jun 02, 2021 at 08:11:50PM +0000, Eric Wong wrote:\n>\n> > Jeff King wrote:\n> > > And so when he gets this error:\n> > >\n> > > fatal: fsync error on '.git/objects/pack/tmp_pack_NkPgqN': Interrupted system call\n> > >\n> > > presumably we were in fsync() when the signal arrived, and unlike most\n> > > other platforms, the call needs to be restarted manually (even though we\n> > > set up the signal with SA_RESTART). I'm not sure if this violates POSIX\n> > > or not (I couldn't find a definitive answer to the set of interruptible\n> > > functions in the standard). But either way, the workaround is probably\n> > > something like:\n> >\n> > \"man 3posix fsync\" says EINTR is allowed (\"manpages-posix-dev\"\n> > package in Debian non-free).\n>\n> Ah, thanks. Linux's fsync(3) doesn't mention it, and nor does it appear\n> in the discussion of interruptible calls in signals(7). So I was looking\n> for a POSIX equivalent of that signals manpage but couldn't find one. :)\n>\n> > > #ifdef FSYNC_NEEDS_RESTART\n> >\n> > The wrapper should apply to all platforms. NFS (and presumably\n> > other network FSes) can be mounted with interrupts enabled.\n>\n> I don't mind that, as the wrapper is pretty low-cost (and one less\n> Makefile knob is nice). If it's widespread, though, I find it curious\n> that nobody has run into it before now.\n\nI was dealing with a similar issue[1] recently, albeit not in the Git\ncodebase but rather with Java. My issue was with epoll_wait, rather\nthan fsync, which is documented on signal(7) as not restartable even\nwith SA_RESTART. That led me to this[2] little bit of code inside the\nJVM:\n#define RESTARTABLE(_cmd, _result) do { \\\n do { \\\n _result = _cmd; \\\n } while((_result == -1) && (errno == EINTR)); \\\n} while(0)\n\nwhich they use like this[3]:\nRESTARTABLE(epoll_wait(epfd, events, numfds, -1), res);\n\nNot sure what the Git maintainers' view on macros is, but if there\nwasn't going to be a Makefile knob perhaps something similar might\nmake sense as a reusable construct. Of course, it's unclear how often\nGit might _need_ such a thing; given this doesn't seem to come up\nmuch, perhaps that's a sign such a macro would end up a waste of\neffort. Anyway, just thought I'd share because I was looking at\nsomething similar.\n\n[1] https://github.com/brettwooldridge/NuProcess/issues/124\n[2] https://github.com/JetBrains/jdk8u_jdk/blob/94318f9185757cc33d2b8d527d36be26ac6b7582/src/solaris/native/sun/nio/ch/nio_util.h#L33-L37\n[3] https://github.com/JetBrains/jdk8u_jdk/blob/94318f9185757cc33d2b8d527d36be26ac6b7582/src/solaris/native/sun/nio/ch/EPoll.c#L92\n\n>\n> -Peff\n"},{"id":"426358","messageId":"009c01d758b7$86e12340$94a369c0$@nexbridge.com","threadId":"55833","inReplyTo":"CAGyf7-G=B+S6m9mifjOCFKGfEx69zzdOoCr03ckK7fJZrNEGtg@mail.gmail.com","subject":"RE: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-06-03T20:32:03Z","receivedAt":"2021-06-03T20:32:23Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On June 3, 2021 4:22 PM, Bryan Turner wrote:\n>On Wed, Jun 2, 2021 at 1:14 PM Jeff King wrote:\n>>\n>> On Wed, Jun 02, 2021 at 08:11:50PM +0000, Eric Wong wrote:\n>>\n>> > Jeff King wrote:\n>> > > And so when he gets this error:\n>> > >\n>> > > fatal: fsync error on '.git/objects/pack/tmp_pack_NkPgqN':\n>> > > Interrupted system call\n>> > >\n>> > > presumably we were in fsync() when the signal arrived, and unlike\n>> > > most other platforms, the call needs to be restarted manually\n>> > > (even though we set up the signal with SA_RESTART). I'm not sure\n>> > > if this violates POSIX or not (I couldn't find a definitive answer\n>> > > to the set of interruptible functions in the standard). But either\n>> > > way, the workaround is probably something like:\n>> >\n>> > \"man 3posix fsync\" says EINTR is allowed (\"manpages-posix-dev\"\n>> > package in Debian non-free).\n>>\n>> Ah, thanks. Linux's fsync(3) doesn't mention it, and nor does it\n>> appear in the discussion of interruptible calls in signals(7). So I\n>> was looking for a POSIX equivalent of that signals manpage but\n>> couldn't find one. :)\n>>\n>> > > #ifdef FSYNC_NEEDS_RESTART\n>> >\n>> > The wrapper should apply to all platforms. NFS (and presumably\n>> > other network FSes) can be mounted with interrupts enabled.\n>>\n>> I don't mind that, as the wrapper is pretty low-cost (and one less\n>> Makefile knob is nice). If it's widespread, though, I find it curious\n>> that nobody has run into it before now.\n>\n>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\n>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\n>JVM:\n>#define RESTARTABLE(_cmd, _result) do { \\\n> do { \\\n> _result = _cmd; \\\n> } while((_result == -1) && (errno == EINTR)); \\ } while(0)\n>\n>which they use like this[3]:\n>RESTARTABLE(epoll_wait(epfd, events, numfds, -1), res);\n>\n>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\n>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\n>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\n>something similar.\n>\n>[1] https://github.com/brettwooldridge/NuProcess/issues/124\n>[2]\n>https://github.com/JetBrains/jdk8u_jdk/blob/94318f9185757cc33d2b8d527d36be26ac6b7582/src/solaris/native/sun/nio/ch/nio_util.h#L33\n>-L37\n>[3]\n>https://github.com/JetBrains/jdk8u_jdk/blob/94318f9185757cc33d2b8d527d36be26ac6b7582/src/solaris/native/sun/nio/ch/EPoll.c#L92\n\nI 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.\n\nI used to be a big fan of macros, but grew out of it đŸ˜‰. Just my take on it.\n\n-Randall\n\n"},{"id":"426374","messageId":"xmqqk0na2yyc.fsf@gitster.g","threadId":"55833","inReplyTo":"YLfm8cqY6EjQuhcO@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-04T01:36:11Z","receivedAt":"2021-06-04T01:36:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This looks as I'd expect. But after seeing Eric's response, we perhaps\n> want to do away with the knob entirely.\n\nThanks. I was hoping somebody in the thread would tie the loose\nends, but upon inspection of the output from\n\n $ git grep -e fsync\\( maint seen -- \\*.[ch]\n\nit turns out that fsync_or_die() is the only place that calls\nfsync(), so perhaps doing it in a way that is quite different from\nwhat has been discussed may be even a better alternative.\n\nIf any new callers care about the return value of fsync(), I'd\nexpect that they would be calling this wrapper, and the \"best\neffort\" callers that do not check the returned value by definition\ndo not care if fsync() does not complete due to an interrupt, so I\nam hoping that the current \"we only call it from this wrapper\" is\nnot just \"the code currently happens to be this way\", but it is\nsensible that the code will stay that way in the future.\n\nObviously I appreciate reviews and possibly tests, but sanity\nchecking my observation that fsync() is called only from here is a\ngood thing to have.\n\n-- >8 --\nSubject: fsync(): be prepared to see EINTR\n\nSome platforms, like NonStop do not automatically restart fsync()\nwhen interrupted by a signal, even when that signal is setup with\nSA_RESTART.\n\nThis can lead to test breakage, e.g., where \"--progress\" is used,\nthus SIGALRM is sent often, and can interrupt an fsync() syscall.\n\nMake sure we deal with such a case by retrying the syscall\nourselves. Luckily, we call fsync() fron a single wrapper,\nfsync_or_die(), so the fix is fairly isolated.\n \nReported-by: Randall S. Becker \nHelped-by: Jeff King \nHelped-by: Taylor Blau \n[jc: the above two did most of the work---I just tied the loose end]\nSigned-off-by: Junio C Hamano \n---\n\n write-or-die.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git i/write-or-die.c w/write-or-die.c\nindex eab8c8d0b9..534b2f5cba 100644\n--- i/write-or-die.c\n+++ w/write-or-die.c\n@@ -57,7 +57,11 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)\n \n void fsync_or_die(int fd, const char *msg)\n {\n-\tif (fsync(fd) < 0) {\n+\tint status;\n+\n+\twhile ((status = fsync(fd)) < 0 && errno == EINTR)\n+\t\t; /* try again */\n+\tif (status < 0) {\n \t\tdie_errno(\"fsync error on '%s'\", msg);\n \t}\n }\n"},{"id":"426379","messageId":"YLmNHpf+dXdK7OeH@nand.local","threadId":"55833","inReplyTo":"xmqqk0na2yyc.fsf@gitster.g","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-04T02:17:02Z","receivedAt":"2021-06-04T02:17:15Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jun 04, 2021 at 10:36:11AM +0900, Junio C Hamano wrote:\n> Jeff King writes:\n>\n> > This looks as I'd expect. But after seeing Eric's response, we perhaps\n> > want to do away with the knob entirely.\n>\n> Thanks. I was hoping somebody in the thread would tie the loose\n> ends, but upon inspection of the output from\n>\n> $ git grep -e fsync\\( maint seen -- \\*.[ch]\n>\n> it turns out that fsync_or_die() is the only place that calls\n> fsync(), so perhaps doing it in a way that is quite different from\n> what has been discussed may be even a better alternative.\n>\n> If any new callers care about the return value of fsync(), I'd\n> expect that they would be calling this wrapper, and the \"best\n> effort\" callers that do not check the returned value by definition\n> do not care if fsync() does not complete due to an interrupt, so I\n> am hoping that the current \"we only call it from this wrapper\" is\n> not just \"the code currently happens to be this way\", but it is\n> sensible that the code will stay that way in the future.\n\nThat makes total sense to me; I can't imagine a scenario where you\nwould want to call fsync() over fsync_or_die(). But if you did, then you\nprobably don't care about whether or not fsync() was interrupted, or can\ncheck the return value yourself.\n\nIf it became more common, then I wouldn't mind #undef-ing fsync() and\nreplacing it with our own hacked up version.\n\n> Obviously I appreciate reviews and possibly tests, but sanity\n> checking my observation that fsync() is called only from here is a\n> good thing to have.\n\nYour patch looks good to me (and Randall already tested my version with\nthe additional knob on NonStop and reported it working as expected[1]),\nso this has my:\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/009901d758b4$12016d80$36044880$@nexbridge.com/\n"},{"id":"426386","messageId":"YLmkI4a4J60KFY2W@coredump.intra.peff.net","threadId":"55833","inReplyTo":"xmqqk0na2yyc.fsf@gitster.g","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-04T03:55:15Z","receivedAt":"2021-06-04T03:55:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 04, 2021 at 10:36:11AM +0900, Junio C Hamano wrote:\n\n> Thanks. I was hoping somebody in the thread would tie the loose\n> ends, but upon inspection of the output from\n> \n> $ git grep -e fsync\\( maint seen -- \\*.[ch]\n> \n> it turns out that fsync_or_die() is the only place that calls\n> fsync(), so perhaps doing it in a way that is quite different from\n> what has been discussed may be even a better alternative.\n> \n> If any new callers care about the return value of fsync(), I'd\n> expect that they would be calling this wrapper, and the \"best\n> effort\" callers that do not check the returned value by definition\n> do not care if fsync() does not complete due to an interrupt, so I\n> am hoping that the current \"we only call it from this wrapper\" is\n> not just \"the code currently happens to be this way\", but it is\n> sensible that the code will stay that way in the future.\n> \n> Obviously I appreciate reviews and possibly tests, but sanity\n> checking my observation that fsync() is called only from here is a\n> good thing to have.\n\nThanks for digging further. I didn't even think to look at how many\ncalls there were, but I agree there is only the one. And moreover, I\nagree that it is unlikely we'd ever have more than one, for the reasons\nyou listed. So I think your patch is a nice and simple solution, and we\ndon't need worry about magic macro wrappers at all.\n\nOne brief aside: I'm still not entirely convinced that NonStop isn't\nviolating POSIX. Yes, as Eric noted, fsync() is allowed to return EINTR.\nBut should it do so when the signal it got was set up with SA_RESTART?\n\nThe sigaction(3posix) page says:\n\n SA_RESTART This flag affects the behavior of interruptible functions;\n\t\t that is, those specified to fail with errno set to\n\t\t [EINTR]. If set, and a function specified as\n\t\t interruptible is interrupted by this signal, the\n\t\t function shall restart and shall not fail with [EINTR]\n\t\t unless otherwise specified. [...]\n\nand I could not find anywhere that it is \"otherwise specified\" for\nfsync(). Of course, whatever POSIX says, if NonStop needs this\nworkaround, we should provide it. But this may explain why we never saw\nit on other systems.\n\nIt also means it's less important for this workaround to kick in\neverywhere. But given how low-cost it is, I'm just as happy to avoid\nhaving a separate knob to enable it.\n\n> -- >8 --\n> Subject: fsync(): be prepared to see EINTR\n\nThe patch itself looks good to me.\n\n-Peff\n"},{"id":"426395","messageId":"xmqq7dja2oyd.fsf@gitster.g","threadId":"55833","inReplyTo":"YLmkI4a4J60KFY2W@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-04T05:12:10Z","receivedAt":"2021-06-04T05:12:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> One brief aside: I'm still not entirely convinced that NonStop isn't\n> violating POSIX. Yes, as Eric noted, fsync() is allowed to return EINTR.\n> But should it do so when the signal it got was set up with SA_RESTART?\n>\n> The sigaction(3posix) page says:\n>\n> SA_RESTART This flag affects the behavior of interruptible functions;\n> \t\t that is, those specified to fail with errno set to\n> \t\t [EINTR]. If set, and a function specified as\n> \t\t interruptible is interrupted by this signal, the\n> \t\t function shall restart and shall not fail with [EINTR]\n> \t\t unless otherwise specified. [...]\n>\n> and I could not find anywhere that it is \"otherwise specified\" for\n> fsync(). Of course, whatever POSIX says, if NonStop needs this\n> workaround, we should provide it. But this may explain why we never saw\n> it on other systems.\n\nYeah, I think all of the above makes sense.\n\n> It also means it's less important for this workaround to kick in\n> everywhere. But given how low-cost it is, I'm just as happy to avoid\n> having a separate knob to enable it.\n\nYes, any caller who cares about the result of fsync() is willing to\nwait, and on a well benaved system, we would never see EINTR so the\nloop won't iterate. Having to carry just a handful of extra bytes\nin ICache in a codepath that is not performance critical is probably\nan acceptable cost for simpler code.\n"},{"id":"426473","messageId":"9b663ba3-6680-0d9a-4910-502cb2abac6b@web.de","threadId":"55833","inReplyTo":"xmqqk0na2yyc.fsf@gitster.g","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"RenĂ© Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-05T07:04:49Z","receivedAt":"2021-06-05T07:05:23Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.06.21 um 03:36 schrieb Junio C Hamano:\n> ---\n>\n> write-or-die.c | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git i/write-or-die.c w/write-or-die.c\n> index eab8c8d0b9..534b2f5cba 100644\n> --- i/write-or-die.c\n> +++ w/write-or-die.c\n> @@ -57,7 +57,11 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)\n>\n> void fsync_or_die(int fd, const char *msg)\n> {\n> -\tif (fsync(fd) < 0) {\n> +\tint status;\n> +\n> +\twhile ((status = fsync(fd)) < 0 && errno == EINTR)\n> +\t\t; /* try again */\n> +\tif (status < 0) {\n> \t\tdie_errno(\"fsync error on '%s'\", msg);\n> \t}\n> }\n\nThe function body can be written like this:\n\n\twhile (fsync(fd) < 0) {\n\t\tif (errno != EINTR)\n\t\t\tdie_errno(\"fsync error on '%s'\", msg);\n\t}\n\nI find it easier to read because it has less syntax elements\nand is shorter. Bikeshedding, of course. :-/\n\nRenĂ©\n"},{"id":"426488","messageId":"xmqqeedg1mgn.fsf@gitster.g","threadId":"55833","inReplyTo":"9b663ba3-6680-0d9a-4910-502cb2abac6b@web.de","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-05T13:15:52Z","receivedAt":"2021-06-05T13:15:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"RenĂ© Scharfe writes:\n\n> The function body can be written like this:\n>\n> \twhile (fsync(fd) < 0) {\n> \t\tif (errno != EINTR)\n> \t\t\tdie_errno(\"fsync error on '%s'\", msg);\n> \t}\n>\n> I find it easier to read because it has less syntax elements\n> and is shorter. Bikeshedding, of course. :-/\n\nIt indeed is easier to follow. Thanks.\n\n"},{"id":"426545","messageId":"015301d75b07$132a0aa0$397e1fe0$@nexbridge.com","threadId":"55833","inReplyTo":"xmqq7dja2oyd.fsf@gitster.g","subject":"RE: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2021-06-06T19:06:31Z","receivedAt":"2021-06-06T19:06:50Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On June 4, 2021 1:12 AM, Junio C Hamano wrote:\n>To: Jeff King \n>Cc: Taylor Blau ; Randall S. Becker ; git@vger.kernel.org\n>Subject: Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86\n>\n>Jeff King writes:\n>\n>> One brief aside: I'm still not entirely convinced that NonStop isn't\n>> violating POSIX. Yes, as Eric noted, fsync() is allowed to return EINTR.\n>> But should it do so when the signal it got was set up with SA_RESTART?\n>>\n>> The sigaction(3posix) page says:\n>>\n>> SA_RESTART This flag affects the behavior of interruptible functions;\n>> \t\t that is, those specified to fail with errno set to\n>> \t\t [EINTR]. If set, and a function specified as\n>> \t\t interruptible is interrupted by this signal, the\n>> \t\t function shall restart and shall not fail with [EINTR]\n>> \t\t unless otherwise specified. [...]\n>>\n>> and I could not find anywhere that it is \"otherwise specified\" for\n>> fsync(). Of course, whatever POSIX says, if NonStop needs this\n>> workaround, we should provide it. But this may explain why we never\n>> saw it on other systems.\n>\n>Yeah, I think all of the above makes sense.\n>\n>> It also means it's less important for this workaround to kick in\n>> everywhere. But given how low-cost it is, I'm just as happy to avoid\n>> having a separate knob to enable it.\n>\n>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\nthe\n>loop won't iterate. Having to carry just a handful of extra bytes in ICache in a codepath that is not performance critical is\nprobably an\n>acceptable cost for simpler code.\n\nJust 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\nthe 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\ntest condition for fsync() EINTR, possibly.\n\n-Randall\n\n"},{"id":"426715","messageId":"YL8Q08sPWCMYN9Y1@coredump.intra.peff.net","threadId":"55833","inReplyTo":"015301d75b07$132a0aa0$397e1fe0$@nexbridge.com","subject":"Re: [ANNOUNCE] Git v2.32.0-rc3 - t5300 Still Broken on NonStop ia64/x86","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-08T06:40:19Z","receivedAt":"2021-06-08T06:40:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 06, 2021 at 03:06:31PM -0400, Randall S. Becker wrote:\n\n> Just something to note: I was running git gc --aggressive on OpenSSL\n> today and got the same error as in this case. I'm assuming that the\n> fix will apply to that too. In this situation, the gc ran for over 2\n> hours until failing. This may be a more widely available test\n> condition for fsync() EINTR, possibly.\n\nThanks. One of my original questions was that we'd expect to see this in\nthe wild, and not just the test suite. But it sounds like now we have. :)\nSo I feel better that we have a full understanding of the issue.\n\n-Peff\n"}]}