Skip to content

Commit

Permalink
rpm: Restore performance in Docker containers
Browse files Browse the repository at this point in the history
If the maximum number of open file descriptors is much greater than the
usual 1024 (for example inside a Docker container), the performance
drops significantly.

This was reported upstream in:
https://bugzilla.redhat.com/show_bug.cgi?id=1537564
which resulted in:
rpm-software-management/rpm#444

The pull request above has now been integrated and this commit contains
a backport of its three patches, which together change the behavior of
rpm so that its performance is now independent of the maximum number of
open file descriptors.

(From OE-Core rev: 6ecb10e3952af4a77bc79160ecd81117e97d022a)

Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
Signed-off-by: Armin Kuster <akuster808@gmail.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
  • Loading branch information
Saur2000 authored and rpurdie committed Jun 15, 2018
1 parent a209813 commit 4b6ff20
Show file tree
Hide file tree
Showing 4 changed files with 304 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
From 9c3e5de3240554c8ea1b29d52eeadee4840fefac Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Tue, 29 May 2018 17:37:05 -0700
Subject: [PATCH 1/3] Factor out and unify setting CLOEXEC

Commit 7a7c31f5 ("Set FD_CLOEXEC on opened files before exec from
lua script is called") copied the code that sets CLOEXEC flag on all
possible file descriptors from lib/rpmscript.c to luaext/lposix.c,
essentially creating two copies of the same code (modulo comments
and the unused assignment).

This commit moves the functionality into its own function, without
any code modifications, using the version from luaext/lposix.c.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Upstream-Status: Accepted [https://github.com/rpm-software-management/rpm/pull/444]
Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---
lib/rpmscript.c | 18 ++----------------
luaext/lposix.c | 13 ++-----------
rpmio/rpmio.c | 16 ++++++++++++++++
rpmio/rpmio_internal.h | 6 ++++++
4 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/lib/rpmscript.c b/lib/rpmscript.c
index 747385a5b..b4ccd3246 100644
--- a/lib/rpmscript.c
+++ b/lib/rpmscript.c
@@ -3,7 +3,6 @@
#include <sys/types.h>
#include <sys/wait.h>
#include <errno.h>
-#include <unistd.h>

#include <rpm/rpmfileutil.h>
#include <rpm/rpmmacro.h>
@@ -14,6 +13,7 @@

#include "rpmio/rpmlua.h"
#include "lib/rpmscript.h"
+#include "rpmio/rpmio_internal.h"

#include "lib/rpmplugins.h" /* rpm plugins hooks */

@@ -170,26 +170,12 @@ static const char * const SCRIPT_PATH = "PATH=/sbin:/bin:/usr/sbin:/usr/bin:/usr
static void doScriptExec(ARGV_const_t argv, ARGV_const_t prefixes,
FD_t scriptFd, FD_t out)
{
- int flag;
- int fdno;
int xx;
- int open_max;

/* SIGPIPE is ignored in rpm, reset to default for the scriptlet */
(void) signal(SIGPIPE, SIG_DFL);

- /* XXX Force FD_CLOEXEC on all inherited fdno's. */
- open_max = sysconf(_SC_OPEN_MAX);
- if (open_max == -1) {
- open_max = 1024;
- }
- for (fdno = 3; fdno < open_max; fdno++) {
- flag = fcntl(fdno, F_GETFD);
- if (flag == -1 || (flag & FD_CLOEXEC))
- continue;
- xx = fcntl(fdno, F_SETFD, FD_CLOEXEC);
- /* XXX W2DO? debug msg for inheirited fdno w/o FD_CLOEXEC */
- }
+ rpmSetCloseOnExec();

if (scriptFd != NULL) {
int sfdno = Fileno(scriptFd);
diff --git a/luaext/lposix.c b/luaext/lposix.c
index 0a7c26c71..5d7ad3c87 100644
--- a/luaext/lposix.c
+++ b/luaext/lposix.c
@@ -27,6 +27,7 @@
#include <unistd.h>
#include <utime.h>
#include <rpm/rpmutil.h>
+#include "rpmio/rpmio_internal.h"

#define MYNAME "posix"
#define MYVERSION MYNAME " library for " LUA_VERSION " / Nov 2003"
@@ -335,21 +336,11 @@ static int Pexec(lua_State *L) /** exec(path,[args]) */
const char *path = luaL_checkstring(L, 1);
int i,n=lua_gettop(L);
char **argv;
- int flag, fdno, open_max;

if (!have_forked)
return luaL_error(L, "exec not permitted in this context");

- open_max = sysconf(_SC_OPEN_MAX);
- if (open_max == -1) {
- open_max = 1024;
- }
- for (fdno = 3; fdno < open_max; fdno++) {
- flag = fcntl(fdno, F_GETFD);
- if (flag == -1 || (flag & FD_CLOEXEC))
- continue;
- fcntl(fdno, F_SETFD, FD_CLOEXEC);
- }
+ rpmSetCloseOnExec();

argv = malloc((n+1)*sizeof(char*));
if (argv==NULL) return luaL_error(L,"not enough memory");
diff --git a/rpmio/rpmio.c b/rpmio/rpmio.c
index c7cbc32aa..ea111d2ec 100644
--- a/rpmio/rpmio.c
+++ b/rpmio/rpmio.c
@@ -1759,3 +1759,19 @@ DIGEST_CTX fdDupDigest(FD_t fd, int id)

return ctx;
}
+
+void rpmSetCloseOnExec(void)
+{
+ int flag, fdno, open_max;
+
+ open_max = sysconf(_SC_OPEN_MAX);
+ if (open_max == -1) {
+ open_max = 1024;
+ }
+ for (fdno = 3; fdno < open_max; fdno++) {
+ flag = fcntl(fdno, F_GETFD);
+ if (flag == -1 || (flag & FD_CLOEXEC))
+ continue;
+ fcntl(fdno, F_SETFD, FD_CLOEXEC);
+ }
+}
diff --git a/rpmio/rpmio_internal.h b/rpmio/rpmio_internal.h
index fbed183b0..370cbdc75 100644
--- a/rpmio/rpmio_internal.h
+++ b/rpmio/rpmio_internal.h
@@ -41,6 +41,12 @@ DIGEST_CTX fdDupDigest(FD_t fd, int id);
int rpmioSlurp(const char * fn,
uint8_t ** bp, ssize_t * blenp);

+/**
+ * Set close-on-exec flag for all opened file descriptors, except
+ * stdin/stdout/stderr.
+ */
+void rpmSetCloseOnExec(void);
+
#ifdef __cplusplus
}
#endif
100 changes: 100 additions & 0 deletions meta/recipes-devtools/rpm/files/0002-Optimize-rpmSetCloseOnExec.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
From 5e6f05cd8dad6c1ee6bd1e6e43f176976c9c3416 Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Tue, 29 May 2018 17:52:56 -0700
Subject: [PATCH 2/3] Optimize rpmSetCloseOnExec

In case maximum number of open files limit is set too high, both
luaext/Pexec() and lib/doScriptExec() spend way too much time
trying to set FD_CLOEXEC flag for all those file descriptors,
resulting in severe increase of time it takes to execute say
rpm or dnf.

This becomes increasingly noticeable when running with e.g. under
Docker, the reason being:

> $ docker run fedora ulimit -n
> 1048576

One obvious fix is to use procfs to get the actual list of opened fds
and iterate over it. My quick-n-dirty benchmark shows the /proc approach
is about 10x faster than iterating through a list of just 1024 fds,
so it's an improvement even for default ulimit values.

Note that the old method is still used in case /proc is not available.

While at it,

1. fix the function by making sure we modify (rather than set)
the existing flags. As the only known flag is FD_CLOEXEC,
this change is currently purely aesthetical, but in case
other flags will appear it will become a real bug fix.

2. get rid of magic number 3; use STDERR_FILENO

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Fixes #444

Upstream-Status: Accepted [https://github.com/rpm-software-management/rpm/pull/444]
Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---
rpmio/rpmio.c | 43 ++++++++++++++++++++++++++++++++++---------
1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/rpmio/rpmio.c b/rpmio/rpmio.c
index ea111d2ec..55351c221 100644
--- a/rpmio/rpmio.c
+++ b/rpmio/rpmio.c
@@ -1760,18 +1760,43 @@ DIGEST_CTX fdDupDigest(FD_t fd, int id)
return ctx;
}

+static void set_cloexec(int fd)
+{
+ int flags = fcntl(fd, F_GETFD);
+
+ if (flags == -1 || (flags & FD_CLOEXEC))
+ return;
+
+ fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
+}
+
void rpmSetCloseOnExec(void)
{
- int flag, fdno, open_max;
+ const int min_fd = STDERR_FILENO; /* don't touch stdin/out/err */
+ int fd;
+
+ DIR *dir = opendir("/proc/self/fd");
+ if (dir == NULL) { /* /proc not available */
+ /* iterate over all possible fds, might be slow */
+ int open_max = sysconf(_SC_OPEN_MAX);
+ if (open_max == -1)
+ open_max = 1024;

- open_max = sysconf(_SC_OPEN_MAX);
- if (open_max == -1) {
- open_max = 1024;
+ for (fd = min_fd + 1; fd < open_max; fd++)
+ set_cloexec(fd);
+
+ return;
}
- for (fdno = 3; fdno < open_max; fdno++) {
- flag = fcntl(fdno, F_GETFD);
- if (flag == -1 || (flag & FD_CLOEXEC))
- continue;
- fcntl(fdno, F_SETFD, FD_CLOEXEC);
+
+ /* iterate over fds obtained from /proc */
+ struct dirent *entry;
+ while ((entry = readdir(dir)) != NULL) {
+ fd = atoi(entry->d_name);
+ if (fd > min_fd)
+ set_cloexec(fd);
}
+
+ closedir(dir);
+
+ return;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
From 307e28b4cb08b05bc044482058eeebc9f59bb9a9 Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Tue, 29 May 2018 18:09:27 -0700
Subject: [PATCH 3/3] rpmSetCloseOnExec: use getrlimit()

In case /proc is not available to get the actual list of opened fds,
we fall back to iterating through the list of all possible fds.

It is possible that during the course of the program execution the limit
on number of open file descriptors might be lowered, so using the
current limit, as returned by sysconf(_SC_OPEN_MAX), might omit some
fds. Therefore, it is better to use rlim_max from the structure
filled in by gertlimit(RLIMIT_NOFILE) to make sure we're checking
all fds.

This slows down the function, but only in the case /proc is not
available, which should be rare in practice.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Upstream-Status: Accepted [https://github.com/rpm-software-management/rpm/pull/444]
Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---
rpmio/rpmio.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/rpmio/rpmio.c b/rpmio/rpmio.c
index 55351c221..e051c9863 100644
--- a/rpmio/rpmio.c
+++ b/rpmio/rpmio.c
@@ -10,6 +10,7 @@
#include <sys/personality.h>
#endif
#include <sys/utsname.h>
+#include <sys/resource.h>

#include <rpm/rpmlog.h>
#include <rpm/rpmmacro.h>
@@ -1778,7 +1779,14 @@ void rpmSetCloseOnExec(void)
DIR *dir = opendir("/proc/self/fd");
if (dir == NULL) { /* /proc not available */
/* iterate over all possible fds, might be slow */
- int open_max = sysconf(_SC_OPEN_MAX);
+ struct rlimit rl;
+ int open_max;
+
+ if (getrlimit(RLIMIT_NOFILE, &rl) == 0 && rl.rlim_max != RLIM_INFINITY)
+ open_max = rl.rlim_max;
+ else
+ open_max = sysconf(_SC_OPEN_MAX);
+
if (open_max == -1)
open_max = 1024;

3 changes: 3 additions & 0 deletions meta/recipes-devtools/rpm/rpm_4.14.1.bb
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ SRC_URI = "git://github.com/rpm-software-management/rpm;branch=rpm-4.14.x \
file://0004-build-pack.c-remove-static-local-variables-from-buil.patch \
file://0001-perl-disable-auto-reqs.patch \
file://0001-configure.ac-add-option-for-dbus.patch \
file://0001-Factor-out-and-unify-setting-CLOEXEC.patch \
file://0002-Optimize-rpmSetCloseOnExec.patch \
file://0003-rpmSetCloseOnExec-use-getrlimit.patch \
"

PE = "1"
Expand Down

0 comments on commit 4b6ff20

Please sign in to comment.