On 03/01/2018 04:26 PM, Bart Van Assche wrote:
> Instead of embedding the completion that is used for waiting for
> command completion in struct se_cmd, let the context that waits for
> command completion allocate it. This makes it possible to have a
> single code path for non-aborted and aborted commands in
> target_release_cmd_kref() and avoids that transport_generic_free_cmd()
> has to call cmd->se_tfo->release_cmd() directly. This patch does not
> change any functionality. Note: transport_generic_free_cmd() only
> waits until the se_cmd reference count has reached zero after it has
> set both CMD_T_FABRIC_STOP and CMD_T_ABORTED.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Mike Christie <mchristi@xxxxxxxxxx>
> ---
> drivers/target/target_core_transport.c | 24 ++++++++----------------
> include/target/target_core_base.h | 2 +-
> 2 files changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index c8f19a143f5c..d7b0b2f4261e 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1276,7 +1276,7 @@ void transport_init_se_cmd(
> INIT_LIST_HEAD(&cmd->se_cmd_list);
> INIT_LIST_HEAD(&cmd->state_list);
> init_completion(&cmd->t_transport_stop_comp);
> - init_completion(&cmd->cmd_wait_comp);
> + cmd->compl = NULL;
> spin_lock_init(&cmd->t_state_lock);
> INIT_WORK(&cmd->work, NULL);
> kref_init(&cmd->cmd_kref);
> @@ -2596,6 +2596,7 @@ static void target_wait_free_cmd(struct se_cmd *cmd, bool *aborted, bool *tas)
> */
> int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
> {
> + DECLARE_COMPLETION_ONSTACK(compl);
> int ret = 0;
> bool aborted = false, tas = false;
>
> @@ -2614,12 +2615,13 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
> if (cmd->se_lun)
> transport_lun_remove_cmd(cmd);
> }
> + if (aborted)
I see that in the current code we always do a wait if the CMD_T_ABORTED
bit was set, so your patch matches that. However, is the original code
correct? In target_release_cmd_kref below we only do a wake up if both
CMD_T_FABRIC_STOP and CMD_T_ABORTED is set. If wait_for_tasks=false and
CMD_T_ABORTED was set, we would end up waiting forever. Is that right or
can it never happen?
........
> }
> -
> - spin_lock(&se_cmd->t_state_lock);
> - fabric_stop = (se_cmd->transport_state & CMD_T_FABRIC_STOP) &&
> - (se_cmd->transport_state & CMD_T_ABORTED);
> - spin_unlock(&se_cmd->t_state_lock);
> -
> - if (fabric_stop) {
> - spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> - target_free_cmd_mem(se_cmd);
> - complete(&se_cmd->cmd_wait_comp);
> - return;
> - }
--
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
