From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D0ECDC001B2 for ; Mon, 19 Dec 2022 22:59:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232444AbiLSW7O (ORCPT ); Mon, 19 Dec 2022 17:59:14 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52692 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231702AbiLSW7N (ORCPT ); Mon, 19 Dec 2022 17:59:13 -0500 Received: from esa6.hgst.iphmx.com (esa6.hgst.iphmx.com [216.71.154.45]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C07279FC5 for ; Mon, 19 Dec 2022 14:59:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=wdc.com; i=@wdc.com; q=dns/txt; s=dkim.wdc.com; t=1671490752; x=1703026752; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=nBOnBM2AbYb8rMTgdV7VKM1gSLrZ9214921NLXhZivI=; b=F7pCc8TxoNQXJedG2BsSB2L2vsjFLULKuWlzoENUxM0tUsN2Wr/5lrOO vrIZNSjOd5lKxQUME0JbogU+lWcZ/jIthYfmV9MtiD7xuojkpU1RlYcQS N9JeI1w5qDD9+irbCADLpYLR3oy2j+nnwbwf1STbkE8lmzriZLJj8YL4/ Uo0Th8K6YRr92CA2pJ21KzC4FPeIRKE8DSU7IRxTWGghFIJlz5ILM+oCB WfpxCo1TYB5Oy16leQj8C5Ci6uUVeW+DlNifCtF1z/uH2kcNwfQLXGJq6 iu7ZRDMz8+xDtZ/qkG/cE/rs9smfpyGQv1l+rALf44cliI31IyPxxiXIT A==; X-IronPort-AV: E=Sophos;i="5.96,257,1665417600"; d="scan'208";a="219253040" Received: from uls-op-cesaip01.wdc.com (HELO uls-op-cesaep01.wdc.com) ([199.255.45.14]) by ob1.hgst.iphmx.com with ESMTP; 20 Dec 2022 06:59:10 +0800 IronPort-SDR: 9NtK+RrBPWbG63Qgb6ccoLVKe/BNk14NA6XlenRCCSRecP4dSjtynqYzyn7cXkQCYoYMKt4+9Q bypetCC3QSIFvVYs2geuORRFqtSmYdxebXXVuKXVVIwolzPZsPKaarOtJ8zS7rhBdnZ+uxUsX3 Yy8ktdQVrDzwRmRMjmegPdWJvjafs8CrdQnP8L8oq/X3sPMIBuVS+5lO1yj1gb+CEzrNObP8Tb IHi1w8kg1irMx/FlJ6LVuoPEsrTgHnCc/osY6nsaSk+yYxV/kIB7FMnZiByS2YsRBRoKrkhczv eYI= Received: from uls-op-cesaip02.wdc.com ([10.248.3.37]) by uls-op-cesaep01.wdc.com with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256; 19 Dec 2022 14:17:27 -0800 IronPort-SDR: 1ck/dPErJGUvPU1FfmQCQ3RM1qA/4wkog+hG2gFoRCUqbT942/0vI/pn40qaTsj+UCtWO+waCg VREfnAZ1n+NL1wQyBuJ1Q/Adap4OFeXY9S+Bj7DhIekOXVDPTqTKpanqRYENgWacOe8CZM7GnY 6CqzWnzNL4HGSmWrseAFV6innpV4ou7j7JOdpSYvWhmArVoNEi960v5tcqVwYkMLRzVn2zUHJg +lNjqVXOyjDpRNTa+gtX8cpuIdBroLdeBVAF2bFJY0RvH69eErEKu1qnNhsVZTVAVKf4m7ctSp dA8= WDCIronportException: Internal Received: from usg-ed-osssrv.wdc.com ([10.3.10.180]) by uls-op-cesaip02.wdc.com with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256; 19 Dec 2022 14:59:11 -0800 Received: from usg-ed-osssrv.wdc.com (usg-ed-osssrv.wdc.com [127.0.0.1]) by usg-ed-osssrv.wdc.com (Postfix) with ESMTP id 4NbZrt4Q1zz1Rwt8 for ; Mon, 19 Dec 2022 14:59:10 -0800 (PST) Authentication-Results: usg-ed-osssrv.wdc.com (amavisd-new); dkim=pass reason="pass (just generated, assumed good)" header.d=opensource.wdc.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d= opensource.wdc.com; h=content-transfer-encoding:content-type :in-reply-to:organization:from:content-language:references:to :subject:user-agent:mime-version:date:message-id; s=dkim; t= 1671490749; x=1674082750; bh=nBOnBM2AbYb8rMTgdV7VKM1gSLrZ9214921 NLXhZivI=; b=Dtvc58csZf+Jy7BMgd54A35CyniVwl59EYZIhYNurtF/xBMu56w aSTwVhUJdfeY3rAyGpVaB3hf7kgh7USFI3c46nN5rRB81SThKZXvLW3BhR74A2yo xJqNAvKRADNFIncenxq/iSiE/HDaR+PD+NUxUcvuz23qpVsNHj9xf5yUSr1svTIE /PFTMPuI4JQ52cBlgVkWaUR4m7dTE6p/b0kSS5Ao/HAXqHmdujvImuxkEynuNcrm XuiYsFa1kpKYrieFAA0JSUr3HG8j+/H/VRtQRtKHJeNLQq/aLs0pCaXJmEs2pGQQ fW+DnOs3HkX5ULaM/N7q4GNTB/NoBJAkK/g== X-Virus-Scanned: amavisd-new at usg-ed-osssrv.wdc.com Received: from usg-ed-osssrv.wdc.com ([127.0.0.1]) by usg-ed-osssrv.wdc.com (usg-ed-osssrv.wdc.com [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id GV87MgVfbfUJ for ; Mon, 19 Dec 2022 14:59:09 -0800 (PST) Received: from [10.225.163.106] (unknown [10.225.163.106]) by usg-ed-osssrv.wdc.com (Postfix) with ESMTPSA id 4NbZrq4hc6z1RvLy; Mon, 19 Dec 2022 14:59:07 -0800 (PST) Message-ID: <234e04db-7539-07e4-a6b8-c6b05f78193d@opensource.wdc.com> Date: Tue, 20 Dec 2022 07:59:06 +0900 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1 Subject: Re: [PATCH V2] scsi: libsas: Directly kick-off EH when ATA device fell off To: Jason Yan , John Garry , Xingui Yang , jejb@linux.ibm.com, martin.petersen@oracle.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> <565fcf28-ec53-8d74-00a3-94be8e5b60e4@oracle.com> <60ace19e-d029-f14d-9aac-d5cef83b5b64@huawei.com> Content-Language: en-US From: Damien Le Moal Organization: Western Digital Research In-Reply-To: <60ace19e-d029-f14d-9aac-d5cef83b5b64@huawei.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org On 12/20/22 00:28, Jason Yan wrote: > On 2022/12/19 17:23, John Garry wrote: >> 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=20 >>> avoids >>> having to wait for block layer timeouts. >>> >>> Signed-off-by: Xingui Yang >>> --- >>> Changes to v1: >>> - Use dev_is_sata() to check ATA device type >>> =C2=A0 drivers/scsi/libsas/sas_discover.c | 3 +++ >>> =C2=A0 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/scsi/libsas/sas_discover.c=20 >>> 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_por= t=20 >>> *port) >>> =C2=A0 void sas_unregister_dev(struct asd_sas_port *port, struct=20 >>> domain_device *dev) >>> =C2=A0 { >>> +=C2=A0=C2=A0=C2=A0 if (test_bit(SAS_DEV_GONE, &dev->state) && dev_is= _sata(dev)) >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sas_ata_device_link_abort= (dev, false); >> >> Firstly, I think that there is a bug in sas_ata_device_link_abort() ->= =20 >> ata_link_abort() code in that the host lock in not grabbed, as the=20 >> comment in ata_port_abort() mentions. Having said that, libsas had=20 >> already some dodgy host locking usage - specifically dropping the lock= =20 >> for the queuing path (that's something else to be fixed up ... I think= =20 >=20 > Taking big locks in queuing path is not a good idea. This will bring=20 > down performance. With HDDs ? You will not see any difference (and SATA SSDs are not a thin= g anymore, enough that we should worry too much. NVMe took over). And that "big lock" is libata is really an integral part of the design. To remove it, you will need to rewrite libata entirely... >=20 >=20 >> that is due to queue command CB calling task_done() in some cases), bu= t=20 >> I still think that sas_ata_device_link_abort() should be fixed (to gra= b=20 >> the host lock). >=20 > For sas_ata_device_link_abort(), it should grab ap->lock. Which is what libata code comments (mistakenly in many places) always refer as host lock. >=20 > Thanks, > Jason --=20 Damien Le Moal Western Digital Research