rpc: Temporarily stop accept()-ing new clients on EMFILE

This commit is related to 5de203f879 which I pushed a few days
ago. While that commit prioritized closing clients socket over
the rest of I/O process, this one goes one step further and
temporarily suspends processing new connection requests.

A brief recapitulation of the problem:

1) assume that libvirt is at the top of RLIMIT_NOFILE (that is no
   new FDs can be opened).

2) we have a client trying to connect to a UNIX/TCP socket

Because of 2) our event loop sees POLLIN on the socket and thus
calls virNetServerServiceAccept(). But since no new FDs can be
opened (because of 1)) the request is not handled and we will get
the same event on next iteration. The poll() will exit
immediately because there is an event on the socket.  Thus we end
up in an endless loop.

To break the loop and stop burning CPU cycles we can stop
listening for events on the socket and set up a timer tho enable
listening again after some time (I chose 5 seconds because of no
obvious reason).

There's another area where we play with temporarily suspending
accept() of new clients - when a client disconnects and we check
max_clients against number of current clients. Problem here is
that max_clients can be orders of magnitude larger than
RLIMIT_NOFILE but more importantly, what this code considers
client disconnect is not equal to closing client's FD.
A client disconnecting means that the corresponding client
structure is removed from the internal list of clients. Closing
of the client's FD is done from event loop - asynchronously.

To avoid this part stepping on the toes of my fix, let's make the
code NOP if socket timer (as described above) is active.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index b4265adf2ee2722b4f8d33d2e130e38ac924a951..942e1013a67efa7a7e0e29cd51ff8e4ce5513524 100644 (file)
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -221,6 +221,7 @@ virNetServerServiceNewTCP;
 virNetServerServiceNewUNIX;
 virNetServerServicePreExecRestart;
 virNetServerServiceSetDispatcher;
+virNetServerServiceTimerActive;
 virNetServerServiceToggle;
 
 

diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index b3214883ee77a718b11df2eaa1d31ea13b929fad..dc8f32b095d707d730f8b354b5e80ad4d0876800 100644 (file)
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -252,6 +252,15 @@ virNetServerDispatchNewMessage(virNetServerClient *client,
 static void
 virNetServerCheckLimits(virNetServer *srv)
 {
+    size_t i;
+
+    for (i = 0; i < srv->nservices; i++) {
+        if (virNetServerServiceTimerActive(srv->services[i])) {
+            VIR_DEBUG("Skipping client-related limits evaluation");
+            return;
+        }
+    }
+
     VIR_DEBUG("Checking client-related limits to re-enable or temporarily "
               "suspend services: nclients=%zu nclients_max=%zu "
               "nclients_unauth=%zu nclients_unauth_max=%zu",

diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
index 0c4c437a49cf85485e1db5d72de2f4f9eff7edfb..214eae1acbcbf500e0b0c61b9a594df326e62aa4 100644 (file)
--- a/src/rpc/virnetserverservice.c
+++ b/src/rpc/virnetserverservice.c
@@ -43,6 +43,8 @@ struct _virNetServerService {
     int auth;
     bool readonly;
     size_t nrequests_client_max;
+    int timer;
+    bool timerActive;
 
     virNetTLSContext *tls;
 
@@ -71,9 +73,25 @@ static void virNetServerServiceAccept(virNetSocket *sock,
 {
     virNetServerService *svc = opaque;
     virNetSocket *clientsock = NULL;
-
-    if (virNetSocketAccept(sock, &clientsock) < 0)
+    int rc;
+
+    rc = virNetSocketAccept(sock, &clientsock);
+    if (rc < 0) {
+        if (rc == -2) {
+            /* Could not accept new client due to EMFILE. Suspend listening on
+             * the socket and set up a timer to enable it later. Hopefully,
+             * some FDs will be closed meanwhile. */
+            VIR_DEBUG("Temporarily suspending listening on svc=%p because accept() on sock=%p failed (errno=%d)",
+                      svc, sock, errno);
+
+            virNetServerServiceToggle(svc, false);
+
+            svc->timerActive = true;
+            /* Retry in 5 seconds. */
+            virEventUpdateTimeout(svc->timer, 5 * 1000);
+        }
         goto cleanup;
+    }
 
     if (!clientsock) /* Connection already went away */
         goto cleanup;
@@ -88,6 +106,21 @@ static void virNetServerServiceAccept(virNetSocket *sock,
 }
 
 
+static void
+virNetServerServiceTimerFunc(int timer,
+                             void *opaque)
+{
+    virNetServerService *svc = opaque;
+
+    VIR_DEBUG("Resuming listening on service svc=%p after previous suspend", svc);
+
+    virNetServerServiceToggle(svc, true);
+
+    virEventUpdateTimeout(timer, -1);
+    svc->timerActive = false;
+}
+
+
 static virNetServerService *
 virNetServerServiceNewSocket(virNetSocket **socks,
                              size_t nsocks,
@@ -117,6 +150,14 @@ virNetServerServiceNewSocket(virNetSocket **socks,
     svc->nrequests_client_max = nrequests_client_max;
     svc->tls = virObjectRef(tls);
 
+    virObjectRef(svc);
+    svc->timer = virEventAddTimeout(-1, virNetServerServiceTimerFunc,
+                                    svc, virObjectFreeCallback);
+    if (svc->timer < 0) {
+        virObjectUnref(svc);
+        goto error;
+    }
+
     for (i = 0; i < svc->nsocks; i++) {
         if (virNetSocketListen(svc->socks[i], max_queued_clients) < 0)
             goto error;
@@ -407,6 +448,9 @@ void virNetServerServiceDispose(void *obj)
     virNetServerService *svc = obj;
     size_t i;
 
+    if (svc->timer >= 0)
+        virEventRemoveTimeout(svc->timer);
+
     for (i = 0; i < svc->nsocks; i++)
        virObjectUnref(svc->socks[i]);
     g_free(svc->socks);
@@ -438,3 +482,10 @@ void virNetServerServiceClose(virNetServerService *svc)
         virNetSocketClose(svc->socks[i]);
     }
 }
+
+
+bool
+virNetServerServiceTimerActive(virNetServerService *svc)
+{
+    return svc->timerActive;
+}

diff --git a/src/rpc/virnetserverservice.h b/src/rpc/virnetserverservice.h
index ab5798938edd93403e073bf42705881a08258423..f3d55a9cc06865f6e44f89f03c32c8e33e4e8bc9 100644 (file)
--- a/src/rpc/virnetserverservice.h
+++ b/src/rpc/virnetserverservice.h
@@ -78,3 +78,5 @@ void virNetServerServiceToggle(virNetServerService *svc,
                                bool enabled);
 
 void virNetServerServiceClose(virNetServerService *svc);
+
+bool virNetServerServiceTimerActive(virNetServerService *svc);

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index bf931e5f598eb2c4af730634afa827c387f5db32..943406cd6489c252c18fffc6aef49951ffd76693 100644 (file)
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -2049,6 +2049,19 @@ int virNetSocketListen(virNetSocket *sock, int backlog)
     return 0;
 }
 
+
+/**
+ * virNetSocketAccept:
+ * @sock: socket to accept connection on
+ * @clientsock: returned client socket
+ *
+ * For given socket @sock accept incoming connection and create
+ * @clientsock representation of the new accepted connection.
+ *
+ * Returns: 0 on success,
+ *         -2 if accepting failed due to EMFILE error,
+ *         -1 otherwise.
+ */
 int virNetSocketAccept(virNetSocket *sock, virNetSocket **clientsock)
 {
     int fd = -1;
@@ -2069,6 +2082,8 @@ int virNetSocketAccept(virNetSocket *sock, virNetSocket **clientsock)
             errno == EAGAIN) {
             ret = 0;
             goto cleanup;
+        } else if (errno == EMFILE) {
+            ret = -2;
         }
 
         virReportSystemError(errno, "%s",