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