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>

1 files changed, 18 insertions, 49 deletions

diff --git a/usr/src/uts/common/fs/zfs/vdev_disk.c b/usr/src/uts/common/fs/zfs/vdev_disk.c
index b046431527..dcb61064eb 100644
--- a/usr/src/uts/common/fs/zfs/vdev_disk.c
+++ b/usr/src/uts/common/fs/zfs/vdev_disk.c
@@ -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 @@ vdev_disk_free(vdev_t *vd)
 	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 @@ vdev_disk_off_notify(ldi_handle_t lh, ldi_ev_cookie_t ecookie, void *arg,
 		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 @@ vdev_disk_off_notify(ldi_handle_t lh, ldi_ev_cookie_t ecookie, void *arg,
 	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 @@ vdev_disk_off_finalize(ldi_handle_t lh, ldi_ev_cookie_t ecookie,
 		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 @@ static ldi_ev_callback_t vdev_disk_off_callb = {
 	.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 @@ vdev_disk_open(vdev_t *vd, uint64_t *psize, uint64_t *max_psize,
 	 * 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 @@ vdev_disk_close(vdev_t *vd)
 	}
 
 	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 @@ vdev_disk_ldi_physio(ldi_handle_t vd_lh, caddr_t data,
 
 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 @@ vdev_disk_dumpio(vdev_t *vd, caddr_t data, size_t size,
 	/*
 	 * 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 @@ vdev_disk_io_start(zio_t *zio)
 	 * 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;