Re: [PATCH v3 1/8] scsi: core: Fix a race between scsi_done() and scsi_timeout()

[Bart Van Assche <bvanassche@acm.org>]

On 9/29/22 17:17, Mike Christie wrote:
> On 9/29/22 5:00 PM, Bart Van Assche wrote:
>>   	if (rtn == BLK_EH_DONE) {
>>   		/*
>> -		 * Set the command to complete first in order to prevent a real
>> -		 * completion from releasing the command while error handling
>> -		 * is using it. If the command was already completed, then the
>> -		 * lower level driver beat the timeout handler, and it is safe
>> -		 * to return without escalating error recovery.
>> -		 *
>> -		 * If timeout handling lost the race to a real completion, the
>> -		 * block layer may ignore that due to a fake timeout injection,
>> -		 * so return RESET_TIMER to allow error handling another shot
> 
> I've been wondering about this code too.
> 
> I think the patch is correct for the normal cases, but I didn't understand the
> old fake timeout comment case. From the comment it seemed like that was the reason
> we did the RESET_TIMER. Does that not exist anymore or was it just bogus?

Before commit 15f73f5b3e59 ("blk-mq: move failure injection out of
blk_mq_complete_request") the scsi_mq_done() function cleared the
SCMD_STATE_COMPLETE bit in case of fake timeout injection. I think
that commit made the above comment incorrect.

> The commit you referenced actually was returning BLK_EH_DONE like we want. This
> commit:
> 
> commit f1342709d18af97b0e71449d5696b8873d1a456c
> Author: Keith Busch <keith.busch@intel.com>
> Date:   Mon Nov 26 09:54:29 2018 -0700
> 
>      scsi: Do not rely on blk-mq for double completions
> 
> 
> changed it to BLK_EH_RESET_TIMER and changed the above comment to mention
> the fake timeout case. However, the commit message mentioned the patch was done
> because we didn't want scsi digging the block layer.
> 
> If the fake injection thingy is bogus, then it seems ok to me.

Hmm ... I probably should modify the Fixes tag.

Thanks,

Bart.