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: <257dcd6c-2ffd-f518-9b13-c309348088d9@oracle.com>
Date:   Mon, 19 Dec 2022 15:55:01 +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
To:     Jason Yan <yanaijie@huawei.com>,
        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>
 <565fcf28-ec53-8d74-00a3-94be8e5b60e4@oracle.com>
 <60ace19e-d029-f14d-9aac-d5cef83b5b64@huawei.com>
Content-Language: en-US
From:   John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <60ace19e-d029-f14d-9aac-d5cef83b5b64@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SmZmWEhqQktaaWNTQ3hsTm1wc1NvZFF0d1FTWDl0UHRyNjJwRkdmUWZVQ1Va?=
 =?utf-8?B?dmVzdDVCczQ4RUhvZFJmbFoxRkJIZldNWDBrVmxvUFVaOWt0em4rVmZkMEdJ?=
 =?utf-8?B?aTJMbXM4RUN3Wk4wMDg4N3BFVEc2dWZGQnJHSHZPUjlUaEx1OEJKNXNRMm0w?=
 =?utf-8?B?M3o0VlhzUDM4SkNBdFY3L214clRDaFU1YlJHTSswRlhZKy9CQzlvb2xrRmVR?=
 =?utf-8?B?YnZzLzVST2d3VzRYOW8rRXJvaW1CQVNyT1cyRzBlSTVxSjFiZTZ1aXNCdGYx?=
 =?utf-8?B?NFRtcXFBajFsSzR5WklyZFlTOHJZWHpWSVdndUkyb0VWT0RrVVRNWkkwZjVH?=
 =?utf-8?B?c3lOS2xZMEJLVnBCazFvbXhpcjdodWNVSWZZZFRpbFY2WVNmbDQyS0E1bkJS?=
 =?utf-8?B?L3RVT3hNNWI4Vkxob3QzUENYTm8xdWtsbGJoV3FTU1QxV0QxN05RdTBHYWNy?=
 =?utf-8?B?OWpFT3hTcWRDazhrVnUyTGx3emV6cGVrVnIrYzE4OGUvMHFLUkN1V1RDSm9w?=
 =?utf-8?B?NGtuNC9vVkl4dEdRMjV5eEJIU2FJOEVRN0lPZDNSZzRsWWt2T1ZIbWZWU3BW?=
 =?utf-8?B?aGhpd01xaTRZUDV3TFk4MmNRUzN1N2J4S1NOOHpKU1VLUUR0WFkzN0JrNlNj?=
 =?utf-8?B?VzNwakdhYTFlT3RmTUs0RS9GNUd3TVRMbUs0SmFHdG82YW5ZRXY3U0o5K0hU?=
 =?utf-8?B?WEovRlZhSG9QemFkM1A3dXZUd1d1dnhyT0JmNXFXUjVYbW5Uc21JTGVhQUNE?=
 =?utf-8?B?Q0Z0M01tNUJDUzMzcXVpZCtHekdYZFNvWEZ4T3d4V1FSNnJ0N3RPYW1GYS9T?=
 =?utf-8?B?M293bHlodUk4bXNkYnRURlROZWN1WkowTFM5SytjNEg2QnoveFQzazBpNjR4?=
 =?utf-8?B?UXdjNEozZ2xVUTl1MHB5Y0dJTk8rMGlTcUlTVjV5UHhkNzk0R2J2QzZ1TGdH?=
 =?utf-8?B?MmVkaXo5UEcyRzJMVEk3V3JvNVhOb3pOa0ZoditwamRjbWpFU3JVTGpVSUJn?=
 =?utf-8?B?Q0tLZXlpQkdmVkI4eEpiMXNUUGZqRFFLVHA1QmZWcDRKbUdSVWZ4dXFmQU1U?=
 =?utf-8?B?TVJtdG8vVFJWWktUU3ZxWEQ0UlNoRjJ3anRiVGhWSi9ZZlN5SGM4K1RIbkM1?=
 =?utf-8?B?ZG9CM2Eya3ZCYmRrazNkejd2Wk9xQmZwWVUrdHhpQUdxRlhSaDUzdzlUV3Uz?=
 =?utf-8?B?QnRMdTRmeUFEVHFNaVRQdStkSm1FMHpNNjFScGsxV2h6aC96c09acFpiNTA0?=
 =?utf-8?B?Rm10WUtnQVlLYmI5aGhLamp0b0hISUc5VDhsNDFCVGJ0VWJ6N0hiMEJmcmtJ?=
 =?utf-8?B?VXIwanV6dzJXeWlEOEloRXlzcEE4eElHKzdMOVFrWUpqRGhORUtSQy95TTVa?=
 =?utf-8?B?bnJ4bVRxNGhBT3pNTUtXVGFSZDliaC9RTDRCVDJtSzFVMXl6VGdueTh5eUxs?=
 =?utf-8?B?MENCaHJOQW1JRHJvbUJzckNabEQyeHhmSDN0Z2pzOE8yMW51aUljQ2FXaThB?=
 =?utf-8?B?RUFSSHVZdzFDNmk1akQ0MDYrUm9ocDcyK0gxVkluRjRHVzB0b1lxTVFwdWlt?=
 =?utf-8?B?UTBzcDJjM3pUQ01DTjM4T0ovdGZ6eXJXNlhoWWNiTjEwdno3b1NHdzQ4K09K?=
 =?utf-8?B?Q2lta3lOR21IckxJSXBqZ1haSklPQWdpRUk0eVdhU1F0My95V0NRN1pmaVFz?=
 =?utf-8?B?aFczS0VUbFBEdWZjdit0cVd4anZiYWhMNGJkbmNSNzNSTlB3UzJWRUltL0Q3?=
 =?utf-8?B?dHdrc09kZld4dXlBL3ljNzRnTVN5MEd5amsrRDJKeTg5NzQzRk9yZmFnclZV?=
 =?utf-8?B?VHE2YnFzMXJDT0xIRmNJTW5jZkRBdDl5ZUJwc1JHOHRqNUlBb0JHMHpaaHVQ?=
 =?utf-8?B?MldlcnZtQk5XRm1vVnRrWWpVc2FQS2FsODY3alVudXBsTEZ3NTBwVlU0MnpO?=
 =?utf-8?B?aEhSTWhwMHMyV1YwV0M5U0p2UmtuSHdDaWtnL0I0K21zVUlERzhRSkh5YzBu?=
 =?utf-8?B?aGhxWGFJZDNrZEJIbGJ6MXV6Z212OEdIVGtVNkpHc014RHYzU2FhRnJrd2c0?=
 =?utf-8?B?OFRlOHV1Zml6SEtLUFlDZ1phTzBESXU0WXB5ajI2bkdDZVpQaytoc2k2Z21s?=
 =?utf-8?Q?OMAiLFPwvMpPteRY+aSMFncy3?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: decd8cfd-f2a3-45c1-062a-08dae1d9660b
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Dec 2022 15:55:06.4684
 (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: SApKQk3/u3+SiSjKxDw+UJCXWdA8aFtMQFLxAmQtGFA52xVhA4EMbQbWoHz8ZH8bNnQkoaG5DGlXBEa/bXi/eQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR10MB5808
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-19_01,2022-12-15_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 bulkscore=0 mlxscore=0
 adultscore=0 phishscore=0 suspectscore=0 spamscore=0 mlxlogscore=949
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2212190141
X-Proofpoint-ORIG-GUID: HPyW_WJW3USVGSUKVt7SUtZXQy_2tiuh
X-Proofpoint-GUID: HPyW_WJW3USVGSUKVt7SUtZXQy_2tiuh
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 19/12/2022 15:28, Jason Yan wrote:
>>> +    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 
> 
> Taking big locks in queuing path is not a good idea. This will bring 
> down performance.

But it is expected that ata_qc_issue() should be called with that the 
host lock grabbed (and keep it).

I think that the reason libsas drops the lock is because some LLDD 
queuecommand CBs calls task_done() in some error paths. If we kept the 
lock held, then we could have a deadlock, for example:

sas_ata_qc_issue (has lock) -> lldd_execute_task() = 
pm8001_queue_command() -> task_done() = sas_ata_task_done() -> grab host 
lock => deadlock.

Thanks,
John