Changeset
src/locking/lock_daemon.c    |  4 +--
src/logging/log_daemon.c     |  4 +--
src/rpc/virnetserver.c       | 29 ++++++++++------
src/rpc/virnetserverclient.c | 81 ++++++++++++++++++++++++++++++--------------
src/rpc/virnetserverclient.h |  9 +++--
5 files changed, 83 insertions(+), 44 deletions(-)
Git apply log
Switched to a new branch '20180306175852.18773-1-berrange@redhat.com'
Applying: rpc: push ref acquisition into RPC dispatch function
Applying: rpc: simplify calling convention of virNetServerClientDispatchFunc
Applying: rpc: invoke the message dispatch callback with client unlocked
Applying: rpc: avoid crashing in pre-exec if no workers are present
Applying: rpc: switch virtlockd and virtlogd to use single-threaded dispatch
To https://github.com/patchew-project/libvirt
 + 98a1ae9b2...004669e75 patchew/20180306175852.18773-1-berrange@redhat.com -> patchew/20180306175852.18773-1-berrange@redhat.com (forced update)
Test failed: syntax-check

loading

[libvirt] [PATCH 0/5] Fix virtlockd loosing locks
Posted by Daniel P. Berrangé, 15 weeks ago
This is a workaround for the bizarre behaviour whereby the kernel throws
away all fcntl() locks on execve()... if any threads are running. We
simply make virtlockd & virlogd single threaded instead.

Daniel P. Berrangé (5):
  rpc: push ref acquisition into RPC dispatch function
  rpc: simplify calling convention of virNetServerClientDispatchFunc
  rpc: invoke the message dispatch callback with client unlocked
  rpc: avoid crashing in pre-exec if no workers are present
  rpc: switch virtlockd and virtlogd to use single-threaded dispatch

 src/locking/lock_daemon.c    |  4 +--
 src/logging/log_daemon.c     |  4 +--
 src/rpc/virnetserver.c       | 29 ++++++++++------
 src/rpc/virnetserverclient.c | 81 ++++++++++++++++++++++++++++++--------------
 src/rpc/virnetserverclient.h |  9 +++--
 5 files changed, 83 insertions(+), 44 deletions(-)

-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/5] Fix virtlockd loosing locks
Posted by Daniel P. Berrangé, 15 weeks ago
On Tue, Mar 06, 2018 at 05:58:47PM +0000, Daniel P. Berrangé wrote:
> This is a workaround for the bizarre behaviour whereby the kernel throws
> away all fcntl() locks on execve()... if any threads are running. We
> simply make virtlockd & virlogd single threaded instead.
> 
> Daniel P. Berrangé (5):
>   rpc: push ref acquisition into RPC dispatch function
>   rpc: simplify calling convention of virNetServerClientDispatchFunc
>   rpc: invoke the message dispatch callback with client unlocked
>   rpc: avoid crashing in pre-exec if no workers are present
>   rpc: switch virtlockd and virtlogd to use single-threaded dispatch
> 
>  src/locking/lock_daemon.c    |  4 +--
>  src/logging/log_daemon.c     |  4 +--
>  src/rpc/virnetserver.c       | 29 ++++++++++------
>  src/rpc/virnetserverclient.c | 81 ++++++++++++++++++++++++++++++--------------
>  src/rpc/virnetserverclient.h |  9 +++--
>  5 files changed, 83 insertions(+), 44 deletions(-)

Forgot to say that we might want to still take a variety on Jim's pathc
to re-aquire locks after execve() so that existing deployments recover
their state. This patch series only fixes things when virtlockd is cold
started, and RPM upgrades will warm-restart with re-exec


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/5] rpc: push ref acquisition into RPC dispatch function
Posted by Daniel P. Berrangé, 15 weeks ago
There's no reason why the virNetServerClientDispatchRead method needs to
acquire an extra reference on the "client" object. An extra reference is
only needed if the registered dispatch callback is going to keep hold of
the "client" for work in the background. Thus we can push reference
acquisition into virNetServerDispatchNewMessage.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/rpc/virnetserver.c       | 2 ++
 src/rpc/virnetserverclient.c | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 204f425264..28afe54e49 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -217,9 +217,11 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
             priority = virNetServerProgramGetPriority(prog, msg->header.proc);
         }
 
+        virObjectRef(client);
         ret = virThreadPoolSendJob(srv->workers, priority, job);
 
         if (ret < 0) {
+            virObjectUnref(client);
             VIR_FREE(job);
             virObjectUnref(prog);
         }
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 388514946b..00459d17ba 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -1315,12 +1315,10 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
 
         /* Send off to for normal dispatch to workers */
         if (msg) {
-            virObjectRef(client);
             if (!client->dispatchFunc ||
                 client->dispatchFunc(client, msg, client->dispatchOpaque) < 0) {
                 virNetMessageFree(msg);
                 client->wantClose = true;
-                virObjectUnref(client);
                 return;
             }
         }
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] rpc: push ref acquisition into RPC dispatch function
Posted by Jim Fehlig, 15 weeks ago
On 03/06/2018 10:58 AM, Daniel P. Berrangé wrote:
> There's no reason why the virNetServerClientDispatchRead method needs to
> acquire an extra reference on the "client" object. An extra reference is
> only needed if the registered dispatch callback is going to keep hold of
> the "client" for work in the background. Thus we can push reference
> acquisition into virNetServerDispatchNewMessage.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   src/rpc/virnetserver.c       | 2 ++
>   src/rpc/virnetserverclient.c | 2 --
>   2 files changed, 2 insertions(+), 2 deletions(-)

I haven't worked on this code much, but this change seems sane to me regardless 
of the lost locks issue.

Reviewed-by: Jim Fehlig <jfehlig@suse.com>

Regards,
Jim

> 
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 204f425264..28afe54e49 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -217,9 +217,11 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
>               priority = virNetServerProgramGetPriority(prog, msg->header.proc);
>           }
>   
> +        virObjectRef(client);
>           ret = virThreadPoolSendJob(srv->workers, priority, job);
>   
>           if (ret < 0) {
> +            virObjectUnref(client);
>               VIR_FREE(job);
>               virObjectUnref(prog);
>           }
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index 388514946b..00459d17ba 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -1315,12 +1315,10 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
>   
>           /* Send off to for normal dispatch to workers */
>           if (msg) {
> -            virObjectRef(client);
>               if (!client->dispatchFunc ||
>                   client->dispatchFunc(client, msg, client->dispatchOpaque) < 0) {
>                   virNetMessageFree(msg);
>                   client->wantClose = true;
> -                virObjectUnref(client);
>                   return;
>               }
>           }
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] rpc: push ref acquisition into RPC dispatch function
Posted by John Ferlan, 15 weeks ago

On 03/06/2018 12:58 PM, Daniel P. Berrangé wrote:
> There's no reason why the virNetServerClientDispatchRead method needs to
> acquire an extra reference on the "client" object. An extra reference is
> only needed if the registered dispatch callback is going to keep hold of
> the "client" for work in the background. Thus we can push reference
> acquisition into virNetServerDispatchNewMessage.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/rpc/virnetserver.c       | 2 ++
>  src/rpc/virnetserverclient.c | 2 --
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 

Makes sense... Chasing for the Unref shows virNetServerHandleJob which
of course had me wondering for the non dispatch path, when was that
Unref done in virNetServerClientDispatchRead...


Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] rpc: simplify calling convention of virNetServerClientDispatchFunc
Posted by Daniel P. Berrangé, 15 weeks ago
Currently virNetServerClientDispatchFunc implementations are only
responsible for free'ing the "msg" parameter upon success. Simplify the
calling convention by making it their unconditional responsibility to
free the "msg", and close the client if desired.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/rpc/virnetserver.c       | 24 +++++++++++++-----------
 src/rpc/virnetserverclient.c |  6 +++---
 src/rpc/virnetserverclient.h |  9 ++++++---
 3 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 28afe54e49..7a1376bf49 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -182,15 +182,14 @@ static void virNetServerHandleJob(void *jobOpaque, void *opaque)
     VIR_FREE(job);
 }
 
-static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
-                                          virNetMessagePtr msg,
-                                          void *opaque)
+static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
+                                           virNetMessagePtr msg,
+                                           void *opaque)
 {
     virNetServerPtr srv = opaque;
     virNetServerProgramPtr prog = NULL;
     unsigned int priority = 0;
     size_t i;
-    int ret = -1;
 
     VIR_DEBUG("server=%p client=%p message=%p",
               srv, client, msg);
@@ -207,7 +206,7 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
         virNetServerJobPtr job;
 
         if (VIR_ALLOC(job) < 0)
-            goto cleanup;
+            goto error;
 
         job->client = client;
         job->msg = msg;
@@ -218,21 +217,24 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
         }
 
         virObjectRef(client);
-        ret = virThreadPoolSendJob(srv->workers, priority, job);
-
-        if (ret < 0) {
+        if (virThreadPoolSendJob(srv->workers, priority, job) < 0) {
             virObjectUnref(client);
             VIR_FREE(job);
             virObjectUnref(prog);
+            goto error;
         }
     } else {
-        ret = virNetServerProcessMsg(srv, client, prog, msg);
+        if (virNetServerProcessMsg(srv, client, prog, msg) < 0)
+            goto error;
     }
 
- cleanup:
     virObjectUnlock(srv);
+    return;
 
-    return ret;
+ error:
+    virNetMessageFree(msg);
+    virNetServerClientClose(client);
+    virObjectUnlock(srv);
 }
 
 /**
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 00459d17ba..ea0d5abdee 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -1315,11 +1315,11 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
 
         /* Send off to for normal dispatch to workers */
         if (msg) {
-            if (!client->dispatchFunc ||
-                client->dispatchFunc(client, msg, client->dispatchOpaque) < 0) {
+            if (!client->dispatchFunc) {
                 virNetMessageFree(msg);
                 client->wantClose = true;
-                return;
+            } else {
+                client->dispatchFunc(client, msg, client->dispatchOpaque);
             }
         }
 
diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
index 1a939ad4e1..b21446eeb7 100644
--- a/src/rpc/virnetserverclient.h
+++ b/src/rpc/virnetserverclient.h
@@ -36,9 +36,12 @@ typedef virNetServer *virNetServerPtr;
 typedef struct _virNetServerClient virNetServerClient;
 typedef virNetServerClient *virNetServerClientPtr;
 
-typedef int (*virNetServerClientDispatchFunc)(virNetServerClientPtr client,
-                                              virNetMessagePtr msg,
-                                              void *opaque);
+/* This function owns the "msg" pointer it is passed and
+ * must arrange for virNetMessageFree to be called on it
+ */
+typedef void (*virNetServerClientDispatchFunc)(virNetServerClientPtr client,
+                                               virNetMessagePtr msg,
+                                               void *opaque);
 
 typedef int (*virNetServerClientFilterFunc)(virNetServerClientPtr client,
                                             virNetMessagePtr msg,
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/5] rpc: simplify calling convention of virNetServerClientDispatchFunc
Posted by Jim Fehlig, 15 weeks ago
On 03/06/2018 10:58 AM, Daniel P. Berrangé wrote:
> Currently virNetServerClientDispatchFunc implementations are only
> responsible for free'ing the "msg" parameter upon success. Simplify the
> calling convention by making it their unconditional responsibility to
> free the "msg", and close the client if desired.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   src/rpc/virnetserver.c       | 24 +++++++++++++-----------
>   src/rpc/virnetserverclient.c |  6 +++---
>   src/rpc/virnetserverclient.h |  9 ++++++---
>   3 files changed, 22 insertions(+), 17 deletions(-)

Same as 1/1. Looks like a nice improvement regardless of lost locks.

Reviewed-by: Jim Fehlig <jfehlig@suse.com>

Regards,
Jim

> 
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 28afe54e49..7a1376bf49 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -182,15 +182,14 @@ static void virNetServerHandleJob(void *jobOpaque, void *opaque)
>       VIR_FREE(job);
>   }
>   
> -static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
> -                                          virNetMessagePtr msg,
> -                                          void *opaque)
> +static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
> +                                           virNetMessagePtr msg,
> +                                           void *opaque)
>   {
>       virNetServerPtr srv = opaque;
>       virNetServerProgramPtr prog = NULL;
>       unsigned int priority = 0;
>       size_t i;
> -    int ret = -1;
>   
>       VIR_DEBUG("server=%p client=%p message=%p",
>                 srv, client, msg);
> @@ -207,7 +206,7 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
>           virNetServerJobPtr job;
>   
>           if (VIR_ALLOC(job) < 0)
> -            goto cleanup;
> +            goto error;
>   
>           job->client = client;
>           job->msg = msg;
> @@ -218,21 +217,24 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
>           }
>   
>           virObjectRef(client);
> -        ret = virThreadPoolSendJob(srv->workers, priority, job);
> -
> -        if (ret < 0) {
> +        if (virThreadPoolSendJob(srv->workers, priority, job) < 0) {
>               virObjectUnref(client);
>               VIR_FREE(job);
>               virObjectUnref(prog);
> +            goto error;
>           }
>       } else {
> -        ret = virNetServerProcessMsg(srv, client, prog, msg);
> +        if (virNetServerProcessMsg(srv, client, prog, msg) < 0)
> +            goto error;
>       }
>   
> - cleanup:
>       virObjectUnlock(srv);
> +    return;
>   
> -    return ret;
> + error:
> +    virNetMessageFree(msg);
> +    virNetServerClientClose(client);
> +    virObjectUnlock(srv);
>   }
>   
>   /**
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index 00459d17ba..ea0d5abdee 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -1315,11 +1315,11 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
>   
>           /* Send off to for normal dispatch to workers */
>           if (msg) {
> -            if (!client->dispatchFunc ||
> -                client->dispatchFunc(client, msg, client->dispatchOpaque) < 0) {
> +            if (!client->dispatchFunc) {
>                   virNetMessageFree(msg);
>                   client->wantClose = true;
> -                return;
> +            } else {
> +                client->dispatchFunc(client, msg, client->dispatchOpaque);
>               }
>           }
>   
> diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
> index 1a939ad4e1..b21446eeb7 100644
> --- a/src/rpc/virnetserverclient.h
> +++ b/src/rpc/virnetserverclient.h
> @@ -36,9 +36,12 @@ typedef virNetServer *virNetServerPtr;
>   typedef struct _virNetServerClient virNetServerClient;
>   typedef virNetServerClient *virNetServerClientPtr;
>   
> -typedef int (*virNetServerClientDispatchFunc)(virNetServerClientPtr client,
> -                                              virNetMessagePtr msg,
> -                                              void *opaque);
> +/* This function owns the "msg" pointer it is passed and
> + * must arrange for virNetMessageFree to be called on it
> + */
> +typedef void (*virNetServerClientDispatchFunc)(virNetServerClientPtr client,
> +                                               virNetMessagePtr msg,
> +                                               void *opaque);
>   
>   typedef int (*virNetServerClientFilterFunc)(virNetServerClientPtr client,
>                                               virNetMessagePtr msg,
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/5] rpc: simplify calling convention of virNetServerClientDispatchFunc
Posted by John Ferlan, 15 weeks ago

On 03/06/2018 12:58 PM, Daniel P. Berrangé wrote:
> Currently virNetServerClientDispatchFunc implementations are only
> responsible for free'ing the "msg" parameter upon success. Simplify the
> calling convention by making it their unconditional responsibility to
> free the "msg", and close the client if desired.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/rpc/virnetserver.c       | 24 +++++++++++++-----------
>  src/rpc/virnetserverclient.c |  6 +++---
>  src/rpc/virnetserverclient.h |  9 ++++++---
>  3 files changed, 22 insertions(+), 17 deletions(-)
> 

Chasing all the error paths for @msg to be freed is mind boggling ;-)

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/5] rpc: invoke the message dispatch callback with client unlocked
Posted by Daniel P. Berrangé, 15 weeks ago
Currently if the virNetServer instance is created with max_workers==0 to
request a non-threaded dispatch process, we deadlock during dispatch

  #0  0x00007fb845f6f42d in __lll_lock_wait () from /lib64/libpthread.so.0
  #1  0x00007fb845f681d3 in pthread_mutex_lock () from /lib64/libpthread.so.0
  #2  0x000055a6628bb305 in virMutexLock (m=<optimized out>) at util/virthread.c:89
  #3  0x000055a6628a984b in virObjectLock (anyobj=<optimized out>) at util/virobject.c:435
  #4  0x000055a66286fcde in virNetServerClientIsAuthenticated (client=client@entry=0x55a663a7b960)
      at rpc/virnetserverclient.c:1565
  #5  0x000055a66286cc17 in virNetServerProgramDispatchCall (msg=0x55a663a7bc50, client=0x55a663a7b960,
      server=0x55a663a77550, prog=0x55a663a78020) at rpc/virnetserverprogram.c:407
  #6  virNetServerProgramDispatch (prog=prog@entry=0x55a663a78020, server=server@entry=0x55a663a77550,
      client=client@entry=0x55a663a7b960, msg=msg@entry=0x55a663a7bc50) at rpc/virnetserverprogram.c:307
  #7  0x000055a662871d56 in virNetServerProcessMsg (msg=0x55a663a7bc50, prog=0x55a663a78020, client=0x55a663a7b960,
      srv=0x55a663a77550) at rpc/virnetserver.c:148
  #8  virNetServerDispatchNewMessage (client=0x55a663a7b960, msg=0x55a663a7bc50, opaque=0x55a663a77550)
      at rpc/virnetserver.c:227
  #9  0x000055a66286e4c0 in virNetServerClientDispatchRead (client=client@entry=0x55a663a7b960)
      at rpc/virnetserverclient.c:1322
  #10 0x000055a66286e813 in virNetServerClientDispatchEvent (sock=<optimized out>, events=1, opaque=0x55a663a7b960)
      at rpc/virnetserverclient.c:1507
  #11 0x000055a662899be0 in virEventPollDispatchHandles (fds=0x55a663a7bdc0, nfds=<optimized out>)
      at util/vireventpoll.c:508
  #12 virEventPollRunOnce () at util/vireventpoll.c:657
  #13 0x000055a6628982f1 in virEventRunDefaultImpl () at util/virevent.c:327
  #14 0x000055a6628716d5 in virNetDaemonRun (dmn=0x55a663a771b0) at rpc/virnetdaemon.c:858
  #15 0x000055a662864c1d in main (argc=<optimized out>, argv=0x7ffd105b4838) at logging/log_daemon.c:1235

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/rpc/virnetserverclient.c | 79 ++++++++++++++++++++++++++++++--------------
 1 file changed, 55 insertions(+), 24 deletions(-)

diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index ea0d5abdee..fb4775235d 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -143,7 +143,7 @@ VIR_ONCE_GLOBAL_INIT(virNetServerClient)
 
 static void virNetServerClientDispatchEvent(virNetSocketPtr sock, int events, void *opaque);
 static void virNetServerClientUpdateEvent(virNetServerClientPtr client);
-static void virNetServerClientDispatchRead(virNetServerClientPtr client);
+static virNetMessagePtr virNetServerClientDispatchRead(virNetServerClientPtr client);
 static int virNetServerClientSendMessageLocked(virNetServerClientPtr client,
                                                virNetMessagePtr msg);
 
@@ -340,18 +340,41 @@ virNetServerClientCheckAccess(virNetServerClientPtr client)
 }
 #endif
 
+static void virNetServerClientDispatchMessage(virNetServerClientPtr client,
+                                              virNetMessagePtr msg)
+{
+    virObjectLock(client);
+    if (!client->dispatchFunc) {
+        virNetMessageFree(msg);
+        client->wantClose = true;
+        virObjectUnlock(client);
+    } else {
+        virObjectUnlock(client);
+        /* Accessing 'client' is safe, because virNetServerClientSetDispatcher
+         * only permits setting 'dispatchFunc' once, so if non-NULL, it will
+         * never change again
+         */
+        client->dispatchFunc(client, msg, client->dispatchOpaque);
+    }
+}
+
 
 static void virNetServerClientSockTimerFunc(int timer,
                                             void *opaque)
 {
     virNetServerClientPtr client = opaque;
+    virNetMessagePtr msg = NULL;
     virObjectLock(client);
     virEventUpdateTimeout(timer, -1);
     /* Although client->rx != NULL when this timer is enabled, it might have
      * changed since the client was unlocked in the meantime. */
     if (client->rx)
-        virNetServerClientDispatchRead(client);
+        msg = virNetServerClientDispatchRead(client);
     virObjectUnlock(client);
+
+    if (msg) {
+        virNetServerClientDispatchMessage(client, msg);
+    }
 }
 
 
@@ -950,8 +973,13 @@ void virNetServerClientSetDispatcher(virNetServerClientPtr client,
                                      void *opaque)
 {
     virObjectLock(client);
-    client->dispatchFunc = func;
-    client->dispatchOpaque = opaque;
+    /* Only set dispatcher if not already set, to avoid race
+     * with dispatch code that runs without locks held
+     */
+    if (!client->dispatchFunc) {
+        client->dispatchFunc = func;
+        client->dispatchOpaque = opaque;
+    }
     virObjectUnlock(client);
 }
 
@@ -1196,26 +1224,32 @@ static ssize_t virNetServerClientRead(virNetServerClientPtr client)
 
 
 /*
- * Read data until we get a complete message to process
+ * Read data until we get a complete message to process.
+ * If a complete message is available, it will be returned
+ * from this method, for dispatch by the caller.
+ *
+ * Returns a complete message for dispatch, or NULL if none is
+ * yet available, or an error occurred. On error, the wantClose
+ * flag will be set.
  */
-static void virNetServerClientDispatchRead(virNetServerClientPtr client)
+static virNetMessagePtr virNetServerClientDispatchRead(virNetServerClientPtr client)
 {
  readmore:
     if (client->rx->nfds == 0) {
         if (virNetServerClientRead(client) < 0) {
             client->wantClose = true;
-            return; /* Error */
+            return NULL; /* Error */
         }
     }
 
     if (client->rx->bufferOffset < client->rx->bufferLength)
-        return; /* Still not read enough */
+        return NULL; /* Still not read enough */
 
     /* Either done with length word header */
     if (client->rx->bufferLength == VIR_NET_MESSAGE_LEN_MAX) {
         if (virNetMessageDecodeLength(client->rx) < 0) {
             client->wantClose = true;
-            return;
+            return NULL;
         }
 
         virNetServerClientUpdateEvent(client);
@@ -1236,7 +1270,7 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
             virNetMessageQueueServe(&client->rx);
             virNetMessageFree(msg);
             client->wantClose = true;
-            return;
+            return NULL;
         }
 
         /* Now figure out if we need to read more data to get some
@@ -1246,7 +1280,7 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
                 virNetMessageQueueServe(&client->rx);
                 virNetMessageFree(msg);
                 client->wantClose = true;
-                return; /* Error */
+                return NULL; /* Error */
             }
 
             /* Try getting the file descriptors (may fail if blocking) */
@@ -1256,7 +1290,7 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
                     virNetMessageQueueServe(&client->rx);
                     virNetMessageFree(msg);
                     client->wantClose = true;
-                    return;
+                    return NULL;
                 }
                 if (rv == 0) /* Blocking */
                     break;
@@ -1270,7 +1304,7 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
                  * again next time we run this method
                  */
                 client->rx->bufferOffset = client->rx->bufferLength;
-                return;
+                return NULL;
             }
         }
 
@@ -1313,16 +1347,6 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
             }
         }
 
-        /* Send off to for normal dispatch to workers */
-        if (msg) {
-            if (!client->dispatchFunc) {
-                virNetMessageFree(msg);
-                client->wantClose = true;
-            } else {
-                client->dispatchFunc(client, msg, client->dispatchOpaque);
-            }
-        }
-
         /* Possibly need to create another receive buffer */
         if (client->nrequests < client->nrequests_max) {
             if (!(client->rx = virNetMessageNew(true))) {
@@ -1338,6 +1362,8 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
             }
         }
         virNetServerClientUpdateEvent(client);
+
+        return msg;
     }
 }
 
@@ -1482,6 +1508,7 @@ static void
 virNetServerClientDispatchEvent(virNetSocketPtr sock, int events, void *opaque)
 {
     virNetServerClientPtr client = opaque;
+    virNetMessagePtr msg = NULL;
 
     virObjectLock(client);
 
@@ -1504,7 +1531,7 @@ virNetServerClientDispatchEvent(virNetSocketPtr sock, int events, void *opaque)
                 virNetServerClientDispatchWrite(client);
             if (events & VIR_EVENT_HANDLE_READABLE &&
                 client->rx)
-                virNetServerClientDispatchRead(client);
+                msg = virNetServerClientDispatchRead(client);
 #if WITH_GNUTLS
         }
 #endif
@@ -1517,6 +1544,10 @@ virNetServerClientDispatchEvent(virNetSocketPtr sock, int events, void *opaque)
         client->wantClose = true;
 
     virObjectUnlock(client);
+
+    if (msg) {
+        virNetServerClientDispatchMessage(client, msg);
+    }
 }
 
 
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] rpc: invoke the message dispatch callback with client unlocked
Posted by Jim Fehlig, 15 weeks ago
On 03/06/2018 10:58 AM, Daniel P. Berrangé wrote:
> Currently if the virNetServer instance is created with max_workers==0 to
> request a non-threaded dispatch process, we deadlock during dispatch
> 
>    #0  0x00007fb845f6f42d in __lll_lock_wait () from /lib64/libpthread.so.0
>    #1  0x00007fb845f681d3 in pthread_mutex_lock () from /lib64/libpthread.so.0
>    #2  0x000055a6628bb305 in virMutexLock (m=<optimized out>) at util/virthread.c:89
>    #3  0x000055a6628a984b in virObjectLock (anyobj=<optimized out>) at util/virobject.c:435
>    #4  0x000055a66286fcde in virNetServerClientIsAuthenticated (client=client@entry=0x55a663a7b960)
>        at rpc/virnetserverclient.c:1565
>    #5  0x000055a66286cc17 in virNetServerProgramDispatchCall (msg=0x55a663a7bc50, client=0x55a663a7b960,
>        server=0x55a663a77550, prog=0x55a663a78020) at rpc/virnetserverprogram.c:407
>    #6  virNetServerProgramDispatch (prog=prog@entry=0x55a663a78020, server=server@entry=0x55a663a77550,
>        client=client@entry=0x55a663a7b960, msg=msg@entry=0x55a663a7bc50) at rpc/virnetserverprogram.c:307
>    #7  0x000055a662871d56 in virNetServerProcessMsg (msg=0x55a663a7bc50, prog=0x55a663a78020, client=0x55a663a7b960,
>        srv=0x55a663a77550) at rpc/virnetserver.c:148
>    #8  virNetServerDispatchNewMessage (client=0x55a663a7b960, msg=0x55a663a7bc50, opaque=0x55a663a77550)
>        at rpc/virnetserver.c:227
>    #9  0x000055a66286e4c0 in virNetServerClientDispatchRead (client=client@entry=0x55a663a7b960)
>        at rpc/virnetserverclient.c:1322
>    #10 0x000055a66286e813 in virNetServerClientDispatchEvent (sock=<optimized out>, events=1, opaque=0x55a663a7b960)
>        at rpc/virnetserverclient.c:1507
>    #11 0x000055a662899be0 in virEventPollDispatchHandles (fds=0x55a663a7bdc0, nfds=<optimized out>)
>        at util/vireventpoll.c:508
>    #12 virEventPollRunOnce () at util/vireventpoll.c:657
>    #13 0x000055a6628982f1 in virEventRunDefaultImpl () at util/virevent.c:327
>    #14 0x000055a6628716d5 in virNetDaemonRun (dmn=0x55a663a771b0) at rpc/virnetdaemon.c:858
>    #15 0x000055a662864c1d in main (argc=<optimized out>, argv=0x7ffd105b4838) at logging/log_daemon.c:1235
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   src/rpc/virnetserverclient.c | 79 ++++++++++++++++++++++++++++++--------------
>   1 file changed, 55 insertions(+), 24 deletions(-)
> 
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index ea0d5abdee..fb4775235d 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -143,7 +143,7 @@ VIR_ONCE_GLOBAL_INIT(virNetServerClient)
>   
>   static void virNetServerClientDispatchEvent(virNetSocketPtr sock, int events, void *opaque);
>   static void virNetServerClientUpdateEvent(virNetServerClientPtr client);
> -static void virNetServerClientDispatchRead(virNetServerClientPtr client);
> +static virNetMessagePtr virNetServerClientDispatchRead(virNetServerClientPtr client);
>   static int virNetServerClientSendMessageLocked(virNetServerClientPtr client,
>                                                  virNetMessagePtr msg);
>   
> @@ -340,18 +340,41 @@ virNetServerClientCheckAccess(virNetServerClientPtr client)
>   }
>   #endif
>   
> +static void virNetServerClientDispatchMessage(virNetServerClientPtr client,
> +                                              virNetMessagePtr msg)
> +{
> +    virObjectLock(client);
> +    if (!client->dispatchFunc) {
> +        virNetMessageFree(msg);
> +        client->wantClose = true;
> +        virObjectUnlock(client);
> +    } else {
> +        virObjectUnlock(client);
> +        /* Accessing 'client' is safe, because virNetServerClientSetDispatcher
> +         * only permits setting 'dispatchFunc' once, so if non-NULL, it will
> +         * never change again
> +         */
> +        client->dispatchFunc(client, msg, client->dispatchOpaque);
> +    }
> +}
> +
>   
>   static void virNetServerClientSockTimerFunc(int timer,
>                                               void *opaque)
>   {
>       virNetServerClientPtr client = opaque;
> +    virNetMessagePtr msg = NULL;
>       virObjectLock(client);
>       virEventUpdateTimeout(timer, -1);
>       /* Although client->rx != NULL when this timer is enabled, it might have
>        * changed since the client was unlocked in the meantime. */
>       if (client->rx)
> -        virNetServerClientDispatchRead(client);
> +        msg = virNetServerClientDispatchRead(client);
>       virObjectUnlock(client);
> +
> +    if (msg) {
> +        virNetServerClientDispatchMessage(client, msg);
> +    }

syntax-check complains about curly brackets around single-line body.

>   }
>   
>   
> @@ -950,8 +973,13 @@ void virNetServerClientSetDispatcher(virNetServerClientPtr client,
>                                        void *opaque)
>   {
>       virObjectLock(client);
> -    client->dispatchFunc = func;
> -    client->dispatchOpaque = opaque;
> +    /* Only set dispatcher if not already set, to avoid race
> +     * with dispatch code that runs without locks held
> +     */
> +    if (!client->dispatchFunc) {
> +        client->dispatchFunc = func;
> +        client->dispatchOpaque = opaque;
> +    }
>       virObjectUnlock(client);
>   }
>   
> @@ -1196,26 +1224,32 @@ static ssize_t virNetServerClientRead(virNetServerClientPtr client)
>   
>   
>   /*
> - * Read data until we get a complete message to process
> + * Read data until we get a complete message to process.
> + * If a complete message is available, it will be returned
> + * from this method, for dispatch by the caller.
> + *
> + * Returns a complete message for dispatch, or NULL if none is
> + * yet available, or an error occurred. On error, the wantClose
> + * flag will be set.
>    */
> -static void virNetServerClientDispatchRead(virNetServerClientPtr client)
> +static virNetMessagePtr virNetServerClientDispatchRead(virNetServerClientPtr client)
>   {
>    readmore:
>       if (client->rx->nfds == 0) {
>           if (virNetServerClientRead(client) < 0) {
>               client->wantClose = true;
> -            return; /* Error */
> +            return NULL; /* Error */
>           }
>       }
>   
>       if (client->rx->bufferOffset < client->rx->bufferLength)
> -        return; /* Still not read enough */
> +        return NULL; /* Still not read enough */
>   
>       /* Either done with length word header */
>       if (client->rx->bufferLength == VIR_NET_MESSAGE_LEN_MAX) {
>           if (virNetMessageDecodeLength(client->rx) < 0) {
>               client->wantClose = true;
> -            return;
> +            return NULL;
>           }
>   
>           virNetServerClientUpdateEvent(client);
> @@ -1236,7 +1270,7 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
>               virNetMessageQueueServe(&client->rx);
>               virNetMessageFree(msg);
>               client->wantClose = true;
> -            return;
> +            return NULL;
>           }
>   
>           /* Now figure out if we need to read more data to get some
> @@ -1246,7 +1280,7 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
>                   virNetMessageQueueServe(&client->rx);
>                   virNetMessageFree(msg);
>                   client->wantClose = true;
> -                return; /* Error */
> +                return NULL; /* Error */
>               }
>   
>               /* Try getting the file descriptors (may fail if blocking) */
> @@ -1256,7 +1290,7 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
>                       virNetMessageQueueServe(&client->rx);
>                       virNetMessageFree(msg);
>                       client->wantClose = true;
> -                    return;
> +                    return NULL;
>                   }
>                   if (rv == 0) /* Blocking */
>                       break;
> @@ -1270,7 +1304,7 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
>                    * again next time we run this method
>                    */
>                   client->rx->bufferOffset = client->rx->bufferLength;
> -                return;
> +                return NULL;
>               }
>           }
>   
> @@ -1313,16 +1347,6 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
>               }
>           }
>   
> -        /* Send off to for normal dispatch to workers */
> -        if (msg) {
> -            if (!client->dispatchFunc) {
> -                virNetMessageFree(msg);
> -                client->wantClose = true;
> -            } else {
> -                client->dispatchFunc(client, msg, client->dispatchOpaque);
> -            }
> -        }
> -
>           /* Possibly need to create another receive buffer */
>           if (client->nrequests < client->nrequests_max) {
>               if (!(client->rx = virNetMessageNew(true))) {
> @@ -1338,6 +1362,8 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
>               }
>           }
>           virNetServerClientUpdateEvent(client);
> +
> +        return msg;
>       }
>   }
>   
> @@ -1482,6 +1508,7 @@ static void
>   virNetServerClientDispatchEvent(virNetSocketPtr sock, int events, void *opaque)
>   {
>       virNetServerClientPtr client = opaque;
> +    virNetMessagePtr msg = NULL;
>   
>       virObjectLock(client);
>   
> @@ -1504,7 +1531,7 @@ virNetServerClientDispatchEvent(virNetSocketPtr sock, int events, void *opaque)
>                   virNetServerClientDispatchWrite(client);
>               if (events & VIR_EVENT_HANDLE_READABLE &&
>                   client->rx)
> -                virNetServerClientDispatchRead(client);
> +                msg = virNetServerClientDispatchRead(client);
>   #if WITH_GNUTLS
>           }
>   #endif
> @@ -1517,6 +1544,10 @@ virNetServerClientDispatchEvent(virNetSocketPtr sock, int events, void *opaque)
>           client->wantClose = true;
>   
>       virObjectUnlock(client);
> +
> +    if (msg) {
> +        virNetServerClientDispatchMessage(client, msg);
> +    }

Same here. Looks good otherwise, but again I haven't spent a lot of time in this 
area of the code.

Reviewed-by: Jim Fehlig <jfehlig@suse.com>

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] rpc: invoke the message dispatch callback with client unlocked
Posted by John Ferlan, 15 weeks ago

On 03/06/2018 12:58 PM, Daniel P. Berrangé wrote:
> Currently if the virNetServer instance is created with max_workers==0 to
> request a non-threaded dispatch process, we deadlock during dispatch
> 
>   #0  0x00007fb845f6f42d in __lll_lock_wait () from /lib64/libpthread.so.0
>   #1  0x00007fb845f681d3 in pthread_mutex_lock () from /lib64/libpthread.so.0
>   #2  0x000055a6628bb305 in virMutexLock (m=<optimized out>) at util/virthread.c:89
>   #3  0x000055a6628a984b in virObjectLock (anyobj=<optimized out>) at util/virobject.c:435
>   #4  0x000055a66286fcde in virNetServerClientIsAuthenticated (client=client@entry=0x55a663a7b960)
>       at rpc/virnetserverclient.c:1565
>   #5  0x000055a66286cc17 in virNetServerProgramDispatchCall (msg=0x55a663a7bc50, client=0x55a663a7b960,
>       server=0x55a663a77550, prog=0x55a663a78020) at rpc/virnetserverprogram.c:407
>   #6  virNetServerProgramDispatch (prog=prog@entry=0x55a663a78020, server=server@entry=0x55a663a77550,
>       client=client@entry=0x55a663a7b960, msg=msg@entry=0x55a663a7bc50) at rpc/virnetserverprogram.c:307
>   #7  0x000055a662871d56 in virNetServerProcessMsg (msg=0x55a663a7bc50, prog=0x55a663a78020, client=0x55a663a7b960,
>       srv=0x55a663a77550) at rpc/virnetserver.c:148
>   #8  virNetServerDispatchNewMessage (client=0x55a663a7b960, msg=0x55a663a7bc50, opaque=0x55a663a77550)
>       at rpc/virnetserver.c:227
>   #9  0x000055a66286e4c0 in virNetServerClientDispatchRead (client=client@entry=0x55a663a7b960)
>       at rpc/virnetserverclient.c:1322
>   #10 0x000055a66286e813 in virNetServerClientDispatchEvent (sock=<optimized out>, events=1, opaque=0x55a663a7b960)
>       at rpc/virnetserverclient.c:1507
>   #11 0x000055a662899be0 in virEventPollDispatchHandles (fds=0x55a663a7bdc0, nfds=<optimized out>)
>       at util/vireventpoll.c:508
>   #12 virEventPollRunOnce () at util/vireventpoll.c:657
>   #13 0x000055a6628982f1 in virEventRunDefaultImpl () at util/virevent.c:327
>   #14 0x000055a6628716d5 in virNetDaemonRun (dmn=0x55a663a771b0) at rpc/virnetdaemon.c:858
>   #15 0x000055a662864c1d in main (argc=<optimized out>, argv=0x7ffd105b4838) at logging/log_daemon.c:1235
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/rpc/virnetserverclient.c | 79 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 55 insertions(+), 24 deletions(-)
> 

I had the same syntax-check notes that Jim had.

Beyond that since both callers Unlock prior to calling
virNetServerClientDispatchMessage and the code relocks right away, would
calling with client lock'd still be prudent/possible?  Callers would
then be able to

    if (msg)
        virNetServerClientDispatchMessage(...)
    else
        Unlock(client)

Once we get to virNetServerProgramDispatch it expects an unlocked
client. Maybe we should comment some of the other callers in the path to
indicate whether server/client need to be locked. Not required though -
only helpful for future spelunkers of this code.

[...]

> @@ -1313,16 +1347,6 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
>              }
>          }
>  
> -        /* Send off to for normal dispatch to workers */
> -        if (msg) {
> -            if (!client->dispatchFunc) {
> -                virNetMessageFree(msg);
> -                client->wantClose = true;
> -            } else {
> -                client->dispatchFunc(client, msg, client->dispatchOpaque);
> -            }
> -        }
> -

I thought about the order change here w/r/t the following code being run
before the dispatch options above; however, that would perhaps more of a
concern for the path where we have no workers. If we have a worker, then
it gets queued and the following would then occur first anyway, right?
So, IOW, I think the reordering here is fine. It would happen before the
worker got around to handling the client and moving it around to
virNetServerClientDispatchMessage could get ugly w/r/t locks.

Reviewed-by: John Ferlan <jferlan@redhat.com>

John


>          /* Possibly need to create another receive buffer */
>          if (client->nrequests < client->nrequests_max) {
>              if (!(client->rx = virNetMessageNew(true))) {
> @@ -1338,6 +1362,8 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
>              }
>          }
>          virNetServerClientUpdateEvent(client);
> +
> +        return msg;
>      }
>  }
>  

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] rpc: invoke the message dispatch callback with client unlocked
Posted by Daniel P. Berrangé, 15 weeks ago
On Wed, Mar 07, 2018 at 06:48:25PM -0500, John Ferlan wrote:
> 
> 
> On 03/06/2018 12:58 PM, Daniel P. Berrangé wrote:
> > Currently if the virNetServer instance is created with max_workers==0 to
> > request a non-threaded dispatch process, we deadlock during dispatch
> > 
> >   #0  0x00007fb845f6f42d in __lll_lock_wait () from /lib64/libpthread.so.0
> >   #1  0x00007fb845f681d3 in pthread_mutex_lock () from /lib64/libpthread.so.0
> >   #2  0x000055a6628bb305 in virMutexLock (m=<optimized out>) at util/virthread.c:89
> >   #3  0x000055a6628a984b in virObjectLock (anyobj=<optimized out>) at util/virobject.c:435
> >   #4  0x000055a66286fcde in virNetServerClientIsAuthenticated (client=client@entry=0x55a663a7b960)
> >       at rpc/virnetserverclient.c:1565
> >   #5  0x000055a66286cc17 in virNetServerProgramDispatchCall (msg=0x55a663a7bc50, client=0x55a663a7b960,
> >       server=0x55a663a77550, prog=0x55a663a78020) at rpc/virnetserverprogram.c:407
> >   #6  virNetServerProgramDispatch (prog=prog@entry=0x55a663a78020, server=server@entry=0x55a663a77550,
> >       client=client@entry=0x55a663a7b960, msg=msg@entry=0x55a663a7bc50) at rpc/virnetserverprogram.c:307
> >   #7  0x000055a662871d56 in virNetServerProcessMsg (msg=0x55a663a7bc50, prog=0x55a663a78020, client=0x55a663a7b960,
> >       srv=0x55a663a77550) at rpc/virnetserver.c:148
> >   #8  virNetServerDispatchNewMessage (client=0x55a663a7b960, msg=0x55a663a7bc50, opaque=0x55a663a77550)
> >       at rpc/virnetserver.c:227
> >   #9  0x000055a66286e4c0 in virNetServerClientDispatchRead (client=client@entry=0x55a663a7b960)
> >       at rpc/virnetserverclient.c:1322
> >   #10 0x000055a66286e813 in virNetServerClientDispatchEvent (sock=<optimized out>, events=1, opaque=0x55a663a7b960)
> >       at rpc/virnetserverclient.c:1507
> >   #11 0x000055a662899be0 in virEventPollDispatchHandles (fds=0x55a663a7bdc0, nfds=<optimized out>)
> >       at util/vireventpoll.c:508
> >   #12 virEventPollRunOnce () at util/vireventpoll.c:657
> >   #13 0x000055a6628982f1 in virEventRunDefaultImpl () at util/virevent.c:327
> >   #14 0x000055a6628716d5 in virNetDaemonRun (dmn=0x55a663a771b0) at rpc/virnetdaemon.c:858
> >   #15 0x000055a662864c1d in main (argc=<optimized out>, argv=0x7ffd105b4838) at logging/log_daemon.c:1235
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  src/rpc/virnetserverclient.c | 79 ++++++++++++++++++++++++++++++--------------
> >  1 file changed, 55 insertions(+), 24 deletions(-)
> > 
> 
> I had the same syntax-check notes that Jim had.
> 
> Beyond that since both callers Unlock prior to calling
> virNetServerClientDispatchMessage and the code relocks right away, would
> calling with client lock'd still be prudent/possible?  Callers would
> then be able to
> 
>     if (msg)
>         virNetServerClientDispatchMessage(...)
>     else
>         Unlock(client)
> 
> Once we get to virNetServerProgramDispatch it expects an unlocked
> client. Maybe we should comment some of the other callers in the path to
> indicate whether server/client need to be locked. Not required though -
> only helpful for future spelunkers of this code.

I generally like to avoid situations where one method locks the object and
then calls another method which unlocks it. IMHO, it leads to harder to
understand code where the lock/unlock calls are spread out.

Since we own the reference on client, I think unlocking & locking again
is harmless here.

> > @@ -1313,16 +1347,6 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
> >              }
> >          }
> >  
> > -        /* Send off to for normal dispatch to workers */
> > -        if (msg) {
> > -            if (!client->dispatchFunc) {
> > -                virNetMessageFree(msg);
> > -                client->wantClose = true;
> > -            } else {
> > -                client->dispatchFunc(client, msg, client->dispatchOpaque);
> > -            }
> > -        }
> > -
> 
> I thought about the order change here w/r/t the following code being run
> before the dispatch options above; however, that would perhaps more of a
> concern for the path where we have no workers. If we have a worker, then
> it gets queued and the following would then occur first anyway, right?
> So, IOW, I think the reordering here is fine. It would happen before the
> worker got around to handling the client and moving it around to
> virNetServerClientDispatchMessage could get ugly w/r/t locks.

Yeah, that was my rationale too - since worker threads are asynchronous
we're effectively running the second block first regardless, so the
ordering should be harmless.

> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> 
> John
> 
> 
> >          /* Possibly need to create another receive buffer */
> >          if (client->nrequests < client->nrequests_max) {
> >              if (!(client->rx = virNetMessageNew(true))) {
> > @@ -1338,6 +1362,8 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
> >              }
> >          }
> >          virNetServerClientUpdateEvent(client);
> > +
> > +        return msg;
> >      }
> >  }
> >  
> 
> [...]

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/5] rpc: avoid crashing in pre-exec if no workers are present
Posted by Daniel P. Berrangé, 15 weeks ago
If max_workers is set to zero, then the worker thread pool won't be
created, so when serializing state for pre-exec we must set various
parameters to zero.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/rpc/virnetserver.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 7a1376bf49..3ce21a8f53 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -580,18 +580,21 @@ virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv)
         goto error;
 
     if (virJSONValueObjectAppendNumberUint(object, "min_workers",
+                                           srv->workers == NULL ? 0 :
                                            virThreadPoolGetMinWorkers(srv->workers)) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("Cannot set min_workers data in JSON document"));
         goto error;
     }
     if (virJSONValueObjectAppendNumberUint(object, "max_workers",
+                                           srv->workers == NULL ? 0 :
                                            virThreadPoolGetMaxWorkers(srv->workers)) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("Cannot set max_workers data in JSON document"));
         goto error;
     }
     if (virJSONValueObjectAppendNumberUint(object, "priority_workers",
+                                           srv->workers == NULL ? 0 :
                                            virThreadPoolGetPriorityWorkers(srv->workers)) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("Cannot set priority_workers data in JSON document"));
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] rpc: avoid crashing in pre-exec if no workers are present
Posted by Jim Fehlig, 15 weeks ago
On 03/06/2018 10:58 AM, Daniel P. Berrangé wrote:
> If max_workers is set to zero, then the worker thread pool won't be
> created, so when serializing state for pre-exec we must set various
> parameters to zero.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   src/rpc/virnetserver.c | 3 +++
>   1 file changed, 3 insertions(+)

Reviewed-by: Jim Fehlig <jfehlig@suse.com>

Regards,
Jim

> 
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 7a1376bf49..3ce21a8f53 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -580,18 +580,21 @@ virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv)
>           goto error;
>   
>       if (virJSONValueObjectAppendNumberUint(object, "min_workers",
> +                                           srv->workers == NULL ? 0 :
>                                              virThreadPoolGetMinWorkers(srv->workers)) < 0) {
>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                          _("Cannot set min_workers data in JSON document"));
>           goto error;
>       }
>       if (virJSONValueObjectAppendNumberUint(object, "max_workers",
> +                                           srv->workers == NULL ? 0 :
>                                              virThreadPoolGetMaxWorkers(srv->workers)) < 0) {
>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                          _("Cannot set max_workers data in JSON document"));
>           goto error;
>       }
>       if (virJSONValueObjectAppendNumberUint(object, "priority_workers",
> +                                           srv->workers == NULL ? 0 :
>                                              virThreadPoolGetPriorityWorkers(srv->workers)) < 0) {
>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                          _("Cannot set priority_workers data in JSON document"));
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] rpc: avoid crashing in pre-exec if no workers are present
Posted by John Ferlan, 15 weeks ago

On 03/06/2018 12:58 PM, Daniel P. Berrangé wrote:
> If max_workers is set to zero, then the worker thread pool won't be
> created, so when serializing state for pre-exec we must set various
> parameters to zero.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/rpc/virnetserver.c | 3 +++
>  1 file changed, 3 insertions(+)
> 

Alternatively, the various virThreadPoolGet* API's could check :

    if (!pool)
        return 0;

and we don't run into the same problem for other callers for all the API's.

This works, but for this limited case of data being fetched.

It'd be a weak R-b at best.

John

> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 7a1376bf49..3ce21a8f53 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -580,18 +580,21 @@ virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv)
>          goto error;
>  
>      if (virJSONValueObjectAppendNumberUint(object, "min_workers",
> +                                           srv->workers == NULL ? 0 :
>                                             virThreadPoolGetMinWorkers(srv->workers)) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("Cannot set min_workers data in JSON document"));
>          goto error;
>      }
>      if (virJSONValueObjectAppendNumberUint(object, "max_workers",
> +                                           srv->workers == NULL ? 0 :
>                                             virThreadPoolGetMaxWorkers(srv->workers)) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("Cannot set max_workers data in JSON document"));
>          goto error;
>      }
>      if (virJSONValueObjectAppendNumberUint(object, "priority_workers",
> +                                           srv->workers == NULL ? 0 :
>                                             virThreadPoolGetPriorityWorkers(srv->workers)) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("Cannot set priority_workers data in JSON document"));
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] rpc: avoid crashing in pre-exec if no workers are present
Posted by Daniel P. Berrangé, 15 weeks ago
On Wed, Mar 07, 2018 at 06:50:09PM -0500, John Ferlan wrote:
> 
> 
> On 03/06/2018 12:58 PM, Daniel P. Berrangé wrote:
> > If max_workers is set to zero, then the worker thread pool won't be
> > created, so when serializing state for pre-exec we must set various
> > parameters to zero.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  src/rpc/virnetserver.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> 
> Alternatively, the various virThreadPoolGet* API's could check :
> 
>     if (!pool)
>         return 0;
> 
> and we don't run into the same problem for other callers for all the API's.

My general opinion is that code should never knowingly pass NULL into
object methods, so I prefer to handle this in callers.

> 
> This works, but for this limited case of data being fetched.
> 
> It'd be a weak R-b at best.
> 
> John
> 
> > diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> > index 7a1376bf49..3ce21a8f53 100644
> > --- a/src/rpc/virnetserver.c
> > +++ b/src/rpc/virnetserver.c
> > @@ -580,18 +580,21 @@ virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv)
> >          goto error;
> >  
> >      if (virJSONValueObjectAppendNumberUint(object, "min_workers",
> > +                                           srv->workers == NULL ? 0 :
> >                                             virThreadPoolGetMinWorkers(srv->workers)) < 0) {
> >          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >                         _("Cannot set min_workers data in JSON document"));
> >          goto error;
> >      }
> >      if (virJSONValueObjectAppendNumberUint(object, "max_workers",
> > +                                           srv->workers == NULL ? 0 :
> >                                             virThreadPoolGetMaxWorkers(srv->workers)) < 0) {
> >          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >                         _("Cannot set max_workers data in JSON document"));
> >          goto error;
> >      }
> >      if (virJSONValueObjectAppendNumberUint(object, "priority_workers",
> > +                                           srv->workers == NULL ? 0 :
> >                                             virThreadPoolGetPriorityWorkers(srv->workers)) < 0) {
> >          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >                         _("Cannot set priority_workers data in JSON document"));
> > 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/5] rpc: switch virtlockd and virtlogd to use single-threaded dispatch
Posted by Daniel P. Berrangé, 15 weeks ago
Currently both virtlogd and virtlockd use a single worker thread for
dispatching RPC messages. Even this is overkill and their RPC message
handling callbacks all run in short, finite time and so blocking the
main loop is not an issue like you'd see in libvirtd with long running
QEMU commands.

By setting max_workers==0, we can turn off the worker thread and run
these daemons single threaded. This in turn fixes a serious problem in
the virtlockd daemon whereby it looses all fcntl() locks at re-exec due
to multiple threads existing. fcntl() locks only get preserved if the
process is single threaded at time of exec().

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/locking/lock_daemon.c | 4 ++--
 src/logging/log_daemon.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 79ab90fc91..7afff42246 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -165,7 +165,7 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool privileged)
         goto error;
 
     if (!(srv = virNetServerNew("virtlockd", 1,
-                                1, 1, 0, config->max_clients,
+                                0, 0, 0, config->max_clients,
                                 config->max_clients, -1, 0,
                                 NULL,
                                 virLockDaemonClientNew,
@@ -180,7 +180,7 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool privileged)
     srv = NULL;
 
     if (!(srv = virNetServerNew("admin", 1,
-                                1, 1, 0, config->admin_max_clients,
+                                0, 0, 0, config->admin_max_clients,
                                 config->admin_max_clients, -1, 0,
                                 NULL,
                                 remoteAdmClientNew,
diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
index d54d26ab9d..35d7ebb6d2 100644
--- a/src/logging/log_daemon.c
+++ b/src/logging/log_daemon.c
@@ -154,7 +154,7 @@ virLogDaemonNew(virLogDaemonConfigPtr config, bool privileged)
         goto error;
 
     if (!(srv = virNetServerNew("virtlogd", 1,
-                                1, 1, 0, config->max_clients,
+                                0, 0, 0, config->max_clients,
                                 config->max_clients, -1, 0,
                                 NULL,
                                 virLogDaemonClientNew,
@@ -169,7 +169,7 @@ virLogDaemonNew(virLogDaemonConfigPtr config, bool privileged)
     srv = NULL;
 
     if (!(srv = virNetServerNew("admin", 1,
-                                1, 1, 0, config->admin_max_clients,
+                                0, 0, 0, config->admin_max_clients,
                                 config->admin_max_clients, -1, 0,
                                 NULL,
                                 remoteAdmClientNew,
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] rpc: switch virtlockd and virtlogd to use single-threaded dispatch
Posted by Jim Fehlig, 15 weeks ago
On 03/06/2018 10:58 AM, Daniel P. Berrangé wrote:
> Currently both virtlogd and virtlockd use a single worker thread for
> dispatching RPC messages. Even this is overkill and their RPC message
> handling callbacks all run in short, finite time and so blocking the
> main loop is not an issue like you'd see in libvirtd with long running
> QEMU commands.
> 
> By setting max_workers==0, we can turn off the worker thread and run
> these daemons single threaded. This in turn fixes a serious problem in
> the virtlockd daemon whereby it looses all fcntl() locks at re-exec due
> to multiple threads existing. fcntl() locks only get preserved if the
> process is single threaded at time of exec().

I suppose this change has no affect when e.g. starting many domains in parallel 
when locking is enabled. Before the change, there's still only one worker thread 
to process requests.

I've tested the series and locks are now preserved across re-execs of virtlockd. 
Question is whether we want this change or pursue fixing the underlying kernel bug?

FYI, via the non-public bug I asked a glibc maintainer about the lost lock 
behavior. He agreed it is a kernel bug and posted the below comment to the bug.

Regards,
Jim

First, I agree that POSIX file record locks (i.e. the fcntl F_SETLK ones, which
you're using) _are_ to be preserved over execve (absent any FD_CLOEXEC of
course, which you aren't using).  (Relevant quote from fcntl(2):

        Record locks are not inherited by  a  child  created  via  fork(2),
        but  are  preserved  across  an execve(2).

Second I agree that the existence or non-existence of threads must not play
a role in the above.

Third I see this from strace of your reproducer (only relevant snippets, with
a bit of explanation):

13:54:09.581429 clone(child_stack=0x7f74b0517ff0,
flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
parent_tidptr=0x7f74b05189d0, tls=0x7f74b0518700, child_tidptr=0x7f74b05189d0)
= 30911
Process 30911 attached

So, here we created the thread 30911.  PID 30910 is now taking the lock:

[pid 30910] 13:54:09.581562 open("lock.txt", O_WRONLY|O_CREAT|O_TRUNC, 0755
<unfinished ...>
[pid 30911] 13:54:09.581595 set_robust_list(0x7f74b05189e0, 24 <unfinished ...>
[pid 30910] 13:54:09.581647 <... open resumed> ) = 3
[pid 30911] 13:54:09.581671 <... set_robust_list resumed> ) = 0
[pid 30910] 13:54:09.581693 fcntl(3, F_SETLK, {type=F_WRLCK, whence=SEEK_SET,
start=0, len=42} <unfinished ...>
[pid 30911] 13:54:09.581722 rt_sigprocmask(SIG_BLOCK, [CHLD],  <unfinished ...>
[pid 30910] 13:54:09.581750 <... fcntl resumed> ) = 0

Lock aquired.  Now we do a little debug output and sleeping, and the only
actions on FD 3 during this are:

[pid 30910] 13:54:09.581790 fcntl(3, F_GETFD <unfinished ...>
[pid 30910] 13:54:09.581872 <... fcntl resumed> ) = 0
[pid 30910] 13:54:09.581911 fcntl(3, F_SETFD, 0 <unfinished ...>
[pid 30910] 13:54:09.581958 <... fcntl resumed> ) = 0

Then comes the execve from 30910, with no intervening thread exit or the like.
But of course during the execve the thread 30911 is killed:

[pid 30910] 13:54:19.582600 execve("/suse/matz/lock-strangeness",
["/suse/matz/lock-strangeness", "3"], [/* 0 vars */] <unfinished ...>
[pid 30911] 13:54:19.583422 +++ exited with 0 +++
13:54:19.583749 <... execve resumed> )  = 0
13:54:19.583845 brk(0)                  = 0xc0c000

So, 30910 is alone again and is executing, loading the shared libs, relocating
and so on.  The first action done to FD 3 (retained over the execve) is:

13:54:19.588016 fcntl(3, F_GETLK, {type=F_UNLCK, whence=SEEK_SET, start=0,
len=42, pid=0}) = 0
13:54:19.588101 fcntl(3, F_GETFD)       = 0
13:54:19.588480 fcntl(3, F_SETFD, FD_CLOEXEC) = 0

Bleah!  F_UNLCK we get, which isn't what we're supposed to get.

As there are no intervening syscalls that could in any way have removed the
lock
(in fact there are none other to file descriptor 3) it can't be glibc or
libpthread (or anything else userspace is doing) that would have caused the
lock to be removed.  It's the kernel itself, and hence a bug therein.

If I may hazard a guess it would be that the forced exits of all non-main
threads done by the kernel somehow lead to cleaning up the locks, possibly
because the actions that need doing during fork(2) (which also includes
other-threads-exit, but also lock-removal) are conflated with the actions that
need happen during execve(2) (which must _not_ include lock-removel).

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] rpc: switch virtlockd and virtlogd to use single-threaded dispatch
Posted by Daniel P. Berrangé, 15 weeks ago
On Tue, Mar 06, 2018 at 04:46:05PM -0700, Jim Fehlig wrote:
> On 03/06/2018 10:58 AM, Daniel P. Berrangé wrote:
> > Currently both virtlogd and virtlockd use a single worker thread for
> > dispatching RPC messages. Even this is overkill and their RPC message
> > handling callbacks all run in short, finite time and so blocking the
> > main loop is not an issue like you'd see in libvirtd with long running
> > QEMU commands.
> > 
> > By setting max_workers==0, we can turn off the worker thread and run
> > these daemons single threaded. This in turn fixes a serious problem in
> > the virtlockd daemon whereby it looses all fcntl() locks at re-exec due
> > to multiple threads existing. fcntl() locks only get preserved if the
> > process is single threaded at time of exec().
> 
> I suppose this change has no affect when e.g. starting many domains in
> parallel when locking is enabled. Before the change, there's still only one
> worker thread to process requests.
> 
> I've tested the series and locks are now preserved across re-execs of
> virtlockd. Question is whether we want this change or pursue fixing the
> underlying kernel bug?
> 
> FYI, via the non-public bug I asked a glibc maintainer about the lost lock
> behavior. He agreed it is a kernel bug and posted the below comment to the
> bug.
> 
> Regards,
> Jim
> 
> First, I agree that POSIX file record locks (i.e. the fcntl F_SETLK ones, which
> you're using) _are_ to be preserved over execve (absent any FD_CLOEXEC of
> course, which you aren't using).  (Relevant quote from fcntl(2):
> 
>        Record locks are not inherited by  a  child  created  via  fork(2),
>        but  are  preserved  across  an execve(2).
> 
> Second I agree that the existence or non-existence of threads must not play
> a role in the above.

I've asked some Red Hat experts too and they suggest it looks like a kernel
bug. The question is whether this is a recent kernel regression, that is easily
fixed, or whether its a long term problem.

I've at least verified that this broken behaviour existed in RHEL-7 (but its
possible it was backported when OFD locks were implemented). I still want to
test RHEL-6 and RHEL-5 to see if this problem goes back indefinitely.

My inclination though is that we'll need to work around the problem in
libvirt regardless.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] rpc: switch virtlockd and virtlogd to use single-threaded dispatch
Posted by Daniel P. Berrangé, 15 weeks ago
On Wed, Mar 07, 2018 at 10:10:29AM +0000, Daniel P. Berrangé wrote:
> On Tue, Mar 06, 2018 at 04:46:05PM -0700, Jim Fehlig wrote:
> > On 03/06/2018 10:58 AM, Daniel P. Berrangé wrote:
> > > Currently both virtlogd and virtlockd use a single worker thread for
> > > dispatching RPC messages. Even this is overkill and their RPC message
> > > handling callbacks all run in short, finite time and so blocking the
> > > main loop is not an issue like you'd see in libvirtd with long running
> > > QEMU commands.
> > > 
> > > By setting max_workers==0, we can turn off the worker thread and run
> > > these daemons single threaded. This in turn fixes a serious problem in
> > > the virtlockd daemon whereby it looses all fcntl() locks at re-exec due
> > > to multiple threads existing. fcntl() locks only get preserved if the
> > > process is single threaded at time of exec().
> > 
> > I suppose this change has no affect when e.g. starting many domains in
> > parallel when locking is enabled. Before the change, there's still only one
> > worker thread to process requests.
> > 
> > I've tested the series and locks are now preserved across re-execs of
> > virtlockd. Question is whether we want this change or pursue fixing the
> > underlying kernel bug?
> > 
> > FYI, via the non-public bug I asked a glibc maintainer about the lost lock
> > behavior. He agreed it is a kernel bug and posted the below comment to the
> > bug.
> > 
> > Regards,
> > Jim
> > 
> > First, I agree that POSIX file record locks (i.e. the fcntl F_SETLK ones, which
> > you're using) _are_ to be preserved over execve (absent any FD_CLOEXEC of
> > course, which you aren't using).  (Relevant quote from fcntl(2):
> > 
> >        Record locks are not inherited by  a  child  created  via  fork(2),
> >        but  are  preserved  across  an execve(2).
> > 
> > Second I agree that the existence or non-existence of threads must not play
> > a role in the above.
> 
> I've asked some Red Hat experts too and they suggest it looks like a kernel
> bug. The question is whether this is a recent kernel regression, that is easily
> fixed, or whether its a long term problem.
> 
> I've at least verified that this broken behaviour existed in RHEL-7 (but its
> possible it was backported when OFD locks were implemented). I still want to
> test RHEL-6 and RHEL-5 to see if this problem goes back indefinitely.

I've checked RHEL6 & RHEL5 and both are affected, so this a long time Linux
problem, and so we'll need to workaround it.

FYI I've got kernel bug open here to track it from RHEL side:

  https://bugzilla.redhat.com/show_bug.cgi?id=1552621

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] rpc: switch virtlockd and virtlogd to use single-threaded dispatch
Posted by Jim Fehlig, 15 weeks ago
On 03/07/2018 06:07 AM, Daniel P. Berrangé wrote:
> On Wed, Mar 07, 2018 at 10:10:29AM +0000, Daniel P. Berrangé wrote:
>> On Tue, Mar 06, 2018 at 04:46:05PM -0700, Jim Fehlig wrote:
>>> On 03/06/2018 10:58 AM, Daniel P. Berrangé wrote:
>>>> Currently both virtlogd and virtlockd use a single worker thread for
>>>> dispatching RPC messages. Even this is overkill and their RPC message
>>>> handling callbacks all run in short, finite time and so blocking the
>>>> main loop is not an issue like you'd see in libvirtd with long running
>>>> QEMU commands.
>>>>
>>>> By setting max_workers==0, we can turn off the worker thread and run
>>>> these daemons single threaded. This in turn fixes a serious problem in
>>>> the virtlockd daemon whereby it looses all fcntl() locks at re-exec due
>>>> to multiple threads existing. fcntl() locks only get preserved if the
>>>> process is single threaded at time of exec().
>>>
>>> I suppose this change has no affect when e.g. starting many domains in
>>> parallel when locking is enabled. Before the change, there's still only one
>>> worker thread to process requests.
>>>
>>> I've tested the series and locks are now preserved across re-execs of
>>> virtlockd. Question is whether we want this change or pursue fixing the
>>> underlying kernel bug?
>>>
>>> FYI, via the non-public bug I asked a glibc maintainer about the lost lock
>>> behavior. He agreed it is a kernel bug and posted the below comment to the
>>> bug.
>>>
>>> Regards,
>>> Jim
>>>
>>> First, I agree that POSIX file record locks (i.e. the fcntl F_SETLK ones, which
>>> you're using) _are_ to be preserved over execve (absent any FD_CLOEXEC of
>>> course, which you aren't using).  (Relevant quote from fcntl(2):
>>>
>>>         Record locks are not inherited by  a  child  created  via  fork(2),
>>>         but  are  preserved  across  an execve(2).
>>>
>>> Second I agree that the existence or non-existence of threads must not play
>>> a role in the above.
>>
>> I've asked some Red Hat experts too and they suggest it looks like a kernel
>> bug. The question is whether this is a recent kernel regression, that is easily
>> fixed, or whether its a long term problem.
>>
>> I've at least verified that this broken behaviour existed in RHEL-7 (but its
>> possible it was backported when OFD locks were implemented). I still want to
>> test RHEL-6 and RHEL-5 to see if this problem goes back indefinitely.
> 
> I've checked RHEL6 & RHEL5 and both are affected, so this a long time Linux
> problem, and so we'll need to workaround it.

We have some vintage distros around for long term support and I managed to 
"bisect" the problem a bit: The reproducer works on kernel 2.6.16 but breaks on 
2.6.32.

> FYI I've got kernel bug open here to track it from RHEL side:
> 
>    https://bugzilla.redhat.com/show_bug.cgi?id=1552621

Thanks!

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] rpc: switch virtlockd and virtlogd to use single-threaded dispatch
Posted by Jim Fehlig, 15 weeks ago
On 03/06/2018 10:58 AM, Daniel P. Berrangé wrote:
> Currently both virtlogd and virtlockd use a single worker thread for
> dispatching RPC messages. Even this is overkill and their RPC message
> handling callbacks all run in short, finite time and so blocking the
> main loop is not an issue like you'd see in libvirtd with long running
> QEMU commands.
> 
> By setting max_workers==0, we can turn off the worker thread and run
> these daemons single threaded. This in turn fixes a serious problem in
> the virtlockd daemon whereby it looses all fcntl() locks at re-exec due
> to multiple threads existing. fcntl() locks only get preserved if the
> process is single threaded at time of exec().
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   src/locking/lock_daemon.c | 4 ++--
>   src/logging/log_daemon.c  | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)

Given the outcome of discussions

Reviewed-by: Jim Fehlig <jfehlig@suse.com>

Regards,
Jim

> 
> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
> index 79ab90fc91..7afff42246 100644
> --- a/src/locking/lock_daemon.c
> +++ b/src/locking/lock_daemon.c
> @@ -165,7 +165,7 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool privileged)
>           goto error;
>   
>       if (!(srv = virNetServerNew("virtlockd", 1,
> -                                1, 1, 0, config->max_clients,
> +                                0, 0, 0, config->max_clients,
>                                   config->max_clients, -1, 0,
>                                   NULL,
>                                   virLockDaemonClientNew,
> @@ -180,7 +180,7 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool privileged)
>       srv = NULL;
>   
>       if (!(srv = virNetServerNew("admin", 1,
> -                                1, 1, 0, config->admin_max_clients,
> +                                0, 0, 0, config->admin_max_clients,
>                                   config->admin_max_clients, -1, 0,
>                                   NULL,
>                                   remoteAdmClientNew,
> diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
> index d54d26ab9d..35d7ebb6d2 100644
> --- a/src/logging/log_daemon.c
> +++ b/src/logging/log_daemon.c
> @@ -154,7 +154,7 @@ virLogDaemonNew(virLogDaemonConfigPtr config, bool privileged)
>           goto error;
>   
>       if (!(srv = virNetServerNew("virtlogd", 1,
> -                                1, 1, 0, config->max_clients,
> +                                0, 0, 0, config->max_clients,
>                                   config->max_clients, -1, 0,
>                                   NULL,
>                                   virLogDaemonClientNew,
> @@ -169,7 +169,7 @@ virLogDaemonNew(virLogDaemonConfigPtr config, bool privileged)
>       srv = NULL;
>   
>       if (!(srv = virNetServerNew("admin", 1,
> -                                1, 1, 0, config->admin_max_clients,
> +                                0, 0, 0, config->admin_max_clients,
>                                   config->admin_max_clients, -1, 0,
>                                   NULL,
>                                   remoteAdmClientNew,
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] rpc: switch virtlockd and virtlogd to use single-threaded dispatch
Posted by John Ferlan, 15 weeks ago

On 03/06/2018 12:58 PM, Daniel P. Berrangé wrote:
> Currently both virtlogd and virtlockd use a single worker thread for
> dispatching RPC messages. Even this is overkill and their RPC message
> handling callbacks all run in short, finite time and so blocking the
> main loop is not an issue like you'd see in libvirtd with long running
> QEMU commands.
> 
> By setting max_workers==0, we can turn off the worker thread and run
> these daemons single threaded. This in turn fixes a serious problem in
> the virtlockd daemon whereby it looses all fcntl() locks at re-exec due

s/looses/loses

> to multiple threads existing. fcntl() locks only get preserved if the
> process is single threaded at time of exec().
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/locking/lock_daemon.c | 4 ++--
>  src/logging/log_daemon.c  | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list