From 1b8ac5ead520146802debd52cb38e5c27b3483a2 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 28 Oct 2016 06:23:07 -0700 Subject: 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* Signed-off-by: Junio C Hamano --- cache.h | 1 + read-cache.c | 9 +-------- sha1_file.c | 39 +++++++++++++++++++-------------------- 3 files changed, 21 insertions(+), 28 deletions(-) diff --git a/cache.h b/cache.h index a902ca1f8e..6f1c21a352 100644 --- a/cache.h +++ b/cache.h @@ -1122,6 +1122,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type, extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags); extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *); extern int force_object_loose(const unsigned char *sha1, time_t mtime); +extern int git_open_cloexec(const char *name, int flags); extern int git_open(const char *name); extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size); extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz); diff --git a/read-cache.c b/read-cache.c index db5d910642..c27d3e240b 100644 --- a/read-cache.c +++ b/read-cache.c @@ -156,14 +156,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st) static int ce_compare_data(const struct cache_entry *ce, struct stat *st) { int match = -1; - static int cloexec = O_CLOEXEC; - int fd = open(ce->name, O_RDONLY | cloexec); - - if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) { - /* Try again w/o O_CLOEXEC: the kernel might not support it */ - cloexec &= ~O_CLOEXEC; - fd = open(ce->name, O_RDONLY | cloexec); - } + int fd = git_open_cloexec(ce->name, O_RDONLY); if (fd >= 0) { unsigned char sha1[20]; diff --git a/sha1_file.c b/sha1_file.c index 09045df1dc..dfbf398183 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -1559,31 +1559,30 @@ int check_sha1_signature(const unsigned char *sha1, void *map, return hashcmp(sha1, real_sha1) ? -1 : 0; } -int git_open(const char *name) +int git_open_cloexec(const char *name, int flags) { - static int sha1_file_open_flag = O_NOATIME | 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 = open(name, flags | 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; - } + cloexec &= ~O_CLOEXEC; + fd = open(name, flags | cloexec); + } + return fd; +} - /* 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; +int git_open(const char *name) +{ + static int noatime = O_NOATIME; + int fd = git_open_cloexec(name, O_RDONLY); + + if (0 <= fd && (noatime & O_NOATIME)) { + int flags = fcntl(fd, F_GETFL); + if (fcntl(fd, F_SETFL, flags | noatime)) + noatime = 0; } + return fd; } static int stat_sha1_file(const unsigned char *sha1, struct stat *st) -- cgit v1.2.3 From 1e3001a8e2386a1433d1769a5a78007596bbc04d Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 31 Oct 2016 10:41:41 -0700 Subject: git_open_cloexec(): use fcntl(2) w/ FD_CLOEXEC fallback A platform might not support open(2) with O_CLOEXEC but may support telling the same with fcntl(2) to flip FD_CLOEXEC bit on on an open file descriptor. It is a fallback that is inherently racy and this may not be worth doing, though. Suggested-by: Linus Torvalds Signed-off-by: Junio C Hamano --- sha1_file.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index dfbf398183..64e1a21fc6 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -1561,14 +1561,28 @@ int check_sha1_signature(const unsigned char *sha1, void *map, int git_open_cloexec(const char *name, int flags) { - static int cloexec = O_CLOEXEC; - int fd = open(name, flags | cloexec); + int fd; + static int o_cloexec = O_CLOEXEC; - if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) { + fd = open(name, flags | o_cloexec); + if ((o_cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) { /* Try again w/o O_CLOEXEC: the kernel might not support it */ - cloexec &= ~O_CLOEXEC; - fd = open(name, flags | cloexec); + o_cloexec &= ~O_CLOEXEC; + fd = open(name, flags | o_cloexec); } + +#if defined(F_GETFL) && defined(F_SETFL) && defined(FD_CLOEXEC) + { + static int fd_cloexec = FD_CLOEXEC; + + if (!o_cloexec && 0 <= fd && fd_cloexec) { + /* Opened w/o O_CLOEXEC? try with fcntl(2) to add it */ + int flags = fcntl(fd, F_GETFL); + if (fcntl(fd, F_SETFL, flags | fd_cloexec)) + fd_cloexec = 0; + } + } +#endif return fd; } -- cgit v1.2.3 From b4d065df03049bacfbc40467b60b13e804b7d289 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 28 Oct 2016 06:29:27 -0700 Subject: 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 [jc: took idea and log message from peff] Signed-off-by: Junio C Hamano --- cache.h | 2 +- sha1_file.c | 21 --------------------- 2 files changed, 1 insertion(+), 22 deletions(-) diff --git a/cache.h b/cache.h index 6f1c21a352..f440d3fd1e 100644 --- a/cache.h +++ b/cache.h @@ -1123,7 +1123,7 @@ extern int hash_sha1_file_literally(const void *buf, unsigned long len, const ch extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *); extern int force_object_loose(const unsigned char *sha1, time_t mtime); extern int git_open_cloexec(const char *name, int flags); -extern int git_open(const char *name); +#define git_open(name) git_open_cloexec(name, O_RDONLY) extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size); extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz); extern int parse_sha1_header(const char *hdr, unsigned long *sizep); diff --git a/sha1_file.c b/sha1_file.c index 64e1a21fc6..4e062edea0 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; } @@ -1586,19 +1578,6 @@ int git_open_cloexec(const char *name, int flags) return fd; } -int git_open(const char *name) -{ - static int noatime = O_NOATIME; - int fd = git_open_cloexec(name, O_RDONLY); - - if (0 <= fd && (noatime & O_NOATIME)) { - int flags = fcntl(fd, F_GETFL); - if (fcntl(fd, F_SETFL, flags | noatime)) - noatime = 0; - } - return fd; -} - static int stat_sha1_file(const unsigned char *sha1, struct stat *st) { struct alternate_object_database *alt; -- cgit v1.2.3