by Hannes Reinecke

[permalink] [raw]

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
[email protected] +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

2023-07-13 21:55:12

by Ewan Milne

[permalink] [raw]

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

The basic issue I am trying to solve with NVMe/FC is that we cannot get systems
to boot reliably from NVMe/FC (boot from SAN) because we don't know how long
the connect is going to take, and we have seen spurious failures in our testing.

In general, I think the whole business of a userspace syscall() -> asynchronous
work in the kernel is problematic because the syscall can return a good status
even if the connect is never going to work due to a condition that is
not going to
clear. It is difficult and cumbersome to script around this reliably.

So what I am saying is the current mechanism doesn't work completely
right anyway.

We also encounter the problem Daniel is addressing trying to get
blktests and other
internal tests to work, for similar reasons.

-Ewan

On Thu, Jul 6, 2023 at 8:07 AM Daniel Wagner <[email protected]> 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.
>
> Hannes thought it would make sense to use the same initial connect logic in
> tcp/rdma, because there could also be transient erros (e.g. spanning tree
> protocol). In short making the tcp/rdma do the same thing as fc?
>
> So let's drop the final patch from this series for the time. Could you give some
> feedback on the rest of the patches?
>
> Thanks,
> Daniel
>