Re: [PATCH v1 29/31] bridge_driver: Drop networkDriverLock() from almost everywhere

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 26.02.2015 15:17, Michal Privoznik wrote:
> Now that we have fine grained locks, there's no need to lock the
> whole driver. We can rely on self-locking APIs.
> 
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
>  src/network/bridge_driver.c | 45 +++------------------------------------------
>  1 file changed, 3 insertions(+), 42 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index c2a992a..56debb3 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -129,9 +129,7 @@ networkObjFromNetwork(virNetworkPtr net)
>      virNetworkObjPtr network;
>      char uuidstr[VIR_UUID_STRING_BUFLEN];
>  
> -    networkDriverLock();
>      network = virNetworkObjFindByUUID(driver->networks, net->uuid);
> -    networkDriverUnlock();
>  
>      if (!network) {
>          virUUIDFormat(net->uuid, uuidstr);
> @@ -264,6 +262,8 @@ networkRemoveInactive(virNetworkObjPtr net)
>  
>      int ret = -1;
>  
> +    networkDriverLock();
> +
>      /* remove the (possibly) existing dnsmasq and radvd files */
>      if (!(dctx = dnsmasqContextNew(def->name,
>                                     driver->dnsmasqStateDir))) {
> @@ -315,6 +315,7 @@ networkRemoveInactive(virNetworkObjPtr net)
>      VIR_FREE(radvdpidbase);
>      VIR_FREE(statusfile);
>      dnsmasqContextFree(dctx);
> +    networkDriverUnlock();
>      return ret;
>  }

This is not gonna fly alone. Consider this squashed in:

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 4df18f2..7d3132a 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -262,7 +262,10 @@ networkRemoveInactive(virNetworkObjPtr net)

     int ret = -1;

+    virObjectRef(net);
+    virObjectUnlock(net);
     networkDriverLock();
+    virObjectLock(net);

     /* remove the (possibly) existing dnsmasq and radvd files */
     if (!(dctx = dnsmasqContextNew(def->name,
@@ -316,6 +319,7 @@ networkRemoveInactive(virNetworkObjPtr net)
     VIR_FREE(statusfile);
     dnsmasqContextFree(dctx);
     networkDriverUnlock();
+    virObjectUnref(net);
     return ret;
 }


The problem is, if there's already another thread doing undefine, it
already has driver and the network object locked. And as soon as it
calls virNetworkRemoveInactive() (not to be seen in the context though)
a deadlock occurs, because virNetworkRemoveInactive() locks the network
list and objects in the list, one after another. So, one thread already
have locked driver and the network object list and is trying to lock a
network object in the list. However, the other thread has the object
locked and is waiting on the network driver lock. Therefore the diff,
which does the locking in correct order.

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list