[libvirt] libvirtd deadlock on shutdown

[Jim Fehlig]

Daniel P. Berrange wrote:
> On Thu, Jun 21, 2012 at 09:56:54AM -0600, Jim Fehlig wrote:
>   
>
>> From 583be33213e922899b23f036494886397b2549dc Mon Sep 17 00:00:00 2001
>> From: Jim Fehlig <jfehlig at suse.com>
>> Date: Thu, 21 Jun 2012 09:21:44 -0600
>> Subject: [PATCH] Fix deadlock on libvirtd shutdown
>>
>> When shutting down libvirtd, the virNetServer shutdown can deadlock
>> if there are in-flight jobs being handled by virNetServerHandleJob().
>> virNetServerFree() will acquire the virNetServer lock and call
>> virThreadPoolFree() to terminate the workers, waiting for the workers
>> to finish.  But in-flight workers will attempt to acquire the
>> virNetServer lock, resulting in deadlock.
>>
>> Fix the deadlock by unlocking the virNetServer lock before calling
>> virThreadPoolFree().  This is safe since the virNetServerPtr object
>> is ref-counted and only decrementing the ref count needs to be
>> protected.  Additionally, there is no need to re-acquire the lock
>> after virThreadPoolFree() completes as all the workers have
>> terminated.
>> ---
>>  src/rpc/virnetserver.c |    6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
>> index ae19e84..9d71e53 100644
>> --- a/src/rpc/virnetserver.c
>> +++ b/src/rpc/virnetserver.c
>> @@ -766,10 +766,9 @@ void virNetServerFree(virNetServerPtr srv)
>>      virNetServerLock(srv);
>>      VIR_DEBUG("srv=%p refs=%d", srv, srv->refs);
>>      srv->refs--;
>> -    if (srv->refs > 0) {
>> -        virNetServerUnlock(srv);
>> +    virNetServerUnlock(srv);
>> +    if (srv->refs > 0)
>>          return;
>> -    }
>>  
>>      for (i = 0 ; i < srv->nservices ; i++)
>>          virNetServerServiceToggle(srv->services[i], false);
>> @@ -805,7 +804,6 @@ void virNetServerFree(virNetServerPtr srv)
>>      virNetServerMDNSFree(srv->mdns);
>>  #endif
>>  
>> -    virNetServerUnlock(srv);
>>      virMutexDestroy(&srv->lock);
>>      VIR_FREE(srv);
>>  }
>>     
>
> ACK,
>
> We really should resurrect the patches adding virAtomic for refcounting :-)
>   

Thanks for the help with this.  I've pushed it now.

Regards,
Jim