Re: [PATCH 1/2] scsi: Do not rely on blk-mq for double completions

[Keith Busch <keith.busch@intel.com>]

On Tue, Nov 13, 2018 at 12:20:46PM -0700, Jens Axboe wrote:
> On 11/13/18 11:57 AM, Keith Busch wrote:
> >  static void scsi_mq_done(struct scsi_cmnd *cmd)
> >  {
> > +	if (test_and_set_bit(__SCMD_COMPLETE, &cmd->flags))
> > +		return;
> >  	trace_scsi_dispatch_cmd_done(cmd);
> >  	blk_mq_complete_request(cmd->request);
> > +
> > +#ifdef CONFIG_FAIL_IO_TIMEOUT
> > +	/*
> > +	 * Clearing complete here serves only to allow the desired recovery to
> > +	 * escalate on blk_rq_should_fake_timeout()'s error injection.
> > +	 */
> > +	clear_bit(__SCMD_COMPLETE, &cmd->flags);
> > +#endif
> >  }
> 
> We could have this be:
> 
> static void scsi_mq_done(struct scsi_cmnd *cmd)
> {
> 	if (test_and_set_bit(__SCMD_COMPLETE, &cmd->flags))
> 		return;
>  	trace_scsi_dispatch_cmd_done(cmd);
> 
>  	if (blk_mq_complete_request(cmd->request)) {
> 		/*
> 		 * Clearing complete here serves only to allow the
> 		 * desired recovery to escalate on
> 		 * blk_rq_should_fake_timeout()'s error injection.
> 		 */
> 		clear_bit(__SCMD_COMPLETE, &cmd->flags);
> 	}
> }
> 
> with
> 
> bool blk_mq_complete_request(struct request *rq)
> {
> 	if (unlikely(blk_should_fake_timeout(rq->q)))
> 		return true;
> 	__blk_mq_complete_request(rq);
> 	return false;
> }
> 
> and not have this CONFIG_FAIL_IO_TIMEOUT dependency, but that'd be a bit
> more expensive.

I was trying to avoid every cost no matter how negligable (those are the
only types of costs left as far as I can see), but I think your proposal
might actually be necessary: if a timeout wasn't faked, clearing the
completion flag unconditionally might have a problem with a real timeout
racing with the real completion. :(