[PATCH 0/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support

[PATCH 0/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support

[Nicholas A. Bellinger]

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

Hi Himanshu + Quinn,

Here is a small series to introduce proper percpu se_lun->lun_ref
counting for TMR, and add common code in target_submit_tmr() to
do tag lookup for unpacked_lun in order to drop the original
driver specific lookup within __qlt_24xx_handle_abts().

It's rather straight-forward, so review and test as a v4.13 item.

Thanks!

Nicholas Bellinger (3):
  target: Add support for TMR percpu reference counting
  target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK
  qla2xxx: Convert QLA_TGT_ABTS to TARGET_SCF_LOOKUP_LUN_FROM_TAG

 drivers/scsi/qla2xxx/qla_target.c      | 39 ++++++-----------------
 drivers/scsi/qla2xxx/tcm_qla2xxx.c     |  4 ++-
 drivers/target/target_core_device.c    | 14 ++++++---
 drivers/target/target_core_transport.c | 56 ++++++++++++++++++++++++++++------
 include/target/target_core_base.h      |  3 +-
 5 files changed, 71 insertions(+), 45 deletions(-)

-- 
1.9.1

[PATCH 1/3] target: Add support for TMR percpu reference counting

[Nicholas A. Bellinger]

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

This patch introduces TMR percpu reference counting using
se_lun->lun_ref in transport_lookup_tmr_lun(), following
how existing non TMR per se_lun reference counting works
within transport_lookup_cmd_lun().

It also adds explicit transport_lun_remove_cmd() calls to
drop the reference in the three tmr related locations that
invoke transport_cmd_check_stop_to_fabric();

   - target_tmr_work() during normal ->queue_tm_rsp()
   - target_complete_tmr_failure() during error ->queue_tm_rsp()
   - transport_generic_handle_tmr() during early failure

Also, note the exception paths in transport_generic_free_cmd()
and transport_cmd_finish_abort() already check SCF_SE_LUN_CMD,
and will invoke transport_lun_remove_cmd() when necessary.

Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
Cc: Quinn Tran <quinn.tran@cavium.com>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_device.c    | 14 ++++++++++----
 drivers/target/target_core_transport.c |  3 +++
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 8f0e0e3..11c80c4 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -168,11 +168,20 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
 	rcu_read_lock();
 	deve = target_nacl_find_deve(nacl, unpacked_lun);
 	if (deve) {
-		se_cmd->se_lun = rcu_dereference(deve->se_lun);
 		se_lun = rcu_dereference(deve->se_lun);
+
+		if (!percpu_ref_tryget_live(&se_lun->lun_ref)) {
+			se_lun = NULL;
+			goto out_unlock;
+		}
+
+		se_cmd->se_lun = rcu_dereference(deve->se_lun);
 		se_cmd->pr_res_key = deve->pr_res_key;
 		se_cmd->orig_fe_lun = unpacked_lun;
+		se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
+		se_cmd->lun_ref_active = true;
 	}
+out_unlock:
 	rcu_read_unlock();
 
 	if (!se_lun) {
@@ -182,9 +191,6 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
 			unpacked_lun);
 		return -ENODEV;
 	}
-	/*
-	 * XXX: Add percpu se_lun->lun_ref reference count for TMR
-	 */
 	se_cmd->se_dev = rcu_dereference_raw(se_lun->lun_se_dev);
 	se_tmr->tmr_dev = rcu_dereference_raw(se_lun->lun_se_dev);
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index f16a789..83bfc97 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1588,6 +1588,7 @@ static void target_complete_tmr_failure(struct work_struct *work)
 	se_cmd->se_tmr_req->response = TMR_LUN_DOES_NOT_EXIST;
 	se_cmd->se_tfo->queue_tm_rsp(se_cmd);
 
+	transport_lun_remove_cmd(se_cmd);
 	transport_cmd_check_stop_to_fabric(se_cmd);
 }
 
@@ -3199,6 +3200,7 @@ static void target_tmr_work(struct work_struct *work)
 	cmd->se_tfo->queue_tm_rsp(cmd);
 
 check_stop:
+	transport_lun_remove_cmd(cmd);
 	transport_cmd_check_stop_to_fabric(cmd);
 }
 
@@ -3221,6 +3223,7 @@ int transport_generic_handle_tmr(
 		pr_warn_ratelimited("handle_tmr caught CMD_T_ABORTED TMR %d"
 			"ref_tag: %llu tag: %llu\n", cmd->se_tmr_req->function,
 			cmd->se_tmr_req->ref_task_tag, cmd->tag);
+		transport_lun_remove_cmd(cmd);
 		transport_cmd_check_stop_to_fabric(cmd);
 		return 0;
 	}
-- 
1.9.1

Re: [PATCH 1/3] target: Add support for TMR percpu reference counting

[Tran, Quinn]

Looks good.

Regards,
Quinn Tran

-----Original Message-----
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Saturday, June 3, 2017 at 3:10 PM
To: target-devel <target-devel@vger.kernel.org>
Cc: linux-scsi <linux-scsi@vger.kernel.org>, lkml <linux-kernel@vger.kernel.org>, Nicholas Bellinger <nab@linux-iscsi.org>, "Madhani, Himanshu" <Himanshu.Madhani@cavium.com>, "Tran, Quinn" <Quinn.Tran@cavium.com>, Mike Christie <mchristi@redhat.com>, Hannes Reinecke <hare@suse.com>, Christoph Hellwig <hch@lst.de>
Subject: [PATCH 1/3] target: Add support for TMR percpu reference counting

    From: Nicholas Bellinger <nab@linux-iscsi.org>
    
    This patch introduces TMR percpu reference counting using
    se_lun->lun_ref in transport_lookup_tmr_lun(), following
    how existing non TMR per se_lun reference counting works
    within transport_lookup_cmd_lun().
    
    It also adds explicit transport_lun_remove_cmd() calls to
    drop the reference in the three tmr related locations that
    invoke transport_cmd_check_stop_to_fabric();
    
       - target_tmr_work() during normal ->queue_tm_rsp()
       - target_complete_tmr_failure() during error ->queue_tm_rsp()
       - transport_generic_handle_tmr() during early failure
    
    Also, note the exception paths in transport_generic_free_cmd()
    and transport_cmd_finish_abort() already check SCF_SE_LUN_CMD,
    and will invoke transport_lun_remove_cmd() when necessary.
    
    Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
    Cc: Quinn Tran <quinn.tran@cavium.com>
    Cc: Mike Christie <mchristi@redhat.com>
    Cc: Hannes Reinecke <hare@suse.com>
    Cc: Christoph Hellwig <hch@lst.de>
    Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
    ---
     drivers/target/target_core_device.c    | 14 ++++++++++----
     drivers/target/target_core_transport.c |  3 +++
     2 files changed, 13 insertions(+), 4 deletions(-)
    
    diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
    index 8f0e0e3..11c80c4 100644
    --- a/drivers/target/target_core_device.c
    +++ b/drivers/target/target_core_device.c
    @@ -168,11 +168,20 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
     	rcu_read_lock();
     	deve = target_nacl_find_deve(nacl, unpacked_lun);
     	if (deve) {
    -		se_cmd->se_lun = rcu_dereference(deve->se_lun);
     		se_lun = rcu_dereference(deve->se_lun);
    +
    +		if (!percpu_ref_tryget_live(&se_lun->lun_ref)) {
    +			se_lun = NULL;
    +			goto out_unlock;
    +		}
    +
    +		se_cmd->se_lun = rcu_dereference(deve->se_lun);
     		se_cmd->pr_res_key = deve->pr_res_key;
     		se_cmd->orig_fe_lun = unpacked_lun;
    +		se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
    +		se_cmd->lun_ref_active = true;
     	}
    +out_unlock:
     	rcu_read_unlock();
     
     	if (!se_lun) {
    @@ -182,9 +191,6 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
     			unpacked_lun);
     		return -ENODEV;
     	}
    -	/*
    -	 * XXX: Add percpu se_lun->lun_ref reference count for TMR
    -	 */
     	se_cmd->se_dev = rcu_dereference_raw(se_lun->lun_se_dev);
     	se_tmr->tmr_dev = rcu_dereference_raw(se_lun->lun_se_dev);
     
    diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
    index f16a789..83bfc97 100644
    --- a/drivers/target/target_core_transport.c
    +++ b/drivers/target/target_core_transport.c
    @@ -1588,6 +1588,7 @@ static void target_complete_tmr_failure(struct work_struct *work)
     	se_cmd->se_tmr_req->response = TMR_LUN_DOES_NOT_EXIST;
     	se_cmd->se_tfo->queue_tm_rsp(se_cmd);
     
    +	transport_lun_remove_cmd(se_cmd);
     	transport_cmd_check_stop_to_fabric(se_cmd);
     }
     
    @@ -3199,6 +3200,7 @@ static void target_tmr_work(struct work_struct *work)
     	cmd->se_tfo->queue_tm_rsp(cmd);
     
     check_stop:
    +	transport_lun_remove_cmd(cmd);
     	transport_cmd_check_stop_to_fabric(cmd);
     }
     
    @@ -3221,6 +3223,7 @@ int transport_generic_handle_tmr(
     		pr_warn_ratelimited("handle_tmr caught CMD_T_ABORTED TMR %d"
     			"ref_tag: %llu tag: %llu\n", cmd->se_tmr_req->function,
     			cmd->se_tmr_req->ref_task_tag, cmd->tag);
    +		transport_lun_remove_cmd(cmd);
     		transport_cmd_check_stop_to_fabric(cmd);
     		return 0;
     	}
    -- 
    1.9.1
    
    

[PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK

[Nicholas A. Bellinger]

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

This patch introduces support in target_submit_tmr() for locating a
unpacked_lun from an existing se_cmd->tag during ABORT_TASK.

When TARGET_SCF_LOOKUP_LUN_FROM_TAG is set, target_submit_tmr()
will do the extra lookup via target_lookup_lun_from_tag() and
subsequently invoke transport_lookup_tmr_lun() so a proper
percpu se_lun->lun_ref is taken before workqueue dispatch into
se_device->tmr_wq happens.

Aside from the extra target_lookup_lun_from_tag(), the existing
code-path remains unchanged.

Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
Cc: Quinn Tran <quinn.tran@cavium.com>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c | 53 ++++++++++++++++++++++++++++------
 include/target/target_core_base.h      |  3 +-
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 83bfc97..dbb8101 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1592,6 +1592,29 @@ static void target_complete_tmr_failure(struct work_struct *work)
 	transport_cmd_check_stop_to_fabric(se_cmd);
 }
 
+static bool target_lookup_lun_from_tag(struct se_session *se_sess, u64 tag,
+				       u64 *unpacked_lun)
+{
+	struct se_cmd *se_cmd;
+	unsigned long flags;
+	bool ret = false;
+
+	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
+	list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) {
+		if (se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
+			continue;
+
+		if (se_cmd->tag == tag) {
+			*unpacked_lun = se_cmd->orig_fe_lun;
+			ret = true;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+
+	return ret;
+}
+
 /**
  * target_submit_tmr - lookup unpacked lun and submit uninitialized se_cmd
  *                     for TMR CDBs
@@ -1639,19 +1662,31 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
 		core_tmr_release_req(se_cmd->se_tmr_req);
 		return ret;
 	}
+	/*
+	 * If this is ABORT_TASK with no explicit fabric provided LUN,
+	 * go ahead and search active session tags for a match to figure
+	 * out unpacked_lun for the original se_cmd.
+	 */
+	if (tm_type == TMR_ABORT_TASK && (flags & TARGET_SCF_LOOKUP_LUN_FROM_TAG)) {
+		if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun))
+			goto failure;
+	}
 
 	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
-	if (ret) {
-		/*
-		 * For callback during failure handling, push this work off
-		 * to process context with TMR_LUN_DOES_NOT_EXIST status.
-		 */
-		INIT_WORK(&se_cmd->work, target_complete_tmr_failure);
-		schedule_work(&se_cmd->work);
-		return 0;
-	}
+	if (ret)
+		goto failure;
+
 	transport_generic_handle_tmr(se_cmd);
 	return 0;
+
+	/*
+	 * For callback during failure handling, push this work off
+	 * to process context with TMR_LUN_DOES_NOT_EXIST status.
+	 */
+failure:
+	INIT_WORK(&se_cmd->work, target_complete_tmr_failure);
+	schedule_work(&se_cmd->work);
+	return 0;
 }
 EXPORT_SYMBOL(target_submit_tmr);
 
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index db2c7b3..a3af69f 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -188,7 +188,8 @@ enum target_sc_flags_table {
 	TARGET_SCF_BIDI_OP		= 0x01,
 	TARGET_SCF_ACK_KREF		= 0x02,
 	TARGET_SCF_UNKNOWN_SIZE		= 0x04,
-	TARGET_SCF_USE_CPUID	= 0x08,
+	TARGET_SCF_USE_CPUID		= 0x08,
+	TARGET_SCF_LOOKUP_LUN_FROM_TAG	= 0x10,
 };
 
 /* fabric independent task management function values */
-- 
1.9.1

Re: [PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK

[Bart Van Assche]

On Sat, 2017-06-03 at 22:10 +0000, Nicholas A. Bellinger wrote:
> +static bool target_lookup_lun_from_tag(struct se_session *se_sess, u64 tag,
> +				       u64 *unpacked_lun)
> +{
> +	struct se_cmd *se_cmd;
> +	unsigned long flags;
> +	bool ret = false;
> +
> +	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> +	list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) {
> +		if (se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
> +			continue;
> +
> +		if (se_cmd->tag == tag) {
> +			*unpacked_lun = se_cmd->orig_fe_lun;
> +			ret = true;
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> +
> +	return ret;
> +}
> +
>  /**
>   * target_submit_tmr - lookup unpacked lun and submit uninitialized se_cmd
>   *                     for TMR CDBs
> @@ -1639,19 +1662,31 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
>  		core_tmr_release_req(se_cmd->se_tmr_req);
>  		return ret;
>  	}
> +	/*
> +	 * If this is ABORT_TASK with no explicit fabric provided LUN,
> +	 * go ahead and search active session tags for a match to figure
> +	 * out unpacked_lun for the original se_cmd.
> +	 */
> +	if (tm_type == TMR_ABORT_TASK && (flags & TARGET_SCF_LOOKUP_LUN_FROM_TAG)) {
> +		if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun))
> +			goto failure;
> +	}
>  
>  	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
> -	if (ret) {
> -		/*
> -		 * For callback during failure handling, push this work off
> -		 * to process context with TMR_LUN_DOES_NOT_EXIST status.
> -		 */
> -		INIT_WORK(&se_cmd->work, target_complete_tmr_failure);
> -		schedule_work(&se_cmd->work);
> -		return 0;
> -	}
> +	if (ret)
> +		goto failure;
> +
>  	transport_generic_handle_tmr(se_cmd);
>  	return 0;

Hello Nic,

So after LUN lookup has finished sess_cmd_lock lock is dropped, a context
switch occurs due to the queue_work() call in transport_generic_handle_tmr()
and next core_tmr_abort_task() reacquires that lock? Sorry but I'm afraid
that if after that lock is dropped and before it is reacquired a command
finishes and the initiator reuses the same tag for another command for the
same LUN that this may result in the wrong command being aborted.

Bart.

Re: [PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK

[Nicholas A. Bellinger]

On Mon, 2017-06-05 at 15:57 +0000, Bart Van Assche wrote:
> On Sat, 2017-06-03 at 22:10 +0000, Nicholas A. Bellinger wrote:
> > +static bool target_lookup_lun_from_tag(struct se_session *se_sess, u64 tag,
> > +				       u64 *unpacked_lun)
> > +{
> > +	struct se_cmd *se_cmd;
> > +	unsigned long flags;
> > +	bool ret = false;
> > +
> > +	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> > +	list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) {
> > +		if (se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
> > +			continue;
> > +
> > +		if (se_cmd->tag == tag) {
> > +			*unpacked_lun = se_cmd->orig_fe_lun;
> > +			ret = true;
> > +			break;
> > +		}
> > +	}
> > +	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> > +
> > +	return ret;
> > +}
> > +
> >  /**
> >   * target_submit_tmr - lookup unpacked lun and submit uninitialized se_cmd
> >   *                     for TMR CDBs
> > @@ -1639,19 +1662,31 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
> >  		core_tmr_release_req(se_cmd->se_tmr_req);
> >  		return ret;
> >  	}
> > +	/*
> > +	 * If this is ABORT_TASK with no explicit fabric provided LUN,
> > +	 * go ahead and search active session tags for a match to figure
> > +	 * out unpacked_lun for the original se_cmd.
> > +	 */
> > +	if (tm_type == TMR_ABORT_TASK && (flags & TARGET_SCF_LOOKUP_LUN_FROM_TAG)) {
> > +		if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun))
> > +			goto failure;
> > +	}
> >  
> >  	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
> > -	if (ret) {
> > -		/*
> > -		 * For callback during failure handling, push this work off
> > -		 * to process context with TMR_LUN_DOES_NOT_EXIST status.
> > -		 */
> > -		INIT_WORK(&se_cmd->work, target_complete_tmr_failure);
> > -		schedule_work(&se_cmd->work);
> > -		return 0;
> > -	}
> > +	if (ret)
> > +		goto failure;
> > +
> >  	transport_generic_handle_tmr(se_cmd);
> >  	return 0;
> 
> Hello Nic,
> 
> So after LUN lookup has finished sess_cmd_lock lock is dropped, a context
> switch occurs due to the queue_work() call in transport_generic_handle_tmr()
> and next core_tmr_abort_task() reacquires that lock? Sorry but I'm afraid
> that if after that lock is dropped and before it is reacquired a command
> finishes and the initiator reuses the same tag for another command for the
> same LUN that this may result in the wrong command being aborted.

This is only getting a unpacked_lun to determine where the incoming
ABORT_TASK should perform a se_lun lookup and percpu ref upon.

The scenario you are talking about would require a tag be reused on the
same session for the same device between when the ABORT_TASK is
dispatched here, and actually processed.

Because qla2xxx doesn't reuse tags, it's not a problem since it's the
only consumer of TARGET_SCF_LOOKUP_LUN_FROM_TAG.

Since Himanshu and Quinn are OK it with, I'm OK with it.

If you have another driver that needs to use this logic where an
ABORT_TASK doesn't know the unpacked_lun to reference, and does reuse
tags then I'd consider a patch for that.

Re: [PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK

[Bart Van Assche]

On 06/08/17 22:52, Nicholas A. Bellinger wrote:
> Because qla2xxx doesn't reuse tags, it's not a problem since it's the
> only consumer of TARGET_SCF_LOOKUP_LUN_FROM_TAG.

Hello Nic,

Can you clarify this? Since all target drivers and also the target core
use a finite number of bits to represent tags, tags *will* be reused
sooner or later.

Bart.

Re: [PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK

[Tran, Quinn]

Looks Good.

Regards,
Quinn Tran

-----Original Message-----
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Saturday, June 3, 2017 at 3:10 PM
To: target-devel <target-devel@vger.kernel.org>
Cc: linux-scsi <linux-scsi@vger.kernel.org>, lkml <linux-kernel@vger.kernel.org>, Nicholas Bellinger <nab@linux-iscsi.org>, "Madhani, Himanshu" <Himanshu.Madhani@cavium.com>, "Tran, Quinn" <Quinn.Tran@cavium.com>, Mike Christie <mchristi@redhat.com>, Hannes Reinecke <hare@suse.com>, Christoph Hellwig <hch@lst.de>
Subject: [PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK

    From: Nicholas Bellinger <nab@linux-iscsi.org>
    
    This patch introduces support in target_submit_tmr() for locating a
    unpacked_lun from an existing se_cmd->tag during ABORT_TASK.
    
    When TARGET_SCF_LOOKUP_LUN_FROM_TAG is set, target_submit_tmr()
    will do the extra lookup via target_lookup_lun_from_tag() and
    subsequently invoke transport_lookup_tmr_lun() so a proper
    percpu se_lun->lun_ref is taken before workqueue dispatch into
    se_device->tmr_wq happens.
    
    Aside from the extra target_lookup_lun_from_tag(), the existing
    code-path remains unchanged.
    
    Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
    Cc: Quinn Tran <quinn.tran@cavium.com>
    Cc: Mike Christie <mchristi@redhat.com>
    Cc: Hannes Reinecke <hare@suse.com>
    Cc: Christoph Hellwig <hch@lst.de>
    Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
    ---
     drivers/target/target_core_transport.c | 53 ++++++++++++++++++++++++++++------
     include/target/target_core_base.h      |  3 +-
     2 files changed, 46 insertions(+), 10 deletions(-)
    
    diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
    index 83bfc97..dbb8101 100644
    --- a/drivers/target/target_core_transport.c
    +++ b/drivers/target/target_core_transport.c
    @@ -1592,6 +1592,29 @@ static void target_complete_tmr_failure(struct work_struct *work)
     	transport_cmd_check_stop_to_fabric(se_cmd);
     }
     
    +static bool target_lookup_lun_from_tag(struct se_session *se_sess, u64 tag,
    +				       u64 *unpacked_lun)
    +{
    +	struct se_cmd *se_cmd;
    +	unsigned long flags;
    +	bool ret = false;
    +
    +	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
    +	list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) {
    +		if (se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
    +			continue;
    +
    +		if (se_cmd->tag == tag) {
    +			*unpacked_lun = se_cmd->orig_fe_lun;
    +			ret = true;
    +			break;
    +		}
    +	}
    +	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
    +
    +	return ret;
    +}
    +
     /**
      * target_submit_tmr - lookup unpacked lun and submit uninitialized se_cmd
      *                     for TMR CDBs
    @@ -1639,19 +1662,31 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
     		core_tmr_release_req(se_cmd->se_tmr_req);
     		return ret;
     	}
    +	/*
    +	 * If this is ABORT_TASK with no explicit fabric provided LUN,
    +	 * go ahead and search active session tags for a match to figure
    +	 * out unpacked_lun for the original se_cmd.
    +	 */
    +	if (tm_type == TMR_ABORT_TASK && (flags & TARGET_SCF_LOOKUP_LUN_FROM_TAG)) {
    +		if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun))
    +			goto failure;
    +	}
     
     	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
    -	if (ret) {
    -		/*
    -		 * For callback during failure handling, push this work off
    -		 * to process context with TMR_LUN_DOES_NOT_EXIST status.
    -		 */
    -		INIT_WORK(&se_cmd->work, target_complete_tmr_failure);
    -		schedule_work(&se_cmd->work);
    -		return 0;
    -	}
    +	if (ret)
    +		goto failure;
    +
     	transport_generic_handle_tmr(se_cmd);
     	return 0;
    +
    +	/*
    +	 * For callback during failure handling, push this work off
    +	 * to process context with TMR_LUN_DOES_NOT_EXIST status.
    +	 */
    +failure:
    +	INIT_WORK(&se_cmd->work, target_complete_tmr_failure);
    +	schedule_work(&se_cmd->work);
    +	return 0;
     }
     EXPORT_SYMBOL(target_submit_tmr);
     
    diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
    index db2c7b3..a3af69f 100644
    --- a/include/target/target_core_base.h
    +++ b/include/target/target_core_base.h
    @@ -188,7 +188,8 @@ enum target_sc_flags_table {
     	TARGET_SCF_BIDI_OP		= 0x01,
     	TARGET_SCF_ACK_KREF		= 0x02,
     	TARGET_SCF_UNKNOWN_SIZE		= 0x04,
    -	TARGET_SCF_USE_CPUID	= 0x08,
    +	TARGET_SCF_USE_CPUID		= 0x08,
    +	TARGET_SCF_LOOKUP_LUN_FROM_TAG	= 0x10,
     };
     
     /* fabric independent task management function values */
    -- 
    1.9.1
    
    

[PATCH 3/3] qla2xxx: Convert QLA_TGT_ABTS to TARGET_SCF_LOOKUP_LUN_FROM_TAG

[Nicholas A. Bellinger]

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

Following Himanshu's earlier patch to drop the redundant tag
lookup within __qlt_24xx_handle_abts(), go ahead and drop this
now QLA_TGT_ABTS can use TARGET_SCF_LOOKUP_LUN_FROM_TAG and
have target_submit_tmr() do this from common code.

Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
Cc: Quinn Tran <quinn.tran@cavium.com>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/scsi/qla2xxx/qla_target.c  | 39 +++++++++-----------------------------
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |  4 +++-
 2 files changed, 12 insertions(+), 31 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 0e03ca2..401e245 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -1847,38 +1847,13 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host *vha,
 	struct abts_recv_from_24xx *abts, struct fc_port *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;
-	bool found_lun = false;
-	unsigned long flags;
-
-	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
-	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 (se_cmd->tag == abts->exchange_addr_to_abort) {
-			lun = cmd->unpacked_lun;
-			found_lun = true;
-			break;
-		}
-	}
-	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 
-	/* cmd not in LIO lists, look in qla list */
-	if (!found_lun) {
-		if (abort_cmd_for_tag(vha, abts->exchange_addr_to_abort)) {
-			/* send TASK_ABORT response immediately */
-			qlt_24xx_send_abts_resp(vha, abts, FCP_TMF_CMPL, false);
-			return 0;
-		} else {
-			ql_dbg(ql_dbg_tgt_mgt, vha, 0xf081,
-			    "unable to find cmd in driver or LIO for tag 0x%x\n",
-			    abts->exchange_addr_to_abort);
-			return -ENOENT;
-		}
+	if (abort_cmd_for_tag(vha, abts->exchange_addr_to_abort)) {
+		/* send TASK_ABORT response immediately */
+		qlt_24xx_send_abts_resp(vha, abts, FCP_TMF_CMPL, false);
+		return 0;
 	}
 
 	ql_dbg(ql_dbg_tgt_mgt, vha, 0xf00f,
@@ -1899,7 +1874,11 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host *vha,
 	mcmd->reset_count = vha->hw->chip_reset;
 	mcmd->tmr_func = QLA_TGT_ABTS;
 
-	rc = ha->tgt.tgt_ops->handle_tmr(mcmd, lun, mcmd->tmr_func,
+	/*
+	 * LUN is looked up by target-core internally based on the passed
+	 * abts->exchange_addr_to_abort tag.
+	 */
+	rc = ha->tgt.tgt_ops->handle_tmr(mcmd, 0, mcmd->tmr_func,
 	    abts->exchange_addr_to_abort);
 	if (rc != 0) {
 		ql_dbg(ql_dbg_tgt_mgt, vha, 0xf052,
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 7443e4e..75aeb9f 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -601,11 +601,13 @@ static int tcm_qla2xxx_handle_tmr(struct qla_tgt_mgmt_cmd *mcmd, uint32_t lun,
 	struct fc_port *sess = mcmd->sess;
 	struct se_cmd *se_cmd = &mcmd->se_cmd;
 	int transl_tmr_func = 0;
+	int flags = TARGET_SCF_ACK_KREF;
 
 	switch (tmr_func) {
 	case QLA_TGT_ABTS:
 		pr_debug("%ld: ABTS received\n", sess->vha->host_no);
 		transl_tmr_func = TMR_ABORT_TASK;
+		flags |= TARGET_SCF_LOOKUP_LUN_FROM_TAG;
 		break;
 	case QLA_TGT_2G_ABORT_TASK:
 		pr_debug("%ld: 2G Abort Task received\n", sess->vha->host_no);
@@ -638,7 +640,7 @@ static int tcm_qla2xxx_handle_tmr(struct qla_tgt_mgmt_cmd *mcmd, uint32_t lun,
 	}
 
 	return target_submit_tmr(se_cmd, sess->se_sess, NULL, lun, mcmd,
-	    transl_tmr_func, GFP_ATOMIC, tag, TARGET_SCF_ACK_KREF);
+	    transl_tmr_func, GFP_ATOMIC, tag, flags);
 }
 
 static int tcm_qla2xxx_queue_data_in(struct se_cmd *se_cmd)
-- 
1.9.1

Re: [PATCH 3/3] qla2xxx: Convert QLA_TGT_ABTS to TARGET_SCF_LOOKUP_LUN_FROM_TAG

[Tran, Quinn]

Nic,

Thanks.  It looks good.

Regards,
Quinn Tran

-----Original Message-----
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Saturday, June 3, 2017 at 3:10 PM
To: target-devel <target-devel@vger.kernel.org>
Cc: linux-scsi <linux-scsi@vger.kernel.org>, lkml <linux-kernel@vger.kernel.org>, Nicholas Bellinger <nab@linux-iscsi.org>, "Madhani, Himanshu" <Himanshu.Madhani@cavium.com>, "Tran, Quinn" <Quinn.Tran@cavium.com>, Mike Christie <mchristi@redhat.com>, Hannes Reinecke <hare@suse.com>, Christoph Hellwig <hch@lst.de>
Subject: [PATCH 3/3] qla2xxx: Convert QLA_TGT_ABTS to TARGET_SCF_LOOKUP_LUN_FROM_TAG

    From: Nicholas Bellinger <nab@linux-iscsi.org>
    
    Following Himanshu's earlier patch to drop the redundant tag
    lookup within __qlt_24xx_handle_abts(), go ahead and drop this
    now QLA_TGT_ABTS can use TARGET_SCF_LOOKUP_LUN_FROM_TAG and
    have target_submit_tmr() do this from common code.
    
    Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
    Cc: Quinn Tran <quinn.tran@cavium.com>
    Cc: Mike Christie <mchristi@redhat.com>
    Cc: Hannes Reinecke <hare@suse.com>
    Cc: Christoph Hellwig <hch@lst.de>
    Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
    ---
     drivers/scsi/qla2xxx/qla_target.c  | 39 +++++++++-----------------------------
     drivers/scsi/qla2xxx/tcm_qla2xxx.c |  4 +++-
     2 files changed, 12 insertions(+), 31 deletions(-)
    
    diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
    index 0e03ca2..401e245 100644
    --- a/drivers/scsi/qla2xxx/qla_target.c
    +++ b/drivers/scsi/qla2xxx/qla_target.c
    @@ -1847,38 +1847,13 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host *vha,
     	struct abts_recv_from_24xx *abts, struct fc_port *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;
    -	bool found_lun = false;
    -	unsigned long flags;
    -
    -	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
    -	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 (se_cmd->tag == abts->exchange_addr_to_abort) {
    -			lun = cmd->unpacked_lun;
    -			found_lun = true;
    -			break;
    -		}
    -	}
    -	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
     
    -	/* cmd not in LIO lists, look in qla list */
    -	if (!found_lun) {
    -		if (abort_cmd_for_tag(vha, abts->exchange_addr_to_abort)) {
    -			/* send TASK_ABORT response immediately */
    -			qlt_24xx_send_abts_resp(vha, abts, FCP_TMF_CMPL, false);
    -			return 0;
    -		} else {
    -			ql_dbg(ql_dbg_tgt_mgt, vha, 0xf081,
    -			    "unable to find cmd in driver or LIO for tag 0x%x\n",
    -			    abts->exchange_addr_to_abort);
    -			return -ENOENT;
    -		}
    +	if (abort_cmd_for_tag(vha, abts->exchange_addr_to_abort)) {
    +		/* send TASK_ABORT response immediately */
    +		qlt_24xx_send_abts_resp(vha, abts, FCP_TMF_CMPL, false);
    +		return 0;
     	}
     
     	ql_dbg(ql_dbg_tgt_mgt, vha, 0xf00f,
    @@ -1899,7 +1874,11 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host *vha,
     	mcmd->reset_count = vha->hw->chip_reset;
     	mcmd->tmr_func = QLA_TGT_ABTS;
     
    -	rc = ha->tgt.tgt_ops->handle_tmr(mcmd, lun, mcmd->tmr_func,
    +	/*
    +	 * LUN is looked up by target-core internally based on the passed
    +	 * abts->exchange_addr_to_abort tag.
    +	 */
    +	rc = ha->tgt.tgt_ops->handle_tmr(mcmd, 0, mcmd->tmr_func,
     	    abts->exchange_addr_to_abort);
     	if (rc != 0) {
     		ql_dbg(ql_dbg_tgt_mgt, vha, 0xf052,
    diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
    index 7443e4e..75aeb9f 100644
    --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
    +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
    @@ -601,11 +601,13 @@ static int tcm_qla2xxx_handle_tmr(struct qla_tgt_mgmt_cmd *mcmd, uint32_t lun,
     	struct fc_port *sess = mcmd->sess;
     	struct se_cmd *se_cmd = &mcmd->se_cmd;
     	int transl_tmr_func = 0;
    +	int flags = TARGET_SCF_ACK_KREF;
     
     	switch (tmr_func) {
     	case QLA_TGT_ABTS:
     		pr_debug("%ld: ABTS received\n", sess->vha->host_no);
     		transl_tmr_func = TMR_ABORT_TASK;
    +		flags |= TARGET_SCF_LOOKUP_LUN_FROM_TAG;
     		break;
     	case QLA_TGT_2G_ABORT_TASK:
     		pr_debug("%ld: 2G Abort Task received\n", sess->vha->host_no);
    @@ -638,7 +640,7 @@ static int tcm_qla2xxx_handle_tmr(struct qla_tgt_mgmt_cmd *mcmd, uint32_t lun,
     	}
     
     	return target_submit_tmr(se_cmd, sess->se_sess, NULL, lun, mcmd,
    -	    transl_tmr_func, GFP_ATOMIC, tag, TARGET_SCF_ACK_KREF);
    +	    transl_tmr_func, GFP_ATOMIC, tag, flags);
     }
     
     static int tcm_qla2xxx_queue_data_in(struct se_cmd *se_cmd)
    -- 
    1.9.1
    
    

Re: [PATCH 0/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support

[Madhani, Himanshu]

> On Jun 3, 2017, at 3:10 PM, Nicholas A. Bellinger <nab@linux-iscsi.org> wrote:
> 
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Hi Himanshu + Quinn,
> 
> Here is a small series to introduce proper percpu se_lun->lun_ref
> counting for TMR, and add common code in target_submit_tmr() to
> do tag lookup for unpacked_lun in order to drop the original
> driver specific lookup within __qlt_24xx_handle_abts().
> 
> It's rather straight-forward, so review and test as a v4.13 item.
> 

Sure will do. Thanks.

> Thanks!
> 
> Nicholas Bellinger (3):
>  target: Add support for TMR percpu reference counting
>  target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK
>  qla2xxx: Convert QLA_TGT_ABTS to TARGET_SCF_LOOKUP_LUN_FROM_TAG
> 
> drivers/scsi/qla2xxx/qla_target.c      | 39 ++++++-----------------
> drivers/scsi/qla2xxx/tcm_qla2xxx.c     |  4 ++-
> drivers/target/target_core_device.c    | 14 ++++++---
> drivers/target/target_core_transport.c | 56 ++++++++++++++++++++++++++++------
> include/target/target_core_base.h      |  3 +-
> 5 files changed, 71 insertions(+), 45 deletions(-)
> 
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


- Himanshu

Re: [PATCH 0/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support

[Madhani, Himanshu]

> On Jun 3, 2017, at 3:10 PM, Nicholas A. Bellinger <nab@linux-iscsi.org> wrote:
> 
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Hi Himanshu + Quinn,
> 
> Here is a small series to introduce proper percpu se_lun->lun_ref
> counting for TMR, and add common code in target_submit_tmr() to
> do tag lookup for unpacked_lun in order to drop the original
> driver specific lookup within __qlt_24xx_handle_abts().
> 
> It's rather straight-forward, so review and test as a v4.13 item.
> 
> Thanks!
> 
> Nicholas Bellinger (3):
>  target: Add support for TMR percpu reference counting
>  target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK
>  qla2xxx: Convert QLA_TGT_ABTS to TARGET_SCF_LOOKUP_LUN_FROM_TAG
> 
> drivers/scsi/qla2xxx/qla_target.c      | 39 ++++++-----------------
> drivers/scsi/qla2xxx/tcm_qla2xxx.c     |  4 ++-
> drivers/target/target_core_device.c    | 14 ++++++---
> drivers/target/target_core_transport.c | 56 ++++++++++++++++++++++++++++------
> include/target/target_core_base.h      |  3 +-
> 5 files changed, 71 insertions(+), 45 deletions(-)
> 
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Series looks good. 

Acked-by: Himanshu Madhani <himanshu.madhani@cavium.com>

Thanks,
- Himanshu

Re: [PATCH 0/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support

[Nicholas A. Bellinger]

Hi Himanshu & Quinn,

On Wed, 2017-06-07 at 05:02 +0000, Madhani, Himanshu wrote:
> > On Jun 3, 2017, at 3:10 PM, Nicholas A. Bellinger <nab@linux-iscsi.org> wrote:
> > 
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > Hi Himanshu + Quinn,
> > 
> > Here is a small series to introduce proper percpu se_lun->lun_ref
> > counting for TMR, and add common code in target_submit_tmr() to
> > do tag lookup for unpacked_lun in order to drop the original
> > driver specific lookup within __qlt_24xx_handle_abts().
> > 
> > It's rather straight-forward, so review and test as a v4.13 item.
> > 
> > Thanks!
> > 
> > Nicholas Bellinger (3):
> >  target: Add support for TMR percpu reference counting
> >  target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK
> >  qla2xxx: Convert QLA_TGT_ABTS to TARGET_SCF_LOOKUP_LUN_FROM_TAG
> > 
> > drivers/scsi/qla2xxx/qla_target.c      | 39 ++++++-----------------
> > drivers/scsi/qla2xxx/tcm_qla2xxx.c     |  4 ++-
> > drivers/target/target_core_device.c    | 14 ++++++---
> > drivers/target/target_core_transport.c | 56 ++++++++++++++++++++++++++++------
> > include/target/target_core_base.h      |  3 +-
> > 5 files changed, 71 insertions(+), 45 deletions(-)
> > 
> > -- 
> > 1.9.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe target-devel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Series looks good. 
> 
> Acked-by: Himanshu Madhani <himanshu.madhani@cavium.com>
> 

Thanks for the review!