[PATCH 0/4] target: LUN_RESET bugfixes for HW target mode

[PATCH 0/4] target: LUN_RESET bugfixes for HW target mode

[Nicholas A. Bellinger]

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

Hi folks,

This patch series addresses a number of issues that have come up recently for
TMR LUN_RESET operation with HW target mode + qla2xxx/tcm_qla2xxx code.  Aside
from the various bugfixes in this series, the big change is the re-org of
core_tmr_lun_reset() logic to prevent possible list corruption while scanning
the various lists and breaking this out of this code into seperate functions.

I know the re-org patch diff is very difficult to read without more context, so
apologies ahead of time for that.  At this point it makes sense to queue these
for v3.2, and CC stable to ensure they are also picked up for v3.1 target code
as well.

Please let us know if you have any comments.

Thank you,

--nab

Nicholas Bellinger (3):
  target: Re-org of core_tmr_lun_reset + FREE_CMD_INTR bugfix
  target: Fix transport_cmd_finish_abort queue removal bug
  target: Prevent transport_send_task_abort when CHECK_CONDITION status

Roland Dreier (1):
  target: Prevent cmd->se_queue_node double add

 drivers/target/target_core_tmr.c       |  228 ++++++++++++++++++++------------
 drivers/target/target_core_transport.c |   45 ++++---
 2 files changed, 169 insertions(+), 104 deletions(-)

-- 
1.7.2.5

[PATCH 1/4] target: Prevent cmd->se_queue_node double add

[Nicholas A. Bellinger]

From: Roland Dreier <roland@purestorage.com>

This patch addresses a bug with the lio-core-2.6.git conversion of
transport_add_cmd_to_queue() to use a single embedded list_head, instead
of individual struct se_queue_req allocations allowing a single se_cmd to
be added to the queue mulitple times.  This was changed in the following:

commit 2a9e4d5ca5d99f4c600578d6285d45142e7e5208
Author: Andy Grover <agrover@redhat.com>
Date:   Tue Apr 26 17:45:51 2011 -0700

    target: Embed qr in struct se_cmd

The problem is that some target code still assumes performing multiple
adds is allowed via transport_add_cmd_to_queue(), which ends up causing
list corruption in qobj->qobj_list code.  This patch addresses this
by removing an existing struct se_cmd from the list before the add, and
removes an unnecessary list walk in transport_remove_cmd_from_queue()

It also changes cmd->t_transport_queue_active to use explict sets intead
of increment/decrement to prevent confusion during exception path handling.

Signed-off-by: Roland Dreier <roland@purestorage.com>
Cc: Andy Grover <agrover@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@risingtidesystems.com>
---
 drivers/target/target_core_tmr.c       |    2 +-
 drivers/target/target_core_transport.c |   31 +++++++++++++++----------------
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 27d4925..7bce92f 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -340,7 +340,7 @@ int core_tmr_lun_reset(
 
 		atomic_dec(&cmd->t_transport_queue_active);
 		atomic_dec(&qobj->queue_cnt);
-		list_del(&cmd->se_queue_node);
+		list_del_init(&cmd->se_queue_node);
 		spin_unlock_irqrestore(&qobj->cmd_queue_lock, flags);
 
 		pr_debug("LUN_RESET: %s from Device Queue: cmd: %p t_state:"
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 315790e..86470b7 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -624,8 +624,6 @@ static void transport_add_cmd_to_queue(
 	struct se_queue_obj *qobj = &dev->dev_queue_obj;
 	unsigned long flags;
 
-	INIT_LIST_HEAD(&cmd->se_queue_node);
-
 	if (t_state) {
 		spin_lock_irqsave(&cmd->t_state_lock, flags);
 		cmd->t_state = t_state;
@@ -634,15 +632,21 @@ static void transport_add_cmd_to_queue(
 	}
 
 	spin_lock_irqsave(&qobj->cmd_queue_lock, flags);
+
+	/* If the cmd is already on the list, remove it before we add it */
+	if (!list_empty(&cmd->se_queue_node))
+		list_del(&cmd->se_queue_node);
+	else
+		atomic_inc(&qobj->queue_cnt);
+
 	if (cmd->se_cmd_flags & SCF_EMULATE_QUEUE_FULL) {
 		cmd->se_cmd_flags &= ~SCF_EMULATE_QUEUE_FULL;
 		list_add(&cmd->se_queue_node, &qobj->qobj_list);
 	} else
 		list_add_tail(&cmd->se_queue_node, &qobj->qobj_list);
-	atomic_inc(&cmd->t_transport_queue_active);
+	atomic_set(&cmd->t_transport_queue_active, 1);
 	spin_unlock_irqrestore(&qobj->cmd_queue_lock, flags);
 
-	atomic_inc(&qobj->queue_cnt);
 	wake_up_interruptible(&qobj->thread_wq);
 }
 
@@ -659,9 +663,9 @@ transport_get_cmd_from_queue(struct se_queue_obj *qobj)
 	}
 	cmd = list_first_entry(&qobj->qobj_list, struct se_cmd, se_queue_node);
 
-	atomic_dec(&cmd->t_transport_queue_active);
+	atomic_set(&cmd->t_transport_queue_active, 0);
 
-	list_del(&cmd->se_queue_node);
+	list_del_init(&cmd->se_queue_node);
 	atomic_dec(&qobj->queue_cnt);
 	spin_unlock_irqrestore(&qobj->cmd_queue_lock, flags);
 
@@ -671,7 +675,6 @@ transport_get_cmd_from_queue(struct se_queue_obj *qobj)
 static void transport_remove_cmd_from_queue(struct se_cmd *cmd,
 		struct se_queue_obj *qobj)
 {
-	struct se_cmd *t;
 	unsigned long flags;
 
 	spin_lock_irqsave(&qobj->cmd_queue_lock, flags);
@@ -679,14 +682,9 @@ static void transport_remove_cmd_from_queue(struct se_cmd *cmd,
 		spin_unlock_irqrestore(&qobj->cmd_queue_lock, flags);
 		return;
 	}
-
-	list_for_each_entry(t, &qobj->qobj_list, se_queue_node)
-		if (t == cmd) {
-			atomic_dec(&cmd->t_transport_queue_active);
-			atomic_dec(&qobj->queue_cnt);
-			list_del(&cmd->se_queue_node);
-			break;
-		}
+	atomic_set(&cmd->t_transport_queue_active, 0);
+	atomic_dec(&qobj->queue_cnt);
+	list_del_init(&cmd->se_queue_node);
 	spin_unlock_irqrestore(&qobj->cmd_queue_lock, flags);
 
 	if (atomic_read(&cmd->t_transport_queue_active)) {
@@ -1070,7 +1068,7 @@ static void transport_release_all_cmds(struct se_device *dev)
 	list_for_each_entry_safe(cmd, tcmd, &dev->dev_queue_obj.qobj_list,
 				 se_queue_node) {
 		t_state = cmd->t_state;
-		list_del(&cmd->se_queue_node);
+		list_del_init(&cmd->se_queue_node);
 		spin_unlock_irqrestore(&dev->dev_queue_obj.cmd_queue_lock,
 				flags);
 
@@ -1601,6 +1599,7 @@ void transport_init_se_cmd(
 	INIT_LIST_HEAD(&cmd->se_delayed_node);
 	INIT_LIST_HEAD(&cmd->se_ordered_node);
 	INIT_LIST_HEAD(&cmd->se_qf_node);
+	INIT_LIST_HEAD(&cmd->se_queue_node);
 
 	INIT_LIST_HEAD(&cmd->t_task_list);
 	init_completion(&cmd->transport_lun_fe_stop_comp);
-- 
1.7.2.5

[PATCH 2/4] target: Re-org of core_tmr_lun_reset + FREE_CMD_INTR bugfix

[Nicholas A. Bellinger]

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

This patch is a re-orginzation of core_tmr_lun_reset() logic to properly
scan the active tmr_list, dev->state_task_list and qobj->qobj_list w/ the
relivent locks held, and performing a list_move_tail onto seperate local
scope lists before performing the full drain.

This involves breaking out the code into three seperate list specific
functions: core_tmr_drain_tmr_list(), core_tmr_drain_task_list() and
core_tmr_drain_cmd_list().

This patch also includes a bugfix related to TRANSPORT_FREE_CMD_INTR
operation, where core_tmr_drain_cmd_list will skip this cause in order
to prevent an ABORT_TASK status from being returned for descriptors that
are already queued up to be related by processing thread context.

Reported-by: Roland Dreier <roland@purestorage.com>
Cc: Roland Dreier <roland@purestorage.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@risingtidesystems.com>
---
 drivers/target/target_core_tmr.c |  226 ++++++++++++++++++++++++--------------
 1 files changed, 141 insertions(+), 85 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 7bce92f..a536f8a 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -67,15 +67,16 @@ void core_tmr_release_req(
 	struct se_tmr_req *tmr)
 {
 	struct se_device *dev = tmr->tmr_dev;
+	unsigned long flags;
 
 	if (!dev) {
 		kmem_cache_free(se_tmr_req_cache, tmr);
 		return;
 	}
 
-	spin_lock_irq(&dev->se_tmr_lock);
+	spin_lock_irqsave(&dev->se_tmr_lock, flags);
 	list_del(&tmr->tmr_list);
-	spin_unlock_irq(&dev->se_tmr_lock);
+	spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
 
 	kmem_cache_free(se_tmr_req_cache, tmr);
 }
@@ -100,54 +101,20 @@ static void core_tmr_handle_tas_abort(
 	transport_cmd_finish_abort(cmd, 0);
 }
 
-int core_tmr_lun_reset(
+static void core_tmr_drain_tmr_list(
 	struct se_device *dev,
 	struct se_tmr_req *tmr,
-	struct list_head *preempt_and_abort_list,
-	struct se_cmd *prout_cmd)
+	struct list_head *preempt_and_abort_list)
 {
-	struct se_cmd *cmd, *tcmd;
-	struct se_node_acl *tmr_nacl = NULL;
-	struct se_portal_group *tmr_tpg = NULL;
-	struct se_queue_obj *qobj = &dev->dev_queue_obj;
+	LIST_HEAD(drain_tmr_list);
 	struct se_tmr_req *tmr_p, *tmr_pp;
-	struct se_task *task, *task_tmp;
+	struct se_cmd *cmd;
 	unsigned long flags;
-	int fe_count, tas;
-	/*
-	 * TASK_ABORTED status bit, this is configurable via ConfigFS
-	 * struct se_device attributes.  spc4r17 section 7.4.6 Control mode page
-	 *
-	 * A task aborted status (TAS) bit set to zero specifies that aborted
-	 * tasks shall be terminated by the device server without any response
-	 * to the application client. A TAS bit set to one specifies that tasks
-	 * aborted by the actions of an I_T nexus other than the I_T nexus on
-	 * which the command was received shall be completed with TASK ABORTED
-	 * status (see SAM-4).
-	 */
-	tas = dev->se_sub_dev->se_dev_attrib.emulate_tas;
-	/*
-	 * Determine if this se_tmr is coming from a $FABRIC_MOD
-	 * 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;
-		if (tmr_nacl && tmr_tpg) {
-			pr_debug("LUN_RESET: TMR caller fabric: %s"
-				" initiator port %s\n",
-				tmr_tpg->se_tpg_tfo->get_fabric_name(),
-				tmr_nacl->initiatorname);
-		}
-	}
-	pr_debug("LUN_RESET: %s starting for [%s], tas: %d\n",
-		(preempt_and_abort_list) ? "Preempt" : "TMR",
-		dev->transport->name, tas);
 	/*
 	 * Release all pending and outgoing TMRs aside from the received
 	 * LUN_RESET tmr..
 	 */
-	spin_lock_irq(&dev->se_tmr_lock);
+	spin_lock_irqsave(&dev->se_tmr_lock, flags);
 	list_for_each_entry_safe(tmr_p, tmr_pp, &dev->dev_tmr_list, tmr_list) {
 		/*
 		 * Allow the received TMR to return with FUNCTION_COMPLETE.
@@ -169,29 +136,48 @@ int core_tmr_lun_reset(
 		    (core_scsi3_check_cdb_abort_and_preempt(
 					preempt_and_abort_list, cmd) != 0))
 			continue;
-		spin_unlock_irq(&dev->se_tmr_lock);
-
-		spin_lock_irqsave(&cmd->t_state_lock, flags);
+		
+		spin_lock(&cmd->t_state_lock);
 		if (!atomic_read(&cmd->t_transport_active)) {
-			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-			spin_lock_irq(&dev->se_tmr_lock);
+			spin_unlock(&cmd->t_state_lock);
 			continue;
 		}
 		if (cmd->t_state == TRANSPORT_ISTATE_PROCESSING) {
-			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-			spin_lock_irq(&dev->se_tmr_lock);
+			spin_unlock(&cmd->t_state_lock);
 			continue;
 		}
-		pr_debug("LUN_RESET: %s releasing TMR %p Function: 0x%02x,"
-			" Response: 0x%02x, t_state: %d\n",
-			(preempt_and_abort_list) ? "Preempt" : "", tmr_p,
-			tmr_p->function, tmr_p->response, cmd->t_state);
-		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+		spin_unlock(&cmd->t_state_lock);
+
+		list_move_tail(&tmr->tmr_list, &drain_tmr_list);
+	}
+	spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
 
+	while (!list_empty(&drain_tmr_list)) {
+		tmr = list_entry(drain_tmr_list.next, struct se_tmr_req, tmr_list);
+		list_del(&tmr->tmr_list);
+		cmd = tmr_p->task_cmd;
+
+		printk("LUN_RESET: %s releasing TMR %p Function: 0x%02x,"
+			" Response: 0x%02x, t_state: %d\n",
+			(preempt_and_abort_list) ? "Preempt" : "", tmr,
+			tmr->function, tmr->response, cmd->t_state);
+		
 		transport_cmd_finish_abort_tmr(cmd);
-		spin_lock_irq(&dev->se_tmr_lock);
 	}
-	spin_unlock_irq(&dev->se_tmr_lock);
+}
+
+static void core_tmr_drain_task_list(
+	struct se_device *dev,
+	struct se_cmd *prout_cmd,
+	struct se_node_acl *tmr_nacl,
+	int tas,
+	struct list_head *preempt_and_abort_list)
+{
+	LIST_HEAD(drain_task_list);
+	struct se_cmd *cmd;
+	struct se_task *task, *task_tmp;
+	unsigned long flags;
+	int fe_count;
 	/*
 	 * Complete outstanding struct se_task CDBs with TASK_ABORTED SAM status.
 	 * This is following sam4r17, section 5.6 Aborting commands, Table 38
@@ -236,19 +222,26 @@ int core_tmr_lun_reset(
 		if (prout_cmd == cmd)
 			continue;
 
-		list_del(&task->t_state_list);
+		list_move_tail(&task->t_state_list, &drain_task_list);
 		atomic_set(&task->task_state_active, 0);
-		spin_unlock_irqrestore(&dev->execute_task_lock, flags);
+		atomic_set(&task->task_execute_queue, 0);
+	}
+	spin_unlock_irqrestore(&dev->execute_task_lock, flags);
+
+	while (!list_empty(&drain_task_list)) {
+		task = list_entry(drain_task_list.next, struct se_task, t_state_list);
+		list_del(&task->t_state_list);
+		cmd = task->task_se_cmd;
 
 		spin_lock_irqsave(&cmd->t_state_lock, flags);
-		pr_debug("LUN_RESET: %s cmd: %p task: %p"
+		printk("LUN_RESET: %s cmd: %p task: %p"
 			" ITT/CmdSN: 0x%08x/0x%08x, i_state: %d, t_state/"
 			"def_t_state: %d/%d cdb: 0x%02x\n",
 			(preempt_and_abort_list) ? "Preempt" : "", cmd, task,
 			cmd->se_tfo->get_task_tag(cmd), 0,
 			cmd->se_tfo->get_cmd_state(cmd), cmd->t_state,
 			cmd->deferred_t_state, cmd->t_task_cdb[0]);
-		pr_debug("LUN_RESET: ITT[0x%08x] - pr_res_key: 0x%016Lx"
+		printk("LUN_RESET: ITT[0x%08x] - pr_res_key: 0x%016Lx"
 			" t_task_cdbs: %d t_task_cdbs_left: %d"
 			" t_task_cdbs_sent: %d -- t_transport_active: %d"
 			" t_transport_stop: %d t_transport_sent: %d\n",
@@ -265,55 +258,58 @@ int core_tmr_lun_reset(
 			spin_unlock_irqrestore(
 				&cmd->t_state_lock, flags);
 
-			pr_debug("LUN_RESET: Waiting for task: %p to shutdown"
+			printk("LUN_RESET: Waiting for task: %p to shutdown"
 				" for dev: %p\n", task, dev);
 			wait_for_completion(&task->task_stop_comp);
-			pr_debug("LUN_RESET Completed task: %p shutdown for"
+			printk("LUN_RESET Completed task: %p shutdown for"
 				" dev: %p\n", task, dev);
 			spin_lock_irqsave(&cmd->t_state_lock, flags);
 			atomic_dec(&cmd->t_task_cdbs_left);
 
 			atomic_set(&task->task_active, 0);
 			atomic_set(&task->task_stop, 0);
-		} else {
-			if (atomic_read(&task->task_execute_queue) != 0)
-				transport_remove_task_from_execute_queue(task, dev);
 		}
 		__transport_stop_task_timer(task, &flags);
 
 		if (!atomic_dec_and_test(&cmd->t_task_cdbs_ex_left)) {
-			spin_unlock_irqrestore(
-					&cmd->t_state_lock, flags);
-			pr_debug("LUN_RESET: Skipping task: %p, dev: %p for"
+			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+			printk("LUN_RESET: Skipping task: %p, dev: %p for"
 				" t_task_cdbs_ex_left: %d\n", task, dev,
 				atomic_read(&cmd->t_task_cdbs_ex_left));
-
-			spin_lock_irqsave(&dev->execute_task_lock, flags);
 			continue;
 		}
 		fe_count = atomic_read(&cmd->t_fe_count);
 
 		if (atomic_read(&cmd->t_transport_active)) {
-			pr_debug("LUN_RESET: got t_transport_active = 1 for"
+			printk("LUN_RESET: got t_transport_active = 1 for"
 				" task: %p, t_fe_count: %d dev: %p\n", task,
 				fe_count, dev);
 			atomic_set(&cmd->t_transport_aborted, 1);
-			spin_unlock_irqrestore(&cmd->t_state_lock,
-						flags);
-			core_tmr_handle_tas_abort(tmr_nacl, cmd, tas, fe_count);
+			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 
-			spin_lock_irqsave(&dev->execute_task_lock, flags);
+			core_tmr_handle_tas_abort(tmr_nacl, cmd, tas, fe_count);
 			continue;
 		}
-		pr_debug("LUN_RESET: Got t_transport_active = 0 for task: %p,"
+		printk("LUN_RESET: Got t_transport_active = 0 for task: %p,"
 			" t_fe_count: %d dev: %p\n", task, fe_count, dev);
 		atomic_set(&cmd->t_transport_aborted, 1);
 		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-		core_tmr_handle_tas_abort(tmr_nacl, cmd, tas, fe_count);
 
-		spin_lock_irqsave(&dev->execute_task_lock, flags);
+		core_tmr_handle_tas_abort(tmr_nacl, cmd, tas, fe_count);
 	}
-	spin_unlock_irqrestore(&dev->execute_task_lock, flags);
+}
+
+static void core_tmr_drain_cmd_list(
+	struct se_device *dev,
+	struct se_cmd *prout_cmd,
+	struct se_node_acl *tmr_nacl,
+	int tas,
+	struct list_head *preempt_and_abort_list)
+{
+	LIST_HEAD(drain_cmd_list);
+	struct se_queue_obj *qobj = &dev->dev_queue_obj;
+	struct se_cmd *cmd, *tcmd;
+	unsigned long flags;
 	/*
 	 * Release all commands remaining in the struct se_device cmd queue.
 	 *
@@ -337,13 +333,28 @@ int core_tmr_lun_reset(
 		 */
 		if (prout_cmd == cmd)
 			continue;
+		/*
+		 * Skip direct processing of TRANSPORT_FREE_CMD_INTR for
+		 * HW target mode fabrics.
+		 */		
+		spin_lock(&cmd->t_state_lock);
+		if (cmd->t_state == TRANSPORT_FREE_CMD_INTR) {
+			spin_unlock(&cmd->t_state_lock);
+			continue;
+		}
+		spin_unlock(&cmd->t_state_lock);
 
-		atomic_dec(&cmd->t_transport_queue_active);
+		atomic_set(&cmd->t_transport_queue_active, 0);
 		atomic_dec(&qobj->queue_cnt);
+		list_move_tail(&cmd->se_queue_node, &drain_cmd_list);
+	}
+	spin_unlock_irqrestore(&qobj->cmd_queue_lock, flags);
+
+	while (!list_empty(&drain_cmd_list)) {
+		cmd = list_entry(drain_cmd_list.next, struct se_cmd, se_queue_node);
 		list_del_init(&cmd->se_queue_node);
-		spin_unlock_irqrestore(&qobj->cmd_queue_lock, flags);
 
-		pr_debug("LUN_RESET: %s from Device Queue: cmd: %p t_state:"
+		printk("LUN_RESET: %s from Device Queue: cmd: %p t_state:"
 			" %d t_fe_count: %d\n", (preempt_and_abort_list) ?
 			"Preempt" : "", cmd, cmd->t_state,
 			atomic_read(&cmd->t_fe_count));
@@ -354,9 +365,53 @@ int core_tmr_lun_reset(
 
 		core_tmr_handle_tas_abort(tmr_nacl, cmd, tas,
 				atomic_read(&cmd->t_fe_count));
-		spin_lock_irqsave(&qobj->cmd_queue_lock, flags);
 	}
-	spin_unlock_irqrestore(&qobj->cmd_queue_lock, flags);
+}
+
+int core_tmr_lun_reset(
+        struct se_device *dev,
+        struct se_tmr_req *tmr,
+        struct list_head *preempt_and_abort_list,
+        struct se_cmd *prout_cmd)
+{
+	struct se_node_acl *tmr_nacl = NULL;
+	struct se_portal_group *tmr_tpg = NULL;
+	int tas;
+        /*
+	 * TASK_ABORTED status bit, this is configurable via ConfigFS
+	 * struct se_device attributes.  spc4r17 section 7.4.6 Control mode page
+	 *
+	 * A task aborted status (TAS) bit set to zero specifies that aborted
+	 * tasks shall be terminated by the device server without any response
+	 * to the application client. A TAS bit set to one specifies that tasks
+	 * aborted by the actions of an I_T nexus other than the I_T nexus on
+	 * which the command was received shall be completed with TASK ABORTED
+	 * status (see SAM-4).
+	 */
+	tas = dev->se_sub_dev->se_dev_attrib.emulate_tas;
+	/*
+	 * Determine if this se_tmr is coming from a $FABRIC_MOD
+	 * 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;
+		if (tmr_nacl && tmr_tpg) {
+			printk("LUN_RESET: TMR caller fabric: %s"
+				" initiator port %s\n",
+				tmr_tpg->se_tpg_tfo->get_fabric_name(),
+				tmr_nacl->initiatorname);
+		}
+	}
+	printk("LUN_RESET: %s starting for [%s], tas: %d\n",
+		(preempt_and_abort_list) ? "Preempt" : "TMR",
+		dev->transport->name, tas);
+
+	core_tmr_drain_tmr_list(dev, tmr, preempt_and_abort_list);
+	core_tmr_drain_task_list(dev, prout_cmd, tmr_nacl, tas,
+				preempt_and_abort_list);
+	core_tmr_drain_cmd_list(dev, prout_cmd, tmr_nacl, tas,
+				preempt_and_abort_list);
 	/*
 	 * Clear any legacy SPC-2 reservation when called during
 	 * LOGICAL UNIT RESET
@@ -367,15 +422,16 @@ int core_tmr_lun_reset(
 		dev->dev_reserved_node_acl = NULL;
 		dev->dev_flags &= ~DF_SPC2_RESERVATIONS;
 		spin_unlock(&dev->dev_reservation_lock);
-		pr_debug("LUN_RESET: SCSI-2 Released reservation\n");
+		printk("LUN_RESET: SCSI-2 Released reservation\n");
 	}
 
 	spin_lock_irq(&dev->stats_lock);
 	dev->num_resets++;
 	spin_unlock_irq(&dev->stats_lock);
 
-	pr_debug("LUN_RESET: %s for [%s] Complete\n",
+	printk("LUN_RESET: %s for [%s] Complete\n",
 			(preempt_and_abort_list) ? "Preempt" : "TMR",
 			dev->transport->name);
 	return 0;
 }
+
-- 
1.7.2.5

[PATCH 3/4] target: Fix transport_cmd_finish_abort queue removal bug

[Nicholas A. Bellinger]

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

This patch fixes a bug in LUN_RESET operation with transport_cmd_finish_abort()
where transport_remove_cmd_from_queue() was incorrectly being called, causing
descriptors with t_state == TRANSPORT_FREE_CMD_INTR to be incorrectly removed
from qobj->qobj_list during process context release.  This change ensures the
descriptor is only removed via transport_remove_cmd_from_queue() when doing a
direct release via transport_generic_remove().

Signed-off-by: Nicholas Bellinger <nab@risingtidesystems.com>
---
 drivers/target/target_core_transport.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 86470b7..33fcb6f 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -597,13 +597,14 @@ check_lun:
 
 void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
 {
-	transport_remove_cmd_from_queue(cmd, &cmd->se_dev->dev_queue_obj);
 	transport_lun_remove_cmd(cmd);
 
 	if (transport_cmd_check_stop_to_fabric(cmd))
 		return;
-	if (remove)
+	if (remove) {
+		transport_remove_cmd_from_queue(cmd, &cmd->se_dev->dev_queue_obj);
 		transport_generic_remove(cmd, 0);
+	}
 }
 
 void transport_cmd_finish_abort_tmr(struct se_cmd *cmd)
-- 
1.7.2.5

[PATCH 4/4] target: Prevent transport_send_task_abort when CHECK_CONDITION status

[Nicholas A. Bellinger]

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

This patch fixes a bug where transport_send_task_abort() could be called
during LUN_RESET to return SAM_STAT_TASK_ABORTED + tfo->queue_status(), when
SCF_SENT_CHECK_CONDITION -> tfo->queue_status() has already been sent from
within another context via transport_send_check_condition_and_sense().

Signed-off-by: Nicholas Bellinger <nab@risingtidesystems.com>
---
 drivers/target/target_core_transport.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 33fcb6f..f5b6347 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -4924,6 +4924,15 @@ EXPORT_SYMBOL(transport_check_aborted_status);
 
 void transport_send_task_abort(struct se_cmd *cmd)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	if (cmd->se_cmd_flags & SCF_SENT_CHECK_CONDITION) {
+		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+		return;
+	}
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
 	/*
 	 * If there are still expected incoming fabric WRITEs, we wait
 	 * until until they have completed before sending a TASK_ABORTED
-- 
1.7.2.5

Re: [PATCH 0/4] target: LUN_RESET bugfixes for HW target mode

[Nicholas A. Bellinger]

On Fri, 2011-09-30 at 04:55 +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Hi folks,
> 
> This patch series addresses a number of issues that have come up recently for
> TMR LUN_RESET operation with HW target mode + qla2xxx/tcm_qla2xxx code.  Aside
> from the various bugfixes in this series, the big change is the re-org of
> core_tmr_lun_reset() logic to prevent possible list corruption while scanning
> the various lists and breaking this out of this code into seperate functions.
> 
> I know the re-org patch diff is very difficult to read without more context, so
> apologies ahead of time for that.  At this point it makes sense to queue these
> for v3.2, and CC stable to ensure they are also picked up for v3.1 target code
> as well.
> 
> Please let us know if you have any comments.
> 
> Thank you,
> 
> --nab
> 
> Nicholas Bellinger (3):
>   target: Re-org of core_tmr_lun_reset + FREE_CMD_INTR bugfix
>   target: Fix transport_cmd_finish_abort queue removal bug
>   target: Prevent transport_send_task_abort when CHECK_CONDITION status
> 
> Roland Dreier (1):
>   target: Prevent cmd->se_queue_node double add
> 
>  drivers/target/target_core_tmr.c       |  228 ++++++++++++++++++++------------
>  drivers/target/target_core_transport.c |   45 ++++---
>  2 files changed, 169 insertions(+), 104 deletions(-)
> 

Hi again folks,

The full context for 'Re-org of core_tmr_lun_reset + FREE_CMD_INTR
bugfix' can be found here for review.

http://www.risingtidesystems.com/git/?p=lio-core-2.6.git;a=blob;f=drivers/target/target_core_tmr.c;h=89459086e68628a87a77dd086c6136871db86f0b;hb=HEAD#l104

Thanks,

--nab

Re: [PATCH 1/4] target: Prevent cmd->se_queue_node double add

[Christoph Hellwig]

> +	/* If the cmd is already on the list, remove it before we add it */
> +	if (!list_empty(&cmd->se_queue_node))
> +		list_del(&cmd->se_queue_node);
> +	else
> +		atomic_inc(&qobj->queue_cnt);
> +

This is not an overly useful comment, as it states the obvious, aka what
we are doing here.  It is much better to explain why the semingly
unexpected (re-adding the same cmd) might be valid under certain
circumstances.  Even better would be asserts that we only do it under
those circumstances.

Re: [PATCH 2/4] target: Re-org of core_tmr_lun_reset + FREE_CMD_INTR bugfix

[Christoph Hellwig]

On Fri, Sep 30, 2011 at 04:55:09AM +0000, Nicholas A. Bellinger wrote:
> This patch also includes a bugfix related to TRANSPORT_FREE_CMD_INTR
> operation, where core_tmr_drain_cmd_list will skip this cause in order
> to prevent an ABORT_TASK status from being returned for descriptors that
> are already queued up to be related by processing thread context.

Please try to avoid mixing random bux fixes into code reorganization.


I also really hate adding more special cases for
TRANSPORT_FREE_CMD_INTR.  Freeing a command from interrupt should really
done by using a qla2xxx specific workqueue, and the core should not know
about the pending free before we call into transport_generic_free_cmd.

This is pretty important for the conversion of the target execution
model to concurrent workqueues in addition to simply not making the
code a mess.  I guess I'll have move making qla2xxx not abusing the
processing thread for everything the highest priority in that area
so that we don't get more special cases for it added.

Except for that is looks fine to me.

Re: [PATCH 3/4] target: Fix transport_cmd_finish_abort queue removal bug

[Christoph Hellwig]

The fix looks reasonable, but shouldn't the reordering also be applied
to transport_cmd_finish_abort_tmr?

Re: [PATCH 2/4] target: Re-org of core_tmr_lun_reset + FREE_CMD_INTR bugfix

[Nicholas A. Bellinger]

On Sat, 2011-10-08 at 21:30 -0400, Christoph Hellwig wrote:
> On Fri, Sep 30, 2011 at 04:55:09AM +0000, Nicholas A. Bellinger wrote:
> > This patch also includes a bugfix related to TRANSPORT_FREE_CMD_INTR
> > operation, where core_tmr_drain_cmd_list will skip this cause in order
> > to prevent an ABORT_TASK status from being returned for descriptors that
> > are already queued up to be related by processing thread context.
> 
> Please try to avoid mixing random bux fixes into code reorganization.
> 
> 

Fair enough, i'm happy to break out this single bug-fix into a separate
patch for mainline.

> I also really hate adding more special cases for
> TRANSPORT_FREE_CMD_INTR.  Freeing a command from interrupt should really
> done by using a qla2xxx specific workqueue, and the core should not know
> about the pending free before we call into transport_generic_free_cmd.
> 

Yes, the usage of TRANSPORT_FREE_CMD_INTR with qla2xx is also
problematic with transport_generic_wait_for_tasks(), because readding
se_cmd via transport_add_cmd_to_queue() will reset the
cmd->t_transport_active bit..

Putting this into a qla2xxx specific workqueue is fine by me.

> 
> This is pretty important for the conversion of the target execution
> model to concurrent workqueues in addition to simply not making the
> code a mess.  I guess I'll have move making qla2xxx not abusing the
> processing thread for everything the highest priority in that area
> so that we don't get more special cases for it added.
> 
> Except for that is looks fine to me.
> 

Currently there are no additional TRANSPORT_FREE_CMD_INTR cases that we
have run into thus far.

--nab

Re: [PATCH 3/4] target: Fix transport_cmd_finish_abort queue removal bug

[Nicholas A. Bellinger]

On Sat, 2011-10-08 at 21:33 -0400, Christoph Hellwig wrote:
> The fix looks reasonable, but shouldn't the reordering also be applied
> to transport_cmd_finish_abort_tmr?
> 

The reordering in transport_cmd_finish_abort() was not the issue here,
but the calling transport_remove_cmd_from_queue() when 'remove == 0'.

This should not be a problem in transport_cmd_finish_abort_tmr() as we
always expect to release the tmr associated command.

--nab

Re: [PATCH 3/4] target: Fix transport_cmd_finish_abort queue removal bug

[Christoph Hellwig]

On Sat, Oct 08, 2011 at 07:03:32PM -0700, Nicholas A. Bellinger wrote:
> The reordering in transport_cmd_finish_abort() was not the issue here,
> but the calling transport_remove_cmd_from_queue() when 'remove == 0'.
>
> This should not be a problem in transport_cmd_finish_abort_tmr() as we
> always expect to release the tmr associated command.

I know, but I'd really not have subtily different calling conventions
between the two.

Right now transport_cmd_finish_abort and transport_cmd_finish_abort_tmr
only differ in:

 - transport_cmd_finish_abort calling transport_lun_remove_cmd
 - transport_cmd_finish_abort having the remove flag to make the
   transport_put_cmd (and with your patch the
   transport_remove_cmd_from_queue) call optional.

I'd much rather add a conditional on the command beeing a TMR and merge
the two into a single helper than having them diverge further.

Re: [PATCH 3/4] target: Fix transport_cmd_finish_abort queue removal bug

[Nicholas A. Bellinger]

On Sat, 2011-10-08 at 22:27 -0400, Christoph Hellwig wrote:
> On Sat, Oct 08, 2011 at 07:03:32PM -0700, Nicholas A. Bellinger wrote:
> > The reordering in transport_cmd_finish_abort() was not the issue here,
> > but the calling transport_remove_cmd_from_queue() when 'remove == 0'.
> >
> > This should not be a problem in transport_cmd_finish_abort_tmr() as we
> > always expect to release the tmr associated command.
> 
> I know, but I'd really not have subtily different calling conventions
> between the two.
> 
> Right now transport_cmd_finish_abort and transport_cmd_finish_abort_tmr
> only differ in:
> 
>  - transport_cmd_finish_abort calling transport_lun_remove_cmd
>  - transport_cmd_finish_abort having the remove flag to make the
>    transport_put_cmd (and with your patch the
>    transport_remove_cmd_from_queue) call optional.
> 
> I'd much rather add a conditional on the command beeing a TMR and merge
> the two into a single helper than having them diverge further.

Fair enough..  Merging the two with the following patch.

commit c2b6a8188ccece6ac225e8e8a72f91bcb7dc295a
Author: Nicholas Bellinger <nab@linux-iscsi.org>
Date:   Sun Oct 9 02:02:51 2011 -0700

    target: Merge transport_cmd_finish_abort_tmr into transport_cmd_finish_abort
    
    This patch merges transport_cmd_finish_abort_tmr() logic into a single
    transport_cmd_finish_abort() function by adding a cmd->se_tmr_req check
    around transport_lun_remove_cmd(), and updates the single caller within
    core_tmr_drain_tmr_list().
    
    Reported-by: Christoph Hellwig <hch@lst.de>
    Cc: Christoph Hellwig <hch@lst.de>
    Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 527e96b..79e26a4 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -162,7 +162,7 @@ static void core_tmr_drain_tmr_list(
                        (preempt_and_abort_list) ? "Preempt" : "", tmr,
                        tmr->function, tmr->response, cmd->t_state);
                
-               transport_cmd_finish_abort_tmr(cmd);
+               transport_cmd_finish_abort(cmd, 1);
        }
 }
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 73856dc..db41eb9 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -595,7 +595,8 @@ check_lun:
 
 void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
 {
-       transport_lun_remove_cmd(cmd);
+       if (!cmd->se_tmr_req)
+               transport_lun_remove_cmd(cmd);
 
        if (transport_cmd_check_stop_to_fabric(cmd))
                return;
@@ -605,16 +606,6 @@ void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
        }
 }
 
-void transport_cmd_finish_abort_tmr(struct se_cmd *cmd)
-{
-       transport_remove_cmd_from_queue(cmd, &cmd->se_dev->dev_queue_obj);
-
-       if (transport_cmd_check_stop_to_fabric(cmd))
-               return;
-
-       transport_put_cmd(cmd);
-}
-
 static void transport_add_cmd_to_queue(
        struct se_cmd *cmd,
        int t_state)
diff --git a/include/target/target_core_transport.h b/include/target/target_core_transport.h
index 7f41fb2..065fb65 100644
--- a/include/target/target_core_transport.h
+++ b/include/target/target_core_transport.h
@@ -134,7 +134,6 @@ extern void transport_free_session(struct se_session *);
 extern void transport_deregister_session_configfs(struct se_session *);
 extern void transport_deregister_session(struct se_session *);
 extern void transport_cmd_finish_abort(struct se_cmd *, int);
-extern void transport_cmd_finish_abort_tmr(struct se_cmd *);
 extern void transport_complete_sync_cache(struct se_cmd *, int);
 extern void transport_complete_task(struct se_task *, int);
 extern void transport_add_task_to_execute_queue(struct se_task *,