On Thu, Apr 12, 2018 at 06:57:12AM -0700, Tejun Heo wrote:
On Thu, Apr 12, 2018 at 07:05:13AM +0800, Ming Lei wrote:
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?
It at least can mess up the timeout duration for the next recycle instance because there can be two competing blk_add_timer() instances. I'm not sure whether there can be other consequences. When ownership isn't clear, it becomes really difficult to reason about these things and can lead to subtle failures. I think it'd be best to always establish who owns what.
Please see the code of blk_add_timer() for blk-mq:
blk_rq_set_deadline(req, jiffies + req->timeout); req->rq_flags &= ~RQF_MQ_TIMEOUT_EXPIRED;
if (!timer_pending(&q->timeout) || time_before(expiry, q->timeout.expires)) mod_timer(&q->timeout, expiry);
If this rq is recycled, blk_add_timer() only touches rq->deadline and the EXPIRED flags, and the only effect is that the timeout may be handled a bit late, but the timeout monitor won't be lost.
And this thing shouldn't be difficult to avoid, as you mentioned, synchronize_rcu() can be added between blk_add_timer() and resetting aborted gstate for avoiding it.
thanks, Ming