Optimize and unite setting CLOEXEC on fds

[kolyshkin]

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 for all those file descriptors that might be open, resulting in severe increase of time it takes to execute rpm or dnf. For example, this happens while running under Docker because:

$ docker run fedora ulimit -n
1048576

One obvious fix is to use procfs to get the actual list of opened file descriptors and iterate over those.

My quick-n-dirty benchmark shows the /proc approach is about 10x faster than iterating through a list of 1024 fds.

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

While at it, unite the two implementations, and future(1) proof it by making sure we modify the existing flags.

(1) - currently the only known flag is FD_CLOEXEC.

This should fix:

• Slow performance when installing RPMs to create new Docker images moby/moby#23137
• https://bugzilla.redhat.com/show_bug.cgi?id=1537564

[cgwalters]

glib code here: https://gitlab.gnome.org/GNOME/glib/blob/487b1fd20c5e494366a82ddc0fa6b53b8bd779ad/glib/gspawn.c#L1207

Also worth looking at what systemd does.

[cgwalters]

At a quick glance anyways, your code looks fine to me.

[Saur2000]

You need to do the same change for doScriptExec() in lib/rpmscript.c as well. At least in our case, that is where all the time is spent.

[kolyshkin]

Yes, ideally those two places need to be unified

[Saur2000]

Change "flag" to "flags". Even if FD_CLOEXEC is currently the only defined flag, we may as well make the code forward compatible,

[Saur2000]

Change "FD_CLOEXEC" to "flags | FD_CLOEXEC".

[kolyshkin]

This I just copied from the old code (and the only known flag for now is CLOEXEC).

Anyway, will do.

[Saur2000]

I see no reason to goto fallback here.

[Saur2000]

I doubt you need to be this conservative. I don't have any measurements to support it, but I'd be very surprised if it is not a win to remove this if statement and always use the code that only traverses the open file descriptors.

[kolyshkin]

I wanted to be conservative for the sole reason to minimize the change to current behavior. What you say makes sense though, let me update the patch.

[kolyshkin]

The CI failure on rawhide is unrelated, it's a dnf error complaining about repo metadata:

raise ValueError("The supplied metadata version isn't supported")

[kolyshkin]

I did some more benchmarks comparing the new /proc way with the old one. It seems that the old one is faster when open_max < 60 which should almost never be the case. Based on that and the suggestion from @Saur2000 I'm dropping the conditional switch to old method completely.

[kolyshkin]

For the reference, here's my quick-n-dirty benchmarking code:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/types.h>
#include <dirent.h>



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);
}

static void set_cloexec_all(int open_max, char method)
{
        const int min_fd = 3; // don't touch stdin/out/err
        int fd;

        if (method == 'i')
		goto fallback;

	// iterate over fds obtained from /proc
        DIR *dir = opendir("/proc/self/fd");
        if (dir == NULL) {
                goto fallback;
        }

        struct dirent *entry;
        while ((entry = readdir(dir)) != NULL) {
                fd = atoi(entry->d_name);
                if (fd >= min_fd)
                        set_cloexec(fd);
        }
        closedir(dir);

        return;

fallback:
        // iterate over all possible fds
        for (fd = min_fd; fd < open_max; fd++)
                set_cloexec(fd);
}

int main(int argc, char **argv) {
	if (argc < 4) {
		fprintf(stderr, "Usage: %s <open_max> <iterations> <method>\n", argv[0]);
		fprintf(stderr, "  <method> is either i (iterate) or p (use proc)\n");
		return 1;
	}

	int open_max = atoi(argv[1]);
	int iter = atoi(argv[2]);
	char m = argv[3][0];
	
	while (iter--)
		set_cloexec_all(open_max, m);

	return 0;
}

[kolyshkin]

kir@kd:~/git/rpm$ time ./a 64 1000000 p

real	0m3.413s
user	0m0.416s
sys	0m2.994s
kir@kd:~/git/rpm$ time ./a 64 1000000 i

real	0m3.661s
user	0m1.421s
sys	0m2.233s

[kolyshkin]

OK, patch updated to take all the review comments into account (thanks @Saur2000 for your suggestions) so it's now also fixing the doScriptExec() from lib/rpmscript.c.

[kolyshkin]

@cgwalters I took a look at glib implementation, they make use of fdwalk() if available (it's a function unique to SunOS/Solaris AFAICS), and reimplement it using /proc/self/fd if not (which is the case for Linux). This seems to be an unnecessary complication (unless we care much about Solaris).

One other thing is, they call getrlimit(RLIMIT_NOFILE, &rl) and use rl.rlim_max which seems to be a mistake -- rlim_cur should be used. But since this code is only used when /proc is not available it's probably nothing.

[kolyshkin]

One other thing is, they call getrlimit(RLIMIT_NOFILE, &rl) and use rl.rlim_max which seems to be a mistake -- rlim_cur should be used.

Filed https://bugzilla.gnome.org/show_bug.cgi?id=796227

[kolyshkin]

Filed https://bugzilla.gnome.org/show_bug.cgi?id=796227

This one is bad, as ulimit might have been changed during the runtime of a process, and so using rlim_max is more correct and safe. For the same reason I have added the second commit aa6cc04 to this PR.

[Saur2000]

Looks good to me. Thanks for the work.

[kolyshkin]

@pmatilai @ffesti PTAL

[pmatilai]

Back from vacation...

This (using /proc/self/fd when available) was what I had in mind, so we're on the right track here, thanks for the work so far. Some quick remarks == requests:

• rpm comment style is "/* foo */", "// foo" should not be used
• just slap the new cloexec function to end of rpmio/rpmio.c (it's quite well at home there, rpmio.c has all/most headers already and this really doesn't need a separate file) and put the prototype to rpmio_internal.h, this is not an interface we want to export
• the jump to fallback label seems klunky and unnecessary, just handle the respective cases in the if-else directly

[ffesti]

The patch set would be even nicer if moving the code into its own function would be separated from any changes to the code.

[kolyshkin]

@ffesti @pmatilai Thank you for your input; please see the updated (and rebased) patch set.

rpm comment style is "/* foo */", "// foo" should not be used

I actually thought about it so I did git grep '// ' which revealed quite a few uses and so I went ahead.
Anyway, done.

and put the prototype to rpmio_internal.h

I'm assuming you are OK with luaext/lposix.c to have #include "rpmio/rpmio_internal.h" added.

The patch set would be even nicer if moving the code into its own function would be separated from any changes to the code.

done

[Saur2000]

I cannot find a way to leave review comments for the commit messages using GitHub, but you should change "say rpm or dnf" to "rpm or dnf" (or "e.g. rpm or dnf") in the "Optimize rpmSetCloseOnExec" commit. You should also change "when running with e.g. under Docker" to "when e.g. running under Docker" in the same commit.

[Saur2000]

Was this intentional? It seems unrelated.

[Saur2000]

Remove.

[ffesti]

Ok, pushed with some minor tweaks like unchanging configure.ac and adding a missing include line.