[libvirt PATCH] src: use closefrom() for mass closing of FDs

Ján Tomko jtomko at redhat.com
Sun Feb 9 01:37:43 UTC 2020


On Fri, Feb 07, 2020 at 04:03:51PM +0000, Daniel P. Berrangé wrote:
>On FreeBSD 12 the default ulimit settings allow for 100,000
>open file descriptors. As a result spawning processes in
>libvirt is abominably slow. Fortunately FreeBSD has long
>since provided a good solution in the form of closefrom(),
>which closes all FDs equal to or larger than the specified
>parameter.
>
>Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
>---
> src/util/vircommand.c | 60 ++++++++++++++++++++++++++++++++++++++++---
> tests/testutils.c     |  9 +++++++
> 2 files changed, 66 insertions(+), 3 deletions(-)
>
>diff --git a/src/util/vircommand.c b/src/util/vircommand.c
>index 904a3023c5..764fb2fe43 100644
>--- a/src/util/vircommand.c
>+++ b/src/util/vircommand.c
>@@ -494,6 +494,59 @@ virCommandMassCloseGetFDsGeneric(virCommandPtr cmd G_GNUC_UNUSED,
> }
> # endif /* !__linux__ */
>
>+# ifdef __FreeBSD__
>+
>+static int
>+virCommandMassClose(virCommandPtr cmd,
>+                    int childin,
>+                    int childout,
>+                    int childerr)
>+{
>+    int lastfd = -1;
>+    int fd = -1;
>+
>+    /*
>+     * Two phases of closing.
>+     *
>+     * The first (inefficient) phase iterates over FDs,
>+     * preserving certain FDs we need to pass down, and
>+     * closing others. The number of iterations is bounded
>+     * to the number of the biggest FD we need to preserve.
>+     *
>+     * The second (speedy) phase uses closefrom() to cull
>+     * all remaining FDs in the process.
>+     *
>+     * Usually the first phase will be fairly quick only
>+     * processing a handful of low FD numbers, and thus using
>+     * closefrom() is a massive win for high ulimit() NFILES
>+     * values.
>+     */
>+    lastfd = MAX(lastfd, childin);
>+    lastfd = MAX(lastfd, childout);
>+    lastfd = MAX(lastfd, childerr);
>+
>+    while (fd < cmd->npassfd)
>+        lastfd = MAX(lastfd, cmd->passfd[fd].fd);

This accesses cmd->passfd[-1] and never increments fd.

Please use a for loop with 'i' for readability.

>+
>+    for (fd = 0; fd <= lastfd; fd++) {
>+        if (fd == childin || fd == childout || fd == childerr)
>+            continue;
>+        if (!virCommandFDIsSet(cmd, fd)) {
>+            int tmpfd = fd;
>+            VIR_MASS_CLOSE(tmpfd);
>+        } else if (virSetInherit(fd, true) < 0) {
>+            virReportSystemError(errno, _("failed to preserve fd %d"), fd);
>+            return -1;
>+        }
>+    }
>+
>+    closefrom(lastfd + 1);
>+
>+    return 0;
>+}
>+
>+# else /* ! __FreeBSD__ */
>+
> static int
> virCommandMassClose(virCommandPtr cmd,
>                     int childin,
>@@ -520,13 +573,13 @@ virCommandMassClose(virCommandPtr cmd,
>     if (!(fds = virBitmapNew(openmax)))
>         return -1;
>
>-# ifdef __linux__
>+#  ifdef __linux__
>     if (virCommandMassCloseGetFDsLinux(cmd, fds) < 0)
>         return -1;
>-# else
>+#  else
>     if (virCommandMassCloseGetFDsGeneric(cmd, fds) < 0)
>         return -1;
>-# endif
>+#  endif
>
>     fd = virBitmapNextSetBit(fds, 2);
>     for (; fd >= 0; fd = virBitmapNextSetBit(fds, fd)) {
>@@ -544,6 +597,7 @@ virCommandMassClose(virCommandPtr cmd,
>     return 0;
> }
>
>+# endif /* ! __FreeBSD__ */
>
> /*
>  * virExec:
>diff --git a/tests/testutils.c b/tests/testutils.c
>index 7b9a5ea05b..662203d707 100644
>--- a/tests/testutils.c
>+++ b/tests/testutils.c
>@@ -333,8 +333,10 @@ static
> void virTestCaptureProgramExecChild(const char *const argv[],
>                                     int pipefd)
> {

I think this whole function should be dropped:
https://www.redhat.com/archives/libvir-list/2020-February/msg00361.html

Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200209/32cfcb28/attachment-0001.sig>


More information about the libvir-list mailing list