[PATCH 1/1] nvme: fix nvme_remove going to uninterruptible sleep for ever

[PATCH 1/1] nvme: fix nvme_remove going to uninterruptible sleep for ever

[Rakesh Pandit]

Once controller is in DEAD or DELETING state a call to delete_destroy
from nvme_uninit_ctrl results in setting the latency tolerance via
nvme_set_latency_tolerance callback even though queues have already
been killed.  This in turn leads the PID to go into uninterruptible
sleep and prevents removal of nvme controller from completion.  The
stack trace is:

[<ffffffff813c9716>] blk_execute_rq+0x56/0x80
[<ffffffff815cb6e9>] __nvme_submit_sync_cmd+0x89/0xf0
[<ffffffff815ce7be>] nvme_set_features+0x5e/0x90
[<ffffffff815ce9f6>] nvme_configure_apst+0x166/0x200
[<ffffffff815cef45>] nvme_set_latency_tolerance+0x35/0x50
[<ffffffff8157bd11>] apply_constraint+0xb1/0xc0
[<ffffffff8157cbb4>] dev_pm_qos_constraints_destroy+0xf4/0x1f0
[<ffffffff8157b44a>] dpm_sysfs_remove+0x2a/0x60
[<ffffffff8156d951>] device_del+0x101/0x320
[<ffffffff8156db8a>] device_unregister+0x1a/0x60
[<ffffffff8156dc4c>] device_destroy+0x3c/0x50
[<ffffffff815cd295>] nvme_uninit_ctrl+0x45/0xa0
[<ffffffff815d4858>] nvme_remove+0x78/0x110
[<ffffffff81452b69>] pci_device_remove+0x39/0xb0
[<ffffffff81572935>] device_release_driver_internal+0x155/0x210
[<ffffffff81572a02>] device_release_driver+0x12/0x20
[<ffffffff815d36fb>] nvme_remove_dead_ctrl_work+0x6b/0x70
[<ffffffff810bf3bc>] process_one_work+0x18c/0x3a0
[<ffffffff810bf61e>] worker_thread+0x4e/0x3b0
[<ffffffff810c5ac9>] kthread+0x109/0x140
[<ffffffff8185800c>] ret_from_fork+0x2c/0x40
[<ffffffffffffffff>] 0xffffffffffffffff

and PID is in 'D' state.  Attached patch returns from callback when
controller is either DELETING state or DEAD which can only happen once
we are in nvme_remove and allows removal to complete and release
remaining resources after nvme_uninit_ctrl.

Fixes: c5552fde102fc ("nvme: Enable autonomous power state transitions")
Signed-off-by: Rakesh Pandit <rakesh@tuxera.com>
---
 drivers/nvme/host/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a609264..c1a632c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1456,6 +1456,9 @@ static void nvme_set_latency_tolerance(struct device *dev, s32 val)
 	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
 	u64 latency;
 
+	if (ctrl->state == NVME_CTRL_DELETING || ctrl->state == NVME_CTRL_DEAD)
+		return;
+
 	switch (val) {
 	case PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT:
 	case PM_QOS_LATENCY_ANY:
-- 
2.9.3

Re: [PATCH 1/1] nvme: fix nvme_remove going to uninterruptible sleep for ever

[Christoph Hellwig]

On Mon, May 29, 2017 at 09:29:54AM +0300, Rakesh Pandit wrote:
> Once controller is in DEAD or DELETING state a call to delete_destroy
> from nvme_uninit_ctrl results in setting the latency tolerance via
> nvme_set_latency_tolerance callback even though queues have already
> been killed.  This in turn leads the PID to go into uninterruptible
> sleep and prevents removal of nvme controller from completion.  The
> stack trace is:
> 
> [<ffffffff813c9716>] blk_execute_rq+0x56/0x80
> [<ffffffff815cb6e9>] __nvme_submit_sync_cmd+0x89/0xf0
> [<ffffffff815ce7be>] nvme_set_features+0x5e/0x90
> [<ffffffff815ce9f6>] nvme_configure_apst+0x166/0x200
> [<ffffffff815cef45>] nvme_set_latency_tolerance+0x35/0x50
> [<ffffffff8157bd11>] apply_constraint+0xb1/0xc0
> [<ffffffff8157cbb4>] dev_pm_qos_constraints_destroy+0xf4/0x1f0
> [<ffffffff8157b44a>] dpm_sysfs_remove+0x2a/0x60
> [<ffffffff8156d951>] device_del+0x101/0x320
> [<ffffffff8156db8a>] device_unregister+0x1a/0x60
> [<ffffffff8156dc4c>] device_destroy+0x3c/0x50
> [<ffffffff815cd295>] nvme_uninit_ctrl+0x45/0xa0
> [<ffffffff815d4858>] nvme_remove+0x78/0x110
> [<ffffffff81452b69>] pci_device_remove+0x39/0xb0
> [<ffffffff81572935>] device_release_driver_internal+0x155/0x210
> [<ffffffff81572a02>] device_release_driver+0x12/0x20
> [<ffffffff815d36fb>] nvme_remove_dead_ctrl_work+0x6b/0x70
> [<ffffffff810bf3bc>] process_one_work+0x18c/0x3a0
> [<ffffffff810bf61e>] worker_thread+0x4e/0x3b0
> [<ffffffff810c5ac9>] kthread+0x109/0x140
> [<ffffffff8185800c>] ret_from_fork+0x2c/0x40
> [<ffffffffffffffff>] 0xffffffffffffffff
> 
> and PID is in 'D' state.  Attached patch returns from callback when
> controller is either DELETING state or DEAD which can only happen once
> we are in nvme_remove and allows removal to complete and release
> remaining resources after nvme_uninit_ctrl.
> 
> Fixes: c5552fde102fc ("nvme: Enable autonomous power state transitions")
> Signed-off-by: Rakesh Pandit <rakesh@tuxera.com>
> ---
>  drivers/nvme/host/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index a609264..c1a632c 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1456,6 +1456,9 @@ static void nvme_set_latency_tolerance(struct device *dev, s32 val)
>  	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>  	u64 latency;
>  
> +	if (ctrl->state == NVME_CTRL_DELETING || ctrl->state == NVME_CTRL_DEAD)
> +		return;
> +

What do you think about moving this into the beginning of
nvme_configure_apst instead?  And please add a comment while you're
at it.

Re: [PATCH 1/1] nvme: fix nvme_remove going to uninterruptible sleep for ever

[Rakesh Pandit]

On Mon, May 29, 2017 at 07:58:39PM +0200, Christoph Hellwig wrote:
> On Mon, May 29, 2017 at 09:29:54AM +0300, Rakesh Pandit wrote:
> > Once controller is in DEAD or DELETING state a call to delete_destroy
> > from nvme_uninit_ctrl results in setting the latency tolerance via
> > nvme_set_latency_tolerance callback even though queues have already
> > been killed.  This in turn leads the PID to go into uninterruptible
> > sleep and prevents removal of nvme controller from completion.  The
> > stack trace is:
...
> 
> What do you think about moving this into the beginning of
> nvme_configure_apst instead?  And please add a comment while you're
> at it.

Thanks, makes sense.  I have posted V2.