Hi jianchao,
On Sat, Jan 27, 2018 at 10:29:30PM +0800, jianchao.wang wrote:
Hi ming
Thanks for your detailed response. That's really appreciated.
On 01/27/2018 09:31 PM, Ming Lei wrote:
But nvme_dev_disable may run with nvme_timeout in parallel or race with it.
But that doesn't mean it is a race, blk_mq_complete_request() can avoid race between timeout and other completions, such as cancel. Yes, I know blk_mq_complete_request could avoid the a request is accessed by timeout
path and other completion path concurrently. :) What's I worry about is the timeout path could hold the expired request, so when nvme_dev_disable return, we cannot ensure all the previous outstanding requests has been handled. That's really bad.
The best way to fix this is to ensure the timeout path has been completed before cancel the previously outstanding requests. (Just ignore the case where the nvme_timeout will invoke nvme_dev_disable, it has to be fixed by other way.)
Maybe your approach looks a bit clean and simplify the implementation, but seems it isn't necessary.
So could you explain a bit what the exact issue you are worrying about? deadlock? or others?
There is indeed potential issue. But it is in very narrow window. Please refer to https://lkml.org/lkml/2018/1/19/68
OK, follows description from the above link:
Yes, once controller disabling completes, any started requests will be handled and cannot expire. But before the _boundary_, there could be a nvme_timeout context runs with nvme_dev_disable in parallel. If a timeout path grabs a request, then nvme_dev_disable cannot get and cancel it.
So even though the nvme_dev_disable completes, there still could be a request in nvme_timeout context.
The worst case is : nvme_timeout nvme_reset_work if (ctrl->state == RESETTING ) nvme_dev_disable nvme_dev_disable initializing procedure
the nvme_dev_disable run with reinit procedure in nvme_reset_work in parallel.
OK, that is the issue, the nvme_dev_disable() in nvme_timeout() may disable queues again, and cause hang in nvme_reset_work().
Looks Keith's suggestion of introducing nvme_sync_queues() should be enough to kill the race, but seems nvme_sync_queues() should have been called at the entry of nvme_reset_work() unconditionally.
Thanks, Ming