[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 at 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 at lists.infradead.org> On Behalf Of Sagi Grimberg
Sent: Wednesday, August 19, 2020 6:24 PM
To: Engel, Amit; Christoph Hellwig
Cc: linux-nvme at 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 at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme