nwfilter: update comment about locking filter updates

The comment against the 'updateMutex' refers to a problem with
lock ordering when looking up filters in the virNWFilterObjList
which uses an array. That problem does indeed exist.

Unfortunately it claims that switching to a hash table would
solve the lock ordering problems during instantiation. That
is not correct because there is a second lock ordering
problem related to how we traverse related filters when
instantiating filters. Consider a set of filters:

  Filter A:
     Reference Filter C
     Reference Filter D

  Filter B:
     Reference Filter D
     Reference Filter C

In one example, we lock A, C, D, in the other example
we lock A, D, C.

Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This commit is contained in:
Daniel P. Berrangé 2022-03-09 17:25:36 +00:00
parent a4947e8f63
commit a19f1e7fc8

View File

@ -55,19 +55,52 @@ static virNWFilterTechDriver *filter_tech_drivers[] = {
NULL NULL
}; };
/* Serializes instantiation of filters. This is necessary /*
* to avoid lock ordering deadlocks. eg virNWFilterInstantiateFilterUpdate * Serializes instantiation of filters.
* will hold a lock on a virNWFilterObj *. This in turn invokes
* virNWFilterDoInstantiate which invokes virNWFilterDetermineMissingVarsRec
* which invokes virNWFilterObjListFindInstantiateFilter. This iterates over
* every single virNWFilterObj *in the list. So if 2 threads try to
* instantiate a filter in parallel, they'll both hold 1 lock at the top level
* in virNWFilterInstantiateFilterUpdate which will cause the other thread
* to deadlock in virNWFilterObjListFindInstantiateFilter.
* *
* XXX better long term solution is to make virNWFilterObjList use a * When instantiating a filter, we need to resolve references
* hash table as is done for virDomainObjList. You can then get * to other filters and acquire locks on them. The act of
* lockless lookup of objects by name. * looking up a filter requires traversing an array, locking
* each filter in turn until we find the one we want.
*
* The mere act of finding a filter by name, while holding
* a lock on another filter, introduces deadlocks due to
* varying lock ordering.
*
* We retain a lock on the referenced filter once found.
* The order in which the locks are acquired depends on
* the order in which filters reference each other.
*
* Filter A:
* Reference Filter C
* Reference Filter D
*
* Filter B:
* Reference Filter D
* Reference Filter C
*
* In one example, we lock A, C, D, in the other example
* we lock A, D, C.
*
* Because C & D are locked in differing orders we are
* once again at risk of deadlocks.
*
* There can be multiple levels of recursion, so it is
* not viable to determine the lock order upfront, it
* has to be done as we traverse the tree.
*
* Thus we serialize any code that needs to traverse
* the filter references.
*
* This covers the following APIs:
*
* virNWFilterDefineXML
* virNWFilterUndefine
* virNWFilterBindingCreate
* virNWFilterBindingDelete
*
* In addition to the asynchronous filter instantiation
* triggered by the IP address learning backends.
*/ */
static virMutex updateMutex; static virMutex updateMutex;