[libvirt] [PATCH] [RFC] nwfilter: resolve deadlock between VM operations and filter update

[Daniel Veillard]

On Wed, Oct 06, 2010 at 05:56:36PM -0400, Stefan Berger wrote:
>  V2:
>   remove the locks from qemudVMFilterRebuild & umlVMFilterRebuild
[...]
> So, the following code paths exist towards
> qemu_driver:qemudVMFilterRebuild where we now want to put a
> qemu_driver lock in front of the filter lock.
> 
> -> nwfilterUndefine()   [ locks the filter ]
>     -> virNWFilterTestUnassignDef()
>         -> virNWFilterTriggerVMFilterRebuild()
>             -> qemudVMFilterRebuild()
> 
> -> nwfilterDefine()
>     -> virNWFilterPoolAssignDef() [ locks the filter ]
>         -> virNWFilterTriggerVMFilterRebuild()
>             -> qemudVMFilterRebuild()
> 
> -> nwfilterDriverReload()
>     -> virNWFilterPoolLoadAllConfigs()
>         ->virNWFilterPoolObjLoad()
>             -> virNWFilterPoolAssignDef() [ locks the filter ]
>                 -> virNWFilterTriggerVMFilterRebuild()
>                     -> qemudVMFilterRebuild()
> 
> -> nwfilterDriverStartup()
>     -> virNWFilterPoolLoadAllConfigs()
>         ->virNWFilterPoolObjLoad()
>             -> virNWFilterPoolAssignDef() [ locks the filter ]
>                 -> virNWFilterTriggerVMFilterRebuild()
>                     -> qemudVMFilterRebuild()
> 
> Qemu is not the only driver using the nwfilter driver, but also the
> UML driver calls into it. Therefore qemuVMFilterRebuild() can be
> exchanged with umlVMFilterRebuild() along with the driver lock of
> qemu_driver that can now be a uml_driver. Further, since UML and
> Qemu domains can be running on the same machine, the triggering of a
> rebuild of the filter can touch both types of drivers and their
> domains.

  okay

> In the patch below I am now extending each nwfilter callback driver
> with functions for locking and unlocking the (VM) driver (UML, QEMU)
> and introduce new functions for locking all registered callback
> drivers and unlocking them. Then I am distributing the
> lock-all-cbdrivers/unlock-all-cbdrivers call into the above call
> paths. The last shown callpath starting with nwfilterDriverStart()
> is problematic since it is initialize before the Qemu and UML drives
> are and thus a lock in the path would result in a NULL pointer
> attempted to be locked -- the call to
> virNWFilterTriggerVMFilterRebuild() is never called, so we never
> lock either the qemu_driver or the uml_driver in that path.
> Therefore, only the first 3 paths now receive calls to lock and
> unlock all callback drivers. Now that the locks are distributed
> where it matters I can remove the qemu_driver and uml_driver lock
> from qemudVMFilterRebuild() and umlVMFilterRebuild() and not
> requiring the recursive locks.

  okay,

> For now I want to put this out as an RFC patch. I have tested it by
> 'stretching' the critical section after the define/undefine
> functions each lock the filter so I can (easily) concurrently
> execute another VM operation (suspend,start). That code is in this
> patch and if you want you can de-activate it. It seems to work ok
> and operations are being blocked while the update is being done.
> I still also want to verify the other assumption above that locking
> filter and qemu_domain always has a preceding qemu_driver lock.

  I looked at the patch and the only thing that need to be cleaned up
is the stretching blocki below, otherwise looks fine to me.

> 
> +if (true) {
> +    fprintf(stderr,"sleep in %s\n", __FUNCTION__);
> +    sleep(CRITICAL_SECTION_STRETCH);
> +    fprintf(stderr,"continue in %s\n", __FUNCTION__);
> +}
> +

  It would be good to have some ways to exercise all code paths
in the locking (okay error handling specific paths aside because
it's really hard), before applying.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/