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

Bart Van Assche
2022-09-30T00:32:38Z

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.