Re: [PATCH] scsi_dh_alua: mark port group as failed after ALUA transitioning timeout

[Martin Wilck <mwilck@suse.com>]

On Wed, 2022-05-25 at 21:33 +0200, Martin Wilck wrote:
> On Wed, 2022-05-25 at 14:07 +0200, Hannes Reinecke wrote:
> > On 5/25/22 13:20, Martin Wilck wrote:
> > > On Wed, 2022-05-25 at 10:11 +0200, Hannes Reinecke wrote:
> > > > When ALUA transitioning timeout triggers the path group state
> > > > must
> > > > be considered invalid. So add a new flag ALUA_PG_FAILED to
> > > > indicate
> > > > that the path state isn't necessarily valid, and keep the
> > > > existing
> > > > path state until we get a valid response from a RTPG.
> > > > 
> > > > Cc: Brian Bunker <brian@purestorage.com>
> > > > Cc: Martin Wilck <mwilck@suse.de>
> > > > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > > > ---
> > > >   drivers/scsi/device_handler/scsi_dh_alua.c | 24 +++++++------
> > > > --
> > > > -----
> > > > --
> > > >   1 file changed, 7 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
> > > > b/drivers/scsi/device_handler/scsi_dh_alua.c
> > > > index 1d9be771f3ee..6921490a5e65 100644
> > > > --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> > > > +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> > > > @@ -49,6 +49,7 @@
> > > >   #define ALUA_PG_RUN_RTPG               0x10
> > > >   #define ALUA_PG_RUN_STPG               0x20
> > > >   #define ALUA_PG_RUNNING                        0x40
> > > > +#define ALUA_PG_FAILED                 0x80
> > > >   
> > > >   static uint optimize_stpg;
> > > >   module_param(optimize_stpg, uint, S_IRUGO|S_IWUSR);
> > > > @@ -420,7 +421,7 @@ static enum scsi_disposition
> > > > alua_check_sense(struct scsi_device *sdev,
> > > >                           */
> > > >                          rcu_read_lock();
> > > >                          pg = rcu_dereference(h->pg);
> > > > -                       if (pg)
> > > > +                       if (pg && !(pg->flags &
> > > > ALUA_PG_FAILED))
> > > >                                  pg->state =
> > > > SCSI_ACCESS_STATE_TRANSITIONING;
> > > >                          rcu_read_unlock();
> > > >                          alua_check(sdev, false);
> > > 
> > > You still return NEEDS_RETRY from alua_check_sense(), even if
> > > ALUA_PG_FAILED is set?
> > > 
> > > > @@ -694,7 +695,7 @@ static int alua_rtpg(struct scsi_device
> > > > *sdev,
> > > > struct alua_port_group *pg)
> > > >   
> > > >    skip_rtpg:
> > > >          spin_lock_irqsave(&pg->lock, flags);
> > > > -       if (transitioning_sense)
> > > > +       if (transitioning_sense && !(pg->flags &
> > > > ALUA_PG_FAILED))
> > > >                  pg->state = SCSI_ACCESS_STATE_TRANSITIONING;
> > > >   
> > > >          if (group_id_old != pg->group_id || state_old != pg-
> > > > > state ||
> > > > @@ -718,23 +719,10 @@ static int alua_rtpg(struct scsi_device
> > > > *sdev,
> > > > struct alua_port_group *pg)
> > > >                          pg->interval = ALUA_RTPG_RETRY_DELAY;
> > > >                          err = SCSI_DH_RETRY;
> > > >                  } else {
> > > > -                       struct alua_dh_data *h;
> > > > -
> > > > -                       /* Transitioning time exceeded, set
> > > > port
> > > > to
> > > > standby */
> > > > +                       /* Transitioning time exceeded, mark pg
> > > > as
> > > > failed */
> > > >                          err = SCSI_DH_IO;
> > > > -                       pg->state = SCSI_ACCESS_STATE_STANDBY;
> > > > +                       pg->flags |= ALUA_PG_FAILED;
> > > >                          pg->expiry = 0;
> > > > -                       rcu_read_lock();
> > > > -                       list_for_each_entry_rcu(h, &pg-
> > > > >dh_list,
> > > > node) {
> > > > -                               if (!h->sdev)
> > > > -                                       continue;
> > > > -                               h->sdev->access_state =
> > > > -                                       (pg->state &
> > > > SCSI_ACCESS_STATE_MASK);
> > > > -                               if (pg->pref)
> > > > -                                       h->sdev->access_state
> > > > |=
> > > > -
> > > >                                                 SCSI_ACCESS_STA
> > > > TE
> > > > _PREFE
> > > > RRED;
> > > > -                       }
> > > > -                       rcu_read_unlock();
> > > >                  }
> > > >                  break;
> > > >          case SCSI_ACCESS_STATE_OFFLINE:
> > > > @@ -746,6 +734,8 @@ static int alua_rtpg(struct scsi_device
> > > > *sdev,
> > > > struct alua_port_group *pg)
> > > >                  /* Useable path if active */
> > > >                  err = SCSI_DH_OK;
> > > >                  pg->expiry = 0;
> > > > +               /* RTPG succeeded, clear ALUA_PG_FAILED */
> > > > +               pg->flags &= ~ALUA_PG_FAILED;
> > > 
> > > Shouldn't this be done for any state except TRANSITIONING?
> > > 
> > Why, but it does.
> > We're only entering this block if the state is not TRANSITIONING.
> > (It's part of a 'switch' statement)
> 
> Right, the only exception is SCSI_ACCESS_STATE_OFFLINE, for which the
> ALUA_PG_FAILED state should of cause persist. Sorry for bothering.
> 

What I'm still missing here is how we avoid sending further I/O down
the path after the timeout has expired.

alua_check_sense() will return NEEDS_RETRY, even if ALUA_PG_FAILED is
set. Without this patch, alua_prep_fn() would return an error because
it would see STANDBY state. With this patch, the state will stay
TRANSITIONING, and (after 6056a92ceb2a ("scsi: scsi_dh_alua: Properly
handle the ALUA transitioning state")) alua_prep_fn() will return
BLK_STS_OK.

IMO alua_check_sense() should return SUCCESS if ALUA_PG_FAILED is set,
and alua_prep_fn() should return an error.

Regards
Martin