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

Ming Lei
2018-04-28T21:57:36Z

On Sat, Apr 28, 2018 at 10:00 PM, jianchao.wang
<jianchao.w.wang@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.

Thanks,
Ming Lei