diff mbox

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

Message ID 1496527808-28789-3-git-send-email-nab@linux-iscsi.org (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Nicholas A. Bellinger June 3, 2017, 10:10 p.m. UTC 
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(-)

Comments

Bart Van Assche June 5, 2017, 3:57 p.m. UTC | #1 
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.
Tran, Quinn June 7, 2017, 1:13 a.m. UTC | #2 
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
Nicholas A. Bellinger June 9, 2017, 5:52 a.m. UTC | #3 
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.
Bart Van Assche June 9, 2017, 6:15 p.m. UTC | #4 
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.
diff mbox

Patch

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 */