[libvirt] [PATCH 3/5] Push nwfilter update locking up to top level

[Daniel P. Berrange]

On Thu, Jan 23, 2014 at 04:13:57PM -0500, Stefan Berger wrote:
> On 01/23/2014 07:37 AM, Daniel P. Berrange wrote:
> 
> Makes sense... there are other paths as well:
> 
> - SIGHUP or restart of libvirt that recreates all filters
> - late instantiation of filters following detection of VM's IP address

Ok, yes, I see those now.

I was struggling to understand just what codepaths could
result in __virNWFilterInstantiateFilter being invoked
so I generated a callgraph for that function

  http://berrange.fedorapeople.org/nwfilter.ps

tracing the calls, confirms there are the 6 entry points

 - filter define
 - filter undefine
 - state reload on sighup or dbus notify
 - vm startup / hotplug
 - vm shutdown / hotunplug
 - dhcp / ip snooping

the first 3 will require write locks, while the last three will only
require read locks.  I'm squashing the conversion to read/write lock
into this patch, since otherwise the intermediate state would be
deadlock prone.

> 
> src/conf/nwfilter_conf.c::virNWFilterObjLoad : I don't see this
> function grabbing the lock. I think this is missing.

virNWFilterObjLoad is called by StateInitialize or StateReload
and only the reload function requires the lock. So I'm adding
the lock to that function


> >diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
> >index 89913cf8..f0e78ed 100644
> >--- a/src/nwfilter/nwfilter_gentech_driver.c
> >+++ b/src/nwfilter/nwfilter_gentech_driver.c
> >@@ -935,7 +935,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
> >      int ifindex;
> >      int rc;
> >
> >-    virNWFilterLockFilterUpdates();
> >
> >      /* after grabbing the filter update lock check for the interface; if
> >         it's not there anymore its filters will be or are being removed
> >@@ -964,7 +963,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
> >                                          foundNewFilter);
> >
> >  cleanup:
> >-    virNWFilterUnlockFilterUpdates();
> >
> >      return rc;
> >  }
> 
> This function is called by virNWFilterInstantiateFilter and
> virNWFilterUpdateInstantiateFilter. So, in the former case this is
> called when a VM is started, in the latter case if VMs' filters are
> updated while the VM is running. I think you are covering the VM
> creation case with the above calls for lxc and further below with
> the changes to the qemu driver and the uml driver.
> 
> There is at least one other part that needs to be covered:
> 
> src/conf/nwfilter_conf.c::virNWFilterInstFiltersOnAllVMs :: kicked
> off by nwfilterStateReload upon SIGHUP. We need a lock there now.
> 
> src/conf/nwfilter_conf.c::virNWFilterTriggerVMFilterRebuild::
>     - called by virNWFilterTestUnassignDef:
>         - called by src/nwfilter/nwfilter/nwfilterUndefine:: You
> seem to just reorder some locks there; now we need a (writer) lock
> there
>     - called by virNWFilterObjAssignDef: which must be called with
> lock called following above reasoning


Yep, I concur and have added lock calls to the StateReload function

> >@@ -984,8 +982,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,
> >      int rc;
> >      bool foundNewFilter = false;
> >
> >-    virNWFilterLockFilterUpdates();
> >-
> >      rc = __virNWFilterInstantiateFilter(driver,
> >                                          vmuuid,
> >                                          true,
> >@@ -1009,8 +1005,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,
> >          }
> >      }
> >
> >-    virNWFilterUnlockFilterUpdates();
> >-
> >      return rc;
> >  }
> 
> This function here is called by
> src/nwfilter/nwfilter_learnip.c::learnIPAddressThread
> src/nwfilter/nwfilter_dhcpsnoop.c::virNWFilterSnoopIPLeaseInstallRule
> 
> They instantiate the filters once a VM's IP address has been
> detected. So this is where the *Late() comes from.
> 
> If you remove the locking from here, you have to lock it there.
> Considering what you do layer, I would keep the lock here and
> convert this into a reader lock layer on.

Yes, now I'm squashing the read/write lock conversion in, I'll
keep the locking in this location.

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 :|