Support keep alive in spdk tcp backend about xnvme

[karlowich]

Hi @LiadOz, thanks for putting this on our radar. We will have to spend some time coming up with the optimal solution.
For now, I think you should be able to achieve the desired outcome by sending the Keep Alive command "manually" through xnvme_cmd_pass_admin(). Can you try this out?

[LiadOz]

This would technically work as a workaround. If keep_alive_timeout_ms is set, spdk will send the keep alive command on it's own when spdk_nvme_ctrlr_process_admin_completions is called. So with the solution you provided 2 keep alives will be sent, one by the user, and the other one by spdk. The target probably doesn't care that it will get multiple keep alives at the same time.
I will test this solution on my environment and see if it works.

[LiadOz]

@karlowich This solution seems to work for my use case. In my fork I added spdk_keep_alive_timeout_ms field to xnvme_opts. Can I open a PR for this change?

[karlowich]

@LiadOz as you point out in your previous comment spdk_keep_alive_timeout_ms is not needed when sending the keep alive manually. So I'm not sure I see why we would use spdk_keep_alive_timeout_ms as long as we don't support manually calling spdk_nvme_ctrlr_process_admin_completions.

But maybe I'm missing something?

[LiadOz]

spdk_keep_alive_timeout_ms is first passed to the target in the initialization phase. In addition when spdk_nvme_ctrlr_process_admin_completions is called it checks if the keep alive timeout is activated and if a sufficient amount of time has passed then it sends the keep alive command. So it has two purposes.

[karlowich]

@LiadOz Yes, but as far as I can tell, if you don't plan on calling spdk_nvme_ctrlr_process_admin_completions there is no need to set spdk_keep_alive_timeout_ms.

[LiadOz]

I'm saying that the parameter spdk_keep_alive_timeout_ms does two things. First it sets the keep alive value for the target. So when the target got that value, it knows that if spdk_keep_alive_timeout_ms passes without a keep alive then it will abort all of the other commands. This is the behavior that I need.

In addition, when this value is set in the spdk of the host side, when spdk_nvme_ctrlr_process_admin_completions is called, then if a sufficient amount of time has passed it will send a keep alive command on its own.

So with the solution you suggested, since I can't call spdk_nvme_ctrlr_process_admin_completions directly, I will send keep alive commands on my own. But I need to let the target know what is the acceptable keep alive timeout, that's why I need to set spdk_keep_alive_timeout_ms.
This parameter affects multiple behaviors, that's why I still need it.

[karlowich]

@LiadOz Sorry, you are absolutely right. Now I see the value of being able to set spdk_keep_alive_timeout_ms, you can go ahead and open a PR.

[karlowich]

@LiadOz Can I close this?

[LiadOz]

My PR only added the option to enable the keep alive feature. The way we are sending the keep alive commands is workaround/hack, I don't think this ticket should be closed until a proper solution for it is implemented. But if you think the current workaround is good enough you can close this ticket. I have no problem with continuing to work with the hack.

[karlowich]

@LiadOz Good point, let's keep this open to remind us in the future.