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>

diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
index 7bbf1e12fb9a9b8d1dedcabba21579de0b91d57c..9f4a755252b995991e10fdba1801f8cf7517b769 100644 (file)
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -55,19 +55,52 @@ static virNWFilterTechDriver *filter_tech_drivers[] = {
     NULL
 };
 
-/* Serializes instantiation of filters. This is necessary
- * to avoid lock ordering deadlocks. eg virNWFilterInstantiateFilterUpdate
- * 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.
+/*
+ * Serializes instantiation of filters.
+ *
+ * When instantiating a filter, we need to resolve references
+ * to other filters and acquire locks on them. The act of
+ * 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
  *
- * XXX better long term solution is to make virNWFilterObjList use a
- * hash table as is done for virDomainObjList. You can then get
- * lockless lookup of objects by name.
+ * In addition to the asynchronous filter instantiation
+ * triggered by the IP address learning backends.
  */
 static virMutex updateMutex;