[PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi folks,

Here is -v4 series to address the set of of LUN_RESET
active I/O + TMR se_cmd->cmd_kref < 0 bugs as reported
recently by Quinn & Co.  This can occur during active
I/O remote port TMR LUN_RESET with multi-port LIO
configurations.

To address this bug, we add a __target_check_io_state()
common handler for ABORT_TASK + LUN_RESET I/O abort
cases, and move the remaining se_cmd SGL page + release
into target_free_cmd_mem() to now be called directly
from final target_release_cmd_kref() callback.

It also adds a target_wait_free_cmd() helper and makes
transport_generic_free_cmd() aware of CMD_T_ABORTED
status during concurrent session disconnects, and
introduces CMD_T_FABRIC_STOP bit to signal this special
case.

Currently this series is running atop v4.5-rc1 + v3.14.y,
and with iscsi-target ports is able to survive active
I/O remote-port LUN resets, plus remote-port LUN_RESET
with concurrent simulated session disconnects.

At this point the changes are stable with iscsi-target
ports, and as Himanshu + Co can verify with tcm_qla2xxx
should be considered ready to merge for -rc4.

Please review + test.

--nab

v4 changes:

- Add explicit CMD_T_FABRIC_STOP check and drop cmd_wait_set
  bit set usage in __target_check_io_state().
- Set early CMD_T_TAS in __target_check_io_state to avoid
  potential race in transport_send_task_abort() with shutdown.
- Add fabric_stop + aborted checks in __transport_wait_for_tasks()
  in order to let TMR CMD_T_ABORTED se_cmd shutdown complete
  during concurrent session disconnect.
- Fix race with driver SCF_SEND_DELAYED_TAS handling when
  __transport_check_aborted_status() could happen before
  transport_send_task_abort() in TMR kthread context.

Nicholas Bellinger (5):
  target: Fix LUN_RESET active I/O handling for ACK_KREF
  target: Fix LUN_RESET active TMR descriptor handling
  target: Fix TAS handling for multi-session se_node_acls
  target: Fix remote-port TMR ABORT + se_cmd fabric stop
  target: Fix race with SCF_SEND_DELAYED_TAS handling

 drivers/target/target_core_tmr.c       | 139 ++++++++++++-----
 drivers/target/target_core_transport.c | 278 +++++++++++++++++++++++----------
 include/target/target_core_base.h      |   3 +
 3 files changed, 301 insertions(+), 119 deletions(-)

-- 
1.9.1

[PATCH-v4 1/5] target: Fix LUN_RESET active I/O handling for ACK_KREF

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch fixes a NULL pointer se_cmd->cmd_kref < 0
refcount bug during TMR LUN_RESET with active se_cmd
I/O, that can be triggered during se_cmd descriptor
shutdown + release via core_tmr_drain_state_list() code.

To address this bug, add common __target_check_io_state()
helper for ABORT_TASK + LUN_RESET w/ CMD_T_COMPLETE
checking, and set CMD_T_ABORTED + obtain ->cmd_kref for
both cases ahead of last target_put_sess_cmd() after
TFO->aborted_task() -> transport_cmd_finish_abort()
callback has completed.

It also introduces SCF_ACK_KREF to determine when
transport_cmd_finish_abort() needs to drop the second
extra reference, ahead of calling target_put_sess_cmd()
for the final kref_put(&se_cmd->cmd_kref).

It also updates transport_cmd_check_stop() to avoid
holding se_cmd->t_state_lock while dropping se_cmd
device state via target_remove_from_state_list(), now
that core_tmr_drain_state_list() is holding the
se_device lock while checking se_cmd state from
within TMR logic.

Finally, move transport_put_cmd() release of SGL +
TMR + extended CDB memory into target_free_cmd_mem()
in order to avoid potential resource leaks in TMR
ABORT_TASK + LUN_RESET code-paths.  Also update
target_release_cmd_kref() accordingly.

Reviewed-by: Quinn Tran <quinn.tran@qlogic.com>
Cc: Himanshu Madhani <himanshu.madhani@qlogic.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Andy Grover <agrover@redhat.com>
Cc: Mike Christie <mchristi@redhat.com>
Cc: stable@vger.kernel.org # 3.10+
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_tmr.c       | 69 ++++++++++++++++++++++------------
 drivers/target/target_core_transport.c | 67 ++++++++++++++-------------------
 include/target/target_core_base.h      |  1 +
 3 files changed, 76 insertions(+), 61 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index fcdcb11..fb3decc 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -107,6 +107,34 @@ static int target_check_cdb_and_preempt(struct list_head *list,
 	return 1;
 }
 
+static bool __target_check_io_state(struct se_cmd *se_cmd)
+{
+	struct se_session *sess = se_cmd->se_sess;
+
+	assert_spin_locked(&sess->sess_cmd_lock);
+	WARN_ON_ONCE(!irqs_disabled());
+	/*
+	 * If command already reached CMD_T_COMPLETE state within
+	 * target_complete_cmd(), this se_cmd has been passed to
+	 * fabric driver and will not be aborted.
+	 *
+	 * Otherwise, obtain a local se_cmd->cmd_kref now for TMR
+	 * ABORT_TASK + LUN_RESET for CMD_T_ABORTED processing as
+	 * long as se_cmd->cmd_kref is still active unless zero.
+	 */
+	spin_lock(&se_cmd->t_state_lock);
+	if (se_cmd->transport_state & CMD_T_COMPLETE) {
+		pr_debug("Attempted to abort io tag: %llu already complete,"
+			" skipping\n", se_cmd->tag);
+		spin_unlock(&se_cmd->t_state_lock);
+		return false;
+	}
+	se_cmd->transport_state |= CMD_T_ABORTED;
+	spin_unlock(&se_cmd->t_state_lock);
+
+	return kref_get_unless_zero(&se_cmd->cmd_kref);
+}
+
 void core_tmr_abort_task(
 	struct se_device *dev,
 	struct se_tmr_req *tmr,
@@ -130,34 +158,22 @@ void core_tmr_abort_task(
 		if (tmr->ref_task_tag != ref_tag)
 			continue;
 
-		if (!kref_get_unless_zero(&se_cmd->cmd_kref))
-			continue;
-
 		printk("ABORT_TASK: Found referenced %s task_tag: %llu\n",
 			se_cmd->se_tfo->get_fabric_name(), ref_tag);
 
-		spin_lock(&se_cmd->t_state_lock);
-		if (se_cmd->transport_state & CMD_T_COMPLETE) {
-			printk("ABORT_TASK: ref_tag: %llu already complete,"
-			       " skipping\n", ref_tag);
-			spin_unlock(&se_cmd->t_state_lock);
+		if (!__target_check_io_state(se_cmd)) {
 			spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
-
 			target_put_sess_cmd(se_cmd);
-
 			goto out;
 		}
-		se_cmd->transport_state |= CMD_T_ABORTED;
-		spin_unlock(&se_cmd->t_state_lock);
-
 		list_del_init(&se_cmd->se_cmd_list);
 		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 
 		cancel_work_sync(&se_cmd->work);
 		transport_wait_for_tasks(se_cmd);
 
-		target_put_sess_cmd(se_cmd);
 		transport_cmd_finish_abort(se_cmd, true);
+		target_put_sess_cmd(se_cmd);
 
 		printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for"
 				" ref_tag: %llu\n", ref_tag);
@@ -242,8 +258,10 @@ static void core_tmr_drain_state_list(
 	struct list_head *preempt_and_abort_list)
 {
 	LIST_HEAD(drain_task_list);
+	struct se_session *sess;
 	struct se_cmd *cmd, *next;
 	unsigned long flags;
+	int rc;
 
 	/*
 	 * Complete outstanding commands with TASK_ABORTED SAM status.
@@ -282,6 +300,16 @@ static void core_tmr_drain_state_list(
 		if (prout_cmd == cmd)
 			continue;
 
+		sess = cmd->se_sess;
+		if (WARN_ON_ONCE(!sess))
+			continue;
+
+		spin_lock(&sess->sess_cmd_lock);
+		rc = __target_check_io_state(cmd);
+		spin_unlock(&sess->sess_cmd_lock);
+		if (!rc)
+			continue;
+
 		list_move_tail(&cmd->state_list, &drain_task_list);
 		cmd->state_active = false;
 	}
@@ -289,7 +317,7 @@ static void core_tmr_drain_state_list(
 
 	while (!list_empty(&drain_task_list)) {
 		cmd = list_entry(drain_task_list.next, struct se_cmd, state_list);
-		list_del(&cmd->state_list);
+		list_del_init(&cmd->state_list);
 
 		pr_debug("LUN_RESET: %s cmd: %p"
 			" ITT/CmdSN: 0x%08llx/0x%08x, i_state: %d, t_state: %d"
@@ -313,16 +341,11 @@ static void core_tmr_drain_state_list(
 		 * loop above, but we do it down here given that
 		 * cancel_work_sync may block.
 		 */
-		if (cmd->t_state == TRANSPORT_COMPLETE)
-			cancel_work_sync(&cmd->work);
-
-		spin_lock_irqsave(&cmd->t_state_lock, flags);
-		target_stop_cmd(cmd, &flags);
-
-		cmd->transport_state |= CMD_T_ABORTED;
-		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+		cancel_work_sync(&cmd->work);
+		transport_wait_for_tasks(cmd);
 
 		core_tmr_handle_tas_abort(tmr_nacl, cmd, tas);
+		target_put_sess_cmd(cmd);
 	}
 }
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 9f3608e..af52f8b 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -534,9 +534,6 @@ void transport_deregister_session(struct se_session *se_sess)
 }
 EXPORT_SYMBOL(transport_deregister_session);
 
-/*
- * Called with cmd->t_state_lock held.
- */
 static void target_remove_from_state_list(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
@@ -561,10 +558,6 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&cmd->t_state_lock, flags);
-	if (write_pending)
-		cmd->t_state = TRANSPORT_WRITE_PENDING;
-
 	if (remove_from_lists) {
 		target_remove_from_state_list(cmd);
 
@@ -574,6 +567,10 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
 		cmd->se_lun = NULL;
 	}
 
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	if (write_pending)
+		cmd->t_state = TRANSPORT_WRITE_PENDING;
+
 	/*
 	 * Determine if frontend context caller is requesting the stopping of
 	 * this command for frontend exceptions.
@@ -627,6 +624,8 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd)
 
 void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
 {
+	bool ack_kref = (cmd->se_cmd_flags & SCF_ACK_KREF);
+
 	if (cmd->se_cmd_flags & SCF_SE_LUN_CMD)
 		transport_lun_remove_cmd(cmd);
 	/*
@@ -638,7 +637,7 @@ void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
 
 	if (transport_cmd_check_stop_to_fabric(cmd))
 		return;
-	if (remove)
+	if (remove && ack_kref)
 		transport_put_cmd(cmd);
 }
 
@@ -706,7 +705,7 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
 	 * Check for case where an explicit ABORT_TASK has been received
 	 * and transport_wait_for_tasks() will be waiting for completion..
 	 */
-	if (cmd->transport_state & CMD_T_ABORTED &&
+	if (cmd->transport_state & CMD_T_ABORTED ||
 	    cmd->transport_state & CMD_T_STOP) {
 		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 		complete_all(&cmd->t_transport_stop_comp);
@@ -2222,20 +2221,14 @@ static inline void transport_free_pages(struct se_cmd *cmd)
 }
 
 /**
- * transport_release_cmd - free a command
- * @cmd:       command to free
+ * transport_put_cmd - release a reference to a command
+ * @cmd:       command to release
  *
- * This routine unconditionally frees a command, and reference counting
- * or list removal must be done in the caller.
+ * This routine releases our reference to the command and frees it if possible.
  */
-static int transport_release_cmd(struct se_cmd *cmd)
+static int transport_put_cmd(struct se_cmd *cmd)
 {
 	BUG_ON(!cmd->se_tfo);
-
-	if (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
-		core_tmr_release_req(cmd->se_tmr_req);
-	if (cmd->t_task_cdb != cmd->__t_task_cdb)
-		kfree(cmd->t_task_cdb);
 	/*
 	 * If this cmd has been setup with target_get_sess_cmd(), drop
 	 * the kref and call ->release_cmd() in kref callback.
@@ -2243,18 +2236,6 @@ static int transport_release_cmd(struct se_cmd *cmd)
 	return target_put_sess_cmd(cmd);
 }
 
-/**
- * transport_put_cmd - release a reference to a command
- * @cmd:       command to release
- *
- * This routine releases our reference to the command and frees it if possible.
- */
-static int transport_put_cmd(struct se_cmd *cmd)
-{
-	transport_free_pages(cmd);
-	return transport_release_cmd(cmd);
-}
-
 void *transport_kmap_data_sg(struct se_cmd *cmd)
 {
 	struct scatterlist *sg = cmd->t_data_sg;
@@ -2452,14 +2433,13 @@ static void transport_write_pending_qf(struct se_cmd *cmd)
 
 int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
 {
-	unsigned long flags;
 	int ret = 0;
 
 	if (!(cmd->se_cmd_flags & SCF_SE_LUN_CMD)) {
 		if (wait_for_tasks && (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB))
-			 transport_wait_for_tasks(cmd);
+			transport_wait_for_tasks(cmd);
 
-		ret = transport_release_cmd(cmd);
+		ret = transport_put_cmd(cmd);
 	} else {
 		if (wait_for_tasks)
 			transport_wait_for_tasks(cmd);
@@ -2468,11 +2448,8 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
 		 * has already added se_cmd to state_list, but fabric has
 		 * failed command before I/O submission.
 		 */
-		if (cmd->state_active) {
-			spin_lock_irqsave(&cmd->t_state_lock, flags);
+		if (cmd->state_active)
 			target_remove_from_state_list(cmd);
-			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-		}
 
 		if (cmd->se_lun)
 			transport_lun_remove_cmd(cmd);
@@ -2517,6 +2494,16 @@ out:
 }
 EXPORT_SYMBOL(target_get_sess_cmd);
 
+static void target_free_cmd_mem(struct se_cmd *cmd)
+{
+	transport_free_pages(cmd);
+
+	if (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
+		core_tmr_release_req(cmd->se_tmr_req);
+	if (cmd->t_task_cdb != cmd->__t_task_cdb)
+		kfree(cmd->t_task_cdb);
+}
+
 static void target_release_cmd_kref(struct kref *kref)
 {
 	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
@@ -2526,17 +2513,20 @@ static void target_release_cmd_kref(struct kref *kref)
 	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
 	if (list_empty(&se_cmd->se_cmd_list)) {
 		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+		target_free_cmd_mem(se_cmd);
 		se_cmd->se_tfo->release_cmd(se_cmd);
 		return;
 	}
 	if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) {
 		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+		target_free_cmd_mem(se_cmd);
 		complete(&se_cmd->cmd_wait_comp);
 		return;
 	}
 	list_del(&se_cmd->se_cmd_list);
 	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 
+	target_free_cmd_mem(se_cmd);
 	se_cmd->se_tfo->release_cmd(se_cmd);
 }
 
@@ -2548,6 +2538,7 @@ int target_put_sess_cmd(struct se_cmd *se_cmd)
 	struct se_session *se_sess = se_cmd->se_sess;
 
 	if (!se_sess) {
+		target_free_cmd_mem(se_cmd);
 		se_cmd->se_tfo->release_cmd(se_cmd);
 		return 1;
 	}
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 5d82816..1a76726 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -140,6 +140,7 @@ enum se_cmd_flags_table {
 	SCF_COMPARE_AND_WRITE		= 0x00080000,
 	SCF_COMPARE_AND_WRITE_POST	= 0x00100000,
 	SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x00200000,
+	SCF_ACK_KREF			= 0x00400000,
 };
 
 /* struct se_dev_entry->lun_flags and struct se_lun->lun_access */
-- 
1.9.1

[PATCH-v4 2/5] target: Fix LUN_RESET active TMR descriptor handling

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch fixes a NULL pointer se_cmd->cmd_kref < 0
refcount bug during TMR LUN_RESET with active TMRs,
triggered during se_cmd + se_tmr_req descriptor
shutdown + release via core_tmr_drain_tmr_list().

To address this bug, go ahead and obtain a local
kref_get_unless_zero(&se_cmd->cmd_kref) for active I/O
to set CMD_T_ABORTED, and transport_wait_for_tasks()
followed by the final target_put_sess_cmd() to drop
the local ->cmd_kref.

Also add two new checks within target_tmr_work() to
avoid CMD_T_ABORTED -> TFO->queue_tm_rsp() callbacks
ahead of invoking the backend -> fabric put in
transport_cmd_check_stop_to_fabric().

For good measure, also change core_tmr_release_req()
to use list_del_init() ahead of se_tmr_req memory
free.

Reviewed-by: Quinn Tran <quinn.tran@qlogic.com>
Cc: Himanshu Madhani <himanshu.madhani@qlogic.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Andy Grover <agrover@redhat.com>
Cc: Mike Christie <mchristi@redhat.com>
Cc: stable@vger.kernel.org # 3.10+
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_tmr.c       | 22 +++++++++++++++++++++-
 drivers/target/target_core_transport.c | 17 +++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index fb3decc..072af07 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -68,7 +68,7 @@ void core_tmr_release_req(struct se_tmr_req *tmr)
 
 	if (dev) {
 		spin_lock_irqsave(&dev->se_tmr_lock, flags);
-		list_del(&tmr->tmr_list);
+		list_del_init(&tmr->tmr_list);
 		spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
 	}
 
@@ -194,9 +194,11 @@ static void core_tmr_drain_tmr_list(
 	struct list_head *preempt_and_abort_list)
 {
 	LIST_HEAD(drain_tmr_list);
+	struct se_session *sess;
 	struct se_tmr_req *tmr_p, *tmr_pp;
 	struct se_cmd *cmd;
 	unsigned long flags;
+	bool rc;
 	/*
 	 * Release all pending and outgoing TMRs aside from the received
 	 * LUN_RESET tmr..
@@ -222,17 +224,31 @@ static void core_tmr_drain_tmr_list(
 		if (target_check_cdb_and_preempt(preempt_and_abort_list, cmd))
 			continue;
 
+		sess = cmd->se_sess;
+		if (WARN_ON_ONCE(!sess))
+			continue;
+
+		spin_lock(&sess->sess_cmd_lock);
 		spin_lock(&cmd->t_state_lock);
 		if (!(cmd->transport_state & CMD_T_ACTIVE)) {
 			spin_unlock(&cmd->t_state_lock);
+			spin_unlock(&sess->sess_cmd_lock);
 			continue;
 		}
 		if (cmd->t_state == TRANSPORT_ISTATE_PROCESSING) {
 			spin_unlock(&cmd->t_state_lock);
+			spin_unlock(&sess->sess_cmd_lock);
 			continue;
 		}
+		cmd->transport_state |= CMD_T_ABORTED;
 		spin_unlock(&cmd->t_state_lock);
 
+		rc = kref_get_unless_zero(&cmd->cmd_kref);
+		spin_unlock(&sess->sess_cmd_lock);
+		if (!rc) {
+			printk("LUN_RESET TMR: non-zero kref_get_unless_zero\n");
+			continue;
+		}
 		list_move_tail(&tmr_p->tmr_list, &drain_tmr_list);
 	}
 	spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
@@ -246,7 +262,11 @@ static void core_tmr_drain_tmr_list(
 			(preempt_and_abort_list) ? "Preempt" : "", tmr_p,
 			tmr_p->function, tmr_p->response, cmd->t_state);
 
+		cancel_work_sync(&cmd->work);
+		transport_wait_for_tasks(cmd);
+
 		transport_cmd_finish_abort(cmd, 1);
+		target_put_sess_cmd(cmd);
 	}
 }
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index af52f8b..94e372a 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2900,8 +2900,17 @@ static void target_tmr_work(struct work_struct *work)
 	struct se_cmd *cmd = container_of(work, struct se_cmd, work);
 	struct se_device *dev = cmd->se_dev;
 	struct se_tmr_req *tmr = cmd->se_tmr_req;
+	unsigned long flags;
 	int ret;
 
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	if (cmd->transport_state & CMD_T_ABORTED) {
+		tmr->response = TMR_FUNCTION_REJECTED;
+		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+		goto check_stop;
+	}
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
 	switch (tmr->function) {
 	case TMR_ABORT_TASK:
 		core_tmr_abort_task(dev, tmr, cmd->se_sess);
@@ -2934,9 +2943,17 @@ static void target_tmr_work(struct work_struct *work)
 		break;
 	}
 
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	if (cmd->transport_state & CMD_T_ABORTED) {
+		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+		goto check_stop;
+	}
 	cmd->t_state = TRANSPORT_ISTATE_PROCESSING;
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
 	cmd->se_tfo->queue_tm_rsp(cmd);
 
+check_stop:
 	transport_cmd_check_stop_to_fabric(cmd);
 }
 
-- 
1.9.1

[PATCH-v4 3/5] target: Fix TAS handling for multi-session se_node_acls

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch fixes a bug in TMR task aborted status (TAS)
handling when multiple sessions are connected to the
same target WWPN endpoint and se_node_acl descriptor,
resulting in TASK_ABORTED status to not be generated
for aborted se_cmds on the remote port.

This is due to core_tmr_handle_tas_abort() incorrectly
comparing se_node_acl instead of se_session, for which
the multi-session case is expected to be sharing the
same se_node_acl.

Instead, go ahead and update core_tmr_handle_tas_abort()
to compare tmr_sess + cmd->se_sess in order to determine
if the LUN_RESET was received on a different I_T nexus,
and TASK_ABORTED status response needs to be generated.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Himanshu Madhani <himanshu.madhani@qlogic.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Andy Grover <agrover@redhat.com>
Cc: Mike Christie <mchristi@redhat.com>
Cc: stable@vger.kernel.org # 3.10+
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_tmr.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 072af07..3e0d77a 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -76,7 +76,7 @@ void core_tmr_release_req(struct se_tmr_req *tmr)
 }
 
 static void core_tmr_handle_tas_abort(
-	struct se_node_acl *tmr_nacl,
+	struct se_session *tmr_sess,
 	struct se_cmd *cmd,
 	int tas)
 {
@@ -84,7 +84,7 @@ static void core_tmr_handle_tas_abort(
 	/*
 	 * TASK ABORTED status (TAS) bit support
 	 */
-	if ((tmr_nacl && (tmr_nacl != cmd->se_sess->se_node_acl)) && tas) {
+	if (tmr_sess && tmr_sess != cmd->se_sess && tas) {
 		remove = false;
 		transport_send_task_abort(cmd);
 	}
@@ -273,7 +273,7 @@ static void core_tmr_drain_tmr_list(
 static void core_tmr_drain_state_list(
 	struct se_device *dev,
 	struct se_cmd *prout_cmd,
-	struct se_node_acl *tmr_nacl,
+	struct se_session *tmr_sess,
 	int tas,
 	struct list_head *preempt_and_abort_list)
 {
@@ -364,7 +364,7 @@ static void core_tmr_drain_state_list(
 		cancel_work_sync(&cmd->work);
 		transport_wait_for_tasks(cmd);
 
-		core_tmr_handle_tas_abort(tmr_nacl, cmd, tas);
+		core_tmr_handle_tas_abort(tmr_sess, cmd, tas);
 		target_put_sess_cmd(cmd);
 	}
 }
@@ -377,6 +377,7 @@ int core_tmr_lun_reset(
 {
 	struct se_node_acl *tmr_nacl = NULL;
 	struct se_portal_group *tmr_tpg = NULL;
+	struct se_session *tmr_sess = NULL;
 	int tas;
         /*
 	 * TASK_ABORTED status bit, this is configurable via ConfigFS
@@ -395,8 +396,9 @@ int core_tmr_lun_reset(
 	 * or struct se_device passthrough..
 	 */
 	if (tmr && tmr->task_cmd && tmr->task_cmd->se_sess) {
-		tmr_nacl = tmr->task_cmd->se_sess->se_node_acl;
-		tmr_tpg = tmr->task_cmd->se_sess->se_tpg;
+		tmr_sess = tmr->task_cmd->se_sess;
+		tmr_nacl = tmr_sess->se_node_acl;
+		tmr_tpg = tmr_sess->se_tpg;
 		if (tmr_nacl && tmr_tpg) {
 			pr_debug("LUN_RESET: TMR caller fabric: %s"
 				" initiator port %s\n",
@@ -409,7 +411,7 @@ int core_tmr_lun_reset(
 		dev->transport->name, tas);
 
 	core_tmr_drain_tmr_list(dev, tmr, preempt_and_abort_list);
-	core_tmr_drain_state_list(dev, prout_cmd, tmr_nacl, tas,
+	core_tmr_drain_state_list(dev, prout_cmd, tmr_sess, tas,
 				preempt_and_abort_list);
 
 	/*
-- 
1.9.1

[PATCH-v4 4/5] target: Fix remote-port TMR ABORT + se_cmd fabric stop

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

To address the bug where fabric driver level shutdown
of se_cmd occurs at the same time when TMR CMD_T_ABORTED
is happening resulting in a -1 ->cmd_kref, this patch
adds a CMD_T_FABRIC_STOP bit that is used to determine
when TMR + driver I_T nexus shutdown is happening
concurrently.

It changes target_sess_cmd_list_set_waiting() to obtain
se_cmd->cmd_kref + set CMD_T_FABRIC_STOP, and drop local
reference in target_wait_for_sess_cmds() and invoke extra
target_put_sess_cmd() during Task Aborted Status (TAS)
when necessary.

Also, it adds a new target_wait_free_cmd() wrapper around
transport_wait_for_tasks() for the special case within
transport_generic_free_cmd() to set CMD_T_FABRIC_STOP,
and is now aware of CMD_T_ABORTED + CMD_T_TAS status
bits to know when an extra transport_put_cmd() during
TAS is required.

Note transport_generic_free_cmd() is expected to block on
cmd->cmd_wait_comp in order to follow what iscsi-target
expects during iscsi_conn context se_cmd shutdown.

Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Himanshu Madhani <himanshu.madhani@qlogic.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Andy Grover <agrover@redhat.com>
Cc: Mike Christie <mchristi@redhat.com>
Cc: stable@vger.kernel.org # 3.10+
Signed-off-by: Nicholas Bellinger <nab@daterainc.com>
---
 drivers/target/target_core_tmr.c       |  54 ++++++++----
 drivers/target/target_core_transport.c | 145 +++++++++++++++++++++++++--------
 include/target/target_core_base.h      |   2 +
 3 files changed, 150 insertions(+), 51 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 3e0d77a..82a663b 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -75,16 +75,18 @@ void core_tmr_release_req(struct se_tmr_req *tmr)
 	kfree(tmr);
 }
 
-static void core_tmr_handle_tas_abort(
-	struct se_session *tmr_sess,
-	struct se_cmd *cmd,
-	int tas)
+static void core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas)
 {
-	bool remove = true;
+	unsigned long flags;
+	bool remove = true, send_tas;
 	/*
 	 * TASK ABORTED status (TAS) bit support
 	 */
-	if (tmr_sess && tmr_sess != cmd->se_sess && tas) {
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	send_tas = (cmd->transport_state & CMD_T_TAS);
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
+	if (send_tas) {
 		remove = false;
 		transport_send_task_abort(cmd);
 	}
@@ -107,7 +109,8 @@ static int target_check_cdb_and_preempt(struct list_head *list,
 	return 1;
 }
 
-static bool __target_check_io_state(struct se_cmd *se_cmd)
+static bool __target_check_io_state(struct se_cmd *se_cmd,
+				    struct se_session *tmr_sess, int tas)
 {
 	struct se_session *sess = se_cmd->se_sess;
 
@@ -115,21 +118,32 @@ static bool __target_check_io_state(struct se_cmd *se_cmd)
 	WARN_ON_ONCE(!irqs_disabled());
 	/*
 	 * If command already reached CMD_T_COMPLETE state within
-	 * target_complete_cmd(), this se_cmd has been passed to
-	 * fabric driver and will not be aborted.
+	 * target_complete_cmd() or CMD_T_FABRIC_STOP due to shutdown,
+	 * this se_cmd has been passed to fabric driver and will
+	 * not be aborted.
 	 *
 	 * Otherwise, obtain a local se_cmd->cmd_kref now for TMR
 	 * ABORT_TASK + LUN_RESET for CMD_T_ABORTED processing as
 	 * long as se_cmd->cmd_kref is still active unless zero.
 	 */
 	spin_lock(&se_cmd->t_state_lock);
-	if (se_cmd->transport_state & CMD_T_COMPLETE) {
-		pr_debug("Attempted to abort io tag: %llu already complete,"
+	if (se_cmd->transport_state & (CMD_T_COMPLETE | CMD_T_FABRIC_STOP)) {
+		pr_debug("Attempted to abort io tag: %llu already complete or"
+			" fabric stop, skipping\n", se_cmd->tag);
+		spin_unlock(&se_cmd->t_state_lock);
+		return false;
+	}
+	if (sess->sess_tearing_down || se_cmd->cmd_wait_set) {
+		pr_debug("Attempted to abort io tag: %llu already shutdown,"
 			" skipping\n", se_cmd->tag);
 		spin_unlock(&se_cmd->t_state_lock);
 		return false;
 	}
 	se_cmd->transport_state |= CMD_T_ABORTED;
+
+	if ((tmr_sess != se_cmd->se_sess) && tas)
+		se_cmd->transport_state |= CMD_T_TAS;
+
 	spin_unlock(&se_cmd->t_state_lock);
 
 	return kref_get_unless_zero(&se_cmd->cmd_kref);
@@ -161,7 +175,7 @@ void core_tmr_abort_task(
 		printk("ABORT_TASK: Found referenced %s task_tag: %llu\n",
 			se_cmd->se_tfo->get_fabric_name(), ref_tag);
 
-		if (!__target_check_io_state(se_cmd)) {
+		if (!__target_check_io_state(se_cmd, se_sess, 0)) {
 			spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 			target_put_sess_cmd(se_cmd);
 			goto out;
@@ -230,7 +244,8 @@ static void core_tmr_drain_tmr_list(
 
 		spin_lock(&sess->sess_cmd_lock);
 		spin_lock(&cmd->t_state_lock);
-		if (!(cmd->transport_state & CMD_T_ACTIVE)) {
+		if (!(cmd->transport_state & CMD_T_ACTIVE) ||
+		     (cmd->transport_state & CMD_T_FABRIC_STOP)) {
 			spin_unlock(&cmd->t_state_lock);
 			spin_unlock(&sess->sess_cmd_lock);
 			continue;
@@ -240,15 +255,22 @@ static void core_tmr_drain_tmr_list(
 			spin_unlock(&sess->sess_cmd_lock);
 			continue;
 		}
+		if (sess->sess_tearing_down || cmd->cmd_wait_set) {
+			spin_unlock(&cmd->t_state_lock);
+			spin_unlock(&sess->sess_cmd_lock);
+			continue;
+		}
 		cmd->transport_state |= CMD_T_ABORTED;
 		spin_unlock(&cmd->t_state_lock);
 
 		rc = kref_get_unless_zero(&cmd->cmd_kref);
-		spin_unlock(&sess->sess_cmd_lock);
 		if (!rc) {
 			printk("LUN_RESET TMR: non-zero kref_get_unless_zero\n");
+			spin_unlock(&sess->sess_cmd_lock);
 			continue;
 		}
+		spin_unlock(&sess->sess_cmd_lock);
+
 		list_move_tail(&tmr_p->tmr_list, &drain_tmr_list);
 	}
 	spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
@@ -325,7 +347,7 @@ static void core_tmr_drain_state_list(
 			continue;
 
 		spin_lock(&sess->sess_cmd_lock);
-		rc = __target_check_io_state(cmd);
+		rc = __target_check_io_state(cmd, tmr_sess, tas);
 		spin_unlock(&sess->sess_cmd_lock);
 		if (!rc)
 			continue;
@@ -364,7 +386,7 @@ static void core_tmr_drain_state_list(
 		cancel_work_sync(&cmd->work);
 		transport_wait_for_tasks(cmd);
 
-		core_tmr_handle_tas_abort(tmr_sess, cmd, tas);
+		core_tmr_handle_tas_abort(cmd, tas);
 		target_put_sess_cmd(cmd);
 	}
 }
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 94e372a..3441b15 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2431,18 +2431,33 @@ static void transport_write_pending_qf(struct se_cmd *cmd)
 	}
 }
 
+static bool
+__transport_wait_for_tasks(struct se_cmd *, bool, bool *, bool *,
+			   unsigned long *flags);
+
+static void target_wait_free_cmd(struct se_cmd *cmd, bool *aborted, bool *tas)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	__transport_wait_for_tasks(cmd, true, aborted, tas, &flags);
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+}
+
 int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
 {
 	int ret = 0;
+	bool aborted = false, tas = false;
 
 	if (!(cmd->se_cmd_flags & SCF_SE_LUN_CMD)) {
 		if (wait_for_tasks && (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB))
-			transport_wait_for_tasks(cmd);
+			target_wait_free_cmd(cmd, &aborted, &tas);
 
-		ret = transport_put_cmd(cmd);
+		if (!aborted || tas)
+			ret = transport_put_cmd(cmd);
 	} else {
 		if (wait_for_tasks)
-			transport_wait_for_tasks(cmd);
+			target_wait_free_cmd(cmd, &aborted, &tas);
 		/*
 		 * Handle WRITE failure case where transport_generic_new_cmd()
 		 * has already added se_cmd to state_list, but fabric has
@@ -2454,7 +2469,20 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
 		if (cmd->se_lun)
 			transport_lun_remove_cmd(cmd);
 
-		ret = transport_put_cmd(cmd);
+		if (!aborted || tas)
+			ret = transport_put_cmd(cmd);
+	}
+	/*
+	 * If the task has been internally aborted due to TMR ABORT_TASK
+	 * or LUN_RESET, target_core_tmr.c is responsible for performing
+	 * the remaining calls to target_put_sess_cmd(), and not the
+	 * callers of this function.
+	 */
+	if (aborted) {
+		pr_debug("Detected CMD_T_ABORTED for ITT: %llu\n", cmd->tag);
+		wait_for_completion(&cmd->cmd_wait_comp);
+		cmd->se_tfo->release_cmd(cmd);
+		ret = 1;
 	}
 	return ret;
 }
@@ -2509,6 +2537,7 @@ static void target_release_cmd_kref(struct kref *kref)
 	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
 	struct se_session *se_sess = se_cmd->se_sess;
 	unsigned long flags;
+	bool fabric_stop;
 
 	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
 	if (list_empty(&se_cmd->se_cmd_list)) {
@@ -2517,13 +2546,19 @@ static void target_release_cmd_kref(struct kref *kref)
 		se_cmd->se_tfo->release_cmd(se_cmd);
 		return;
 	}
-	if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) {
+
+	spin_lock(&se_cmd->t_state_lock);
+	fabric_stop = (se_cmd->transport_state & CMD_T_FABRIC_STOP);
+	spin_unlock(&se_cmd->t_state_lock);
+
+	if (se_cmd->cmd_wait_set || fabric_stop) {
+		list_del_init(&se_cmd->se_cmd_list);
 		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 		target_free_cmd_mem(se_cmd);
 		complete(&se_cmd->cmd_wait_comp);
 		return;
 	}
-	list_del(&se_cmd->se_cmd_list);
+	list_del_init(&se_cmd->se_cmd_list);
 	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 
 	target_free_cmd_mem(se_cmd);
@@ -2555,6 +2590,7 @@ void target_sess_cmd_list_set_waiting(struct se_session *se_sess)
 {
 	struct se_cmd *se_cmd;
 	unsigned long flags;
+	int rc;
 
 	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
 	if (se_sess->sess_tearing_down) {
@@ -2564,8 +2600,15 @@ void target_sess_cmd_list_set_waiting(struct se_session *se_sess)
 	se_sess->sess_tearing_down = 1;
 	list_splice_init(&se_sess->sess_cmd_list, &se_sess->sess_wait_list);
 
-	list_for_each_entry(se_cmd, &se_sess->sess_wait_list, se_cmd_list)
-		se_cmd->cmd_wait_set = 1;
+	list_for_each_entry(se_cmd, &se_sess->sess_wait_list, se_cmd_list) {
+		rc = kref_get_unless_zero(&se_cmd->cmd_kref);
+		if (rc) {
+			se_cmd->cmd_wait_set = 1;
+			spin_lock(&se_cmd->t_state_lock);
+			se_cmd->transport_state |= CMD_T_FABRIC_STOP;
+			spin_unlock(&se_cmd->t_state_lock);
+		}
+	}
 
 	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 }
@@ -2578,15 +2621,25 @@ void target_wait_for_sess_cmds(struct se_session *se_sess)
 {
 	struct se_cmd *se_cmd, *tmp_cmd;
 	unsigned long flags;
+	bool tas;
 
 	list_for_each_entry_safe(se_cmd, tmp_cmd,
 				&se_sess->sess_wait_list, se_cmd_list) {
-		list_del(&se_cmd->se_cmd_list);
+		list_del_init(&se_cmd->se_cmd_list);
 
 		pr_debug("Waiting for se_cmd: %p t_state: %d, fabric state:"
 			" %d\n", se_cmd, se_cmd->t_state,
 			se_cmd->se_tfo->get_cmd_state(se_cmd));
 
+		spin_lock_irqsave(&se_cmd->t_state_lock, flags);
+		tas = (se_cmd->transport_state & CMD_T_TAS);
+		spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
+
+		if (!target_put_sess_cmd(se_cmd)) {
+			if (tas)
+				target_put_sess_cmd(se_cmd);
+		}
+
 		wait_for_completion(&se_cmd->cmd_wait_comp);
 		pr_debug("After cmd_wait_comp: se_cmd: %p t_state: %d"
 			" fabric state: %d\n", se_cmd, se_cmd->t_state,
@@ -2608,53 +2661,75 @@ void transport_clear_lun_ref(struct se_lun *lun)
 	wait_for_completion(&lun->lun_ref_comp);
 }
 
-/**
- * transport_wait_for_tasks - wait for completion to occur
- * @cmd:	command to wait
- *
- * Called from frontend fabric context to wait for storage engine
- * to pause and/or release frontend generated struct se_cmd.
- */
-bool transport_wait_for_tasks(struct se_cmd *cmd)
+static bool
+__transport_wait_for_tasks(struct se_cmd *cmd, bool fabric_stop,
+			   bool *aborted, bool *tas, unsigned long *flags)
+	__releases(&cmd->t_state_lock)
+	__acquires(&cmd->t_state_lock)
 {
-	unsigned long flags;
 
-	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	assert_spin_locked(&cmd->t_state_lock);
+	WARN_ON_ONCE(!irqs_disabled());
+
+	if (fabric_stop)
+		cmd->transport_state |= CMD_T_FABRIC_STOP;
+
+	if (cmd->transport_state & CMD_T_ABORTED)
+		*aborted = true;
+
+	if (cmd->transport_state & CMD_T_TAS)
+		*tas = true;
+
 	if (!(cmd->se_cmd_flags & SCF_SE_LUN_CMD) &&
-	    !(cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)) {
-		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+	    !(cmd->se_cmd_flags & SCF_SCSI_TMR_CDB))
 		return false;
-	}
 
 	if (!(cmd->se_cmd_flags & SCF_SUPPORTED_SAM_OPCODE) &&
-	    !(cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)) {
-		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+	    !(cmd->se_cmd_flags & SCF_SCSI_TMR_CDB))
 		return false;
-	}
 
-	if (!(cmd->transport_state & CMD_T_ACTIVE)) {
-		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+	if (!(cmd->transport_state & CMD_T_ACTIVE))
+		return false;
+
+	if (fabric_stop && *aborted)
 		return false;
-	}
 
 	cmd->transport_state |= CMD_T_STOP;
 
-	pr_debug("wait_for_tasks: Stopping %p ITT: 0x%08llx i_state: %d, t_state: %d, CMD_T_STOP\n",
-		cmd, cmd->tag, cmd->se_tfo->get_cmd_state(cmd), cmd->t_state);
+	pr_debug("wait_for_tasks: Stopping %p ITT: 0x%08llx i_state: %d,"
+		 " t_state: %d, CMD_T_STOP\n", cmd, cmd->tag,
+		 cmd->se_tfo->get_cmd_state(cmd), cmd->t_state);
 
-	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+	spin_unlock_irqrestore(&cmd->t_state_lock, *flags);
 
 	wait_for_completion(&cmd->t_transport_stop_comp);
 
-	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	spin_lock_irqsave(&cmd->t_state_lock, *flags);
 	cmd->transport_state &= ~(CMD_T_ACTIVE | CMD_T_STOP);
 
-	pr_debug("wait_for_tasks: Stopped wait_for_completion(&cmd->t_transport_stop_comp) for ITT: 0x%08llx\n",
-		cmd->tag);
+	pr_debug("wait_for_tasks: Stopped wait_for_completion(&cmd->"
+		 "t_transport_stop_comp) for ITT: 0x%08llx\n", cmd->tag);
 
+	return true;
+}
+
+/**
+ * transport_wait_for_tasks - wait for completion to occur
+ * @cmd:	command to wait
+ *
+ * Called from frontend fabric context to wait for storage engine
+ * to pause and/or release frontend generated struct se_cmd.
+ */
+bool transport_wait_for_tasks(struct se_cmd *cmd)
+{
+	unsigned long flags;
+	bool ret, aborted = false, tas = false;
+
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	ret = __transport_wait_for_tasks(cmd, false, &aborted, &tas, &flags);
 	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 
-	return true;
+	return ret;
 }
 EXPORT_SYMBOL(transport_wait_for_tasks);
 
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 1a76726..1579539e 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -493,6 +493,8 @@ struct se_cmd {
 #define CMD_T_DEV_ACTIVE	(1 << 7)
 #define CMD_T_REQUEST_STOP	(1 << 8)
 #define CMD_T_BUSY		(1 << 9)
+#define CMD_T_TAS		(1 << 10)
+#define CMD_T_FABRIC_STOP	(1 << 11)
 	spinlock_t		t_state_lock;
 	struct kref		cmd_kref;
 	struct completion	t_transport_stop_comp;
-- 
1.9.1

[PATCH-v4 5/5] target: Fix race with SCF_SEND_DELAYED_TAS handling

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch fixes a race between setting of SCF_SEND_DELAYED_TAS
in transport_send_task_abort(), and check of the same bit in
transport_check_aborted_status().

It adds a __transport_check_aborted_status() version that is
used by target_execute_cmd() when se_cmd->t_state_lock is
held, and a transport_check_aborted_status() wrapper for
all other existing callers.

Also, it handles the case where the check happens before
transport_send_task_abort() gets called.  For this, go
ahead and set SCF_SEND_DELAYED_TAS early when necessary,
and have transport_send_task_abort() send the abort.

Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Himanshu Madhani <himanshu.madhani@qlogic.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Andy Grover <agrover@redhat.com>
Cc: Mike Christie <mchristi@redhat.com>
Cc: stable@vger.kernel.org # 3.10+
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c | 53 ++++++++++++++++++++++++++--------
 1 file changed, 41 insertions(+), 12 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 3441b15..2e0b23a 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1858,19 +1858,21 @@ static bool target_handle_task_attr(struct se_cmd *cmd)
 	return true;
 }
 
+static int __transport_check_aborted_status(struct se_cmd *, int);
+
 void target_execute_cmd(struct se_cmd *cmd)
 {
 	/*
-	 * If the received CDB has aleady been aborted stop processing it here.
-	 */
-	if (transport_check_aborted_status(cmd, 1))
-		return;
-
-	/*
 	 * Determine if frontend context caller is requesting the stopping of
 	 * this command for frontend exceptions.
+	 *
+	 * If the received CDB has aleady been aborted stop processing it here.
 	 */
 	spin_lock_irq(&cmd->t_state_lock);
+	if (__transport_check_aborted_status(cmd, 1)) {
+		spin_unlock_irq(&cmd->t_state_lock);
+		return;
+	}
 	if (cmd->transport_state & CMD_T_STOP) {
 		pr_debug("%s:%d CMD_T_STOP for ITT: 0x%08llx\n",
 			__func__, __LINE__, cmd->tag);
@@ -2911,28 +2913,49 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd,
 }
 EXPORT_SYMBOL(transport_send_check_condition_and_sense);
 
-int transport_check_aborted_status(struct se_cmd *cmd, int send_status)
+static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
+	__releases(&cmd->t_state_lock)
+	__acquires(&cmd->t_state_lock)
 {
+	assert_spin_locked(&cmd->t_state_lock);
+	WARN_ON_ONCE(!irqs_disabled());
+
 	if (!(cmd->transport_state & CMD_T_ABORTED))
 		return 0;
-
 	/*
 	 * If cmd has been aborted but either no status is to be sent or it has
 	 * already been sent, just return
 	 */
-	if (!send_status || !(cmd->se_cmd_flags & SCF_SEND_DELAYED_TAS))
+	if (!send_status || !(cmd->se_cmd_flags & SCF_SEND_DELAYED_TAS)) {
+		if (send_status)
+			cmd->se_cmd_flags |= SCF_SEND_DELAYED_TAS;
 		return 1;
+	}
 
-	pr_debug("Sending delayed SAM_STAT_TASK_ABORTED status for CDB: 0x%02x ITT: 0x%08llx\n",
-		 cmd->t_task_cdb[0], cmd->tag);
+	pr_debug("Sending delayed SAM_STAT_TASK_ABORTED status for CDB:"
+		" 0x%02x ITT: 0x%08llx\n", cmd->t_task_cdb[0], cmd->tag);
 
 	cmd->se_cmd_flags &= ~SCF_SEND_DELAYED_TAS;
 	cmd->scsi_status = SAM_STAT_TASK_ABORTED;
 	trace_target_cmd_complete(cmd);
+
+	spin_unlock_irq(&cmd->t_state_lock);
 	cmd->se_tfo->queue_status(cmd);
+	spin_lock_irq(&cmd->t_state_lock);
 
 	return 1;
 }
+
+int transport_check_aborted_status(struct se_cmd *cmd, int send_status)
+{
+	int ret;
+
+	spin_lock_irq(&cmd->t_state_lock);
+	ret = __transport_check_aborted_status(cmd, send_status);
+	spin_unlock_irq(&cmd->t_state_lock);
+
+	return ret;
+}
 EXPORT_SYMBOL(transport_check_aborted_status);
 
 void transport_send_task_abort(struct se_cmd *cmd)
@@ -2954,11 +2977,17 @@ void transport_send_task_abort(struct se_cmd *cmd)
 	 */
 	if (cmd->data_direction == DMA_TO_DEVICE) {
 		if (cmd->se_tfo->write_pending_status(cmd) != 0) {
-			cmd->transport_state |= CMD_T_ABORTED;
+			spin_lock_irqsave(&cmd->t_state_lock, flags);
+			if (cmd->se_cmd_flags & SCF_SEND_DELAYED_TAS) {
+				spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+				goto send_abort;
+			}
 			cmd->se_cmd_flags |= SCF_SEND_DELAYED_TAS;
+			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 			return;
 		}
 	}
+send_abort:
 	cmd->scsi_status = SAM_STAT_TASK_ABORTED;
 
 	transport_lun_remove_cmd(cmd);
-- 
1.9.1

Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Bart Van Assche]

On 02/06/16 19:17, Nicholas A. Bellinger wrote:
> Here is -v4 series to address the set of of LUN_RESET
> active I/O + TMR se_cmd->cmd_kref < 0 bugs as reported
> recently by Quinn & Co.  This can occur during active
> I/O remote port TMR LUN_RESET with multi-port LIO
> configurations.

Hi Nic,

If I understood the purpose of this patch series correctly then this 
patch series is a brave attempt to fix what is also fixed by my patch 
called "target: Make ABORT and LUN RESET handling synchronous". Wouldn't 
it be better to focus on that patch instead of trying to fix the current 
approach in which TMR handling happens from the another context than the 
command processing context ?

Thanks,

Bart.

Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Nicholas A. Bellinger]

On Sat, 2016-02-06 at 20:19 -0800, Bart Van Assche wrote:
> On 02/06/16 19:17, Nicholas A. Bellinger wrote:
> > Here is -v4 series to address the set of of LUN_RESET
> > active I/O + TMR se_cmd->cmd_kref < 0 bugs as reported
> > recently by Quinn & Co.  This can occur during active
> > I/O remote port TMR LUN_RESET with multi-port LIO
> > configurations.
> 
> Hi Nic,
> 
> If I understood the purpose of this patch series correctly then this 
> patch series is a brave attempt to fix what is also fixed by my patch 
> called "target: Make ABORT and LUN RESET handling synchronous". Wouldn't 
> it be better to focus on that patch instead of trying to fix the current 
> approach in which TMR handling happens from the another context than the 
> command processing context ?
> 

This statement is a gross oversimplification of the issues involved.

If you'll recall, this was already highlighted in the context of your
patch here:

http://www.spinics.net/lists/target-devel/msg11057.html

There are a number of comments on why the bug-fix was incorrect and
broken, the basics of what needed to be done and in what order it should
happen.

But instead of replying to the comments, this was your response:

http://www.spinics.net/lists/target-devel/msg11542.html

If you are authentically interested in understanding the issues
involved, you'll probably need to go back and comment on those topics
individually, instead of ignoring them.

Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Bart Van Assche]

On 02/06/16 21:19, Nicholas A. Bellinger wrote:
> On Sat, 2016-02-06 at 20:19 -0800, Bart Van Assche wrote:
>> On 02/06/16 19:17, Nicholas A. Bellinger wrote:
>>> Here is -v4 series to address the set of of LUN_RESET
>>> active I/O + TMR se_cmd->cmd_kref < 0 bugs as reported
>>> recently by Quinn & Co.  This can occur during active
>>> I/O remote port TMR LUN_RESET with multi-port LIO
>>> configurations.
>>
>> Hi Nic,
>>
>> If I understood the purpose of this patch series correctly then this
>> patch series is a brave attempt to fix what is also fixed by my patch
>> called "target: Make ABORT and LUN RESET handling synchronous". Wouldn't
>> it be better to focus on that patch instead of trying to fix the current
>> approach in which TMR handling happens from the another context than the
>> command processing context ?
>>
>
> This statement is a gross oversimplification of the issues involved.
>
> If you'll recall, this was already highlighted in the context of your
> patch here:
>
> http://www.spinics.net/lists/target-devel/msg11057.html
>
> There are a number of comments on why the bug-fix was incorrect and
> broken, the basics of what needed to be done and in what order it should
> happen.
>
> But instead of replying to the comments, this was your response:
>
> http://www.spinics.net/lists/target-devel/msg11542.html
>
> If you are authentically interested in understanding the issues
> involved, you'll probably need to go back and comment on those topics
> individually, instead of ignoring them.

Hi Nic,

What you write is not correct. All your review comments that made sense 
have been addressed in the latest version of my patch 
(http://www.spinics.net/lists/target-devel/msg11666.html).

Additionally, you haven't answered my question. My question was: why to 
spend more energy on trying to fix the current approach if the LIO TMR 
handling code can be simplified greatly by handling ABORT and LUN RESET 
from the regular command execution path ?

Bart.

Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Nicholas A. Bellinger]

On Sun, 2016-02-07 at 08:02 -0800, Bart Van Assche wrote:
> On 02/06/16 21:19, Nicholas A. Bellinger wrote:
> > On Sat, 2016-02-06 at 20:19 -0800, Bart Van Assche wrote:
> >> On 02/06/16 19:17, Nicholas A. Bellinger wrote:
> >>> Here is -v4 series to address the set of of LUN_RESET
> >>> active I/O + TMR se_cmd->cmd_kref < 0 bugs as reported
> >>> recently by Quinn & Co.  This can occur during active
> >>> I/O remote port TMR LUN_RESET with multi-port LIO
> >>> configurations.
> >>
> >> Hi Nic,
> >>
> >> If I understood the purpose of this patch series correctly then this
> >> patch series is a brave attempt to fix what is also fixed by my patch
> >> called "target: Make ABORT and LUN RESET handling synchronous". Wouldn't
> >> it be better to focus on that patch instead of trying to fix the current
> >> approach in which TMR handling happens from the another context than the
> >> command processing context ?
> >>
> >
> > This statement is a gross oversimplification of the issues involved.
> >
> > If you'll recall, this was already highlighted in the context of your
> > patch here:
> >
> > http://www.spinics.net/lists/target-devel/msg11057.html
> >
> > There are a number of comments on why the bug-fix was incorrect and
> > broken, the basics of what needed to be done and in what order it should
> > happen.
> >
> > But instead of replying to the comments, this was your response:
> >
> > http://www.spinics.net/lists/target-devel/msg11542.html
> >
> > If you are authentically interested in understanding the issues
> > involved, you'll probably need to go back and comment on those topics
> > individually, instead of ignoring them.
> 
> Hi Nic,
> 
> What you write is not correct. All your review comments that made sense 
> have been addressed in the latest version of my patch 
> (http://www.spinics.net/lists/target-devel/msg11666.html).

Did you respond to the specific feedback of my email..?  No.

Did you include a change-log in the subsequent patch explaining the
changes..?  No.

If you're still not willing or able to have a technical discussion on
the specific issues involved or give feedback inline to the patch series
itself, then you are just trying to waste everyone's time.

> 
> Additionally, you haven't answered my question. My question was: why to 
> spend more energy on trying to fix the current approach if the LIO TMR 
> handling code can be simplified greatly by handling ABORT and LUN RESET 
> from the regular command execution path ?
> 

Because your patch was incorrect and broken, and you still don't seem
interested in taking the time to actually understand why that is.

Listen, Bart, I'm getting tired of your inability to have a technical
discussion of the issues.

So that said, I'm going to put down some ground rules for our future
interactions.  I expect you to:

   - Ask questions when you're unsure of a specific piece of code, 
     before attempting to push changes to re-write significant pieces 
     and spend community review cycles. 
   - Comment inline to all feedback for changes of substance.
   - Stop ignoring subsystem maintainer feedback.
   - Provide a change-log between patches for all changes of substance.

If you are genuinely interested in contributing to LIO, then these
should be a no-brainer. 

If you aren't genuinely interested in contributing to LIO, then keep
doing what you're doing.

Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Himanshu Madhani]

[-- Attachment #1: Type: text/plain, Size: 2854 bytes --]

Hi Nic, 


On 2/6/16, 7:17 PM, "Nicholas A. Bellinger" <nab@daterainc.com> wrote:

>From: Nicholas Bellinger <nab@linux-iscsi.org>
>
>Hi folks,
>
>Here is -v4 series to address the set of of LUN_RESET
>active I/O + TMR se_cmd->cmd_kref < 0 bugs as reported
>recently by Quinn & Co.  This can occur during active
>I/O remote port TMR LUN_RESET with multi-port LIO
>configurations.
>
>To address this bug, we add a __target_check_io_state()
>common handler for ABORT_TASK + LUN_RESET I/O abort
>cases, and move the remaining se_cmd SGL page + release
>into target_free_cmd_mem() to now be called directly
>from final target_release_cmd_kref() callback.
>
>It also adds a target_wait_free_cmd() helper and makes
>transport_generic_free_cmd() aware of CMD_T_ABORTED
>status during concurrent session disconnects, and
>introduces CMD_T_FABRIC_STOP bit to signal this special
>case.
>
>Currently this series is running atop v4.5-rc1 + v3.14.y,
>and with iscsi-target ports is able to survive active
>I/O remote-port LUN resets, plus remote-port LUN_RESET
>with concurrent simulated session disconnects.
>
>At this point the changes are stable with iscsi-target
>ports, and as Himanshu + Co can verify with tcm_qla2xxx
>should be considered ready to merge for -rc4.
>
>Please review + test.
>
>--nab
>
>v4 changes:
>
>- Add explicit CMD_T_FABRIC_STOP check and drop cmd_wait_set
>  bit set usage in __target_check_io_state().
>- Set early CMD_T_TAS in __target_check_io_state to avoid
>  potential race in transport_send_task_abort() with shutdown.
>- Add fabric_stop + aborted checks in __transport_wait_for_tasks()
>  in order to let TMR CMD_T_ABORTED se_cmd shutdown complete
>  during concurrent session disconnect.
>- Fix race with driver SCF_SEND_DELAYED_TAS handling when
>  __transport_check_aborted_status() could happen before
>  transport_send_task_abort() in TMR kthread context.
>
>Nicholas Bellinger (5):
>  target: Fix LUN_RESET active I/O handling for ACK_KREF
>  target: Fix LUN_RESET active TMR descriptor handling
>  target: Fix TAS handling for multi-session se_node_acls
>  target: Fix remote-port TMR ABORT + se_cmd fabric stop
>  target: Fix race with SCF_SEND_DELAYED_TAS handling
>
> drivers/target/target_core_tmr.c       | 139 ++++++++++++-----
> drivers/target/target_core_transport.c | 278
>+++++++++++++++++++++++----------
> include/target/target_core_base.h      |   3 +
> 3 files changed, 301 insertions(+), 119 deletions(-)
>
>-- 
>1.9.1
>

I am testing this series with with 4.5.0-rc2+ kernel and I am seeing issue
where trying to trigger
sg_reset with option of host/device/bus in loop at 120second interval
causes call stack. At this point
removing configuration hangs indefinitely. See attached dmesg output from
my setup. 

Thanks,
Himanshu


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 152414 bytes --]

Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Nicholas A. Bellinger]

Hi Himanshu,

On Mon, 2016-02-08 at 23:27 +0000, Himanshu Madhani wrote:
> 
> I am testing this series with with 4.5.0-rc2+ kernel and I am seeing issue
> where trying to trigger
> sg_reset with option of host/device/bus in loop at 120second interval
> causes call stack. At this point
> removing configuration hangs indefinitely. See attached dmesg output from
> my setup. 
> 

Thanks alot for testing this.

So It looks like we're still hitting a indefinite schedule() on
se_cmd->cmd_wait_comp once tcm_qla2xxx session disconnect/reconnect
occurs, after repeated explicit active I/O remote-port sg_resets.

Does this trigger on the first tcm_qla2xxx session reconnect after
explicit remote-port sg_reset..?  Are session reconnects actively being
triggered during the test..?

To verify the latter for iscsi-target, I've been using a small patch to
trigger session reset from TMR kthread context in order to simulate the
I_T disconnects.  Something like that would be useful for verifying with
tcm_qla2xxx too.

That said, I'll be reproducing with tcm_qla2xxx ports this week, and
will enable various debug in a WIP branch for testing.

Thank you,

--nab

Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Himanshu Madhani]

Hi Nic, 


On 2/8/16, 9:25 PM, "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:

>Hi Himanshu,
>
>On Mon, 2016-02-08 at 23:27 +0000, Himanshu Madhani wrote:
>> 
>> I am testing this series with with 4.5.0-rc2+ kernel and I am seeing
>>issue
>> where trying to trigger
>> sg_reset with option of host/device/bus in loop at 120second interval
>> causes call stack. At this point
>> removing configuration hangs indefinitely. See attached dmesg output
>>from
>> my setup. 
>> 
>
>Thanks alot for testing this.
>
>So It looks like we're still hitting a indefinite schedule() on
>se_cmd->cmd_wait_comp once tcm_qla2xxx session disconnect/reconnect
>occurs, after repeated explicit active I/O remote-port sg_resets.
>
>Does this trigger on the first tcm_qla2xxx session reconnect after
>explicit remote-port sg_reset..?  Are session reconnects actively being
>triggered during the test..?
>
>To verify the latter for iscsi-target, I've been using a small patch to
>trigger session reset from TMR kthread context in order to simulate the
>I_T disconnects.  Something like that would be useful for verifying with
>tcm_qla2xxx too.
>
>That said, I'll be reproducing with tcm_qla2xxx ports this week, and
>will enable various debug in a WIP branch for testing.

Let me know if I can help in any way for testing/validating this series.

>
>Thank you,
>
>--nab
>

Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Nicholas A. Bellinger]

Hi Himanshu & Co,

On Tue, 2016-02-09 at 18:03 +0000, Himanshu Madhani wrote:
> Hi Nic, 
> 
> 
> On 2/8/16, 9:25 PM, "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:
> 
> >Hi Himanshu,
> >
> >On Mon, 2016-02-08 at 23:27 +0000, Himanshu Madhani wrote:
> >> 
> >> I am testing this series with with 4.5.0-rc2+ kernel and I am seeing
> >>issue
> >> where trying to trigger
> >> sg_reset with option of host/device/bus in loop at 120second interval
> >> causes call stack. At this point
> >> removing configuration hangs indefinitely. See attached dmesg output
> >>from
> >> my setup. 
> >> 
> >
> >Thanks alot for testing this.
> >
> >So It looks like we're still hitting a indefinite schedule() on
> >se_cmd->cmd_wait_comp once tcm_qla2xxx session disconnect/reconnect
> >occurs, after repeated explicit active I/O remote-port sg_resets.
> >
> >Does this trigger on the first tcm_qla2xxx session reconnect after
> >explicit remote-port sg_reset..?  Are session reconnects actively being
> >triggered during the test..?
> >
> >To verify the latter for iscsi-target, I've been using a small patch to
> >trigger session reset from TMR kthread context in order to simulate the
> >I_T disconnects.  Something like that would be useful for verifying with
> >tcm_qla2xxx too.
> >
> >That said, I'll be reproducing with tcm_qla2xxx ports this week, and
> >will enable various debug in a WIP branch for testing.

Following up here..

So far using my test setup with ISP2532 ports in P2P + RAMDISK_MCP and
v4.5-rc1, repeated remote-port active I/O LUN_RESET (sg_reset -d) has
been functioning as expected with a blocksize_range=4k-256k + iodepth=32
fio write-verify style workload.

No ->cmd_kref -1 OOPsen or qla2xxx initiator generated ABORT_TASKs from
outstanding target TAS responses, nor fio write-verify failures to
report after 800x remote-port active I/O LUN_RESETS.

Next step will be to verify explicit tcm_qla2xxx port + module shutdown
after 1K test iterations, and then IBLOCK async completions <-> NVMe
backends with the same case.

> Let me know if I can help in any way for testing/validating this series.
> 

Thanks.  :)

So based on your original log, it's still unclear clear if the session
reset resulting in se_cmd->cmd_wait_comp indefinite sleep + ->cmd_kref
leak is happen concurrently with repeated remote port LUN_RESET, or the
session reset -> target_wait_for_sess_cmds() occurs after active I/O has
already completed..?  Please confirm.

To that end, target-pending/debug-for-himanshu has been pushed to enable
extra debug for test, please update.

Also, you'll want to enable microsecond ring buffer timestamps in your
kernel build too, as it's very useful for type this debugging.

Thank you,

--nab

Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Nicholas A. Bellinger]

On Wed, 2016-02-10 at 22:53 -0800, Nicholas A. Bellinger wrote:
> On Tue, 2016-02-09 at 18:03 +0000, Himanshu Madhani wrote:
> > On 2/8/16, 9:25 PM, "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:
> > >On Mon, 2016-02-08 at 23:27 +0000, Himanshu Madhani wrote:
> > >> 
> > >> I am testing this series with with 4.5.0-rc2+ kernel and I am seeing
> > >>issue
> > >> where trying to trigger
> > >> sg_reset with option of host/device/bus in loop at 120second interval
> > >> causes call stack. At this point
> > >> removing configuration hangs indefinitely. See attached dmesg output
> > >>from
> > >> my setup. 
> > >> 
> > >
> > >Thanks alot for testing this.
> > >
> > >So It looks like we're still hitting a indefinite schedule() on
> > >se_cmd->cmd_wait_comp once tcm_qla2xxx session disconnect/reconnect
> > >occurs, after repeated explicit active I/O remote-port sg_resets.
> > >
> > >Does this trigger on the first tcm_qla2xxx session reconnect after
> > >explicit remote-port sg_reset..?  Are session reconnects actively being
> > >triggered during the test..?
> > >
> > >To verify the latter for iscsi-target, I've been using a small patch to
> > >trigger session reset from TMR kthread context in order to simulate the
> > >I_T disconnects.  Something like that would be useful for verifying with
> > >tcm_qla2xxx too.
> > >
> > >That said, I'll be reproducing with tcm_qla2xxx ports this week, and
> > >will enable various debug in a WIP branch for testing.
> 
> Following up here..
> 
> So far using my test setup with ISP2532 ports in P2P + RAMDISK_MCP and
> v4.5-rc1, repeated remote-port active I/O LUN_RESET (sg_reset -d) has
> been functioning as expected with a blocksize_range=4k-256k + iodepth=32
> fio write-verify style workload.
> 
> No ->cmd_kref -1 OOPsen or qla2xxx initiator generated ABORT_TASKs from
> outstanding target TAS responses, nor fio write-verify failures to
> report after 800x remote-port active I/O LUN_RESETS.
> 
> Next step will be to verify explicit tcm_qla2xxx port + module shutdown
> after 1K test iterations, and then IBLOCK async completions <-> NVMe
> backends with the same case.
> 

After letting this test run over-night up to 7k active I/O remote-port
LUN_RESETs, things are still functioning as expected.

Also, /etc/init.d/target stop was able to successfully shutdown all
active sessions and unload tcm_qla2xxx after the test run.

So AFAICT, the active I/O remote-port LUN_RESET changes are stable with
tcm_qla2xxx ports, separate from concurrent session disconnect hung task
you reported earlier.

That said, I'll likely push this series as-is for -rc4, given that Dan
has also been able to verify the non conncurrent session disconnect case
on his setup generating constant ABORT_TASKs, and it's still surviving
both cases for iscsi-target ports.

Please give the debug patch from last night a shot, and see if we can
determine the se_cmd states when you hit the hung task.

Thank you,

-nab

Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Himanshu Madhani]

[-- Attachment #1: Type: text/plain, Size: 4164 bytes --]

Hi Nic, 



On 2/11/16, 3:47 PM, "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:

>On Wed, 2016-02-10 at 22:53 -0800, Nicholas A. Bellinger wrote:
>> On Tue, 2016-02-09 at 18:03 +0000, Himanshu Madhani wrote:
>> > On 2/8/16, 9:25 PM, "Nicholas A. Bellinger" <nab@linux-iscsi.org>
>>wrote:
>> > >On Mon, 2016-02-08 at 23:27 +0000, Himanshu Madhani wrote:
>> > >> 
>> > >> I am testing this series with with 4.5.0-rc2+ kernel and I am
>>seeing
>> > >>issue
>> > >> where trying to trigger
>> > >> sg_reset with option of host/device/bus in loop at 120second
>>interval
>> > >> causes call stack. At this point
>> > >> removing configuration hangs indefinitely. See attached dmesg
>>output
>> > >>from
>> > >> my setup. 
>> > >> 
>> > >
>> > >Thanks alot for testing this.
>> > >
>> > >So It looks like we're still hitting a indefinite schedule() on
>> > >se_cmd->cmd_wait_comp once tcm_qla2xxx session disconnect/reconnect
>> > >occurs, after repeated explicit active I/O remote-port sg_resets.
>> > >
>> > >Does this trigger on the first tcm_qla2xxx session reconnect after
>> > >explicit remote-port sg_reset..?  Are session reconnects actively
>>being
>> > >triggered during the test..?
>> > >
>> > >To verify the latter for iscsi-target, I've been using a small patch
>>to
>> > >trigger session reset from TMR kthread context in order to simulate
>>the
>> > >I_T disconnects.  Something like that would be useful for verifying
>>with
>> > >tcm_qla2xxx too.
>> > >
>> > >That said, I'll be reproducing with tcm_qla2xxx ports this week, and
>> > >will enable various debug in a WIP branch for testing.
>> 
>> Following up here..
>> 
>> So far using my test setup with ISP2532 ports in P2P + RAMDISK_MCP and
>> v4.5-rc1, repeated remote-port active I/O LUN_RESET (sg_reset -d) has
>> been functioning as expected with a blocksize_range=4k-256k + iodepth=32
>> fio write-verify style workload.
>> 
>> No ->cmd_kref -1 OOPsen or qla2xxx initiator generated ABORT_TASKs from
>> outstanding target TAS responses, nor fio write-verify failures to
>> report after 800x remote-port active I/O LUN_RESETS.
>> 
>> Next step will be to verify explicit tcm_qla2xxx port + module shutdown
>> after 1K test iterations, and then IBLOCK async completions <-> NVMe
>> backends with the same case.
>> 
>
>After letting this test run over-night up to 7k active I/O remote-port
>LUN_RESETs, things are still functioning as expected.
>
>Also, /etc/init.d/target stop was able to successfully shutdown all
>active sessions and unload tcm_qla2xxx after the test run.
>
>So AFAICT, the active I/O remote-port LUN_RESET changes are stable with
>tcm_qla2xxx ports, separate from concurrent session disconnect hung task
>you reported earlier.
>
>That said, I'll likely push this series as-is for -rc4, given that Dan
>has also been able to verify the non conncurrent session disconnect case
>on his setup generating constant ABORT_TASKs, and it's still surviving
>both cases for iscsi-target ports.
>
>Please give the debug patch from last night a shot, and see if we can
>determine the se_cmd states when you hit the hung task.

I¹ll give your latest debug patch try in a little while

>From the testing that I have done, what is seen is that active IO has
already been completed and qla2xxx driver is waiting for commands to be
Completed and it¹s waiting indefinitely for cmd_wait_comp.
So it looks like there is a missing complete call from target_core. I¹ve
attached our analysis from crash debug on a live system after the issues
happens.


I can recreate this issue at will within 5 minute of triggering sg_reset
with following steps

1. Export 4 RAM disk LUNs on each of 2 port adapter. Initiator will see 8
RAM disk targets
2. Start IO with 4K block size and 8 threads with 80% write 20% read and
100% dandom. 
(I am using vdbench for generating IO. I can provide setup/config script
if needed)
3. Start sg_reset for each LUNs with first device, bus and host with 120s
delay. (I¹ve attached
My script that I am using for triggering sg_reset)

>
>Thank you,
>
>-nab
>


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 21353 bytes --]

Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Nicholas A. Bellinger]

On Fri, 2016-02-12 at 05:30 +0000, Himanshu Madhani wrote:
> Hi Nic, 
> 
> 
> 
> On 2/11/16, 3:47 PM, "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:
> 
> >On Wed, 2016-02-10 at 22:53 -0800, Nicholas A. Bellinger wrote:
> >> On Tue, 2016-02-09 at 18:03 +0000, Himanshu Madhani wrote:
> >> > On 2/8/16, 9:25 PM, "Nicholas A. Bellinger" <nab@linux-iscsi.org>
> >>wrote:
> >> > >On Mon, 2016-02-08 at 23:27 +0000, Himanshu Madhani wrote:
> >> > >> 
> >> > >> I am testing this series with with 4.5.0-rc2+ kernel and I am
> >>seeing
> >> > >>issue
> >> > >> where trying to trigger
> >> > >> sg_reset with option of host/device/bus in loop at 120second
> >>interval
> >> > >> causes call stack. At this point
> >> > >> removing configuration hangs indefinitely. See attached dmesg
> >>output
> >> > >>from
> >> > >> my setup. 
> >> > >> 
> >> > >
> >> > >Thanks alot for testing this.
> >> > >
> >> > >So It looks like we're still hitting a indefinite schedule() on
> >> > >se_cmd->cmd_wait_comp once tcm_qla2xxx session disconnect/reconnect
> >> > >occurs, after repeated explicit active I/O remote-port sg_resets.
> >> > >
> >> > >Does this trigger on the first tcm_qla2xxx session reconnect after
> >> > >explicit remote-port sg_reset..?  Are session reconnects actively
> >>being
> >> > >triggered during the test..?
> >> > >
> >> > >To verify the latter for iscsi-target, I've been using a small patch
> >>to
> >> > >trigger session reset from TMR kthread context in order to simulate
> >>the
> >> > >I_T disconnects.  Something like that would be useful for verifying
> >>with
> >> > >tcm_qla2xxx too.
> >> > >
> >> > >That said, I'll be reproducing with tcm_qla2xxx ports this week, and
> >> > >will enable various debug in a WIP branch for testing.
> >> 
> >> Following up here..
> >> 
> >> So far using my test setup with ISP2532 ports in P2P + RAMDISK_MCP and
> >> v4.5-rc1, repeated remote-port active I/O LUN_RESET (sg_reset -d) has
> >> been functioning as expected with a blocksize_range=4k-256k + iodepth=32
> >> fio write-verify style workload.
> >> 
> >> No ->cmd_kref -1 OOPsen or qla2xxx initiator generated ABORT_TASKs from
> >> outstanding target TAS responses, nor fio write-verify failures to
> >> report after 800x remote-port active I/O LUN_RESETS.
> >> 
> >> Next step will be to verify explicit tcm_qla2xxx port + module shutdown
> >> after 1K test iterations, and then IBLOCK async completions <-> NVMe
> >> backends with the same case.
> >> 
> >
> >After letting this test run over-night up to 7k active I/O remote-port
> >LUN_RESETs, things are still functioning as expected.
> >
> >Also, /etc/init.d/target stop was able to successfully shutdown all
> >active sessions and unload tcm_qla2xxx after the test run.
> >
> >So AFAICT, the active I/O remote-port LUN_RESET changes are stable with
> >tcm_qla2xxx ports, separate from concurrent session disconnect hung task
> >you reported earlier.
> >
> >That said, I'll likely push this series as-is for -rc4, given that Dan
> >has also been able to verify the non conncurrent session disconnect case
> >on his setup generating constant ABORT_TASKs, and it's still surviving
> >both cases for iscsi-target ports.
> >
> >Please give the debug patch from last night a shot, and see if we can
> >determine the se_cmd states when you hit the hung task.
> 
> I¹ll give your latest debug patch try in a little while
> 
> From the testing that I have done, what is seen is that active IO has
> already been completed and qla2xxx driver is waiting for commands to be
> Completed and it¹s waiting indefinitely for cmd_wait_comp.
> So it looks like there is a missing complete call from target_core. I¹ve
> attached our analysis from crash debug on a live system after the issues
> happens.
> 
> 

Thanks for the crash dump output.

So it's a t_state = TRANSPORT_WRITE_PENDING descriptor with
SAM_STAT_CHECK_CONDITION + cmd_kref.refcount = 0:

struct qla_tgt_cmd {
  se_cmd = {
    scsi_status = 0x2
    se_cmd_flags = 0x80090d,

    <SNIP>

    cmd_kref = {
      refcount = {
        counter = 0x0
      }
    }, 
}

The se_cmd_flags=0x80090d translation to enum se_cmd_flags_table:

- SCF_TRANSPORT_TASK_SENSE
- SCF_EMULATED_TASK_SENSE
- SCF_SCSI_DATA_CDB
- SCF_SE_LUN_CMD
- SCF_SENT_CHECK_CONDITION
- SCF_USE_CPUID

Also, ->cmd_wait_comp has a zero rlock counter + dead magic:

cmd_wait_comp = {
      done = 0x0, 
      wait = {
        lock = {
          {
            rlock = {
              raw_lock = {
                val = {
                  counter = 0x0
                }
              }, 
              magic = 0xdead4ead, 
              owner_cpu = 0xffffffff, 
              owner = 0xffffffffffffffff, 

> I can recreate this issue at will within 5 minute of triggering sg_reset
> with following steps
> 
> 1. Export 4 RAM disk LUNs on each of 2 port adapter. Initiator will see 8
> RAM disk targets
> 2. Start IO with 4K block size and 8 threads with 80% write 20% read and
> 100% dandom. 
> (I am using vdbench for generating IO. I can provide setup/config script
> if needed)
> 3. Start sg_reset for each LUNs with first device, bus and host with 120s
> delay. (I¹ve attached
> My script that I am using for triggering sg_reset)
> 

Thanks, will keep looking and try to reproduce with your script.

Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Nicholas A. Bellinger]

[-- Attachment #1: Type: text/plain, Size: 6437 bytes --]

Hi Himanshu & Co,

On Fri, 2016-02-12 at 00:48 -0800, Nicholas A. Bellinger wrote:
> On Fri, 2016-02-12 at 05:30 +0000, Himanshu Madhani wrote:

<SNIP>

> Thanks for the crash dump output.
> 
> So it's a t_state = TRANSPORT_WRITE_PENDING descriptor with
> SAM_STAT_CHECK_CONDITION + cmd_kref.refcount = 0:
> 
> struct qla_tgt_cmd {
>   se_cmd = {
>     scsi_status = 0x2
>     se_cmd_flags = 0x80090d,
> 
>     <SNIP>
> 
>     cmd_kref = {
>       refcount = {
>         counter = 0x0
>       }
>     }, 
> }
> 
> The se_cmd_flags=0x80090d translation to enum se_cmd_flags_table:
> 
> - SCF_TRANSPORT_TASK_SENSE
> - SCF_EMULATED_TASK_SENSE
> - SCF_SCSI_DATA_CDB
> - SCF_SE_LUN_CMD
> - SCF_SENT_CHECK_CONDITION
> - SCF_USE_CPUID
> 

After groking your dump some more:

For SAM_STAT_CHECK_CONDITION with t_state = TRANSPORT_WRITE_PENDING plus
se_cmd->transport_state = 0x880 bits set, is:

- CMD_T_DEV_ACTIVE
- CMD_T_FABRIC_STOP

and sense buffer = 0x70 00 0b 00 00 00 00 0a 00 00 00 00 29 03 00,
which is the following from sense_info_table[]:

      [TCM_CHECK_CONDITION_ABORT_CMD] = {
                .key = ABORTED_COMMAND,
                .asc = 0x29, /* BUS DEVICE RESET FUNCTION OCCURRED */
                .ascq = 0x03,
        },

The descriptor looks like it did make it to tcm_qla2xxx_complete_free()
-> transport_generic_free_cmd() with both qla_tgt_cmd->cmd_sent_to_fw=0,
and qla_tgt_cmd->write_data_transferred=0 set.

The best I can tell, it looks like tcm_qla2xxx_handle_data_work() ->
transport_generic_request_failure() w/ TCM_CHECK_CONDITION_ABORT_CMD is
occurring..

So to confirm, this specific bug was not a result of active I/O
LUN_RESET w/ CMD_T_ABORTED during session disconnect, or otherwise.

> 
> > I can recreate this issue at will within 5 minute of triggering sg_reset
> > with following steps
> > 
> > 1. Export 4 RAM disk LUNs on each of 2 port adapter. Initiator will see 8
> > RAM disk targets
> > 2. Start IO with 4K block size and 8 threads with 80% write 20% read and
> > 100% dandom. 
> > (I am using vdbench for generating IO. I can provide setup/config script
> > if needed)
> > 3. Start sg_reset for each LUNs with first device, bus and host with 120s
> > delay. (I¹ve attached
> > My script that I am using for triggering sg_reset)
> > 
> 
> Thanks, will keep looking and try to reproduce with your script.

So here's my test setup with 3x Intel P3600 NVMe/IBLOCK backends, across
dual ISP2532 ports:

o- / ......................................................................................... [...]
  o- backstores .............................................................................. [...]
  | o- fileio ................................................................... [0 Storage Object]
  | o- iblock .................................................................. [3 Storage Objects]
  | | o- nvme0n1 ............................................................ [/dev/nvme0n1, in use]
  | | o- nvme1n1 ............................................................ [/dev/nvme1n1, in use]
  | | o- nvme2n1 ............................................................ [/dev/nvme2n1, in use]
  | o- pscsi .................................................................... [0 Storage Object]
  | o- rd_mcp ................................................................... [1 Storage Object]
  |   o- ramdisk ...................................................... [16.0G, ramdisk, not in use]
  o- qla2xxx ........................................................................... [2 Targets]
  | o- 21:00:00:24:ff:48:97:7e ........................................................... [enabled]
  | | o- acls .............................................................................. [1 ACL]
  | | | o- 21:00:00:24:ff:48:97:7c ................................................. [3 Mapped LUNs]
  | | |   o- mapped_lun0 ............................................................... [lun0 (rw)]
  | | |   o- mapped_lun1 ............................................................... [lun1 (rw)]
  | | |   o- mapped_lun2 ............................................................... [lun2 (rw)]
  | | o- luns ............................................................................. [3 LUNs]
  | |   o- lun0 .................................................... [iblock/nvme0n1 (/dev/nvme0n1)]
  | |   o- lun1 .................................................... [iblock/nvme1n1 (/dev/nvme1n1)]
  | |   o- lun2 .................................................... [iblock/nvme2n1 (/dev/nvme2n1)]
  | o- 21:00:00:24:ff:48:97:7f ........................................................... [enabled]
  |   o- acls .............................................................................. [1 ACL]
  |   | o- 21:00:00:24:ff:48:97:7d ................................................. [3 Mapped LUNs]
  |   |   o- mapped_lun0 ............................................................... [lun0 (rw)]
  |   |   o- mapped_lun1 ............................................................... [lun1 (rw)]
  |   |   o- mapped_lun2 ............................................................... [lun2 (rw)]
  |   o- luns ............................................................................. [3 LUNs]
  |     o- lun0 .................................................... [iblock/nvme0n1 (/dev/nvme0n1)]
  |     o- lun1 .................................................... [iblock/nvme1n1 (/dev/nvme1n1)]
  |     o- lun2 .................................................... [iblock/nvme2n1 (/dev/nvme2n1)]

Attached is the fio write-verify workload for reference.

Also, a few changes made to your test script:
 
- Use sg_reset -H (-h is help :) for host reset op
- Wait sleep 10 between calls instead of 2 mins
- Limit sg_reset SCSI device list to 3x remote-ports

The last one is to verify with various sg_reset ops across remote-ports
only, separate from any existing active I/O session disconnect bugs that
may exist beyond this specific PATCH-v4 series.

To that end, after verifying tonight with 100x iterations of your script
with the above changes, fio write-verify is still functioning as
expected with remote-port only sg_reset ops using NVMe/IBLOCK backends.

So that said, I'll be pushing what's in target-pending/master as -rc4
code, and continue to debug the hung task as a separate active I/O
shutdown related issue.

Thanks again for your help,

--nab

[-- Attachment #2: fio-nvme.txt --]
[-- Type: text/plain, Size: 248 bytes --]

[global]
thread=1
blocksize_range=4k-256k
direct=1
ioengine=libaio
verify=crc32c-intel
verify_interval=512
iodepth=32
size=1000G
loops=100
numjobs=1
invalidate=0
filename=/dev/sdb
filename=/dev/sdc
filename=/dev/sdd

[verify]
rw=randrw
do_verify=1

Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Himanshu Madhani]

[-- Attachment #1: Type: text/plain, Size: 7103 bytes --]

Hi Nic,



On 2/12/16, 11:03 PM, "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:

>Hi Himanshu & Co,
>
>On Fri, 2016-02-12 at 00:48 -0800, Nicholas A. Bellinger wrote:
>> On Fri, 2016-02-12 at 05:30 +0000, Himanshu Madhani wrote:
>
><SNIP>
>
>> Thanks for the crash dump output.
>> 
>> So it's a t_state = TRANSPORT_WRITE_PENDING descriptor with
>> SAM_STAT_CHECK_CONDITION + cmd_kref.refcount = 0:
>> 
>> struct qla_tgt_cmd {
>>   se_cmd = {
>>     scsi_status = 0x2
>>     se_cmd_flags = 0x80090d,
>> 
>>     <SNIP>
>> 
>>     cmd_kref = {
>>       refcount = {
>>         counter = 0x0
>>       }
>>     }, 
>> }
>> 
>> The se_cmd_flags=0x80090d translation to enum se_cmd_flags_table:
>> 
>> - SCF_TRANSPORT_TASK_SENSE
>> - SCF_EMULATED_TASK_SENSE
>> - SCF_SCSI_DATA_CDB
>> - SCF_SE_LUN_CMD
>> - SCF_SENT_CHECK_CONDITION
>> - SCF_USE_CPUID
>> 
>
>After groking your dump some more:
>
>For SAM_STAT_CHECK_CONDITION with t_state = TRANSPORT_WRITE_PENDING plus
>se_cmd->transport_state = 0x880 bits set, is:
>
>- CMD_T_DEV_ACTIVE
>- CMD_T_FABRIC_STOP
>
>and sense buffer = 0x70 00 0b 00 00 00 00 0a 00 00 00 00 29 03 00,
>which is the following from sense_info_table[]:
>
>      [TCM_CHECK_CONDITION_ABORT_CMD] = {
>                .key = ABORTED_COMMAND,
>                .asc = 0x29, /* BUS DEVICE RESET FUNCTION OCCURRED */
>                .ascq = 0x03,
>        },
>
>The descriptor looks like it did make it to tcm_qla2xxx_complete_free()
>-> transport_generic_free_cmd() with both qla_tgt_cmd->cmd_sent_to_fw=0,
>and qla_tgt_cmd->write_data_transferred=0 set.
>
>The best I can tell, it looks like tcm_qla2xxx_handle_data_work() ->
>transport_generic_request_failure() w/ TCM_CHECK_CONDITION_ABORT_CMD is
>occurring..
>
>So to confirm, this specific bug was not a result of active I/O
>LUN_RESET w/ CMD_T_ABORTED during session disconnect, or otherwise.
>
>> 
>> > I can recreate this issue at will within 5 minute of triggering
>>sg_reset
>> > with following steps
>> > 
>> > 1. Export 4 RAM disk LUNs on each of 2 port adapter. Initiator will
>>see 8
>> > RAM disk targets
>> > 2. Start IO with 4K block size and 8 threads with 80% write 20% read
>>and
>> > 100% dandom. 
>> > (I am using vdbench for generating IO. I can provide setup/config
>>script
>> > if needed)
>> > 3. Start sg_reset for each LUNs with first device, bus and host with
>>120s
>> > delay. (I¹ve attached
>> > My script that I am using for triggering sg_reset)
>> > 
>> 
>> Thanks, will keep looking and try to reproduce with your script.
>
>So here's my test setup with 3x Intel P3600 NVMe/IBLOCK backends, across
>dual ISP2532 ports:
>
>o- / 
>..........................................................................
>............... [...]
>  o- backstores 
>..........................................................................
>.... [...]
>  | o- fileio 
>................................................................... [0
>Storage Object]
>  | o- iblock 
>.................................................................. [3
>Storage Objects]
>  | | o- nvme0n1 
>............................................................
>[/dev/nvme0n1, in use]
>  | | o- nvme1n1 
>............................................................
>[/dev/nvme1n1, in use]
>  | | o- nvme2n1 
>............................................................
>[/dev/nvme2n1, in use]
>  | o- pscsi 
>.................................................................... [0
>Storage Object]
>  | o- rd_mcp 
>................................................................... [1
>Storage Object]
>  |   o- ramdisk ......................................................
>[16.0G, ramdisk, not in use]
>  o- qla2xxx 
>..........................................................................
>. [2 Targets]
>  | o- 21:00:00:24:ff:48:97:7e
>........................................................... [enabled]
>  | | o- acls 
>..........................................................................
>.... [1 ACL]
>  | | | o- 21:00:00:24:ff:48:97:7c
>................................................. [3 Mapped LUNs]
>  | | |   o- mapped_lun0
>............................................................... [lun0
>(rw)]
>  | | |   o- mapped_lun1
>............................................................... [lun1
>(rw)]
>  | | |   o- mapped_lun2
>............................................................... [lun2
>(rw)]
>  | | o- luns 
>..........................................................................
>... [3 LUNs]
>  | |   o- lun0 ....................................................
>[iblock/nvme0n1 (/dev/nvme0n1)]
>  | |   o- lun1 ....................................................
>[iblock/nvme1n1 (/dev/nvme1n1)]
>  | |   o- lun2 ....................................................
>[iblock/nvme2n1 (/dev/nvme2n1)]
>  | o- 21:00:00:24:ff:48:97:7f
>........................................................... [enabled]
>  |   o- acls 
>..........................................................................
>.... [1 ACL]
>  |   | o- 21:00:00:24:ff:48:97:7d
>................................................. [3 Mapped LUNs]
>  |   |   o- mapped_lun0
>............................................................... [lun0
>(rw)]
>  |   |   o- mapped_lun1
>............................................................... [lun1
>(rw)]
>  |   |   o- mapped_lun2
>............................................................... [lun2
>(rw)]
>  |   o- luns 
>..........................................................................
>... [3 LUNs]
>  |     o- lun0 ....................................................
>[iblock/nvme0n1 (/dev/nvme0n1)]
>  |     o- lun1 ....................................................
>[iblock/nvme1n1 (/dev/nvme1n1)]
>  |     o- lun2 ....................................................
>[iblock/nvme2n1 (/dev/nvme2n1)]
>
>Attached is the fio write-verify workload for reference.
>
>Also, a few changes made to your test script:
> 
>- Use sg_reset -H (-h is help :) for host reset op
>- Wait sleep 10 between calls instead of 2 mins
>- Limit sg_reset SCSI device list to 3x remote-ports
>
>The last one is to verify with various sg_reset ops across remote-ports
>only, separate from any existing active I/O session disconnect bugs that
>may exist beyond this specific PATCH-v4 series.
>
>To that end, after verifying tonight with 100x iterations of your script
>with the above changes, fio write-verify is still functioning as
>expected with remote-port only sg_reset ops using NVMe/IBLOCK backends.
>
>So that said, I'll be pushing what's in target-pending/master as -rc4
>code, and continue to debug the hung task as a separate active I/O
>shutdown related issue.

Sure. Thanks for the details about your test and verification. Also,
let me know if you need any help to debug hung task issue.

>
>Thanks again for your help,
>
>--nab

Thanks,
Himanshu


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 7791 bytes --]

Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling

[Dan Lane]

On Fri, Feb 12, 2016 at 12:30 AM, Himanshu Madhani
<himanshu.madhani@qlogic.com> wrote:
> Hi Nic,
>
>
>
> On 2/11/16, 3:47 PM, "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:
>
>>On Wed, 2016-02-10 at 22:53 -0800, Nicholas A. Bellinger wrote:
>>> On Tue, 2016-02-09 at 18:03 +0000, Himanshu Madhani wrote:
>>> > On 2/8/16, 9:25 PM, "Nicholas A. Bellinger" <nab@linux-iscsi.org>
>>>wrote:
>>> > >On Mon, 2016-02-08 at 23:27 +0000, Himanshu Madhani wrote:
>>> > >>
>>> > >> I am testing this series with with 4.5.0-rc2+ kernel and I am
>>>seeing
>>> > >>issue
>>> > >> where trying to trigger
>>> > >> sg_reset with option of host/device/bus in loop at 120second
>>>interval
>>> > >> causes call stack. At this point
>>> > >> removing configuration hangs indefinitely. See attached dmesg
>>>output
>>> > >>from
>>> > >> my setup.
>>> > >>
>>> > >
>>> > >Thanks alot for testing this.
>>> > >
>>> > >So It looks like we're still hitting a indefinite schedule() on
>>> > >se_cmd->cmd_wait_comp once tcm_qla2xxx session disconnect/reconnect
>>> > >occurs, after repeated explicit active I/O remote-port sg_resets.
>>> > >
>>> > >Does this trigger on the first tcm_qla2xxx session reconnect after
>>> > >explicit remote-port sg_reset..?  Are session reconnects actively
>>>being
>>> > >triggered during the test..?
>>> > >
>>> > >To verify the latter for iscsi-target, I've been using a small patch
>>>to
>>> > >trigger session reset from TMR kthread context in order to simulate
>>>the
>>> > >I_T disconnects.  Something like that would be useful for verifying
>>>with
>>> > >tcm_qla2xxx too.
>>> > >
>>> > >That said, I'll be reproducing with tcm_qla2xxx ports this week, and
>>> > >will enable various debug in a WIP branch for testing.
>>>
>>> Following up here..
>>>
>>> So far using my test setup with ISP2532 ports in P2P + RAMDISK_MCP and
>>> v4.5-rc1, repeated remote-port active I/O LUN_RESET (sg_reset -d) has
>>> been functioning as expected with a blocksize_range=4k-256k + iodepth=32
>>> fio write-verify style workload.
>>>
>>> No ->cmd_kref -1 OOPsen or qla2xxx initiator generated ABORT_TASKs from
>>> outstanding target TAS responses, nor fio write-verify failures to
>>> report after 800x remote-port active I/O LUN_RESETS.
>>>
>>> Next step will be to verify explicit tcm_qla2xxx port + module shutdown
>>> after 1K test iterations, and then IBLOCK async completions <-> NVMe
>>> backends with the same case.
>>>
>>
>>After letting this test run over-night up to 7k active I/O remote-port
>>LUN_RESETs, things are still functioning as expected.
>>
>>Also, /etc/init.d/target stop was able to successfully shutdown all
>>active sessions and unload tcm_qla2xxx after the test run.
>>
>>So AFAICT, the active I/O remote-port LUN_RESET changes are stable with
>>tcm_qla2xxx ports, separate from concurrent session disconnect hung task
>>you reported earlier.
>>
>>That said, I'll likely push this series as-is for -rc4, given that Dan
>>has also been able to verify the non conncurrent session disconnect case
>>on his setup generating constant ABORT_TASKs, and it's still surviving
>>both cases for iscsi-target ports.
>>
>>Please give the debug patch from last night a shot, and see if we can
>>determine the se_cmd states when you hit the hung task.
>
> I¹ll give your latest debug patch try in a little while
>
> From the testing that I have done, what is seen is that active IO has
> already been completed and qla2xxx driver is waiting for commands to be
> Completed and it¹s waiting indefinitely for cmd_wait_comp.
> So it looks like there is a missing complete call from target_core. I¹ve
> attached our analysis from crash debug on a live system after the issues
> happens.
>
>
> I can recreate this issue at will within 5 minute of triggering sg_reset
> with following steps
>
> 1. Export 4 RAM disk LUNs on each of 2 port adapter. Initiator will see 8
> RAM disk targets
> 2. Start IO with 4K block size and 8 threads with 80% write 20% read and
> 100% dandom.
> (I am using vdbench for generating IO. I can provide setup/config script
> if needed)
> 3. Start sg_reset for each LUNs with first device, bus and host with 120s
> delay. (I¹ve attached
> My script that I am using for triggering sg_reset)
>
>>
>>Thank you,
>>
>>-nab
>>
>

Has there been any update to this?

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html