d/freebsd-skq
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

g_access: deal with races created by geoms that drop the topology lock

Browse Source 
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
This commit is contained in:
Andriy Gapon 2018-03-15 09:16:10 +00:00
parent 3c9d418b7e
commit aca41af247
2 changed files with 51 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
sys/geom/geom.h
View File

@ -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;
};

52
sys/geom/geom_subr.c
View File

@ -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 dce)
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 dce)
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
@ -903,6 +908,27 @@ g_access(struct g_consumer *cp, int dcr, int dcw, int dce)
(dcr > 0 || dcw > 0 || dce > 0))
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 dce)
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 dce)
/* 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 dce)
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);