Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

Jeff King [off-list ref] writes:

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

quoted

quoted

I actually wonder if it is worth carrying around the O_NOATIME hack at
all.

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.

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

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:

Jeff King [off-list ref] writes:

quoted

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

quoted

quoted

I actually wonder if it is worth carrying around the O_NOATIME hack at
all.

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.

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

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

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 <redacted>
---
 sha1_file.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 09045df1dc..6f02a57d8b 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -27,14 +27,6 @@
 #include "list.h"
 #include "mergesort.h"
 
-#ifndef O_NOATIME
-#if defined(__linux__) && (defined(__i386__) || defined(__PPC__))
-#define O_NOATIME 01000000
-#else
-#define O_NOATIME 0
-#endif
-#endif
-
 #define SZ_FMT PRIuMAX
 static inline uintmax_t sz_fmt(size_t s) { return s; }
 

@@ -1561,7 +1553,7 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
 
 int git_open(const char *name)
 {
-	static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
+	static int sha1_file_open_flag = O_CLOEXEC;
 
 	for (;;) {
 		int fd;

@@ -1577,11 +1569,6 @@ int git_open(const char *name)
 			continue;
 		}
 
-		/* Might the failure be due to O_NOATIME? */
-		if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
-			sha1_file_open_flag &= ~O_NOATIME;
-			continue;
-		}
 		return -1;
 	}
 }

-- 
2.10.1.916.g0d2035c

On Thu, Oct 27, 2016 at 3:24 AM, Jeff King [off-list ref] wrote:

+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.

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

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

On Thu, Oct 27, 2016 at 3:24 AM, Jeff King [off-list ref] wrote:

quoted

+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.

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.

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