On 05/21/15 11:13, Christoph Hellwig wrote:
On Wed, May 20, 2015 at 02:52:25PM +0200, 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.
I like this in general, but you should also mention that this means any
queued command will now be executed. I think that makes sense for
any command already being processed in some way as aborting it out
of that complicated setup has the problems mention above. I think it
might make sense to still cancel anything that is in the workqueue
before entering the real target processing. But for that we'd need
a version of cancel_delayed_work that works on regular work structures.
Hello Christoph,
Had you already noticed the following code at the start of
target_execute_cmd():
/*
* If the received CDB has aleady been aborted stop processing.
*/
if (transport_check_aborted_status(cmd, 1))
return;
Do you think that code is sufficient to prevent execution of an aborted
command ?
Canceling se_cmd.work is not something I'm enthusiast about because the
only way to avoid that that approach triggers a race condition is to
perform all INIT_WORK(&cmd->work, ...) calls before a command is added
to the per-session command list.
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
