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

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

[Nicholas A. Bellinger]

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

Hi folks,

This is -v3 series to address three 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, it adds __target_check_io_state()
common handler for ABORT_TASK + LUN_RESET I/O abort
cases, and moves remaining se_cmd SGL page + release
into target_free_cmd_mem() to now be called directly
from the final target_release_cmd_kref() callback.

New for -v3, patch #4 is to address the case where
se_cmd driver level shutdown occurs while TMR
CMD_T_ABORTED is already happening.  It introduces
CMD_T_FABRIC_STOP to replace CMD_T_REQUEST_STOP,
and use existing ->cmd_wait_set + ->cmd->wait_comp
to handle this special case for existing drivers.

Note that #4 + #5 needs more testing, so likely
patch #1 + #3 will be pushed in the next -rc to
address the primary case, ahead of the other to
fix the special case and drop legacy left-overs.

Please review.

--nab

v3 changes:

- Drop unncessary braces in core_tmr_handle_tas_abort
- Drop misleading printk() during __target_check_io_state() failure
- Avoid calling target_remove_from_state_list() from
  transport_generic_free_cmd with cmd->t_state_lock held.
- Make TMR ABORTED use se_cmd->cmd_wait_set + ->cmd_wait_comp
  during shutdown.
- Take ->cmd_kref during target_sess_cmd_list_set_waiting(),
  and drop in target_wait_for_sess_cmds()
- Pass 'fabric_stop' to transport_wait_for_tasks(), and make
  transport_generic_free_cmd() aware of aborted + tas status
  bits.
- Drop left-over ->task_stop_comp + CMD_T_REQUEST_STOP

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: Drop legacy se_cmd->task_stop_comp + REQUEST_STOP usage

 drivers/target/iscsi/iscsi_target_erl2.c |   3 +-
 drivers/target/target_core_internal.h    |   1 -
 drivers/target/target_core_tmr.c         | 137 ++++++++++++++++++-----
 drivers/target/target_core_transport.c   | 186 +++++++++++++++++--------------
 include/target/target_core_base.h        |   7 +-
 include/target/target_core_fabric.h      |   2 +-
 6 files changed, 220 insertions(+), 116 deletions(-)

-- 
1.9.1

[PATCH-v3 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       | 64 +++++++++++++++++++++++---------
 drivers/target/target_core_transport.c | 67 +++++++++++++++-------------------
 include/target/target_core_base.h      |  1 +
 3 files changed, 76 insertions(+), 56 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index fcdcb11..2248075 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(&se_session->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,
@@ -136,28 +164,21 @@ void core_tmr_abort_task(
 		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 +263,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 +305,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 +322,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 +346,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

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

[Christoph Hellwig]

> @@ -282,6 +305,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;
> +

I still don't understand why we care about the session or sess_cmd_lock
here.  There isn't anything in __target_check_io_state that relies
on them (except for the assert checking the lock).

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

[Nicholas A. Bellinger]

On Tue, 2016-02-02 at 10:37 +0100, Christoph Hellwig wrote:
> > @@ -282,6 +305,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;
> > +
> 
> I still don't understand why we care about the session or sess_cmd_lock
> here.  There isn't anything in __target_check_io_state that relies
> on them (except for the assert checking the lock).

Like I said earlier:

"I'm still working on -v3 series to handle se_session shutdown during
this specific multi-port LUN_RESET case, and considering using the
existing se_cmd->cmd_wait_set bit to signal when this special case
happens.

Currently ->sess_cmd_lock is held in target_release_cmd_kref() and
target_sess_cmd_list_set_waiting() while checking and setting this
value."

[PATCH-v3 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 2248075..c75ef29 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);
 	}
 
@@ -199,9 +199,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..
@@ -227,17 +229,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);
@@ -251,7 +267,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

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

[Christoph Hellwig]

> +		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);

Same here - what is the point of sess_cmd_lock?

[PATCH-v3 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 | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index c75ef29..9d67d16 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);
 	}
@@ -111,7 +111,7 @@ static bool __target_check_io_state(struct se_cmd *se_cmd)
 {
 	struct se_session *sess = se_cmd->se_sess;
 
-	assert_spin_locked(&se_session->sess_cmd_lock);
+	assert_spin_locked(&sess->sess_cmd_lock);
 	WARN_ON_ONCE(!irqs_disabled());
 	/*
 	 * If command already reached CMD_T_COMPLETE state within
@@ -278,7 +278,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)
 {
@@ -369,7 +369,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);
 	}
 }
@@ -382,6 +382,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
@@ -400,8 +401,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",
@@ -414,7 +416,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

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

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@daterainc.com>

To address a bug where se_cmd fabric driver level shutdown
occurs while TMR CMD_T_ABORTED is happening, resulting in
potential -1 ->cmd_kref, this patch adds target_tmr_put_cmd()
wrapper and CMD_T_FABRIC_STOP bit used to determine when
TMR + front-end driver I_T nexus shutdown is occuring.

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() + invoke extra
target_put_sess_cmd() during Task Aborted Status (TAS)
when necessary.

Also, allow transport_wait_for_tasks() callers to set
CMD_T_FABRIC_STOP, and aware of CMD_T_ABORTED + CMD_T_TAS
status bits.

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/iscsi/iscsi_target_erl2.c |  3 +-
 drivers/target/target_core_tmr.c         | 49 +++++++++++++++++++----
 drivers/target/target_core_transport.c   | 69 +++++++++++++++++++++++++++-----
 include/target/target_core_base.h        |  2 +
 include/target/target_core_fabric.h      |  2 +-
 5 files changed, 106 insertions(+), 19 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_erl2.c b/drivers/target/iscsi/iscsi_target_erl2.c
index e24f1c7..1e79acd 100644
--- a/drivers/target/iscsi/iscsi_target_erl2.c
+++ b/drivers/target/iscsi/iscsi_target_erl2.c
@@ -316,6 +316,7 @@ int iscsit_prepare_cmds_for_realligance(struct iscsi_conn *conn)
 	u32 cmd_count = 0;
 	struct iscsi_cmd *cmd, *cmd_tmp;
 	struct iscsi_conn_recovery *cr;
+	bool aborted = false, tas = false;
 
 	/*
 	 * Allocate an struct iscsi_conn_recovery for this connection.
@@ -398,7 +399,7 @@ int iscsit_prepare_cmds_for_realligance(struct iscsi_conn *conn)
 
 		iscsit_free_all_datain_reqs(cmd);
 
-		transport_wait_for_tasks(&cmd->se_cmd);
+		transport_wait_for_tasks(&cmd->se_cmd, false, &aborted, &tas);
 		/*
 		 * Add the struct iscsi_cmd to the connection recovery cmd list
 		 */
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 9d67d16..3f86f22 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -110,6 +110,7 @@ static int target_check_cdb_and_preempt(struct list_head *list,
 static bool __target_check_io_state(struct se_cmd *se_cmd)
 {
 	struct se_session *sess = se_cmd->se_sess;
+	bool ret;
 
 	assert_spin_locked(&sess->sess_cmd_lock);
 	WARN_ON_ONCE(!irqs_disabled());
@@ -129,10 +130,36 @@ static bool __target_check_io_state(struct se_cmd *se_cmd)
 		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;
 	spin_unlock(&se_cmd->t_state_lock);
 
-	return kref_get_unless_zero(&se_cmd->cmd_kref);
+	ret = kref_get_unless_zero(&se_cmd->cmd_kref);
+	if (ret)
+		se_cmd->cmd_wait_set = 1;
+	return ret;
+}
+
+static void target_tmr_put_cmd(struct se_cmd *se_cmd)
+{
+	unsigned long flags;
+	bool fabric_stop;
+
+	spin_lock_irqsave(&se_cmd->t_state_lock, flags);
+	fabric_stop = (se_cmd->transport_state & CMD_T_FABRIC_STOP);
+	spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
+
+	target_put_sess_cmd(se_cmd);
+
+	if (!fabric_stop) {
+		wait_for_completion(&se_cmd->cmd_wait_comp);
+		se_cmd->se_tfo->release_cmd(se_cmd);
+	}
 }
 
 void core_tmr_abort_task(
@@ -143,6 +170,7 @@ void core_tmr_abort_task(
 	struct se_cmd *se_cmd;
 	unsigned long flags;
 	u64 ref_tag;
+	bool aborted = false, tas = false;
 
 	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
 	list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) {
@@ -175,10 +203,10 @@ void core_tmr_abort_task(
 		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 
 		cancel_work_sync(&se_cmd->work);
-		transport_wait_for_tasks(se_cmd);
+		transport_wait_for_tasks(se_cmd, false, &aborted, &tas);
 
 		transport_cmd_finish_abort(se_cmd, true);
-		target_put_sess_cmd(se_cmd);
+		target_tmr_put_cmd(se_cmd);
 
 		printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for"
 				" ref_tag: %llu\n", ref_tag);
@@ -203,7 +231,7 @@ static void core_tmr_drain_tmr_list(
 	struct se_tmr_req *tmr_p, *tmr_pp;
 	struct se_cmd *cmd;
 	unsigned long flags;
-	bool rc;
+	bool rc, aborted = false, tas = false;
 	/*
 	 * Release all pending and outgoing TMRs aside from the received
 	 * LUN_RESET tmr..
@@ -252,8 +280,12 @@ static void core_tmr_drain_tmr_list(
 		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;
 		}
+		cmd->cmd_wait_set = true;
+		spin_unlock(&sess->sess_cmd_lock);
+
 		list_move_tail(&tmr_p->tmr_list, &drain_tmr_list);
 	}
 	spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
@@ -268,10 +300,10 @@ static void core_tmr_drain_tmr_list(
 			tmr_p->function, tmr_p->response, cmd->t_state);
 
 		cancel_work_sync(&cmd->work);
-		transport_wait_for_tasks(cmd);
+		transport_wait_for_tasks(cmd, false, &aborted, &tas);
 
 		transport_cmd_finish_abort(cmd, 1);
-		target_put_sess_cmd(cmd);
+		target_tmr_put_cmd(cmd);
 	}
 }
 
@@ -287,6 +319,7 @@ static void core_tmr_drain_state_list(
 	struct se_cmd *cmd, *next;
 	unsigned long flags;
 	int rc;
+	bool aborted = false, tas_set = false;
 
 	/*
 	 * Complete outstanding commands with TASK_ABORTED SAM status.
@@ -367,10 +400,10 @@ static void core_tmr_drain_state_list(
 		 * cancel_work_sync may block.
 		 */
 		cancel_work_sync(&cmd->work);
-		transport_wait_for_tasks(cmd);
+		transport_wait_for_tasks(cmd, false, &aborted, &tas_set);
 
 		core_tmr_handle_tas_abort(tmr_sess, cmd, tas);
-		target_put_sess_cmd(cmd);
+		target_tmr_put_cmd(cmd);
 	}
 }
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 94e372a..36a70e0 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2434,15 +2434,17 @@ static void transport_write_pending_qf(struct se_cmd *cmd)
 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);
+			transport_wait_for_tasks(cmd, true, &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);
+			transport_wait_for_tasks(cmd, true, &aborted, &tas);
 		/*
 		 * Handle WRITE failure case where transport_generic_new_cmd()
 		 * has already added se_cmd to state_list, but fabric has
@@ -2454,9 +2456,21 @@ 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);
 	}
-	return ret;
+	/*
+	 * 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);
+	}
+	return (aborted) ? 1 : ret;
 }
 EXPORT_SYMBOL(transport_generic_free_cmd);
 
@@ -2517,7 +2531,7 @@ 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) {
+	if (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);
@@ -2555,6 +2569,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 +2579,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,6 +2600,7 @@ 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) {
@@ -2587,6 +2610,15 @@ void target_wait_for_sess_cmds(struct se_session *se_sess)
 			" %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,
@@ -2611,15 +2643,33 @@ void transport_clear_lun_ref(struct se_lun *lun)
 /**
  * transport_wait_for_tasks - wait for completion to occur
  * @cmd:	command to wait
+ * @fabric_stop: set if command is being stopped + shutdown from
+ * 		 fabric driver
+ * @aborted:	set if command has been internally aborted
+ * @tas:	set if command is has task aborted status
  *
  * 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)
+bool transport_wait_for_tasks(struct se_cmd *cmd, bool fabric_stop,
+			      bool *aborted, bool *tas)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	if (fabric_stop)
+		cmd->transport_state |= CMD_T_FABRIC_STOP;
+
+	if (cmd->transport_state & CMD_T_ABORTED)
+		*aborted = true;
+	else
+		*aborted = false;
+
+	if (cmd->transport_state & CMD_T_TAS)
+		*tas = true;
+	else
+		*tas = false;
+
 	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);
@@ -2869,6 +2919,7 @@ void transport_send_task_abort(struct se_cmd *cmd)
 		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 		return;
 	}
+	cmd->transport_state |= CMD_T_TAS;
 	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 
 	/*
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;
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 5665340..240ea10 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -148,7 +148,7 @@ void	target_execute_cmd(struct se_cmd *cmd);
 
 int	transport_generic_free_cmd(struct se_cmd *, int);
 
-bool	transport_wait_for_tasks(struct se_cmd *);
+bool	transport_wait_for_tasks(struct se_cmd *, bool, bool *, bool *);
 int	transport_check_aborted_status(struct se_cmd *, int);
 int	transport_send_check_condition_and_sense(struct se_cmd *,
 		sense_reason_t, int);
-- 
1.9.1

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

[Christoph Hellwig]

> +	bool aborted = false, tas = false;
>  
>  	/*
>  	 * Allocate an struct iscsi_conn_recovery for this connection.
> @@ -398,7 +399,7 @@ int iscsit_prepare_cmds_for_realligance(struct iscsi_conn *conn)
>  
>  		iscsit_free_all_datain_reqs(cmd);
>  
> -		transport_wait_for_tasks(&cmd->se_cmd);
> +		transport_wait_for_tasks(&cmd->se_cmd, false, &aborted, &tas);

please keep the existing transport_wait_for_tasks prototype and factor
out a new helper for the transport_generic_free_cmd special case to avoid
all this boiler plate.

> +	ret = kref_get_unless_zero(&se_cmd->cmd_kref);
> +	if (ret)
> +		se_cmd->cmd_wait_set = 1;

I don't like the dual use of cmd_wait_set and the combination
with CMD_T_FABRIC_STOP.

How about the following alternative:

pull in my prep patch to rewrite target_sess_cmd_list_set_waiting so
that it doesn't need to iterate over all commands and set cmd_wait_set:

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

This gives us a free hand use a different completion for per-command
waiting.  Now your new usage of cmd_wait_set can be simplified as we
don't need a CMD_T_FABRIC_STOP to undo it, it can simply be cleared.
While we're at it I'd suggest using a CMD_T_ flag instead of a bitfield
for it.

>  	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, true, &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);
> +			transport_wait_for_tasks(cmd, true, &aborted, &tas);
>  		/*
>  		 * Handle WRITE failure case where transport_generic_new_cmd()
>  		 * has already added se_cmd to state_list, but fabric has
> @@ -2454,9 +2456,21 @@ 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);
>  	}

FYI, this can be simplified a bit more.

Call transport_wait_for_tasks

	if (wait_for_tasks &&
	    (cmd->se_cmd_flags & (SCF_SE_LUN_CMD | SCF_SCSI_TMR_CDB))) {

and then the

	if (!aborted || tas)
		et = transport_put_cmd(cmd);

can be moved behind the conditional for LUN_CMDs as well.

> -	return ret;
> +	/*
> +	 * 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);
> +	}
> +	return (aborted) ? 1 : ret;

This could be simplified to:

	if (aborted) {
		...

		ret = 1;
	}

	return ret;

> +	if (cmd->transport_state & CMD_T_ABORTED)
> +		*aborted = true;
> +	else
> +		*aborted = false;
> +
> +	if (cmd->transport_state & CMD_T_TAS)
> +		*tas = true;
> +	else
> +		*tas = false;

All callers already initialize these two to false, so the else
branches seem superflous.

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

[Nicholas A. Bellinger]

On Tue, 2016-02-02 at 11:54 +0100, Christoph Hellwig wrote:
> > +	bool aborted = false, tas = false;
> >  
> >  	/*
> >  	 * Allocate an struct iscsi_conn_recovery for this connection.
> > @@ -398,7 +399,7 @@ int iscsit_prepare_cmds_for_realligance(struct iscsi_conn *conn)
> >  
> >  		iscsit_free_all_datain_reqs(cmd);
> >  
> > -		transport_wait_for_tasks(&cmd->se_cmd);
> > +		transport_wait_for_tasks(&cmd->se_cmd, false, &aborted, &tas);
> 
> please keep the existing transport_wait_for_tasks prototype and factor
> out a new helper for the transport_generic_free_cmd special case to avoid
> all this boiler plate.

Seems reasonable enough.

Moved existing code to __transport_wait_for_tasks() minus
taking/dropping the lock on entry/exit so a transport_wait_for_tasks()
wrapper works for existing callers, plus adding a new
target_wait_free_cmd() for transport_generic_free_cmd() special case.

> 
> > +	ret = kref_get_unless_zero(&se_cmd->cmd_kref);
> > +	if (ret)
> > +		se_cmd->cmd_wait_set = 1;
> 
> I don't like the dual use of cmd_wait_set and the combination
> with CMD_T_FABRIC_STOP.
> 

The dual use is required because there needs to be a mechanism for the
TMR path to release the command, but at the same time must also be aware
when the front-end driver needs to shutdown the command (from it's
calling context) at the same time, due to session reset.

For the session reset case, the key aspect is the fabric driver calling
target_wait_for_sess_cmds() or transport_generic_free_cmd() MUST NOT
return until se_cmd->se_tfo->release_cmd() has completed.

This is why the caller who sets ->cmd_wait_set is responsible for
calling ->release_cmd(), and not target_release_cmd_kref() callback
itself.

> How about the following alternative:
> 
> pull in my prep patch to rewrite target_sess_cmd_list_set_waiting so
> that it doesn't need to iterate over all commands and set cmd_wait_set:
> 
> 	http://www.spinics.net/lists/target-devel/msg11446.html
> 

Your patch has one key assumption incorrect.

That is, it's not safe for target_wait_for_sess_cmds() to return until
all se_cmd->se_tfo->release_cmd() callbacks have been invoked.  This is
because current code assumes that once this function completes, all
possible se_cmd related dereferences to se_session are finished.

The patch mentioned above ignores this requirement, and allows
target_wait_for_sess_cmds() to return before all ->release_cmd() calls
finish, while target_release_cmd_kref() completes in the back-ground
resulting in a sure-fire use-after-free.

> This gives us a free hand use a different completion for per-command
> waiting.  Now your new usage of cmd_wait_set can be simplified as we
> don't need a CMD_T_FABRIC_STOP to undo it, it can simply be cleared.
> While we're at it I'd suggest using a CMD_T_ flag instead of a bitfield
> for it.
> 
> >  	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, true, &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);
> > +			transport_wait_for_tasks(cmd, true, &aborted, &tas);
> >  		/*
> >  		 * Handle WRITE failure case where transport_generic_new_cmd()
> >  		 * has already added se_cmd to state_list, but fabric has
> > @@ -2454,9 +2456,21 @@ 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);
> >  	}
> 
> FYI, this can be simplified a bit more.
> 
> Call transport_wait_for_tasks
> 
> 	if (wait_for_tasks &&
> 	    (cmd->se_cmd_flags & (SCF_SE_LUN_CMD | SCF_SCSI_TMR_CDB))) {
> 
> and then the
> 
> 	if (!aborted || tas)
> 		et = transport_put_cmd(cmd);
> 
> can be moved behind the conditional for LUN_CMDs as well.
> 

That doesn't quite work, because target_remove_from_state_list() +
transport_lun_remove_cmd() must be called before the potentially final
transport_put_cmd() -> target_put_sess_cmd() callback.

Also, SCF_SCSI_TMR_CDB is still slightly different in this regard, as it
doesn't take se_lun->lun_ref atm.

That said, I'm in agreement that this can be cleaned up further, but
given this patch will need to back-ported stable, I'd prefer to keep
additional cleanups as separate post bug-fix improvements.

> > -	return ret;
> > +	/*
> > +	 * 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);
> > +	}
> > +	return (aborted) ? 1 : ret;
> 
> This could be simplified to:
> 
> 	if (aborted) {
> 		...
> 
> 		ret = 1;
> 	}
> 
> 	return ret;
> 

Done.

> > +	if (cmd->transport_state & CMD_T_ABORTED)
> > +		*aborted = true;
> > +	else
> > +		*aborted = false;
> > +
> > +	if (cmd->transport_state & CMD_T_TAS)
> > +		*tas = true;
> > +	else
> > +		*tas = false;
> 
> All callers already initialize these two to false, so the else
> branches seem superflous.

Dropped.

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

[Nicholas A. Bellinger]

Btw, here's the updated patch:

>From 731b9981fc49d9b11e93b5c2f0a56e27862b4335 Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <nab@daterainc.com>
Date: Tue, 19 Jan 2016 15:23:02 -0800
Subject: [PATCH] target: Fix remote-port TMR ABORT + se_cmd fabric stop

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 target_tmr_put_cmd() wrapper + 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       |  39 +++++++++-
 drivers/target/target_core_transport.c | 132 ++++++++++++++++++++++++---------
 include/target/target_core_base.h      |   2 +
 3 files changed, 136 insertions(+), 37 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 9d67d16..2ac3228 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -110,6 +110,7 @@ static int target_check_cdb_and_preempt(struct list_head *list,
 static bool __target_check_io_state(struct se_cmd *se_cmd)
 {
 	struct se_session *sess = se_cmd->se_sess;
+	bool ret;
 
 	assert_spin_locked(&sess->sess_cmd_lock);
 	WARN_ON_ONCE(!irqs_disabled());
@@ -129,10 +130,36 @@ static bool __target_check_io_state(struct se_cmd *se_cmd)
 		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;
 	spin_unlock(&se_cmd->t_state_lock);
 
-	return kref_get_unless_zero(&se_cmd->cmd_kref);
+	ret = kref_get_unless_zero(&se_cmd->cmd_kref);
+	if (ret)
+		se_cmd->cmd_wait_set = 1;
+	return ret;
+}
+
+static void target_tmr_put_cmd(struct se_cmd *se_cmd)
+{
+	unsigned long flags;
+	bool fabric_stop;
+
+	spin_lock_irqsave(&se_cmd->t_state_lock, flags);
+	fabric_stop = (se_cmd->transport_state & CMD_T_FABRIC_STOP);
+	spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
+
+	target_put_sess_cmd(se_cmd);
+
+	if (!fabric_stop) {
+		wait_for_completion(&se_cmd->cmd_wait_comp);
+		se_cmd->se_tfo->release_cmd(se_cmd);
+	}
 }
 
 void core_tmr_abort_task(
@@ -178,7 +205,7 @@ void core_tmr_abort_task(
 		transport_wait_for_tasks(se_cmd);
 
 		transport_cmd_finish_abort(se_cmd, true);
-		target_put_sess_cmd(se_cmd);
+		target_tmr_put_cmd(se_cmd);
 
 		printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for"
 				" ref_tag: %llu\n", ref_tag);
@@ -252,8 +279,12 @@ static void core_tmr_drain_tmr_list(
 		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;
 		}
+		cmd->cmd_wait_set = true;
+		spin_unlock(&sess->sess_cmd_lock);
+
 		list_move_tail(&tmr_p->tmr_list, &drain_tmr_list);
 	}
 	spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
@@ -271,7 +302,7 @@ static void core_tmr_drain_tmr_list(
 		transport_wait_for_tasks(cmd);
 
 		transport_cmd_finish_abort(cmd, 1);
-		target_put_sess_cmd(cmd);
+		target_tmr_put_cmd(cmd);
 	}
 }
 
@@ -370,7 +401,7 @@ static void core_tmr_drain_state_list(
 		transport_wait_for_tasks(cmd);
 
 		core_tmr_handle_tas_abort(tmr_sess, cmd, tas);
-		target_put_sess_cmd(cmd);
+		target_tmr_put_cmd(cmd);
 	}
 }
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 94e372a..a0c6e2e 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;
 }
@@ -2517,7 +2545,7 @@ 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) {
+	if (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);
@@ -2555,6 +2583,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 +2593,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,6 +2614,7 @@ 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) {
@@ -2587,6 +2624,15 @@ void target_wait_for_sess_cmds(struct se_session *se_sess)
 			" %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 +2654,72 @@ 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;
-	}
 
 	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);
 
@@ -2869,6 +2934,7 @@ void transport_send_task_abort(struct se_cmd *cmd)
 		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 		return;
 	}
+	cmd->transport_state |= CMD_T_TAS;
 	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 
 	/*
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;

[PATCH-v3 5/5] target: Drop legacy se_cmd->task_stop_comp + REQUEST_STOP usage

[Nicholas A. Bellinger]

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

With CMD_T_FABRIC_STOP + se_cmd->cmd_wait_set usage in place,
go ahead and drop left-over CMD_T_REQUEST_STOP checks in
target_complete_cmd() and unused target_stop_cmd().

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>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_internal.h  |  1 -
 drivers/target/target_core_transport.c | 37 ----------------------------------
 include/target/target_core_base.h      |  4 ----
 3 files changed, 42 deletions(-)

diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index dae0750c..db4412f 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -141,7 +141,6 @@ void	transport_dump_vpd_proto_id(struct t10_vpd *, unsigned char *, int);
 int	transport_dump_vpd_assoc(struct t10_vpd *, unsigned char *, int);
 int	transport_dump_vpd_ident_type(struct t10_vpd *, unsigned char *, int);
 int	transport_dump_vpd_ident(struct t10_vpd *, unsigned char *, int);
-bool	target_stop_cmd(struct se_cmd *cmd, unsigned long *flags);
 void	transport_clear_lun_ref(struct se_lun *);
 void	transport_send_task_abort(struct se_cmd *);
 sense_reason_t	target_cmd_size_check(struct se_cmd *cmd, unsigned int size);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 36a70e0..463838d 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -693,15 +693,6 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
 	}
 
 	/*
-	 * See if we are waiting to complete for an exception condition.
-	 */
-	if (cmd->transport_state & CMD_T_REQUEST_STOP) {
-		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-		complete(&cmd->task_stop_comp);
-		return;
-	}
-
-	/*
 	 * Check for case where an explicit ABORT_TASK has been received
 	 * and transport_wait_for_tasks() will be waiting for completion..
 	 */
@@ -1202,7 +1193,6 @@ void transport_init_se_cmd(
 	INIT_LIST_HEAD(&cmd->state_list);
 	init_completion(&cmd->t_transport_stop_comp);
 	init_completion(&cmd->cmd_wait_comp);
-	init_completion(&cmd->task_stop_comp);
 	spin_lock_init(&cmd->t_state_lock);
 	kref_init(&cmd->cmd_kref);
 	cmd->transport_state = CMD_T_DEV_ACTIVE;
@@ -1634,33 +1624,6 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
 EXPORT_SYMBOL(target_submit_tmr);
 
 /*
- * If the cmd is active, request it to be stopped and sleep until it
- * has completed.
- */
-bool target_stop_cmd(struct se_cmd *cmd, unsigned long *flags)
-	__releases(&cmd->t_state_lock)
-	__acquires(&cmd->t_state_lock)
-{
-	bool was_active = false;
-
-	if (cmd->transport_state & CMD_T_BUSY) {
-		cmd->transport_state |= CMD_T_REQUEST_STOP;
-		spin_unlock_irqrestore(&cmd->t_state_lock, *flags);
-
-		pr_debug("cmd %p waiting to complete\n", cmd);
-		wait_for_completion(&cmd->task_stop_comp);
-		pr_debug("cmd %p stopped successfully\n", cmd);
-
-		spin_lock_irqsave(&cmd->t_state_lock, *flags);
-		cmd->transport_state &= ~CMD_T_REQUEST_STOP;
-		cmd->transport_state &= ~CMD_T_BUSY;
-		was_active = true;
-	}
-
-	return was_active;
-}
-
-/*
  * Handle SAM-esque emulation for generic transport request failures.
  */
 void transport_generic_request_failure(struct se_cmd *cmd,
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 1579539e..d71a3ea 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -491,7 +491,6 @@ struct se_cmd {
 #define CMD_T_SENT		(1 << 4)
 #define CMD_T_STOP		(1 << 5)
 #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)
@@ -514,9 +513,6 @@ struct se_cmd {
 
 	struct list_head	state_list;
 
-	/* old task stop completion, consider merging with some of the above */
-	struct completion	task_stop_comp;
-
 	/* backend private data */
 	void			*priv;
 
-- 
1.9.1

Re: [PATCH-v3 5/5] target: Drop legacy se_cmd->task_stop_comp + REQUEST_STOP usage

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>