On Sat, 2014-03-15 at 10:20 -0700, Alex Leung wrote:
>
> 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.
>
Er, sorry. I meant to say transport_cmd_finish_abort() gets called for
both code paths..
> >>
> >> 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?
So the TFO->queue_status() only happens to notify the fabric that a
TASK_ABORTED status will be sent, and not that the descriptor should be
released.
TFO->release_cmd() will be called via target_put_sess_cmd() upon the
last kref_put() for both TASK_ABORTED status and no TASK_ABORTED status
cases, but at different times.
Eg: For the TASK_ABORTED status case, the fabric driver calls
transport_generic_free_cmd() after the TASK_ABORTED response has been
acknowledged, thus invoking transport_put_cmd() transport_release_cmd()
-> target_put_sess_cmd() -> TFO->release_cmd()
For the no TASK_ABORTED status case, transport_cmd_finish_abort() ->
transport_put_cmd() -> target_put_sess_cmd() will invoke
TFO->release_cmd() directly from TMR logic.
>
> 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?
>
<nod>, makes sense.
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..? ;)
--nab
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 70c638f..021045e 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,
@@ -151,17 +154,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..8719bbf 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)
--
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
