Date: Thu, 19 May 2016 09:31:47 -0700 From: Alfred Perlstein <bright@mu.org> To: Konstantin Belousov <kostikbel@gmail.com>, geom@freebsd.org Cc: arch@freebsd.org Subject: Re: Removing Giant asserts from geom Message-ID: <573DEA73.4080408@mu.org> In-Reply-To: <20160519105634.GO89104@kib.kiev.ua> References: <20160519105634.GO89104@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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"
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?573DEA73.4080408>