Date: Thu, 19 May 2016 22:12:47 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Alfred Perlstein <bright@mu.org> Cc: geom@freebsd.org, arch@freebsd.org Subject: Re: Removing Giant asserts from geom Message-ID: <20160519191247.GQ89104@kib.kiev.ua> In-Reply-To: <573DEA73.4080408@mu.org> References: <20160519105634.GO89104@kib.kiev.ua> <573DEA73.4080408@mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, May 19, 2016 at 09:31:47AM -0700, Alfred Perlstein wrote:
> 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.
Did you read the third paragraph of my email ?
FWIW, the assumed model of the kernel locking which must be in somebody
mind when talking about 'pushing back Giant' is not true for last 5-6
years for our kernel in general, and for the VFS in particular.
>
> -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"
> >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160519191247.GQ89104>