On Tue, 2017-02-07 at 07:40 -0800, Nicholas A. Bellinger wrote:
> However, there is a basic fatal flaw in this approach
> with this patch as-is that results in se_cmd->finished never being able
> to complete for any se_cmd with CMD_T_ABORTED .
>
> For the above, core_tmr_abort_task() calls __target_check_io_state() to
> obtain a se_cmd->cmd_kref if the descriptor is able to be aborted and
> sets CMD_T_ABORTED. Then, wait_for_completion_timeout() sits in a while
> looping waiting for se_cmd->finished to be completed.
>
> However, the complete_all(&se_cmd->finished) you've added below is only
> invoked from the final se_cmd->cmd_kref callback in
> target_release_cmd_kref().
>
> Which means core_tmr_abort_task() will always be stuck indefinitely on
> se_cmd->finished above, because the subsequent target_put_sess_cmd()
> after the wait_for_completion_timeout() is the caller responsible for
> doing the final kref_put(&se_cmd->cmd_kref, target_release_cmd_kref)
> to perform complete_all(&se_cmd->finished).
A fix for this is in test. I will post a new patch as soon as my tests have
finished.
> > printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for"
> > @@ -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 thing here for TMR abort.
>
> There is no way for target_release_cmd_kref() to ever invoke
> complete_all(&se_cmd->finished) to make forward progress beyond the
> wait_for_completion_timeout() here until the subsequent
> target_put_sess_cmd() is called to drop se_cmd->cmd_kref to zero.
>
> > @@ -340,6 +339,10 @@ static void core_tmr_drain_state_list(
> > */
> > spin_lock_irqsave(&dev->execute_task_lock, flags);
> > list_for_each_entry_safe(cmd, next, &dev->state_list, state_list) {
> > + /* Skip task management functions. */
> > + if (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
> > + continue;
> > +
>
> TMRs are never added to se_device->state_list, so this extra check is
> unnecessary.
OK, I will leave this out.
> > void target_execute_cmd(struct se_cmd *cmd)
> > {
> > /*
> > + * If the received CDB has aleady been aborted stop processing it here.
> > + */
> > + if (transport_check_aborted_status(cmd, 1) != 0)
> > + return;
> > +
> > + /*
> > * Determine if frontend context caller is requesting the stopping of
> > * this command for frontend exceptions.
> > *
> > * If the received CDB has aleady been aborted stop processing it here.
> > */
> > spin_lock_irq(&cmd->t_state_lock);
> > - if (__transport_check_aborted_status(cmd, 1)) {
> > - spin_unlock_irq(&cmd->t_state_lock);
> > - return;
> > - }
> > if (cmd->transport_state & CMD_T_STOP) {
> > pr_debug("%s:%d CMD_T_STOP for ITT: 0x%08llx\n",
> > __func__, __LINE__, cmd->tag);
>
> The use of transport_check_aborted_status(), instead of using
> __transport_check_aborted_status() means se_cmd->t_state_lock is taken
> here twice for every I/O.
>
> There is no reason to take this lock twice, so to avoid additional
> fast-path overhead this part should be avoided.
A later patch makes transport_check_aborted_status() lockless.
> > @@ -2534,10 +2547,8 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
> > if (aborted) {
> > pr_debug("Detected CMD_T_ABORTED for ITT: %llu\n", cmd->tag);
> > wait_for_completion(&cmd->cmd_wait_comp);
> > - cmd->se_tfo->release_cmd(cmd);
> > - ret = 1;
> > }
> > - return ret;
> > + return transport_put_cmd(cmd);
> > }
>
> This code snippet is specific to the second order issue mentioned above
> involving concurrent session shutdown (CMD_T_FABRIC_STOP) and
> CMD_T_ABORTED.
>
> transport_generic_free_cmd() is used by iscsi-target + iser-target
> during session shutdown, and for the second order case existing code
> expects I/O to be quiesced and blocked on ->cmd_wait_comp waiting for
> se_cmd->cmd_kref to drop to zero, before invoking se_tfo->release_cmd()
> directly.
>
> Specifically, existing iscsi-target + iser-target code expects when
> transport_generic_free_cmd() returns the command must no longer be in
> flight, and for the CMD_T_ABORTED + CMD_T_FABRIC_STOP case, se_cmd
> descriptor memory must be released directly.
I will make sure that all target driver work has completed before
transport_generic_free_cmd() returns.
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
