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;
IronPort-SDR: B3rUsuGDL+R5XU1NQDUvQzdUu/qKUzWrmgGBjY+3lErpSYkc21VigrKTeKrdup847pXXqv8GkD
 S/ZFQnIuCwJtYNJYoElwBO9EN8gIvee1XW0jyQB4MvKDh43fdfDSOlFtWIazLFNH/OM2WNsqU0
 vPoBpgCVj+bq+mnTjevdPHdmDdFJUjKnuz6JzIv3kwgnQNEdY8scPlvl58P2Lj4D1J7fMjp1Gg
 ZIIW+dWDbajlbZciNSt1Hog9/CJnV1ItAk58OV2G2Pm3zieO8IHSCGgOBvY729fWpFyyVswgg8
 yHU=
IronPort-SDR: QFVI1SyqF4jTnTSN2VrRp7XsZU7ZLWEvqSxx4drW+Jz1HnM3gjDQZY0SV0wPNshQkZIGuInkEz
 JSC0aIYMxZMf5L41SwYGvMhglBBoUEPfKdsv08sO2JYiyASRTz3NEoImpZIbYy0gBG73OQfb4A
 yS788awr2f6FTbUi/KUPmn9MAdoNm5OMK/gFRag6bnfGZyClAqMG2Ffg3dPcgNHlzlRVzdIkNK
 cdQRES1ZIRgAl4JrD6T4r58qNMi1+Zhb4FpQdKGAmnfpq4ogxnaFDQuX2bZI45UAPK+aDS4CtL
 lGM=
WDCIronportException: Internal
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 <yanaijie@huawei.com>,
        John Garry <john.g.garry@oracle.com>,
        Xingui Yang <yangxingui@huawei.com>, 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 <damien.lemoal@opensource.wdc.com>
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

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 <yangxingui@huawei.com>
>>> ---
>>> 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