[PATCH 3/3] nvme: start keep-alive after admin queue setup

[Hannes Reinecke]

On 11/20/23 14:39, Sagi Grimberg wrote:
> 
>> Setting up I/O queues might take quite some time on larger and/or
>> busy setups, so KATO might expire before all I/O queues could be
>> set up.
>> Fix this by start keep alive from the ->init_ctrl_finish() callback,
>> and stopping it when calling nvme_cancel_admin_tagset().
> 
> If this is a fix, the title should describe the issue it is fixing, and
> the body should say how it is fixing it.
> 
>> Signed-off-by: Hannes Reinecke <hare at suse.de>
>> ---
>>   drivers/nvme/host/core.c | 6 +++---
>>   drivers/nvme/host/fc.c   | 6 ++++++
>>   2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 62612f87aafa..f48b4f735d2d 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -483,6 +483,7 @@ EXPORT_SYMBOL_GPL(nvme_cancel_tagset);
>>   void nvme_cancel_admin_tagset(struct nvme_ctrl *ctrl)
>>   {
>> +    nvme_stop_keep_alive(ctrl);
>>       if (ctrl->admin_tagset) {
>>           blk_mq_tagset_busy_iter(ctrl->admin_tagset,
>>                   nvme_cancel_request, ctrl);
> 
> There is a cross dependency here, now nvme_cancel_admin_tagset needs to
> have the keep-alive stopped first, which may be waiting on I/O, which
> needs to be cancelled...
> 
> Keep in mind that kato can be arbitrarily long, and now this function
> may be blocked on this kato period.
> 
> I also think that now the function is doing something that is more
> than simply cancelling the inflight admin tagset, as it is named.
> 
I am having a hard time following this reasoning.
While I do accept that nvme_stop_keep_alive() might trigger I/O
(ie if the work queue has just been started when calling 
cancel_delayed_work), nvme_tcp_error_recovery_work() has this:

	nvme_stop_keep_alive(ctrl);
	flush_work(&ctrl->async_event_work);
	nvme_tcp_teardown_io_queues(ctrl, false);
	/* unquiesce to fail fast pending requests */
	nvme_unquiesce_io_queues(ctrl);
	nvme_tcp_teardown_admin_queue(ctrl, false);

and nvme_tcp_teardown_admin_queue() calls nvme_cancel_admin_tagset().
So by your above reasoning this code should be wrong, too.
What am I missing?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare at suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Ivo Totev, Andrew McDonald,
Werner Knoblich