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

2017-06-09 Thread 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

2017-06-08 Thread Nicholas A. Bellinger 
On Mon, 2017-06-05 at 15:57 +, Bart Van Assche wrote:
> On Sat, 2017-06-03 at 22:10 +, 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(_sess->sess_cmd_lock, flags);
> > +   list_for_each_entry(se_cmd, _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(_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, _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(_cmd->work, target_complete_tmr_failure);
> > -   schedule_work(_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

2017-06-06 Thread Tran, Quinn 
Looks Good.

Regards,
Quinn Tran

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

From: Nicholas Bellinger <n...@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.madh...@cavium.com>
Cc: Quinn Tran <quinn.t...@cavium.com>
Cc: Mike Christie <mchri...@redhat.com>
Cc: Hannes Reinecke <h...@suse.com>
Cc: Christoph Hellwig <h...@lst.de>
Signed-off-by: Nicholas Bellinger <n...@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(_sess->sess_cmd_lock, flags);
+   list_for_each_entry(se_cmd, _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(_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, _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(_cmd->work, target_complete_tmr_failure);
-   schedule_work(_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(_cmd->work, target_complete_tmr_failure);
+   schedule_work(_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,

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

2017-06-05 Thread Bart Van Assche 
On Sat, 2017-06-03 at 22:10 +, 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(_sess->sess_cmd_lock, flags);
> + list_for_each_entry(se_cmd, _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(_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, _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(_cmd->work, target_complete_tmr_failure);
> - schedule_work(_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.

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

2017-06-03 Thread Nicholas A. Bellinger 
From: Nicholas Bellinger 

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 
Cc: Quinn Tran 
Cc: Mike Christie 
Cc: Hannes Reinecke 
Cc: Christoph Hellwig 
Signed-off-by: Nicholas Bellinger 
---
 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(_sess->sess_cmd_lock, flags);
+   list_for_each_entry(se_cmd, _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(_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, _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(_cmd->work, target_complete_tmr_failure);
-   schedule_work(_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(_cmd->work, target_complete_tmr_failure);
+   schedule_work(_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