t5552: fix flakiness by introducing proper locking for GIT_TRACE

From: Johannes Schindelin <[email protected]>

This function will be used to make write accesses in trace_write() a bit safer.
...
To set a precedent for a better approach, let's introduce a proper
abstraction: a function that says in its name precisely what Git wants it to do (as opposed to *how* it does it on Linux):
lock_or_unlock_fd_for_appending().

The next commit will provide a Windows-specific implementation of this function/functionality.

Signed-off-by Johannes Schindelin <[email protected]>

squash! Introduce a function to lock/unlock file descriptors when appending

---
 git-compat-util.h |  2 ++
 wrapper.c         | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

git-compat-util.h +2 -0

index 9a64998b2..13b83bade 100644
@@ -1202,6 +1202,8 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
 #define getc_unlocked(fh) getc(fh)
 #endif
 
+extern int lock_or_unlock_fd_for_appending(int fd, int lock_it);
+
 /*
  * Our code often opens a path to an optional file, to work on its
  * contents when we can successfully open it.  We can ignore a failure

wrapper.c +0 -0

index e4fa9d84c..6c2116272 100644
@@ -690,3 +690,17 @@ int xgethostname(char *buf, size_t len)
 		buf[len - 1] = 0;
 	return ret;
 }

wrapper.c +3 -0

+	return fcntl(fd, F_SETLKW, &flock);
+}
+#endif

The culprit is that two processes try simultaneously to write to the same file specified via GIT_TRACE_PACKET, and it is not well defined how that should work, even on Linux.

This patch series addresses that by locking the trace fd. I chose to use flock() instead of fcntl() because the Win32 API LockFileEx()
[https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-lockfileex]
(which does exactly what I want in this context) has much more similar semantics to the former than the latter.

Are you sure that it's not well-defined? We open the path with O_APPEND, which means every write() will be atomically positioned at the end of file. So we would never lose or overwrite data.

We do our own buffering in a strbuf, writing the result out in a single write() call (modulo the OS returning a short write, but that should not generally happen when writing short strings to a file). So we should get individual trace lines as atomic units.

The order of lines from the two processes is undefined, of course.

t/t5552-skipping-fetch-negotiator.sh +18 -5

index 3b60bd44e0..5ad5bece55 100755
@@ -28,6 +28,19 @@ have_not_sent () {
 	done
 }
 
+# trace_fetch <client_dir> <server_dir> [args]
+#
+# Trace the packet output of fetch, but make sure we disable the variable
+# in the child upload-pack, so we don't combine the results in the same file.
+trace_fetch () {
+	client=$1; shift
+	server=$1; shift
+	GIT_TRACE_PACKET="$(pwd)/trace" \
+	git -C "$client" fetch \
+	  --upload-pack 'unset GIT_TRACE_PACKET; git-upload-pack' \
+	  "$server" "$@"
+}
+
 test_expect_success 'commits with no parents are sent regardless of skip distance' '
 	git init server &&
 	test_commit -C server to_fetch &&

@@ -42,7 +55,7 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc
 	# "c1" has no parent, it is still sent as "have" even though it would
 	# normally be skipped.
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	trace_fetch client "$(pwd)/server" &&
 	have_sent c7 c5 c2 c1 &&
 	have_not_sent c6 c4 c3
 '

@@ -88,7 +101,7 @@ test_expect_success 'when two skips collide, favor the larger one' '
 	# the next "have" sent will be "c1" (from "c6" skip 4) and not "c4"
 	# (from "c5side" skip 1).
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	trace_fetch client "$(pwd)/server" &&
 	have_sent c5side c11 c9 c6 c1 &&
 	have_not_sent c10 c8 c7 c5 c4 c3 c2
 '

@@ -114,7 +127,7 @@ test_expect_success 'use ref advertisement to filter out commits' '
 	# not need to send any ancestors of "c3", but we still need to send "c3"
 	# itself.
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch origin to_fetch &&
+	trace_fetch client origin to_fetch &&
 	have_sent c5 c4^ c2side &&
 	have_not_sent c4 c4^^ c4^^^
 '

@@ -144,7 +157,7 @@ test_expect_success 'handle clock skew' '
 	# and sent, because (due to clock skew) its only parent has already been
 	# popped off the priority queue.
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	trace_fetch client "$(pwd)/server" &&
 	have_sent c2 c1 old4 old2 old1 &&
 	have_not_sent old3
 '

@@ -176,7 +189,7 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
 	test_commit -C server commit-on-b1 &&
 
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" to_fetch &&
+	trace_fetch client "$(pwd)/server" to_fetch &&
 	grep "  fetch" trace &&
 
 	# fetch-pack sends 2 requests each containing 16 "have" lines before

Quoted 10 lines · Jeff King

Are you sure that it's not well-defined? We open the path with O_APPEND, which means every write() will be atomically positioned at the end of file. So we would never lose or overwrite data.

We do our own buffering in a strbuf, writing the result out in a single write() call (modulo the OS returning a short write, but that should not generally happen when writing short strings to a file). So we should get individual trace lines as atomic units.

The order of lines from the two processes is undefined, of course.

Correct. But I am more worried about the "mixed/overwriting" breakage, if there is one; it means we may need to be prepared for systems that lack O_APPEND that works correctly. I initially just assumed that it was what Dscho was seeing, but after re-reading his message, I am not sure anymore.

I think the "do not trace the other side" approach you suggest for these tests that only care about one side is more appropriate solution for this particular case. We then do not have to worry about overwriting or output from both sides mixed randomly.

Quoted 10 lines

Correct. But I am more worried about the "mixed/overwriting" breakage, if there is one; it means we may need to be prepared for systems that lack O_APPEND that works correctly. I initially just assumed that it was what Dscho was seeing, but after re-reading his message, I am not sure anymore.

I think the "do not trace the other side" approach you suggest for these tests that only care about one side is more appropriate solution for this particular case. We then do not have to worry about overwriting or output from both sides mixed randomly.

A concluding sentence I forgot to add, after saying "this is simpler and better to fix test breakage", was

But if we really are seeing O_APPEND breakage, a mandatory locking mechanism like this one might be necessary to work around it (I seriously hope we do not have to, though).

Sorry for an additional noise.

Quoted 10 lines · Jeff King

Are you sure that it's not well-defined? We open the path with O_APPEND, which means every write() will be atomically positioned at the end of file. So we would never lose or overwrite data.

We do our own buffering in a strbuf, writing the result out in a single write() call (modulo the OS returning a short write, but that should not generally happen when writing short strings to a file). So we should get individual trace lines as atomic units.

The order of lines from the two processes is undefined, of course.

Correct. But I am more worried about the "mixed/overwriting" breakage, if there is one; it means we may need to be prepared for systems that lack O_APPEND that works correctly. I initially just assumed that it was what Dscho was seeing, but after re-reading his message, I am not sure anymore.

I think the "do not trace the other side" approach you suggest for these tests that only care about one side is more appropriate solution for this particular case. We then do not have to worry about overwriting or output from both sides mixed randomly.

Here it is as a patch on top of jt/fetch-negotiator-skipping, which lets us pursue any fix for interleaved trace output on Windows without the pressure of an impending flaky test.

My gut says that looking into making O_APPEND work there is going to be the nicest solution, but my gut is not very well versed in Windows subtleties. ;)

-- >8 --
Subject: [PATCH] t5552: suppress upload-pack trace output

The t5552 test script uses GIT_TRACE_PACKET to monitor what git-fetch sends and receives. However, because we're accessing a local repository, the child upload-pack also sends trace output to the same file.

On Linux, this works out OK. We open the trace file with O_APPEND, so all writes are atomically positioned at the end of the file. No data can be overwritten or omitted. And since we prepare our small writes in a strbuf and write them with a single write(), we should see each line as an atomic unit. The order of lines between the two processes is undefined, but the test script greps only for "fetch>" or "fetch<" lines. So under Linux, the test results are deterministic.

The test fails intermittently on Windows, however, reportedly even overwriting bits of the output file (i.e., O_APPEND does not seem to give us an atomic position+write).

Since the test only cares about the trace output from fetch, we can just disable the output from upload-pack. That doesn't solve the greater question of O_APPEND/trace issues under Windows, but it easily fixes the flakiness from this test.

Reported-by Johannes Schindelin <[email protected]>

Signed-off-by Jeff King <[email protected]>

---
I'm assuming that this really isn't triggerable on Linux. I tried and couldn't manage to get it to fail, and the reasoning above explains why. But I wasn't 100% clear that Dscho hadn't seen it fail on non-Windows.

 t/t5552-skipping-fetch-negotiator.sh | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

t/t5552-skipping-fetch-negotiator.sh +18 -5

index 0a8e0e42ed..0ad50dd839 100755
@@ -28,6 +28,19 @@ have_not_sent () {
 	done
 }
 
+# trace_fetch <client_dir> <server_dir> [args]
+#
+# Trace the packet output of fetch, but make sure we disable the variable
+# in the child upload-pack, so we don't combine the results in the same file.
+trace_fetch () {
+	client=$1; shift
+	server=$1; shift
+	GIT_TRACE_PACKET="$(pwd)/trace" \
+	git -C "$client" fetch \
+	  --upload-pack 'unset GIT_TRACE_PACKET; git-upload-pack' \
+	  "$server" "$@"
+}
+
 test_expect_success 'commits with no parents are sent regardless of skip distance' '
 	git init server &&
 	test_commit -C server to_fetch &&

@@ -42,7 +55,7 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc
 	# "c1" has no parent, it is still sent as "have" even though it would
 	# normally be skipped.
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	trace_fetch client "$(pwd)/server" &&
 	have_sent c7 c5 c2 c1 &&
 	have_not_sent c6 c4 c3
 '

@@ -65,7 +78,7 @@ test_expect_success 'when two skips collide, favor the larger one' '
 	# the next "have" sent will be "c1" (from "c6" skip 4) and not "c4"
 	# (from "c5side" skip 1).
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	trace_fetch client "$(pwd)/server" &&
 	have_sent c5side c11 c9 c6 c1 &&
 	have_not_sent c10 c8 c7 c5 c4 c3 c2
 '

@@ -91,7 +104,7 @@ test_expect_success 'use ref advertisement to filter out commits' '
 	# not need to send any ancestors of "c3", but we still need to send "c3"
 	# itself.
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch origin to_fetch &&
+	trace_fetch client origin to_fetch &&
 	have_sent c5 c4^ c2side &&
 	have_not_sent c4 c4^^ c4^^^
 '

@@ -121,7 +134,7 @@ test_expect_success 'handle clock skew' '
 	# and sent, because (due to clock skew) its only parent has already been
 	# popped off the priority queue.
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	trace_fetch client "$(pwd)/server" &&
 	have_sent c2 c1 old4 old2 old1 &&
 	have_not_sent old3
 '

@@ -153,7 +166,7 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
 	test_commit -C server commit-on-b1 &&
 
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" to_fetch &&
+	trace_fetch client "$(pwd)/server" to_fetch &&
 	grep "  fetch" trace &&
 
 	# fetch-pack sends 2 requests each containing 16 "have" lines before

Here it is as a patch on top of jt/fetch-negotiator-skipping, which lets us pursue any fix for interleaved trace output on Windows without the pressure of an impending flaky test.

My gut says that looking into making O_APPEND work there is going to be the nicest solution, but my gut is not very well versed in Windows subtleties. ;)

-- >8 --
Subject: [PATCH] t5552: suppress upload-pack trace output

The t5552 test script uses GIT_TRACE_PACKET to monitor what git-fetch sends and receives. However, because we're accessing a local repository, the child upload-pack also sends trace output to the same file.

On Linux, this works out OK. We open the trace file with O_APPEND, so all writes are atomically positioned at the end of the file. No data can be overwritten or omitted. And since we prepare our small writes in a strbuf and write them with a single write(), we should see each line as an atomic unit. The order of lines between the two processes is undefined, but the test script greps only for "fetch>" or "fetch<" lines. So under Linux, the test results are deterministic.

The test fails intermittently on Windows, however, reportedly even overwriting bits of the output file (i.e., O_APPEND does not seem to give us an atomic position+write).

Since the test only cares about the trace output from fetch, we can just disable the output from upload-pack. That doesn't solve the greater question of O_APPEND/trace issues under Windows, but it easily fixes the flakiness from this test.

Reported-by Johannes Schindelin <[email protected]>

Signed-off-by Jeff King <[email protected]>

---
I'm assuming that this really isn't triggerable on Linux. I tried and couldn't manage to get it to fail, and the reasoning above explains why. But I wasn't 100% clear that Dscho hadn't seen it fail on non-Windows.

 t/t5552-skipping-fetch-negotiator.sh | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

t/t5552-skipping-fetch-negotiator.sh +18 -5

index 0a8e0e42ed..0ad50dd839 100755
@@ -28,6 +28,19 @@ have_not_sent () {
 	done
 }
 
+# trace_fetch <client_dir> <server_dir> [args]
+#
+# Trace the packet output of fetch, but make sure we disable the variable
+# in the child upload-pack, so we don't combine the results in the same file.
+trace_fetch () {
+	client=$1; shift
+	server=$1; shift
+	GIT_TRACE_PACKET="$(pwd)/trace" \
+	git -C "$client" fetch \
+	  --upload-pack 'unset GIT_TRACE_PACKET; git-upload-pack' \
+	  "$server" "$@"
+}
+
 test_expect_success 'commits with no parents are sent regardless of skip distance' '
 	git init server &&
 	test_commit -C server to_fetch &&

@@ -42,7 +55,7 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc
 	# "c1" has no parent, it is still sent as "have" even though it would
 	# normally be skipped.
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	trace_fetch client "$(pwd)/server" &&
 	have_sent c7 c5 c2 c1 &&
 	have_not_sent c6 c4 c3
 '

@@ -65,7 +78,7 @@ test_expect_success 'when two skips collide, favor the larger one' '
 	# the next "have" sent will be "c1" (from "c6" skip 4) and not "c4"
 	# (from "c5side" skip 1).
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	trace_fetch client "$(pwd)/server" &&
 	have_sent c5side c11 c9 c6 c1 &&
 	have_not_sent c10 c8 c7 c5 c4 c3 c2
 '

@@ -91,7 +104,7 @@ test_expect_success 'use ref advertisement to filter out commits' '
 	# not need to send any ancestors of "c3", but we still need to send "c3"
 	# itself.
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch origin to_fetch &&
+	trace_fetch client origin to_fetch &&
 	have_sent c5 c4^ c2side &&
 	have_not_sent c4 c4^^ c4^^^
 '

@@ -121,7 +134,7 @@ test_expect_success 'handle clock skew' '
 	# and sent, because (due to clock skew) its only parent has already been
 	# popped off the priority queue.
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	trace_fetch client "$(pwd)/server" &&
 	have_sent c2 c1 old4 old2 old1 &&
 	have_not_sent old3
 '

@@ -153,7 +166,7 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
 	test_commit -C server commit-on-b1 &&
 
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" to_fetch &&
+	trace_fetch client "$(pwd)/server" to_fetch &&
 	grep "  fetch" trace &&
 
 	# fetch-pack sends 2 requests each containing 16 "have" lines before

I reported a couple of times that t5552 is not passing reliably. It has now reached next, and will no doubt infect master soon.

Turns out that it is not a Windows-specific issue, even if it occurs a lot more often on Windows than elsewhere.

The culprit is that two processes try simultaneously to write to the same file specified via GIT_TRACE_PACKET, and it is not well defined how that should work, even on Linux.

Quoted 19 lines · Junio C Hamano

Quoted 10 lines · Jeff King

Are you sure that it's not well-defined? We open the path with O_APPEND, which means every write() will be atomically positioned at the end of file. So we would never lose or overwrite data.

We do our own buffering in a strbuf, writing the result out in a single write() call (modulo the OS returning a short write, but that should not generally happen when writing short strings to a file). So we should get individual trace lines as atomic units.

The order of lines from the two processes is undefined, of course.

Correct. But I am more worried about the "mixed/overwriting" breakage, if there is one; it means we may need to be prepared for systems that lack O_APPEND that works correctly. I initially just assumed that it was what Dscho was seeing, but after re-reading his message, I am not sure anymore.

Quoted 4 lines

I think the "do not trace the other side" approach you suggest for these tests that only care about one side is more appropriate solution for this particular case. We then do not have to worry about overwriting or output from both sides mixed randomly.

Here it is as a patch on top of jt/fetch-negotiator-skipping, which lets us pursue any fix for interleaved trace output on Windows without the pressure of an impending flaky test.

My gut says that looking into making O_APPEND work there is going to be the nicest solution, but my gut is not very well versed in Windows subtleties. ;)

-- >8 --
Subject: [PATCH] t5552: suppress upload-pack trace output

The t5552 test script uses GIT_TRACE_PACKET to monitor what git-fetch sends and receives. However, because we're accessing a local repository, the child upload-pack also sends trace output to the same file.

On Linux, this works out OK. We open the trace file with O_APPEND, so all writes are atomically positioned at the end of the file. No data can be overwritten or omitted. And since we prepare our small writes in a strbuf and write them with a single write(), we should see each line as an atomic unit. The order of lines between the two processes is undefined, but the test script greps only for "fetch>" or "fetch<" lines. So under Linux, the test results are deterministic.

The test fails intermittently on Windows, however, reportedly even overwriting bits of the output file (i.e., O_APPEND does not seem to give us an atomic position+write).

Since the test only cares about the trace output from fetch, we can just disable the output from upload-pack. That doesn't solve the greater question of O_APPEND/trace issues under Windows, but it easily fixes the flakiness from this test.

Reported-by Johannes Schindelin <[email protected]>

Signed-off-by Jeff King <[email protected]>

---
I'm assuming that this really isn't triggerable on Linux. I tried and couldn't manage to get it to fail, and the reasoning above explains why. But I wasn't 100% clear that Dscho hadn't seen it fail on non-Windows.

 t/t5552-skipping-fetch-negotiator.sh | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

t/t5552-skipping-fetch-negotiator.sh +18 -5

index 0a8e0e42ed..0ad50dd839 100755
@@ -28,6 +28,19 @@ have_not_sent () {
 	done
 }
 
+# trace_fetch <client_dir> <server_dir> [args]
+#
+# Trace the packet output of fetch, but make sure we disable the variable
+# in the child upload-pack, so we don't combine the results in the same file.
+trace_fetch () {
+	client=$1; shift
+	server=$1; shift
+	GIT_TRACE_PACKET="$(pwd)/trace" \
+	git -C "$client" fetch \
+	  --upload-pack 'unset GIT_TRACE_PACKET; git-upload-pack' \
+	  "$server" "$@"
+}
+
 test_expect_success 'commits with no parents are sent regardless of skip distance' '
 	git init server &&
 	test_commit -C server to_fetch &&

@@ -42,7 +55,7 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc
 	# "c1" has no parent, it is still sent as "have" even though it would
 	# normally be skipped.
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	trace_fetch client "$(pwd)/server" &&
 	have_sent c7 c5 c2 c1 &&
 	have_not_sent c6 c4 c3
 '

@@ -65,7 +78,7 @@ test_expect_success 'when two skips collide, favor the larger one' '
 	# the next "have" sent will be "c1" (from "c6" skip 4) and not "c4"
 	# (from "c5side" skip 1).
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	trace_fetch client "$(pwd)/server" &&
 	have_sent c5side c11 c9 c6 c1 &&
 	have_not_sent c10 c8 c7 c5 c4 c3 c2
 '

@@ -91,7 +104,7 @@ test_expect_success 'use ref advertisement to filter out commits' '
 	# not need to send any ancestors of "c3", but we still need to send "c3"
 	# itself.
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch origin to_fetch &&
+	trace_fetch client origin to_fetch &&
 	have_sent c5 c4^ c2side &&
 	have_not_sent c4 c4^^ c4^^^
 '

@@ -121,7 +134,7 @@ test_expect_success 'handle clock skew' '
 	# and sent, because (due to clock skew) its only parent has already been
 	# popped off the priority queue.
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	trace_fetch client "$(pwd)/server" &&
 	have_sent c2 c1 old4 old2 old1 &&
 	have_not_sent old3
 '

@@ -153,7 +166,7 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
 	test_commit -C server commit-on-b1 &&
 
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" to_fetch &&
+	trace_fetch client "$(pwd)/server" to_fetch &&
 	grep "  fetch" trace &&
 
 	# fetch-pack sends 2 requests each containing 16 "have" lines before

Signature · 1 line

2.18.0.1058.g0926f0b71f

Am 09.08.2018 um 19:35 schrieb Johannes Schindelin via GitGitGadget:

Quoted 9 lines

I reported a couple of times that t5552 is not passing reliably. It has now reached next, and will no doubt infect master soon.

Turns out that it is not a Windows-specific issue, even if it occurs a lot more often on Windows than elsewhere.

The culprit is that two processes try simultaneously to write to the same file specified via GIT_TRACE_PACKET, and it is not well defined how that should work, even on Linux.

Thanks for digging down to the root cause. As has been said, the behavior of O_APPEND is well-defined under POSIX, but last time I looked for equivalent feature on Windows, I did not find any.

Last time was when I worked around the same failure in t5503-tagfollow.sh in my private builds:

https://github.com/j6t/git/commit/9a447a6844b50b43746d9765b3ac809e2793d742

It is basically the same as Peff suggests: log only one side of the fetch.

As this buglet looks like a recurring theme, and a proper fix is preferable over repeated work-arounds. To me it looks like we need some sort of locking on Windows. Unless your friends at Microsoft have an ace in their sleeves that lets us have atomic O_APPEND the POSIX way...

Quoted 13 lines · Johannes Sixt

Am 09.08.2018 um 19:35 schrieb Johannes Schindelin via GitGitGadget:

Quoted 9 lines

I reported a couple of times that t5552 is not passing reliably. It has now reached next, and will no doubt infect master soon.

Turns out that it is not a Windows-specific issue, even if it occurs a lot more often on Windows than elsewhere.

The culprit is that two processes try simultaneously to write to the same file specified via GIT_TRACE_PACKET, and it is not well defined how that should work, even on Linux.

Thanks for digging down to the root cause. As has been said, the behavior of O_APPEND is well-defined under POSIX, but last time I looked for equivalent feature on Windows, I did not find any.

Quoted 8 lines

Last time was when I worked around the same failure in t5503-tagfollow.sh in my private builds:

https://github.com/j6t/git/commit/9a447a6844b50b43746d9765b3ac809e2793d742

It is basically the same as Peff suggests: log only one side of the fetch.

As this buglet looks like a recurring theme, and a proper fix is preferable over repeated work-arounds. To me it looks like we need some sort of locking on Windows. Unless your friends at Microsoft have an ace in their sleeves that lets us have atomic O_APPEND the POSIX way...

The official Windows CRT for open() does document an O_APPEND, but after looking at the distributed source, I'm not sure I trust it.

I have not looked at the MSYS version of the CRT yet so I cannot comment on it.

I did find that the following code does what we want.  Notice the use of FILE_APPEND_DATA in place of the usual GENERIC_READ.  (And yes, the docs are very vague here.)

Before we try a locking solution, perhaps we should tweak mingw_open() to use something like this to get a proper HANDLE directly and then use _open_osfhandle() to get a "fd" for it.

Jeff

---------------------------------------------------------------------

#include <Windows.h>

int main()
{
    HANDLE h = CreateFileW(L"foo.txt", FILE_APPEND_DATA,
        FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);

    const char * buf = "this is a test\n";
    DWORD len = strlen(buf);
    DWORD nbw;

    WriteFile(h, buf, len, &nbw, NULL);
    CloseHandle(h);

    return 0;
}

Quoted 18 lines

Am 09.08.2018 um 19:35 schrieb Johannes Schindelin via GitGitGadget:

Quoted 9 lines

I reported a couple of times that t5552 is not passing reliably. It has now reached next, and will no doubt infect master soon.

Turns out that it is not a Windows-specific issue, even if it occurs a lot more often on Windows than elsewhere.

The culprit is that two processes try simultaneously to write to the same file specified via GIT_TRACE_PACKET, and it is not well defined how that should work, even on Linux.

Thanks for digging down to the root cause. As has been said, the behavior of O_APPEND is well-defined under POSIX, but last time I looked for equivalent feature on Windows, I did not find any.

Last time was when I worked around the same failure in t5503-tagfollow.sh in my private builds:

https://github.com/j6t/git/commit/9a447a6844b50b43746d9765b3ac809e2793d742

Quoted 3 lines

It is basically the same as Peff suggests: log only one side of the fetch.

As this buglet looks like a recurring theme, and a proper fix is preferable over repeated work-arounds. To me it looks like we need some sort of locking on Windows. Unless your friends at Microsoft have an ace in their sleeves that lets us have atomic O_APPEND the POSIX way...

The official Windows CRT for open() does document an O_APPEND, but after looking at the distributed source, I'm not sure I trust it.

I have not looked at the MSYS version of the CRT yet so I cannot comment on it.

I did find that the following code does what we want.  Notice the use of FILE_APPEND_DATA in place of the usual GENERIC_READ.  (And yes, the docs are very vague here.)

Before we try a locking solution, perhaps we should tweak mingw_open() to use something like this to get a proper HANDLE directly and then use _open_osfhandle() to get a "fd" for it.

Jeff

---------------------------------------------------------------------

#include <Windows.h>

int main()
{
    HANDLE h = CreateFileW(L"foo.txt", FILE_APPEND_DATA,
        FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);

    const char * buf = "this is a test\n";
    DWORD len = strlen(buf);
    DWORD nbw;

    WriteFile(h, buf, len, &nbw, NULL);
    CloseHandle(h);

    return 0;
}

Quoted 6 lines

Quoted 6 lines

Correct. But I am more worried about the "mixed/overwriting" breakage, if there is one; it means we may need to be prepared for systems that lack O_APPEND that works correctly. I initially just assumed that it was what Dscho was seeing, but after re-reading his message, I am not sure anymore.

Quoted 4 lines

Quoted 4 lines

I think the "do not trace the other side" approach you suggest for these tests that only care about one side is more appropriate solution for this particular case. We then do not have to worry about overwriting or output from both sides mixed randomly.

Just so everybody knows: I never received the above two mails. Something is seriously rotten in GMX. This seems to have started a week or two ago.

What you cannot see easily, unless you go the route that I offered Jonathan (open a PR on gitgitgadget/git which will automatically run the test suite on Windows, macOS and Linux, and of course you can do anything in a PR, including narrowing down what tests are run) is that sometimes those lines in the trace file were clearly *incomplete*. That is, even if both processes tried to write atomically, one managed to overwrite the other's buffer in flight.

This is a pretty serious thing, even worse than the failing test suite, and I don't think that your patch acknowledges how serious this is.

And please don't give me that "but it works on Linux" response. Seriously. Sheesh.

Even if you have not managed to trigger it, the POSIX standard seems not to define clearly what the behavior is of two competing write() calls, unless you lock the files appropriately, as my patch series does.

So unless you are willing to ignore, to willfully keep this breakage, I would suggest not to introduce the ugliness of an overridden upload-pack for the sole purpose of disabling the tracing on one side, but instead to get this here bug fixed, by helping me with this here patch series.

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 10 lines · writes:

From: Johannes Schindelin <[email protected]>

This function will be used to make write accesses in trace_write() a bit safer.
...
To set a precedent for a better approach, let's introduce a proper
abstraction: a function that says in its name precisely what Git wants it to do (as opposed to *how* it does it on Linux):
lock_or_unlock_fd_for_appending().

Quoted 6 lines

The next commit will provide a Windows-specific implementation of this function/functionality.

Signed-off-by Johannes Schindelin <[email protected]>

squash! Introduce a function to lock/unlock file descriptors when appending

If we can keep the custom, narrow and easy-to-port API (like this patch introduces) focused and resist feature creep over time, then it would be worth spending effort to come up with such a custom helper that is easy to port. So I agree with the approach in general but I tend to think "set a precedent for a better approach" is a way-too-early and wishful verdict. We do not know if we can really keep that custom API easy-to-port-and-maintain yet.

In short, even though I agree with the approach, most of the verbiage above is unnecessary and mere distraction.

Quoted 31 lines

---
 git-compat-util.h |  2 ++
 wrapper.c         | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

git-compat-util.h +2 -0

index 9a64998b2..13b83bade 100644
@@ -1202,6 +1202,8 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
 #define getc_unlocked(fh) getc(fh)
 #endif
 
+extern int lock_or_unlock_fd_for_appending(int fd, int lock_it);
+
 /*
  * Our code often opens a path to an optional file, to work on its
  * contents when we can successfully open it.  We can ignore a failure

wrapper.c +5 -0

index e4fa9d84c..6c2116272 100644
@@ -690,3 +690,17 @@ int xgethostname(char *buf, size_t len)
 		buf[len - 1] = 0;
 	return ret;
 }
+
+#ifndef GIT_WINDOWS_NATIVE
+int lock_or_unlock_fd_for_appending(int fd, int lock_it)
+{
+	struct flock flock;

wrapper.c int xgethostname(char *buf, size_t len) · line 702

Quoted 5 lines

wrapper.c +5 -0

+
+	flock.l_type = lock_it ? F_WRLCK : F_UNLCK;
+	flock.l_whence = SEEK_SET;
+	flock.l_start = 0;
+	flock.l_len = 0xffffffff; /* arbitrary number of bytes */

If this can be an arbitrary range, do we need to cover this many (or only this few, depending on your point of view) bytes?

Would it be sufficient to cover just the first one byte instead?

Or perhaps give l_len==0 to cover all no matter how large the file grows, which sounds like a better range specification.

A lock shall be set to extend to the largest possible value of the file offset for that file by setting l_len to 0. If such a lock also has l_start set to 0 and l_whence is set to SEEK_SET, the whole file shall be locked.

wrapper.c int xgethostname(char *buf, size_t len) · line 706

Quoted 3 lines

wrapper.c +3 -0

+	return fcntl(fd, F_SETLKW, &flock);
+}
+#endif

As this buglet looks like a recurring theme, and a proper fix is preferable over repeated work-arounds. To me it looks like we need some sort of locking on Windows. Unless your friends at Microsoft have an ace in their sleeves that lets us have atomic O_APPEND the POSIX way...

I have tried to help with the actual problem, by identifying the root cause (that the trace code's strategy is not fundamentally broken, but that it relies on O_APPEND whose guarantees are obviously not portable to Windows) and exploring possible solutions there (FILE_APPEND_DATA).

Lacking any kind of Windows development environment, I'd just as soon stop there and let more knowledgeable people take over. But please don't characterize my presence in this thread as some kind of willful harm or ignoring of the problem.

Quoted 5 lines · Johannes Sixt

As this buglet looks like a recurring theme, and a proper fix is preferable over repeated work-arounds. To me it looks like we need some sort of locking on Windows. Unless your friends at Microsoft have an ace in their sleeves that lets us have atomic O_APPEND the POSIX way...

Just to put the severity of the issue in context, we use O_APPEND in a few codepaths, and the trace thing for debugging is the only thing that could have multiple writers. Other users of O_APPEND are:

* refs/files-backend.c uses it so that a reflog entry can be appended at the end, but because update to each ref is protected from racing at a lot higher layer with a lock, no two people would try to append to the same reflog file, so atomicity of O_APPEND does not matter here.

It may make sense to allow GIT_TRACE to have a placeholder (e.g. "/tmp/traceout.$$") to help debuggers arrange to give different processes their own trace output file, which perhaps may be a simpler and less impactful to the performance solution than having to make locks at an application layer.

Quoted 5 lines · Johannes Schindelin

So unless you are willing to ignore, to willfully keep this breakage, I would suggest not to introduce the ugliness of an overridden upload-pack for the sole purpose of disabling the tracing on one side, but instead to get this here bug fixed, by helping me with this here patch series.

I'm OK if you want to live with the broken test in the interim.

wrapper.c +6 -0

+
+#ifndef GIT_WINDOWS_NATIVE
+int lock_or_unlock_fd_for_appending(int fd, int lock_it)
+{
+	struct flock flock;
+

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 11 lines · writes:

wrapper.c +11 -0

+
+#ifndef GIT_WINDOWS_NATIVE
+int lock_or_unlock_fd_for_appending(int fd, int lock_it)
+{
+	struct flock flock;
+
+	flock.l_type = lock_it ? F_WRLCK : F_UNLCK;
+
+	/* (un-)lock the whole file */
+	flock.l_whence = SEEK_SET;
+	flock.l_start = 0;

wrapper.c int xgethostname(char *buf, size_t len) · line 708

Quoted 5 lines

wrapper.c +5 -0

+	flock.l_len = 0;
+
+	return fcntl(fd, F_SETLKW, &flock);
+}
+#endif

I think people already told you that this is not needed on systems with properly working O_APPEND [*1*]

Side note #1: and with network filesystems where O_APPEND may not work reliably, fcntl based range locking would not work either, so having this would not help.

I saw other Johannes and other Jeff peeking into fixing O_APPEND; I do not know how well that effort goes, but it would be preferrable if we can successfully go that route.

As I said in my review of the first patch in v1 series, I am not fundamentally opposed to a few "lock here to work around lack of O_APPEND" and "unlock here for the same reason" calls to limited codepaths as a workaround, as the damage is limited (that is why I earlier looked at our use of O_APPEND), but that would be the last resort if O_APPEND cannot be made to work reliably on Windows.

But even if we end up doing so, on systems with POSIX O_APPEND working, I think that function should be

    #define lock_or_unlock_for_appending(fd, lock) 0 /* nothing to do */

The Windows CRT implements O_APPEND "manually": on write() calls, the file pointer is set to EOF before the data is written. Clearly, this is not atomic. And in fact, this is the root cause of failures observed in t5552-skipping-fetch-negotiator.sh and t5503-tagfollow.sh, where different processes write to the same trace file simultanously; it also occurred in t5400-send-pack.sh, but there it was worked around in 71406ed4d6 ("t5400: avoid concurrent writes into a trace file", 2017-05-18).

Fortunately, Windows does support atomic O_APPEND semantics using the file access mode FILE_APPEND_DATA. Provide an implementation that does.

This implementation is minimal in such a way that it only implements the open modes that are actually used in the Git code base. Emulation for other modes can be added as necessary later. To become aware of the necessity early, the unusal error ENOSYS is reported if an unsupported mode is encountered.

Diagnosed-by Johannes Schindelin <[email protected]>

Helped-by Jeff Hostetler <[email protected]>

Signed-off-by Johannes Sixt <[email protected]>

---
 compat/mingw.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

Quoted 18 lines · Johannes Sixt

The Windows CRT implements O_APPEND "manually": on write() calls, the file pointer is set to EOF before the data is written. Clearly, this is not atomic. And in fact, this is the root cause of failures observed in t5552-skipping-fetch-negotiator.sh and t5503-tagfollow.sh, where different processes write to the same trace file simultanously; it also occurred in t5400-send-pack.sh, but there it was worked around in 71406ed4d6 ("t5400: avoid concurrent writes into a trace file", 2017-05-18).

Fortunately, Windows does support atomic O_APPEND semantics using the file access mode FILE_APPEND_DATA. Provide an implementation that does.

This implementation is minimal in such a way that it only implements the open modes that are actually used in the Git code base. Emulation for other modes can be added as necessary later. To become aware of the necessity early, the unusal error ENOSYS is reported if an unsupported mode is encountered.

Quoted 6 lines

Diagnosed-by Johannes Schindelin <[email protected]>

Helped-by Jeff Hostetler <[email protected]>

Signed-off-by Johannes Sixt <[email protected]>

---
  compat/mingw.c | 41 +++++++++++++++++++++++++++++++++++++++--
  1 file changed, 39 insertions(+), 2 deletions(-)

Nice.

I wonder how much more expensive using this implementation is compared with the original "race susceptible" open(), when raciness is known not to be an issue (e.g. there is higher level lock that protects the appending).

...[define race_safe_append_open]... and replace
the call to open(... O_APPEND ...) in trace.c::get_trace_fd() with a call to that wrapper. That way, other codepaths that use O_APPEND (namely, reflog and todo-list writers) can avoid the additional cost, if any.

Some may find it beneficial from code readability POV because that approach marks the codepath that needs to have non-racy fd more explicitly.

Am 13.08.2018 um 22:20 schrieb Junio C Hamano:

Quoted 27 lines

Quoted 18 lines · Johannes Sixt

The Windows CRT implements O_APPEND "manually": on write() calls, the file pointer is set to EOF before the data is written. Clearly, this is not atomic. And in fact, this is the root cause of failures observed in t5552-skipping-fetch-negotiator.sh and t5503-tagfollow.sh, where different processes write to the same trace file simultanously; it also occurred in t5400-send-pack.sh, but there it was worked around in 71406ed4d6 ("t5400: avoid concurrent writes into a trace file", 2017-05-18).

Fortunately, Windows does support atomic O_APPEND semantics using the file access mode FILE_APPEND_DATA. Provide an implementation that does.

This implementation is minimal in such a way that it only implements the open modes that are actually used in the Git code base. Emulation for other modes can be added as necessary later. To become aware of the necessity early, the unusal error ENOSYS is reported if an unsupported mode is encountered.

Quoted 6 lines

Diagnosed-by Johannes Schindelin <[email protected]>

Helped-by Jeff Hostetler <[email protected]>

Signed-off-by Johannes Sixt <[email protected]>

---
  compat/mingw.c | 41 +++++++++++++++++++++++++++++++++++++++--
  1 file changed, 39 insertions(+), 2 deletions(-)

Quoted 6 lines

Nice.

I wonder how much more expensive using this implementation is compared with the original "race susceptible" open(), when raciness is known not to be an issue (e.g. there is higher level lock that protects the appending).

Certainly, the former way that uses two syscalls (SetFilePointer+WriteFile) is more costly than this new way with just one syscall (WriteFile). Of course, I don't know how atomic append would be implemented in the kernel, but I can't think of a reason why it should be slow on Windows, but fast on POSIX.

(But I can't provide numbers to back up my gut feeling...)

(And I also assume that you are not worried about the performance of open() itself.)

Quoted 9 lines

...[define race_safe_append_open]... and replace
the call to open(... O_APPEND ...) in trace.c::get_trace_fd() with a call to that wrapper. That way, other codepaths that use O_APPEND (namely, reflog and todo-list writers) can avoid the additional cost, if any.

Some may find it beneficial from code readability POV because that approach marks the codepath that needs to have non-racy fd more explicitly.

O_APPEND is POSIX and means race-free append. If you mark some call sites with O_APPEND, then that must be the ones that need race-free append. Hence, you would have to go the other route: Mark those call sites that do _not_ need race-free append with some custom function/macro. (Or mark both with different helpers and avoid writing down O_APPEND.)

O_APPEND in POSIX is race-free only up to PIPE_MAX bytes written at a time, which is e.g. 2^12 by default on linux, after that all bets are off and the kernel is free to interleave different write calls.

So there's still a use for a race_safe_append_open() wrapper function, to O_APPEND and do the PIPE_MAX assertion. Otherwise you're calling a "safe" function which isn't safe at all anymore.

Quoted 6 lines

O_APPEND is POSIX and means race-free append. If you mark some call sites with O_APPEND, then that must be the ones that need race-free append. Hence, you would have to go the other route: Mark those call sites that do _not_ need race-free append with some custom function/macro. (Or mark both with different helpers and avoid writing down O_APPEND.)

O_APPEND in POSIX is race-free only up to PIPE_MAX bytes written at a time, which is e.g. 2^12 by default on linux, after that all bets are off and the kernel is free to interleave different write calls.

out ;# clean up from last run

The Windows CRT implements O_APPEND "manually": on write() calls, the file pointer is set to EOF before the data is written. Clearly, this is not atomic. And in fact, this is the root cause of failures observed in t5552-skipping-fetch-negotiator.sh and t5503-tagfollow.sh, where different processes write to the same trace file simultanously; it also occurred in t5400-send-pack.sh, but there it was worked around in 71406ed4d6 ("t5400: avoid concurrent writes into a trace file", 2017-05-18).

Fortunately, Windows does support atomic O_APPEND semantics using the file access mode FILE_APPEND_DATA. Provide an implementation that does.

This implementation is minimal in such a way that it only implements the open modes that are actually used in the Git code base. Emulation for other modes can be added as necessary later. To become aware of the necessity early, the unusal error ENOSYS is reported if an unsupported mode is encountered.

Diagnosed-by Johannes Schindelin <[email protected]>

Helped-by Jeff Hostetler <[email protected]>

Signed-off-by Johannes Sixt <[email protected]>

---
  compat/mingw.c | 41 +++++++++++++++++++++++++++++++++++++++--
  1 file changed, 39 insertions(+), 2 deletions(-)

Quoted 7 lines · Ævar Arnfjörð Bjarmason

Quoted 6 lines

O_APPEND is POSIX and means race-free append. If you mark some call sites with O_APPEND, then that must be the ones that need race-free append. Hence, you would have to go the other route: Mark those call sites that do _not_ need race-free append with some custom function/macro. (Or mark both with different helpers and avoid writing down O_APPEND.)

Quoted 3 lines

O_APPEND in POSIX is race-free only up to PIPE_MAX bytes written at a time, which is e.g. 2^12 by default on linux, after that all bets are off and the kernel is free to interleave different write calls.

This is a claim I've run across often, but I've never seen a good citation for it.

Certainly atomic writes to _pipes_ are determined by PIPE_BUF (which IIRC is not even a constant on Linux, but can be changed at run-time). But is it relevant for regular-file writes?

Another gem I found while digging on this O_APPEND/FILE_APPEND_DATA stuff the other day: somebody claimed that the max atomic-append size on Linux is 4096 and 1024 on Windows. But their experimental script was done in bash! So I suspect they were really just measuring the size of stdio buffers.

Here's my attempt at a test setup. This C program forces two processes to write simultaneously to the same file with O_APPEND:

-- >8 --
#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#include <sys/types.h>
#include <fcntl.h>
#include <unistd.h>

static void doit(int size, const char *fn, char c)

{
	int fd;
	char *buf;

	fd = open(fn, O_WRONLY|O_APPEND|O_CREAT, 0666);
	if (fd < 0) {
		perror("open");
		return;
	}

	buf = malloc(size);
	memset(buf, c, size);

	while (1)
		write(fd, buf, size);
}

int main(int argc, const char **argv)

{
	int size = atoi(argv[1]);

	if (fork())
		doit(size, argv[2], '1');
	else
		doit(size, argv[2], '2');
	return 0;
}

-- 8< --

and then this program checks that we saw atomic units of the correct size:

-- >8 --
#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>

int main(int argc, const char **argv)

{
	int size = atoi(argv[1]);
	char *buf;

	buf = malloc(size);
	while (1) {
		int i;
		/* assume atomic reads, i.e., no signals */
		int r = read(0, buf, size);
		if (!r)
			break;
		for (i = 1; i < size; i++) {
			if (buf[i] != buf[0]) {
				fprintf(stderr, "overlap\n");
				return 1;
			}
		}
	}
	return 0;
}

-- 8< --

And then you can do something like:

for size in 4097 8193 16385 32769 65537 131073 262145 524289 1048577; do

Quoted 1 line

out ;# clean up from last run

echo "Trying $size..."
timeout 5 ./write $size out
if ! ./check $size <out; then
echo "$size failed"
break
fi
done

On my Linux system, each of those seems to write several gigabytes without overlapping. I did manage to hit some failing cases, but they were never sheared writes, but rather cases where there was an incomplete write at the end-of-file.

So obviously this is all a bit of a tangent. I'd be fine declaring that trace output is generally small enough not to worry about this in the first place. But those results show that it shouldn't matter even if we're writing 1MB trace lines on Linux. I wouldn't be at all surprised to see different results on other operating systems, though.

Quoted 5 lines · Johannes Sixt

Fortunately, Windows does support atomic O_APPEND semantics using the file access mode FILE_APPEND_DATA. Provide an implementation that does.
...

Quoted 4 lines

Diagnosed-by Johannes Schindelin <[email protected]>

Helped-by Jeff Hostetler <[email protected]>

Signed-off-by Johannes Sixt <[email protected]>

---

This looks good. Thanks for following up on this.

The relevant POSIX docs are here:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html

Write requests of {PIPE_BUF} bytes or less shall not be interleaved with data from other processes doing writes on the same pipe. Writes of greater than {PIPE_BUF} bytes may have data interleaved, on arbitrary boundaries, with writes by other processes, whether or not the O_NONBLOCK flag of the file status flags is set.

Quoted 3 lines

Certainly atomic writes to _pipes_ are determined by PIPE_BUF (which IIRC is not even a constant on Linux, but can be changed at run-time). But is it relevant for regular-file writes?

I believe it's hardcoded at PIPE_BUF which is defined as PAGE_SIZE on linux. I think you may be thinking of /proc/sys/fs/pipe-max-pages which is the number of pages that a pipe will take before filling up, but I may be wrong.

Quoted 5 lines

So obviously this is all a bit of a tangent. I'd be fine declaring that trace output is generally small enough not to worry about this in the first place. But those results show that it shouldn't matter even if we're writing 1MB trace lines on Linux. I wouldn't be at all surprised to see different results on other operating systems, though.

I don't know how this works internally in the kernel, but I'd be very careful to make that assertion. Most likely this in practice depends on what FS you're using, its mount options, whether fsync() is involved, I/O pressure etc.

FWIW this is something I've ran into in the past on Linux as a practical matter, but that was many kernel versions ago, so maybe the semantics changed.

We had an ad-hoc file format with each chunk a "<start marker><length><content><end marker>\n" format (and the <content> was guaranteed not to contain "\n"). These would be written to the same file by N workers. We would get corrupt data because of this in cases where we were writing more than PIPE_BUF, e.g. start markers for unrelated payloads interleaved with content, lines that were incorrectly formed etc.

But yeah, whether this is a practical concern for git trace output is another matter. I just wanted to chime in to note that atomic appends to files are only portable on POSIX up to PIPE_BUF.

And then you can do something like:

for size in 4097 8193 16385 32769 65537 131073 262145 524289 1048577; do
>out ;# clean up from last run
echo "Trying $size..."
timeout 5 ./write $size out
if ! ./check $size <out; then
echo "$size failed"
break
fi
done

On my Linux system, each of those seems to write several gigabytes without overlapping. I did manage to hit some failing cases, but they were never sheared writes, but rather cases where there was an incomplete write at the end-of-file.

Am 14.08.2018 um 00:37 schrieb Jeff King:

Quoted 12 lines

And then you can do something like:

for size in 4097 8193 16385 32769 65537 131073 262145 524289 1048577; do
>out ;# clean up from last run
echo "Trying $size..."
timeout 5 ./write $size out
if ! ./check $size <out; then
echo "$size failed"
break
fi
done

Quoted 4 lines

On my Linux system, each of those seems to write several gigabytes without overlapping. I did manage to hit some failing cases, but they were never sheared writes, but rather cases where there was an incomplete write at the end-of-file.

I used your programs with necessary adjustments (as fork() is not available), and did similar tests with concurrent processes. With packet sizes 1025, 4093, 7531 (just to include some odd number), and 8193 I did not observe any overlapping or short writes.

I'm now very confident that we are on the safe side for our purposes.