Re: [PATCH] blk-mq: fix race between complete and BLK_EH_RESET_TIMER

[Ming Lei <ming.lei@redhat.com>]

On Wed, Apr 11, 2018 at 03:47:12PM -0700, Tejun Heo wrote:
> Hello,
> 
> On Thu, Apr 12, 2018 at 06:43:45AM +0800, Ming Lei wrote:
> > On Wed, Apr 11, 2018 at 02:30:07PM -0700, Tejun Heo wrote:
> > > Hello, Ming.
> > > 
> > > On Thu, Apr 12, 2018 at 04:55:29AM +0800, Ming Lei wrote:
> > > ...
> > > > +		spin_lock_irqsave(req->q->queue_lock, flags);
> > > > +		if (blk_mq_rq_state(req) != MQ_RQ_COMPLETE_IN_RESET) {
> > > > +			blk_mq_rq_update_aborted_gstate(req, 0);
> > > > +			blk_add_timer(req);
> > > 
> > > Nothing prevents the above blk_add_timer() racing against the next
> > > recycle instance of the request, so this still leaves a small race
> > > window.
> > 
> > OK.
> > 
> > But this small race window can be avoided by running blk_add_timer(req)
> > before blk_mq_rq_update_aborted_gstate(req, 0), can't it?
> 
> Not really because aborted_gstate right now doesn't have any memory
> barrier around it, so nothing ensures blk_add_timer() actually appears
> before.  We can either add the matching barriers in aborted_gstate
> update and when it's read in the normal completion path, or we can
> wait for the update to be visible everywhere by waiting for rcu grace
> period (because the reader is rcu protected).

Seems not necessary.

Suppose it is out of order, the only side-effect is that the new
recycled request is timed out as a bit late, I think that is what
we can survive, right?

But it need to be documented.

--
Ming