Re: [PATCH v2 4/5] nvme-fc: Make initial connect attempt synchronous

[Hannes Reinecke <hare@suse.de>]

On 7/12/23 00:47, James Smart wrote:
> On 7/6/2023 5:07 AM, Daniel Wagner wrote:
>> Hi James,
>>
>> On Sat, Jul 01, 2023 at 05:11:11AM -0700, James Smart wrote:
>>> As much as you want to make this change to make transports "similar", 
>>> I am dead set against it unless you are completing a long qualification
>>> of the change on real FC hardware and FC-NVME devices. There is probably 
>>> 1.5 yrs of testing of different race conditions that drove this change.
>>> You cannot declare success from a simplistic toy tool such as fcloop for 
>>> validation.
>>>
>>> The original issues exist, probably have even morphed given the time 
>>> from
>>> the original change, and this will seriously disrupt the transport 
>>> and any
>>> downstream releases.  So I have a very strong NACK on this change.
>>>
>>> Yes - things such as the connect failure results are difficult to return
>>> back to nvme-cli. I have had many gripes about the nvme-cli's 
>>> behavior over
>>> the years, especially on negative cases due to race conditions which
>>> required retries. It still fails this miserably.  The async reconnect 
>>> path
>>> solved many of these issues for fc.
>>>
>>> For the auth failure, how do we deal with things if auth fails over 
>>> time as
>>> reconnects fail due to a credential changes ?  I would think 
>>> commonality of
>>> this behavior drives part of the choice.
>>
>> Alright, what do you think about the idea to introduce a new '--sync' 
>> option to
>> nvme-cli which forwards this info to the kernel that we want to wait 
>> for the
>> initial connect to succeed or fail? Obviously, this needs to handle 
>> signals too.
>>
>>  From what I understood this is also what Ewan would like to have
> To me this is not sync vs non-sync option, it's a max_reconnects value 
> tested for in nvmf_should_reconnect(). Which, if set to 0 (or 1), should 
> fail if the initial connect fails.
> 
Well, this is more a technical detail while we continue to harp about 
'sync' vs 'non-sync'.
Currently all instances of ->create_ctrl() are running asynchronously,
ie ->create_ctrl() returns a 'ctrl' object which is still in the process
of establishing the connection.
(And there it doesn't really matter whether it's FC or TCP/RDMA; FC is 
kicking of a workqueue for the 'reconnect' call, whereas TCP/RDMA is 
creating the association and issues the actual 'connect' NVMe SQE via
an I/O workqueue; net result is identical).
And when we talk about 'sync' connect we are planning to _wait_ until
this asynchronous operation reaches a steady state, ie either after the 
connect attempts succeeded or after the connect retries are exhausted.

And yes, we _are_ aware that this might be a quite long time.

> Right now max_reconnects is calculated by the ctrl_loss_tmo and 
> reconnect_delay. So there's already a way via the cli to make sure 
> there's only 1 connect attempt. I wouldn't mind seeing an exact cli 
> option that sets it to 1 connection attempt w/o the user calculation and 
> 2 value specification.
> 
Again, we do _not_ propose to change any of the default settings.
The 'sync' option will not modify the reconnect settings, it will just 
wait until a steady state it reached.

> I also assume that this is not something that would be set by default in 
> the auto-connect scripts or automated cli startup scripts.
> 
You assume correctly. That's why it'll be an additional option.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman