Mercurial > illumos > illumos-gate

changeset 11078:97a14b54f53b

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
6885523 three-way deadlock in idm 6897731 ASSERT in idm_conn_hold under iscsi_handle_logout when target goes offline
author Peter Cudhea - Sun Microsystems - Burlington, MA United States <Peter.Cudhea@Sun.COM>
date Tue, 17 Nov 2009 14:56:25 -0500
parents 4bd474a3ea0e
children 81e8def314f5
files usr/src/uts/common/io/ib/clients/iser/iser_ib.c usr/src/uts/common/io/idm/idm.c usr/src/uts/common/io/idm/idm_conn_sm.c usr/src/uts/common/io/idm/idm_impl.c usr/src/uts/common/io/idm/idm_so.c usr/src/uts/common/io/scsi/adapters/iscsi/iscsi_conn.c usr/src/uts/common/io/scsi/adapters/iscsi/iscsi_io.c usr/src/uts/common/sys/ib/clients/iser/iser_ib.h
diffstat 8 files changed, 64 insertions(+), 75 deletions(-) [+]
line wrap: on
line diff
--- a/usr/src/uts/common/io/ib/clients/iser/iser_ib.c	Tue Nov 17 11:42:22 2009 -0800
+++ b/usr/src/uts/common/io/ib/clients/iser/iser_ib.c	Tue Nov 17 14:56:25 2009 -0500
@@ -474,7 +474,7 @@
 
 	chan = kmem_zalloc(sizeof (iser_chan_t), KM_SLEEP);
 
-	mutex_init(&chan->ic_lock, NULL, MUTEX_DRIVER, NULL);
+	mutex_init(&chan->ic_chan_lock, NULL, MUTEX_DRIVER, NULL);
 	mutex_init(&chan->ic_sq_post_lock, NULL, MUTEX_DRIVER, NULL);
 
 	/* Set up the iSER channel handle with HCA */
@@ -515,7 +515,7 @@
 	    &chan->ic_sendcq);
 	if (status != ISER_STATUS_SUCCESS) {
 		iser_ib_fini_qp(&chan->ic_qp);
-		mutex_destroy(&chan->ic_lock);
+		mutex_destroy(&chan->ic_chan_lock);
 		mutex_destroy(&chan->ic_sq_post_lock);
 		kmem_free(chan, sizeof (iser_chan_t));
 		return (NULL);
@@ -529,7 +529,7 @@
 	if (status != ISER_STATUS_SUCCESS) {
 		(void) ibt_free_cq(chan->ic_sendcq);
 		iser_ib_fini_qp(&chan->ic_qp);
-		mutex_destroy(&chan->ic_lock);
+		mutex_destroy(&chan->ic_chan_lock);
 		mutex_destroy(&chan->ic_sq_post_lock);
 		kmem_free(chan, sizeof (iser_chan_t));
 		return (NULL);
@@ -549,7 +549,7 @@
 		(void) ibt_free_cq(chan->ic_sendcq);
 		(void) ibt_free_cq(chan->ic_recvcq);
 		iser_ib_fini_qp(&chan->ic_qp);
-		mutex_destroy(&chan->ic_lock);
+		mutex_destroy(&chan->ic_chan_lock);
 		mutex_destroy(&chan->ic_sq_post_lock);
 		kmem_free(chan, sizeof (iser_chan_t));
 		return (NULL);
@@ -574,7 +574,7 @@
 	ibt_rc_returns_t	ocreturns;
 	int			status;
 
-	mutex_enter(&chan->ic_lock);
+	mutex_enter(&chan->ic_chan_lock);
 
 	/*
 	 * For connection establishment, the initiator sends a CM REQ using the
@@ -601,7 +601,7 @@
 	    sizeof (iser_private_data_t), &iser_priv_data);
 	if (status != IBT_SUCCESS) {
 		ISER_LOG(CE_NOTE, "iser_ib_open_rc_channel failed: %d", status);
-		mutex_exit(&chan->ic_lock);
+		mutex_exit(&chan->ic_chan_lock);
 		return (status);
 	}
 
@@ -630,11 +630,11 @@
 
 	if (status != IBT_SUCCESS) {
 		ISER_LOG(CE_NOTE, "iser_ib_open_rc_channel failed: %d", status);
-		mutex_exit(&chan->ic_lock);
+		mutex_exit(&chan->ic_chan_lock);
 		return (status);
 	}
 
-	mutex_exit(&chan->ic_lock);
+	mutex_exit(&chan->ic_chan_lock);
 	return (IDM_STATUS_SUCCESS);
 }
 
@@ -648,14 +648,14 @@
 {
 	int			status;
 
-	mutex_enter(&chan->ic_lock);
+	mutex_enter(&chan->ic_chan_lock);
 	status = ibt_close_rc_channel(chan->ic_chanhdl, IBT_BLOCKING, NULL,
 	    0, NULL, NULL, 0);
 	if (status != IBT_SUCCESS) {
 		ISER_LOG(CE_NOTE, "iser_ib_close_rc_channel: "
 		    "ibt_close_rc_channel failed: status (%d)", status);
 	}
-	mutex_exit(&chan->ic_lock);
+	mutex_exit(&chan->ic_chan_lock);
 }
 
 /*
@@ -706,7 +706,7 @@
 	ibt_free_cq(chan->ic_recvcq);
 
 	/* Free the chan handle */
-	mutex_destroy(&chan->ic_lock);
+	mutex_destroy(&chan->ic_chan_lock);
 	kmem_free(chan, sizeof (iser_chan_t));
 }
 
--- a/usr/src/uts/common/io/idm/idm.c	Tue Nov 17 11:42:22 2009 -0800
+++ b/usr/src/uts/common/io/idm/idm.c	Tue Nov 17 14:56:25 2009 -0500
@@ -1260,20 +1260,23 @@
 	ASSERT(ic != NULL);
 
 	/* Don't allocate new tasks if we are not in FFP */
-	mutex_enter(&ic->ic_state_mutex);
 	if (!ic->ic_ffp) {
-		mutex_exit(&ic->ic_state_mutex);
 		return (NULL);
 	}
 	idt = kmem_cache_alloc(idm.idm_task_cache, KM_NOSLEEP);
 	if (idt == NULL) {
-		mutex_exit(&ic->ic_state_mutex);
 		return (NULL);
 	}
 
 	ASSERT(list_is_empty(&idt->idt_inbufv));
 	ASSERT(list_is_empty(&idt->idt_outbufv));
 
+	mutex_enter(&ic->ic_state_mutex);
+	if (!ic->ic_ffp) {
+		mutex_exit(&ic->ic_state_mutex);
+		kmem_cache_free(idm.idm_task_cache, idt);
+		return (NULL);
+	}
 	idm_conn_hold(ic);
 	mutex_exit(&ic->ic_state_mutex);
 
@@ -1355,7 +1358,7 @@
 
 	/*
 	 * It's possible for items to still be in the idt_inbufv list if
-	 * they were added after idm_task_cleanup was called.  We rely on
+	 * they were added after idm_free_task_rsrc was called.  We rely on
 	 * STMF to free all buffers associated with the task however STMF
 	 * doesn't know that we have this reference to the buffers.
 	 * Use list_create so that we don't end up with stale references
@@ -1700,51 +1703,6 @@
 	(*idt->idt_ic->ic_conn_ops.icb_task_aborted)(idt, status);
 }
 
-void
-idm_task_cleanup(idm_task_t *idt)
-{
-	idm_buf_t *idb, *next_idb;
-	list_t		tmp_buflist;
-	ASSERT((idt->idt_state == TASK_SUSPENDED) ||
-	    (idt->idt_state == TASK_ABORTED));
-
-	list_create(&tmp_buflist, sizeof (idm_buf_t),
-	    offsetof(idm_buf_t, idb_buflink));
-
-	/*
-	 * Remove all the buffers from the task and add them to a
-	 * temporary local list -- we do this so that we can hold
-	 * the task lock and prevent the task from going away if
-	 * the client decides to call idm_task_done/idm_task_free.
-	 * This could happen during abort in iscsit.
-	 */
-	mutex_enter(&idt->idt_mutex);
-	for (idb = list_head(&idt->idt_inbufv);
-	    idb != NULL;
-	    idb = next_idb) {
-		next_idb = list_next(&idt->idt_inbufv, idb);
-		idm_buf_unbind_in_locked(idt, idb);
-		list_insert_tail(&tmp_buflist, idb);
-	}
-
-	for (idb = list_head(&idt->idt_outbufv);
-	    idb != NULL;
-	    idb = next_idb) {
-		next_idb = list_next(&idt->idt_outbufv, idb);
-		idm_buf_unbind_out_locked(idt, idb);
-		list_insert_tail(&tmp_buflist, idb);
-	}
-	mutex_exit(&idt->idt_mutex);
-
-	for (idb = list_head(&tmp_buflist); idb != NULL; idb = next_idb) {
-		next_idb = list_next(&tmp_buflist, idb);
-		list_remove(&tmp_buflist, idb);
-		(*idb->idb_buf_cb)(idb, IDM_STATUS_ABORTED);
-	}
-	list_destroy(&tmp_buflist);
-}
-
-
 /*
  * idm_pdu_tx
  *
--- a/usr/src/uts/common/io/idm/idm_conn_sm.c	Tue Nov 17 11:42:22 2009 -0800
+++ b/usr/src/uts/common/io/idm/idm_conn_sm.c	Tue Nov 17 14:56:25 2009 -0500
@@ -220,6 +220,8 @@
 {
 	idm_conn_event_ctx_t	*event_ctx;
 
+	ASSERT(mutex_owned(&ic->ic_state_mutex));
+
 	idm_sm_audit_event(&ic->ic_state_audit, SAS_IDM_CONN,
 	    (int)ic->ic_state, (int)event, event_info);
 
@@ -573,6 +575,7 @@
 		ic->ic_client_callback = NULL;
 		idm_pdu_complete(pdu, pdu->isp_status);
 		(void) idm_notify_client(ic, CN_LOGIN_FAIL, NULL);
+		(void) untimeout(ic->ic_state_timeout);
 		idm_update_state(ic, CS_S9_INIT_ERROR, event_ctx);
 		break;
 	case CE_LOGIN_FAIL_SND:
@@ -681,6 +684,7 @@
 	case CE_MISC_RX:
 	case CE_TX_PROTOCOL_ERROR:
 	case CE_RX_PROTOCOL_ERROR:
+	case CE_LOGIN_TIMEOUT:
 		/* Don't care */
 		break;
 	default:
@@ -796,6 +800,7 @@
 	case CE_RX_PROTOCOL_ERROR:
 	case CE_MISC_TX:
 	case CE_MISC_RX:
+	case CE_LOGIN_TIMEOUT:
 		/* Don't care */
 		break;
 	default:
@@ -875,6 +880,7 @@
 	case CE_RX_PROTOCOL_ERROR:
 	case CE_MISC_TX:
 	case CE_MISC_RX:
+	case CE_LOGIN_TIMEOUT:
 		/* Don't care */
 		break;
 	default:
@@ -930,6 +936,7 @@
 	case CE_MISC_TX:
 	case CE_MISC_RX:
 	case CE_TRANSPORT_FAIL:
+	case CE_LOGIN_TIMEOUT:
 	case CE_LOGOUT_TIMEOUT:
 		/* Don't care */
 		break;
@@ -992,6 +999,7 @@
 	case CE_RX_PROTOCOL_ERROR:
 	case CE_MISC_TX:
 	case CE_MISC_RX:
+	case CE_LOGIN_TIMEOUT:
 	case CE_LOGOUT_TIMEOUT:
 		/* Don't care */
 		break;
@@ -1149,8 +1157,8 @@
 
 		if (ic->ic_reinstate_conn) {
 			/* Connection reinstatement is complete */
-			idm_conn_event_locked(ic->ic_reinstate_conn,
-			    CE_CONN_REINSTATE_SUCCESS, NULL, CT_NONE);
+			idm_conn_event(ic->ic_reinstate_conn,
+			    CE_CONN_REINSTATE_SUCCESS, NULL);
 		}
 		break;
 	case CS_S6_IN_LOGOUT:
--- a/usr/src/uts/common/io/idm/idm_impl.c	Tue Nov 17 11:42:22 2009 -0800
+++ b/usr/src/uts/common/io/idm/idm_impl.c	Tue Nov 17 14:56:25 2009 -0500
@@ -388,6 +388,11 @@
 	idm_conn_t	*ic;
 	idm_status_t	rc;
 
+	/*
+	 * Skip some work if we can already tell we are going offline.
+	 * Otherwise we will destroy this connection later as part of
+	 * shutting down the svc.
+	 */
 	mutex_enter(&is->is_mutex);
 	if (!is->is_online) {
 		mutex_exit(&is->is_mutex);
--- a/usr/src/uts/common/io/idm/idm_so.c	Tue Nov 17 11:42:22 2009 -0800
+++ b/usr/src/uts/common/io/idm/idm_so.c	Tue Nov 17 14:56:25 2009 -0500
@@ -1317,7 +1317,7 @@
 static idm_status_t
 idm_so_free_task_rsrc(idm_task_t *idt)
 {
-	idm_buf_t	*idb;
+	idm_buf_t	*idb, *next_idb;
 
 	/*
 	 * There is nothing to cleanup on initiator connections
@@ -1336,8 +1336,8 @@
 	 */
 	mutex_enter(&idt->idt_mutex);
 
-	for (idb = list_head(&idt->idt_outbufv); idb != NULL;
-	    idb = list_next(&idt->idt_outbufv, idb)) {
+	for (idb = list_head(&idt->idt_outbufv); idb != NULL; idb = next_idb) {
+		next_idb = list_next(&idt->idt_outbufv, idb);
 		if (idb->idb_in_transport) {
 			/*
 			 * idm_buf_rx_from_ini_done releases idt->idt_mutex
@@ -1353,8 +1353,8 @@
 		}
 	}
 
-	for (idb = list_head(&idt->idt_inbufv); idb != NULL;
-	    idb = list_next(&idt->idt_inbufv, idb)) {
+	for (idb = list_head(&idt->idt_inbufv); idb != NULL; idb = next_idb) {
+		next_idb = list_next(&idt->idt_inbufv, idb);
 		/*
 		 * We want to remove these items from the tx_list as well,
 		 * but knowing it's in the idt_inbufv list is not a guarantee
--- a/usr/src/uts/common/io/scsi/adapters/iscsi/iscsi_conn.c	Tue Nov 17 11:42:22 2009 -0800
+++ b/usr/src/uts/common/io/scsi/adapters/iscsi/iscsi_conn.c	Tue Nov 17 14:56:25 2009 -0500
@@ -253,9 +253,11 @@
 	case ISCSI_CONN_STATE_FREE:
 		break;
 	case ISCSI_CONN_STATE_LOGGED_IN:
-		if (icp->conn_state_ffp)
+		if (icp->conn_state_ffp) {
+			/* Hold is released in iscsi_handle_logout */
+			idm_conn_hold(icp->conn_ic);
 			(void) iscsi_handle_logout(icp);
-		else {
+		} else {
 			icp->conn_state_destroy = B_FALSE;
 			mutex_exit(&icp->conn_state_mutex);
 			return (ISCSI_STATUS_INTERNAL_ERROR);
--- a/usr/src/uts/common/io/scsi/adapters/iscsi/iscsi_io.c	Tue Nov 17 11:42:22 2009 -0800
+++ b/usr/src/uts/common/io/scsi/adapters/iscsi/iscsi_io.c	Tue Nov 17 14:56:25 2009 -0500
@@ -1422,6 +1422,9 @@
 		icp->conn_async_logout = B_TRUE;
 		mutex_exit(&icp->conn_state_mutex);
 
+		/* Hold is released in iscsi_handle_logout. */
+		idm_conn_hold(ic);
+
 		/* Target has requested this connection to logout. */
 		itp = kmem_zalloc(sizeof (iscsi_task_t), KM_SLEEP);
 		itp->t_arg = icp;
@@ -1429,6 +1432,7 @@
 		if (ddi_taskq_dispatch(isp->sess_taskq,
 		    (void(*)())iscsi_logout_start, itp, DDI_SLEEP) !=
 		    DDI_SUCCESS) {
+			idm_conn_rele(ic);
 			/* Disconnect if we couldn't dispatch the task */
 			idm_ini_conn_disconnect(ic);
 		}
@@ -1478,6 +1482,9 @@
 		 * now we will request a logout.  We can't
 		 * just ignore this or it might force corruption?
 		 */
+
+		/* Hold is released in iscsi_handle_logout */
+		idm_conn_hold(ic);
 		itp = kmem_zalloc(sizeof (iscsi_task_t), KM_SLEEP);
 		itp->t_arg = icp;
 		itp->t_blocking = B_FALSE;
@@ -1485,6 +1492,7 @@
 		    (void(*)())iscsi_logout_start, itp, DDI_SLEEP) !=
 		    DDI_SUCCESS) {
 			/* Disconnect if we couldn't dispatch the task */
+			idm_conn_rele(ic);
 			idm_ini_conn_disconnect(ic);
 		}
 		break;
@@ -2656,7 +2664,8 @@
 }
 
 /*
- * iscsi_lgoout_start - task handler for deferred logout
+ * iscsi_logout_start - task handler for deferred logout
+ * Acquire a hold before call, released in iscsi_handle_logout
  */
 static void
 iscsi_logout_start(void *arg)
@@ -2674,6 +2683,7 @@
 /*
  * iscsi_handle_logout - This function will issue a logout for
  * the session from a specific connection.
+ * Acquire idm_conn_hold before call.  Released internally.
  */
 iscsi_status_t
 iscsi_handle_logout(iscsi_conn_t *icp)
@@ -2691,11 +2701,17 @@
 	ASSERT(mutex_owned(&icp->conn_state_mutex));
 
 	/*
-	 * We may want to explicitly disconnect if something goes wrong so
-	 * grab a hold to ensure that the IDM connection context can't
-	 * disappear.
+	 * If the connection has already gone down (e.g. if the transport
+	 * failed between when this LOGOUT was generated and now) then we
+	 * can and must skip sending the LOGOUT.  Check the same condition
+	 * we use below to determine that connection has "settled".
 	 */
-	idm_conn_hold(ic);
+	if ((icp->conn_state == ISCSI_CONN_STATE_FREE) ||
+	    (icp->conn_state == ISCSI_CONN_STATE_FAILED) ||
+	    (icp->conn_state == ISCSI_CONN_STATE_POLLING)) {
+		idm_conn_rele(ic);
+		return (0);
+	}
 
 	icmdp = iscsi_cmd_alloc(icp, KM_SLEEP);
 	ASSERT(icmdp != NULL);
--- a/usr/src/uts/common/sys/ib/clients/iser/iser_ib.h	Tue Nov 17 11:42:22 2009 -0800
+++ b/usr/src/uts/common/sys/ib/clients/iser/iser_ib.h	Tue Nov 17 14:56:25 2009 -0500
@@ -115,7 +115,7 @@
  * iSER RC channel information
  */
 typedef struct iser_chan_s {
-	kmutex_t		ic_lock;
+	kmutex_t		ic_chan_lock;
 
 	/* IBT channel handle */
 	ibt_channel_hdl_t	ic_chanhdl;