Author: avg Date: Thu Mar 15 09:16:10 2018 New Revision: 330977 URL: https://svnweb.freebsd.org/changeset/base/330977
Log: g_access: deal with races created by geoms that drop the topology lock The problem is that g_access() must be called with the GEOM topology lock held. And that gives a false impression that the lock is indeed held across the call. But this isn't always true because many classes, ZVOL being one of the many, need to drop the lock. It's either to perform an I/O on the first open or to acquire a different lock (like in g_mirror_access). That, of course, can break many assumptions. For example, g_slice_access() adds an extra exclusive count on the first open. As described above, an underlying geom may drop the topology lock and that would open a race with another thread that would also request another extra exclusive count. In general, two consumers may be granted incompatible accesses. To avoid this problem the code is changed to mark a geom with special flag before calling its access method and clear the flag afterwards. If another thread sees that flag, then it means that the topology lock has been dropped (either by the geom in question or downstream from it), so it is not safe to make another access call. So, the second thread would use g_topology_sleep() to wait until the flag is cleared and only then would it proceed with the access. Also see http://docs.freebsd.org/cgi/mid.cgi?809d9254-ee56-59d8-69a4-08838e985cea PR: 225960 Reported by: asomers Reviewed by: markj, mav MFC after: 3 weeks Differential Revision: https://reviews.freebsd.org/D14533 Modified: head/sys/geom/geom.h head/sys/geom/geom_subr.c Modified: head/sys/geom/geom.h ============================================================================== --- head/sys/geom/geom.h Thu Mar 15 09:04:23 2018 (r330976) +++ head/sys/geom/geom.h Thu Mar 15 09:16:10 2018 (r330977) @@ -159,8 +159,10 @@ struct g_geom { void *spare1; void *softc; unsigned flags; -#define G_GEOM_WITHER 1 -#define G_GEOM_VOLATILE_BIO 2 +#define G_GEOM_WITHER 0x01 +#define G_GEOM_VOLATILE_BIO 0x02 +#define G_GEOM_IN_ACCESS 0x04 +#define G_GEOM_ACCESS_WAIT 0x08 LIST_HEAD(,g_geom_alias) aliases; }; Modified: head/sys/geom/geom_subr.c ============================================================================== --- head/sys/geom/geom_subr.c Thu Mar 15 09:04:23 2018 (r330976) +++ head/sys/geom/geom_subr.c Thu Mar 15 09:16:10 2018 (r330977) @@ -876,7 +876,11 @@ int g_access(struct g_consumer *cp, int dcr, int dcw, int dce) { struct g_provider *pp; + struct g_geom *gp; int pw, pe; +#ifdef INVARIANTS + int sr, sw, se; +#endif int error; g_topology_assert(); @@ -884,6 +888,7 @@ g_access(struct g_consumer *cp, int dcr, int dcw, int pp = cp->provider; KASSERT(pp != NULL, ("access but not attached")); G_VALID_PROVIDER(pp); + gp = pp->geom; g_trace(G_T_ACCESS, "g_access(%p(%s), %d, %d, %d)", cp, pp->name, dcr, dcw, dce); @@ -892,7 +897,7 @@ g_access(struct g_consumer *cp, int dcr, int dcw, int KASSERT(cp->acw + dcw >= 0, ("access resulting in negative acw")); KASSERT(cp->ace + dce >= 0, ("access resulting in negative ace")); KASSERT(dcr != 0 || dcw != 0 || dce != 0, ("NOP access request")); - KASSERT(pp->geom->access != NULL, ("NULL geom->access")); + KASSERT(gp->access != NULL, ("NULL geom->access")); /* * If our class cares about being spoiled, and we have been, we @@ -904,6 +909,27 @@ g_access(struct g_consumer *cp, int dcr, int dcw, int return (ENXIO); /* + * A number of GEOM classes either need to perform an I/O on the first + * open or to acquire a different subsystem's lock. To do that they + * may have to drop the topology lock. + * Other GEOM classes perform special actions when opening a lower rank + * geom for the first time. As a result, more than one thread may + * end up performing the special actions. + * So, we prevent concurrent "first" opens by marking the consumer with + * special flag. + * + * Note that if the geom's access method never drops the topology lock, + * then we will never see G_GEOM_IN_ACCESS here. + */ + while ((gp->flags & G_GEOM_IN_ACCESS) != 0) { + g_trace(G_T_ACCESS, + "%s: race on geom %s via provider %s and consumer of %s", + __func__, gp->name, pp->name, cp->geom->name); + gp->flags |= G_GEOM_ACCESS_WAIT; + g_topology_sleep(gp, 0); + } + + /* * Figure out what counts the provider would have had, if this * consumer had (r0w0e0) at this time. */ @@ -918,7 +944,7 @@ g_access(struct g_consumer *cp, int dcr, int dcw, int pp, pp->name); /* If foot-shooting is enabled, any open on rank#1 is OK */ - if ((g_debugflags & 16) && pp->geom->rank == 1) + if ((g_debugflags & 16) && gp->rank == 1) ; /* If we try exclusive but already write: fail */ else if (dce > 0 && pw > 0) @@ -935,11 +961,27 @@ g_access(struct g_consumer *cp, int dcr, int dcw, int /* Ok then... */ - error = pp->geom->access(pp, dcr, dcw, dce); +#ifdef INVARIANTS + sr = cp->acr; + sw = cp->acw; + se = cp->ace; +#endif + gp->flags |= G_GEOM_IN_ACCESS; + error = gp->access(pp, dcr, dcw, dce); KASSERT(dcr > 0 || dcw > 0 || dce > 0 || error == 0, ("Geom provider %s::%s dcr=%d dcw=%d dce=%d error=%d failed " - "closing ->access()", pp->geom->class->name, pp->name, dcr, dcw, + "closing ->access()", gp->class->name, pp->name, dcr, dcw, dce, error)); + + g_topology_assert(); + gp->flags &= ~G_GEOM_IN_ACCESS; + KASSERT(cp->acr == sr && cp->acw == sw && cp->ace == se, + ("Access counts changed during geom->access")); + if ((gp->flags & G_GEOM_ACCESS_WAIT) != 0) { + gp->flags &= ~G_GEOM_ACCESS_WAIT; + wakeup(gp); + } + if (!error) { /* * If we open first write, spoil any partner consumers. @@ -949,7 +991,7 @@ g_access(struct g_consumer *cp, int dcr, int dcw, int if (pp->acw == 0 && dcw != 0) g_spoil(pp, cp); else if (pp->acw != 0 && pp->acw == -dcw && pp->error == 0 && - !(pp->geom->flags & G_GEOM_WITHER)) + !(gp->flags & G_GEOM_WITHER)) g_post_event(g_new_provider_event, pp, M_WAITOK, pp, NULL); _______________________________________________ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
