On 3/19/14, 9:25 PM, Alex Leung wrote:
> 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?
>
I noticed something else during my testing. For Abort Task, there's a
extra target_put_sess_cmd() that I believe now needs to be removed as a
result of adding the call to transport_cmd_finish_abort() in
core_tmr_abort_task(). I'm also not sure the kref_get,
target_put_sess_cmd pair that used to surround the call to
the now removed transport_send_task_abort() is necessary either, but
I left that. See updated patch below.
The extra kref_put results tfo->release_cmd() being called during
"check_stop_to_fabric" in transport_cmd_finish_abort() for Abort Task
(unexpected), but during "transport_put_cmd()" (expected) for LUN Reset:
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);
// tfo->release_cmd() called here for Task Abort
if (transport_cmd_check_stop_to_fabric(cmd))
return;
if (remove)
// tfo->release_cmd() called here for LUN Reset
transport_put_cmd(cmd);
}
If we're in agreement so far, I have a question about the
tfo->release_cmd() path in general. By the time the fabric driver gets
the release_cmd(), the SGL (se_cmd->t_data_sg) has been freed and it is
too late for the fabric driver to unmap it (if it were previously
mapped by the fabric). check_stop_free() seems to be my last chance
to do the unmap while the SGL still exists. Is this the right place?
Thanks,
Alex
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 70c638f..3f0338f 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -87,14 +87,17 @@ 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,
@@ -150,18 +153,9 @@ 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
