From owner-freebsd-geom@freebsd.org Thu May 19 10:56:44 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 8948BB423AC for ; Thu, 19 May 2016 10:56:44 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mailman.ysv.freebsd.org (unknown [127.0.1.3]) by mx1.freebsd.org (Postfix) with ESMTP id 75302147B for ; Thu, 19 May 2016 10:56:44 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: by mailman.ysv.freebsd.org (Postfix) id 6DF73B423AA; Thu, 19 May 2016 10:56:44 +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 6D328B423A8; Thu, 19 May 2016 10:56:44 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id F2C791478; Thu, 19 May 2016 10:56:40 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id u4JAuYfr059664 (version=TLSv1 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Thu, 19 May 2016 13:56:35 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua u4JAuYfr059664 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id u4JAuYhq059663; Thu, 19 May 2016 13:56:34 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 19 May 2016 13:56:34 +0300 From: Konstantin Belousov To: geom@freebsd.org Cc: arch@freebsd.org Subject: Removing Giant asserts from geom Message-ID: <20160519105634.GO89104@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.6.1 (2016-04-27) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home 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 10:56:44 -0000 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