On Wed, 2018-05-16 at 19:31 +0200, hch@xxxxxx wrote: > On Wed, May 16, 2018 at 04:47:54PM +0000, Bart Van Assche wrote: > > I think your patch changes the order of changing the request state and > > calling mod_timer(). In my patch the request state and the deadline are > > updated first and mod_timer() is called afterwards. I think your patch > > changes the order of these operations into the following: > > (1) __blk_mq_start_request() sets the request deadline. > > (2) __blk_mq_start_request() calls __blk_add_timer() which in turn calls > > mod_timer(). > > (3) __blk_mq_start_request() changes the request state into MQ_RQ_IN_FLIGHT. > > > > In the unlikely event of a significant delay between (2) and (3) it can > > happen that the timer fires and examines and ignores the request because > > its state differs from MQ_RQ_IN_FLIGHT. If the request for which this > > happened times out its timeout will only be handled the next time > > blk_mq_timeout_work() is called. Is this the behavior you intended? > > We can move the timer manipulation after the change easily I think. > It would make sense to add comments explaining the ordering. Hello Christoph, I'm afraid that could lead to mod_timer() being called in another order than intended. If e.g. the code that handles BLK_EH_RESET_TIMER changes the request state first to in-flight and next calls mod_timer() then it can happen that another context completes and restarts the request, resulting in a concurrent mod_timer() call. I'm not sure reordering of the mod_timer() calls would result in correct behavior. Bart.
