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>