On Sun, Apr 29, 2018 at 10:21 AM, jianchao.wang <jianchao.w.wang@xxxxxxxxxx> wrote: > Hi ming > > On 04/29/2018 09:36 AM, Ming Lei wrote: >> On Sun, Apr 29, 2018 at 6:27 AM, Ming Lei <tom.leiming@xxxxxxxxx> wrote: >>> On Sun, Apr 29, 2018 at 5:57 AM, Ming Lei <tom.leiming@xxxxxxxxx> wrote: >>>> On Sat, Apr 28, 2018 at 10:00 PM, jianchao.wang >>>> <jianchao.w.wang@xxxxxxxxxx> wrote: >>>>> Hi ming >>>>> >>>>> On 04/27/2018 10:57 PM, Ming Lei wrote: >>>>>> I may not understand your point, once blk_sync_queue() returns, the >>>>>> timer itself is deactivated, meantime the synced .nvme_timeout() only >>>>>> returns EH_NOT_HANDLED before the deactivation. >>>>>> >>>>>> That means this timer won't be expired any more, so could you explain >>>>>> a bit why timeout can come again after blk_sync_queue() returns >>>>> >>>>> Please consider the following case: >>>>> >>>>> blk_sync_queue >>>>> -> del_timer_sync >>>>> blk_mq_timeout_work >>>>> -> blk_mq_check_expired // return the timeout value >>>>> -> blk_mq_terninate_expired >>>>> -> .timeout //return EH_NOT_HANDLED >>>>> -> mod_timer // setup the timer again based on the result of blk_mq_check_expired >>>>> -> cancel_work_sync >>>>> So after the blk_sync_queue, the timer may come back again, then the timeout work. >>>> >>>> OK, I was trying to avoid to use blk_abort_request(), but looks we >>>> may have to depend on it or similar way. >>>> >>>> BTW, that means blk_sync_queue() has been broken, even though the uses >>>> in blk_cleanup_queue(). >>>> >>>> Another approach is to introduce one perpcu_ref of >>>> 'q->timeout_usage_counter' for >>>> syncing timeout, seems a bit over-kill too, but simpler in both theory >>>> and implement. >>> >>> Or one timout_mutex is enough. >> >> Turns out it is SRCU. >> > after split the timeout path into timer and workqueue two parts, if we don't drain the in-flight requests, the request_queue->timeout and the timeout work look like an issue of 'chicken and egg'. > how about introduce a flag to disable triggering of timeout work ? Yes, that is correct approach, no matter what kind of sync is used, and the flag is inevitable. The timeout sync issue is fixed in this way, now I am thinking about race related with freezing queue. Either freezing need to be removed for non-shutdown, or introduce one flag to avoid the race. Thanks, Ming Lei
