From 8e060ec225b74bbf22e5bdbacd604efcc73294c0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 25 Oct 2018 21:16:47 +0200 Subject: [PATCH 1/2] fs-util: increase start buffer size in readlinkat_malloc() I noticed while profiling journald that we invoke readlinkat() a ton on open /proc/self/fd/, and that the returned paths are more often than not longer than the 99 chars used before, when we look at archived journal files. This means for these cases we generally need to execute two rather than one syscalls. Let's increase the buffer size a tiny bit, so that we reduce the number of syscalls executed. This is really a low-hanging fruit of optimization. --- src/basic/fs-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index fd19518271..55651baa80 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -132,7 +132,7 @@ int rename_noreplace(int olddirfd, const char *oldpath, int newdirfd, const char } int readlinkat_malloc(int fd, const char *p, char **ret) { - size_t l = 100; + size_t l = FILENAME_MAX+1; int r; assert(p); From f267719c389de57ceda433f6288a505e7eeb2f8f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 25 Oct 2018 21:27:00 +0200 Subject: [PATCH 2/2] fd-util: optimize fd_get_path() a bit journald calls fd_get_path() a lot (it probably shouldn't, there's some room for improvement there, but I'll leave that for another time), hence it's worth optimizing the call a bit, in particular as it's easy. Previously we'd open the dir /proc/self/fd/ first, before reading the symlink inside it. This means the whole function requires three system calls: open(), readlinkat(), close(). The reason for doing it this way is to distinguish the case when we see ENOENT because /proc is not mounted and the case when the fd doesn't exist. With this change we'll directly go for the readlink(), and only if that fails do an access() to see if /proc is mounted at all. This optimizes the common case (where the fd is valid and /proc mounted), in favour of the uncommon case (where the fd doesn#t exist or /proc is not mounted). --- src/basic/fd-util.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index b97bd191ab..5775a13e3e 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -353,22 +353,22 @@ bool fdname_is_valid(const char *s) { } int fd_get_path(int fd, char **ret) { - _cleanup_close_ int dir = -1; - char fdname[DECIMAL_STR_MAX(int)]; + char procfs_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int)]; int r; - dir = open("/proc/self/fd/", O_CLOEXEC | O_DIRECTORY | O_PATH); - if (dir < 0) - /* /proc is not available or not set up properly, we're most likely - * in some chroot environment. */ - return errno == ENOENT ? -EOPNOTSUPP : -errno; + xsprintf(procfs_path, "/proc/self/fd/%i", fd); + r = readlink_malloc(procfs_path, ret); + if (r == -ENOENT) { + /* ENOENT can mean two things: that the fd does not exist or that /proc is not mounted. Let's make + * things debuggable and distuingish the two. */ - xsprintf(fdname, "%i", fd); + if (access("/proc/self/fd/", F_OK) < 0) + /* /proc is not available or not set up properly, we're most likely in some chroot + * environment. */ + return errno == ENOENT ? -EOPNOTSUPP : -errno; - r = readlinkat_malloc(dir, fdname, ret); - if (r == -ENOENT) - /* If the file doesn't exist the fd is invalid */ - return -EBADF; + return -EBADF; /* The directory exists, hence it's the fd that doesn't. */ + } return r; }