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;
Message-ID: <565fcf28-ec53-8d74-00a3-94be8e5b60e4@oracle.com>
Date:   Mon, 19 Dec 2022 09:23:03 +0000
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH V2] scsi: libsas: Directly kick-off EH when ATA device
 fell off
Content-Language: en-US
To:     Xingui Yang <yangxingui@huawei.com>, jejb@linux.ibm.com,
        martin.petersen@oracle.com, damien.lemoal@opensource.wdc.com,
        linux-ide@vger.kernel.org, hare@suse.com, hch@lst.de
Cc:     linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        linuxarm@huawei.com, prime.zeng@hisilicon.com,
        kangfenglong@huawei.com
References: <20221216100327.7386-1-yangxingui@huawei.com>
From:   John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <20221216100327.7386-1-yangxingui@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dDd3c0J0TWF6OC9PYk1CTnJpTFVSZmFiMndsWjZzb1c4a0hwS0VDb3ZwNG4z?=
 =?utf-8?B?UGZCNThJZkxLRTRGRmpEVFcxWENnRHFkL3JlOVNFRmsxaWNLR3ZZM2FKc1Fs?=
 =?utf-8?B?Sk45UXlldFBVU1lOcXlxMStMbFYveUNUNkR5NERKd3VhZ2FYbTlZTm1INUhB?=
 =?utf-8?B?cGxPTjRKa2oxaldMQllTaHZ4a2d4c3lnZ2NrNU1NbmJRRFQ1bk1OLzJJVmsv?=
 =?utf-8?B?aGlYQWJWbnkrRmM2bTFrZGtkb0JieFZUazJpbUxpN2ZzWmMxSGNQV1ZRRG9w?=
 =?utf-8?B?YmhwVmh2Rk44NHRWUjVzR3dZY2IyMmFqUjZHbkxKOXpMVENIRXlHaENXb3BS?=
 =?utf-8?B?d0d4VWhSUS9UTXoyQUk4TnIxeEN1ek50eWEvc0ZHOTVNZW1acVVKZUsrams1?=
 =?utf-8?B?MSs3cUhVWWZ3dThROGZUK3FkNUJjL3laREdxMzhERnNJVksrK2w1R3h2cnpQ?=
 =?utf-8?B?cjVzQ1BSMVFVdVRBWlJFM1EzUnR5R0dhdW5jMjFlc045N0hPdTdPK2wrUlBl?=
 =?utf-8?B?aVc1NE1OdXhJYTdZNVFhQ1NTOFQ5OTkxSXVKWm9UZThhWkVibXJMUmxLUDdl?=
 =?utf-8?B?THBPVitiMGFScXJVa0ZHcUFKMU1DVzk0ekczM1Nqc2NQUFllNmNWNU5MR0d5?=
 =?utf-8?B?OWM3aTlkeDlTZXlIWGQzTDRZNkhBZHZ0UWlyb25na05mci9FZFVJSURSdk1u?=
 =?utf-8?B?SG1aZ1dVMVNkSWMzME1qajdGT1JNQnl4eFJhM25CdWRuYTVxRDZiK2VSRklk?=
 =?utf-8?B?NmtWemVFTkt1RlNUS20wcyttWTU0T0JiVU1iaFpmL0tnYTYwR1lzZjBWbmRI?=
 =?utf-8?B?REdnWEVza3R3WXVOYWFpa3JIdEJRUmFsMHVzUmhoRm9KdzZBbGRqbzV6U2lT?=
 =?utf-8?B?cXdqc1Flb1VQSnFSMXNFeHBpSDdTZkJra0FMWGVLemszTFFhVU10S2VydXlr?=
 =?utf-8?B?K05xNTBja21xb1l1c1liNjBLN1FrOFpaTk1HbUZNRHdWT05IZVU5MUtrdk5t?=
 =?utf-8?B?amFGU1l5ckhHeHgrMlc0NEpDbTJvaDFQTi9HSDg0TVQwQjVnWkhjS2lZOW9v?=
 =?utf-8?B?Rk1Xd1FUR2xUbUZDQXRuQ1ZPaUZScThXc1l6bi9vSVcyS29rcm1UTjBOanhU?=
 =?utf-8?B?OHdpNUF3cHRzUk0vamxpQUZQaGZIMGdwMjJyUE5EV3hPamNyT25BeDJodFo3?=
 =?utf-8?B?bHdGUTlwMzFkTGYwOWUyVWtUT01EYXhpQU9qQVQ3WUlDT2NiRkhvT3FqMElt?=
 =?utf-8?B?bUNnRVNob3FXYktxRjM2RVRRcktScnBITW5EZm1SemkzWlhyVDh0T1ZXV0hw?=
 =?utf-8?B?YzFmNTNMWmdwbGJFRGVLamVLNHRza1NmN3Q4UGh1elNVRTZIejlWTVlWdGlu?=
 =?utf-8?B?SE5sajA1N25rQUpDOG9wWkkvSEkvdjZkeFpvNkpZZm9OcTlqZEZNdWpmdDUx?=
 =?utf-8?B?cHg1ejQyakROUVU3cGo5cEp0V2ZaaGRUdk16ZWorcE5oMy9xK1UxbGpjSkdN?=
 =?utf-8?B?NmNjdEd2YnBxeXM3NWEwMWhYUHZscDVMZW00eU1hcWUxRnQ2c01oMjBXdi9N?=
 =?utf-8?B?dFZJYlFpZDR5RmhmSUc1c0lCeUhmOC9wcjdiVTZXYmZNL2N3ejlSa0pzOUlt?=
 =?utf-8?B?RFNyWVd2RHlkS3ZORmhVR3VlN0IvL0Q5N0s4QlNaVlQ3VXloVVVaVGtlOExQ?=
 =?utf-8?B?cVZTbm5XWTY3RUVJYUxqdDJUNVkrQWlLZHJ6UFRSVW4ySSs1ZzhEK2cyaWtH?=
 =?utf-8?B?YUpsQXJIem5scmR3b2Y1NXN1c2F3QU9Ock1xUU5UREpsTE8rV2pINFhqT0Nx?=
 =?utf-8?B?SG55am9qQktXZnhldE9jVG9CT0pDcmxZVUNVQ0lHV1U4Z0tQbEVnLzYvMmFM?=
 =?utf-8?B?S0k1OWF5TUhZRWE2TkIvcGEvbVhma2d5N2tUbTZic21zT2VzUnJuZlRmeEd5?=
 =?utf-8?B?cTZlVE5jSndrUDUwWGR6WDdROFVvWnlnWDZxcDcyMnprYVBiNXdoUW5MSGNZ?=
 =?utf-8?B?MGJMR3VWUnEzbUNwZ1EwREpMUjNOUEtoTGY4dDdYWnU0RmEzMm9NQUpNN3A3?=
 =?utf-8?B?aXJQZnBNQmd3WlVUQzMydDlvM3VNNUY1THJtcHJIV2JnMlplMmY1M3Z4SWcr?=
 =?utf-8?B?WjdGNGlpMllIdzBDTWNmR203M2lIREkrUHdFd2oyV0pQeWZTclJiOGc5aXI4?=
 =?utf-8?B?V3c9PQ==?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ed17a9f7-cfba-46e8-03cb-08dae1a2a45e
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Dec 2022 09:23:08.6967
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: gyawhX828cl83Wq3RvcFmR/BSZ+IB82+artnQFW3QV0EgHXsLKRHr2GN5pPhDkVipHbasUYKwJz57/vzgdy70A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB4173
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-12-18_13,2022-12-15_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 adultscore=0 phishscore=0
 mlxlogscore=999 bulkscore=0 malwarescore=0 suspectscore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2212190083
X-Proofpoint-ORIG-GUID: x6wjCMK8UssQd5GbOpzY0gD1zlNNTfHa
X-Proofpoint-GUID: x6wjCMK8UssQd5GbOpzY0gD1zlNNTfHa
X-Spam-Status: No, score=-3.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,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 16/12/2022 10:03, Xingui Yang wrote:
> If the ATA device fell off, call sas_ata_device_link_abort() directly and
> mark all outstanding QCs as failed and kick-off EH Immediately. This avoids
> having to wait for block layer timeouts.
> 
> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
> ---
> Changes to v1:
> - Use dev_is_sata() to check ATA device type
>   drivers/scsi/libsas/sas_discover.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> index d5bc1314c341..a12b65eb4a2a 100644
> --- a/drivers/scsi/libsas/sas_discover.c
> +++ b/drivers/scsi/libsas/sas_discover.c
> @@ -362,6 +362,9 @@ static void sas_destruct_ports(struct asd_sas_port *port)
>   
>   void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *dev)
>   {
> +	if (test_bit(SAS_DEV_GONE, &dev->state) && dev_is_sata(dev))
> +		sas_ata_device_link_abort(dev, false);

Firstly, I think that there is a bug in sas_ata_device_link_abort() -> 
ata_link_abort() code in that the host lock in not grabbed, as the 
comment in ata_port_abort() mentions. Having said that, libsas had 
already some dodgy host locking usage - specifically dropping the lock 
for the queuing path (that's something else to be fixed up ... I think 
that is due to queue command CB calling task_done() in some cases), but 
I still think that sas_ata_device_link_abort() should be fixed (to grab 
the host lock).

Secondly, this just seems like a half solution to the age-old problem - 
that is, EH eventually kicking in only after 30 seconds when a disk is 
removed with active IO. I say half solution as SAS disks still have this 
issue for libsas. Can we instead push to try to solve both of them now?

There was a broad previous discussion on this:
https://urldefense.com/v3/__https://lore.kernel.org/linux-scsi/Ykqg0kr0F*2Fyzk2XW@infradead.org/__;JQ!!ACWV5N9M2RV99hQ!MwAZFXXIwuP0lv-kuUIJ0ekUiGBWlTBhU3oQjyOf_yuP1rHDJb8UKMzJjndXNQ-W1PQGJXzgc0bQUsHh4NGh21EOc50$

 From that discussion, Hannes was doing some related prep work series, 
but I don't think it got completed.

Thanks,
John

> +
>   	if (!test_bit(SAS_DEV_DESTROY, &dev->state) &&
>   	    !list_empty(&dev->disco_list_node)) {
>   		/* this rphy never saw sas_rphy_add */