Am 13.08.2018 um 22:20 schrieb Junio C Hamano:
Johannes Sixt <j6t@xxxxxxxx> writes:
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 <Johannes.Schindelin@xxxxxx>
Helped-by: Jeff Hostetler <git@xxxxxxxxxxxxxxxxx>
Signed-off-by: Johannes Sixt <j6t@xxxxxxxx>
---
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).
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.)
...[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.)
-- Hannes
