Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 27, 2016, 21:49 UTC
- Message-ID
- <xmqq60od5up3.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20161027102419.dbzigj7wtr355ofh@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 37 quoted lines
> +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> > ---
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)