[PATCH v2 1/2] nvmet: reset keep alive timer in controller enable

[PATCH v2 1/2] nvmet: reset keep alive timer in controller enable

[Sagi Grimberg]

From: Max Gurtuvoy <maxg@mellanox.com>

Controllers that are not yet enabled should not really
enforce keep alive timeout, but we still want to track
a timeout and cleanup in case a host died before it enabled
us. Hence, simply reset the keep alive timer when the controller
is enabled.

Suggested-by: Max Gurtovoy <maxg at mellanox.com>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
Changes from v1:
- Added some code documentation

 drivers/nvme/target/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index a03da764ecae..1ade6fe9b830 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -686,6 +686,14 @@ static void nvmet_start_ctrl(struct nvmet_ctrl *ctrl)
 	}
 
 	ctrl->csts = NVME_CSTS_RDY;
+	/*
+	 * Controllers that are not yet enabled should not really
+	 * enforce keep alive timeout, but we still want to track
+	 * a timeout and cleanup in case a host died before it enabled
+	 * us. Hence, simply reset the keep alive timer when the controller
+	 * is enabled.
+	 */
+	mod_delayed_work(system_wq, &ctrl->ka_work, ctrl->kato * HZ);
 }
 
 static void nvmet_clear_ctrl(struct nvmet_ctrl *ctrl)
-- 
2.14.1

[PATCH v2 2/2] nvme: start keep alive timer when enabling the controller

[Sagi Grimberg]

From: Max Gurtuvoy <maxg@mellanox.com>

We start the keep alive timer after all the I/O queues were
created, this might be too late as the controller might have
start its keep alive timer when it was enabled. Hence, start
it right after controller enable was completed.

We stop the keep alive in nvme_stop_ctrl as its before the
disable/shutdown and its independent of the transport connectivity.

Suggested-by: Max Gurtovoy <maxg at mellanox.com>
Suggested-by: James Smart <james.smart at broadcom.com>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 3ba4232d114e..8673b7816e9a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1761,7 +1761,14 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl, u64 cap)
 	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
 	if (ret)
 		return ret;
-	return nvme_wait_ready(ctrl, cap, true);
+
+	ret = nvme_wait_ready(ctrl, cap, true);
+	if (ret)
+		return ret;
+
+	nvme_start_keep_alive(ctrl);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(nvme_enable_ctrl);
 
@@ -3418,9 +3425,6 @@ EXPORT_SYMBOL_GPL(nvme_stop_ctrl);
 
 void nvme_start_ctrl(struct nvme_ctrl *ctrl)
 {
-	if (ctrl->kato)
-		nvme_start_keep_alive(ctrl);
-
 	if (ctrl->queue_count > 1) {
 		nvme_queue_scan(ctrl);
 		nvme_enable_aen(ctrl);
-- 
2.14.1

[PATCH v2 1/2] nvmet: reset keep alive timer in controller enable

[Christoph Hellwig]

I've applied both patches to nvme-4.18.

[PATCH v2 1/2] nvmet: reset keep alive timer in controller enable

[Max Gurtovoy]

On 6/20/2018 12:06 PM, Christoph Hellwig wrote:
> I've applied both patches to nvme-4.18.


the second one is not complete for error flows (I've mentioned it 
yesterday).  We should re-think about my V1 suggestion to start/stop 
keep-alive during configure/destroy admin queue, or find another 
solution. I find it strange to have exported nvme_stop_keep_alive and 
non exported nvme_start_keep_alive.


> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-nvme&data=02%7C01%7Cmaxg%40mellanox.com%7C47a667b338604182f4dd08d5d68be4cf%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1%7C636650818702308730&sdata=m7GH93WOn%2F0CZhx0YN6XNjWdqlOh4V16Kaz5sl1JpOk%3D&reserved=0
> 

[PATCH v2 1/2] nvmet: reset keep alive timer in controller enable

[Sagi Grimberg]

>> I've applied both patches to nvme-4.18.
> 
> 
> the second one is not complete for error flows (I've mentioned it 
> yesterday).

Yea - lets hold off on patch 2

[PATCH v2 1/2] nvmet: reset keep alive timer in controller enable

[Christoph Hellwig]

On Wed, Jun 20, 2018@01:41:08PM +0300, Sagi Grimberg wrote:
>
>>> I've applied both patches to nvme-4.18.
>>
>>
>> the second one is not complete for error flows (I've mentioned it 
>> yesterday).
>
> Yea - lets hold off on patch 2

Ok. I've dropped patch 2 for now.