Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 9 Apr 2010 12:40:36 +0300
From:      Jaakko Heinonen <jh@FreeBSD.org>
To:        freebsd-geom@FreeBSD.org
Subject:   Re: GEOM class unload deadlock
Message-ID:  <20100409094035.GA949@a91-153-117-195.elisa-laajakaista.fi>
In-Reply-To: <20100405092611.GA1948@a91-153-117-195.elisa-laajakaista.fi>
References:  <20100405092611.GA1948@a91-153-117-195.elisa-laajakaista.fi>

next in thread | previous in thread | raw e-mail | index | archive | help
On 2010-04-05, Jaakko Heinonen wrote:
> Below is  a patch to fix a possible deadlock between g_unload_class()
> and withering.

Another way to fix the deadlock is to not run g_unload_class() as a GEOM
event. This might a preferred solution for these reasons:

- If there are many geoms which need withering, there will be at least
  one g_unload_class() call per geom with my previous patch. This is
  unnecessary.
- GEOM tracing is cleaner (one g_unload_class() call per unload).

Comments?

---

Fix deadlock between GEOM class unloading and withering. Withering can't
proceed while g_unload_class() blocks the event thread. Fix this by
not running g_unload_class() as a GEOM event and dropping the topology
lock when withering needs to proceed.

PR:		kern/139847

%%%
Index: sys/geom/geom.h
===================================================================
--- sys/geom/geom.h	(revision 206411)
+++ sys/geom/geom.h	(working copy)
@@ -353,6 +353,12 @@ g_free(void *ptr)
 		sx_assert(&topology_lock, SX_UNLOCKED);		\
 	} while (0)
 
+#define g_topology_sleep(chan, timo)				\
+	do {							\
+		sx_sleep(chan, &topology_lock, 0,		\
+		    "gtopol", timo);				\
+	} while (0)
+
 #define DECLARE_GEOM_CLASS(class, name) 			\
 	static moduledata_t name##_mod = {			\
 		#name, g_modevent, &class			\
Index: sys/geom/geom_subr.c
===================================================================
--- sys/geom/geom_subr.c	(revision 206411)
+++ sys/geom/geom_subr.c	(working copy)
@@ -130,43 +130,45 @@ g_load_class(void *arg, int flag)
 	}
 }
 
-static void
-g_unload_class(void *arg, int flag)
+static int
+g_unload_class(struct g_class *mp)
 {
-	struct g_hh00 *hh;
-	struct g_class *mp;
 	struct g_geom *gp;
 	struct g_provider *pp;
 	struct g_consumer *cp;
 	int error;
 
-	g_topology_assert();
-	hh = arg;
-	mp = hh->mp;
+	g_topology_lock();
 	G_VALID_CLASS(mp);
 	g_trace(G_T_TOPOLOGY, "g_unload_class(%s)", mp->name);
 
-	/*
-	 * We allow unloading if we have no geoms, or a class
-	 * method we can use to get rid of them.
-	 */
-	if (!LIST_EMPTY(&mp->geom) && mp->destroy_geom == NULL) {
-		hh->error = EOPNOTSUPP;
-		return;
-	}
-
-	/* We refuse to unload if anything is open */
+retry:
 	LIST_FOREACH(gp, &mp->geom, geom) {
+		/* We refuse to unload if anything is open */
 		LIST_FOREACH(pp, &gp->provider, provider)
 			if (pp->acr || pp->acw || pp->ace) {
-				hh->error = EBUSY;
-				return;
+				g_topology_unlock();
+				return (EBUSY);
 			}
 		LIST_FOREACH(cp, &gp->consumer, consumer)
 			if (cp->acr || cp->acw || cp->ace) {
-				hh->error = EBUSY;
-				return;
+				g_topology_unlock();
+				return (EBUSY);
 			}
+		/* If the geom is withering, wait it to finish. */
+		if (gp->flags & G_GEOM_WITHER) {
+			g_topology_sleep(mp, 1);
+			goto retry;
+		}
+	}
+
+	/*
+	 * We allow unloading if we have no geoms, or a class
+	 * method we can use to get rid of them.
+	 */
+	if (!LIST_EMPTY(&mp->geom) && mp->destroy_geom == NULL) {
+		g_topology_unlock();
+		return (EOPNOTSUPP);
 	}
 
 	/* Bar new entries */
@@ -174,21 +176,28 @@ g_unload_class(void *arg, int flag)
 	mp->config = NULL;
 
 	error = 0;
+	LIST_FOREACH(gp, &mp->geom, geom) {
+		error = mp->destroy_geom(NULL, mp, gp);
+		if (error != 0) {
+			g_topology_unlock();
+			return (error);
+		}
+	}
+	/* Wait withering to finish. */
 	for (;;) {
 		gp = LIST_FIRST(&mp->geom);
 		if (gp == NULL)
 			break;
-		error = mp->destroy_geom(NULL, mp, gp);
-		if (error != 0)
-			break;
-	}
-	if (error == 0) {
-		if (mp->fini != NULL)
-			mp->fini(mp);
-		LIST_REMOVE(mp, class);
-	}
-	hh->error = error;
-	return;
+		KASSERT(gp->flags & G_GEOM_WITHER,
+		   ("Non-withering geom in class %s", mp->name));
+		g_topology_sleep(mp, 1);
+	}
+	if (mp->fini != NULL)
+		mp->fini(mp);
+	LIST_REMOVE(mp, class);
+	g_topology_unlock();
+
+	return (0);
 }
 
 int
@@ -209,12 +218,12 @@ g_modevent(module_t mod, int type, void 
 		g_ignition++;
 		g_init();
 	}
-	hh = g_malloc(sizeof *hh, M_WAITOK | M_ZERO);
-	hh->mp = data;
 	error = EOPNOTSUPP;
 	switch (type) {
 	case MOD_LOAD:
-		g_trace(G_T_TOPOLOGY, "g_modevent(%s, LOAD)", hh->mp->name);
+		g_trace(G_T_TOPOLOGY, "g_modevent(%s, LOAD)", mp->name);
+		hh = g_malloc(sizeof *hh, M_WAITOK | M_ZERO);
+		hh->mp = mp;
 		/*
 		 * Once the system is not cold, MOD_LOAD calls will be
 		 * from the userland and the g_event thread will be able
@@ -232,18 +241,14 @@ g_modevent(module_t mod, int type, void 
 		}
 		break;
 	case MOD_UNLOAD:
-		g_trace(G_T_TOPOLOGY, "g_modevent(%s, UNLOAD)", hh->mp->name);
-		error = g_waitfor_event(g_unload_class, hh, M_WAITOK, NULL);
-		if (error == 0)
-			error = hh->error;
+		g_trace(G_T_TOPOLOGY, "g_modevent(%s, UNLOAD)", mp->name);
+		DROP_GIANT();
+		error = g_unload_class(mp);
+		PICKUP_GIANT();
 		if (error == 0) {
-			KASSERT(LIST_EMPTY(&hh->mp->geom),
-			    ("Unloaded class (%s) still has geom", hh->mp->name));
+			KASSERT(LIST_EMPTY(&mp->geom),
+			    ("Unloaded class (%s) still has geom", mp->name));
 		}
-		g_free(hh);
-		break;
-	default:
-		g_free(hh);
 		break;
 	}
 	return (error);
%%%

-- 
Jaakko



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100409094035.GA949>