Re: [libvirt] [PATCH 3/5] rpc: invoke the message dispatch callback with client unlocked

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(a)redhat.com&gt; > --- > 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(a)redhat.com&gt; 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; > } > } > [...]