10623 ZFS should be more aggressive in updating vdev devid

Reviewed by: Toomas Soome <tsoome@me.com>
Reviewed by: Gordon Ross <gwr@nexenta.com>
Reviewed by: Andy Fiddaman <omnios@citrus-it.co.uk>
Reviewed by: Jerry Jelinek <jerry.jelinek@joyent.com>
Approved by: Robert Mustacchi <rm@joyent.com>

1 files changed, 115 insertions, 23 deletions

diff --git a/usr/src/uts/common/fs/zfs/vdev_disk.c b/usr/src/uts/common/fs/zfs/vdev_disk.c
index f7e0b36524..a7aba4a65d 100644
--- a/usr/src/uts/common/fs/zfs/vdev_disk.c
+++ b/usr/src/uts/common/fs/zfs/vdev_disk.c
@@ -292,7 +292,6 @@ vdev_disk_open(vdev_t *vd, uint64_t *psize, uint64_t *max_psize,
 	dev_t dev;
 	int otyp;
 	boolean_t validate_devid = B_FALSE;
-	ddi_devid_t devid;
 	uint64_t capacity = 0, blksz = 0, pbsize;
 
 	/*
@@ -396,9 +395,20 @@ vdev_disk_open(vdev_t *vd, uint64_t *psize, uint64_t *max_psize,
 		/*
 		 * Compare the devid to the stored value.
 		 */
-		if (error == 0 && vd->vdev_devid != NULL &&
-		    ldi_get_devid(dvd->vd_lh, &devid) == 0) {
-			if (ddi_devid_compare(devid, dvd->vd_devid) != 0) {
+		if (error == 0 && vd->vdev_devid != NULL) {
+			ddi_devid_t devid = NULL;
+
+			if (ldi_get_devid(dvd->vd_lh, &devid) != 0) {
+				/*
+				 * We expected a devid on this device but it no
+				 * longer appears to have one.  The validation
+				 * step may need to remove it from the
+				 * configuration.
+				 */
+				validate_devid = B_TRUE;
+
+			} else if (ddi_devid_compare(devid, dvd->vd_devid) !=
+			    0) {
 				/*
 				 * A mismatch here is unexpected, log it.
 				 */
@@ -417,7 +427,10 @@ vdev_disk_open(vdev_t *vd, uint64_t *psize, uint64_t *max_psize,
 				    kcred);
 				dvd->vd_lh = NULL;
 			}
-			ddi_devid_free(devid);
+
+			if (devid != NULL) {
+				ddi_devid_free(devid);
+			}
 		}
 
 		/*
@@ -447,26 +460,27 @@ vdev_disk_open(vdev_t *vd, uint64_t *psize, uint64_t *max_psize,
 	 * as reliable as the devid, this will give us something, and the higher
 	 * level vdev validation will prevent us from opening the wrong device.
 	 */
-	if (error) {
-		if (vd->vdev_devid != NULL)
-			validate_devid = B_TRUE;
+	if (error != 0) {
+		validate_devid = B_TRUE;
 
 		if (vd->vdev_physpath != NULL &&
-		    (dev = ddi_pathname_to_dev_t(vd->vdev_physpath)) != NODEV)
+		    (dev = ddi_pathname_to_dev_t(vd->vdev_physpath)) != NODEV) {
 			error = ldi_open_by_dev(&dev, OTYP_BLK, spa_mode(spa),
 			    kcred, &dvd->vd_lh, zfs_li);
+		}
 
 		/*
 		 * Note that we don't support the legacy auto-wholedisk support
 		 * as above.  This hasn't been used in a very long time and we
 		 * don't need to propagate its oddities to this edge condition.
 		 */
-		if (error && vd->vdev_path != NULL)
+		if (error != 0 && vd->vdev_path != NULL) {
 			error = ldi_open_by_name(vd->vdev_path, spa_mode(spa),
 			    kcred, &dvd->vd_lh, zfs_li);
+		}
 	}
 
-	if (error) {
+	if (error != 0) {
 		vd->vdev_stat.vs_aux = VDEV_AUX_OPEN_FAILED;
 		vdev_dbgmsg(vd, "vdev_disk_open: failed to open [error=%d]",
 		    error);
@@ -477,22 +491,100 @@ vdev_disk_open(vdev_t *vd, uint64_t *psize, uint64_t *max_psize,
 	 * Now that the device has been successfully opened, update the devid
 	 * if necessary.
 	 */
-	if (validate_devid && spa_writeable(spa) &&
-	    ldi_get_devid(dvd->vd_lh, &devid) == 0) {
-		if (ddi_devid_compare(devid, dvd->vd_devid) != 0) {
-			char *vd_devid;
+	if (validate_devid) {
+		ddi_devid_t devid = NULL;
+		char *minorname = NULL;
+		char *vd_devid = NULL;
+		boolean_t remove = B_FALSE, update = B_FALSE;
+
+		/*
+		 * Get the current devid and minor name for the device we
+		 * opened.
+		 */
+		if (ldi_get_devid(dvd->vd_lh, &devid) != 0 ||
+		    ldi_get_minor_name(dvd->vd_lh, &minorname) != 0) {
+			/*
+			 * If we are unable to get the devid or the minor name
+			 * for the device, we need to remove them from the
+			 * configuration to prevent potential inconsistencies.
+			 */
+			if (dvd->vd_minor != NULL || dvd->vd_devid != NULL ||
+			    vd->vdev_devid != NULL) {
+				/*
+				 * We only need to remove the devid if one
+				 * exists.
+				 */
+				remove = B_TRUE;
+			}
 
-			vd_devid = ddi_devid_str_encode(devid, dvd->vd_minor);
+		} else if (dvd->vd_devid == NULL || dvd->vd_minor == NULL) {
+			/*
+			 * There was previously no devid at all so we need to
+			 * add one.
+			 */
+			update = B_TRUE;
+
+		} else if (ddi_devid_compare(devid, dvd->vd_devid) != 0 ||
+		    strcmp(minorname, dvd->vd_minor) != 0) {
+			/*
+			 * The devid or minor name on file does not match the
+			 * one from the opened device.
+			 */
+			update = B_TRUE;
+		}
+
+		if (update) {
+			/*
+			 * Render the new devid and minor name as a string for
+			 * logging and to store in the vdev configuration.
+			 */
+			vd_devid = ddi_devid_str_encode(devid, minorname);
+		}
+
+		if (update || remove) {
 			vdev_dbgmsg(vd, "vdev_disk_open: update devid from "
-			    "'%s' to '%s'", vd->vdev_devid, vd_devid);
+			    "'%s' to '%s'",
+			    vd->vdev_devid != NULL ? vd->vdev_devid : "<none>",
+			    vd_devid != NULL ? vd_devid : "<none>");
 			cmn_err(CE_NOTE, "vdev_disk_open %s: update devid "
-			    "from '%s' to '%s'", vd->vdev_path != NULL ?
-			    vd->vdev_path : "?", vd->vdev_devid, vd_devid);
-			spa_strfree(vd->vdev_devid);
-			vd->vdev_devid = spa_strdup(vd_devid);
-			ddi_devid_str_free(vd_devid);
+			    "from '%s' to '%s'",
+			    vd->vdev_path != NULL ? vd->vdev_path : "?",
+			    vd->vdev_devid != NULL ? vd->vdev_devid : "<none>",
+			    vd_devid != NULL ? vd_devid : "<none>");
+
+			/*
+			 * Remove and free any existing values.
+			 */
+			if (dvd->vd_minor != NULL) {
+				ddi_devid_str_free(dvd->vd_minor);
+				dvd->vd_minor = NULL;
+			}
+			if (dvd->vd_devid != NULL) {
+				ddi_devid_free(dvd->vd_devid);
+				dvd->vd_devid = NULL;
+			}
+			if (vd->vdev_devid != NULL) {
+				spa_strfree(vd->vdev_devid);
+				vd->vdev_devid = NULL;
+			}
+		}
+
+		if (update) {
+			/*
+			 * Install the new values.
+			 */
+			vd->vdev_devid = vd_devid;
+			dvd->vd_minor = minorname;
+			dvd->vd_devid = devid;
+
+		} else {
+			if (devid != NULL) {
+				ddi_devid_free(devid);
+			}
+			if (minorname != NULL) {
+				kmem_free(minorname, strlen(minorname) + 1);
+			}
 		}
-		ddi_devid_free(devid);
 	}
 
 	/*