Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <775f75db-edb8-8f39-2592-862756811710@oracle.com>
Date:   Sun, 3 Apr 2022 11:58:59 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH] scsi: core: always finish successfully aborted notry
 command
Content-Language: en-US
To:     Wenchao Hao <haowenchao@huawei.com>,
        "James E . J . Bottomley" <jejb@linux.ibm.com>,
        "Martin K . Petersen" <martin.petersen@oracle.com>,
        Hannes Reinecke <hare@suse.de>, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     linfeilong@huawei.com
References: <20220401035020.1043239-1-haowenchao@huawei.com>
From:   Mike Christie <michael.christie@oracle.com>
In-Reply-To: <20220401035020.1043239-1-haowenchao@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OWhHQWZ4SFNkVS9xRDJJNFMzbm8zSjRjelNBSVU4Vyt4UVZod2p5N3lKTkRx?=
 =?utf-8?B?NkU2NzUyRTZSazFFRnh3Ylo0T3VWS0tNVm1aWHYvcVNBcU94eGJuZ0szZzlK?=
 =?utf-8?B?UDJCeDQzTWovaVFzMGFlazg4RGQwdG5BSk11Z2hSOXA5WnR0Z0V1SDgva1o1?=
 =?utf-8?B?QkRnQWxnaVlrUmRxdVlIaG5sRHo4V0RDd0lWV3VlSDZkdzJKa0toajM1RWE3?=
 =?utf-8?B?eERUc0xOZDlUT2VJZnh1aXU2a25BcnhZNVZmMUNIMDNFRFpJaWpiVlhLaGFD?=
 =?utf-8?B?Vkp0TlI0NjRPUkpNMWNWUWl1WWNXNytaMzhZMk9IOE9TbThhMk1kQzZKV0RL?=
 =?utf-8?B?SkxMN0FpLzRtNVpMSGN2M0sxRFYvR2YvaWRiY0JnOG80cEdTMmNaRHJ5bGdj?=
 =?utf-8?B?QUF5bG1qbGJ4aDdqbEhWQjh6L3Bzc0hFOVFrT1pkejJPY1VxNTUrV2dNbGky?=
 =?utf-8?B?bFBabjlIc043WmZtUnNodHlVR0w4RXhxWUcyMXhLTXVGVUpHdjVJYTNJZzJZ?=
 =?utf-8?B?dU5va1RnM3dyRXhJTGowQUh5OERNRUNENVdlMVhCVEY5UzJ5WW9PWUFSbWVZ?=
 =?utf-8?B?V1R2MjNxVWhJVmdEMjI1MDdhZ3ZaNFd5M3JMK29TbDNXSzA5SHZTcThYc0t1?=
 =?utf-8?B?ZUJuRUxCT3pDMTNuVklLRENPUzUxRGxMdm1oZkUrUWswRUZhcGJnbzJzSHJq?=
 =?utf-8?B?T08rV3dtb1BBYm04MjFqQUpXY0V6a0dXR1RKWTUvY2I3L05Gd1FYU3NLcm95?=
 =?utf-8?B?RlRJY1JVcC9KQWh2bmFTWS9wTGVYYnBTSkRpSlpHNzlndVRzMTh1RUZrSDdy?=
 =?utf-8?B?bFJIZ2hzRThGdWNFMmxWd0lKYkpIa3JMcFVtaWwza3pjVkEyemtwSHZ5Q2Q4?=
 =?utf-8?B?di8wOW5KQjNpUkhsYmcybDZXUDFobkpzZmFXUFZFUjFmWFA0a0UxOUVGUUxD?=
 =?utf-8?B?MU5jd1U1QjdLbEZ2RVdsYW52MjVPc28wNE9iM2RqVmhWOWlkbkl5cE9GSlpy?=
 =?utf-8?B?TCtEVHQxSXFUZ0lYRW1nWWJVclg0QTZhTmhGTDZYTmFHTXN3WkhrYXc0RURO?=
 =?utf-8?B?bkRuMHhqbk0xclJnMUxaMTE1T2FKMkxVcFZoZzkzT2VvbFdHbVYvSnpOd2du?=
 =?utf-8?B?bkxzdjlkUnpRMkoxdnE4ME1JWGJuUEtBSUEySkc3SjNmcUU3cjl3WStXVWwz?=
 =?utf-8?B?c2dOc2Z2NVlEWEFGelNHby9nMyt0VGxRNVhmT1N1a2RpWmlxSCtTbmhHWGxW?=
 =?utf-8?B?N0lHMEdCZUxFekZNVE43blU0Nm8vUmREeVhOZFRtR2pFWlBOYmtKK3I4Q0hS?=
 =?utf-8?B?bFhXUmtiL0tIM3pnN3d2MzRwRlAxNTV0L1pvVFp5eUpWNktsWGtTdFJGbmEy?=
 =?utf-8?B?bGJNNUYzSTFMVm0vS2M0eW9qRzBpTG85ajFJYVlWWHo5bGNLRVBQdDZURm05?=
 =?utf-8?B?S2grMkk2ZnBpdXYrSUV0WUV6Q0hzQW1CUWNBbWJUODNLNmtTdFFieTJhM0FZ?=
 =?utf-8?B?bFZVdnNOSmI4VldwZUJQczBvenZRRy9iTmUrN1VSRGVkd2Jua1lhYkFwTXov?=
 =?utf-8?B?bHA1elRqTitWQWJ3Y2VXNlVUQTlkeTd5M2dPVysydFhwd0M1OG0vQTY5bzM1?=
 =?utf-8?B?ZXpjUzNVK2tCTUo5bmxvNFBzK0JRTkJwRnFUOGFFMlhmTk1YT2dyMExLRDlT?=
 =?utf-8?B?K0hUYktVMnBSbEtWckw2SWg3QVM0bnlKVWtNSkNiWWpORk1oTG5RR1R6TnNo?=
 =?utf-8?B?K0lyRGVvNkxYc0NQR1JyYUJpSkY1WWZHekJFMFhHTk1ubkozK3k0bm9IeU16?=
 =?utf-8?B?bU9xRmVSQnBGeldRTjVLTHByN255K1JvSjVEMVlaNmZ6VWlGYlJiam05T0hi?=
 =?utf-8?B?b3Vzei9sUUw2YjFGeE5TYmszN2lZUHkvQXQ4RjJWRHFyR25rRm95UjRKMHl3?=
 =?utf-8?B?Q3Y5UUhjYjJhWWluWGcvbGlpTW5BdDZXeHAyd3VyUEtqOHcrcjhMOE1FWmVG?=
 =?utf-8?B?ZDNHMlJUbWpXOEg1cGtLQXp3T3JXRXlwQzcyQ3JEYWJPK0R0aVd6ZXpleGVz?=
 =?utf-8?B?VjdwTVNDbFpHS1R3NFo0TjF0NHRGaTczeDVCdytxUy9BYjlkMy9JekpxSDJ3?=
 =?utf-8?B?YS9RdWZac1FwRUFhMXA0WGQ4YzMyOGMwRGhYNXQ2cHJwK3hJaDNiRG5SUzhB?=
 =?utf-8?B?U0Qybk9hSHBDQWZMcjVFUDdDZ0FaMkFkTXVCdlVRd3pyTGNJdWw3bTVVckwz?=
 =?utf-8?B?a0I2UlZXdXd6VVRXK09pb0JROExTZzZTQ3ZEQjFlRVJFdEFteEhyZUJUZTZ3?=
 =?utf-8?B?cFp0eTAxSSsrdDc1WkVaWDNMa2cxcEhydy9iM3FSSmk0c01LTlZkUmtBczZV?=
 =?utf-8?Q?pTBDVPrFrAJNjZ5E=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 58e1dbee-b26f-4614-2a3e-08da15934099
X-MS-Exchange-CrossTenant-AuthSource: DM5PR10MB1466.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Apr 2022 16:59:01.6689
 (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: OtpW1OOA8C6aYTYxC+N/e0jOTw0yW/67y4PQtjiulXiMBdoccIZm+FoR7tszKsmwHXvJIvZSJaGazj0/NS4qiSjvkDWSOfgH+6L+QyITpf8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR10MB1655
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.425,18.0.850
 definitions=2022-04-03_04:2022-03-30,2022-04-03 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 adultscore=0 mlxscore=0
 bulkscore=0 suspectscore=0 spamscore=0 mlxlogscore=999 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2204030112
X-Proofpoint-ORIG-GUID: Vc5PvtW9S26ZWfIBU53rF5DmBx9sajOs
X-Proofpoint-GUID: Vc5PvtW9S26ZWfIBU53rF5DmBx9sajOs
X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable 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 3/31/22 10:50 PM, Wenchao Hao wrote:
> If the abort command succeed and it does not need to be retired, do not add
> it to error handle list. 
> 
> Adding command to error handle list is an annoying flow which would stop I/O
> of all LUNs which shared a same HBA.
> 
> So here if we successfully abort a command, we can finish it via
> scsi_finish_command() which would reduce time spent on scsi_error_handler()
> 
> Signed-off-by: Wenchao Hao <haowenchao@huawei.com>
> ---
>  drivers/scsi/scsi_error.c | 55 +++++++++++++++++++++------------------
>  1 file changed, 29 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index cdaca13ac1f1..15299603b7ee 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -173,41 +173,44 @@ scmd_eh_abort_handler(struct work_struct *work)
>  		goto out;
>  	}
>  	set_host_byte(scmd, DID_TIME_OUT);
> -	if (scsi_host_eh_past_deadline(shost)) {
> -		SCSI_LOG_ERROR_RECOVERY(3,
> -			scmd_printk(KERN_INFO, scmd,
> -				    "eh timeout, not retrying "
> -				    "aborted command\n"));
> -		goto out;
> -	}
>  
> -	spin_lock_irqsave(shost->host_lock, flags);
> -	list_del_init(&scmd->eh_entry);
> +	if (scsi_noretry_cmd(scmd) ||
> +	    !scsi_cmd_retry_allowed(scmd) &&
> +	    !scsi_eh_should_retry_cmd(scmd)) {


I don't think this test is correct. Did you want all ||s?

For what the patch is trying to accomplish I'm not sure if it's
the behavior people wanted. Do all drivers have configurable
abort timeouts? If an abort takes N minutes to complete ok,
and that's put us over the eh deadline maybe the user wanted
that device to be offlined.