Re: [PATCH] scsi: target: tcmu: check cmd aborted earlier

[Bodo Stroesser <bostroesser@gmail.com>]

Hi Liu,

to be honest I don't like this change.

Why do we check CMD_T_ABORTED here? It's just because a concurrently
incoming TMR might already have aborted the cmd, and we don't want to
queue up aborted cmds in tcmu.

But that means, we better check CMD_T_ABORTED after taking cmdr_lock,
because we might sleep a while waiting for the lock, which opens a
possibly long time frame for new TMRs.

So, I'd prefer to have the overhead of alloc/free over not catching
aborted cmds, which causes even more overhead.
  
Bodo

On 08.07.22 05:32, Guixin Liu wrote:
> Lift the check of cmd aborted to the head of tcmu_queue_cmd(), which
> avoids pointless tcmu_cmd allocation if the cmd is already aborted.
> 
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>   drivers/target/target_core_user.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 3deaeec..c1c1d2f 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -1204,16 +1204,18 @@ static int queue_cmd_ring(struct tcmu_cmd *tcmu_cmd, sense_reason_t *scsi_err)
>   	struct se_device *se_dev = se_cmd->se_dev;
>   	struct tcmu_dev *udev = TCMU_DEV(se_dev);
>   	struct tcmu_cmd *tcmu_cmd;
> -	sense_reason_t scsi_ret = TCM_CHECK_CONDITION_ABORT_CMD;
> -	int ret = -1;
> +	sense_reason_t scsi_ret;
> +	int ret;
> +
> +	if (se_cmd->transport_state & CMD_T_ABORTED)
> +		return TCM_CHECK_CONDITION_ABORT_CMD;
>   
>   	tcmu_cmd = tcmu_alloc_cmd(se_cmd);
>   	if (!tcmu_cmd)
>   		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>   
>   	mutex_lock(&udev->cmdr_lock);
> -	if (!(se_cmd->transport_state & CMD_T_ABORTED))
> -		ret = queue_cmd_ring(tcmu_cmd, &scsi_ret);
> +	ret = queue_cmd_ring(tcmu_cmd, &scsi_ret);
>   	if (ret < 0)
>   		tcmu_free_cmd(tcmu_cmd);
>   	else