[PATCH 1/2] nvme: pci: simplify timeout handling

[Ming Lei]

On Sun, Apr 29, 2018 at 10:21 AM, jianchao.wang
<jianchao.w.wang at oracle.com> 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 at gmail.com> wrote:
>>> On Sun, Apr 29, 2018 at 5:57 AM, Ming Lei <tom.leiming at gmail.com> wrote:
>>>> On Sat, Apr 28, 2018 at 10:00 PM, jianchao.wang
>>>> <jianchao.w.wang at oracle.com> 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