[PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value

[PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value

[amit.engel]

From: Amit Engel <amit.engel@dell.com>

We want to avoid starting the keep-alive timer with a zero timeout value

Signed-off-by: Amit Engel <amit.engel@dell.com>
---
 drivers/nvme/target/core.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index b92f45f5cd5b..b7883435c5e4 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1363,11 +1363,7 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 
 	ctrl->ops = req->ops;
 
-	/*
-	 * Discovery controllers may use some arbitrary high value
-	 * in order to cleanup stale discovery sessions
-	 */
-	if ((ctrl->subsys->type == NVME_NQN_DISC) && !kato)
+	if (!kato)
 		kato = NVMET_DISC_KATO_MS;
 
 	/* keep-alive timeout in seconds */
-- 
2.16.2


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value

[Christoph Hellwig]

On Tue, Aug 18, 2020 at 09:51:14AM +0300, amit.engel@dell.com wrote:
> From: Amit Engel <amit.engel@dell.com>
> 
> We want to avoid starting the keep-alive timer with a zero timeout value

So looking at the spec:

"This field specifies the timeout value for the Keep Alive feature in
milliseconds. The controller rounds up the value specified to the granularity
indicated in the KAS
field in the Identify Controller data structure. If cleared to 0h, then the
Keep Alive Timer is disabled."

we should not just set a random high timeout but just disable the
keep alive entirely.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value

[Sagi Grimberg]

>> We want to avoid starting the keep-alive timer with a zero timeout value
> 
> So looking at the spec:
> 
> "This field specifies the timeout value for the Keep Alive feature in
> milliseconds. The controller rounds up the value specified to the granularity
> indicated in the KAS
> field in the Identify Controller data structure. If cleared to 0h, then the
> Keep Alive Timer is disabled."
> 
> we should not just set a random high timeout but just disable the
> keep alive entirely.

It is already disabled, not sure I understand the patch:

--
static void nvme_start_keep_alive(struct nvme_ctrl *ctrl)
{
         if (unlikely(ctrl->kato == 0))
                 return;

         queue_delayed_work(nvme_wq, &ctrl->ka_work, ctrl->kato * HZ);
}
--

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value

[Sagi Grimberg]

>>> We want to avoid starting the keep-alive timer with a zero timeout value
>>
>> So looking at the spec:
>>
>> "This field specifies the timeout value for the Keep Alive feature in
>> milliseconds. The controller rounds up the value specified to the 
>> granularity
>> indicated in the KAS
>> field in the Identify Controller data structure. If cleared to 0h, 
>> then the
>> Keep Alive Timer is disabled."
>>
>> we should not just set a random high timeout but just disable the
>> keep alive entirely.
> 
> It is already disabled, not sure I understand the patch:
> 
> -- 
> static void nvme_start_keep_alive(struct nvme_ctrl *ctrl)
> {
>          if (unlikely(ctrl->kato == 0))
>                  return;
> 
>          queue_delayed_work(nvme_wq, &ctrl->ka_work, ctrl->kato * HZ);
> }
> -- 

Just noticed its nvmet.

We need an equivalent patch for nvmet, that's all

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

RE: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value

[Engel, Amit]

I will send an equivalent patch for nvmet

-----Original Message-----
From: Sagi Grimberg <sagi@grimberg.me> 
Sent: Tuesday, August 18, 2020 11:59 PM
To: Christoph Hellwig; Engel, Amit
Cc: linux-nvme@lists.infradead.org
Subject: Re: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value


[EXTERNAL EMAIL] 


>>> We want to avoid starting the keep-alive timer with a zero timeout 
>>> value
>>
>> So looking at the spec:
>>
>> "This field specifies the timeout value for the Keep Alive feature in 
>> milliseconds. The controller rounds up the value specified to the 
>> granularity indicated in the KAS field in the Identify Controller 
>> data structure. If cleared to 0h, then the Keep Alive Timer is 
>> disabled."
>>
>> we should not just set a random high timeout but just disable the 
>> keep alive entirely.
> 
> It is already disabled, not sure I understand the patch:
> 
> --
> static void nvme_start_keep_alive(struct nvme_ctrl *ctrl) {
>          if (unlikely(ctrl->kato == 0))
>                  return;
> 
>          queue_delayed_work(nvme_wq, &ctrl->ka_work, ctrl->kato * HZ); 
> }
> --

Just noticed its nvmet.

We need an equivalent patch for nvmet, that's all
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

RE: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value

[Engel, Amit]

Actually the spec says that NVME/TCP requires keep alive so it shouldn’t be 0.
Maybe we do need to set a default value in case of zero kato value ?
NVMe-oF 1.1:
7.4.8 Keep Alive 
The NVMe/TCP Transport requires the use of the Keep Alive feature (refer to section 7.12 in the NVMe base specification). The NVMe/TCP Transport does not impose any limitations on the minimum and maximum Keep Alive Timeout value. The minimum should be set large enough to account for any transient fabric interconnect failures between the host and controller. 
TCP level Keep Alive functionality is not prohibited but it is recommended that TCP level Keep Alive timeout is set to a higher value than the NVMe Keep Alive Timeout to avoid conflicting policies.

-----Original Message-----
From: Sagi Grimberg <sagi@grimberg.me> 
Sent: Tuesday, August 18, 2020 11:59 PM
To: Christoph Hellwig; Engel, Amit
Cc: linux-nvme@lists.infradead.org
Subject: Re: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value


[EXTERNAL EMAIL] 


>>> We want to avoid starting the keep-alive timer with a zero timeout 
>>> value
>>
>> So looking at the spec:
>>
>> "This field specifies the timeout value for the Keep Alive feature in 
>> milliseconds. The controller rounds up the value specified to the 
>> granularity indicated in the KAS field in the Identify Controller 
>> data structure. If cleared to 0h, then the Keep Alive Timer is 
>> disabled."
>>
>> we should not just set a random high timeout but just disable the 
>> keep alive entirely.
> 
> It is already disabled, not sure I understand the patch:
> 
> --
> static void nvme_start_keep_alive(struct nvme_ctrl *ctrl) {
>          if (unlikely(ctrl->kato == 0))
>                  return;
> 
>          queue_delayed_work(nvme_wq, &ctrl->ka_work, ctrl->kato * HZ); 
> }
> --

Just noticed its nvmet.

We need an equivalent patch for nvmet, that's all
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value

[Sagi Grimberg]

> Actually the spec says that NVME/TCP requires keep alive so it shouldn’t be 0.

Nothing in this language requires it. The keyword should means advice.

> Maybe we do need to set a default value in case of zero kato value ?

No, the patch is fine.

> NVMe-oF 1.1:
> 7.4.8 Keep Alive
> The NVMe/TCP Transport requires the use of the Keep Alive feature (refer to section 7.12 in the NVMe base specification). The NVMe/TCP Transport does not impose any limitations on the minimum and maximum Keep Alive Timeout value. The minimum should be set large enough to account for any transient fabric interconnect failures between the host and controller.
> TCP level Keep Alive functionality is not prohibited but it is recommended that TCP level Keep Alive timeout is set to a higher value than the NVMe Keep Alive Timeout to avoid conflicting policies.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

RE: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value

[Engel, Amit]

I was confused by the word "requires" which the spec uses (there is no "should" in the spec and "requires" sounds pretty strong):
NVMe-oF 1.1; 7.4.8 Keep Alive:
"...The NVMe/TCP Transport requires the use of the Keep Alive feature"

The same "requires" is also mentioned in RDMA:
NVMe-oF 1.1; 7.3.5 Keep Alive:
"...The RDMA Transport requires the use of the Keep Alive Feature"

Thanks for the clarification

-----Original Message-----
From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> On Behalf Of Sagi Grimberg
Sent: Wednesday, August 19, 2020 6:24 PM
To: Engel, Amit; Christoph Hellwig
Cc: linux-nvme@lists.infradead.org
Subject: Re: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value


[EXTERNAL EMAIL] 


> Actually the spec says that NVME/TCP requires keep alive so it shouldn’t be 0.

Nothing in this language requires it. The keyword should means advice.

> Maybe we do need to set a default value in case of zero kato value ?

No, the patch is fine.

> NVMe-oF 1.1:
> 7.4.8 Keep Alive
> The NVMe/TCP Transport requires the use of the Keep Alive feature (refer to section 7.12 in the NVMe base specification). The NVMe/TCP Transport does not impose any limitations on the minimum and maximum Keep Alive Timeout value. The minimum should be set large enough to account for any transient fabric interconnect failures between the host and controller.
> TCP level Keep Alive functionality is not prohibited but it is recommended that TCP level Keep Alive timeout is set to a higher value than the NVMe Keep Alive Timeout to avoid conflicting policies.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

RE: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value

[Engel, Amit]

Hi Sagi,
Still dealing with keep-alive timeout  zero value and how it affects other scenarios:
With my latest patch, when keep alive timeout is set to zero the keep-alive timer is disabled. (Upstream commit 0d3b6a8d213a30387b5104b2fb25376d18636f23)
But that’s only part of the picture,
What happens if a user starts with kato > 0 and modifying it by set feature a while after?
After modifying the kato to 0 with a set features we will never stop the timer
In a similar way, when starting with kato 0 and then modify to kato > 0 we will never start the timer

We thought about solve it by adding checks in ‘nvmet_set_feat_kato’ for the above cases
And handle the ka_work accordingly 
What do you think ?

Thanks
Amit Engel

-----Original Message-----
From: Engel, Amit 
Sent: Wednesday, August 19, 2020 7:05 PM
To: 'Sagi Grimberg'; Christoph Hellwig
Cc: linux-nvme@lists.infradead.org
Subject: RE: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value

I was confused by the word "requires" which the spec uses (there is no "should" in the spec and "requires" sounds pretty strong):
NVMe-oF 1.1; 7.4.8 Keep Alive:
"...The NVMe/TCP Transport requires the use of the Keep Alive feature"

The same "requires" is also mentioned in RDMA:
NVMe-oF 1.1; 7.3.5 Keep Alive:
"...The RDMA Transport requires the use of the Keep Alive Feature"

Thanks for the clarification

-----Original Message-----
From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> On Behalf Of Sagi Grimberg
Sent: Wednesday, August 19, 2020 6:24 PM
To: Engel, Amit; Christoph Hellwig
Cc: linux-nvme@lists.infradead.org
Subject: Re: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value


[EXTERNAL EMAIL] 


> Actually the spec says that NVME/TCP requires keep alive so it shouldn’t be 0.

Nothing in this language requires it. The keyword should means advice.

> Maybe we do need to set a default value in case of zero kato value ?

No, the patch is fine.

> NVMe-oF 1.1:
> 7.4.8 Keep Alive
> The NVMe/TCP Transport requires the use of the Keep Alive feature (refer to section 7.12 in the NVMe base specification). The NVMe/TCP Transport does not impose any limitations on the minimum and maximum Keep Alive Timeout value. The minimum should be set large enough to account for any transient fabric interconnect failures between the host and controller.
> TCP level Keep Alive functionality is not prohibited but it is recommended that TCP level Keep Alive timeout is set to a higher value than the NVMe Keep Alive Timeout to avoid conflicting policies.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value

[Sagi Grimberg]

> Hi Sagi,
> Still dealing with keep-alive timeout  zero value and how it affects other scenarios:
> With my latest patch, when keep alive timeout is set to zero the keep-alive timer is disabled. (Upstream commit 0d3b6a8d213a30387b5104b2fb25376d18636f23)
> But that’s only part of the picture,
> What happens if a user starts with kato > 0 and modifying it by set feature a while after?
> After modifying the kato to 0 with a set features we will never stop the timer
> In a similar way, when starting with kato 0 and then modify to kato > 0 we will never start the timer
> 
> We thought about solve it by adding checks in ‘nvmet_set_feat_kato’ for the above cases
> And handle the ka_work accordingly
> What do you think ?

We can probably just cancel and restart the keep alive timer in that
case..

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value

[Christoph Hellwig]

On Mon, Sep 14, 2020 at 04:53:47PM -0700, Sagi Grimberg wrote:
>
>> Hi Sagi,
>> Still dealing with keep-alive timeout  zero value and how it affects other scenarios:
>> With my latest patch, when keep alive timeout is set to zero the keep-alive timer is disabled. (Upstream commit 0d3b6a8d213a30387b5104b2fb25376d18636f23)
>> But that’s only part of the picture,
>> What happens if a user starts with kato > 0 and modifying it by set feature a while after?
>> After modifying the kato to 0 with a set features we will never stop the timer
>> In a similar way, when starting with kato 0 and then modify to kato > 0 we will never start the timer
>>
>> We thought about solve it by adding checks in ‘nvmet_set_feat_kato’ for the above cases
>> And handle the ka_work accordingly
>> What do you think ?
>
> We can probably just cancel and restart the keep alive timer in that
> case..

Yes, the Set Features handler should:

 (1) cancel the timer
 (2) update it to the new value
 (3) start the timer again (nvmet_start_keep_alive_timer will handle
     the kato == 0 case)

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

RE: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value

[Engel, Amit]

Agree. I will send a patch soon

-----Original Message-----
From: Christoph Hellwig <hch@lst.de> 
Sent: Tuesday, September 15, 2020 9:44 AM
To: Sagi Grimberg
Cc: Engel, Amit; Christoph Hellwig; linux-nvme@lists.infradead.org
Subject: Re: [PATCH] nvmet: Set ctrl->kato to a default NVMET_DISC_KATO value


[EXTERNAL EMAIL] 

On Mon, Sep 14, 2020 at 04:53:47PM -0700, Sagi Grimberg wrote:
>
>> Hi Sagi,
>> Still dealing with keep-alive timeout  zero value and how it affects other scenarios:
>> With my latest patch, when keep alive timeout is set to zero the 
>> keep-alive timer is disabled. (Upstream commit 
>> 0d3b6a8d213a30387b5104b2fb25376d18636f23)
>> But that’s only part of the picture,
>> What happens if a user starts with kato > 0 and modifying it by set feature a while after?
>> After modifying the kato to 0 with a set features we will never stop 
>> the timer In a similar way, when starting with kato 0 and then modify 
>> to kato > 0 we will never start the timer
>>
>> We thought about solve it by adding checks in ‘nvmet_set_feat_kato’ 
>> for the above cases And handle the ka_work accordingly What do you 
>> think ?
>
> We can probably just cancel and restart the keep alive timer in that 
> case..

Yes, the Set Features handler should:

 (1) cancel the timer
 (2) update it to the new value
 (3) start the timer again (nvmet_start_keep_alive_timer will handle
     the kato == 0 case)
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme