[libvirt] [PATCH 1/2] qemu: Add missing lock of virDomainObj before calling virDomainUnref

[Daniel P. Berrange]

On Fri, Mar 04, 2011 at 06:03:36PM -0700, Eric Blake wrote:
> On 03/04/2011 02:46 PM, Eric Blake wrote:
> >> +++ b/src/qemu/qemu_process.c
> >> @@ -596,7 +596,9 @@ static void qemuProcessHandleMonitorDestroy(qemuMonitorPtr mon,
> >>      qemuDomainObjPrivatePtr priv = vm->privateData;
> >>      if (priv->mon == mon)
> >>          priv->mon = NULL;
> >> -    virDomainObjUnref(vm);
> >> +    virDomainObjLock(vm);
> >> +    if (virDomainObjUnref(vm) > 0)
> >> +        virDomainObjUnlock(vm);
> > 
> > Phooey, this causes 'virsh save domain file' to deadlock.
> > 
> > I'm still looking into why, but it may be that we're going about fixing
> > this wrong.  Maybe we should be making sure that qemuProcessStop ensures
> > that the monitor callback function won't trigger if there is no domain
> > for it to trigger on.
> 
> I think the real reason for the deadlock is that:
> 
> qemudDomainSaveFlag holds the domain lock while it calls
> qemuDomainEventQueue, which requests the event loop lock
> 
> the event loop thread holds the event loop lock, and can make several
> different callbacks that can result in requesting a domain lock

I don't think this can be the deadlock scenario. The event loop lock
is completely isolated from other locks, because it is only ever held
while code in event.c is executing. It is always released when invoking
any external callbacks, so you can't have any lock ordering dependencies
wrt the event loop lock.

Regards,
Daniel
--
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|