On Thu, 2017-01-26 at 09:06 +0100, Hannes Reinecke wrote:
> On 01/26/2017 12:36 AM, Bart Van Assche wrote:
> > Instead of invoking target driver callback functions from the
> > context that handles an abort or LUN RESET task management function,
> > only set the abort flag from that context and perform the actual
> > abort handling from the context of the regular command processing
> > flow. This approach has the following advantages:
> > - The task management function code becomes much easier to read and
> > to verify since the number of potential race conditions against
> > the command processing flow is strongly reduced.
> > - It is no longer needed to store the command state into the command
> > itself since that information is no longer needed from the context
> > where a task management function is processed.
> >
> > Notes:
> > - With this patch applied the CMD_T_ABORTED flag is checked at two points
> > by the target core: just before local execution of a command starts
> > (see also target_execute_cmd()) and also just before the response is
> > sent (see also target_complete_cmd()).
> > - Two .release_cmd() calls have been changed into transport_put_cmd()
> > calls to ensure that the 'finished' completion is set before
> > .release_cmd() is called.
> >
> > Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxx>
> > Cc: Andy Grover <agrover@xxxxxxxxxx>
> > Cc: David Disseldorp <ddiss@xxxxxxx>
> > ---
> > drivers/target/target_core_internal.h | 2 -
> > drivers/target/target_core_tmr.c | 73 ++++++++--------
> > drivers/target/target_core_transport.c | 153 +++++++++++++--------------------
> > include/target/target_core_base.h | 2 +
> > 4 files changed, 95 insertions(+), 135 deletions(-)
> >
>
> [ .. ]
> > static int target_check_cdb_and_preempt(struct list_head *list,
> > struct se_cmd *cmd)
> > {
> > @@ -192,13 +173,13 @@ void core_tmr_abort_task(
> > continue;
> > }
> >
> > - list_del_init(&se_cmd->se_cmd_list);
> > + se_cmd->send_abort_response = false;
> > spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> >
> > - cancel_work_sync(&se_cmd->work);
> > - transport_wait_for_tasks(se_cmd);
> > + while (!wait_for_completion_timeout(&se_cmd->finished, 180 * HZ))
> > + pr_debug("ABORT TASK: still waiting for ITT %#llx\n",
> > + ref_tag);
> >
> > - transport_cmd_finish_abort(se_cmd, true);
> > target_put_sess_cmd(se_cmd);
> >
> > printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for"
>
> What happens if the timeout expires here?
> Can you still call target_put_sess_cmd()?
> And is it valid to return TMR_FUNCTION_COMPLETE, seeing that the
> function most definitely has _not_ completed?
>
> > @@ -295,14 +276,31 @@ static void core_tmr_drain_tmr_list(
> > (preempt_and_abort_list) ? "Preempt" : "", tmr_p,
> > tmr_p->function, tmr_p->response, cmd->t_state);
> >
> > - cancel_work_sync(&cmd->work);
> > - transport_wait_for_tasks(cmd);
> > -
> > - transport_cmd_finish_abort(cmd, 1);
> > + while (!wait_for_completion_timeout(&cmd->finished, 180 * HZ))
> > + pr_debug("LUN RESET: still waiting for ITT %#llx\n",
> > + cmd->tag);
> > target_put_sess_cmd(cmd);
> > }
> > }
> >
>
> Same problem as above...
>
> [ .. ]
> > @@ -387,17 +392,9 @@ static void core_tmr_drain_state_list(
> > (cmd->transport_state & CMD_T_STOP) != 0,
> > (cmd->transport_state & CMD_T_SENT) != 0);
> >
> > - /*
> > - * If the command may be queued onto a workqueue cancel it now.
> > - *
> > - * This is equivalent to removal from the execute queue in the
> > - * loop above, but we do it down here given that
> > - * cancel_work_sync may block.
> > - */
> > - cancel_work_sync(&cmd->work);
> > - transport_wait_for_tasks(cmd);
> > -
> > - core_tmr_handle_tas_abort(cmd, tas);
> > + while (!wait_for_completion_timeout(&cmd->finished, 180 * HZ))
> > + pr_debug("LUN RESET: still waiting for cmd with ITT %#llx\n",
> > + cmd->tag);
> > target_put_sess_cmd(cmd);
> > }
> > }
>
> And here, too.
Hello Hannes,
If the timeout expires that means that there is a bug in the target driver
through which the SCSI command has been received, namely a reference leak.
The only purpose of the pr_debug() commands in the above three while
(!wait_for_completion_timeout()) loops is to help target driver developers
with debugging such reference leaks.
Bart.--
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
