On Mon, Jan 27, 2014 at 04:50:56PM -0500, Stefan Berger wrote:
On 01/27/2014 12:18 PM, Daniel P. Berrange wrote:
The nwfilter conf update mutex previously serialized updates to the internal data structures for firewall rules, and updates to the firewall itself. Since the
Hm, wasn't aware (anymore) of this double-purpose.
It wasn't entirely clear to me either, but I am right in believing that it isn't safe to have multiple threads all creating iptables rules for different VMs at the same time, aren't I ?
I also hadn't looked at this patch in the first round...
former is going to be turned into a read/write lock instead of a mutex, a new lock is required to serialize access to the firewall itself.
With this new lock, the lock ordering rules will be for virNWFilter{Define,Undefine}
1. nwfilter driver lock 2. nwfilter update lock
Insert: 3. nwfilter callback drivers lock
This is then used in this order also by nwfilterStateReload
3. virt driver lock 4. domain object lock 5. gentech driver lock
and VM start
1. nwfilter update lock 2. virt driver lock 3. domain object lock 4. gentech driver lock
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/nwfilter/nwfilter_driver.c | 4 +++- src/nwfilter/nwfilter_gentech_driver.c | 19 +++++++++++++++++-- src/nwfilter/nwfilter_gentech_driver.h | 2 +- 3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 89913cf8..d500963 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -936,6 +943,7 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int rc;
virNWFilterLockFilterUpdates(); + virMutexLock(&updateMutex);
Since the filter updates lock had the two purposes before, you are now introducing a separate lock to assign a purpose to each lock. Further below you are preventing concurrent teardowns to this here.
I am wondering how much further down this lock here could actually be pushed. This and the other function (virNWFilterInstantiateFilterLate) where you place this lock are calling __virNWFilterInstantiateFilter and nothing else calls that function [and the filter read protection above the lock call will remain]. So I think this lock could be placed inside __virNWFilterInstantiateFilter(). Also looking at that function I am not sure whether there is anything worth protecting using this newly introduced lock then. It ends up calling virNWFilterInstantiate(). Here I would be a bit careful with the threads being started to learn the IP addresses. So maybe this function could be the place where to serialize access. What's your take?
The callgraph showed the three entry points into this area of code look like: virNWFilterInstantiateFilterLate virNWFilterInstantiateFilter virNWFilterTeardownFilter | | | +-------------------------|-----+ | | +------------+ | | | | | | V V V V _virNWFilterInstantiateFilter _virNWFilterTeardownFilter Looking at the code I don't see how it is safe to allow teardown to happen in parallel with instantiate. The teardown code could confuse and conflict with the instantiate code causing it to fail I believe. In particular a VM could exit causing QEMU to request teardown, while the DHCP snoop thread is in the middle of re-creating the iptables ruleset for a VM, so we surely require serialization of modifications to iptables. I could, push the locking down one level, but it wouldn't change the level of serialization, and the lock calls are clearer at the level of the code I have them I believe. 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 :|