changeset 19425:d07ea35a3ffd

4454 ldi notifications trigger vdev_disk_free() without spa_config_lock() Portions contributed by: Jason King <jason.king@joyent.com> Reviewed by: Jerry Jelinek <jerry.jelinek@joyent.com> Approved by: Dan McDonald <danmcd@joyent.com>
author Mike Gerdts <mike.gerdts@joyent.com>
date Sun, 15 Dec 2019 04:05:04 +0000
parents f1c9ab245adb
children fab60082a367
files usr/src/uts/common/fs/zfs/vdev_disk.c
diffstat 1 files changed, 18 insertions(+), 49 deletions(-) [+]
line wrap: on
line diff
--- a/usr/src/uts/common/fs/zfs/vdev_disk.c	Wed Jan 29 06:55:18 2020 +0100
+++ b/usr/src/uts/common/fs/zfs/vdev_disk.c	Sun Dec 15 04:05:04 2019 +0000
@@ -22,7 +22,7 @@
  * Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved.
  * Copyright (c) 2012, 2018 by Delphix. All rights reserved.
  * Copyright 2016 Nexenta Systems, Inc.  All rights reserved.
- * Copyright 2019 Joyent, Inc.
+ * Copyright 2020 Joyent, Inc.
  */
 
 #include <sys/zfs_context.h>
@@ -127,10 +127,9 @@
 	vd->vdev_tsd = NULL;
 }
 
-/* ARGSUSED */
 static int
-vdev_disk_off_notify(ldi_handle_t lh, ldi_ev_cookie_t ecookie, void *arg,
-    void *ev_data)
+vdev_disk_off_notify(ldi_handle_t lh __unused, ldi_ev_cookie_t ecookie,
+    void *arg, void *ev_data __unused)
 {
 	vdev_t *vd = (vdev_t *)arg;
 	vdev_disk_t *dvd = vd->vdev_tsd;
@@ -142,19 +141,15 @@
 		return (LDI_EV_SUCCESS);
 
 	/*
-	 * All LDI handles must be closed for the state change to succeed, so
-	 * call on vdev_disk_close() to do this.
-	 *
-	 * We inform vdev_disk_close that it is being called from offline
-	 * notify context so it will defer cleanup of LDI event callbacks and
-	 * freeing of vd->vdev_tsd to the offline finalize or a reopen.
+	 * Tell any new threads that stumble upon this vdev that they should not
+	 * try to do I/O.
 	 */
 	dvd->vd_ldi_offline = B_TRUE;
-	vdev_disk_close(vd);
 
 	/*
-	 * Now that the device is closed, request that the spa_async_thread
-	 * mark the device as REMOVED and notify FMA of the removal.
+	 * Request that the spa_async_thread mark the device as REMOVED and
+	 * notify FMA of the removal.  This should also trigger a vdev_close()
+	 * in the async thread.
 	 */
 	zfs_post_remove(vd->vdev_spa, vd);
 	vd->vdev_remove_wanted = B_TRUE;
@@ -163,10 +158,9 @@
 	return (LDI_EV_SUCCESS);
 }
 
-/* ARGSUSED */
 static void
-vdev_disk_off_finalize(ldi_handle_t lh, ldi_ev_cookie_t ecookie,
-    int ldi_result, void *arg, void *ev_data)
+vdev_disk_off_finalize(ldi_handle_t lh __unused, ldi_ev_cookie_t ecookie,
+    int ldi_result, void *arg, void *ev_data __unused)
 {
 	vdev_t *vd = (vdev_t *)arg;
 
@@ -177,12 +171,6 @@
 		return;
 
 	/*
-	 * We have already closed the LDI handle in notify.
-	 * Clean up the LDI event callbacks and free vd->vdev_tsd.
-	 */
-	vdev_disk_free(vd);
-
-	/*
 	 * Request that the vdev be reopened if the offline state change was
 	 * unsuccessful.
 	 */
@@ -198,10 +186,9 @@
 	.cb_finalize = vdev_disk_off_finalize
 };
 
-/* ARGSUSED */
 static void
-vdev_disk_dgrd_finalize(ldi_handle_t lh, ldi_ev_cookie_t ecookie,
-    int ldi_result, void *arg, void *ev_data)
+vdev_disk_dgrd_finalize(ldi_handle_t lh __unused, ldi_ev_cookie_t ecookie,
+    int ldi_result, void *arg, void *ev_data __unused)
 {
 	vdev_t *vd = (vdev_t *)arg;
 
@@ -326,17 +313,8 @@
 	 * just update the physical size of the device.
 	 */
 	if (dvd != NULL) {
-		if (dvd->vd_ldi_offline && dvd->vd_lh == NULL) {
-			/*
-			 * If we are opening a device in its offline notify
-			 * context, the LDI handle was just closed. Clean
-			 * up the LDI event callbacks and free vd->vdev_tsd.
-			 */
-			vdev_disk_free(vd);
-		} else {
-			ASSERT(vd->vdev_reopening);
-			goto skip_open;
-		}
+		ASSERT(vd->vdev_reopening);
+		goto skip_open;
 	}
 
 	/*
@@ -768,14 +746,6 @@
 	}
 
 	vd->vdev_delayed_close = B_FALSE;
-	/*
-	 * If we closed the LDI handle due to an offline notify from LDI,
-	 * don't free vd->vdev_tsd or unregister the callbacks here;
-	 * the offline finalize callback or a reopen will take care of it.
-	 */
-	if (dvd->vd_ldi_offline)
-		return;
-
 	vdev_disk_free(vd);
 }
 
@@ -809,7 +779,8 @@
 
 static int
 vdev_disk_dumpio(vdev_t *vd, caddr_t data, size_t size,
-    uint64_t offset, uint64_t origoffset, boolean_t doread, boolean_t isdump)
+    uint64_t offset, uint64_t origoffset __unused, boolean_t doread,
+    boolean_t isdump)
 {
 	vdev_disk_t *dvd = vd->vdev_tsd;
 	int flags = doread ? B_READ : B_WRITE;
@@ -817,11 +788,9 @@
 	/*
 	 * If the vdev is closed, it's likely in the REMOVED or FAULTED state.
 	 * Nothing to be done here but return failure.
-	 *
-	 * XXX-mg there is still a race here with off_notify
 	 */
 	if (dvd == NULL || dvd->vd_ldi_offline) {
-		return (EIO);
+		return (SET_ERROR(ENXIO));
 	}
 
 	ASSERT(vd->vdev_ops == &vdev_disk_ops);
@@ -905,7 +874,7 @@
 	 * If the vdev is closed, it's likely in the REMOVED or FAULTED state.
 	 * Nothing to be done here but return failure.
 	 */
-	if (dvd == NULL || (dvd->vd_ldi_offline && dvd->vd_lh == NULL)) {
+	if (dvd == NULL || dvd->vd_ldi_offline) {
 		zio->io_error = ENXIO;
 		zio_interrupt(zio);
 		return;