From owner-freebsd-geom@freebsd.org Thu May 19 16:31:54 2016 Return-Path: Delivered-To: freebsd-geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 847AFB427A7 for ; Thu, 19 May 2016 16:31:54 +0000 (UTC) (envelope-from bright@mu.org) Received: from mailman.ysv.freebsd.org (mailman.ysv.freebsd.org [IPv6:2001:1900:2254:206a::50:5]) by mx1.freebsd.org (Postfix) with ESMTP id 750021F26 for ; Thu, 19 May 2016 16:31:54 +0000 (UTC) (envelope-from bright@mu.org) Received: by mailman.ysv.freebsd.org (Postfix) id 745C8B427A5; Thu, 19 May 2016 16:31:54 +0000 (UTC) Delivered-To: geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 73D89B427A4; Thu, 19 May 2016 16:31:54 +0000 (UTC) (envelope-from bright@mu.org) Received: from elvis.mu.org (elvis.mu.org [IPv6:2001:470:1f05:b76::196]) by mx1.freebsd.org (Postfix) with ESMTP id 644B91F25; Thu, 19 May 2016 16:31:54 +0000 (UTC) (envelope-from bright@mu.org) Received: from AlfredMacbookAir.local (unknown [IPv6:2601:645:8003:a4d6:1000:9833:bf6a:cc6b]) by elvis.mu.org (Postfix) with ESMTPSA id 9F159346DDE2; Thu, 19 May 2016 09:31:48 -0700 (PDT) Subject: Re: Removing Giant asserts from geom To: Konstantin Belousov , geom@freebsd.org References: <20160519105634.GO89104@kib.kiev.ua> Cc: arch@freebsd.org From: Alfred Perlstein Message-ID: <573DEA73.4080408@mu.org> Date: Thu, 19 May 2016 09:31:47 -0700 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <20160519105634.GO89104@kib.kiev.ua> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 May 2016 16:31:54 -0000 It seems like it should be the opposite, the DROP_GIANTs should be turned into mtx_assert(&Giant, MA_NOTOWNED) as giant is removed from the tree. Meaning Giant should be pushed further back until it is eliminated. Doing as this patch proposes hides that we still have callers holding Giant which is not good. -Alfred On 5/19/16 3:56 AM, Konstantin Belousov wrote: > I propose to remove asserts > mtx_assert(&Giant, MA_NOTOWNED); > from the geom KPI. The asserts do not serve any purposes, at least not > in the current kernel. They might provided some agenda in the 5.x days, > but now they only force filesystems to add cruft to satisfy GEOM KPI > requirements. > > As an example, consider DROP_GIANT/PICKUP_GIANT in the ffs code. VFS is > currently Giant-free, including the mount and unmount path, UFS/FFS are > also Giant-free, but mount and unmount paths have to do the Giant dance > to allow root mount to proceed. This is because startup is executed with > the Giant owned by the thread0. > > Giant asserts are even more ridiculous in the geom code since geom cdev > geom.ctl is marked as needing Giant. And there are vestiges of Giant > around calls to add geom kthreads, which is not need by KPI. Not to note > that Giant is already held there due to startup protocol. > > Any objections ? > > diff --git a/sys/geom/eli/g_eli.c b/sys/geom/eli/g_eli.c > index 403d0b6..422883d 100644 > --- a/sys/geom/eli/g_eli.c > +++ b/sys/geom/eli/g_eli.c > @@ -1229,7 +1229,6 @@ g_eli_shutdown_pre_sync(void *arg, int howto) > int error; > > mp = arg; > - DROP_GIANT(); > g_topology_lock(); > LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) { > sc = gp->softc; > @@ -1245,7 +1244,6 @@ g_eli_shutdown_pre_sync(void *arg, int howto) > } > } > g_topology_unlock(); > - PICKUP_GIANT(); > } > > static void > diff --git a/sys/geom/geom.h b/sys/geom/geom.h > index bf70d0b..7c1d714 100644 > --- a/sys/geom/geom.h > +++ b/sys/geom/geom.h > @@ -369,7 +369,6 @@ g_free(void *ptr) > > #define g_topology_lock() \ > do { \ > - mtx_assert(&Giant, MA_NOTOWNED); \ > sx_xlock(&topology_lock); \ > } while (0) > > diff --git a/sys/geom/geom_event.c b/sys/geom/geom_event.c > index 2ded638..3c2ee49 100644 > --- a/sys/geom/geom_event.c > +++ b/sys/geom/geom_event.c > @@ -83,7 +83,6 @@ g_waitidle(void) > { > > g_topology_assert_not(); > - mtx_assert(&Giant, MA_NOTOWNED); > > mtx_lock(&g_eventlock); > while (!TAILQ_EMPTY(&g_events)) > diff --git a/sys/geom/geom_kern.c b/sys/geom/geom_kern.c > index dbced0f..9f3f120 100644 > --- a/sys/geom/geom_kern.c > +++ b/sys/geom/geom_kern.c > @@ -90,7 +90,6 @@ static void > g_up_procbody(void *arg) > { > > - mtx_assert(&Giant, MA_NOTOWNED); > thread_lock(g_up_td); > sched_prio(g_up_td, PRIBIO); > thread_unlock(g_up_td); > @@ -103,7 +102,6 @@ static void > g_down_procbody(void *arg) > { > > - mtx_assert(&Giant, MA_NOTOWNED); > thread_lock(g_down_td); > sched_prio(g_down_td, PRIBIO); > thread_unlock(g_down_td); > @@ -116,7 +114,6 @@ static void > g_event_procbody(void *arg) > { > > - mtx_assert(&Giant, MA_NOTOWNED); > thread_lock(g_event_td); > sched_prio(g_event_td, PRIBIO); > thread_unlock(g_event_td); > @@ -147,14 +144,12 @@ g_init(void) > g_io_init(); > g_event_init(); > g_ctl_init(); > - mtx_lock(&Giant); > kproc_kthread_add(g_event_procbody, NULL, &g_proc, &g_event_td, > RFHIGHPID, 0, "geom", "g_event"); > kproc_kthread_add(g_up_procbody, NULL, &g_proc, &g_up_td, > RFHIGHPID, 0, "geom", "g_up"); > kproc_kthread_add(g_down_procbody, NULL, &g_proc, &g_down_td, > RFHIGHPID, 0, "geom", "g_down"); > - mtx_unlock(&Giant); > EVENTHANDLER_REGISTER(shutdown_pre_sync, geom_shutdown, NULL, > SHUTDOWN_PRI_FIRST); > } > diff --git a/sys/geom/geom_mbr.c b/sys/geom/geom_mbr.c > index 86ee860..a811e35 100644 > --- a/sys/geom/geom_mbr.c > +++ b/sys/geom/geom_mbr.c > @@ -190,7 +190,6 @@ g_mbr_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct thr > case DIOCSMBR: { > if (!(fflag & FWRITE)) > return (EPERM); > - DROP_GIANT(); > g_topology_lock(); > cp = LIST_FIRST(&gp->consumer); > if (cp->acw == 0) { > @@ -205,7 +204,6 @@ g_mbr_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct thr > if (opened) > g_access(cp, 0, -1 , 0); > g_topology_unlock(); > - PICKUP_GIANT(); > return(error); > } > default: > diff --git a/sys/geom/geom_pc98.c b/sys/geom/geom_pc98.c > index 42c9962..f4435cb 100644 > --- a/sys/geom/geom_pc98.c > +++ b/sys/geom/geom_pc98.c > @@ -176,7 +176,6 @@ g_pc98_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct th > case DIOCSPC98: { > if (!(fflag & FWRITE)) > return (EPERM); > - DROP_GIANT(); > g_topology_lock(); > cp = LIST_FIRST(&gp->consumer); > if (cp->acw == 0) { > @@ -191,7 +190,6 @@ g_pc98_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct th > if (opened) > g_access(cp, 0, -1 , 0); > g_topology_unlock(); > - PICKUP_GIANT(); > return(error); > } > default: > diff --git a/sys/geom/geom_subr.c b/sys/geom/geom_subr.c > index 54a99bf..8517991 100644 > --- a/sys/geom/geom_subr.c > +++ b/sys/geom/geom_subr.c > @@ -247,9 +247,7 @@ g_modevent(module_t mod, int type, void *data) > break; > case MOD_UNLOAD: > 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(&mp->geom), > ("Unloaded class (%s) still has geom", mp->name)); > diff --git a/sys/geom/journal/g_journal.c b/sys/geom/journal/g_journal.c > index 871bd8e4..0678003 100644 > --- a/sys/geom/journal/g_journal.c > +++ b/sys/geom/journal/g_journal.c > @@ -2697,7 +2697,6 @@ g_journal_shutdown(void *arg, int howto __unused) > if (panicstr != NULL) > return; > mp = arg; > - DROP_GIANT(); > g_topology_lock(); > LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) { > if (gp->softc == NULL) > @@ -2706,7 +2705,6 @@ g_journal_shutdown(void *arg, int howto __unused) > g_journal_destroy(gp->softc); > } > g_topology_unlock(); > - PICKUP_GIANT(); > } > > /* > @@ -2725,7 +2723,6 @@ g_journal_lowmem(void *arg, int howto __unused) > > g_journal_stats_low_mem++; > mp = arg; > - DROP_GIANT(); > g_topology_lock(); > LIST_FOREACH(gp, &mp->geom, geom) { > sc = gp->softc; > @@ -2756,7 +2753,6 @@ g_journal_lowmem(void *arg, int howto __unused) > break; > } > g_topology_unlock(); > - PICKUP_GIANT(); > } > > static void g_journal_switcher(void *arg); > @@ -2871,7 +2867,6 @@ g_journal_do_switch(struct g_class *classp) > char *mountpoint; > int error, save; > > - DROP_GIANT(); > g_topology_lock(); > LIST_FOREACH(gp, &classp->geom, geom) { > sc = gp->softc; > @@ -2886,7 +2881,6 @@ g_journal_do_switch(struct g_class *classp) > mtx_unlock(&sc->sc_mtx); > } > g_topology_unlock(); > - PICKUP_GIANT(); > > mtx_lock(&mountlist_mtx); > TAILQ_FOREACH(mp, &mountlist, mnt_list) { > @@ -2901,11 +2895,9 @@ g_journal_do_switch(struct g_class *classp) > continue; > /* mtx_unlock(&mountlist_mtx) was done inside vfs_busy() */ > > - DROP_GIANT(); > g_topology_lock(); > sc = g_journal_find_device(classp, mp->mnt_gjprovider); > g_topology_unlock(); > - PICKUP_GIANT(); > > if (sc == NULL) { > GJ_DEBUG(0, "Cannot find journal geom for %s.", > @@ -2984,7 +2976,6 @@ next: > > sc = NULL; > for (;;) { > - DROP_GIANT(); > g_topology_lock(); > LIST_FOREACH(gp, &g_journal_class.geom, geom) { > sc = gp->softc; > @@ -3000,7 +2991,6 @@ next: > sc = NULL; > } > g_topology_unlock(); > - PICKUP_GIANT(); > if (sc == NULL) > break; > mtx_assert(&sc->sc_mtx, MA_OWNED); > diff --git a/sys/geom/mirror/g_mirror.c b/sys/geom/mirror/g_mirror.c > index 91f1367..379f615 100644 > --- a/sys/geom/mirror/g_mirror.c > +++ b/sys/geom/mirror/g_mirror.c > @@ -3310,7 +3310,6 @@ g_mirror_shutdown_post_sync(void *arg, int howto) > int error; > > mp = arg; > - DROP_GIANT(); > g_topology_lock(); > g_mirror_shutdown = 1; > LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) { > @@ -3329,7 +3328,6 @@ g_mirror_shutdown_post_sync(void *arg, int howto) > g_topology_lock(); > } > g_topology_unlock(); > - PICKUP_GIANT(); > } > > static void > diff --git a/sys/geom/mountver/g_mountver.c b/sys/geom/mountver/g_mountver.c > index eafccc8..61375ef 100644 > --- a/sys/geom/mountver/g_mountver.c > +++ b/sys/geom/mountver/g_mountver.c > @@ -611,12 +611,10 @@ g_mountver_shutdown_pre_sync(void *arg, int howto) > struct g_geom *gp, *gp2; > > mp = arg; > - DROP_GIANT(); > g_topology_lock(); > LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) > g_mountver_destroy(gp, 1); > g_topology_unlock(); > - PICKUP_GIANT(); > } > > static void > diff --git a/sys/geom/raid/g_raid.c b/sys/geom/raid/g_raid.c > index 4885319..e590e35 100644 > --- a/sys/geom/raid/g_raid.c > +++ b/sys/geom/raid/g_raid.c > @@ -2462,7 +2462,6 @@ g_raid_shutdown_post_sync(void *arg, int howto) > struct g_raid_volume *vol; > > mp = arg; > - DROP_GIANT(); > g_topology_lock(); > g_raid_shutdown = 1; > LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) { > @@ -2477,7 +2476,6 @@ g_raid_shutdown_post_sync(void *arg, int howto) > g_topology_lock(); > } > g_topology_unlock(); > - PICKUP_GIANT(); > } > > static void > diff --git a/sys/geom/raid3/g_raid3.c b/sys/geom/raid3/g_raid3.c > index a2ffe53..9b3c483 100644 > --- a/sys/geom/raid3/g_raid3.c > +++ b/sys/geom/raid3/g_raid3.c > @@ -3543,7 +3543,6 @@ g_raid3_shutdown_post_sync(void *arg, int howto) > int error; > > mp = arg; > - DROP_GIANT(); > g_topology_lock(); > g_raid3_shutdown = 1; > LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) { > @@ -3562,7 +3561,6 @@ g_raid3_shutdown_post_sync(void *arg, int howto) > g_topology_lock(); > } > g_topology_unlock(); > - PICKUP_GIANT(); > } > > static void > _______________________________________________ > freebsd-arch@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/freebsd-arch > To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org" >