Skip site navigation (1)Skip section navigation (2) 
Date:      Fri, 6 Dec 2019 16:48:37 +0000 (UTC)
From:      Alexander Motin <mav@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r355451 - head/sys/geom
Message-ID:  <201912061648.xB6Gmbqo048252@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: mav
Date: Fri Dec  6 16:48:36 2019
New Revision: 355451
URL: https://svnweb.freebsd.org/changeset/base/355451

Log:
  Remove some branching from GEOM_DISK hot path.
  
  pp->private just can not be NULL in those places.
  
  In g_disk_start() and g_disk_ioctl() both dp != NULL and !dp->d_destroyed
  should always be true if disk_gone() and disk_destroy() are used properly,
  since GEOM does not send requests to errored providers.  If the protocol is
  not followed, then no amount of additional checks here give real safety.
  
  In g_disk_access() though the checks are useful, since GEOM blocks only
  new opens for errored providers, but allows closes.  It should not happen
  if disk_gone() and disk_destroy() are used properly, but may otherwise.
  
  To improve cases when disk_gone() is not used, call it from disk_destroy().
  It does not give full guaranties, but it errors the provider and makes
  GEOM block unwanted requests at least after some race.
  
  MFC after:	2 weeks

Modified:
  head/sys/geom/geom_disk.c

Modified: head/sys/geom/geom_disk.c
==============================================================================
--- head/sys/geom/geom_disk.c	Fri Dec  6 16:42:58 2019	(r355450)
+++ head/sys/geom/geom_disk.c	Fri Dec  6 16:48:36 2019	(r355451)
@@ -108,7 +108,7 @@ g_disk_access(struct g_provider *pp, int r, int w, int
 	    pp->name, r, w, e);
 	g_topology_assert();
 	sc = pp->private;
-	if (sc == NULL || (dp = sc->dp) == NULL || dp->d_destroyed) {
+	if ((dp = sc->dp) == NULL || dp->d_destroyed) {
 		/*
 		 * Allow decreasing access count even if disk is not
 		 * available anymore.
@@ -274,6 +274,8 @@ g_disk_ioctl(struct g_provider *pp, u_long cmd, void *
 
 	sc = pp->private;
 	dp = sc->dp;
+	KASSERT(dp != NULL && !dp->d_destroyed,
+	    ("g_disk_ioctl(%lx) on destroyed disk %s", cmd, pp->name));
 
 	if (dp->d_ioctl == NULL)
 		return (ENOIOCTL);
@@ -432,10 +434,9 @@ g_disk_start(struct bio *bp)
 	biotrack(bp, __func__);
 
 	sc = bp->bio_to->private;
-	if (sc == NULL || (dp = sc->dp) == NULL || dp->d_destroyed) {
-		g_io_deliver(bp, ENXIO);
-		return;
-	}
+	dp = sc->dp;
+	KASSERT(dp != NULL && !dp->d_destroyed,
+	    ("g_disk_start(%p) on destroyed disk %s", bp, bp->bio_to->name));
 	error = EJUSTRETURN;
 	switch(bp->bio_cmd) {
 	case BIO_DELETE:
@@ -896,8 +897,9 @@ void
 disk_destroy(struct disk *dp)
 {
 
-	g_cancel_event(dp);
+	disk_gone(dp);
 	dp->d_destroyed = 1;
+	g_cancel_event(dp);
 	if (dp->d_devstat != NULL)
 		devstat_remove_entry(dp->d_devstat);
 	g_post_event(g_disk_destroy, dp, M_WAITOK, NULL);
@@ -922,6 +924,16 @@ disk_gone(struct disk *dp)
 	struct g_provider *pp;
 
 	mtx_pool_lock(mtxpool_sleep, dp);
+
+	/*
+	 * Second wither call makes no sense, plus we can not access the list
+	 * of providers without topology lock after calling wither once.
+	 */
+	if (dp->d_goneflag != 0) {
+		mtx_pool_unlock(mtxpool_sleep, dp);
+		return;
+	}
+
 	dp->d_goneflag = 1;
 
 	/*
@@ -946,13 +958,11 @@ disk_gone(struct disk *dp)
 	mtx_pool_unlock(mtxpool_sleep, dp);
 
 	gp = dp->d_geom;
-	if (gp != NULL) {
-		pp = LIST_FIRST(&gp->provider);
-		if (pp != NULL) {
-			KASSERT(LIST_NEXT(pp, provider) == NULL,
-			    ("geom %p has more than one provider", gp));
-			g_wither_provider(pp, ENXIO);
-		}
+	pp = LIST_FIRST(&gp->provider);
+	if (pp != NULL) {
+		KASSERT(LIST_NEXT(pp, provider) == NULL,
+		    ("geom %p has more than one provider", gp));
+		g_wither_provider(pp, ENXIO);
 	}
 }

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201912061648.xB6Gmbqo048252>