[libvirt] [PATCH v2 3/4] Add a mutex to serialize updates to firewall

[Daniel P. Berrange]

On Tue, Jan 28, 2014 at 07:31:04AM -0500, Stefan Berger wrote:
> On 01/28/2014 06:15 AM, Daniel P. Berrange wrote:
> >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 ?
> 
> At least one thread shouldn't try to instantiate filters for one
> (VM) interface while another one tears them down. So that would be
> serialization per interface. I think instantiation of filters could
> be done concurrently for different interfaces, but not the execution
> of the iptables commands themselves, though. The latter is locking
> that needs to be done by the ebtables/iptables driver and that
> driver does serialize the execution of all scripts using the
> execCLIMutex. The ebtables and iptables rules are created on a
> per-interface basis, all hooking into some form of 'root' chains.
> The critical part here is that the 'root' chains can be accessed
> concurrently by different interfaces -- from what I can tell is that
> all the scripts that are run by the eb/iptables driver only need to
> be serialized and that is done with that execCLIMutex. So we should
> be fine from that perspective.
> 
> At least locking on a per-interface basis is already happening in
> the 'gentech' driver:
> 
> 
> nwfilter_gentech_driver.c::virNWFilterInstantiate
> 
> [...]
>         if (virNWFilterLockIface(ifname) < 0)
>             goto err_exit;
> 
>         rc = techdriver->applyNewRules(ifname, nptrs, ptrs);
> 
>         if (teardownOld && rc == 0)
>             techdriver->tearOldRules(ifname);
> 
>         if (rc == 0 && (virNetDevValidateConfig(ifname, NULL,
> ifindex) <= 0)) {
>             virResetLastError();
>             /* interface changed/disppeared */
>             techdriver->allTeardown(ifname);
>             rc = -1;
>         }
> 
>         virNWFilterUnlockIface(ifname);
> [...]
> 
> 
> 
> nwfilter_gentech_driver.c::_virNWFilterTeardownFilter
> 
> [...]
>     if (virNWFilterLockIface(ifname) < 0)
>        return -1;
> 
>     techdriver->allTeardown(ifname);
> 
>     virNWFilterIPAddrMapDelIPAddr(ifname, NULL);
> 
>     virNWFilterUnlockIface(ifname);
> [...]
> 
> 
> (Besides the above calls to the 'techdriver', there are others that
> call into the techdriver during the test phases of a filter updated.
> They hold the writer lock to the filter updates and with this block
> every concurrent thread then.)
> 
> I may be missing something subtle, but I think there is already
> enough serialization happening per interface.

Ok, I think you might be right then, and we can just skip this
patch entirely and rely in the ifname locks.

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