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

[PATCH 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 Gurtuvoy <maxg at mellanox.com>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index fe151d672241..a81bf4d5e60c 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -643,6 +643,7 @@ static void nvmet_start_ctrl(struct nvmet_ctrl *ctrl)
 	}
 
 	ctrl->csts = NVME_CSTS_RDY;
+	mod_delayed_work(system_wq, &ctrl->ka_work, ctrl->kato * HZ);
 }
 
 static void nvmet_clear_ctrl(struct nvmet_ctrl *ctrl)
-- 
2.14.1

[PATCH 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 Gurtuvoy <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 e1b708ee6783..4b5c3f7addeb 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1739,7 +1739,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);
 
@@ -3393,9 +3400,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);
 		queue_work(nvme_wq, &ctrl->async_event_work);
-- 
2.14.1

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

[Max Gurtovoy]

On 4/15/2018 11:47 AM, Sagi Grimberg wrote:
> From: Max Gurtuvoy <maxg at 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.
> 

Christoph suggested to add the keep-alive stop to the disable/shutdown 
(I guess we need to add it to both, right ?) and in the target side to 
start expecting also once ctrl is enabled and stop when disabled.

Would you like me to send another version ? This one should also change 
the FC code since there is no use in nvme_disable_ctrl...


> Suggested-by: Max Gurtuvoy <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 e1b708ee6783..4b5c3f7addeb 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1739,7 +1739,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);
>   
> @@ -3393,9 +3400,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);
>   		queue_work(nvme_wq, &ctrl->async_event_work);
> 

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

[Sagi Grimberg]

> Christoph suggested to add the keep-alive stop to the disable/shutdown 
> (I guess we need to add it to both, right ?) and in the target side to 
> start expecting also once ctrl is enabled and stop when disabled.

I don't necessarily think we need to. Also, its better to do this sooner
rather than later (stop_ctrl happens before disable/shutdown) and also,
disable/shutdown might not even execute if the transport is not connected.

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

[Christoph Hellwig]

On Sun, Apr 15, 2018@01:17:58PM +0300, Sagi Grimberg wrote:
>> Christoph suggested to add the keep-alive stop to the disable/shutdown (I 
>> guess we need to add it to both, right ?) and in the target side to start 
>> expecting also once ctrl is enabled and stop when disabled.
>
> I don't necessarily think we need to. Also, its better to do this sooner
> rather than later (stop_ctrl happens before disable/shutdown) and also,
> disable/shutdown might not even execute if the transport is not connected.

I was suggesting to do it in disable as that is the point at which
we can't send one for sure.  But yes, I suspect stop_ctrl is even
better due to the reasons Sagi stated.  Sorry for the confusion.

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

[Max Gurtovoy]

Hi Sagi/Christoph,

On 4/17/2018 6:24 PM, Christoph Hellwig wrote:
> On Sun, Apr 15, 2018@01:17:58PM +0300, Sagi Grimberg wrote:
>>> Christoph suggested to add the keep-alive stop to the disable/shutdown (I
>>> guess we need to add it to both, right ?) and in the target side to start
>>> expecting also once ctrl is enabled and stop when disabled.
>>
>> I don't necessarily think we need to. Also, its better to do this sooner
>> rather than later (stop_ctrl happens before disable/shutdown) and also,
>> disable/shutdown might not even execute if the transport is not connected.
> 
> I was suggesting to do it in disable as that is the point at which
> we can't send one for sure.  But yes, I suspect stop_ctrl is even
> better due to the reasons Sagi stated.  Sorry for the confusion.
> 

Actually after running more tests in our lab we found out that it must 
be symmetric (similar to my V1 that started the discussion).
For example a situation that we have many subsystems per portal.
This will create many controllers and QPs (in RDMA transport).
If we unload nvme_rdma module we'll call for each ctrl:
nvme_delete_ctrl
     queue nvme_delete_ctrl_work
         nvme_stop_ctrl
             nvme_stop_keep_alive


in this situation we'll stop the keep_alive mechanism at the initiator
before starting the IO queues destruction, that may take a while during 
high load. In this situation the KA timer can expire in the target side 
and this will follow ctrl destruction (QPs are freed...).
Let's continue in the initiator side:
we'll try to call nvme_shutdown_ctrl that reg_write32/reg_read32 from 
the ctrl that was already destroyed in the target side. This may cause 
the __nvme_submit_sync_cmd to stuck forever... (may stuck if we return 
BLK_EH_RESET_TIMER in nvme_rdma_timeout callback, as was proposed in 
IsraelR patchset that is disscussed in the mailing list and on hold now).

So I suggest we re-think about the KA fix and make the start/stop keep 
alive as symmetric as possible, even if we'll need to update RDMA/FC code...

-Max.

[Suspected-Phishing]Re: [PATCH 2/2] nvme: start keep alive timer when enabling the controller

[Max Gurtovoy]

On 4/23/2018 1:08 PM, Max Gurtovoy wrote:
> Hi Sagi/Christoph,
> 
> On 4/17/2018 6:24 PM, Christoph Hellwig wrote:
>> On Sun, Apr 15, 2018@01:17:58PM +0300, Sagi Grimberg wrote:
>>>> Christoph suggested to add the keep-alive stop to the 
>>>> disable/shutdown (I
>>>> guess we need to add it to both, right ?) and in the target side to 
>>>> start
>>>> expecting also once ctrl is enabled and stop when disabled.
>>>
>>> I don't necessarily think we need to. Also, its better to do this sooner
>>> rather than later (stop_ctrl happens before disable/shutdown) and also,
>>> disable/shutdown might not even execute if the transport is not 
>>> connected.
>>
>> I was suggesting to do it in disable as that is the point at which
>> we can't send one for sure.? But yes, I suspect stop_ctrl is even
>> better due to the reasons Sagi stated.? Sorry for the confusion.
>>
> 
> Actually after running more tests in our lab we found out that it must 
> be symmetric (similar to my V1 that started the discussion).
> For example a situation that we have many subsystems per portal.
> This will create many controllers and QPs (in RDMA transport).
> If we unload nvme_rdma module we'll call for each ctrl:
> nvme_delete_ctrl
>  ??? queue nvme_delete_ctrl_work
>  ??????? nvme_stop_ctrl
>  ??????????? nvme_stop_keep_alive
> 
> 
> in this situation we'll stop the keep_alive mechanism at the initiator
> before starting the IO queues destruction, that may take a while during 
> high load. In this situation the KA timer can expire in the target side 
> and this will follow ctrl destruction (QPs are freed...).
> Let's continue in the initiator side:
> we'll try to call nvme_shutdown_ctrl that reg_write32/reg_read32 from 
> the ctrl that was already destroyed in the target side. This may cause 
> the __nvme_submit_sync_cmd to stuck forever... (may stuck if we return 
> BLK_EH_RESET_TIMER in nvme_rdma_timeout callback, as was proposed in 
> IsraelR patchset that is disscussed in the mailing list and on hold now).

Seems like James commit "nvme: expand nvmf_check_if_ready checks" 
elimenates this command to stuck since it fails during queue_rq now..

> 
> So I suggest we re-think about the KA fix and make the start/stop keep 
> alive as symmetric as possible, even if we'll need to update RDMA/FC 
> code...
> 
> -Max.
> 
> _______________________________________________
> 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%7C20861682aa09445a86f908d5a9024756%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636600749628505218&sdata=K5g4rTCnadeCWSMBkrURad7iWxPb%2F05IhFfDz6FWGg0%3D&reserved=0 
> 

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

[Max Gurtovoy]

On 4/17/2018 6:24 PM, Christoph Hellwig wrote:
> On Sun, Apr 15, 2018@01:17:58PM +0300, Sagi Grimberg wrote:
>>> Christoph suggested to add the keep-alive stop to the disable/shutdown (I
>>> guess we need to add it to both, right ?) and in the target side to start
>>> expecting also once ctrl is enabled and stop when disabled.
>>
>> I don't necessarily think we need to. Also, its better to do this sooner
>> rather than later (stop_ctrl happens before disable/shutdown) and also,
>> disable/shutdown might not even execute if the transport is not connected.
> 
> I was suggesting to do it in disable as that is the point at which
> we can't send one for sure.  But yes, I suspect stop_ctrl is even
> better due to the reasons Sagi stated.  Sorry for the confusion.
> 

This wasn't pushed eventually to upstream.
I see an issue during error flow in case nvme_rdma_configure_io_queues 
is failed during nvme_rdma_create_ctrl.
Should we stop the keep-alive explicitly (start keep-alive was called 
during nvme_enable_ctrl ?

I can send V2..

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

[Christoph Hellwig]

On Sun, Jun 17, 2018@01:26:01PM +0300, Max Gurtovoy wrote:
>> I was suggesting to do it in disable as that is the point at which
>> we can't send one for sure.  But yes, I suspect stop_ctrl is even
>> better due to the reasons Sagi stated.  Sorry for the confusion.
>>
>
> This wasn't pushed eventually to upstream.
> I see an issue during error flow in case nvme_rdma_configure_io_queues is 
> failed during nvme_rdma_create_ctrl.
> Should we stop the keep-alive explicitly (start keep-alive was called 
> during nvme_enable_ctrl ?
>
> I can send V2..

Please resend after this much time, I don't even rember the issues we
were talking about..

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

[James Smart]

On 4/15/2018 1:47 AM, Sagi Grimberg wrote:
> From: Max Gurtuvoy <maxg at 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 Gurtuvoy <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 e1b708ee6783..4b5c3f7addeb 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1739,7 +1739,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);
>   
> @@ -3393,9 +3400,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);
>   		queue_work(nvme_wq, &ctrl->async_event_work);


Signed-off-by:? James Smart <james.smart at broadcom.com>

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

[James Smart]

On 4/15/2018 1:47 AM, Sagi Grimberg wrote:
> From: Max Gurtuvoy <maxg at 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 Gurtuvoy <maxg at mellanox.com>
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/target/core.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index fe151d672241..a81bf4d5e60c 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -643,6 +643,7 @@ static void nvmet_start_ctrl(struct nvmet_ctrl *ctrl)
>   	}
>   
>   	ctrl->csts = NVME_CSTS_RDY;
> +	mod_delayed_work(system_wq, &ctrl->ka_work, ctrl->kato * HZ);
>   }
>   
>   static void nvmet_clear_ctrl(struct n

Signed-off-by:? James Smart <james.smart at broadcom.com>
> vmet_ctrl *ctrl)

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

[Christoph Hellwig]

On Sun, Apr 15, 2018@11:47:40AM +0300, Sagi Grimberg wrote:
> From: Max Gurtuvoy <maxg at 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.

Please move a comment like this into the code (and use all 80 chars
you have available in source files :)).

Otherwise this looks good.

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

[Max Gurtovoy]

hi Sagi,

On 4/17/2018 6:24 PM, Christoph Hellwig wrote:
> On Sun, Apr 15, 2018@11:47:40AM +0300, Sagi Grimberg wrote:
>> From: Max Gurtuvoy <maxg at mellanox.com>

please copy-paste my signature for both patches (small typo):
Max Gurtovoy <maxg at 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.
> 
> Please move a comment like this into the code (and use all 80 chars
> you have available in source files :)).

are you sending V2 or I will ?


> 
> Otherwise this looks good.
>