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@xxxxxxxxx>
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.
