* [PATCH-v2 0/3] target: Fix LUN_RESET active I/O + TMR handling
@ 2016-01-23 1:37 Nicholas A. Bellinger
2016-01-23 1:37 ` [PATCH-v2 1/3] target: Fix LUN_RESET active I/O handling for ACK_KREF Nicholas A. Bellinger
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Nicholas A. Bellinger @ 2016-01-23 1:37 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, Quinn Tran, Himanshu Madhani, Sagi Grimberg,
Christoph Hellwig, Hannes Reinecke, Andy Grover, Mike Christie,
Nicholas Bellinger
From: Nicholas Bellinger <nab@linux-iscsi.org>
Hi folks,
Here is -v2 series to address two LUN_RESET active
I/O + TMR se_cmd->cmd_kref < 0 bugs as reported
recently by Quinn & Co. This bug can occur during
active I/O remote port TMR LUN_RESET with multi-port
LIO configurations.
To address this bug, it adds __target_check_io_state()
common handler for ABORT_TASK + LUN_RESET I/O abort
cases, and moves remaining se_cmd SGL page + release
into target_free_cmd_mem() to now be called directly
from the final target_release_cmd_kref() callback.
Following __target_check_io_state(), it also obtains
kref_get_unless_zero(&se_cmd->cmd_kref) for TMR descs
during LUN_RESET in core_tmr_drain_tmr_list(), in order
to utilize common transport_wait_for_tasks() code when
se_cmd + se_tmr_req descriptors are being aborted or
shutdown.
At this point the changes are able to survive multi-port
active I/O LUN_RESET using iscsi-target ports against
Linux + ESX hosts with TAS=1.
Note there is still a known issue wrt multi-port active
I/O LUN_RESET when se_session shutdown occurs at the
same time, that I'm still groking and will need to be
addressed for -v3 code.
Quinn + Co, please have a look at qla2xxx + TAS with
this series when you've got a moment, and let us know
how active I/O LUN_RESET works with your test-case.
Thank you,
--nab
v2 changes:
- Fix TAS handling for multi-session se_node_acls
- Change target_complete_cmd() to check for ABORTED || STOP
- Avoid calling target_remove_from_state_list() with
se_cmd->t_state_lock held in transport_cmd_check_stop()
- Add missing target_free_cmd_mem() in target_put_sess_cmd()
special case.
- Add a more useful __target_check_io_state() comment
- Use assert_spin_locked() + WARN_ON_ONCE() when necessary
- Drop unnecessary !list_empty check
Nicholas Bellinger (3):
target: Fix LUN_RESET active I/O handling for ACK_KREF
target: Fix LUN_RESET active TMR descriptor handling
target: Fix TAS handling for multi-session se_node_acls
drivers/target/target_core_tmr.c | 104 ++++++++++++++++++++++++---------
drivers/target/target_core_transport.c | 73 ++++++++++++++---------
include/target/target_core_base.h | 1 +
3 files changed, 122 insertions(+), 56 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH-v2 1/3] target: Fix LUN_RESET active I/O handling for ACK_KREF
2016-01-23 1:37 [PATCH-v2 0/3] target: Fix LUN_RESET active I/O + TMR handling Nicholas A. Bellinger
@ 2016-01-23 1:37 ` Nicholas A. Bellinger
2016-01-26 17:19 ` Christoph Hellwig
2016-01-28 17:00 ` Sagi Grimberg
2016-01-23 1:37 ` [PATCH-v2 2/3] target: Fix LUN_RESET active TMR descriptor handling Nicholas A. Bellinger
2016-01-23 1:37 ` [PATCH-v2 3/3] target: Fix TAS handling for multi-session se_node_acls Nicholas A. Bellinger
2 siblings, 2 replies; 9+ messages in thread
From: Nicholas A. Bellinger @ 2016-01-23 1:37 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, Quinn Tran, Himanshu Madhani, Sagi Grimberg,
Christoph Hellwig, Hannes Reinecke, Andy Grover, Mike Christie,
Nicholas Bellinger
From: Nicholas Bellinger <nab@linux-iscsi.org>
This patch fixes a NULL pointer se_cmd->cmd_kref < 0
refcount bug during TMR LUN_RESET with active se_cmd
I/O, that can be triggered during se_cmd descriptor
shutdown + release via core_tmr_drain_state_list() code.
To address this bug, add common __target_check_io_state()
helper for ABORT_TASK + LUN_RESET w/ CMD_T_COMPLETE
checking, and set CMD_T_ABORTED + obtain ->cmd_kref for
both cases ahead of last target_put_sess_cmd() after
TFO->aborted_task() -> transport_cmd_finish_abort()
callback has completed.
It also introduces SCF_ACK_KREF to determine when
transport_cmd_finish_abort() needs to drop the second
extra reference, ahead of calling target_put_sess_cmd()
for the final kref_put(&se_cmd->cmd_kref).
It also updates transport_cmd_check_stop() to avoid
holding se_cmd->t_state_lock while dropping se_cmd
device state via target_remove_from_state_list(), now
that core_tmr_drain_state_list() is holding the
se_device lock while checking se_cmd state from
within TMR logic.
Finally, move transport_put_cmd() release of SGL +
TMR + extended CDB memory into target_free_cmd_mem()
in order to avoid potential resource leaks in TMR
ABORT_TASK + LUN_RESET code-paths. Also update
target_release_cmd_kref() accordingly.
Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Himanshu Madhani <himanshu.madhani@qlogic.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Andy Grover <agrover@redhat.com>
Cc: Mike Christie <mchristi@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
drivers/target/target_core_tmr.c | 66 ++++++++++++++++++++++++----------
drivers/target/target_core_transport.c | 56 ++++++++++++++---------------
include/target/target_core_base.h | 1 +
3 files changed, 75 insertions(+), 48 deletions(-)
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 7137042..5f315b4 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -107,6 +107,34 @@ static int target_check_cdb_and_preempt(struct list_head *list,
return 1;
}
+static bool __target_check_io_state(struct se_cmd *se_cmd)
+{
+ struct se_session *sess = se_cmd->se_sess;
+
+ assert_spin_locked(&se_session->sess_cmd_lock);
+ WARN_ON_ONCE(!irqs_disabled());
+ /*
+ * If command already reached CMD_T_COMPLETE state within
+ * target_complete_cmd(), this se_cmd has been passed to
+ * fabric driver and will not be aborted.
+ *
+ * Otherwise, obtain a local se_cmd->cmd_kref now for TMR
+ * ABORT_TASK + LUN_RESET for CMD_T_ABORTED processing as
+ * long as se_cmd->cmd_kref is still active unless zero.
+ */
+ spin_lock(&se_cmd->t_state_lock);
+ if (se_cmd->transport_state & CMD_T_COMPLETE) {
+ pr_debug("Attempted to abort io tag: %llu already complete,"
+ " skipping\n", se_cmd->tag);
+ spin_unlock(&se_cmd->t_state_lock);
+ return false;
+ }
+ se_cmd->transport_state |= CMD_T_ABORTED;
+ spin_unlock(&se_cmd->t_state_lock);
+
+ return kref_get_unless_zero(&se_cmd->cmd_kref);
+}
+
void core_tmr_abort_task(
struct se_device *dev,
struct se_tmr_req *tmr,
@@ -133,26 +161,18 @@ void core_tmr_abort_task(
printk("ABORT_TASK: Found referenced %s task_tag: %llu\n",
se_cmd->se_tfo->get_fabric_name(), ref_tag);
- spin_lock(&se_cmd->t_state_lock);
- if (se_cmd->transport_state & CMD_T_COMPLETE) {
- printk("ABORT_TASK: ref_tag: %llu already complete,"
- " skipping\n", ref_tag);
- spin_unlock(&se_cmd->t_state_lock);
+ if (!__target_check_io_state(se_cmd)) {
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
goto out;
}
- se_cmd->transport_state |= CMD_T_ABORTED;
- spin_unlock(&se_cmd->t_state_lock);
-
list_del_init(&se_cmd->se_cmd_list);
- kref_get(&se_cmd->cmd_kref);
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
cancel_work_sync(&se_cmd->work);
transport_wait_for_tasks(se_cmd);
- target_put_sess_cmd(se_cmd);
transport_cmd_finish_abort(se_cmd, true);
+ target_put_sess_cmd(se_cmd);
printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for"
" ref_tag: %llu\n", ref_tag);
@@ -237,8 +257,10 @@ static void core_tmr_drain_state_list(
struct list_head *preempt_and_abort_list)
{
LIST_HEAD(drain_task_list);
+ struct se_session *sess;
struct se_cmd *cmd, *next;
unsigned long flags;
+ int rc;
/*
* Complete outstanding commands with TASK_ABORTED SAM status.
@@ -277,6 +299,17 @@ static void core_tmr_drain_state_list(
if (prout_cmd == cmd)
continue;
+ sess = cmd->se_sess;
+ if (WARN_ON_ONCE(!sess))
+ continue;
+
+ spin_lock(&sess->sess_cmd_lock);
+ rc = __target_check_io_state(cmd);
+ spin_unlock(&sess->sess_cmd_lock);
+ if (!rc) {
+ printk("LUN_RESET I/O: non-zero kref_get_unless_zero\n");
+ continue;
+ }
list_move_tail(&cmd->state_list, &drain_task_list);
cmd->state_active = false;
}
@@ -284,7 +317,7 @@ static void core_tmr_drain_state_list(
while (!list_empty(&drain_task_list)) {
cmd = list_entry(drain_task_list.next, struct se_cmd, state_list);
- list_del(&cmd->state_list);
+ list_del_init(&cmd->state_list);
pr_debug("LUN_RESET: %s cmd: %p"
" ITT/CmdSN: 0x%08llx/0x%08x, i_state: %d, t_state: %d"
@@ -308,16 +341,11 @@ static void core_tmr_drain_state_list(
* loop above, but we do it down here given that
* cancel_work_sync may block.
*/
- if (cmd->t_state == TRANSPORT_COMPLETE)
- cancel_work_sync(&cmd->work);
-
- spin_lock_irqsave(&cmd->t_state_lock, flags);
- target_stop_cmd(cmd, &flags);
-
- cmd->transport_state |= CMD_T_ABORTED;
- spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+ cancel_work_sync(&cmd->work);
+ transport_wait_for_tasks(cmd);
core_tmr_handle_tas_abort(tmr_nacl, cmd, tas);
+ target_put_sess_cmd(cmd);
}
}
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index a7c1bb5..19500e3 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -561,10 +561,6 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
{
unsigned long flags;
- spin_lock_irqsave(&cmd->t_state_lock, flags);
- if (write_pending)
- cmd->t_state = TRANSPORT_WRITE_PENDING;
-
if (remove_from_lists) {
target_remove_from_state_list(cmd);
@@ -574,6 +570,10 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
cmd->se_lun = NULL;
}
+ spin_lock_irqsave(&cmd->t_state_lock, flags);
+ if (write_pending)
+ cmd->t_state = TRANSPORT_WRITE_PENDING;
+
/*
* Determine if frontend context caller is requesting the stopping of
* this command for frontend exceptions.
@@ -627,6 +627,8 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd)
void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
{
+ bool ack_kref = (cmd->se_cmd_flags & SCF_ACK_KREF);
+
if (cmd->se_cmd_flags & SCF_SE_LUN_CMD)
transport_lun_remove_cmd(cmd);
/*
@@ -638,7 +640,7 @@ void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
if (transport_cmd_check_stop_to_fabric(cmd))
return;
- if (remove)
+ if (remove && ack_kref)
transport_put_cmd(cmd);
}
@@ -706,7 +708,7 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
* Check for case where an explicit ABORT_TASK has been received
* and transport_wait_for_tasks() will be waiting for completion..
*/
- if (cmd->transport_state & CMD_T_ABORTED &&
+ if (cmd->transport_state & CMD_T_ABORTED ||
cmd->transport_state & CMD_T_STOP) {
spin_unlock_irqrestore(&cmd->t_state_lock, flags);
complete_all(&cmd->t_transport_stop_comp);
@@ -2220,20 +2222,14 @@ static inline void transport_free_pages(struct se_cmd *cmd)
}
/**
- * transport_release_cmd - free a command
- * @cmd: command to free
+ * transport_put_cmd - release a reference to a command
+ * @cmd: command to release
*
- * This routine unconditionally frees a command, and reference counting
- * or list removal must be done in the caller.
+ * This routine releases our reference to the command and frees it if possible.
*/
-static int transport_release_cmd(struct se_cmd *cmd)
+static int transport_put_cmd(struct se_cmd *cmd)
{
BUG_ON(!cmd->se_tfo);
-
- if (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
- core_tmr_release_req(cmd->se_tmr_req);
- if (cmd->t_task_cdb != cmd->__t_task_cdb)
- kfree(cmd->t_task_cdb);
/*
* If this cmd has been setup with target_get_sess_cmd(), drop
* the kref and call ->release_cmd() in kref callback.
@@ -2241,18 +2237,6 @@ static int transport_release_cmd(struct se_cmd *cmd)
return target_put_sess_cmd(cmd);
}
-/**
- * transport_put_cmd - release a reference to a command
- * @cmd: command to release
- *
- * This routine releases our reference to the command and frees it if possible.
- */
-static int transport_put_cmd(struct se_cmd *cmd)
-{
- transport_free_pages(cmd);
- return transport_release_cmd(cmd);
-}
-
void *transport_kmap_data_sg(struct se_cmd *cmd)
{
struct scatterlist *sg = cmd->t_data_sg;
@@ -2457,7 +2441,7 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
if (wait_for_tasks && (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB))
transport_wait_for_tasks(cmd);
- ret = transport_release_cmd(cmd);
+ ret = transport_put_cmd(cmd);
} else {
if (wait_for_tasks)
transport_wait_for_tasks(cmd);
@@ -2515,6 +2499,16 @@ out:
}
EXPORT_SYMBOL(target_get_sess_cmd);
+static void target_free_cmd_mem(struct se_cmd *cmd)
+{
+ transport_free_pages(cmd);
+
+ if (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
+ core_tmr_release_req(cmd->se_tmr_req);
+ if (cmd->t_task_cdb != cmd->__t_task_cdb)
+ kfree(cmd->t_task_cdb);
+}
+
static void target_release_cmd_kref(struct kref *kref)
__releases(&se_cmd->se_sess->sess_cmd_lock)
{
@@ -2523,17 +2517,20 @@ static void target_release_cmd_kref(struct kref *kref)
if (list_empty(&se_cmd->se_cmd_list)) {
spin_unlock(&se_sess->sess_cmd_lock);
+ target_free_cmd_mem(se_cmd);
se_cmd->se_tfo->release_cmd(se_cmd);
return;
}
if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) {
spin_unlock(&se_sess->sess_cmd_lock);
+ target_free_cmd_mem(se_cmd);
complete(&se_cmd->cmd_wait_comp);
return;
}
list_del(&se_cmd->se_cmd_list);
spin_unlock(&se_sess->sess_cmd_lock);
+ target_free_cmd_mem(se_cmd);
se_cmd->se_tfo->release_cmd(se_cmd);
}
@@ -2545,6 +2542,7 @@ int target_put_sess_cmd(struct se_cmd *se_cmd)
struct se_session *se_sess = se_cmd->se_sess;
if (!se_sess) {
+ target_free_cmd_mem(se_cmd);
se_cmd->se_tfo->release_cmd(se_cmd);
return 1;
}
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index a4bed07..1060c52 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -140,6 +140,7 @@ enum se_cmd_flags_table {
SCF_COMPARE_AND_WRITE = 0x00080000,
SCF_COMPARE_AND_WRITE_POST = 0x00100000,
SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x00200000,
+ SCF_ACK_KREF = 0x00400000,
};
/* struct se_dev_entry->lun_flags and struct se_lun->lun_access */
--
1.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH-v2 2/3] target: Fix LUN_RESET active TMR descriptor handling
2016-01-23 1:37 [PATCH-v2 0/3] target: Fix LUN_RESET active I/O + TMR handling Nicholas A. Bellinger
2016-01-23 1:37 ` [PATCH-v2 1/3] target: Fix LUN_RESET active I/O handling for ACK_KREF Nicholas A. Bellinger
@ 2016-01-23 1:37 ` Nicholas A. Bellinger
2016-01-26 17:21 ` Christoph Hellwig
2016-01-23 1:37 ` [PATCH-v2 3/3] target: Fix TAS handling for multi-session se_node_acls Nicholas A. Bellinger
2 siblings, 1 reply; 9+ messages in thread
From: Nicholas A. Bellinger @ 2016-01-23 1:37 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, Quinn Tran, Himanshu Madhani, Sagi Grimberg,
Christoph Hellwig, Hannes Reinecke, Andy Grover, Mike Christie,
Nicholas Bellinger
From: Nicholas Bellinger <nab@linux-iscsi.org>
This patch fixes a NULL pointer se_cmd->cmd_kref < 0
refcount bug during TMR LUN_RESET with active TMRs,
triggered during se_cmd + se_tmr_req descriptor
shutdown + release via core_tmr_drain_tmr_list().
To address this bug, go ahead and obtain a local
kref_get_unless_zero(&se_cmd->cmd_kref) for active I/O
to set CMD_T_ABORTED, and transport_wait_for_tasks()
followed by the final target_put_sess_cmd() to drop
the local ->cmd_kref.
Also add two new checks within target_tmr_work() to
avoid CMD_T_ABORTED -> TFO->queue_tm_rsp() callbacks
ahead of invoking the backend -> fabric put in
transport_cmd_check_stop_to_fabric().
For good measure, also change core_tmr_release_req()
to use list_del_init() ahead of se_tmr_req memory
free.
Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Himanshu Madhani <himanshu.madhani@qlogic.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Andy Grover <agrover@redhat.com>
Cc: Mike Christie <mchristi@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
drivers/target/target_core_tmr.c | 22 +++++++++++++++++++++-
drivers/target/target_core_transport.c | 17 +++++++++++++++++
2 files changed, 38 insertions(+), 1 deletion(-)
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 5f315b4..d4f30aa 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -68,7 +68,7 @@ void core_tmr_release_req(struct se_tmr_req *tmr)
if (dev) {
spin_lock_irqsave(&dev->se_tmr_lock, flags);
- list_del(&tmr->tmr_list);
+ list_del_init(&tmr->tmr_list);
spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
}
@@ -193,9 +193,11 @@ static void core_tmr_drain_tmr_list(
struct list_head *preempt_and_abort_list)
{
LIST_HEAD(drain_tmr_list);
+ struct se_session *sess;
struct se_tmr_req *tmr_p, *tmr_pp;
struct se_cmd *cmd;
unsigned long flags;
+ bool rc;
/*
* Release all pending and outgoing TMRs aside from the received
* LUN_RESET tmr..
@@ -221,17 +223,31 @@ static void core_tmr_drain_tmr_list(
if (target_check_cdb_and_preempt(preempt_and_abort_list, cmd))
continue;
+ sess = cmd->se_sess;
+ if (WARN_ON_ONCE(!sess))
+ continue;
+
+ spin_lock(&sess->sess_cmd_lock);
spin_lock(&cmd->t_state_lock);
if (!(cmd->transport_state & CMD_T_ACTIVE)) {
spin_unlock(&cmd->t_state_lock);
+ spin_unlock(&sess->sess_cmd_lock);
continue;
}
if (cmd->t_state == TRANSPORT_ISTATE_PROCESSING) {
spin_unlock(&cmd->t_state_lock);
+ spin_unlock(&sess->sess_cmd_lock);
continue;
}
+ cmd->transport_state |= CMD_T_ABORTED;
spin_unlock(&cmd->t_state_lock);
+ rc = kref_get_unless_zero(&cmd->cmd_kref);
+ spin_unlock(&sess->sess_cmd_lock);
+ if (!rc) {
+ printk("LUN_RESET TMR: non-zero kref_get_unless_zero\n");
+ continue;
+ }
list_move_tail(&tmr_p->tmr_list, &drain_tmr_list);
}
spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
@@ -245,7 +261,11 @@ static void core_tmr_drain_tmr_list(
(preempt_and_abort_list) ? "Preempt" : "", tmr_p,
tmr_p->function, tmr_p->response, cmd->t_state);
+ cancel_work_sync(&cmd->work);
+ transport_wait_for_tasks(cmd);
+
transport_cmd_finish_abort(cmd, 1);
+ target_put_sess_cmd(cmd);
}
}
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 19500e3..6235067 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2905,8 +2905,17 @@ static void target_tmr_work(struct work_struct *work)
struct se_cmd *cmd = container_of(work, struct se_cmd, work);
struct se_device *dev = cmd->se_dev;
struct se_tmr_req *tmr = cmd->se_tmr_req;
+ unsigned long flags;
int ret;
+ spin_lock_irqsave(&cmd->t_state_lock, flags);
+ if (cmd->transport_state & CMD_T_ABORTED) {
+ tmr->response = TMR_FUNCTION_REJECTED;
+ spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+ goto check_stop;
+ }
+ spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
switch (tmr->function) {
case TMR_ABORT_TASK:
core_tmr_abort_task(dev, tmr, cmd->se_sess);
@@ -2939,9 +2948,17 @@ static void target_tmr_work(struct work_struct *work)
break;
}
+ spin_lock_irqsave(&cmd->t_state_lock, flags);
+ if (cmd->transport_state & CMD_T_ABORTED) {
+ spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+ goto check_stop;
+ }
cmd->t_state = TRANSPORT_ISTATE_PROCESSING;
+ spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
cmd->se_tfo->queue_tm_rsp(cmd);
+check_stop:
transport_cmd_check_stop_to_fabric(cmd);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH-v2 3/3] target: Fix TAS handling for multi-session se_node_acls
2016-01-23 1:37 [PATCH-v2 0/3] target: Fix LUN_RESET active I/O + TMR handling Nicholas A. Bellinger
2016-01-23 1:37 ` [PATCH-v2 1/3] target: Fix LUN_RESET active I/O handling for ACK_KREF Nicholas A. Bellinger
2016-01-23 1:37 ` [PATCH-v2 2/3] target: Fix LUN_RESET active TMR descriptor handling Nicholas A. Bellinger
@ 2016-01-23 1:37 ` Nicholas A. Bellinger
2016-01-26 17:22 ` Christoph Hellwig
2 siblings, 1 reply; 9+ messages in thread
From: Nicholas A. Bellinger @ 2016-01-23 1:37 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, Quinn Tran, Himanshu Madhani, Sagi Grimberg,
Christoph Hellwig, Hannes Reinecke, Andy Grover, Mike Christie,
Nicholas Bellinger
From: Nicholas Bellinger <nab@linux-iscsi.org>
This patch fixes a bug in TMR task aborted status (TAS)
handling when multiple sessions are connected to the
same target WWPN endpoint and se_node_acl descriptor,
resulting in TASK_ABORTED status to not be generated
for aborted se_cmds on the remote port.
This is due to core_tmr_handle_tas_abort() incorrectly
comparing se_node_acl instead of se_session, for which
the multi-session case is expected to be sharing the
same se_node_acl.
Instead, go ahead and update core_tmr_handle_tas_abort()
to compare tmr_sess + cmd->se_sess in order to determine
if the LUN_RESET was received on a different I_T nexus,
and TASK_ABORTED status response needs to be generated.
Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Himanshu Madhani <himanshu.madhani@qlogic.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Andy Grover <agrover@redhat.com>
Cc: Mike Christie <mchristi@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
drivers/target/target_core_tmr.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index d4f30aa..3eff563 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -76,7 +76,7 @@ void core_tmr_release_req(struct se_tmr_req *tmr)
}
static void core_tmr_handle_tas_abort(
- struct se_node_acl *tmr_nacl,
+ struct se_session *tmr_sess,
struct se_cmd *cmd,
int tas)
{
@@ -84,7 +84,7 @@ static void core_tmr_handle_tas_abort(
/*
* TASK ABORTED status (TAS) bit support
*/
- if ((tmr_nacl && (tmr_nacl != cmd->se_sess->se_node_acl)) && tas) {
+ if ((tmr_sess && (tmr_sess != cmd->se_sess)) && tas) {
remove = false;
transport_send_task_abort(cmd);
}
@@ -111,7 +111,7 @@ static bool __target_check_io_state(struct se_cmd *se_cmd)
{
struct se_session *sess = se_cmd->se_sess;
- assert_spin_locked(&se_session->sess_cmd_lock);
+ assert_spin_locked(&sess->sess_cmd_lock);
WARN_ON_ONCE(!irqs_disabled());
/*
* If command already reached CMD_T_COMPLETE state within
@@ -272,7 +272,7 @@ static void core_tmr_drain_tmr_list(
static void core_tmr_drain_state_list(
struct se_device *dev,
struct se_cmd *prout_cmd,
- struct se_node_acl *tmr_nacl,
+ struct se_session *tmr_sess,
int tas,
struct list_head *preempt_and_abort_list)
{
@@ -364,7 +364,7 @@ static void core_tmr_drain_state_list(
cancel_work_sync(&cmd->work);
transport_wait_for_tasks(cmd);
- core_tmr_handle_tas_abort(tmr_nacl, cmd, tas);
+ core_tmr_handle_tas_abort(tmr_sess, cmd, tas);
target_put_sess_cmd(cmd);
}
}
@@ -377,6 +377,7 @@ int core_tmr_lun_reset(
{
struct se_node_acl *tmr_nacl = NULL;
struct se_portal_group *tmr_tpg = NULL;
+ struct se_session *tmr_sess = NULL;
int tas;
/*
* TASK_ABORTED status bit, this is configurable via ConfigFS
@@ -395,8 +396,9 @@ int core_tmr_lun_reset(
* or struct se_device passthrough..
*/
if (tmr && tmr->task_cmd && tmr->task_cmd->se_sess) {
- tmr_nacl = tmr->task_cmd->se_sess->se_node_acl;
- tmr_tpg = tmr->task_cmd->se_sess->se_tpg;
+ tmr_sess = tmr->task_cmd->se_sess;
+ tmr_nacl = tmr_sess->se_node_acl;
+ tmr_tpg = tmr_sess->se_tpg;
if (tmr_nacl && tmr_tpg) {
pr_debug("LUN_RESET: TMR caller fabric: %s"
" initiator port %s\n",
@@ -409,7 +411,7 @@ int core_tmr_lun_reset(
dev->transport->name, tas);
core_tmr_drain_tmr_list(dev, tmr, preempt_and_abort_list);
- core_tmr_drain_state_list(dev, prout_cmd, tmr_nacl, tas,
+ core_tmr_drain_state_list(dev, prout_cmd, tmr_sess, tas,
preempt_and_abort_list);
/*
--
1.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH-v2 1/3] target: Fix LUN_RESET active I/O handling for ACK_KREF
2016-01-23 1:37 ` [PATCH-v2 1/3] target: Fix LUN_RESET active I/O handling for ACK_KREF Nicholas A. Bellinger
@ 2016-01-26 17:19 ` Christoph Hellwig
2016-01-28 5:34 ` Nicholas A. Bellinger
2016-01-28 17:00 ` Sagi Grimberg
1 sibling, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2016-01-26 17:19 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: target-devel, linux-scsi, Quinn Tran, Himanshu Madhani,
Sagi Grimberg, Christoph Hellwig, Hannes Reinecke, Andy Grover,
Mike Christie, Nicholas Bellinger
> +static bool __target_check_io_state(struct se_cmd *se_cmd)
> +{
> + struct se_session *sess = se_cmd->se_sess;
> +
> + assert_spin_locked(&se_session->sess_cmd_lock);
> + WARN_ON_ONCE(!irqs_disabled());
Btw, I looked a the code and can't really see what sess_cmd_lock is
supposed to protect here.
> + sess = cmd->se_sess;
> + if (WARN_ON_ONCE(!sess))
> + continue;
> +
> + spin_lock(&sess->sess_cmd_lock);
> + rc = __target_check_io_state(cmd);
> + spin_unlock(&sess->sess_cmd_lock);
> + if (!rc) {
> + printk("LUN_RESET I/O: non-zero kref_get_unless_zero\n");
> + continue;
> + }
And thus why we care about taking it here.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-v2 2/3] target: Fix LUN_RESET active TMR descriptor handling
2016-01-23 1:37 ` [PATCH-v2 2/3] target: Fix LUN_RESET active TMR descriptor handling Nicholas A. Bellinger
@ 2016-01-26 17:21 ` Christoph Hellwig
0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2016-01-26 17:21 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: target-devel, linux-scsi, Quinn Tran, Himanshu Madhani,
Sagi Grimberg, Christoph Hellwig, Hannes Reinecke, Andy Grover,
Mike Christie, Nicholas Bellinger
> + sess = cmd->se_sess;
> + if (WARN_ON_ONCE(!sess))
> + continue;
> +
> + spin_lock(&sess->sess_cmd_lock);
> spin_lock(&cmd->t_state_lock);
> if (!(cmd->transport_state & CMD_T_ACTIVE)) {
> spin_unlock(&cmd->t_state_lock);
> + spin_unlock(&sess->sess_cmd_lock);
> continue;
> }
> if (cmd->t_state == TRANSPORT_ISTATE_PROCESSING) {
> spin_unlock(&cmd->t_state_lock);
> + spin_unlock(&sess->sess_cmd_lock);
> continue;
> }
> + cmd->transport_state |= CMD_T_ABORTED;
> spin_unlock(&cmd->t_state_lock);
>
> + rc = kref_get_unless_zero(&cmd->cmd_kref);
> + spin_unlock(&sess->sess_cmd_lock);
Similar to the previous patch I don't understand the need for
sess_cmd_lock here.
> + spin_lock_irqsave(&cmd->t_state_lock, flags);
> + if (cmd->transport_state & CMD_T_ABORTED) {
> + tmr->response = TMR_FUNCTION_REJECTED;
> + spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> + goto check_stop;
> + }
> + spin_unlock_irqrestore(&cmd->t_state_lock, flags);
Taking a lock for checking a single bit is pointless.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-v2 3/3] target: Fix TAS handling for multi-session se_node_acls
2016-01-23 1:37 ` [PATCH-v2 3/3] target: Fix TAS handling for multi-session se_node_acls Nicholas A. Bellinger
@ 2016-01-26 17:22 ` Christoph Hellwig
0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2016-01-26 17:22 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: target-devel, linux-scsi, Quinn Tran, Himanshu Madhani,
Sagi Grimberg, Christoph Hellwig, Hannes Reinecke, Andy Grover,
Mike Christie, Nicholas Bellinger
> - if ((tmr_nacl && (tmr_nacl != cmd->se_sess->se_node_acl)) && tas) {
> + if ((tmr_sess && (tmr_sess != cmd->se_sess)) && tas) {
Care to drop the pointless braces here if you touch this anyway?
if (tmr_sess && tmr_sess != cmd->se_sess && tas)
Otherwise this looks fine to me:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-v2 1/3] target: Fix LUN_RESET active I/O handling for ACK_KREF
2016-01-26 17:19 ` Christoph Hellwig
@ 2016-01-28 5:34 ` Nicholas A. Bellinger
0 siblings, 0 replies; 9+ messages in thread
From: Nicholas A. Bellinger @ 2016-01-28 5:34 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Nicholas A. Bellinger, target-devel, linux-scsi, Quinn Tran,
Himanshu Madhani, Sagi Grimberg, Hannes Reinecke, Andy Grover,
Mike Christie
On Tue, 2016-01-26 at 18:19 +0100, Christoph Hellwig wrote:
> > +static bool __target_check_io_state(struct se_cmd *se_cmd)
> > +{
> > + struct se_session *sess = se_cmd->se_sess;
> > +
> > + assert_spin_locked(&se_session->sess_cmd_lock);
> > + WARN_ON_ONCE(!irqs_disabled());
>
> Btw, I looked a the code and can't really see what sess_cmd_lock is
> supposed to protect here.
>
> > + sess = cmd->se_sess;
> > + if (WARN_ON_ONCE(!sess))
> > + continue;
> > +
> > + spin_lock(&sess->sess_cmd_lock);
> > + rc = __target_check_io_state(cmd);
> > + spin_unlock(&sess->sess_cmd_lock);
> > + if (!rc) {
> > + printk("LUN_RESET I/O: non-zero kref_get_unless_zero\n");
> > + continue;
> > + }
>
> And thus why we care about taking it here.
I'm still working on -v3 series to handle se_session shutdown during
this specific multi-port LUN_RESET case, and considering using the
existing se_cmd->cmd_wait_set bit to signal when this special case
happens.
Currently ->sess_cmd_lock is held in target_release_cmd_kref() and
target_sess_cmd_list_set_waiting() while checking and setting this
value.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-v2 1/3] target: Fix LUN_RESET active I/O handling for ACK_KREF
2016-01-23 1:37 ` [PATCH-v2 1/3] target: Fix LUN_RESET active I/O handling for ACK_KREF Nicholas A. Bellinger
2016-01-26 17:19 ` Christoph Hellwig
@ 2016-01-28 17:00 ` Sagi Grimberg
1 sibling, 0 replies; 9+ messages in thread
From: Sagi Grimberg @ 2016-01-28 17:00 UTC (permalink / raw)
To: Nicholas A. Bellinger, target-devel
Cc: linux-scsi, Quinn Tran, Himanshu Madhani, Sagi Grimberg,
Christoph Hellwig, Hannes Reinecke, Andy Grover, Mike Christie,
Nicholas Bellinger
> + sess = cmd->se_sess;
> + if (WARN_ON_ONCE(!sess))
> + continue;
> +
> + spin_lock(&sess->sess_cmd_lock);
> + rc = __target_check_io_state(cmd);
> + spin_unlock(&sess->sess_cmd_lock);
> + if (!rc) {
> + printk("LUN_RESET I/O: non-zero kref_get_unless_zero\n");
Is this message always correct? __target_check_io_state() will return
false for commands that were completed..
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-01-28 17:00 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-23 1:37 [PATCH-v2 0/3] target: Fix LUN_RESET active I/O + TMR handling Nicholas A. Bellinger
2016-01-23 1:37 ` [PATCH-v2 1/3] target: Fix LUN_RESET active I/O handling for ACK_KREF Nicholas A. Bellinger
2016-01-26 17:19 ` Christoph Hellwig
2016-01-28 5:34 ` Nicholas A. Bellinger
2016-01-28 17:00 ` Sagi Grimberg
2016-01-23 1:37 ` [PATCH-v2 2/3] target: Fix LUN_RESET active TMR descriptor handling Nicholas A. Bellinger
2016-01-26 17:21 ` Christoph Hellwig
2016-01-23 1:37 ` [PATCH-v2 3/3] target: Fix TAS handling for multi-session se_node_acls Nicholas A. Bellinger
2016-01-26 17:22 ` Christoph Hellwig
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.