From: Nicholas Bellinger <[email protected]>
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
From: Nicholas Bellinger <[email protected]>
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 <[email protected]>
Cc: Quinn Tran <[email protected]>
Cc: Mike Christie <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
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
From: Nicholas Bellinger <[email protected]>
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 <[email protected]>
Cc: Quinn Tran <[email protected]>
Cc: Mike Christie <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
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
From: Nicholas Bellinger <[email protected]>
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 <[email protected]>
Cc: Quinn Tran <[email protected]>
Cc: Mike Christie <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
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
> On Jun 3, 2017, at 3:10 PM, Nicholas A. Bellinger <[email protected]> wrote:
>
> From: Nicholas Bellinger <[email protected]>
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
- Himanshu
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.
Nic,
Thanks. It looks good.
Regards,
Quinn Tran
-----Original Message-----
From: Nicholas Bellinger <[email protected]>
Date: Saturday, June 3, 2017 at 3:10 PM
To: target-devel <[email protected]>
Cc: linux-scsi <[email protected]>, lkml <[email protected]>, Nicholas Bellinger <[email protected]>, "Madhani, Himanshu" <[email protected]>, "Tran, Quinn" <[email protected]>, Mike Christie <[email protected]>, Hannes Reinecke <[email protected]>, Christoph Hellwig <[email protected]>
Subject: [PATCH 3/3] qla2xxx: Convert QLA_TGT_ABTS to TARGET_SCF_LOOKUP_LUN_FROM_TAG
From: Nicholas Bellinger <[email protected]>
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 <[email protected]>
Cc: Quinn Tran <[email protected]>
Cc: Mike Christie <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
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
Looks Good.
Regards,
Quinn Tran
-----Original Message-----
From: Nicholas Bellinger <[email protected]>
Date: Saturday, June 3, 2017 at 3:10 PM
To: target-devel <[email protected]>
Cc: linux-scsi <[email protected]>, lkml <[email protected]>, Nicholas Bellinger <[email protected]>, "Madhani, Himanshu" <[email protected]>, "Tran, Quinn" <[email protected]>, Mike Christie <[email protected]>, Hannes Reinecke <[email protected]>, Christoph Hellwig <[email protected]>
Subject: [PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK
From: Nicholas Bellinger <[email protected]>
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 <[email protected]>
Cc: Quinn Tran <[email protected]>
Cc: Mike Christie <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
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
Looks good.
Regards,
Quinn Tran
-----Original Message-----
From: Nicholas Bellinger <[email protected]>
Date: Saturday, June 3, 2017 at 3:10 PM
To: target-devel <[email protected]>
Cc: linux-scsi <[email protected]>, lkml <[email protected]>, Nicholas Bellinger <[email protected]>, "Madhani, Himanshu" <[email protected]>, "Tran, Quinn" <[email protected]>, Mike Christie <[email protected]>, Hannes Reinecke <[email protected]>, Christoph Hellwig <[email protected]>
Subject: [PATCH 1/3] target: Add support for TMR percpu reference counting
From: Nicholas Bellinger <[email protected]>
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 <[email protected]>
Cc: Quinn Tran <[email protected]>
Cc: Mike Christie <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
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
> On Jun 3, 2017, at 3:10 PM, Nicholas A. Bellinger <[email protected]> wrote:
>
> From: Nicholas Bellinger <[email protected]>
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Series looks good.
Acked-by: Himanshu Madhani <[email protected]>
Thanks,
- Himanshu
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 <[email protected]> wrote:
> >
> > From: Nicholas Bellinger <[email protected]>
> >
> > 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Series looks good.
>
> Acked-by: Himanshu Madhani <[email protected]>
>
Thanks for the review!
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.
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.