Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <594f73f2-59b0-bbcb-d7a0-6d89e2446830@gmail.com>
Date:   Sat, 1 Jul 2023 05:11:11 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.2
Subject: Re: [PATCH v2 4/5] nvme-fc: Make initial connect attempt synchronous
Content-Language: en-US
To:     Daniel Wagner <dwagner@suse.de>, linux-nvme@lists.infradead.org
Cc:     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>,
        James Smart <jsmart2021@gmail.com>
References: <20230620133711.22840-1-dwagner@suse.de>
 <20230620133711.22840-5-dwagner@suse.de>
From:   James Smart <jsmart2021@gmail.com>
In-Reply-To: <20230620133711.22840-5-dwagner@suse.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 6/20/2023 6:37 AM, Daniel Wagner wrote:
> Commit 4c984154efa1 ("nvme-fc: change controllers first connect to use
> reconnect path") made the connection attempt asynchronous in order to
> make the connection attempt from autoconnect/boot via udev/systemd up
> case a bit more reliable.
> 
> Unfortunately, one side effect of this is that any wrong parameters
> provided from userspace will not be directly reported as invalid, e.g.
> auth keys.
> 
> So instead having the policy code inside the kernel it's better to
> address this in userspace, for example in nvme-cli or nvme-stas.
> 
> This aligns the fc transport with tcp and rdma.

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.

-- james