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

[Mike Christie <michael.christie@oracle.com>]

On 9/29/22 5:00 PM, Bart Van Assche wrote:
> If there is a race between scsi_done() and scsi_timeout() and if
> scsi_timeout() loses the race, scsi_timeout() should not reset the
> request timer. Hence change the return value for this case from
> BLK_EH_RESET_TIMER into BLK_EH_DONE.
> 
> Although the block layer holds a reference on a request (req->ref) while
> calling a timeout handler, restarting the timer (blk_add_timer()) while
> a request is being completed is racy.
> 
> Cc: Keith Busch <kbusch@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Reported-by: Adrian Hunter <adrian.hunter@intel.com>
> Fixes: 065990bd198e ("scsi: set timed out out mq requests to complete")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/scsi_error.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 16bd0adc2339..d1b07ff64a96 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -343,19 +343,11 @@ enum blk_eh_timer_return scsi_timeout(struct request *req)
>  
>  	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?

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.

Reviewed-by: Mike Christie <michael.christie@oracle.com>


> -		 * at this command.
> +		 * If scsi_done() has already set SCMD_STATE_COMPLETE, do not
> +		 * modify *scmd.
>  		 */
>  		if (test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state))
> -			return BLK_EH_RESET_TIMER;
> +			return BLK_EH_DONE;
>  		if (scsi_abort_command(scmd) != SUCCESS) {
>  			set_host_byte(scmd, DID_TIME_OUT);
>  			scsi_eh_scmd_add(scmd);