From: James Smart <jsmart2021@gmail.com>
To: Daniel Wagner <dwagner@suse.de>
Cc: linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org,
linux-block@vger.kernel.org, Chaitanya Kulkarni <kch@nvidia.com>,
Shin'ichiro Kawasaki <shinichiro@fastmail.com>,
Sagi Grimberg <sagi@grimberg.me>, Hannes Reinecke <hare@suse.de>,
Ewan Milne <emilne@redhat.com>
Subject: Re: [PATCH v2 4/5] nvme-fc: Make initial connect attempt synchronous
Date: Tue, 11 Jul 2023 15:47:45 -0700 [thread overview]
Message-ID: <0605ac36-16d5-2026-d3c6-62d346db6dfb@gmail.com> (raw)
In-Reply-To: <7kcc75btp5bo5oqjnpqlwwo37l2f4atwfemknbmvqagrqicl2i@njn4tai7e4m7>
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.
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.
I also assume that this is not something that would be set by default in
the auto-connect scripts or automated cli startup scripts.
>
> 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?
I agree that the same connect logic makes sense for tcp/rdma. Certainly
one connect/teardown path vs one at create and one at reconnect makes
sense. The transient errors during 1st connect was the why FC added it
and I would assume tcp/rdma has it's own transient errors or timing
relationships at initial connection setups, etc.
For FC, we're trying to work around errors to transport commands (FC
NVME ELS's) that fail (dropped or timeout) or commands used to
initialize the controller which may be dropped/timeout thus fail
controller init. Although NVMe-FC does have a retransmission option, it
generally doesn't apply to the FC NVME LS's, and few of the FC devices
have yet to turn on the retransmission option to deal with the errors.
So the general behavior is connection termination and/or association
termination which then depends on the reconnect path to retry. It's also
critical as connection requests are automated on FC based on
connectivity events. If we fail out to the cli due to the fabric
dropping some up front command, there's no guarantee there will be
another connectivity event to restart the controller create and we end
up without device connectivity. The other issue we had to deal with was
how long sysadm hung out waiting for the auto-connect script to
complete. We couldn't wait the entire multiple retry case, and returning
before the 1st attempt was complete was against the spirit of the cli -
so we waited for the 1st attempt to try, released sysadm and let the
reconnect go on in the background.
>
> 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
I'll look at them.
-- james
next prev parent reply other threads:[~2023-07-11 22:48 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-20 13:37 [PATCH v2 0/5] nvme-fc: Fix blktests hangers Daniel Wagner
2023-06-20 13:37 ` [PATCH v2 1/5] nvme-fc: Do not wait in vain when unloading module Daniel Wagner
2023-06-20 13:37 ` [PATCH v2 2/5] nvme-fcloop: queue work items correctly Daniel Wagner
2023-06-20 13:37 ` [PATCH v2 3/5] nvmet-fcloop: Remove remote port from list when unlinking Daniel Wagner
2023-06-20 13:37 ` [PATCH v2 4/5] nvme-fc: Make initial connect attempt synchronous Daniel Wagner
2023-06-26 10:59 ` Dan Carpenter
2023-06-26 11:33 ` Dan Carpenter
2023-06-27 6:18 ` Daniel Wagner
2023-06-27 6:39 ` Hannes Reinecke
2023-06-27 6:51 ` Hannes Reinecke
2023-07-01 12:11 ` James Smart
2023-07-06 12:07 ` Daniel Wagner
2023-07-11 22:47 ` James Smart [this message]
2023-07-12 6:50 ` Hannes Reinecke
2023-07-13 20:35 ` Ewan Milne
2023-06-20 13:37 ` [PATCH v2 5/5] nvme-fc: do no free ctrl opts Daniel Wagner
2023-06-30 13:33 ` [PATCH v2 0/5] nvme-fc: Fix blktests hangers Ewan Milne
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0605ac36-16d5-2026-d3c6-62d346db6dfb@gmail.com \
--to=jsmart2021@gmail.com \
--cc=dwagner@suse.de \
--cc=emilne@redhat.com \
--cc=hare@suse.de \
--cc=kch@nvidia.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvme@lists.infradead.org \
--cc=sagi@grimberg.me \
--cc=shinichiro@fastmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).