Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Martin Wilck <martin.wilck@suse.com>
To:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "brian@purestorage.com" <brian@purestorage.com>
CC:     "krishna.kant@purestorage.com" <krishna.kant@purestorage.com>,
        "sconnor@purestorage.com" <sconnor@purestorage.com>,
        "hch@infradead.org" <hch@infradead.org>,
        "hare@suse.de" <hare@suse.de>, "mwilck@suse.de" <mwilck@suse.de>
Subject: Re: [PATCH] scsi_lib: Don't fail the path in ALUA transition state
Thread-Topic: [PATCH] scsi_lib: Don't fail the path in ALUA transition state
Thread-Index: AQHYe1sfwymmLN8NMUyPKNpnwN1BWA==
Date:   Wed, 8 Jun 2022 17:13:51 +0000
Message-ID: <773e57f47ecfe98d2a89fb0611696a754c55505f.camel@suse.com>
References: <20220607195800.43485-1-brian@purestorage.com>
In-Reply-To: <20220607195800.43485-1-brian@purestorage.com>
Accept-Language: en-US
Content-Language: en-US
user-agent: Evolution 3.44.2 
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?iso-8859-15?Q?TNJtw8qvL2j/p4SmIdJ4//AJ0CNqXAgUljTvA+4KBiFtPxzhv57cu44ke?=
 =?iso-8859-15?Q?LbfLAytwfyW2WPfQEPXa5a7U+/4Ho3tYILliSls50O7UNYTsLGHYUKELs?=
 =?iso-8859-15?Q?f1HRZslaegTSjbj1WHxI1McgKA7ZvXJ/cZWdFUynrRma00Y5FdRzk6A6F?=
 =?iso-8859-15?Q?g5Tup9Oe8MVvUu3sOBsxLkPFOuy7MEbIV1baIlkFp+0bM5dt2vRhBRCzO?=
 =?iso-8859-15?Q?TdaDEdaifz+pwSt8ZmHbBoE99gk0GWES7hdXTZOGDm21ItZMaexFNndPT?=
 =?iso-8859-15?Q?mTjcYeZH+jBdbDbuw9SjHfUe6Cl/z3WKLP2eSJGtfg1JAe7hX/CoXeApj?=
 =?iso-8859-15?Q?z3GDv2RaYxNms9RCTsTn0dep1Mq9UtsGVtdd+o1eH8wNfQq094IeMIH4+?=
 =?iso-8859-15?Q?MhFYH3NMgroonEV+K+NvaP5Jm8DLWyewUznmkYBRCV/4T/GHxOoRnqONj?=
 =?iso-8859-15?Q?k7uBQmaccs5xtTpq/1Z7inn+LFMSke0VXchLvJBU5V6hWUJauj/A2aCEs?=
 =?iso-8859-15?Q?t7sUn2GsS5IUgWyOn1eKVyCSs3CepljN3BKuZat8U0Su1qV9pnP60A5kQ?=
 =?iso-8859-15?Q?7U3YREFtl3j0QHbryuiZvToUUBWSNMLpFCHqilM7bHg7vU4NBChSlcaNM?=
 =?iso-8859-15?Q?vcsb5gC6dGrfscgyNE4dv2mRTVlrPJl1ZcWM8XOJeOkZjOZ646pxE3Ang?=
 =?iso-8859-15?Q?HB3OjAYd3knREu1w/6y/43TAT/j8/KYnG1nGnTVFGryjmKwDVdYRjSMsN?=
 =?iso-8859-15?Q?kP6OL9H/xDnJxaY8qwKb8C/GTltzA0HcBOxH8VwGMHD0mapQcuCG562Hs?=
 =?iso-8859-15?Q?etxrjPoGudMe5q5mOw3ewdvR3PKm/ejmlgXGEb4CM9Bw2TqQiM5+PNanL?=
 =?iso-8859-15?Q?zSNjnGaR5JXowgS/YFJdAXNyDuL3hIFIUKDCfhosPUIXN/CQAAsk/1N7Q?=
 =?iso-8859-15?Q?yGrCTsg/vDVW5bgKndcaTaCWCdzuhHPQAcr0Mv/JZNFV3TOoVd5cqsTeT?=
 =?iso-8859-15?Q?3GPuy5mehAZb07g+fwFwOxw1ctCrfPcpy1I+JZ88P7DErn3jRMuelpJio?=
 =?iso-8859-15?Q?hhqV6IHXpgdOwxn74q1igG+PP08wSxUn2c+GrpVzPl4koyKqYMGsxl+H4?=
 =?iso-8859-15?Q?i/LoRqY0zZsxFeiSX8PDu6hcXTz7Ow46BXAAi4nAvZo41hMKthhNGufnP?=
 =?iso-8859-15?Q?6Go3hZZLKEvEQ2efSoTiodlSMpOVv3aqBJljxeKNaqZHDR7XC4alvVMHG?=
 =?iso-8859-15?Q?glXPU/5dSIBTbg4FvJE679rntMf4aum60oKyWjP2Hl+w2PJh/4eIs1ne2?=
 =?iso-8859-15?Q?YPTSmObCCpuW79eXKoD5YlqReeiU38s/kt1tm9frKvAHTye6hZjtqvoI5?=
 =?iso-8859-15?Q?WNHD0T0+1/A3hJM6vHWAeSypzLZGET3LjqlflbJOzSl9iDRi0okQzhGkU?=
 =?iso-8859-15?Q?efw8V8XqrWgyTFnmEGujiRUATKTN3iMxBUERDdvDiSD/98bxvcfuhLNIm?=
 =?iso-8859-15?Q?zUJ+4o6odv4RN+RjxDDQ1GModcryXM1KYk6b5oF8CJcqHcE8BIQWVjO0L?=
 =?iso-8859-15?Q?1oDUtTeRw8UCNy/FnMib9UnYV6hwuLc5xjXM9yetaIc9QTlLN0vbttHZL?=
 =?iso-8859-15?Q?wQJ1l8jH3pieh+H6yAlX95aCvv6UHTDmGgvj/CPitGrUp8h6m7n0H9gxz?=
 =?iso-8859-15?Q?JnqovvB0/R7q1UWTkSXBOl2KF515zzWEu2kSeWsIZqBqNioHMaUUbjj8C?=
 =?iso-8859-15?Q?sW7CYpQz5L2tgGkl5aXAObFYslZv3z+u1mNlqYHszsELi8iJed1kS/XOz?=
 =?iso-8859-15?Q?dRZ6dFxrcL2KIGbcS7ouQ3Wuh4Q6KxxVOiLStEwzkgvvvVVgi8RtDlkBS?=
 =?iso-8859-15?Q?EV1c=3D?=
Content-Type: text/plain; charset="iso-8859-15"
Content-ID: <5E9285D63F1FE84AB329A7D09C9C1592@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DB8PR04MB6555.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a4d9df04-09d2-43ca-610f-08da49724272
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Jun 2022 17:13:51.5925
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: YgYLjcROrYhJDC9IoHaLmT7EIXXg8g1v8P4O6D2KtwkOeUXfOgocPKMA2Ll4mfO2phqpV7MRrVC14y9CxLpiMw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR04MB9048
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,
        SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.
>=20
> 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).
>=20
> 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>

> ---
> =A0drivers/scsi/scsi_lib.c | 44 +++++++++++++++++++++++----------------
> --
> =A01 file changed, 25 insertions(+), 19 deletions(-)
>=20
> 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)
> =A0=A0=A0=A0=A0=A0=A0=A0}
> =A0}
> =A0
> -static void scsi_mq_requeue_cmd(struct scsi_cmnd *cmd)
> +static void scsi_mq_requeue_cmd(struct scsi_cmnd *cmd, unsigned long
> msecs)
> =A0{
> =A0=A0=A0=A0=A0=A0=A0=A0struct request *rq =3D scsi_cmd_to_rq(cmd);
> =A0
> @@ -128,7 +128,12 @@ static void scsi_mq_requeue_cmd(struct scsi_cmnd
> *cmd)
> =A0=A0=A0=A0=A0=A0=A0=A0} else {
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0WARN_ON_ONCE(true);
> =A0=A0=A0=A0=A0=A0=A0=A0}
> -=A0=A0=A0=A0=A0=A0=A0blk_mq_requeue_request(rq, true);
> +
> +=A0=A0=A0=A0=A0=A0=A0if (msecs) {
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0blk_mq_requeue_request(rq, =
false);
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0blk_mq_delay_kick_requeue_l=
ist(rq->q, msecs);
> +=A0=A0=A0=A0=A0=A0=A0} else
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0blk_mq_requeue_request(rq, =
true);
> =A0}
> =A0
> =A0/**
> @@ -658,14 +663,6 @@ static unsigned int scsi_rq_err_bytes(const
> struct request *rq)
> =A0=A0=A0=A0=A0=A0=A0=A0return bytes;
> =A0}
> =A0
> -/* Helper for scsi_io_completion() when "reprep" action required. */
> -static void scsi_io_completion_reprep(struct scsi_cmnd *cmd,
> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct request_queue *q)
> -{
> -=A0=A0=A0=A0=A0=A0=A0/* A new command will be prepared and issued. */
> -=A0=A0=A0=A0=A0=A0=A0scsi_mq_requeue_cmd(cmd);
> -}
> -
> =A0static bool scsi_cmd_runtime_exceeced(struct scsi_cmnd *cmd)
> =A0{
> =A0=A0=A0=A0=A0=A0=A0=A0struct request *req =3D scsi_cmd_to_rq(cmd);
> @@ -683,14 +680,21 @@ static bool scsi_cmd_runtime_exceeced(struct
> scsi_cmnd *cmd)
> =A0=A0=A0=A0=A0=A0=A0=A0return false;
> =A0}
> =A0
> +/*
> + * 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=A0=A0=A01000
> +
> =A0/* Helper for scsi_io_completion() when special action required. */
> =A0static void scsi_io_completion_action(struct scsi_cmnd *cmd, int
> result)
> =A0{
> -=A0=A0=A0=A0=A0=A0=A0struct request_queue *q =3D cmd->device->request_qu=
eue;
> =A0=A0=A0=A0=A0=A0=A0=A0struct request *req =3D scsi_cmd_to_rq(cmd);
> =A0=A0=A0=A0=A0=A0=A0=A0int level =3D 0;
> -=A0=A0=A0=A0=A0=A0=A0enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 ACTION_DELAYED_RETRY} action;
> +=A0=A0=A0=A0=A0=A0=A0enum {ACTION_FAIL, ACTION_REPREP, ACTION_DELAYED_RE=
PREP,
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 ACTION_RETRY, ACTION_DELAYED_RETRY}=
 action;
> =A0=A0=A0=A0=A0=A0=A0=A0struct scsi_sense_hdr sshdr;
> =A0=A0=A0=A0=A0=A0=A0=A0bool sense_valid;
> =A0=A0=A0=A0=A0=A0=A0=A0bool sense_current =3D true;=A0=A0=A0=A0=A0 /* fa=
lse implies "deferred
> sense" */
> @@ -779,8 +783,8 @@ static void scsi_io_completion_action(struct
> scsi_cmnd *cmd, int result)
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0action =3D
> ACTION_DELAYED_RETRY;
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0break;
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0case 0x0a: /* ALUA state transition
> */
> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0blk_stat =3D BLK_STS_TRANSPORT=
;
> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0fallthrough;
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0action =3D
> ACTION_DELAYED_REPREP;
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0break;
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0default:
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0action =3D ACTION_FAIL;
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0break;
> @@ -839,7 +843,10 @@ static void scsi_io_completion_action(struct
> scsi_cmnd *cmd, int result)
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0r=
eturn;
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0fallthrough;
> =A0=A0=A0=A0=A0=A0=A0=A0case ACTION_REPREP:
> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0scsi_io_completion_reprep(c=
md, q);
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0scsi_mq_requeue_cmd(cmd, 0)=
;
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0break;
> +=A0=A0=A0=A0=A0=A0=A0case ACTION_DELAYED_REPREP:
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0scsi_mq_requeue_cmd(cmd,
> ALUA_TRANSITION_REPREP_DELAY);
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0break;
> =A0=A0=A0=A0=A0=A0=A0=A0case ACTION_RETRY:
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0/* Retry the same command=
 immediately */
> @@ -933,7 +940,7 @@ static int scsi_io_completion_nz_result(struct
> scsi_cmnd *cmd, int result,
> =A0 * command block will be released and the queue function will be
> goosed. If we
> =A0 * are not done then we have to figure out what to do next:
> =A0 *
> - *=A0=A0 a) We can call scsi_io_completion_reprep().=A0 The request will
> be
> + *=A0=A0 a) We can call scsi_mq_requeue_cmd().=A0 The request will be
> =A0 *=A0=A0=A0=A0=A0unprepared and put back on the queue.=A0 Then a new c=
ommand
> will
> =A0 *=A0=A0=A0=A0=A0be created for it.=A0 This should be used if we made =
forward
> =A0 *=A0=A0=A0=A0=A0progress, or if we want to switch from READ(10) to RE=
AD(6)
> for
> @@ -949,7 +956,6 @@ static int scsi_io_completion_nz_result(struct
> scsi_cmnd *cmd, int result,
> =A0void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int
> good_bytes)
> =A0{
> =A0=A0=A0=A0=A0=A0=A0=A0int result =3D cmd->result;
> -=A0=A0=A0=A0=A0=A0=A0struct request_queue *q =3D cmd->device->request_qu=
eue;
> =A0=A0=A0=A0=A0=A0=A0=A0struct request *req =3D scsi_cmd_to_rq(cmd);
> =A0=A0=A0=A0=A0=A0=A0=A0blk_status_t blk_stat =3D BLK_STS_OK;
> =A0
> @@ -986,7 +992,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd,
> unsigned int good_bytes)
> =A0=A0=A0=A0=A0=A0=A0=A0 * request just queue the command up again.
> =A0=A0=A0=A0=A0=A0=A0=A0 */
> =A0=A0=A0=A0=A0=A0=A0=A0if (likely(result =3D=3D 0))
> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0scsi_io_completion_reprep(c=
md, q);
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0scsi_mq_requeue_cmd(cmd, 0)=
;
> =A0=A0=A0=A0=A0=A0=A0=A0else
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0scsi_io_completion_action=
(cmd, result);
> =A0}