Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 11 May 2017 16:26:56 +0000 (UTC)
From:      Alan Somers <asomers@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r318189 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
Message-ID:  <201705111626.v4BGQuaK018998@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: asomers
Date: Thu May 11 16:26:56 2017
New Revision: 318189
URL: https://svnweb.freebsd.org/changeset/base/318189

Log:
  vdev_geom may associate multiple vdevs per g_consumer
  
  vdev_geom.c currently uses the g_consumer's private field to point to a
  vdev_t. That way, a GEOM event can cause a change to a ZFS vdev. For
  example, when you remove a disk, the vdev's status will change to REMOVED.
  However, vdev_geom will sometimes attach multiple vdevs to the same GEOM
  consumer. If this happens, then geom events will only be propagated to one
  of the vdevs.
  
  Fix this by storing a linked list of vdevs in g_consumer's private field.
  
  sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
  
  * g_consumer.private now stores a linked list of vdev pointers associated
    with the consumer instead of just a single vdev pointer.
  
  * Change vdev_geom_set_physpath's signature to more closely match
    vdev_geom_set_rotation_rate
  
  * Don't bother calling g_access in vdev_geom_set_physpath. It's guaranteed
    that we've already accessed the consumer by the time we get here.
  
  * Don't call vdev_geom_set_physpath in vdev_geom_attach. Instead, call it
    in vdev_geom_open, after we know that the open has succeeded.
  
  PR:		218634
  Reviewed by:	gibbs
  MFC after:	1 week
  Sponsored by:	Spectra Logic Corp
  Differential Revision:	https://reviews.freebsd.org/D10391

Modified:
  head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c

Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c	Thu May 11 15:19:04 2017	(r318188)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c	Thu May 11 16:26:56 2017	(r318189)
@@ -49,6 +49,16 @@ struct g_class zfs_vdev_class = {
 	.attrchanged = vdev_geom_attrchanged,
 };
 
+struct consumer_vdev_elem {
+	SLIST_ENTRY(consumer_vdev_elem)	elems;
+	vdev_t				*vd;
+};
+
+SLIST_HEAD(consumer_priv_t, consumer_vdev_elem);
+_Static_assert(sizeof(((struct g_consumer*)NULL)->private)
+    == sizeof(struct consumer_priv_t*),
+    "consumer_priv_t* can't be stored in g_consumer.private");
+
 DECLARE_GEOM_CLASS(zfs_vdev_class, zfs_vdev);
 
 SYSCTL_DECL(_vfs_zfs_vdev);
@@ -85,21 +95,16 @@ vdev_geom_set_rotation_rate(vdev_t *vd, 
 }
 
 static void
-vdev_geom_set_physpath(struct g_consumer *cp, boolean_t do_null_update)
+vdev_geom_set_physpath(vdev_t *vd, struct g_consumer *cp,
+		       boolean_t do_null_update)
 {
 	boolean_t needs_update = B_FALSE;
-	vdev_t *vd;
 	char *physpath;
 	int error, physpath_len;
 
-	if (g_access(cp, 1, 0, 0) != 0)
-		return;
-
-	vd = cp->private;
 	physpath_len = MAXPATHLEN;
 	physpath = g_malloc(physpath_len, M_WAITOK|M_ZERO);
 	error = g_io_getattr("GEOM::physpath", cp, &physpath_len, physpath);
-	g_access(cp, -1, 0, 0);
 	if (error == 0) {
 		char *old_physpath;
 
@@ -130,37 +135,40 @@ vdev_geom_set_physpath(struct g_consumer
 static void
 vdev_geom_attrchanged(struct g_consumer *cp, const char *attr)
 {
-	vdev_t *vd;
 	char *old_physpath;
+	struct consumer_priv_t *priv;
+	struct consumer_vdev_elem *elem;
 	int error;
 
-	vd = cp->private;
-	if (vd == NULL)
+	priv = (struct consumer_priv_t*)&cp->private;
+	if (SLIST_EMPTY(priv))
 		return;
 
-	if (strcmp(attr, "GEOM::rotation_rate") == 0) {
-		vdev_geom_set_rotation_rate(vd, cp);
-		return;
-	}
-
-	if (strcmp(attr, "GEOM::physpath") == 0) {
-		vdev_geom_set_physpath(cp, /*do_null_update*/B_TRUE);
-		return;
+	SLIST_FOREACH(elem, priv, elems) {
+		vdev_t *vd = elem->vd;
+		if (strcmp(attr, "GEOM::rotation_rate") == 0) {
+			vdev_geom_set_rotation_rate(vd, cp);
+			return;
+		}
+		if (strcmp(attr, "GEOM::physpath") == 0) {
+			vdev_geom_set_physpath(vd, cp, /*null_update*/B_TRUE);
+			return;
+		}
 	}
 }
 
 static void
 vdev_geom_orphan(struct g_consumer *cp)
 {
-	vdev_t *vd;
+	struct consumer_priv_t *priv;
+	struct consumer_vdev_elem *elem;
 
 	g_topology_assert();
 
-	vd = cp->private;
-	if (vd == NULL) {
+	priv = (struct consumer_priv_t*)&cp->private;
+	if (SLIST_EMPTY(priv))
 		/* Vdev close in progress.  Ignore the event. */
 		return;
-	}
 
 	/*
 	 * Orphan callbacks occur from the GEOM event thread.
@@ -176,8 +184,12 @@ vdev_geom_orphan(struct g_consumer *cp)
 	 * async removal support to invoke a close on this
 	 * vdev once it is safe to do so.
 	 */
-	vd->vdev_remove_wanted = B_TRUE;
-	spa_async_request(vd->vdev_spa, SPA_ASYNC_REMOVE);
+	SLIST_FOREACH(elem, priv, elems) {
+		vdev_t *vd = elem->vd;
+
+		vd->vdev_remove_wanted = B_TRUE;
+		spa_async_request(vd->vdev_spa, SPA_ASYNC_REMOVE);
+	}
 }
 
 static struct g_consumer *
@@ -265,21 +277,8 @@ vdev_geom_attach(struct g_provider *pp, 
 		}
 	}
 
-	/* 
-	 * BUG: cp may already belong to a vdev.  This could happen if:
-	 * 1) That vdev is a shared spare, or
-	 * 2) We are trying to reopen a missing vdev and we are scanning by
-	 *    guid.  In that case, we'll ultimately fail to open this consumer,
-	 *    but not until after setting the private field.
-	 * The solution is to:
-	 * 1) Don't set the private field until after the open succeeds, and
-	 * 2) Set it to a linked list of vdevs, not just a single vdev
-	 */
-	cp->private = vd;
-	if (vd != NULL) {
+	if (vd != NULL)
 		vd->vdev_tsd = cp;
-		vdev_geom_set_physpath(cp, /*do_null_update*/B_FALSE);
-	}
 
 	cp->flags |= G_CF_DIRECT_SEND | G_CF_DIRECT_RECEIVE;
 	return (cp);
@@ -289,16 +288,12 @@ static void
 vdev_geom_detach(struct g_consumer *cp, boolean_t open_for_read)
 {
 	struct g_geom *gp;
-	vdev_t *vd;
 
 	g_topology_assert();
 
 	ZFS_LOG(1, "Detaching from %s.",
 	    cp->provider && cp->provider->name ? cp->provider->name : "NULL");
 
-	vd = cp->private;
-	cp->private = NULL;
-
 	gp = cp->geom;
 	if (open_for_read)
 		g_access(cp, -1, 0, -1);
@@ -324,16 +319,26 @@ static void
 vdev_geom_close_locked(vdev_t *vd)
 {
 	struct g_consumer *cp;
+	struct consumer_priv_t *priv;
+	struct consumer_vdev_elem *elem, *elem_temp;
 
 	g_topology_assert();
 
 	cp = vd->vdev_tsd;
-	vd->vdev_tsd = NULL;
 	vd->vdev_delayed_close = B_FALSE;
 	if (cp == NULL)
 		return;
 
 	ZFS_LOG(1, "Closing access to %s.", cp->provider->name);
+	KASSERT(cp->private != NULL, ("%s: cp->private is NULL", __func__));
+	priv = (struct consumer_priv_t*)&cp->private;
+	vd->vdev_tsd = NULL;
+	SLIST_FOREACH_SAFE(elem, priv, elems, elem_temp) {
+		if (elem->vd == vd) {
+			SLIST_REMOVE(priv, elem, consumer_vdev_elem, elems);
+			g_free(elem);
+		}
+	}
 
 	vdev_geom_detach(cp, B_TRUE);
 }
@@ -870,11 +875,27 @@ vdev_geom_open(vdev_t *vd, uint64_t *psi
 			cp = NULL;
 		}
 	}
+	if (cp != NULL) {
+		struct consumer_priv_t *priv;
+		struct consumer_vdev_elem *elem;
+
+		priv = (struct consumer_priv_t*)&cp->private;
+		if (cp->private == NULL)
+			SLIST_INIT(priv);
+		elem = g_malloc(sizeof(*elem), M_WAITOK|M_ZERO);
+		elem->vd = vd;
+		SLIST_INSERT_HEAD(priv, elem, elems);
+	}
 
 	/* Fetch initial physical path information for this device. */
-	if (cp != NULL)
+	if (cp != NULL) {
 		vdev_geom_attrchanged(cp, "GEOM::physpath");
 	
+		/* Set other GEOM characteristics */
+		vdev_geom_set_physpath(vd, cp, /*do_null_update*/B_FALSE);
+		vdev_geom_set_rotation_rate(vd, cp);
+	}
+
 	g_topology_unlock();
 	PICKUP_GIANT();
 	if (cp == NULL) {
@@ -905,11 +926,6 @@ skip_open:
 	 */
 	vd->vdev_nowritecache = B_FALSE;
 
-	/*
-	 * Determine the device's rotation rate.
-	 */
-	vdev_geom_set_rotation_rate(vd, cp);
-
 	return (0);
 }
 



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