>I believe these retries should be skipped by returning SUCCESS from
alua_check_sense(). But that can be done in a separate patch.
Maybe. There might be targets that return ALUA state transition for
such a short interval that they could still be helped by the
needs_retry. For us those retries don't really help since our
transition time is in the order of a few seconds.
>I agree, meanwhile :-) We still have to see how this works together
with dm-multipath, for different storage devices with different
transitioning semantics.
I don't think it will be a problem. Until pretty recently, the ALUA
transition state just returned BLK_STS_RESOURCE in the prep_fn which
led to the commands not even being sent to the target while in that
state. So with this patch the requests will be sent to the target at 1
second intervals as long as the target responds with transitioning
state. The experience for the multipath layer will be the same as it
was before the patch to fix the misbehaving targets that stayed in the
transition state too long leading us to this point. With this patch,
the misbehaving targets are still handled, but the path is continually
retried up to the transition timeout which I think is the right thing
to do.
>Note that this would change with Hannes' latest patch.
("scsi_dh_alua: mark port group as failed after ALUA transitioning
timeout").
This will still result in a path failure when the transition timeout
is exceeded I think.
Thanks,
Brian
On Wed, Jun 8, 2022 at 10:13 AM Martin Wilck <martin.wilck@xxxxxxxx> wrote:
>
> On Tue, 2022-06-07 at 12:58 -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 in the transition state if the target has exceeded
> > the transition timeout (default 60 seconds).
> >
> > Acked-by: Krishna Kant <krishna.kant@xxxxxxxxxxxxxxx>
> > Acked-by: Seamus Connor <sconnor@xxxxxxxxxxxxxxx>
> > Signed-off-by: Brian Bunker <brian@xxxxxxxxxxxxxxx>
>
> Reviewed-by: Martin Wilck <mwilck@xxxxxxxx>
>
> > ---
> > 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);
> > }
>
--
Brian Bunker
PURE Storage, Inc.
brian@xxxxxxxxxxxxxxx
