Re: [PATCH v3 0/2] nvme-fabrics: short-circuit connect retries

[Sagi Grimberg <sagi@grimberg.me>]

On 07/03/2024 14:52, Hannes Reinecke wrote:
> On 3/7/24 13:14, Sagi Grimberg wrote:
>>
>>
>> On 07/03/2024 13:45, Hannes Reinecke wrote:
>>> On 3/7/24 12:30, Sagi Grimberg wrote:
>>>>
> [ .. ]
>>>>
>>>> Where is this retried today, I don't see where connect failure is 
>>>> retried, outside of a periodic reconnect.
>>>> Maybe I'm missing where what is the actual failure here.
>>>
>>> static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
>>> {
>>>         struct nvme_tcp_ctrl *tcp_ctrl =
>>>                         container_of(to_delayed_work(work),
>>>                         struct nvme_tcp_ctrl, connect_work);
>>>         struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
>>>
>>>         ++ctrl->nr_reconnects;
>>>
>>>         if (nvme_tcp_setup_ctrl(ctrl, false))
>>>                 goto requeue;
>>>
>>>         dev_info(ctrl->device, "Successfully reconnected (%d 
>>> attempt)\n",
>>>                         ctrl->nr_reconnects);
>>>
>>>         ctrl->nr_reconnects = 0;
>>>
>>>         return;
>>>
>>> requeue:
>>>         dev_info(ctrl->device, "Failed reconnect attempt %d\n",
>>>
>>> and nvme_tcp_setup_ctrl() returns either a negative errno or an NVMe 
>>> status code (which might include the DNR bit).
>>
>> I thought this is about the initialization. yes today we ignore the 
>> status in re-connection assuming that whatever
>> happened, may (or may not) resolve itself. The basis for this 
>> assumption is that if we managed to connect the first
>> time there is no reason to assume that connecting again should fail 
>> persistently.
>>
> And that is another issue where I'm not really comfortable with.
> While it would make sense to have the connect functionality to be
> one-shot, and let userspace retry if needed, the problem is that we
> don't have a means of transporting that information to userspace.
> The only thing which we can transport is an error number, which
> could be anything and mean anything.

Not necessarily. error codes semantics exists for a reason.
I just really don't think that doing reconnects on a user-driven 
initialization is a good idea at all
unlike the case where the controller was connected and got disrupted, 
this is not user driven and
hence makes sense.

> If we had a defined way stating: 'This is a retryable, retry with the 
> same options.' vs 'This is retryable error, retry with modified 
> options.' vs 'This a non-retryable error, don't bother.' I'd be
> fine with delegating retries to userspace.
> But currently we don't.

Well, TBH I don't know if userspace even needs it. Most likely what a 
user would want is to define
a number of retries and give up if they expire. Adding the intelligence 
for what connect is retry-able or
not does not seem all that useful to me.

>
>> If there is a consensus that we should not assume it, its a valid 
>> argument. I didn't see where this happens with respect
>> to authentication though.
>
> nvmf_connect_admin_queue():
>
>             /* Authentication required */
>             ret = nvme_auth_negotiate(ctrl, 0);
>             if (ret) {
>                     dev_warn(ctrl->device,
>                              "qid 0: authentication setup failed\n");
>                     ret = NVME_SC_AUTH_REQUIRED;
>                     goto out_free_data;
>             }
>             ret = nvme_auth_wait(ctrl, 0);
>             if (ret)
>                     dev_warn(ctrl->device,
>                              "qid 0: authentication failed\n");
>             else
>                     dev_info(ctrl->device,
>                              "qid 0: authenticated\n");
>
> The first call to 'nvme_auth_negotiate()' is just for setting up
> the negotiation context and start the protocol. So if we get
> an error here it's pretty much non-retryable as it's completely
> controlled by the fabrics options.
> nvme_auth_wait(), OTOH, contains the actual result from the negotiation,
> so there we might or might not retry, depending on the value of 'ret'.
>
> Cheers,
>
> Hannes
>