Re: [PATCH] blk-mq: Fix a race between resetting the timer and completion handling

["tj@kernel.org" <tj@kernel.org>]

Hello, Bart.

On Mon, Feb 05, 2018 at 09:33:03PM +0000, Bart Van Assche wrote:
> My goal with this patch is to fix the race between resetting the timer and
> the completion path. Hence change (3). Changes (1) and (2) are needed to
> make the changes in blk_mq_rq_timed_out() work.

Ah, I see.  That makes sense.  Can I ask you to elaborate the scenario
you were fixing?

> > > @@ -831,13 +834,12 @@ static void blk_mq_rq_timed_out(struct request *req, bool reserved)
> > >  		__blk_mq_complete_request(req);
> > >  		break;
> > >  	case BLK_EH_RESET_TIMER:
> > > -		/*
> > > -		 * As nothing prevents from completion happening while
> > > -		 * ->aborted_gstate is set, this may lead to ignored
> > > -		 * completions and further spurious timeouts.
> > > -		 */
> > > -		blk_mq_rq_update_aborted_gstate(req, 0);
> > > +		local_irq_disable();
> > > +		write_seqcount_begin(&req->gstate_seq);
> > >  		blk_add_timer(req);
> > > +		req->aborted_gstate = 0;
> > > +		write_seqcount_end(&req->gstate_seq);
> > > +		local_irq_enable();
> > >  		break;
> > 
> > So, this is #3 and I'm not sure how adding gstate_seq protection gets
> > rid of the race condition mentioned in the comment.  It's still the
> > same that nothing is protecting against racing w/ completion.
> 
> I think you are right. I will see whether I can rework this patch to address
> that race.

That race is harmless and has always been there tho.  It only happens
when the actual completion coincides with timeout expiring, which is
very unlikely, and the only downside is that the completion gets lost
and the request will get timed out down the line.  It'd of course be
better to close the race window but code simplicity likely is an
important trade-off factor here.

Thanks.

-- 
tejun