v2, 2 partsUse CLOEXEC to avoid fd leaks

From: Lars Schneider <larsxschneider@gmail.com>

Use the CLOEXEC flag similar to 05d1ed61 to fix leaked file descriptors.

This mini patch series is necessary to make the "ls/filter-process" topic work properly for Windows. Right now the topic generates test failures on Windows as reported by Dscho: http://public-inbox.org/git/alpine.DEB.2.20.1610211457030.3264@virtualbox/

Patch 1/2 was contemplated by Junio here: http://public-inbox.org/git/xmqq37lnbbpk.fsf@gitster.mtv.corp.google.com/

Thanks to
    Eric, Jakub, Dscho, and Junio for the review of v1,
Lars

## Changes since v1
 * add fallbacks in case O_CLOEXEC is not available
 * rename 'git_open_noatime_cloexec' to 'git_open' (Eric, Dscho)
 * rebased the topic on `next` to fix a merge conflict

Lars Schneider (2):
  sha1_file: open window into packfiles with CLOEXEC
  read-cache: make sure file handles are not inherited by child
    processes

 builtin/pack-objects.c |  2 +-
 cache.h                |  2 +-
 pack-bitmap.c          |  2 +-
 read-cache.c           |  6 +++++-
 sha1_file.c            | 26 ++++++++++++++++----------
 5 files changed, 24 insertions(+), 14 deletions(-)

## Interdiff (v1..v2)

From: Lars Schneider <larsxschneider@gmail.com>

All processes that the Git main process spawns inherit the open file descriptors of the main process. These leaked file descriptors can cause problems.

Use the CLOEXEC flag similar to 05d1ed61 to fix the leaked file descriptors. Since `git_open_noatime` does not describe the function properly anymore rename it to `git_open`.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 builtin/pack-objects.c |  2 +-
 cache.h                |  2 +-
 pack-bitmap.c          |  2 +-
 sha1_file.c            | 26 ++++++++++++++++----------
 4 files changed, 19 insertions(+), 13 deletions(-)

Hi Lars,

On Mon, 24 Oct 2016, larsxschneider@gmail.com wrote:

The patch series may be a little bit more pleasant to read if you renamed git_open_noatime() to git_open() first, in a separate commit.

How about

		if ((O_CLOEXEC & sha1_file_open_flag) && errno == EINVAL) {
			sha1_file_open_flag &= ~O_CLOEXEC;

instead? It is shorter and should be just as easily optimized out by a C compiler if O_CLOEXEC was defined as 0.

I *think* the --patience diff option would have made that patch a little more obvious.

Otherwise, the patch looks fine to me, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Possibly.

If this were "we added a new parameter at the same time and each calling site of the renamed function with an extra parameter chooses to pass different value to it", then keeping the rename and the interface enhancement as two separate steps would help a lot.

But this one only renames and updates the internal implementation, without changing what the calling sites need to do. I am OK with having them together in a single patch like the one posted.

Yup, I think that makes things easier to read for humans, too.

From: Lars Schneider <larsxschneider@gmail.com>

This fixes "convert: add filter.<driver>.process option" (edcc8581) on Windows.

Consider the case of a file that requires filtering and is present in branch A but not in branch B. If A is the current HEAD and we checkout B then the following happens:

1. ce_compare_data() opens the file
2.   index_fd() detects that the file requires to run a clean filter and
     calls index_stream_convert_blob()
4.     index_stream_convert_blob() calls convert_to_git_filter_fd()
5.       convert_to_git_filter_fd() calls apply_filter() which creates a
         new long running filter process (in case it is the first file
         of this kind to be filtered)
6.       The new filter process inherits all file handles. This is the
         default on Linux/OSX and is explicitly defined in the
         `CreateProcessW` call in `mingw.c` on Windows.
7. ce_compare_data() closes the file
8. Git unlinks the file as it is not present in B

The unlink operation does not work on Windows because the filter process has still an open handle to the file. On Linux/OSX the unlink operation succeeds but the file descriptors still leak into the child process.

Fix this problem by opening files in read-cache with the CLOEXEC flag to ensure that the file descriptor does not remain open in a newly spawned process similar to 05d1ed61.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 read-cache.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

larsxschneider@gmail.com wrote:

In the case of O_CLOEXEC != 0 and repeated EINVALs, it'd be good to use something like sha1_file_open_flag as in 1/2 so we don't repeatedly hit EINVAL. Thanks.

Eric Wong <e@80x24.org> writes:

Sounds sane.  

It's just only once, so perhaps we do not mind a recursion like this?

 read-cache.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Hi Junio,

On Mon, 24 Oct 2016, Junio C Hamano wrote:

That still looks overly complicated, repeatedly ORing cloexec and recursing without need. How about this instead?

	static int oflags = O_RDONLY | O_CLOEXEC;
	int fd = open(ce->name, oflags);

	if ((O_CLOEXEC & oflags) && fd < 0 && errno == EINVAL) {
  		/* Try again w/o O_CLOEXEC: the kernel might not support it */
		oflags &= ~O_CLOEXEC;
		fd = open(ce->name, oflags);
	}

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

I deliberately separated the part that can and designed to be toggled (O_CLOEXEC) and the part that is meant to be constant (O_RDONLY), and I do not think the first part of suggestion is particularly a good idea.

I didn't write the same open twice, but I agree that an extra "we fallback to opening with a different flags" inside the if () { } block that is there exactly for implementing that fallback is an excellent idea. I like that part of the suggestion.

Thanks.

Am 24.10.2016 um 20:03 schrieb larsxschneider@gmail.com:

Today's next falls flat on its face on Windows in t0021.15 "required process filter should filter data"; might it be the failure meant here? (I haven't dug deeper, yet.)

++ test_config_global filter.protocol.process 
'/d/Src/mingw-git/t/t0021/rot13-filter.pl clean smudge'
++ test_when_finished 'test_unconfig --global 
'\''filter.protocol.process'\'''
++ test 0 = 0
++ test_cleanup='{ test_unconfig --global '\''filter.protocol.process'\''
                 } && (exit "$eval_ret"); eval_ret=$?; :'
++ git config --global filter.protocol.process 
'/d/Src/mingw-git/t/t0021/rot13-filter.pl clean smudge'
++ test_config_global filter.protocol.required true
++ test_when_finished 'test_unconfig --global 
'\''filter.protocol.required'\'''
++ test 0 = 0
++ test_cleanup='{ test_unconfig --global '\''filter.protocol.required'\''
                 } && (exit "$eval_ret"); eval_ret=$?; { test_unconfig 
--global '\''filter.protocol.process'\''
                 } && (exit "$eval_ret"); eval_ret=$?; :'
++ git config --global filter.protocol.required true
++ rm -rf repo
++ mkdir repo
++ cd repo
++ git init
Initialized empty Git repository in d:/Src/mingw-git/t/trash 
directory.t0021-conversion/repo/.git/
++ echo git-stderr.log
++ echo '*.r filter=protocol'
++ git add .
++ git commit . -m 'test commit 1'
[master (root-commit) aa5dd37] test commit 1
  Author: A U Thor <author@example.com>
  2 files changed, 2 insertions(+)
  create mode 100644 .gitattributes
  create mode 100644 .gitignore
++ git branch empty-branch
++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test.o' test.r
++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test2.o' test2.r
++ mkdir testsubdir
++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test3 
'\''sq'\'',$x.o' 'testsubdir/test3 '\''sq'\'',$x.r'
+++ file_size test.r
+++ cat test.r
+++ wc -c
+++ sed 's/^[ ]*//'
++ S=57
+++ file_size test2.r
+++ cat test2.r
+++ wc -c
+++ sed 's/^[ ]*//'
++ S2=14
+++ file_size 'testsubdir/test3 '\''sq'\'',$x.r'
+++ cat 'testsubdir/test3 '\''sq'\'',$x.r'
+++ wc -c
+++ sed 's/^[ ]*//'
++ S3=49
++ filter_git add .
++ rm -f rot13-filter.log
++ git add .
error: last command exited with $?=128
not ok 15 - required process filter should filter data

Yes, this is the failure meant here :-)

Cheers, Lars

Am 24.10.2016 um 21:53 schrieb Lars Schneider:

I trust your word and Dscho's that it fixes a failure on Windows. In my case, however, it was an outdated perl (5.8.8) which left a message along the lines of "lookup of member flush in IO::Handle failed" in one of the *.log files. I came up with the following workaround.

Fix-method: shot-in-the-dark
Perl-foo: not-present
Not-signed-off-by: Me

larsxschneider@gmail.com writes:

That is a good idea.

OK. This is the old git_open_noatime() that is meant to be used ONLY for the files Git uses for its internal implementation, and never for end-user files. I think it is a good idea to open them with O_CLOEXEC.

And the separate patch to use O_CLOEXEC in ce_compare_data() that opens a working tree file for reading does not use git_open(), which is also correct. I like it.

I think this applies cleanly to 'master', so that is where I'd fork my copy at.

Thanks.

Hi Lars,

On Mon, 24 Oct 2016, larsxschneider@gmail.com wrote:

I acknowledge that this fixes the test failures in Git for Windows' SDK.

Thanks, Dscho

Here is to make sure everybody is on the same page regarding the series. The patches are adjusted for comments from Eric and Dscho.

Lars Schneider (3):
  sha1_file: rename git_open_noatime() to git_open()
  sha1_file: open window into packfiles with O_CLOEXEC
  read-cache: make sure file handles are not inherited by child
    processes

 builtin/pack-objects.c |  2 +-
 cache.h                |  2 +-
 pack-bitmap.c          |  2 +-
 read-cache.c           |  9 ++++++++-
 sha1_file.c            | 25 +++++++++++++++----------
 5 files changed, 26 insertions(+), 14 deletions(-)

From: Lars Schneider <larsxschneider@gmail.com>

All processes that the Git main process spawns inherit the open file descriptors of the main process. These leaked file descriptors can cause problems.

Use the O_CLOEXEC flag similar to 05d1ed61 to fix the leaked file descriptors.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * And the remainder of original 1/2, again taking suggestion by DScho.

 sha1_file.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

On Tue, Oct 25, 2016 at 11:16:20AM -0700, Junio C Hamano wrote:

So if we start with O_CLOEXEC|O_NOATIME, we drop CLOEXEC here and try again with just O_NOATIME. And then if _that_ fails...

We drop O_NOATIME, and end up with an empty flag field.

But we will never have tried just O_CLOEXEC, which might have worked.

Because of the order here, this would not be a regression (i.e., any system that used to work will still eventually find a working comb), but it does mean that systems without O_NOATIME do not get the benefit of the new O_CLOEXEC protection.

Unfortunately, I think covering all of the cases would be 2^nr_flags. That's only 4 right now, but it does not bode well as a pattern.

I'm not sure it's worth worrying about or not; I don't know which systems are actually lacking either of the flags, or if they tend to have both.

-Peff

Jeff King <peff@peff.net> writes:

Yes, doing so would smudge atime, so one question is which one between noatime or cloexec is more important to be done at open(2) time.

It may be possible to open(2) only with cloexec and then fcntl(2) FD_SET noatime immediately after, but going that way would explode the combination even more, as it may not be possible to set these two flags the other way around.

On Wed, Oct 26, 2016 at 09:23:21AM -0700, Junio C Hamano wrote:

Yes, but the missing case is one where we know that O_NOATIME does not work (but O_CLOEXEC does), so we know we have to smudge the atime.

Of the two flags, I would say CLOEXEC is the more important one to respect because it may actually impact correctness (e.g., leaking descriptors to sub-processes). Whereas O_NOATIME is purely a performance optimization.

I actually wonder if it is worth carrying around the O_NOATIME hack at all. Linus added it on 2005-04-23 via 144bde78e9; the aim was to reduce the cost of opening loose object files. Some things have changed since then:

  1. In June 2005, git learned about packfiles, which means we would do
     a lot fewer atime updates (rather than one per object access, we'd
     generally get one per packfile).

  2. In late 2006, Linux learned about "relatime", which is generally
     the default on modern installs. So performance around atime updates
     is a non-issue there these days.

     All the world isn't Linux, of course, but I can't help that feel
     that atime performance hackery is something that belongs at the
     system level, not in individual applications.

So I don't have hard numbers, but I'd be surprised if O_NOATIME is really buying us anything these days.

-Peff

Jeff King <peff@peff.net> writes:

I tend to agree.

Yes, I share the thought. We no longer have too many loose objects to matter.

I do not mind flipping the order, but I'd prefer to cook the result even longer. I am tempted to suggest we take two step route:

 - ship 2.11 with the "atime has been there and we won't regress it"
   shape, while cooking the "cloexec is semantically more
   important" version in 'next' during the feature freeze

 - immediately after 2.11 merge it to 'master' for 2.12 to make sure
   there is no fallout.

On Wed, Oct 26, 2016 at 10:52:41AM -0700, Junio C Hamano wrote:

That sounds reasonable, though I'd consider jumping straight to "NOATIME is not worth it; drop it" as the patch for post-2.11.

-Peff

Jeff King <peff@peff.net> writes:

That endgame is fine by me too.  Thanks for a sanity-check.

On Wed, Oct 26, 2016 at 02:15:33PM -0700, Junio C Hamano wrote:

So here's that endgame patch. My main concern with it was that there might be non-Linux systems that could be affected. But when I dug into it, I found that this code was never activated anywhere besides Linux in the first place. So I really doubt this will have any negative impact at all. I certainly don't mind cooking it until post-2.11, though.

+cc Linus as the original author of 144bde78e9 in case there is something subtle I'm missing, but this really just seems like it's an outdated optimization.

-- >8 --
Subject: [PATCH] sha1_file: stop opening files with O_NOATIME

When we open object files, we try to do so with O_NOATIME. This dates back to 144bde78e9 (Use O_NOATIME when opening the sha1 files., 2005-04-23), which is an optimization to avoid creating a bunch of dirty inodes when we're accessing many objects. But a few things have changed since then:

  1. In June 2005, git learned about packfiles, which means
     we would do a lot fewer atime updates (rather than one
     per object access, we'd generally get one per packfile).

  2. In late 2006, Linux learned about "relatime", which is
     generally the default on modern installs. So
     performance around atimes updates is a non-issue there
     these days.

     All the world isn't Linux, but as it turns out, Linux
     is the only platform to implement O_NOATIME in the
     first place.

So it's very unlikely that this code is helping anybody these days.

It's not a particularly large amount of code, but the fallback-retry creates complexity. E.g., we do a similar fallback for CLOEXEC; which one should take precedence, or should we try all possible combinations? Dropping O_NOATIME makes those questions go away.

Signed-off-by: Jeff King <peff@peff.net>
---
 sha1_file.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

Jeff King <peff@peff.net> writes:

We may want to lose the surrounding for (;;) loop as there is only one flag to retry without, which was the original code structure back when 144bde78e9 ("Use O_NOATIME when opening the sha1 files.", 2005-04-23) was written and refactored by 44d1c19ee8 ("Make loose object file reading more careful", 2008-06-14).

IOW, this on top. The update to ce_compare_data() Lars has in a0a6cb9662 ("read-cache: make sure file handles are not inherited by child processes", 2016-10-24) could then made into a call to git_open().

 sha1_file.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

On Thu, Oct 27, 2016 at 3:24 AM, Jeff King <peff@peff.net> wrote:

I'd *really* like to keep O_NOATIME if at all possible. It made a huge difference on older kernels, and I'm not convinced that relatime really fixes it as well as O_NOATIME.

There are people who don't like relatime. And even if you do have relatime enabled, it will update atime once every day, so then this makes your filesystem have a storm of nasty inode writebacks if you haven't touched that git repo in a while.

If this is purely about mixing things up with O_CLOEXEC, then that is *trivially* fixable by just using

     fcntl(fd, F_SETFD, FD_CLOEXEC);

after the open.

Which is what you have to do anyway if you want to be portable, so its' not like you could avoid that complexity in the first place.

Note that you can *not* do the same thing with O_NOATIME, since the whole point of O_NOATIME is that it changes the behavior of the open itself (unlike O_CLOEXEC which changes _later_ behavior, and can always be replaced by FD_CLOEXEC fnclt modulo races that are immaterial for git)

             Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

A tangent.  

I also thought O_NOATIME shouldn't be an effective fcntl(2) thing, for the reasons you stated above, when I was studying the area because of the discussion on these patches. I was surprised to see that http://man7.org/linux/man-pages/man2/fcntl.2.html contradicts by saying F_SETFL can take O_NOATIME.

Perhaps it deserves a bugreport?

On Thu, Oct 27, 2016 at 3:56 PM, Junio C Hamano <gitster@pobox.com> wrote:

It's not a bug per se.

You can indeed change O_NOATIME with F_SETFL. And it actually does matter, since atime is normally changed by mmap(), read() and write() calls.

So F_SETFL O_NOATIME actually does make sense, but it doesn't cover the atime update done by open() itself.

That said, now that I think about it, I should double-check: maybe open() doesn't actually set atime at all, and we *could* do NOATIME with SETFL after all.

The exact [acm]time semantics are damn subtle.

             Linus

On Thu, Oct 27, 2016 at 4:09 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:

Checked. Yup. O_NOATIME could easily be done with SETFL:, because as with O_CLOEXEC, it only affects operations _after_ the open. The open itself doesn't set the access time.

So I was full of it.

But the basic issue still remains - I'd really prefer to have NOATIME stay around for all those poor misguided souls that for some reason don't like "relatime" or run old kernels. But whether it is with O_NOATIME at open time or with F_SETFL, I don't care.

            Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

Understood.  

Would the best endgame shape for this function be to open with O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2) but ignoring an error from it, I guess? That would be the closest to what we historically had, I would think.

On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:

I think that's the best model.

Note that the O_NOATIME code is very much designed to try O_NOATIME only _once_. Because even when the kernel supports O_NOATIME, if you have a shared object tree where you may not be the owner of all the files, a O_NOATIME open can fail with NOPERM ("You are not allowed to hide your accesses to this file").

This is why it uses that

    static unsigned int sha1_file_open_flag = O_NOATIME;

and if the O_NOATIME open ever fails (and the no-O_NOATIME open succeeds), it clears that flag. Exactly so that it will *not* end up in some kind of "let's open and fail and re-open" loop. It's designed to fail once.

Or at least that's how it used to be originally. This code has obviously changed since that early design. Now it seems to clear it for any non-ENOENT error. Which looks fine too.

          Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

OK, so perhaps like this.

-- >8 --
Subject: git_open(): untangle possible NOATIME and CLOEXEC interactions

The way we structured the fallback-retry for opening with O_NOATIME and O_CLOEXEC meant that if we failed due to lack of support to open the file with O_NOATIME option (i.e. EINVAL), we would still try to drop O_CLOEXEC first and retry, and then drop O_NOATIME. A platform on which O_NOATIME is defined in the header without support from the kernel wouldn't have a chance to open with O_CLOEXEC option due to this code structure.

Arguably, O_CLOEXEC is more important than O_NOATIME, as the latter is mostly about performance, while the former can affect correctness. Let's revert the recent changes to the way git_open() attempts to open a file with O_NOATIME and retries without to the original sequence, and then use a separate fcntl(fd, F_SETFD, FD_CLOEXEC) on the resulting file descriptor. The helper to do the latter can be usable in the codepath in ce_compare_data() that was recently added to open a file descriptor with O_CLOEXEC, so let's refactor that codepath with the helper while we are at it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-compat-util.h |  5 +++--
 read-cache.c      | 12 ++++--------
 sha1_file.c       | 49 ++++++++++++++++++++++++++++++-------------------
 3 files changed, 37 insertions(+), 29 deletions(-)

Junio C Hamano <gitster@pobox.com> writes:

Hmph.  This may not fly well in practice, though.  

To Unix folks, CLOEXEC is not a huge correctness issue. A child process may hold onto an open file descriptor a bit longer than the lifetime of the parent but as long as the child eventually exits, nothing is affected. Over there, things are different. The parent cannot even rename(2) or unlink(2) a file it created and closed while the child is still holding the file descriptor open and the lack of CLOEXEC will make the parent fail. I do not know how well fcntl(2) emulation works on Windows, but I would not be surprised if J6t or Dscho comes back and says that FD_CLOEXEC given to F_SETFD would not work while O_CLOEXEC given to open(2) does.

Junio C Hamano <gitster@pobox.com> wrote:

Actually, I would flip the order of flags. O_CLOEXEC is more important from a correctness standpoint.

I'm not too familiar with C internals of git; but I know we use threads in some places, and fork+execve in others.

If our usage of threads and execve intersects, and we run untrusted code in an execve-ed child, then only having cloexec on open() will save us time when auditing for leaking FDs.

fcntl(fd, F_SETFD, O_CLOEXEC) is racy in if there are other threads doing execve; so I wouldn't rely on it as a first choice.

So I suppose something like this:

	static int noatime = 1;
	int fd = open(... | O_CLOEXEC);
	...error checking and retrying...

	if (fd >= 0 && noatime && fcntl(fd, F_SETFL, O_NOATIME) != 0)
		noatime = 0;

	return fd;

Hi,

On Thu, 27 Oct 2016, Junio C Hamano wrote:

You guys. I mean: You guys! You sure make my life hard. A brief look at mingw.h could have answered your implicit question:

	static inline int fcntl(int fd, int cmd, ...)
	{
		if (cmd == F_GETFD || cmd == F_SETFD)
			return 0;
		errno = EINVAL;
		return -1;
	}

So while you discuss in your Linux Ivory Tower how to optimize Git for Linux, and Linux only, I'll have to drop everything else and spend the rest of my Friday trying to find a way to adjust a file handle *immediately after opening it with undesired flags* (when it could have been opened with the desired flags, as suggested, to begin with).

Ciao, Johannes

On Fri, Oct 28, 2016 at 4:11 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

So here's what you guys should do:

 - leave O_NOATIME damn well alone. It works. It has worked for 10+
years. Stop arguing against it, people who do.

 - get rid of all O_CLOEXEC games. They don't work. If you want to
close file descriptors at execve(), you - gasp - close the file
descriptor before doing an execve.

So O_CLOEXEC or FD_CLOEXEC is broken.

DO NOT BREAK O_NOATIME JUST TO ADD COMPLETELY NEW BREAKAGE.

                 Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

Excuse me, but care to elaborate a bit more?

Did I botch the way I ask fcntl(2) to set O_NOATIME in *1*? Its follow-up in *2* is optional and as peff said in *3*, I do not think we mind dropping that step and keeping O_NOATIME. It is not that much code to keep.

Setting FD_CLOEXEC with fcntl(2) may be racy, but in what way it is broken to open(2) with O_CLOEXEC?

If we want to close file descriptors we opened at execve() time ourselves, we'd need to somehow keep track of which file descriptors are meant to be closed upon execve() time, and one way to mark that may be to use O_CLOEXEC, but once we mark a file descriptor with the bit, execve() would take care of "closing" it for us---wouldn't that be the whole point of that bit?

*1* http://public-inbox.org/git/xmqqh97w38gj.fsf@gitster.mtv.corp.google.com *2* http://public-inbox.org/git/xmqqd1ik38f4.fsf@gitster.mtv.corp.google.com *3* http://public-inbox.org/git/20161028075104.la24zydnr3ogb6qv@sigill.intra.peff.net

On Fri, Oct 28, 2016 at 9:48 AM, Junio C Hamano <gitster@pobox.com> wrote:

It just seems entirely pointless to change the games with O_xyz.

Apparently windows doesn't even support it, at least not mingw. So you'll have to have some supprt for just explicitly closing things at execve anyway. And if you do that for windows, then what's the point of using O_CLOEXEC on Linux, and having two totally different paths that will just mean maintenance headache.

In contrast, O_NOATIME isn't a maintenance problem, since it's purely an optimization and has no semantic difference, so it not existing on some platform is immaterial.

End result: leave O_NOATIME as it is (working), and just forget about O_CLOEXEC.

I have no actual idea about mingw support for this, but Johannes' email certainly implies it's painful on Windows. Some googling also shows people doing things like

  #ifdef O_CLOEXEC
          flags |= O_CLOEXEC;
  #endif

or

  #ifndef O_CLOEXEC
  #define O_CLOEXEC 0
  #endif

so O_CLOEXEC definitely isn't considered portable. Do you really want to rely on it?

              Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

Ah, that's where your reaction comes from. If I understood Dscho correctly, what he said was that I cannot set FD_CLOEXEC via fcntl() emulation they have. It wasn't an objection to O_CLOEXEC. In fact, since 05d1ed6148 ("mingw: ensure temporary file handles are not inherited by child processes", 2016-08-22), we have been relying on open(2) with O_CLOEXEC even on Windows (by emulating it with O_NOINHERIT, which Windows has) on some codepaths.

Linus Torvalds <torvalds@linux-foundation.org> writes:

Assuming that the above was a misunderstanding, assuming that we can do O_CLOEXEC (but not FD_CLOEXEC) on Windows just fine, where stray file descriptors held open in the children matter more, and ...

... assuming that I didn't screw up my use of fcntl() to set O_NOATIME on a fd opened with O_CLOEXEC, are you OK with the approach in patch *1*? We can drop *2* to keep O_NOATIME, which has been working for those with old kernels with many loose objects, and it is not too much code to keep.

*1* http://public-inbox.org/git/xmqqh97w38gj.fsf@gitster.mtv.corp.google.com *2* http://public-inbox.org/git/xmqqd1ik38f4.fsf@gitster.mtv.corp.google.com

Hi,

On Fri, 28 Oct 2016, Junio C Hamano wrote:

Correct. We cannot change an open file handle's state ("FD_CLOEXEC") on Windows, but we can ask open() to open said file handle with the correct flag ("O_CLOEXEC", which is mapped to O_NOINHERIT on Windows.

And for the record: I agree with Junio that we cannot simply drop this O_CLOEXEC business.

Because it was introduced to fix bugs.

Thank you for your time, Johannes

On Sat, Oct 29, 2016 at 1:25 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Ok. So then I have no issues with it, and let's use O_CLOEXEC if it exists and fcntl(FD_CLOEXEC) if O_CLOEXEC doesn't exist.

              Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

I am not sure if the fallback to FD_CLOEXEC is worth doing, because it does not exist on Windows, where leaking an open file descriptor to a child process matters more than other platforms [*1*].

Of course, the world is not all Windows, and there may be other platforms this fallback may help. But there is a chance that some other thread may fork between the time we open(2) and the time we call fcntl(2), leaving FD_CLOEXEC ineffective and unreliable, so that's another thing that makes me doubt if FD_CLOEXEC fallback is worth doing.

Having said that, here is a rewrite of [*2*] to use the fallback would look like. After this topic settles, we may want to also update the tempfile.c::create_tempfile() implementation to use the helper function we use here.

[Footnotes]

*1* The call in ce_compare_data() is to see if the contents in the
    working tree file match what is in the index.  Perhaps the
    program is "git checkout another-branch" that knows this path is
    absent in the branch being switched to and trying to make sure
    we lose no local modification.  When the path needs to be passed
    through the clean/smudge filter before hashing to see if the
    working tree contents hashes to the object in the index, we need
    to fork and then after the filter finishes its processing, we
    close the file descriptor and then unlink(2) the path if it is
    clean.  We are about to add a variant of clean/smudge mechanism
    where a lazily spawned process can clean contents of multiple
    paths and that is where cloexec starts to matter.  This function
    starts to inspect one path, opens a fd to it, finds that it
    needs filtering, spawns a long-lingering process, while leaking
    the original fd to it.  After feeding the contents via pipe and
    then receiving the filtered contents (the protocol is not full
    duplex and there won't be a stuck pipe), the parent would decide
    that the path is clean and attempts to unlink(2).  Windows does
    not let you unlink a path with open filedescriptor held, causing
    "checkout" to fail.  As it is only one fd that leaks to the
    child, no matter how many subsequent paths are filtered by the
    same long-lingering child process, it will be a far less
    important issue on other platforms that lets you unlink(2) such
    a file.

*2* http://public-inbox.org/git/xmqqh97w38gj.fsf@gitster.mtv.corp.google.com

---
 cache.h           |  1 +
 git-compat-util.h |  4 ++++
 read-cache.c      |  9 +--------
 sha1_file.c       | 47 ++++++++++++++++++++++++++++-------------------
 4 files changed, 34 insertions(+), 27 deletions(-)

On Fri, Oct 28, 2016 at 09:13:41AM -0700, Linus Torvalds wrote:

For some definition of worked, perhaps.

If you set a probe on touch_atime() in the kernel (which is called for every attempt to smudge the atime, regardless of mount options, but is skipped when the descriptor was opened with O_NOATIME), you can see the impact. Here's a command I picked because it reads a lot of objects (run on my git.git clone):

  $ perf stat -e probe:touch-atime git log -Sfoo >/dev/null

And the probe:touch_atime counts before (stock git) and after (a patch to drop O_NOATIME):

  before: 22,235
   after: 22,362

So that's only half a percent difference. And it's on a reasonably messy clone that is partway to triggering an auto-repack:

  $ git count-objects -v
  count: 6167
  size: 61128
  in-pack: 275773
  packs: 18
  size-pack: 86857
  prune-packable: 25
  garbage: 0
  size-garbage: 0

Running "git gc" drops the probe count to 21,733.

It makes a bigger difference for some commands (it's more like 10% for git-status). And smaller for others ("git log -p" triggers it over 100,000 times).

One thing missing in that count is how many of those calls would have resulted in an actual disk write. Looking at strace, most of the filesystem activity is opening .gitattributes files, and we end up opening the same ones repeatedly (e.g., t/.gitattributes in git.git). Multiple hits for a given inode in the same second get coalesced into at most a single disk write.

So I guess it's possible that it produces a noticeable effect in some cases, but I'm still somewhat doubtful. And actually repacking your repository had a greater effect in every case I measured (in addition to providing other speedups).

Like I said, I'm OK keeping O_NOATIME. It's just not that much code. But if you really care about the issue of dirtying inodes via atime, you should look into vastly increasing our use of O_NOATIME. Or possibly looking at caching more in the attribute code.

-Peff

Jeff King <peff@peff.net> writes:

Let's keep doubting. I prefer one-step-at-a-time approach to things anyway, and what I plan in the near term are:

 * use the "open() with O_NOATIME|O_CLOEXEC, gradually losing the
   bits during fallback" approach in the ls/git-open-cloexec topic,
   in order to help ls/filter-process topic be part of the upcoming
   release;

 * simplify the logic to the "open(2) with O_CLOEXEC, set O_NOATIME
   with fcntl(2)" in jc/git-open-cloexec~1 after 2.11 ships;

 * cook "drop the latter half of setting O_NOATIME" which is at the
   tip of jc/git-open-cloexec in 'next', and while Linus is looking
   the other way ^W^W^W^W^W^W^W after people had chance to complain
   with numbers, merge it to a future release iff it still looked OK
   to drop O_NOATIME thing.

On Mon, Oct 31, 2016 at 10:55:32AM -0700, Junio C Hamano wrote:

Great, that sounds like a good way to proceed (and if the final step never happens, no big deal).

-Peff

Junio C Hamano <gitster@pobox.com> writes:

We can flip the order around, and then it becomes simpler to later drop atime support. The first step goes like this.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 28 Oct 2016 06:23:07 -0700
Subject: [PATCH] git_open(): untangle possible NOATIME and CLOEXEC interactions

The way we structured the fallback/retry mechanism for opening with O_NOATIME and O_CLOEXEC meant that if we failed due to lack of support to open the file with O_NOATIME option (i.e. EINVAL), we would still try to drop O_CLOEXEC first and retry, and then drop O_NOATIME. A platform on which O_NOATIME is defined in the header without support from the kernel wouldn't have a chance to open with O_CLOEXEC option due to this code structure.

Arguably, O_CLOEXEC is more important than O_NOATIME, as the latter is mostly about performance, while the former can affect correctness.

Instead use O_CLOEXEC to open the file, and then use fcntl(2) to set O_NOATIME on the resulting file descriptor. open(2) itself does not cause atime to be updated according to Linus [*1*].

The helper to do the former can be usable in the codepath in ce_compare_data() that was recently added to open a file descriptor with O_CLOEXEC; use it while we are at it.

*1* <CA+55aFw83E+zOd+z5h-CA-3NhrLjVr-anL6pubrSWttYx3zu8g@mail.gmail.com>

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h      |  1 +
 read-cache.c |  9 +--------
 sha1_file.c  | 39 +++++++++++++++++++--------------------
 3 files changed, 21 insertions(+), 28 deletions(-)

Junio C Hamano <gitster@pobox.com> writes:

And the second step would be like this.

-- >8 --
Subject: [PATCH] sha1_file: stop opening files with O_NOATIME

When we open object files, we try to do so with O_NOATIME. This dates back to 144bde78e9 (Use O_NOATIME when opening the sha1 files., 2005-04-23), which is an optimization to avoid creating a bunch of dirty inodes when we're accessing many objects. But a few things have changed since then:

  1. In June 2005, git learned about packfiles, which means
     we would do a lot fewer atime updates (rather than one
     per object access, we'd generally get one per packfile).

  2. In late 2006, Linux learned about "relatime", which is
     generally the default on modern installs. So
     performance around atimes updates is a non-issue there
     these days.

     All the world isn't Linux, but as it turns out, Linux
     is the only platform to implement O_NOATIME in the
     first place.

So it's very unlikely that this code is helping anybody these days.

Helped-by: Jeff King <peff@peff.net>
[jc: took idea and log message from peff]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h     |  2 +-
 sha1_file.c | 21 ---------------------
 2 files changed, 1 insertion(+), 22 deletions(-)

On Thu, Oct 27, 2016 at 03:38:59PM -0700, Linus Torvalds wrote:

The existence of "relatime" is only half the story of its outdatedness.

The other half is packfiles, so that we are paying atime only once per packfile, not once per object (technically once per mmap(), so on a 32-bit system with large packfiles, it would be multiple, depending on your window size).

So I'm not convinced that "storm" is really the right word in a modern context. The atime updates due to object accesses are probably smaller than those from all the other read() calls being done on non-object files (like config, refs, etc).

That being said, if you really care, it's not that much code to keep.

-Peff

From: Lars Schneider <larsxschneider@gmail.com>

This function is meant to be used when reading from files in the object store, and the original objective was to avoid smudging atime of loose object files too often, hence its name. Because we'll be extending its role in the next commit to also arrange the file descriptors they return auto-closed in the child processes, rename it to lose "noatime" part that is too specific.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is a pure-rename step suggested by Dscho.

 builtin/pack-objects.c |  2 +-
 cache.h                |  2 +-
 pack-bitmap.c          |  2 +-
 sha1_file.c            | 12 ++++++------
 4 files changed, 9 insertions(+), 9 deletions(-)

From: Lars Schneider <larsxschneider@gmail.com>

This fixes "convert: add filter.<driver>.process option" (edcc8581) on Windows.

Consider the case of a file that requires filtering and is present in branch A but not in branch B. If A is the current HEAD and we checkout B then the following happens:

1. ce_compare_data() opens the file
2.   index_fd() detects that the file requires to run a clean filter and
     calls index_stream_convert_blob()
4.     index_stream_convert_blob() calls convert_to_git_filter_fd()
5.       convert_to_git_filter_fd() calls apply_filter() which creates a
         new long running filter process (in case it is the first file
         of this kind to be filtered)
6.       The new filter process inherits all file handles. This is the
         default on Linux/OSX and is explicitly defined in the
         `CreateProcessW` call in `mingw.c` on Windows.
7. ce_compare_data() closes the file
8. Git unlinks the file as it is not present in B

The unlink operation does not work on Windows because the filter process has still an open handle to the file. On Linux/OSX the unlink operation succeeds but the file descriptors still leak into the child process.

Fix this problem by opening files in read-cache with the O_CLOEXEC flag to ensure that the file descriptor does not remain open in a newly spawned process similar to 05d1ed6148 ("mingw: ensure temporary file handles are not inherited by child processes", 2016-08-22).

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * With Eric's suggestion to avoid repeated EINVAL and fallback,
   implemented with Dscho's "write open twice explicitly, we are
   retrying after all" coding style.

 read-cache.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Junio C Hamano <gitster@pobox.com> wrote:

Seems fine, I prefer not using recursion so it's easier-to-review.

But I have a _slight_ preference towards Dscho's version in <alpine.DEB.2.20.1610251230150.3264@virtualbox> in case we decide to start using another O_* flag in here. (but I'm not usually a C programmer)

Eric Wong <e@80x24.org> writes:

Interesting. The reason why I have a slight preferene to separate the fixed part and the toggle-able part is exactly because I want the code to be prepared in case we decide to start using other O_* flags in the future.

I guess different people have different tastes, as usual ;-)

Thank you, Junio! Your v3 looks good to me and I compiled and tested it on Windows.

There is one catch though: I ran the tests multiple times on Windows and I noticed that the following test seems flaky: t0021 `required process filter should be used only for "clean" operation only'

This flaky-ness was not introduced by your v3. I was able to reproduce the same behavior for v2. I wonder why I did not noticed that before. The only difference to before is that my Windows machines does some heavy CPU/network task in the background now (I can't/don't want to stop that ATM).

Here is the relevant part of the log:

++ rm -f rot13-filter.log
++ git checkout --quiet --no-progress .
+ test_eval_ret_=255
+ want_trace
+ test t = t
+ test t = t
+ set +x
error: last command exited with $?=255

Looks like Git exists with 255. Any idea why that might happen?

@Dscho/Johannes: Can you try this on your Windows machines?

Thanks, Lars

Lars Schneider <larsxschneider@gmail.com> writes:

Other than "the --quiet above may be hiding it?", I do not immediately have a useful guess, sorry.