Re: [PATCH] scsi_lib: Allow the ALUA transitioning state enough time

[Martin Wilck <mwilck@suse.com>]

On Fri, 2022-07-29 at 14:41 -0700, Brian Bunker wrote:
> The error path for the SCSI check condition of not ready, target in
> ALUA state transition, will result in the failure of that path after
> the retries are exhausted. In most cases that is well ahead of the
> transition timeout established in the SCSI ALUA device handler.
> 
> Instead, reprep the command and re-add it to the queue after a 1
> second
> delay. This will allow the handler to take care of the timeout and
> only fail the path if the target has exceeded the transition expiry
> timeout (default 60 seconds). If the expiry timeout is exceeded, the
> handler will change the path state from transitioning to standby
> leading to a path failure eliminating the potential of this re-prep
> to continue endlessly. In most cases the target will exit the
> transitioning state well before the expiry timeout but after the
> retries are exhausted as mentioned.
> 
> Additionally remove the scsi_io_completion_reprep function which
> provides little value.
> 
> Acked-by: Krishna Kant <krishna.kant@purestorage.com>
> Acked-by: Seamus Connor <sconnor@purestorage.com>
> Signed-off-by: Brian Bunker <brian@purestorage.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

> ---
>  drivers/scsi/scsi_lib.c | 44 +++++++++++++++++++++++----------------
> --
>  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 6ffc9e4258a8..1afb267ff9a2 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -118,7 +118,7 @@ scsi_set_blocked(struct scsi_cmnd *cmd, int
> reason)
>         }
>  }
>  
> -static void scsi_mq_requeue_cmd(struct scsi_cmnd *cmd)
> +static void scsi_mq_requeue_cmd(struct scsi_cmnd *cmd, unsigned long
> msecs)
>  {
>         struct request *rq = scsi_cmd_to_rq(cmd);
>  
> @@ -128,7 +128,12 @@ static void scsi_mq_requeue_cmd(struct scsi_cmnd
> *cmd)
>         } else {
>                 WARN_ON_ONCE(true);
>         }
> -       blk_mq_requeue_request(rq, true);
> +
> +       if (msecs) {
> +               blk_mq_requeue_request(rq, false);
> +               blk_mq_delay_kick_requeue_list(rq->q, msecs);
> +       } else
> +               blk_mq_requeue_request(rq, true);
>  }
>  
>  /**
> @@ -658,14 +663,6 @@ static unsigned int scsi_rq_err_bytes(const
> struct request *rq)
>         return bytes;
>  }
>  
> -/* Helper for scsi_io_completion() when "reprep" action required. */
> -static void scsi_io_completion_reprep(struct scsi_cmnd *cmd,
> -                                     struct request_queue *q)
> -{
> -       /* A new command will be prepared and issued. */
> -       scsi_mq_requeue_cmd(cmd);
> -}
> -
>  static bool scsi_cmd_runtime_exceeced(struct scsi_cmnd *cmd)
>  {
>         struct request *req = scsi_cmd_to_rq(cmd);
> @@ -683,14 +680,21 @@ static bool scsi_cmd_runtime_exceeced(struct
> scsi_cmnd *cmd)
>         return false;
>  }
>  
> +/*
> + * When ALUA transition state is returned, reprep the cmd to
> + * use the ALUA handlers transition timeout. Delay the reprep
> + * 1 sec to avoid aggressive retries of the target in that
> + * state.
> + */
> +#define ALUA_TRANSITION_REPREP_DELAY   1000
> +
>  /* Helper for scsi_io_completion() when special action required. */
>  static void scsi_io_completion_action(struct scsi_cmnd *cmd, int
> result)
>  {
> -       struct request_queue *q = cmd->device->request_queue;
>         struct request *req = scsi_cmd_to_rq(cmd);
>         int level = 0;
> -       enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
> -             ACTION_DELAYED_RETRY} action;
> +       enum {ACTION_FAIL, ACTION_REPREP, ACTION_DELAYED_REPREP,
> +             ACTION_RETRY, ACTION_DELAYED_RETRY} action;
>         struct scsi_sense_hdr sshdr;
>         bool sense_valid;
>         bool sense_current = true;      /* false implies "deferred
> sense" */
> @@ -779,8 +783,8 @@ static void scsi_io_completion_action(struct
> scsi_cmnd *cmd, int result)
>                                         action =
> ACTION_DELAYED_RETRY;
>                                         break;
>                                 case 0x0a: /* ALUA state transition
> */
> -                                       blk_stat = BLK_STS_TRANSPORT;
> -                                       fallthrough;
> +                                       action =
> ACTION_DELAYED_REPREP;
> +                                       break;
>                                 default:
>                                         action = ACTION_FAIL;
>                                         break;
> @@ -839,7 +843,10 @@ static void scsi_io_completion_action(struct
> scsi_cmnd *cmd, int result)
>                         return;
>                 fallthrough;
>         case ACTION_REPREP:
> -               scsi_io_completion_reprep(cmd, q);
> +               scsi_mq_requeue_cmd(cmd, 0);
> +               break;
> +       case ACTION_DELAYED_REPREP:
> +               scsi_mq_requeue_cmd(cmd,
> ALUA_TRANSITION_REPREP_DELAY);
>                 break;
>         case ACTION_RETRY:
>                 /* Retry the same command immediately */
> @@ -933,7 +940,7 @@ static int scsi_io_completion_nz_result(struct
> scsi_cmnd *cmd, int result,
>   * command block will be released and the queue function will be
> goosed. If we
>   * are not done then we have to figure out what to do next:
>   *
> - *   a) We can call scsi_io_completion_reprep().  The request will
> be
> + *   a) We can call scsi_mq_requeue_cmd().  The request will be
>   *     unprepared and put back on the queue.  Then a new command
> will
>   *     be created for it.  This should be used if we made forward
>   *     progress, or if we want to switch from READ(10) to READ(6)
> for
> @@ -949,7 +956,6 @@ static int scsi_io_completion_nz_result(struct
> scsi_cmnd *cmd, int result,
>  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int
> good_bytes)
>  {
>         int result = cmd->result;
> -       struct request_queue *q = cmd->device->request_queue;
>         struct request *req = scsi_cmd_to_rq(cmd);
>         blk_status_t blk_stat = BLK_STS_OK;
>  
> @@ -986,7 +992,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd,
> unsigned int good_bytes)
>          * request just queue the command up again.
>          */
>         if (likely(result == 0))
> -               scsi_io_completion_reprep(cmd, q);
> +               scsi_mq_requeue_cmd(cmd, 0);
>         else
>                 scsi_io_completion_action(cmd, result);
>  }