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:   Niklas Cassel <Niklas.Cassel@wdc.com>
To:     Wu Bo <wubo40@huawei.com>
CC:     Damien Le Moal <damien.lemoal@opensource.wdc.com>,
        "linux-ide@vger.kernel.org" <linux-ide@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "qiuchangqi.qiu@huawei.com" <qiuchangqi.qiu@huawei.com>
Subject: Re: [RFC PATCH] ata: libata-eh: Retry the cmnd when normal complete
 occurrd after scsi timeout
Thread-Topic: [RFC PATCH] ata: libata-eh: Retry the cmnd when normal complete
 occurrd after scsi timeout
Thread-Index: AQHZGuYWLF0ErbPJak+5UhizCm3L1g==
Date:   Wed, 28 Dec 2022 17:59:10 +0000
Message-ID: <Y6yD7Jy0vNr2ZAmE@x1-carbon>
References: <1672220216-46938-1-git-send-email-wubo40@huawei.com>
In-Reply-To: <1672220216-46938-1-git-send-email-wubo40@huawei.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?BCPoKKAw52Kde6yIBGh1BzYavQDWFv8Y/3xRzqV4DYDLcjITRszV8lw1hdAH?=
 =?us-ascii?Q?WQ1GKCrM4knMiYHuLDZC3JPDoH5IJs3JeGTvKYJSa075Zbj/ytlRGBJTC5+L?=
 =?us-ascii?Q?BUjIe0Y8oyZ8zwij/oImju+ScMmXbIb8xvoGHhClsbjt+tniqJyeLMCCPgO6?=
 =?us-ascii?Q?/V6HNsAxwOFsn+rjcfP+vAfmG2IjoPaKa762ztSVTmrrJyD6+xWlNzdUHadb?=
 =?us-ascii?Q?4S4o19e1B4ypHTMYwAIm/x0j/ZdzsJD1KJQkguJpYZPH1BVC9xuhoNW5jBNX?=
 =?us-ascii?Q?LqopXzNRl2GQao79U/rd8bce+lhRvJYq4NAtGnXlTEsDQxIfshaXCUayNu8X?=
 =?us-ascii?Q?iFitCLN0Ul3eyfuU7joliHxVQRzdjEc2KHO/P2KDHvhO0gzObl8QaSvDDaRz?=
 =?us-ascii?Q?GSplZDVv++a/pQ/gDGVApqVNS0ZtuPR0PgM1N5C5JofO2uGW8oCSepMC7KxL?=
 =?us-ascii?Q?OmJ+89iFUDy3IIAR2N2cXi7gzjYp6f2Nr+lH+G6vIbz0RV6KvWCgEl5u/7vX?=
 =?us-ascii?Q?qNvduaP7+U+w2Qq7mzijsNHB3xQnyQNihe15o1YF1p0FAcoEPq3ExO1txGpi?=
 =?us-ascii?Q?QXYiVAfqs0s7kNx2/+m9RYZoq6ZP8oSSbQlT5pg25/M9cclifbG/VXMtIomC?=
 =?us-ascii?Q?ai8XoUYj4eiYHBF8e3D9cxEyV/hKNHys47KMMdtF+59Cz+f2cjhtKdr3qtem?=
 =?us-ascii?Q?LaCpITELyBztQRbbApd4vuaoDeegi/JS/oGCPPF5yQQssClrmahWpC2PhKYd?=
 =?us-ascii?Q?B61rbe01t7hEUqQrRkcjm6TDu0ih3oLHY/CJW5vAgzPE3mEfZo9e0NfRaTa9?=
 =?us-ascii?Q?befO6AnTZlHRCHhD24y+AjZNfW4EniWOJVOX/lkwbg8qG8qnPYZGd/F76OOo?=
 =?us-ascii?Q?nnJ3iVWLBjL+jzbBI05i4U0OF9+s/Y5FQN9g9z1G4DV4b7K/BFRzMom+PZTk?=
 =?us-ascii?Q?XGnSLopikghIarPa0iSMbtqYVTE2fKn791TxQUAz+uJu+n7jNkItoXhFHkOa?=
 =?us-ascii?Q?YT8ov2+2wR+tQDzR2F6Ygw3JdvdMeLNRvUOqmS9AZf1rjonM/3/aSmX+5MYD?=
 =?us-ascii?Q?TNa6hfUqRek4sW3BpUPEvgHCIRttdQ5mH5RuX/ZYH329BMANjxdlOq+9IFHR?=
 =?us-ascii?Q?4Bx85L/9sghnL9pHPIAvqJCqqzqoBoSvHhpPwvnFPXN6Xjghx7ZVLZ73ffrT?=
 =?us-ascii?Q?btjjOwgMdr/gVPPDwBEcti6s1OAhOiQ2rfyCMDw6/OiKrwy6M+9ZBiOyvYlY?=
 =?us-ascii?Q?gPxAJ8qYvXLiQPr1RG3mYvMx/JjZGvHL/7P/GX+xtyPshm/uCQkaQtSVFLce?=
 =?us-ascii?Q?mNYV5nkGoPLBSCpt0qV4XqtFrDrhoITkZ+zsP0TTUkGvPF0E2ZO3bTtl0h8/?=
 =?us-ascii?Q?7Ic6gUklnZOobV2sNPWrF7MzFhp1JHV3wHPanzo19PoOAN2ptU9LPwuIXpTh?=
 =?us-ascii?Q?CQDvbNfogGstSssk7RGWdDK5k0C6Tix+E95C2BLvaFyuaoQl3SDeAYQB3h4i?=
 =?us-ascii?Q?qkeTiS9U1OtcUgs5w4c0lCKTkEb6qzwFyGGe8Qdnq5sK7mQF/SuvPrby/RoO?=
 =?us-ascii?Q?0Hhgp9nESBvwEQL9VtWVWSZex2yBROgrDQsv/sjuTHQaxDHOBUkTv4EVbxdy?=
 =?us-ascii?Q?QQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <F94093310E84CB41A1AFFC250FDCA190@namprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: wdc.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: MN2PR04MB6272.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: acbaabc2-80bf-43b0-3bce-08dae8fd38a8
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Dec 2022 17:59:10.0797
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: MnD2swai1W6QqX5jKmsl2Ch6hr5KZOHSrj4fkaf84mYTEFp5b6GCX3MjLwrFdhS3vgVL/oIct6YnBkP/M+WACQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR04MB6434
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,
        SPF_NONE 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 Wed, Dec 28, 2022 at 05:36:56PM +0800, Wu Bo wrote:
> From: wubo <wubo40@huawei.com>
>=20
> Hi,
>=20
> Now SCSI middle layer EH and normal IO handler can only choose one of the=
m,
> after the SCSI command is completed normally after scsi timeout period,
> Should this scenario be given a chance to retry?
>=20
> Signed-off-by: wubo <wubo40@huawei.com>
> ---
>  drivers/ata/libata-eh.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>=20
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 34303ce..8d1856f 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -617,14 +617,8 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *ho=
st, struct ata_port *ap,
>  					qc->flags |=3D ATA_QCFLAG_FAILED;
>  					nr_timedout++;
>  				}
> -			} else {
> -				/* Normal completion occurred after
> -				 * SCSI timeout but before this point.
> -				 * Successfully complete it.
> -				 */
> -				scmd->retries =3D scmd->allowed;
> +			} else
>  				scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
> -			}
>  		}
>

Hello Wu,


The function comment looks like this:
	/* For new EH, all qcs are finished in one of three ways -
	 * normal completion, error completion, and SCSI timeout.
	 * Both completions can race against SCSI timeout.  When normal
	 * completion wins, the qc never reaches EH.  When error
	 * completion wins, the qc has ATA_QCFLAG_FAILED set.
	 *
	 * When SCSI timeout wins, things are a bit more complex.
	 * Normal or error completion can occur after the timeout but
	 * before this point.  In such cases, both types of
	 * completions are honored.  A scmd is determined to have
	 * timed out iff its associated qc is active and not failed.
	 */

And the code looks like this:

			if (i < ATA_MAX_QUEUE) {
				/* the scmd has an associated qc */
				if (!(qc->flags & ATA_QCFLAG_FAILED)) {
					/* which hasn't failed yet, timeout */
					qc->err_mask |=3D AC_ERR_TIMEOUT;
					qc->flags |=3D ATA_QCFLAG_FAILED;
					nr_timedout++;
				}
			} else {
				/* Normal completion occurred after
				 * SCSI timeout but before this point.
				 * Successfully complete it.
				 */
				scmd->retries =3D scmd->allowed;
				scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
			}


If SCSI timeout wins, but there came an error completion after,
then we will go into the if (i < ATA_MAX_QUEUE) case, but we will
not enter the if !(qc->flags & ATA_QCFLAG_FAILED), as ATA_QCFLAG_FAILED
will already be set by the irq handler. This QC will be completed
by the ata_scsi_port_error_handler(), which gets to run just after
this function has returned:
https://github.com/torvalds/linux/blob/v6.2-rc1/drivers/ata/libata-eh.c#L54=
6


The else clause you are modifying however is for the case where SCSI timeou=
t
wins, but there came a normal completion occurred after the SCSI timeout.

In more detail, what happens first is that scsi_timeout() gets called,
and if scsi timeout wins, it sets SCMD_STATE_COMPLETE:
https://github.com/torvalds/linux/blob/v6.2-rc1/drivers/scsi/scsi_error.c#L=
355
and then schedules EH for that command using scsi_eh_scmd_add().

What happens next in this specific case is that the IRQ handler is called,
takes the ap->lock (which is also taken is this function so that it can run
at the same time as the IRQ handler), then the IRQ handler calls
__ata_qc_complete() for the QC, however, when scsi_done() is finally called
in ata_qc_done() (from IRQ context), it will not be a no-op, since
SCMD_STATE_COMPLETE is already set:
https://github.com/torvalds/linux/blob/v6.2-rc1/drivers/scsi/scsi_lib.c#L16=
23

Since scsi_done() never finished the scsi_cmd, we need to finish it here,
in the else clause, by calling scsi_eh_finish_cmd().

When the EH queue is flushed, it will check if scsi_cmd_retry_allowed()
and if it is, the command will be retried, otherwise it will call scsi_fini=
sh()
on the command:
https://github.com/torvalds/linux/blob/v6.2-rc1/drivers/scsi/scsi_error.c#L=
2150

Considering that we want to only finish the scmd here, libata sets
scmd->retries =3D scmd->allowed; such that the check:
	return ++cmd->retries <=3D cmd->allowed;
in scsi_cmd_retry_allowed() will evaulate to false.


So TL;DR:
It is absolutely essential to set scmd->retries =3D scmd->allowed;
in this else clause, as that is the only reason why this command will
be finished instead of retried.
Since this else clause is for a command that timed out, but got completed
successfully via the IRQ handler after timing out (so the QC got freed),
we only need to finish the scmd. Retrying the scmd is wrong in this case.


Kind regards,
Niklas=