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

[Stefan Berger]

On 02/09/2018 01:48 AM, Michal Privoznik wrote:
> On 02/08/2018 10:13 PM, Stefan Berger wrote:
>> 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 at redhat.com>
>>>> ---
>>>>    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.
>> Could we keep the read lock and grab a 2nd lock as a write-lock? It
>> wouldn't be a virObject call, though.
> That is not possible because write & read lock must exclude each other
> by definition.

We can grab lock A and then lock B. Lock B would either be a read/write 
lock locked as a write lock or would be an exclusive lock. Why would 
that not work?

    Stefan

>
> Michal
>