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.
>
> 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..?
--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
