Hi Nicholas,
On 3/18/14, 10:04 PM, Nicholas A. Bellinger wrote:
> On Mon, 2014-03-17 at 19:57 -0700, Alex Leung wrote:
>> On 3/17/14, 11:56 AM, Nicholas A. Bellinger wrote:
>>
>> <SNIP>
>>
>>> Here's an updated patch to drop the transport_send_task_abort() in
>>> core_tmr_abort_task(), and add the transport_cmd_finish_abort() to make
>>> the final target_put_sess_cmd(), invoking TFO->release_cmd() to release
>>> the associated descriptor.
>>>
>>> Care to test..? ;)
>>
>> I'd be happy to -- just give me a day or two. I assume it's okay that
>> I'll be testing with a non-upstream fabric driver (Emulex OCS SDK)?
>>
>
> For this particular patch that's fine, and I'll also confirm the change
> with a couple of other fabric drivers before pushing.
I did some testing with your most recent patch applied to the "for-next"
branch and the following now results in the tfo->release_cmd() path
instead of the tfo->queue_status() with TASK_ABORTED status path.
1. Abort Task
2. LUN Reset with TAS=0
3. LUN Reset with TAS=1 but IT Nexus(cmd) matches IT Nexus(tmr)
To test TAS=1 where the IT Nexuses do not match, I did not have another
initiator handy, so I took a shortcut and changed the
core_tmr_handle_tas_abort() to just be "if (tas)" (removing the IT Nexus
not matching qualifier) and made sure that tfo->queue_status
(TASK_ABORTED) was called -- it was.
So far, so good.
However, it looks transport_check_aborted_status() is also not quite
right. Currently it calls tfo->queue_status() with the TASK_ABORTED
status regardless of TAS and Abort Task v. LUN Reset. I can fabricate
a race condition where, just before tfo->release_cmd() is called
for the abort, the write data phase completes and calls
target_execute_cmd() (which calls transport_check_aborted_status(),
which calls tfo->queue_status()).
I believe transport_check_aborted_status() is for the case where
transport_send_task_abort() is called but the fabric driver is not
ready to send the response, so it returns non-zero for
tfo->write_pending_status(). When the dataphase is complete,
target_execute_cmd() --> transport_check_aborted_status() is called.
If my understanding correct, it seems like CMD_T_ABORTED has
two meanings: 1. the command has been aborted and 2. we wish to send
a task aborted status at a later time (when the data phase has
completed). If we use an se_cmd_flag to distinguish the aborted w/
TASK_ABORTED case from the aborted w/o TASK_ABORTED case, we would not
send an unexpected tfo->queue_status (TASK_ABORTED) in the above case.
The below patch (that includes your changes) works in this case (and
still sends the delayed TASK_ABORTED when appropriate). Thoughts?
>
> Also out of curiosity, what are the plans for this work wrt to
> mainline..?
Not sure I understand your question. Do you mean, what will we do wrt
TMR/TAS etc. for current kernel versions (that use the queue_status()
path)?
Thanks,
Alex
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 70c638f..9f1dd53 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -87,14 +87,21 @@ static void core_tmr_handle_tas_abort(
struct se_cmd *cmd,
int tas)
{
+ bool remove = true;
/*
* TASK ABORTED status (TAS) bit support
*/
if ((tmr_nacl &&
- (tmr_nacl == cmd->se_sess->se_node_acl)) || tas)
+ (tmr_nacl != cmd->se_sess->se_node_acl)) && tas) {
+ remove = false;
transport_send_task_abort(cmd);
+ }
- transport_cmd_finish_abort(cmd, 0);
+ transport_cmd_finish_abort(cmd, remove);
}
static int target_check_cdb_and_preempt(struct list_head *list,
@@ -151,17 +158,13 @@ void core_tmr_abort_task(
cancel_work_sync(&se_cmd->work);
transport_wait_for_tasks(se_cmd);
/*
- * Now send SAM_STAT_TASK_ABORTED status for the referenced
- * se_cmd descriptor..
- */
- transport_send_task_abort(se_cmd);
- /*
* Also deal with possible extra acknowledge reference..
*/
if (se_cmd->se_cmd_flags & SCF_ACK_KREF)
target_put_sess_cmd(se_sess, se_cmd);
target_put_sess_cmd(se_sess, se_cmd);
+ transport_cmd_finish_abort(se_cmd, true);
printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for"
" ref_tag: %d\n", ref_tag);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 0a359fa..4bdcd14 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -603,6 +603,9 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd)
void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
{
+ if (cmd->se_cmd_flags & SCF_SE_LUN_CMD)
+ transport_lun_remove_cmd(cmd);
+
if (transport_cmd_check_stop_to_fabric(cmd))
return;
if (remove)
@@ -2784,13 +2787,16 @@ int transport_check_aborted_status(struct se_cmd *cmd, int send_status)
if (!(cmd->transport_state & CMD_T_ABORTED))
return 0;
- if (!send_status || (cmd->se_cmd_flags & SCF_SENT_DELAYED_TAS))
+ /* if cmd has been aborted but either no status is to be sent or it has
+ * already been sent, just return
+ */
+ if (!send_status || !(cmd->se_cmd_flags & SCF_SEND_DELAYED_TAS))
return 1;
pr_debug("Sending delayed SAM_STAT_TASK_ABORTED status for CDB: 0x%02x ITT: 0x%08x\n",
cmd->t_task_cdb[0], cmd->se_tfo->get_task_tag(cmd));
- cmd->se_cmd_flags |= SCF_SENT_DELAYED_TAS;
+ cmd->se_cmd_flags &= ~SCF_SEND_DELAYED_TAS;
cmd->scsi_status = SAM_STAT_TASK_ABORTED;
trace_target_cmd_complete(cmd);
cmd->se_tfo->queue_status(cmd);
@@ -2804,7 +2810,7 @@ void transport_send_task_abort(struct se_cmd *cmd)
unsigned long flags;
spin_lock_irqsave(&cmd->t_state_lock, flags);
- if (cmd->se_cmd_flags & (SCF_SENT_CHECK_CONDITION | SCF_SENT_DELAYED_TAS)) {
+ if (cmd->se_cmd_flags & (SCF_SENT_CHECK_CONDITION)) {
spin_unlock_irqrestore(&cmd->t_state_lock, flags);
return;
}
@@ -2819,6 +2825,7 @@ void transport_send_task_abort(struct se_cmd *cmd)
if (cmd->data_direction == DMA_TO_DEVICE) {
if (cmd->se_tfo->write_pending_status(cmd) != 0) {
cmd->transport_state |= CMD_T_ABORTED;
+ cmd->se_cmd_flags |= SCF_SEND_DELAYED_TAS;
smp_mb__after_atomic_inc();
return;
}
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index f13fd09..ec3e3a3 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -162,7 +162,7 @@ enum se_cmd_flags_table {
SCF_SENT_CHECK_CONDITION = 0x00000800,
SCF_OVERFLOW_BIT = 0x00001000,
SCF_UNDERFLOW_BIT = 0x00002000,
- SCF_SENT_DELAYED_TAS = 0x00004000,
+ SCF_SEND_DELAYED_TAS = 0x00004000,
SCF_ALUA_NON_OPTIMIZED = 0x00008000,
SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC = 0x00020000,
SCF_ACK_KREF = 0x00040000,
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
