[PATCH 0/4] Make aborts work on tcm_qla2xxx, other cleanups

[PATCH 0/4] Make aborts work on tcm_qla2xxx, other cleanups

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

Hi Nic,

Here's a series that makes aborts actually work on qla2xxx.  Stopping
and releasing commands is quite convoluted so I'm not sure the first
patch is totally correct, but without it I can easily reproduce task
hangs or list corruption by having an initiator flood a tcm_qla2xxx
target with aborts.  With those fixes, Steve's patch is pretty
straightforward.

The last two patches are just cleanups I noticed while debugging this.

Just to be clear: to the extent that this is copyrightable work, it is
released exclusively under the GPL.  No permission is granted to
redistribute this under any other terms.

 - R.

Roland Dreier (3):
  target: Fix handling of aborted commands
  target: Clean up logic in transport_put_cmd()
  target: Clean up flow in transport_check_aborted_status()

Steve Hodgson (1):
  qla2xxx: Look up LUN for abort requests

 drivers/scsi/qla2xxx/qla_target.c      |   19 ++++++++++++++-
 drivers/target/target_core_transport.c |   40 ++++++++++++++------------------
 2 files changed, 36 insertions(+), 23 deletions(-)

-- 
1.7.10.4

[PATCH 1/4] target: Fix handling of aborted commands

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

- If we stop processing an already-aborted command in
  target_execute_cmd(), then we need to complete t_transport_stop_comp
  to wake up the the TMR handling thread, or else it will end up
  waiting forever.

- If we've a already sent an "aborted" status for a command in
  transport_check_aborted_status() then we should bail out of
  transport_send_task_abort() to avoid freeing the command twice.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/target/target_core_transport.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 9097155..dcecbfb 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1819,8 +1819,10 @@ void target_execute_cmd(struct se_cmd *cmd)
 	/*
 	 * If the received CDB has aleady been aborted stop processing it here.
 	 */
-	if (transport_check_aborted_status(cmd, 1))
+	if (transport_check_aborted_status(cmd, 1)) {
+		complete(&cmd->t_transport_stop_comp);
 		return;
+	}
 
 	/*
 	 * Determine if IOCTL context caller in requesting the stopping of this
@@ -3067,7 +3069,7 @@ 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) {
+	if (cmd->se_cmd_flags & (SCF_SENT_CHECK_CONDITION | SCF_SENT_DELAYED_TAS)) {
 		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 		return;
 	}
-- 
1.7.10.4

[PATCH 2/4] qla2xxx: Look up LUN for abort requests

[Roland Dreier]

From: Steve Hodgson <steve@purestorage.com>

Search through the list of pending commands on the session list to find
the command the initiator is actually aborting, so that we can pass the
correct LUN to the core TMR handling code.

Signed-off-by: Steve Hodgson <steve@purestorage.com>
Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/scsi/qla2xxx/qla_target.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 62aa558..79fbdd7 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -1264,9 +1264,26 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host *vha,
 	struct abts_recv_from_24xx *abts, struct qla_tgt_sess *sess)
 {
 	struct qla_hw_data *ha = vha->hw;
+	struct se_session *se_sess = sess->se_sess;
 	struct qla_tgt_mgmt_cmd *mcmd;
+	struct se_cmd *se_cmd;
+	u32 lun = 0;
 	int rc;
 
+	spin_lock(&se_sess->sess_cmd_lock);
+	list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) {
+		struct qla_tgt_cmd *cmd =
+			container_of(se_cmd, struct qla_tgt_cmd, se_cmd);
+		if (cmd->tag == abts->exchange_addr_to_abort) {
+			lun = cmd->unpacked_lun;
+			break;
+		}
+	}
+	spin_unlock(&se_sess->sess_cmd_lock);
+
+	if (!lun)
+		return -ENOENT;
+
 	ql_dbg(ql_dbg_tgt_mgt, vha, 0xf00f,
 	    "qla_target(%d): task abort (tag=%d)\n",
 	    vha->vp_idx, abts->exchange_addr_to_abort);
@@ -1283,7 +1300,7 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host *vha,
 	mcmd->sess = sess;
 	memcpy(&mcmd->orig_iocb.abts, abts, sizeof(mcmd->orig_iocb.abts));
 
-	rc = ha->tgt.tgt_ops->handle_tmr(mcmd, 0, TMR_ABORT_TASK,
+	rc = ha->tgt.tgt_ops->handle_tmr(mcmd, lun, TMR_ABORT_TASK,
 	    abts->exchange_addr_to_abort);
 	if (rc != 0) {
 		ql_dbg(ql_dbg_tgt_mgt, vha, 0xf052,
-- 
1.7.10.4

Re: [PATCH 2/4] qla2xxx: Look up LUN for abort requests

[Nicholas A. Bellinger]

On Fri, 2012-11-16 at 08:06 -0800, Roland Dreier wrote:
> From: Steve Hodgson <steve@purestorage.com>
> 
> Search through the list of pending commands on the session list to find
> the command the initiator is actually aborting, so that we can pass the
> correct LUN to the core TMR handling code.
> 
> Signed-off-by: Steve Hodgson <steve@purestorage.com>
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  drivers/scsi/qla2xxx/qla_target.c |   19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index 62aa558..79fbdd7 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -1264,9 +1264,26 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host *vha,
>  	struct abts_recv_from_24xx *abts, struct qla_tgt_sess *sess)
>  {
>  	struct qla_hw_data *ha = vha->hw;
> +	struct se_session *se_sess = sess->se_sess;
>  	struct qla_tgt_mgmt_cmd *mcmd;
> +	struct se_cmd *se_cmd;
> +	u32 lun = 0;
>  	int rc;
>  
> +	spin_lock(&se_sess->sess_cmd_lock);
> +	list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) {
> +		struct qla_tgt_cmd *cmd =
> +			container_of(se_cmd, struct qla_tgt_cmd, se_cmd);
> +		if (cmd->tag == abts->exchange_addr_to_abort) {
> +			lun = cmd->unpacked_lun;
> +			break;
> +		}
> +	}
> +	spin_unlock(&se_sess->sess_cmd_lock);
> +
> +	if (!lun)
> +		return -ENOENT;
> +

So I'm sure this patch works fine against a private target tree that
uses a hack to always use virtual LUN=0 for management purposes, but
that's not what mainline target does now, does it..?

Fixing this up now with the following patch + squashing into the
original.  Also, please CC' the Qlogic folks in the future for these
types of patches.

Thanks,

--nab

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 79fbdd7..661d33e 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -1269,6 +1269,7 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host *vha,
        struct se_cmd *se_cmd;
        u32 lun = 0;
        int rc;
+       bool found_lun = false;
 
        spin_lock(&se_sess->sess_cmd_lock);
        list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) {
@@ -1276,12 +1277,13 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host *vha,
                        container_of(se_cmd, struct qla_tgt_cmd, se_cmd);
                if (cmd->tag == abts->exchange_addr_to_abort) {
                        lun = cmd->unpacked_lun;
+                       found_lun = true;
                        break;
                }
        }
        spin_unlock(&se_sess->sess_cmd_lock);
 
-       if (!lun)
+       if (!found_lun)
                return -ENOENT;
 
        ql_dbg(ql_dbg_tgt_mgt, vha, 0xf00f,

[PATCH 3/4] target: Clean up logic in transport_put_cmd()

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

No need to have a goto where a return is clearer.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/target/target_core_transport.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index dcecbfb..0f29d70 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2183,9 +2183,10 @@ static void transport_put_cmd(struct se_cmd *cmd)
 	unsigned long flags;
 
 	spin_lock_irqsave(&cmd->t_state_lock, flags);
-	if (atomic_read(&cmd->t_fe_count)) {
-		if (!atomic_dec_and_test(&cmd->t_fe_count))
-			goto out_busy;
+	if (atomic_read(&cmd->t_fe_count) &&
+	    !atomic_dec_and_test(&cmd->t_fe_count)) {
+		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+		return;
 	}
 
 	if (cmd->transport_state & CMD_T_DEV_ACTIVE) {
@@ -2196,9 +2197,6 @@ static void transport_put_cmd(struct se_cmd *cmd)
 
 	transport_free_pages(cmd);
 	transport_release_cmd(cmd);
-	return;
-out_busy:
-	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 }
 
 /*
-- 
1.7.10.4

[PATCH 4/4] target: Clean up flow in transport_check_aborted_status()

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/target/target_core_transport.c |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 0f29d70..f225f90 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -3042,23 +3042,19 @@ EXPORT_SYMBOL(transport_send_check_condition_and_sense);
 
 int transport_check_aborted_status(struct se_cmd *cmd, int send_status)
 {
-	int ret = 0;
+	if (!(cmd->transport_state & CMD_T_ABORTED))
+		return 0;
 
-	if (cmd->transport_state & CMD_T_ABORTED) {
-		if (!send_status ||
-		     (cmd->se_cmd_flags & SCF_SENT_DELAYED_TAS))
-			return 1;
+	if (!send_status || (cmd->se_cmd_flags & SCF_SENT_DELAYED_TAS))
+		return 1;
 
-		pr_debug("Sending delayed SAM_STAT_TASK_ABORTED"
-			" status for CDB: 0x%02x ITT: 0x%08x\n",
-			cmd->t_task_cdb[0],
-			cmd->se_tfo->get_task_tag(cmd));
+	pr_debug("Sending delayed SAM_STAT_TASK_ABORTED status for CDB: 0x%02x ITT: 0x%08x\n",
+		 cmd->t_task_cdb[0], cmd->se_tfo->get_task_tag(cmd));
 
-		cmd->se_cmd_flags |= SCF_SENT_DELAYED_TAS;
-		cmd->se_tfo->queue_status(cmd);
-		ret = 1;
-	}
-	return ret;
+	cmd->se_cmd_flags |= SCF_SENT_DELAYED_TAS;
+	cmd->se_tfo->queue_status(cmd);
+
+	return 1;
 }
 EXPORT_SYMBOL(transport_check_aborted_status);
 
-- 
1.7.10.4

Re: [PATCH 0/4] Make aborts work on tcm_qla2xxx, other cleanups

[Nicholas A. Bellinger]

On Fri, 2012-11-16 at 08:06 -0800, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> Hi Nic,
> 
> Here's a series that makes aborts actually work on qla2xxx.  Stopping
> and releasing commands is quite convoluted so I'm not sure the first
> patch is totally correct, but without it I can easily reproduce task
> hangs or list corruption by having an initiator flood a tcm_qla2xxx
> target with aborts.  With those fixes, Steve's patch is pretty
> straightforward.
> 

The first patch looks safe enough a bugfix to queue for v3.7.0.  Applied
to target-pending/master, with CC' to stable.

Replying to the second patch inline..

> The last two patches are just cleanups I noticed while debugging this.
> 

Applying #3 + #4 to for-next, thanks.

> Just to be clear: to the extent that this is copyrightable work, it is
> released exclusively under the GPL.  No permission is granted to
> redistribute this under any other terms.
> 

The GPL license for these files w/ my authorship in mainline is
perfectly clear.   This extra legalese in cover-letters for a few lines
of code only serves to be annoying.

Please leave it off the target list in the future, Thanks.

--nab