[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5f239919-d4bb-17ed-f8d1-8edb4f3659fc@suse.de>
Date: Wed, 12 Jul 2023 08:50:40 +0200
From: Hannes Reinecke <hare@...e.de>
To: James Smart <jsmart2021@...il.com>, Daniel Wagner <dwagner@...e.de>
Cc: linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-block@...r.kernel.org, Chaitanya Kulkarni <kch@...dia.com>,
Shin'ichiro Kawasaki <shinichiro@...tmail.com>,
Sagi Grimberg <sagi@...mberg.me>,
Ewan Milne <emilne@...hat.com>
Subject: Re: [PATCH v2 4/5] nvme-fc: Make initial connect attempt synchronous
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@...e.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
Powered by blists - more mailing lists