1. Patchew
  2. Libvirt
  3. View series

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

Daniel P. Berrangé posted 1 patch 4 years, 2 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200207160351.2832800-1-berrange@redhat.com
src/util/vircommand.c | 60 ++++++++++++++++++++++++++++++++++++++++---
tests/testutils.c     |  9 +++++++
2 files changed, 66 insertions(+), 3 deletions(-)
[libvirt PATCH] src: use closefrom() for mass closing of FDs
Posted by Daniel P. Berrangé 4 years, 2 months ago 
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@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);
+
+    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)
 {
+# ifndef __FreeBSD__
     size_t i;
     int open_max;
+# endif /* ! __FreeBSD__ */
     int stdinfd = -1;
     const char *const env[] = {
         "LANG=C",
@@ -344,6 +346,7 @@ void virTestCaptureProgramExecChild(const char *const argv[],
     if ((stdinfd = open("/dev/null", O_RDONLY)) < 0)
         goto cleanup;
 
+# ifndef __FreeBSD__
     open_max = sysconf(_SC_OPEN_MAX);
     if (open_max < 0)
         goto cleanup;
@@ -356,6 +359,7 @@ void virTestCaptureProgramExecChild(const char *const argv[],
             VIR_FORCE_CLOSE(tmpfd);
         }
     }
+# endif /* __FreeBSD__ */
 
     if (dup2(stdinfd, STDIN_FILENO) != STDIN_FILENO)
         goto cleanup;
@@ -364,6 +368,11 @@ void virTestCaptureProgramExecChild(const char *const argv[],
     if (dup2(pipefd, STDERR_FILENO) != STDERR_FILENO)
         goto cleanup;
 
+# ifdef __FreeBSD__
+    closefrom(STDERR_FILENO);
+    stdinfd = pipefd = -1;
+# endif
+
     /* SUS is crazy here, hence the cast */
     execve(argv[0], (char *const*)argv, (char *const*)env);
 
-- 
2.24.1
Re: [libvirt PATCH] src: use closefrom() for mass closing of FDs
Posted by Ján Tomko 4 years, 2 months ago 
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@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@redhat.com>

Jano

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.