[libvirt] libvirtd deadlock on shutdown

[Jim Fehlig]

Daniel P. Berrange wrote:
> On Wed, Jun 20, 2012 at 03:07:16PM -0600, Jim Fehlig wrote:
>   
>> I'm looking into a libvirtd deadlock on daemon shutdown.  The deadlock
>> occurs when shutting down virNetServer.  If the handling of a job is in
>> flight in virNetServerHandleJob(), the virNetServer lock is acquired
>> when freeing job->prog (src/rpc/virnetserver.c:167).  But the lock is
>> already held in virNetServerFree(), which is blocked in
>> virThreadPoolFree() waiting for all the workers to finish.  No progress
>> can be made.
>>
>> The attached hack fixes the problem, but I'm not convinced this is an
>> appropriate fix.  Is it necessary to hold the virNetServer lock when
>> calling virNetServerProgramFree(job->prog)?  I notice the lock is not
>> held in the error path of virNetServerHandleJob().
>>
>> Thanks,
>> Jim
>>
>>     
>
>   
>> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
>> index ae19e84..edd3196 100644
>> --- a/src/rpc/virnetserver.c
>> +++ b/src/rpc/virnetserver.c
>> @@ -774,7 +774,9 @@ void virNetServerFree(virNetServerPtr srv)
>>      for (i = 0 ; i < srv->nservices ; i++)
>>          virNetServerServiceToggle(srv->services[i], false);
>>  
>> +    virNetServerUnlock(srv);
>>      virThreadPoolFree(srv->workers);
>> +    virNetServerLock(srv);
>>  
>>      for (i = 0 ; i < srv->nsignals ; i++) {
>>          sigaction(srv->signals[i]->signum, &srv->signals[i]->oldaction, NULL);
>>     
>
> The virNetServerPtr object is ref-counted, so technical;ly
> only decrementing the ref count needs to be protected. Once
> the ref count hits 0, then only the current thread (and the
> workers) should be using the virNetServerPtr instance.
>   

Ah, right.  Thanks for clarifying.

> So, yes, it is safe to call virNetServerUnlock before
> virThreadPoolFree. Furthermore, it is /not/ required
> to call virNetServerLock afterwards - no other thread
> should be using it now the workers are dead. So you
> can skip that extra lock call, and also remove the
> unlock call much later
>   

Something like the attached patch?

Regards,
Jim

-------------- next part --------------
A non-text attachment was scrubbed...
Name: libvirtd-deadlock.patch
Type: text/x-patch
Size: 1767 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120621/96f319bd/attachment-0001.bin>