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

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.

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(a)redhat.com&gt; >--- > 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?