On Fri, 2014-03-14 at 15:09 -0700, 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.
>
> >
> > 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..?
>
Here is an updated version to only call transport_lun_remove_cmd() in
transport_cmd_finish_abort() for the non TMR cases, so that it's also
safe to call from core_tmr_drain_tmr_list().
--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..9f649e3 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
