Re: [v2 1/2] target: allows backend drivers to fail with specific sense codes

[David Disseldorp <ddiss@suse.de>]

Hi Sergey,

On Mon, 26 Jul 2021 18:16:45 +0300, Sergey Samoylenko wrote:

> Currently, backend drivers can fail IO with
> SAM_STAT_CHECK_CONDITION which gets us
> TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE. The patch adds
> a new helper that allows backend drivers to fail with
> specific sense codes.
> 
> This is based on a patch from Mike Christie <michael.christie@oracle.com>.

This looks good and works for me, but I have one comment...

> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: David Disseldorp <ddiss@suse.de>
> [ Moved target_complete_cmd_with_sense() helper to mainline ]
> Signed-off-by: Sergey Samoylenko <s.samoylenko@yadro.com>
> ---
>  drivers/target/target_core_transport.c | 21 ++++++++++++++++++---
>  include/target/target_core_backend.h   |  1 +
>  include/target/target_core_base.h      |  2 ++
>  3 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 26ceabe34de5..d2a2892bda9c 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -736,8 +736,7 @@ static void target_complete_failure_work(struct work_struct *work)
>  {
>  	struct se_cmd *cmd = container_of(work, struct se_cmd, work);
>  
> -	transport_generic_request_failure(cmd,
> -			TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE);
> +	transport_generic_request_failure(cmd, cmd->sense_reason);
>  }
>  
>  /*
> @@ -855,7 +854,8 @@ static bool target_cmd_interrupted(struct se_cmd *cmd)
>  }
>  
>  /* May be called from interrupt context so must not sleep. */
> -void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
> +static void __target_complete_cmd(struct se_cmd *cmd, u8 scsi_status,
> +				  sense_reason_t sense_reason)
>  {
>  	struct se_wwn *wwn = cmd->se_sess->se_tpg->se_tpg_wwn;
>  	int success, cpu;
> @@ -865,6 +865,7 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
>  		return;
>  
>  	cmd->scsi_status = scsi_status;
> +	cmd->sense_reason = sense_reason;
>  
>  	spin_lock_irqsave(&cmd->t_state_lock, flags);
>  	switch (cmd->scsi_status) {
> @@ -893,8 +894,22 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
>  
>  	queue_work_on(cpu, target_completion_wq, &cmd->work);
>  }
> +
> +void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
> +{
> +	__target_complete_cmd(cmd, scsi_status, scsi_status ?
> +			      TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE :
> +			      TCM_NO_SENSE);
> +}
>  EXPORT_SYMBOL(target_complete_cmd);
>  
> +void target_complete_cmd_with_sense(struct se_cmd *cmd,
> +				    sense_reason_t sense_reason)
> +{
> +	__target_complete_cmd(cmd, SAM_STAT_CHECK_CONDITION, sense_reason);
> +}
> +EXPORT_SYMBOL(target_complete_cmd_with_sense);
> +

It's a little unclear from the function prototype that this actually
fails the command with SAM_STAT_CHECK_CONDITION. I could imagine people
erroneously calling target_complete_cmd_with_sense(cmd, TCM_NO_SENSE)
and expecting success.
I think it might be a bit clearer if you just export
__target_complete_cmd() as target_complete_cmd_with_sense() with all
three parameters and leave it up to the caller to flag
CHECK_CONDITION.

Cheers, David