svn commit: r330977 - head/sys/geom
Andriy Gapon
avg at FreeBSD.org
Thu Mar 15 09:16:11 UTC 2018
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);
More information about the svn-src-all
mailing list