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

Jeff King [off-list ref] writes:

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

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(-)

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

@@ -1553,24 +1553,17 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
 
 int git_open(const char *name)
 {
-	static int sha1_file_open_flag = O_CLOEXEC;
-
-	for (;;) {
-		int fd;
-
-		errno = 0;
-		fd = open(name, O_RDONLY | sha1_file_open_flag);
-		if (fd >= 0)
-			return fd;
+	static int cloexec = O_CLOEXEC;
+	int fd;
 
+	errno = 0;
+	fd = open(name, O_RDONLY | cloexec);
+	if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
 		/* Try again w/o O_CLOEXEC: the kernel might not support it */
-		if ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {
-			sha1_file_open_flag &= ~O_CLOEXEC;
-			continue;
-		}
-
-		return -1;
+		cloexec &= ~O_CLOEXEC;
+		fd = open(name, O_RDONLY | cloexec);
 	}
+	return fd;
 }
 
 static int stat_sha1_file(const unsigned char *sha1, struct stat *st)