Jeff King <peff@xxxxxxxx> 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 <peff@xxxxxxxx>
> ---
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)