From owner-freebsd-geom@FreeBSD.ORG Fri Apr 9 09:40:39 2010 Return-Path: Delivered-To: freebsd-geom@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 40D7C1065672 for ; Fri, 9 Apr 2010 09:40:39 +0000 (UTC) (envelope-from jh@FreeBSD.org) Received: from gw02.mail.saunalahti.fi (gw02.mail.saunalahti.fi [195.197.172.116]) by mx1.freebsd.org (Postfix) with ESMTP id C9B4F8FC12 for ; Fri, 9 Apr 2010 09:40:38 +0000 (UTC) Received: from a91-153-117-195.elisa-laajakaista.fi (a91-153-117-195.elisa-laajakaista.fi [91.153.117.195]) by gw02.mail.saunalahti.fi (Postfix) with SMTP id 5E9631395EE for ; Fri, 9 Apr 2010 12:40:36 +0300 (EEST) Date: Fri, 9 Apr 2010 12:40:36 +0300 From: Jaakko Heinonen To: freebsd-geom@FreeBSD.org Message-ID: <20100409094035.GA949@a91-153-117-195.elisa-laajakaista.fi> References: <20100405092611.GA1948@a91-153-117-195.elisa-laajakaista.fi> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100405092611.GA1948@a91-153-117-195.elisa-laajakaista.fi> User-Agent: Mutt/1.5.20 (2009-06-14) Cc: Subject: Re: GEOM class unload deadlock X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Apr 2010 09:40:39 -0000 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