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@xxxxxxxxxx>.
This looks good and works for me, but I have one comment...
> Cc: Mike Christie <michael.christie@xxxxxxxxxx>
> Cc: David Disseldorp <ddiss@xxxxxxx>
> [ Moved target_complete_cmd_with_sense() helper to mainline ]
> Signed-off-by: Sergey Samoylenko <s.samoylenko@xxxxxxxxx>
> ---
> 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
