Re: [libvirt] [PATCH v5 2/3] nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable

On 02/08/2018 08:13 AM, Michal Privoznik wrote: > On 02/06/2018 08:20 PM, John Ferlan wrote: >> Implement the self locking object list for nwfilter object lists >> that uses two hash tables to store the nwfilter object by UUID or >> by Name. >> >> As part of this alter the uuid argument to virNWFilterObjLookupByUUID >> to expect an already formatted uuidstr. >> >> Alter the existing list traversal code to implement the hash table >> find/lookup/search functionality. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/conf/virnwfilterobj.c | 405 ++++++++++++++++++++++++++++------------- >> src/conf/virnwfilterobj.h | 2 +- >> src/nwfilter/nwfilter_driver.c | 5 +- >> 3 files changed, 283 insertions(+), 129 deletions(-) > > >> @@ -425,24 +483,26 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, >> return NULL; >> } >> >> - >> - /* Get a READ lock and immediately promote to WRITE while we adjust >> - * data within. */ >> - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) { >> + /* We're about to make some changes to objects on the list - so get the >> + * list READ lock in order to Find the object and WRITE lock the object >> + * since both paths would immediately promote it anyway */ >> + virObjectRWLockRead(nwfilters); >> + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) { >> + virObjectRWLockWrite(obj); >> + virObjectRWUnlock(nwfilters); >> >> objdef = obj->def; >> if (virNWFilterDefEqual(def, objdef)) { >> - virNWFilterObjPromoteToWrite(obj); >> virNWFilterDefFree(objdef); >> obj->def = def; >> virNWFilterObjDemoteFromWrite(obj); >> return obj; >> } >> >> - /* Set newDef and run the trigger with a demoted lock since it may need >> - * to grab a read lock on this object and promote before returning. */ >> - virNWFilterObjPromoteToWrite(obj); >> obj->newDef = def; >> + >> + /* Demote while the trigger runs since it may need to grab a read >> + * lock on this object and promote before returning. */ >> virNWFilterObjDemoteFromWrite(obj); >> >> /* trigger the update on VMs referencing the filter */ >> @@ -462,39 +522,113 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, >> return obj; >> } >> >> + /* Promote the nwfilters to add a new object */ >> + virObjectRWUnlock(nwfilters); >> + virObjectRWLockWrite(nwfilters); > > How can this work? What if there's another thread waiting for the write > lock (e.g. just to define an NWFilter)? The Unlock(nwfilters) wakes up > the other thread, it does its job, unlocks the list waking us in turn. > So we lock @nwfilters for writing and add something into the hash table > - without all the checks (e.g. duplicity check) done earlier. > I dunno - works in the tests I've run (libvirt-tck and avocado).