Johannes Sixt [off-list ref] writes:
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.
* sequencer.c wants to use it when moving one insn from the todo
list to the 'done' list when it finishes one operation. If you
are running two sequences in a single repository, intermixed
'done' list would be the least of your problem, so presumably we
are fine 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.
On Fri, Aug 10, 2018 at 11:34:59AM -0700, Junio C Hamano wrote:
Johannes Sixt [off-list ref] writes:
quoted
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.
Just an interesting side note, but I think one of the reasons we use
dot-locks and not flock() is that the former generally works on network
filesystems. I think O_APPEND also is not reliable for non-local files,
though, so short of actually dot-locking trace files (yuck) I don't
think there's a great solution there.
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.
Heh, I actually wrote that patch several years ago, as part of an
abandoned effort to have config-triggered tracing (e.g., on the server
side where you're getting hundreds of requests and you want to enable
tracing on a repo for a slice of time).
One annoying thing for a case like the test in question is that you
don't actually know the pid of the process you're interested in. So we'd
generate two trace files, and then you'd have to figure out which one is
which. In my earlier work, I coupled it with some magic to allow
per-command config, like:
[trace "fetch"]
packet = /tmp/trace.%p
but you could also imagine a "%n" which uses the command name.
I don't remember why I abandoned it, but I think a lot had to do with
violating the "don't look at config" rules for our repo startup code. A
lot of that has been fixed since then, but I haven't really needed it.
If anybody is really interested, the patches are at:
https://github.com/peff/git jk/trace-stdin
(my ultimate goal was to record stdin for pack-objects to replay slow
fetches, but I ended up hacking together a patch to pack-objects
instead).
-Peff
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 [off-list ref]
Helped-by: Jeff Hostetler [off-list ref]
Signed-off-by: Johannes Sixt <redacted>
---
compat/mingw.c | 41 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 39 insertions(+), 2 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 6ded1c859f..858ca14a57 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -341,12 +341,44 @@ int mingw_mkdir(const char *path, int mode)
return ret;
}
+static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
+{
+ HANDLE handle;
+ int fd;
+ DWORD create = (oflags & O_CREAT) ? OPEN_ALWAYS : OPEN_EXISTING;
+
+ /* only these flags are supported */
+ if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND))
+ return errno = ENOSYS, -1;
+
+ /*
+ * FILE_SHARE_WRITE is required to permit child processes
+ * to append to the file.
+ */
+ handle = CreateFileW(wfilename, FILE_APPEND_DATA,
+ FILE_SHARE_WRITE | FILE_SHARE_READ,
+ NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
+ if (handle == INVALID_HANDLE_VALUE)
+ return errno = err_win_to_posix(GetLastError()), -1;
+ /*
+ * No O_APPEND here, because the CRT uses it only to reset the
+ * file pointer to EOF on write(); but that is not necessary
+ * for a file created with FILE_APPEND_DATA.
+ */
+ fd = _open_osfhandle((intptr_t)handle, O_BINARY);
+ if (fd < 0)
+ CloseHandle(handle);
+ return fd;
+}
+
int mingw_open (const char *filename, int oflags, ...)
{
+ typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...);
va_list args;
unsigned mode;
int fd;
wchar_t wfilename[MAX_PATH];
+ open_fn_t open_fn;
va_start(args, oflags);
mode = va_arg(args, int);@@ -355,9 +387,14 @@ int mingw_open (const char *filename, int oflags, ...)
if (filename && !strcmp(filename, "/dev/null"))
filename = "nul";
+ if (oflags & O_APPEND)
+ open_fn = mingw_open_append;
+ else
+ open_fn = _wopen;
+
if (xutftowcs_path(wfilename, filename) < 0)
return -1;
- fd = _wopen(wfilename, oflags, mode);
+ fd = open_fn(wfilename, oflags, mode);
if (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {
DWORD attrs = GetFileAttributesW(wfilename);@@ -375,7 +412,7 @@ int mingw_open (const char *filename, int oflags, ...)
* CREATE_ALWAYS flag of CreateFile()).
*/
if (fd < 0 && errno == EACCES)
- fd = _wopen(wfilename, oflags & ~O_CREAT, mode);
+ fd = open_fn(wfilename, oflags & ~O_CREAT, mode);
if (fd >= 0 && set_hidden_flag(wfilename, 1))
warning("could not mark '%s' as hidden.", filename);
}--
2.18.0.rc0.112.g5f42c6ebd3
On 8/13/2018 3:02 PM, Johannes Sixt wrote:
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 [off-list ref]
Helped-by: Jeff Hostetler [off-list ref]
Signed-off-by: Johannes Sixt <redacted>
---
compat/mingw.c | 41 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 39 insertions(+), 2 deletions(-)
[...]
This looks good. Thanks for following up on this.
Jeff