Re: [PATCH] mingw: enable atomic O_APPEND
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 13, 2018, 20:20 UTC
- Message-ID
- <xmqqr2j23tnb.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <bc9132ab-3030-07e3-03d1-d2ddae6a1b45@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
Show 24 quoted lines
> 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@gmx.de>
> Helped-by: Jeff Hostetler <git@jeffhostetler.com>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> 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).
If it turns out to be quite more costly, I do not think we'd mind introducing a thin wrapper
#ifndef race_safe_append_open #ifndef race_safe_append_open(fn) open(fn, O_WRONLY|O_APPEND|O_CREAT, 0666) #endif
in git-compat-util.h after it includes "compat/mingw.h" 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.
I am assuming that in this case other users of O_APPEND are not performance critical, so hopefully the following is only theoretical and not necessary.
git-compat-util.h | 5 +++++ trace.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/git-compat-util.h b/git-compat-util.h index 9a64998b24..a981c50800 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -218,6 +218,11 @@ #else #include <stdint.h> #endif + +#ifndef race_safe_append_open +#define race_safe_append_open(fn) open(fn, O_WRONLY|O_APPEND|O_CREAT, 0666) +#endif + #ifdef NO_INTPTR_T /* * On I16LP32, ILP32 and LP64 "long" is the save bet, however diff --git a/trace.c b/trace.c index fc623e91fd..bef9cee1da 100644 --- a/trace.c +++ b/trace.c @@ -47,7 +47,7 @@ static int get_trace_fd(struct trace_key *key) else if (strlen(trace) == 1 && isdigit(*trace)) key->fd = atoi(trace); else if (is_absolute_path(trace)) { - int fd = open(trace, O_WRONLY | O_APPEND | O_CREAT, 0666); + int fd = race_safe_append_open(trace); if (fd == -1) { warning("could not open '%s' for tracing: %s", trace, strerror(errno));