5:53 a.m.
This series slightly refactors the code in virnetserver.c in order to resolve
https://bugzilla.redhat.com/show_bug.cgi?id=1357776. The problem resides in
updating of the client limits which even though updated, thus "permitting" new
connections to be established, do not re-enable polling for the socket file
descriptor which results in all new connections still queueing on the socket.
Erik Skultety (5):
rpc: virnetserver: Rename ClientSetProcessingControls to
ClientSetLimits
rpc: virnetserver: Move virNetServerCheckLimits which is static up in
the file
rpc: virnetserver: Add code to CheckLimits to handle suspending of
services
admin: rpc: virnetserver: Fix updating of the client limits
rpc: virnetserver: Remove dead code checking the client limits
daemon/admin_server.c | 4 +--
src/rpc/virnetserver.c | 97 +++++++++++++++++++++++---------------------------
src/rpc/virnetserver.h | 6 ++--
3 files changed, 50 insertions(+), 57 deletions(-)
--
2.5.5
5:53 a.m.
New subject: [libvirt] [PATCH 1/5] rpc: virnetserver: Rename ClientSetProcessingControls to ClientSetLimits
The original naming was just a leftover that should have been fixed in commit
8b1f0469.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
daemon/admin_server.c | 4 ++--
src/rpc/virnetserver.c | 6 +++---
src/rpc/virnetserver.h | 6 +++---
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/daemon/admin_server.c b/daemon/admin_server.c
index 9f24f68..25bfc7b 100644
--- a/daemon/admin_server.c
+++ b/daemon/admin_server.c
@@ -382,8 +382,8 @@ adminServerSetClientLimits(virNetServerPtr srv,
VIR_SERVER_CLIENTS_UNAUTH_MAX)))
maxClientsUnauth = param->value.ui;
- if (virNetServerSetClientProcessingControls(srv, maxClients,
- maxClientsUnauth) < 0)
+ if (virNetServerSetClientLimits(srv, maxClients,
+ maxClientsUnauth) < 0)
return -1;
return 0;
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 8c8af97..13b5bd3 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -1046,9 +1046,9 @@ virNetServerGetClient(virNetServerPtr srv,
}
int
-virNetServerSetClientProcessingControls(virNetServerPtr srv,
- long long int maxClients,
- long long int maxClientsUnauth)
+virNetServerSetClientLimits(virNetServerPtr srv,
+ long long int maxClients,
+ long long int maxClientsUnauth)
{
int ret = -1;
size_t max, max_unauth;
diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index 38107b4..151bac8 100644
--- a/src/rpc/virnetserver.h
+++ b/src/rpc/virnetserver.h
@@ -116,8 +116,8 @@ size_t virNetServerGetCurrentClients(virNetServerPtr srv);
size_t virNetServerGetMaxUnauthClients(virNetServerPtr srv);
size_t virNetServerGetCurrentUnauthClients(virNetServerPtr srv);
-int virNetServerSetClientProcessingControls(virNetServerPtr srv,
- long long int maxClients,
- long long int maxClientsUnauth);
+int virNetServerSetClientLimits(virNetServerPtr srv,
+ long long int maxClients,
+ long long int maxClientsUnauth);
#endif /* __VIR_NET_SERVER_H__ */
--
2.5.5
5:53 a.m.
New subject: [libvirt] [PATCH 2/5] rpc: virnetserver: Move virNetServerCheckLimits which is static up in the file
Since virNetServerAddClient checks for the limits in order to temporarily
suspend the services, thus not accepting any more clients, there is no reason
why virNetServerCheckLimits, which is only responsible for re-enabling
previously disabled services according to the limits, could not do both. To be
able to do that however, it needs to be moved up in the file since it's static
(and because it's just a helper and there's only one caller it should remain
static).
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/rpc/virnetserver.c | 57 +++++++++++++++++++++++++-------------------------
1 file changed, 28 insertions(+), 29 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 13b5bd3..0c502d9 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -234,6 +234,34 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr
client,
return ret;
}
+/**
+ * virNetServerCheckLimits:
+ * @srv: server to check limits on
+ *
+ * Check if limits like max_clients or max_anonymous_clients
+ * are satisfied and if so, re-enable accepting new clients.
+ * The @srv must be locked when this function is called.
+ */
+static void
+virNetServerCheckLimits(virNetServerPtr srv)
+{
+ /* Enable services if we can accept a new client.
+ * The new client can be accepted if both max_clients and
+ * max_anonymous_clients wouldn't get overcommitted by
+ * accepting it. */
+ VIR_DEBUG("Considering re-enabling services: "
+ "nclients=%zu nclients_max=%zu "
+ "nclients_unauth=%zu nclients_unauth_max=%zu",
+ srv->nclients, srv->nclients_max,
+ srv->nclients_unauth, srv->nclients_unauth_max);
+ if (srv->nclients < srv->nclients_max &&
+ (!srv->nclients_unauth_max ||
+ srv->nclients_unauth < srv->nclients_unauth_max)) {
+ /* Now it makes sense to accept() a new client. */
+ VIR_INFO("Re-enabling services");
+ virNetServerUpdateServicesLocked(srv, true);
+ }
+}
int virNetServerAddClient(virNetServerPtr srv,
virNetServerClientPtr client)
@@ -737,35 +765,6 @@ void virNetServerUpdateServices(virNetServerPtr srv,
virObjectUnlock(srv);
}
-/**
- * virNetServerCheckLimits:
- * @srv: server to check limits on
- *
- * Check if limits like max_clients or max_anonymous_clients
- * are satisfied and if so, re-enable accepting new clients.
- * The @srv must be locked when this function is called.
- */
-static void
-virNetServerCheckLimits(virNetServerPtr srv)
-{
- /* Enable services if we can accept a new client.
- * The new client can be accepted if both max_clients and
- * max_anonymous_clients wouldn't get overcommitted by
- * accepting it. */
- VIR_DEBUG("Considering re-enabling services: "
- "nclients=%zu nclients_max=%zu "
- "nclients_unauth=%zu nclients_unauth_max=%zu",
- srv->nclients, srv->nclients_max,
- srv->nclients_unauth, srv->nclients_unauth_max);
- if (srv->nclients < srv->nclients_max &&
- (!srv->nclients_unauth_max ||
- srv->nclients_unauth < srv->nclients_unauth_max)) {
- /* Now it makes sense to accept() a new client. */
- VIR_INFO("Re-enabling services");
- virNetServerUpdateServicesLocked(srv, true);
- }
-}
-
void virNetServerDispose(void *obj)
{
virNetServerPtr srv = obj;
--
2.5.5
5:53 a.m.
New subject: [libvirt] [PATCH 3/5] rpc: virnetserver: Add code to CheckLimits to handle suspending of services
So far, virNetServerCheckLimits was only used to possibly re-enable accepting
new clients that might have previously by disabled due to client limits
violation (max_clients, max_anonymous_clients). This patch refactors
virNetServerAddClient which is currently the only place where the services get
disabled in order to use the virNetServerCheckLimits helper instead of checking
the limits by itself.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/rpc/virnetserver.c | 45 ++++++++++++++++++++++-----------------------
1 file changed, 22 insertions(+), 23 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 0c502d9..c5caef3 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -239,24 +239,35 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr
client,
* @srv: server to check limits on
*
* Check if limits like max_clients or max_anonymous_clients
- * are satisfied and if so, re-enable accepting new clients.
+ * are satisfied. If so, re-enable accepting new clients. If these are violated
+ * however, temporarily disable accepting new clients.
* The @srv must be locked when this function is called.
*/
static void
virNetServerCheckLimits(virNetServerPtr srv)
{
- /* Enable services if we can accept a new client.
- * The new client can be accepted if both max_clients and
- * max_anonymous_clients wouldn't get overcommitted by
- * accepting it. */
- VIR_DEBUG("Considering re-enabling services: "
- "nclients=%zu nclients_max=%zu "
+ 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",
srv->nclients, srv->nclients_max,
srv->nclients_unauth, srv->nclients_unauth_max);
- if (srv->nclients < srv->nclients_max &&
- (!srv->nclients_unauth_max ||
- srv->nclients_unauth < srv->nclients_unauth_max)) {
+
+ /* Check the max_anonymous_clients and max_clients limits so that we can
+ * decide whether the services should be temporarily suspended, thus not
+ * accepting any more clients for a while or re-enabling the previously
+ * suspended services in order to accept new clients again.
+ * A new client can only be accepted if both max_clients and
+ * max_anonymous_clients wouldn't get overcommitted by accepting it.
+ */
+ if (srv->nclients >= srv->nclients_max ||
+ (srv->nclients_unauth_max &&
+ srv->nclients_unauth >= srv->nclients_unauth_max)) {
+ /* Temporarily stop accepting new clients */
+ VIR_INFO("Temporarily suspending services");
+ virNetServerUpdateServicesLocked(srv, false);
+ } else if (srv->nclients < srv->nclients_max &&
+ (!srv->nclients_unauth_max ||
+ srv->nclients_unauth < srv->nclients_unauth_max)) {
/* Now it makes sense to accept() a new client. */
VIR_INFO("Re-enabling services");
virNetServerUpdateServicesLocked(srv, true);
@@ -286,19 +297,7 @@ int virNetServerAddClient(virNetServerPtr srv,
if (virNetServerClientNeedAuth(client))
virNetServerTrackPendingAuthLocked(srv);
- if (srv->nclients_unauth_max &&
- srv->nclients_unauth == srv->nclients_unauth_max) {
- /* Temporarily stop accepting new clients */
- VIR_INFO("Temporarily suspending services "
- "due to max_anonymous_clients");
- virNetServerUpdateServicesLocked(srv, false);
- }
-
- if (srv->nclients == srv->nclients_max) {
- /* Temporarily stop accepting new clients */
- VIR_INFO("Temporarily suspending services due to max_clients");
- virNetServerUpdateServicesLocked(srv, false);
- }
+ virNetServerCheckLimits(srv);
virNetServerClientSetDispatcher(client,
virNetServerDispatchNewMessage,
--
2.5.5
5:53 a.m.
New subject: [libvirt] [PATCH 4/5] admin: rpc: virnetserver: Fix updating of the client limits
Commit 2737aaaf changed our policy for accepting new clients in a way, that
instead of accepting new clients only to disconnect them immediately because
that would overcommit the limit we temporarily disable polling for the
dedicated file descriptor, so any new connection will queue on the socket.
Commit 8b1f0469 then added the possibility to change the limits during runtime
but it didn't re-enable polling for the previously disabled file descriptor,
thus any new connection would still continue to queue on the socket. This patch
forces an update of the services each time the limits were changed in some way.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1357776
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/rpc/virnetserver.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index c5caef3..5b6bc4a 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -1071,6 +1071,8 @@ virNetServerSetClientLimits(virNetServerPtr srv,
if (maxClientsUnauth >= 0)
srv->nclients_unauth_max = maxClientsUnauth;
+ virNetServerCheckLimits(srv);
+
ret = 0;
cleanup:
virObjectUnlock(srv);
--
2.5.5
5:53 a.m.
New subject: [libvirt] [PATCH 5/5] rpc: virnetserver: Remove dead code checking the client limits
Prior to commit 2737aaaf, we allowed every client to connect successfully,
however, if accepting the client would eventually lead to an overcommit of the
limits, we would disconnect it immediately with "Too many active clients,
dropping connection from...". Commits 4d693241 and e34fbb9e refactored
the code in a way, that it is not possible for the client-related callback to
be dispatched and the client to be accepted if the limits would permit to do
so, therefore a check if a connection should be dropped due to limits violation
has become a dead code that could be removed.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/rpc/virnetserver.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 5b6bc4a..f06643a 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -279,13 +279,6 @@ int virNetServerAddClient(virNetServerPtr srv,
{
virObjectLock(srv);
- if (srv->nclients >= srv->nclients_max) {
- virReportError(VIR_ERR_RPC,
- _("Too many active clients (%zu), dropping connection from
%s"),
- srv->nclients_max,
virNetServerClientRemoteAddrStringURI(client));
- goto error;
- }
-
if (virNetServerClientInit(client) < 0)
goto error;
--
2.5.5
8:44 a.m.
New subject: [libvirt] [PATCH 5/5] rpc: virnetserver: Remove dead code checking the client limits
On 07/20/2016 06:53 AM, Erik Skultety wrote:
Prior to commit 2737aaaf, we allowed every client to connect
successfully,
however, if accepting the client would eventually lead to an overcommit of the
limits, we would disconnect it immediately with "Too many active clients,
dropping connection from...". Commits 4d693241 and e34fbb9e refactored
Are these two commit id's from your local branch? EG, the previous two
commits? I assume so, since they weren't found... Perhaps change the
text to indicate "recent adjustments" or push the first four, get their
upsteam commit id's and then replace these two with the "real" ones.
John
the code in a way, that it is not possible for the client-related
callback to
be dispatched and the client to be accepted if the limits would permit to do
so, therefore a check if a connection should be dropped due to limits violation
has become a dead code that could be removed.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/rpc/virnetserver.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 5b6bc4a..f06643a 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -279,13 +279,6 @@ int virNetServerAddClient(virNetServerPtr srv,
{
virObjectLock(srv);
- if (srv->nclients >= srv->nclients_max) {
- virReportError(VIR_ERR_RPC,
- _("Too many active clients (%zu), dropping connection from
%s"),
- srv->nclients_max,
virNetServerClientRemoteAddrStringURI(client));
- goto error;
- }
-
if (virNetServerClientInit(client) < 0)
goto error;
8:44 a.m.
On 07/20/2016 06:53 AM, Erik Skultety wrote:
This series slightly refactors the code in virnetserver.c in order to
resolve
https://bugzilla.redhat.com/show_bug.cgi?id=1357776. The problem resides in
updating of the client limits which even though updated, thus "permitting" new
connections to be established, do not re-enable polling for the socket file
descriptor which results in all new connections still queueing on the socket.
Erik Skultety (5):
rpc: virnetserver: Rename ClientSetProcessingControls to
ClientSetLimits
rpc: virnetserver: Move virNetServerCheckLimits which is static up in
the file
rpc: virnetserver: Add code to CheckLimits to handle suspending of
services
admin: rpc: virnetserver: Fix updating of the client limits
rpc: virnetserver: Remove dead code checking the client limits
daemon/admin_server.c | 4 +--
src/rpc/virnetserver.c | 97 +++++++++++++++++++++++---------------------------
src/rpc/virnetserver.h | 6 ++--
3 files changed, 50 insertions(+), 57 deletions(-)
ACK series, but consider my note in 5/5
John
8:02 a.m.
On 30/07/16 15:44, John Ferlan wrote:
On 07/20/2016 06:53 AM, Erik Skultety wrote:
> This series slightly refactors the code in virnetserver.c in order to resolve
> https://bugzilla.redhat.com/show_bug.cgi?id=1357776. The problem resides in
> updating of the client limits which even though updated, thus "permitting"
new
> connections to be established, do not re-enable polling for the socket file
> descriptor which results in all new connections still queueing on the socket.
>
> Erik Skultety (5):
> rpc: virnetserver: Rename ClientSetProcessingControls to
> ClientSetLimits
> rpc: virnetserver: Move virNetServerCheckLimits which is static up in
> the file
> rpc: virnetserver: Add code to CheckLimits to handle suspending of
> services
> admin: rpc: virnetserver: Fix updating of the client limits
> rpc: virnetserver: Remove dead code checking the client limits
>
> daemon/admin_server.c | 4 +--
> src/rpc/virnetserver.c | 97 +++++++++++++++++++++++---------------------------
> src/rpc/virnetserver.h | 6 ++--
> 3 files changed, 50 insertions(+), 57 deletions(-)
>
ACK series, but consider my note in 5/5
Adjusted and pushed, thanks.
Erik
3064
days inactive
3077
days old
8 comments
2 participants
participants (2)
-
Erik Skultety -
John Ferlan