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

[Sagi Grimberg]

>>>>> 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.

>>> And that error recover does _not_ wait for KATO, but rather
>>> causes an immediate retry.
>>
>> We can't blindly wait for KATO. If the controller saw the socket
>> closure, there is no point waiting kato.
>>
> Agreed. So we should be marking out FIN_WAIT1 as that one requiring us
> to wait for KATO.

You can't do that, that will wait on FIN_WAIT1 for every queue.