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

[Hannes Reinecke]

On 6/28/21 3:10 AM, Chao Leng wrote:
> 
> 
> On 2021/6/27 21:39, James Smart wrote:
>> On 6/26/2021 5:09 AM, Hannes Reinecke wrote:
>>> On 6/26/21 3:03 AM, Chao Leng wrote:
>>>>
>>>>
>>>> On 2021/6/24 16:10, Hannes Reinecke wrote:
>>>>> On 6/24/21 9:29 AM, Chao Leng wrote:
>>>>>>
>>>>>>
>>>>>> On 2021/6/24 13:51, Hannes Reinecke wrote:
>>>>>>> On 6/23/21 11:38 PM, Sagi Grimberg wrote:
>>>>>>>>
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> commit f25f8ef70ce2 ("nvme-fc: short-circuit reconnect retries")
>>>>>>>>> allowed the fc transport to honour the DNR bit during reconnect
>>>>>>>>> retries, allowing to speed up error recovery.
>>>>>>>>
>>>>>>>> How does this speed up error recovery?
>>>>>>>
>>>>>>> Well, not exactly error recovery (as there is nothing to recover).
>>>>>>> But we won't attempt pointless retries, thereby reducing the 
>>>>>>> noise in
>>>>>>> the message log.
>>>>>> This conflict with the tcp and rdma target.
>>>>>> You may need to delete the improper NVME_SC_DNR at the target.
>>>>>> However, this will cause compatibility issues between different 
>>>>>> versions.
>>>>>
>>>>> Which ones?
>>>> In many scenarios, the destination sets DNR for abnormal packets,
>>>> but each new connection may not have the same error.
>>>
>>> This patch series is only for the DNR bit set in response to the 
>>> 'connect' command.
>>> If the target is not able to process the 'connect' command, but may 
>>> be so in the future it really should not set the DNR bit.
>>>
>>>>> I checked the DNR usage in the target code, and they seem to set it
>>>>> correctly (ie the result would not change when the command is 
>>>>> retried).
>>>>> With the possible exception of ENOSPC handling, as this is arguably
>>>>> dynamic and might change with a retry.
>>>> The DNR status of the old connection may not be relevant to the 
>>>> re-established connection.
>>>
>>> See above.
>>> We are just checking the DNR settings for the 'connect' command (or 
>>> any other commands being sent during initial controller configuration).
>>> If that fails the connect never was properly initialized; if the 
>>> controller would return a different status after reconnect it simply 
>>> should not set the DNR bit ...
>>>
>>> Cheers,
>>>
>>> Hannes
>>
>> Agreed. Since 1.3 spec says: "If set to ‘1’, indicates that if the 
>> same command is re-submitted to any controller in the NVM subsystem, 
>> then that re-submitted command is expected to fail."
> According to the definition of the protocol, this is not strictly 
> implemented on the target side.
> In nvme/target, there are many external errors, the DNR is set to 1.
> For example, abormal fabrics cmd.

Yes; but an abnormal fabrics cmd will be abnormal even after a reconnect.

>>
>> Thus if the initial connect fails in this manner, any new association 
>> will be on a different controller, where it is now expected connect on 
>> that controller will fail too.  Thus - why continue to connect when 
>> it's expected each will fail.
> Agree.
> We should not attempt to re-establish the connection if target can not 
> work due to target inner error .
> However, now  target does not behave exactly like this, so there are 
> conflicts and compatibility issues.

Note: we never said the target should _not_ send a DNR state. We only 
said the target should only set the DNR bit if a retry (even after 
reconnect or on a different controller) will not change the result.
Which from what I can see _is_ the case, so I do not see any issues here.

Please give details for the issues you are concerned with.

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), Geschäftsführer: Felix Imendörffer