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 beeing 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.
> +/**
> + * __target_abort_cmd - set abort flag if it has not yet been set
> + * @cmd: Command to be aborted.
> + * @tmr_nacl: Node ACL through which the task management function had been
> + * received. NULL if TASK ABORT STATUS has not been set.
> + * @uncond_cb: Whether to call the callback function upon command completion
> + * if the abort flag had already been set.
> + * @done: Callback function to be called upon command completion.
> + * @done_arg: Argument to be passed to the function @done.
> + */
> +static bool __target_abort_cmd(struct se_cmd *cmd, struct se_node_acl *tmr_nacl,
> + bool uncond_cb, void (*done)(void *),
> + void *done_arg)
> +{
> + bool ret = false;
> +
> + lockdep_assert_held(&cmd->t_state_lock);
I think this function should get the abort_ctx pased n a properly typed
way and also increment the command count. The done argument isn't needed
either as it's alwasy the same. Also it might make sense to move
the logic to find the tmr_nacl into this function as well.
With all that we'll have a much simpler function signature and don't need
the return value at all.
> +static void target_finish_abort(struct cmd_abort_ctx *ctx, const char *action,
> + void (*show)(void *), void *arg)
> +{
> + target_cmd_abort_done(ctx);
> + while (wait_for_completion_interruptible_timeout(&ctx->done, 30 * HZ)
> + <= 0) {
> + pr_info("%s: waiting for %d commands to finish\n", action,
> + atomic_read(&ctx->cmd_count));
> + if (show)
> + show(arg);
> + }
Might be better to just open code this function in the callers, there
is very little code and a huge variance in arguments.
> void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
> {
> struct se_device *dev = cmd->se_dev;
> int success = scsi_status == GOOD;
> unsigned long flags;
>
> - cmd->scsi_status = scsi_status;
> -
> + if (cmd->transport_state & CMD_T_ABORTED) {
> + transport_handle_abort(cmd);
> + return;
> + }
Do we really want to call transport_handle_abort from irq context?
I think it should be called from workqueue context like all the
other completion functionality.
--
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
