| Message ID | YDXZY8XFRayiM1If@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | wrapper: add workaround for open() returning EINTR | expand |
On Tue, Feb 23, 2021 at 11:43:16PM -0500, Jeff King wrote: > The workaround here is enabled all the time, without a Makefile knob, > since it's a complete noop if open() never returns EINTR. I did push it > into its own compat/ source file, though, since it has to #undef our > macro redirection. Putting it in a file with other code risks confusion > if more code is added after that #undef. Whoops, sorry, I had a last-minute change of heart here and just stuck it in wrapper.c with a bit of preprocessor magic to guard it. It felt weird having compat/open.c, when the rest of compat/ is always conditional on Makefile knobs. But I'm happy to go the other way if anybody feels strongly. -Peff
On Tue, Feb 23, 2021 at 11:43:15PM -0500, Jeff King wrote: > The workaround here is enabled all the time, without a Makefile knob, > since it's a complete noop if open() never returns EINTR. I did push it > into its own compat/ source file, though, since it has to #undef our > macro redirection. Putting it in a file with other code risks confusion > if more code is added after that #undef. Hmm. The patch below defines it in wrapper.c. Intentional? > I do still think the OS is doing the wrong thing here. But even if I'm > right, it's probably prudent to work around it. Regardless of the above, I agree that if your explanation is true (and I have no reason to believe that it isn't) that the OS is indeed doing the wrong thing here. That patch below looks quite reasonable, thanks. Thanks, Taylor
Jeff King <peff@peff.net> writes: > On Tue, Feb 23, 2021 at 11:43:16PM -0500, Jeff King wrote: > >> The workaround here is enabled all the time, without a Makefile knob, >> since it's a complete noop if open() never returns EINTR. I did push it >> into its own compat/ source file, though, since it has to #undef our >> macro redirection. Putting it in a file with other code risks confusion >> if more code is added after that #undef. > > Whoops, sorry, I had a last-minute change of heart here and just stuck > it in wrapper.c with a bit of preprocessor magic to guard it. It felt > weird having compat/open.c, when the rest of compat/ is always > conditional on Makefile knobs. But I'm happy to go the other way if > anybody feels strongly. Hmph, just like other workarounds, shouldn't this be "if your platform screws this up, define this knob to work it around"? That would make it fit better with the rest of compat/. I dunno. A no-op wrapper makes the code less transparent, which I am somewhat unhappy. But a wrapper that is always used even on platforms that do not need might give us a better chance of noticing a bug in the wrapper itself, making it less likely for us to leave only the users of minority broken platforms behind. So...
Am 24.02.21 um 05:43 schrieb Jeff King: > The workaround here is enabled all the time, without a Makefile knob, > since it's a complete noop if open() never returns EINTR. I did push it > into its own compat/ source file, though, since it has to #undef our > macro redirection. Putting it in a file with other code risks confusion > if more code is added after that #undef. I'm not so much opposed to "enable it all the time" in general, but when we already have an override of open(), like for the Windows case in compat/mingw.h, I find it a bit rough to put another wrapper around it, even more so since we won't have the EINTR problem on Windows due to the absence of signals. -- Hannes
On Tue, Feb 23, 2021 at 09:36:25PM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On Tue, Feb 23, 2021 at 11:43:16PM -0500, Jeff King wrote: > > > >> The workaround here is enabled all the time, without a Makefile knob, > >> since it's a complete noop if open() never returns EINTR. I did push it > >> into its own compat/ source file, though, since it has to #undef our > >> macro redirection. Putting it in a file with other code risks confusion > >> if more code is added after that #undef. > > > > Whoops, sorry, I had a last-minute change of heart here and just stuck > > it in wrapper.c with a bit of preprocessor magic to guard it. It felt > > weird having compat/open.c, when the rest of compat/ is always > > conditional on Makefile knobs. But I'm happy to go the other way if > > anybody feels strongly. > > Hmph, just like other workarounds, shouldn't this be "if your > platform screws this up, define this knob to work it around"? That > would make it fit better with the rest of compat/. I actually started that way, but then I got stuck deciding which platforms in config.mak.uname to enable it for. We've seen it on Linux and on macOS. I don't know how prevalent it is on those platforms (or whether it is indeed a bug that may even get fixed). Since the check is basically zero-cost at run-time, it seemed like it was worth just being on guard for it so that nobody ever had to worry about it again. But I don't feel that strongly. I'd be happy to do it with a Makefile knob if you prefer. > I dunno. A no-op wrapper makes the code less transparent, which I > am somewhat unhappy. Yeah. I think the run-time cost is essentially zero, but it does add a bit of complexity if you are debugging. And handling the magic mode vararg definitely makes it uglier. The other thing that gives me pause is that there may be other syscalls which need the same treatment. Here's a recent discussion that Big Sur may also return EINTR from the opendir() family: https://github.com/dotnet/runtime/issues/47584 Git is presumably susceptible to that, too; it's just that we call open() a lot more frequently, so that was the obvious one that came up. I don't think there is a more general way to address this, though. We'd have to wrap each suspected syscall (but we could presumably at least tie them all to a single Makefile knob). I'm not excited at the prospect of preemptively adding such wrappers when we aren't even 100% sure that there is a problem. > But a wrapper that is always used even on > platforms that do not need might give us a better chance of noticing > a bug in the wrapper itself, making it less likely for us to leave > only the users of minority broken platforms behind. So... That part I'm less worried about. It sounds like we'd want it on for recent versions of macOS, which would give us pretty wide coverage to notice a bug. -Peff
On Wed, Feb 24, 2021 at 08:20:57AM +0100, Johannes Sixt wrote: > Am 24.02.21 um 05:43 schrieb Jeff King: > > The workaround here is enabled all the time, without a Makefile knob, > > since it's a complete noop if open() never returns EINTR. I did push it > > into its own compat/ source file, though, since it has to #undef our > > macro redirection. Putting it in a file with other code risks confusion > > if more code is added after that #undef. > > I'm not so much opposed to "enable it all the time" in general, but when > we already have an override of open(), like for the Windows case in > compat/mingw.h, I find it a bit rough to put another wrapper around it, > even more so since we won't have the EINTR problem on Windows due to the > absence of signals. That's fair. I don't think my new wrapper would interact well with mingw_open(). They are both trying to #define open. I think since mine comes later in git-compat-util.h, it is probably overriding mingw_open() completely on Windows, which is quite bad (we call into my function in wrapper.c, but then its "#undef open" means we get the original open(), not the previously defined wrapper). -Peff
On Wed, Feb 24, 2021 at 01:23:51PM -0500, Jeff King wrote: > > I'm not so much opposed to "enable it all the time" in general, but when > > we already have an override of open(), like for the Windows case in > > compat/mingw.h, I find it a bit rough to put another wrapper around it, > > even more so since we won't have the EINTR problem on Windows due to the > > absence of signals. > > That's fair. I don't think my new wrapper would interact well with > mingw_open(). They are both trying to #define open. I think since mine > comes later in git-compat-util.h, it is probably overriding mingw_open() > completely on Windows, which is quite bad (we call into my function in > wrapper.c, but then its "#undef open" means we get the original open(), > not the previously defined wrapper). Thanks again for pointing out mingw_open(). The v2 I posted should avoid any bad interactions there, even if we do end up enabling it all the time. By the way, I did notice something funny when looking at mingw_open(). It unconditionally does: va_start(args, oflags); mode = va_arg(args, int); va_end(args); no matter what we see in oflags. But callers who are not passing O_CREAT will not provide a third argument at all. So probably it is loading random trash from the stack and passing it around in the "mode" variable. This doesn't hurt anything, because ultimately we pass the trash along to the real open function, which will ignore it without O_CREAT. But it is definitely undefined behavior to call va_arg() at all in this instance. -Peff
diff --git a/git-compat-util.h b/git-compat-util.h index 838246289c..2be022c422 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -788,6 +788,10 @@ int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap); #endif +#undef open +#define open git_open_with_retry +int git_open_with_retry(const char *path, int flag, ...); + #ifdef __GLIBC_PREREQ #if __GLIBC_PREREQ(2, 1) #define HAVE_STRCHRNUL diff --git a/wrapper.c b/wrapper.c index bcda41e374..130f441064 100644 --- a/wrapper.c +++ b/wrapper.c @@ -678,3 +678,32 @@ int is_empty_or_missing_file(const char *filename) return !st.st_size; } + +/* + * Do not add other callers of open() after this. Our redefinition + * below will ensure the compiler catches any. + */ +#undef open +int git_open_with_retry(const char *path, int flags, ...) +{ + mode_t mode = 0; + int ret; + + /* + * Also O_TMPFILE would take a mode, but it isn't defined everywhere. + * And anyway, we don't use it in our code base. + */ + if (flags & O_CREAT) { + va_list ap; + va_start(ap, flags); + mode = va_arg(ap, int); + va_end(ap); + } + + do { + ret = open(path, flags, mode); + } while (ret < 0 && errno == EINTR); + + return ret; +} +#define open do_not_allow_use_of_bare_open
On some platforms, open() reportedly returns EINTR when opening regular files and we receive a signal (usually SIGALRM from our progress meter). This shouldn't happen, as open() should be a restartable syscall, and we specify SA_RESTART when setting up the alarm handler. So it may actually be a kernel or libc bug for this to happen. But it has been reported on at least one version of Linux (on a network filesystem): https://lore.kernel.org/git/c8061cce-71e4-17bd-a56a-a5fed93804da@neanderfunk.de/ as well as on macOS starting with Big Sur even on a regular filesystem. We can work around it by retrying open() calls that get EINTR, just as we do for read(), etc. Since we don't ever _want_ to interrupt an open() call, we can get away with just redefining open, rather than insisting all callsites use xopen(). We actually do have an xopen() wrapper already (and it even does this retry, though there's no indication of it being an observed problem back then; it seems simply to have been lifted from xread(), etc). But it is used hardly anywhere, and isn't suitable for general use because it will die() on error. In theory we could combine the two, but it's awkward to do so because of the variable-args interface of open(). The workaround here is enabled all the time, without a Makefile knob, since it's a complete noop if open() never returns EINTR. I did push it into its own compat/ source file, though, since it has to #undef our macro redirection. Putting it in a file with other code risks confusion if more code is added after that #undef. Reported-by: Aleksey Kliger <alklig@microsoft.com> Signed-off-by: Jeff King <peff@peff.net> --- This was reported to me off-list. Aleksey was kind enough to test the patch on a system that was reliably showing the problem during the checkout phase of git-clone (which naturally has both a progress meter and is calling open() on a ton of files). I do still think the OS is doing the wrong thing here. But even if I'm right, it's probably prudent to work around it. git-compat-util.h | 4 ++++ wrapper.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+)