[PATCH 0/3] nvme-tcp: start error recovery after KATO

[Sagi Grimberg]

On 12/4/23 13:37, Hannes Reinecke wrote:
> On 12/4/23 11:30, Sagi Grimberg wrote:
>>
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> there have been some very insistent reports of data corruption
>>>>>>>> with certain target implementations due to command retries.
>>>>>>>
>>>>>>> None of which were reported on this list...
>>>>>>>
>>>>>> Correct. I can ask them to post their finding here if that would 
>>>>>> make a difference.
>>>>>>
>>>>>>>> Problem here is that for TCP we're starting error recovery
>>>>>>>> immediately after either a command timeout or a (local) link loss.
>>>>>>>
>>>>>>> It does so only in one occasion, when the user triggered a
>>>>>>> reset_controller. a command timeout is greater than the default
>>>>>>> kato (6 times in fact), was this the case where the issue was
>>>>>>> observed? If so, the timeout handler should probably just wait
>>>>>>> the kato remaining time.
>>>>>>>
>>>>>> Nothing to do with reset_controller.
>>>>>> The problem really is in nvme_tcp_state_change(), which will 
>>>>>> blindly start error recovery (and a subsequent command retry) 
>>>>>> whenever the local link drops.
>>>>>
>>>>> That is incorrect. A link drop does not trigger a socket state change.
>>>>> a state change is triggered for specific socket states
>>>>> (see net/ipv4/tcp.c for description of socket states).
>>>>>
>>>>> These are a set of states that describes local and remote sides
>>>>> shutting down (or a TIME_WAIT). The cases where the host
>>>>> triggers error recovery without the controller acknowledging it
>>>>> (in case the link is down) are FIN_WAIT1/2, and these happen
>>>>> when the user triggers a controller reset/disconnect.
>>>>>
>>>>> In the other circumstances, we'll see a TCP_CLOSE/CLOSE_WAIT/CLOSING
>>>>> which should indicate the controller acknowledged the shutdown.
>>>>>
>>>>
>>>> Ah, now. It's really neither.
>>>> Complaint from the customer was that we are retrying commands 
>>>> without waiting for KATO. Actual claim was that we're retrying 
>>>> more-or-less immediately.
>>>>
>>>> So, the problem here is not the first command running into a timeout;
>>>> the problem really is with the _subsequent_ commands running into a 
>>>> timeout (cf my comments to patch 1/3).
>>>> The first command will set the status to RESETTING and start error 
>>>> recovery, all fine. But all _other_ commands will be aborted directly,
>>>> causing them to be retried immediately without any delay.
>>>
>>> This is still unclear to me.
>>> The default io timeout is 30s, which is 6 times larger than the
>>> default kato. So effectively we are hitting a case where we waited
>>> 6 kato periods, and with your patch we are waiting 7 kato periods?
>>>
>>> There is a disconnect for me here.
>>>
>>> Can you please clarify:
>>> 1. what is your kato?
>>> 2. what is your io timeout?
>>> 3. was the controller RESETTING state triggered from kato? or
>>>     from io timeout?
>>>     if the latter, it indicates that kato is higher than io timeout,
>>>     otherwise the transport is perfectly fine, and this is essentially
>>>     a controller io that is taking longer than the host io timeout?
>>>
>>> Do you have some trace that points out how a command is sent down to
>>> the controller (successfully arrives to the controller) the network goes
>>> down, and the io is retried before kato expires?
>>>
>>> What is unclear to me here, is how kato expires, but the controller
>>> continues to process commands.
>>>
> 
> Argument here is that there is a race window between command timeout
> triggering (ie the call to nvme_tcp_timeout()) and scheduling of the
> 'err_work' workqueue function. If you have a burst of commands all
> expiring at roughly the same time the first command will set the 
> controller state to 'RESETTING', and schedule the 'err_work' workqueue
> function.

Yes, that is correct. It is also the keep-alive itself that can
trigger the timeout handler. In fact, it is the more likely one
because it will by default have a lower timeout than IO commands.

This is a part of where I lose the trail here, because I'd expect
the keep-alive to expire far before any of the IO commands, especially
with the latest changes to expedite the keep-alive execution pace.

> The second command will timing out _before_ err_work is scheduled,
> will see the RESETTING state in nvme_tcp_timeout(), and directly
> complete and retry (via nvme_tcp_complete_timed_out()) without
> waiting for KATO.

That is something we probably can address by analyzing the controller
state a bit deeper. We've always failed IO commands quickly in the
timeout handler because these commands can be part of the ctrl setup
sequence, which means the error recovery will not run and fail the 
commands on their behalf reliably. I also think there were issues with
issues in the teardown stage (shutdown or disable).

> That is the issue the customer had been seing, and that is what is
> causing data corruption on their end.
> 
> This patchset is an attempt to fix this issue.

In your patch 3, you take nvme_keep_alive_work_period(), which is
either kato/2 or kato/4. It is not clear to me why these values
were chosen other than "the issue went away".

Also, what happens if kato (which is user configurable), is arbitrarily
long? This means that IO failover can take say 60 minutes? Do you think
its reasonable? This is where I'm concerned about the unconditional
delay.

I think that the idea was that an appropriate delay would be indicated
by the controller, rather than being determined by a user controlled
variable.

Regardless we should limit to exactly when such a delay is actually
needed.

Plus, I haven't seen anything for nvme-rdma and it is unclear to me
why this is a host specific to tcp.

> We'd be happy to pick up this thread, it just had been pushed back
> internally as we put in a band-aid for that customer in our kernels...

Well, my assumption is that we want this addressed upstream. Just a
matter of how.