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.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@xxxxxxxx +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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
