On 3/14/14, 3:09 PM, Nicholas A. Bellinger wrote:
> On Thu, 2014-03-13 at 18:06 -0700, Alex Leung wrote:
>> On 3/13/2014 12:04 PM, Nicholas A. Bellinger wrote:
>>> On Wed, 2014-03-12 at 21:43 +0000, Alex Leung wrote:
>>
>>>> On Wed, 2014-03-12 10:40 AM, Nicholas A. Bellinger wrote:
>>>>> On Tue, 2014-03-11 at 04:27 +0000, Alex Leung wrote:
>>>>>> On Mon, March 10, 2014 6:01 PM, Nicholas A. Bellinger wrote:
>>>>>>> On Fri, 2014-03-07 at 02:04 +0000, Alex Leung wrote:
>>
>> <SNIP>
>>
>>>> Or maybe a "suppress response" bit? So TASK_ABORTED with
>>>> suppress_resp=0 would tell fabric driver to send TASK_ABORTED for the
>>>> FCP_CMND and TASK_ABORTED with suppress_resp=1 would indicate
>>>> to fabric driver to cleanup internally without sending a response. The
>>>> fabric driver would then neither know nor care about what caused the
>>>> abort.
>>>>
>>>
>>> Ok, so given that current TAS logic is incorrect as you describe below,
>>> is this bit still necessary..?
>>>
>>
>> I believe so. More below...
>>
>> <SNIP>
>>
>>> Care to send a (tested) patch that changes core_tmr_handle_tas_abort()
>>> to follow the above..?
>>
>> The problem with simply changing the statement to only call
>> transport_check_aborted_status() when TAS=1 and IT Nexus (cmd) !=
>> IT Nexus (tmr) is, if TAS=0 (or IT Nexus matches), there won't be any
>> notification to the fabric driver that the given se_cmd has been aborted.
>
> That's not entirely true.
>
> So today for all cases the transport_check_aborted_status() codepath via
> transport_cmd_check_stop_to_fabric() -> transport_cmd_check_stop() will
> end up invoking TFO->check_stop_free().
>
> Normally this callback is used by the fabric driver to invoke
> target_put_sess_cmd() to drop the backend completion side
> se_cmd->cmd_kref, while the second target_put_sess_cmd() occurs from the
> fabric acknowledgement side codepath, usually via
> transport_generic_free_cmd() -> transport_put_cmd() ->
> transport_release_cmd() to make the final kref_put + release the
> descriptor + associated resources.
>
Hmm. I don't really see how this path
(transport_check_aborted_status()) comes into play when the
core_tmr_handle_tas_abort() path is taken.
>>
>> What we could do is call core_tmr_handle_tas_abort (probably rename it)
>> no matter what and set a "suppress_response" bit in the se_cmd
>> which is set based on TAS and IT Nexus matching/not matching. That way,
>> the fabric driver will get the notification that the command has been
>> aborted (se_cmd.transport_state & CMD_T_ABORTED) and whether or
>> not a TASK_ABORTED status needs to be sent (!(se_cmd.se_cmd_flags &
>> SCF_SUPPRESS_RESP)).
>>
>> Should I put go down this route and put a patch together?
>>
>
> So after spending some time looking at current code, there is definitely
> a separate bug wrt the transport_check_aborted_status() only case where
> the lun->lun_ref is not decremented via transport_lun_remove_cmd().
>
> Also, I'm not entirely convinced that a SCF_SUPRESS_RESP is still really
> necessary. Consider if transport_cmd_finish_abort() is called with
> remove = 1 to signal that transport_put_cmd() should make the final
> target_put_sess_cmd() call, thus invoking the normal TFO->release_cmd()
> codepath into fabric code.
>
> This would avoid the ->queue_status() + transport_send_task_abort()
> logic for cases where no SAM_STAT_TASK_ABORTED status is sent, but still
> make the TFO->release_cmd() to notify the fabric driver that it should
> release the associated descriptor + resources.
>
> Below is a untested patch for what I think needs to happen based upon
> your original feedback.
>
> WDYT..?
>
Okay, I see what you did where transport_cmd_finish_abort(cmd, 1) will
now be called if transport_send_task_abort() isn't. Couple of questions
here:
Now the LUN Reset path: core_tmr_lun_reset --> core_tmr_drain_state_list
--> core_tmr_handle_tas_abort will have two possible notifications to
the fabric driver that the cmd has been aborted:
1. TFO->queue_status(status=TASK_ABORTED, se_cmd->transport_state &
CMD_T_ABORTED)
2. TFO->release_cmd (se_cmd->transport_state & CMD_T_ABORTED).
Correct?
However, for the Abort Task case (core_tmr_abort_task),
transport_send_task_abort is currently called regardless of TAS. Seems
like this should be removed since TASK_ABORTED won't be sent for the
se_cmd being aborted. Like the LUN Reset (where tas=0) case,
TFO->release_cmd() will be the trigger for the fabric driver to perform
the chip and internal fabric driver cleanup. Agree?
Thanks,
Alex
> --nab
>
> diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
> index 70c638f..b7b1757 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,
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index fb7fac5..6806d08 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -603,6 +603,8 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd)
>
> void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
> {
> + transport_lun_remove_cmd(cmd);
> +
> if (transport_cmd_check_stop_to_fabric(cmd))
> return;
> if (remove)
>
>
--
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
