14759 sd open/close semaphore is unfortunate

Reviewed by: Hans Rosenfeld <rosenfeld@grumpf.hope-2000.org
Reviewed by: Jerry Jelinek <gjelinek@gmail.com>
Approved by: Dan McDonald <danmcd@mnx.io>

3 files changed, 20 insertions, 260 deletions

diff --git a/usr/src/cmd/mdb/common/modules/sd/sd.c b/usr/src/cmd/mdb/common/modules/sd/sd.c
index 7e87cf005b..87414d7831 100644
--- a/usr/src/cmd/mdb/common/modules/sd/sd.c
+++ b/usr/src/cmd/mdb/common/modules/sd/sd.c
@@ -23,6 +23,9 @@
  * Copyright 2003 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  */
+/*
+ * Copyright 2022 RackTop Systems, Inc.
+ */
 
 #include <sys/mdb_modapi.h>
 #include <sys/scsi/scsi.h>
@@ -409,64 +412,6 @@ sd_state_walk_fini(mdb_walk_state_t *wsp)
 }
 
 /*
- *    Function: process_semo_sleepq
- *
- * Description: Iterate over the semoclose wait Q members of the soft state.
- *		Print the contents of each member. In case of silent mode
- *		the contents are avoided and only the address is printed.
- *
- *   Arguments: starting queue address, print mode.
- */
-static int
-process_semo_sleepq(uintptr_t	walk_addr, int silent)
-{
-	uintptr_t	rootBuf;
-	buf_t		currentBuf;
-	int		semo_sleepq_count = 0;
-
-	/* Set up to process the device's semoclose wait Q */
-	rootBuf = walk_addr;
-
-	if (!silent) {
-		mdb_printf("\nSEMOCLOSE SLEEP Q:\n");
-		mdb_printf("----------\n");
-	}
-
-	mdb_printf("SEMOCLOSE sleep Q head: %lx\n", rootBuf);
-
-	while (rootBuf) {
-		/* Process the device's cmd. wait Q */
-		if (!silent) {
-			mdb_printf("SEMOCLOSE SLEEP Q list entry:\n");
-			mdb_printf("------------------\n");
-		}
-
-		if (mdb_vread((void *)&currentBuf, sizeof (buf_t),
-		    rootBuf) == -1) {
-			mdb_warn("failed to read buf at %p", rootBuf);
-			return (FAIL);
-		}
-
-		if (!silent) {
-			mdb_set_dot(rootBuf);
-			mdb_eval("$<buf");
-			mdb_printf("---\n");
-		}
-		++semo_sleepq_count;
-		rootBuf = (uintptr_t)currentBuf.av_forw;
-	}
-
-	if (rootBuf == 0) {
-		mdb_printf("------------------------------\n");
-		mdb_printf("Processed %d SEMOCLOSE SLEEP Q entries\n",
-		    semo_sleepq_count);
-		mdb_printf("------------------------------\n");
-
-	}
-	return (SUCCESS);
-}
-
-/*
  *    Function: process_sdlun_waitq
  *
  * Description: Iterate over the wait Q members of the soft state.
@@ -688,12 +633,6 @@ sd_callback(uintptr_t addr, const void *walk_data, void *flg_silent)
 	/* process device cmd wait Q */
 	process_sdlun_waitq((uintptr_t)sdLun.un_waitq_headp, silent);
 
-	/* process device semoclose wait Q */
-	if (sdLun.un_semoclose._opaque[1] == 0) {
-		process_semo_sleepq((uintptr_t)sdLun.un_semoclose._opaque[0],
-		    silent);
-	}
-
 	/* print the actual number of soft state processed */
 	print_footer(walk_data);
 	return (SUCCESS);
@@ -760,12 +699,6 @@ dcmd_sd_state(uintptr_t addr, uint_t flags, int argc, const mdb_arg_t *argv)
 
 		/* process device' cmd wait Q */
 		process_sdlun_waitq((uintptr_t)sdLun.un_waitq_headp, silent);
-
-		/* process device's semoclose wait Q */
-		if (sdLun.un_semoclose._opaque[1] == 0) {
-			process_semo_sleepq(
-			    (uintptr_t)sdLun.un_semoclose._opaque[0], silent);
-		}
 	}
 	return (DCMD_OK);
 }
@@ -831,12 +764,6 @@ dcmd_ssd_state(uintptr_t addr, uint_t flags, int argc, const mdb_arg_t *argv)
 
 		/* process device' cmd wait Q */
 		process_sdlun_waitq((uintptr_t)sdLun.un_waitq_headp, silent);
-
-		/* process device's semoclose wait Q */
-		if (sdLun.un_semoclose._opaque[1] == 0) {
-			process_semo_sleepq(
-			    (uintptr_t)sdLun.un_semoclose._opaque[0], silent);
-		}
 	}
 	return (DCMD_OK);
 }
diff --git a/usr/src/uts/common/io/scsi/targets/sd.c b/usr/src/uts/common/io/scsi/targets/sd.c
index a144318420..95440f6c73 100644
--- a/usr/src/uts/common/io/scsi/targets/sd.c
+++ b/usr/src/uts/common/io/scsi/targets/sd.c
@@ -182,8 +182,6 @@ static	char *sd_config_list		= "sd-config-list";
 #define	sd_qfull_throttle_enable	ssd_qfull_throttle_enable
 #define	sd_check_media_time		ssd_check_media_time
 #define	sd_wait_cmds_complete		ssd_wait_cmds_complete
-#define	sd_label_mutex			ssd_label_mutex
-#define	sd_detach_mutex			ssd_detach_mutex
 #define	sd_log_buf			ssd_log_buf
 #define	sd_log_mutex			ssd_log_mutex
 
@@ -287,21 +285,6 @@ static int sd_check_media_time		= 3000000;
 static int sd_wait_cmds_complete	= SD_WAIT_CMDS_COMPLETE;
 
 /*
- * sd_label_mutex protects a static buffer used in the disk label
- * component of the driver
- */
-static kmutex_t sd_label_mutex;
-
-/*
- * sd_detach_mutex protects un_layer_count, un_detach_count, and
- * un_opens_in_progress in the sd_lun structure.
- */
-static kmutex_t sd_detach_mutex;
-
-_NOTE(MUTEX_PROTECTS_DATA(sd_detach_mutex,
-	sd_lun::{un_layer_count un_detach_count un_opens_in_progress}))
-
-/*
  * Global buffer and mutex for debug logging
  */
 static char	sd_log_buf[1024];
@@ -2413,9 +2396,7 @@ _init(void)
 		return (err);
 	}
 
-	mutex_init(&sd_detach_mutex, NULL, MUTEX_DRIVER, NULL);
 	mutex_init(&sd_log_mutex,    NULL, MUTEX_DRIVER, NULL);
-	mutex_init(&sd_label_mutex,  NULL, MUTEX_DRIVER, NULL);
 
 	mutex_init(&sd_tr.srq_resv_reclaim_mutex, NULL, MUTEX_DRIVER, NULL);
 	cv_init(&sd_tr.srq_resv_reclaim_cv, NULL, CV_DRIVER, NULL);
@@ -2440,9 +2421,7 @@ _init(void)
 		/* delete taskq if install fails */
 		sd_taskq_delete();
 
-		mutex_destroy(&sd_detach_mutex);
 		mutex_destroy(&sd_log_mutex);
-		mutex_destroy(&sd_label_mutex);
 
 		mutex_destroy(&sd_tr.srq_resv_reclaim_mutex);
 		cv_destroy(&sd_tr.srq_resv_reclaim_cv);
@@ -2482,9 +2461,7 @@ _fini(void)
 
 	sd_taskq_delete();
 
-	mutex_destroy(&sd_detach_mutex);
 	mutex_destroy(&sd_log_mutex);
-	mutex_destroy(&sd_label_mutex);
 	mutex_destroy(&sd_tr.srq_resv_reclaim_mutex);
 
 	sd_scsi_probe_cache_fini();
@@ -6580,20 +6557,18 @@ sd_pm_idletimeout_handler(void *arg)
 	const hrtime_t idletime = sd_pm_idletime * NANOSEC;
 	struct sd_lun *un = arg;
 
-	mutex_enter(&sd_detach_mutex);
-	if (un->un_detach_count != 0) {
-		/* Abort if the instance is detaching */
-		mutex_exit(&sd_detach_mutex);
-		return;
-	}
-	mutex_exit(&sd_detach_mutex);
-
 	/*
 	 * Grab both mutexes, in the proper order, since we're accessing
 	 * both PM and softstate variables.
 	 */
 	mutex_enter(SD_MUTEX(un));
 	mutex_enter(&un->un_pm_mutex);
+	/* if timeout id is NULL, we are being canceled via untimeout */
+	if (un->un_pm_idle_timeid == NULL) {
+		mutex_exit(&un->un_pm_mutex);
+		mutex_exit(SD_MUTEX(un));
+		return;
+	}
 	if (((gethrtime() - un->un_pm_idle_time) > idletime) &&
 	    (un->un_ncmds_in_driver == 0) && (un->un_pm_count == 0)) {
 		/*
@@ -6667,7 +6642,6 @@ sdpower(dev_info_t *devi, int component, int level)
 	uchar_t		save_state = SD_STATE_NORMAL;
 	int		sval;
 	uchar_t		state_before_pm;
-	int		got_semaphore_here;
 	sd_ssc_t	*ssc;
 	int	last_power_level = SD_SPINDLE_UNINIT;
 
@@ -6682,19 +6656,6 @@ sdpower(dev_info_t *devi, int component, int level)
 
 	SD_TRACE(SD_LOG_IO_PM, un, "sdpower: entry, level = %d\n", level);
 
-	/*
-	 * Must synchronize power down with close.
-	 * Attempt to decrement/acquire the open/close semaphore,
-	 * but do NOT wait on it. If it's not greater than zero,
-	 * ie. it can't be decremented without waiting, then
-	 * someone else, either open or close, already has it
-	 * and the try returns 0. Use that knowledge here to determine
-	 * if it's OK to change the device power level.
-	 * Also, only increment it on exit if it was decremented, ie. gotten,
-	 * here.
-	 */
-	got_semaphore_here = sema_tryp(&un->un_semoclose);
-
 	mutex_enter(SD_MUTEX(un));
 
 	SD_INFO(SD_LOG_POWER, un, "sdpower: un_ncmds_in_driver = %ld\n",
@@ -6702,19 +6663,15 @@ sdpower(dev_info_t *devi, int component, int level)
 
 	/*
 	 * If un_ncmds_in_driver is non-zero it indicates commands are
-	 * already being processed in the driver, or if the semaphore was
-	 * not gotten here it indicates an open or close is being processed.
+	 * already being processed in the driver.
 	 * At the same time somebody is requesting to go to a lower power
 	 * that can't perform I/O, which can't happen, therefore we need to
 	 * return failure.
 	 */
 	if ((!SD_PM_IS_IO_CAPABLE(un, level)) &&
-	    ((un->un_ncmds_in_driver != 0) || (got_semaphore_here == 0))) {
+	    (un->un_ncmds_in_driver != 0)) {
 		mutex_exit(SD_MUTEX(un));
 
-		if (got_semaphore_here != 0) {
-			sema_v(&un->un_semoclose);
-		}
 		SD_TRACE(SD_LOG_IO_PM, un,
 		    "sdpower: exit, device has queued cmds.\n");
 
@@ -6733,9 +6690,6 @@ sdpower(dev_info_t *devi, int component, int level)
 	    (un->un_state == SD_STATE_SUSPENDED)) {
 		mutex_exit(SD_MUTEX(un));
 
-		if (got_semaphore_here != 0) {
-			sema_v(&un->un_semoclose);
-		}
 		SD_TRACE(SD_LOG_IO_PM, un,
 		    "sdpower: exit, device is off-line.\n");
 
@@ -6794,9 +6748,6 @@ sdpower(dev_info_t *devi, int component, int level)
 			kmem_free(log_page_data, log_page_size);
 			/* Cannot support power management on those drives */
 
-			if (got_semaphore_here != 0) {
-				sema_v(&un->un_semoclose);
-			}
 			/*
 			 * On exit put the state back to it's original value
 			 * and broadcast to anyone waiting for the power
@@ -6889,11 +6840,8 @@ sdpower(dev_info_t *devi, int component, int level)
 			} else {
 				mutex_exit(&un->un_pm_mutex);
 			}
-			if (got_semaphore_here != 0) {
-				sema_v(&un->un_semoclose);
-			}
 			/*
-			 * On exit put the state back to it's original value
+			 * On exit put the state back to its original value
 			 * and broadcast to anyone waiting for the power
 			 * change completion.
 			 */
@@ -6907,11 +6855,8 @@ sdpower(dev_info_t *devi, int component, int level)
 
 			goto sdpower_failed;
 		case -1:
-			if (got_semaphore_here != 0) {
-				sema_v(&un->un_semoclose);
-			}
 			/*
-			 * On exit put the state back to it's original value
+			 * On exit put the state back to its original value
 			 * and broadcast to anyone waiting for the power
 			 * change completion.
 			 */
@@ -6948,11 +6893,8 @@ sdpower(dev_info_t *devi, int component, int level)
 		 */
 		if ((rval = sd_pm_state_change(un, level, SD_PM_STATE_CHANGE))
 		    == DDI_FAILURE) {
-			if (got_semaphore_here != 0) {
-				sema_v(&un->un_semoclose);
-			}
 			/*
-			 * On exit put the state back to it's original value
+			 * On exit put the state back to its original value
 			 * and broadcast to anyone waiting for the power
 			 * change completion.
 			 */
@@ -7101,11 +7043,8 @@ sdpower(dev_info_t *devi, int component, int level)
 		}
 	}
 
-	if (got_semaphore_here != 0) {
-		sema_v(&un->un_semoclose);
-	}
 	/*
-	 * On exit put the state back to it's original value
+	 * On exit put the state back to its original value
 	 * and broadcast to anyone waiting for the power
 	 * change completion.
 	 */
@@ -7653,13 +7592,6 @@ sd_unit_attach(dev_info_t *devi)
 	un->un_f_wcc_inprog = 0;
 
 	/*
-	 * The open/close semaphore is used to serialize threads executing
-	 * in the driver's open & close entry point routines for a given
-	 * instance.
-	 */
-	(void) sema_init(&un->un_semoclose, 1, NULL, SEMA_DRIVER, NULL);
-
-	/*
 	 * The conf file entry and softstate variable is a forceful override,
 	 * meaning a non-zero value must be entered to change the default.
 	 */
@@ -8576,7 +8508,6 @@ create_errstats_failed:
 	ddi_xbuf_attr_destroy(un->un_xbuf_attr);
 
 	ddi_prop_remove_all(devi);
-	sema_destroy(&un->un_semoclose);
 	cv_destroy(&un->un_state_cv);
 
 	sd_free_rqs(un);
@@ -8626,33 +8557,20 @@ sd_unit_detach(dev_info_t *devi)
 	dev_info_t		*pdip = ddi_get_parent(devi);
 	int			instance = ddi_get_instance(devi);
 
-	mutex_enter(&sd_detach_mutex);
-
 	/*
 	 * Fail the detach for any of the following:
 	 *  - Unable to get the sd_lun struct for the instance
-	 *  - A layered driver has an outstanding open on the instance
-	 *  - Another thread is already detaching this instance
-	 *  - Another thread is currently performing an open
+	 *  - There is pending I/O
 	 */
 	devp = ddi_get_driver_private(devi);
 	if ((devp == NULL) ||
 	    ((un = (struct sd_lun *)devp->sd_private) == NULL) ||
-	    (un->un_ncmds_in_driver != 0) || (un->un_layer_count != 0) ||
-	    (un->un_detach_count != 0) || (un->un_opens_in_progress != 0)) {
-		mutex_exit(&sd_detach_mutex);
+	    (un->un_ncmds_in_driver != 0)) {
 		return (DDI_FAILURE);
 	}
 
 	SD_TRACE(SD_LOG_ATTACH_DETACH, un, "sd_unit_detach: entry 0x%p\n", un);
 
-	/*
-	 * Mark this instance as currently in a detach, to inhibit any
-	 * opens from a layered driver.
-	 */
-	un->un_detach_count++;
-	mutex_exit(&sd_detach_mutex);
-
 	tgt = ddi_prop_get_int(DDI_DEV_T_ANY, devi, DDI_PROP_DONTPASS,
 	    SCSI_ADDR_PROP_TARGET, -1);
 
@@ -8958,12 +8876,6 @@ sd_unit_detach(dev_info_t *devi)
 	cmlb_free_handle(&un->un_cmlbhandle);
 
 	/*
-	 * Hold the detach mutex here, to make sure that no other threads ever
-	 * can access a (partially) freed soft state structure.
-	 */
-	mutex_enter(&sd_detach_mutex);
-
-	/*
 	 * Clean up the soft state struct.
 	 * Cleanup is done in reverse order of allocs/inits.
 	 * At this point there should be no competing threads anymore.
@@ -9042,9 +8954,6 @@ sd_unit_detach(dev_info_t *devi)
 
 	cv_destroy(&un->un_wcc_cv);
 
-	/* Open/close semaphore */
-	sema_destroy(&un->un_semoclose);
-
 	/* Removable media condvar. */
 	cv_destroy(&un->un_state_cv);
 
@@ -9061,8 +8970,6 @@ sd_unit_detach(dev_info_t *devi)
 
 	ddi_soft_state_free(sd_state, instance);
 
-	mutex_exit(&sd_detach_mutex);
-
 	/* This frees up the INQUIRY data associated with the device. */
 	scsi_unprobe(devp);
 
@@ -9089,10 +8996,6 @@ err_stillbusy:
 	_NOTE(NO_COMPETING_THREADS_NOW);
 
 err_remove_event:
-	mutex_enter(&sd_detach_mutex);
-	un->un_detach_count--;
-	mutex_exit(&sd_detach_mutex);
-
 	SD_TRACE(SD_LOG_ATTACH_DETACH, un, "sd_unit_detach: exit failure\n");
 	return (DDI_FAILURE);
 }
@@ -10168,15 +10071,11 @@ sdopen(dev_t *dev_p, int flag, int otyp, cred_t *cred_p)
 
 	dev = *dev_p;
 	instance = SDUNIT(dev);
-	mutex_enter(&sd_detach_mutex);
 
 	/*
-	 * Fail the open if there is no softstate for the instance, or
-	 * if another thread somewhere is trying to detach the instance.
+	 * Fail the open if there is no softstate for the instance.
 	 */
-	if (((un = ddi_get_soft_state(sd_state, instance)) == NULL) ||
-	    (un->un_detach_count != 0)) {
-		mutex_exit(&sd_detach_mutex);
+	if ((un = ddi_get_soft_state(sd_state, instance)) == NULL) {
 		/*
 		 * The probe cache only needs to be cleared when open (9e) fails
 		 * with ENXIO (4238046).
@@ -10191,41 +10090,10 @@ sdopen(dev_t *dev_p, int flag, int otyp, cred_t *cred_p)
 		return (ENXIO);
 	}
 
-	/*
-	 * The un_layer_count is to prevent another thread in specfs from
-	 * trying to detach the instance, which can happen when we are
-	 * called from a higher-layer driver instead of thru specfs.
-	 * This will not be needed when DDI provides a layered driver
-	 * interface that allows specfs to know that an instance is in
-	 * use by a layered driver & should not be detached.
-	 *
-	 * Note: the semantics for layered driver opens are exactly one
-	 * close for every open.
-	 */
-	if (otyp == OTYP_LYR) {
-		un->un_layer_count++;
-	}
-
-	/*
-	 * Keep a count of the current # of opens in progress. This is because
-	 * some layered drivers try to call us as a regular open. This can
-	 * cause problems that we cannot prevent, however by keeping this count
-	 * we can at least keep our open and detach routines from racing against
-	 * each other under such conditions.
-	 */
-	un->un_opens_in_progress++;
-	mutex_exit(&sd_detach_mutex);
-
 	nodelay  = (flag & (FNDELAY | FNONBLOCK));
 	part	 = SDPART(dev);
 	partmask = 1 << part;
 
-	/*
-	 * We use a semaphore here in order to serialize
-	 * open and close requests on the device.
-	 */
-	sema_p(&un->un_semoclose);
-
 	mutex_enter(SD_MUTEX(un));
 
 	/*
@@ -10410,12 +10278,6 @@ sdopen(dev_t *dev_p, int flag, int otyp, cred_t *cred_p)
 		sd_pm_exit(un);
 	}
 
-	sema_v(&un->un_semoclose);
-
-	mutex_enter(&sd_detach_mutex);
-	un->un_opens_in_progress--;
-	mutex_exit(&sd_detach_mutex);
-
 	SD_TRACE(SD_LOG_OPEN_CLOSE, un, "sdopen: exit success\n");
 	return (DDI_SUCCESS);
 
@@ -10433,14 +10295,6 @@ open_fail:
 		sd_pm_exit(un);
 	}
 open_failed_with_pm:
-	sema_v(&un->un_semoclose);
-
-	mutex_enter(&sd_detach_mutex);
-	un->un_opens_in_progress--;
-	if (otyp == OTYP_LYR) {
-		un->un_layer_count--;
-	}
-	mutex_exit(&sd_detach_mutex);
 
 	return (rval);
 }
@@ -10485,12 +10339,6 @@ sdclose(dev_t dev, int flag, int otyp, cred_t *cred_p)
 	SD_TRACE(SD_LOG_OPEN_CLOSE, un,
 	    "sdclose: close of part %d type %d\n", part, otyp);
 
-	/*
-	 * We use a semaphore here in order to serialize
-	 * open and close requests on the device.
-	 */
-	sema_p(&un->un_semoclose);
-
 	mutex_enter(SD_MUTEX(un));
 
 	/* Don't proceed if power is being changed. */
@@ -10645,17 +10493,6 @@ sdclose(dev_t dev, int flag, int otyp, cred_t *cred_p)
 	}
 
 	mutex_exit(SD_MUTEX(un));
-	sema_v(&un->un_semoclose);
-
-	if (otyp == OTYP_LYR) {
-		mutex_enter(&sd_detach_mutex);
-		/*
-		 * The detach routine may run when the layer count
-		 * drops to zero.
-		 */
-		un->un_layer_count--;
-		mutex_exit(&sd_detach_mutex);
-	}
 
 	return (rval);
 }
diff --git a/usr/src/uts/common/sys/scsi/targets/sddef.h b/usr/src/uts/common/sys/scsi/targets/sddef.h
index fe73b1acde..7789a22711 100644
--- a/usr/src/uts/common/sys/scsi/targets/sddef.h
+++ b/usr/src/uts/common/sys/scsi/targets/sddef.h
@@ -25,6 +25,7 @@
  * Copyright 2011 cyril.galibern@opensvc.com
  * Copyright 2017 Nexenta Systems, Inc.  All rights reserved.
  * Copyright 2019 Joyent, Inc.
+ * Copyright 2022 RackTop Systems, Inc.
  */
 
 #ifndef	_SYS_SCSI_TARGETS_SDDEF_H
@@ -346,11 +347,6 @@ struct sd_lun {
 	uchar_t	un_last_pkt_reason;	/* used to suppress multiple msgs */
 	int	un_tagflags;		/* Pkt Flags for Tagged Queueing  */
 	short	un_resvd_status;	/* Reservation Status */
-	ulong_t	un_detach_count;	/* !0 if executing detach routine */
-	ulong_t	un_layer_count;		/* Current total # of layered opens */
-	ulong_t un_opens_in_progress;	/* Current # of threads in sdopen */
-
-	ksema_t	un_semoclose;		/* serialize opens/closes */
 
 	/*
 	 * Control & status info for command throttling