Received-SPF: pass (google.com: domain of linux-kernel+bounces-146372-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 16 Apr 2024 08:57:01 +0200
From: Daniel Wagner <dwagner@suse.de>
To: Sagi Grimberg <sagi@grimberg.me>
Cc: Christoph Hellwig <hch@lst.de>, Keith Busch <kbusch@kernel.org>, 
	James Smart <james.smart@broadcom.com>, Hannes Reinecke <hare@suse.de>, linux-nvme@lists.infradead.org, 
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 5/5] nvme-fabrics: handle transient auth failures
Message-ID: <qn4grwiboyzfoeqnow2decvvnq7wpymsc6yt5o6kxmmllrdlpj@6ymom3pxgsj6>
References: <20240415124220.5433-1-dwagner@suse.de>
 <20240415124220.5433-6-dwagner@suse.de>
 <51fc1a3f-7166-49cf-9652-a95bdc5d2119@grimberg.me>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <51fc1a3f-7166-49cf-9652-a95bdc5d2119@grimberg.me>

On Mon, Apr 15, 2024 at 11:51:19PM +0300, Sagi Grimberg wrote:
 > -	if (status < 0)
> > +	if (status < 0) {
> > +		/*
> > +		 * authentication errors can be transient, thus retry a couple
> > +		 * of times before giving up.
> > +		 */
> > +		if (status == -EKEYREJECTED &&
> > +		    ++ctrl->nr_auth_retries < 3)
> > +			return true;
> 
> I did not suggest nr_auth_retries.

Correct. I explained why I didn't want to use nr_reconnect counter here.

> Where is the 3 coming from? The controller already
> has a number of reconnects before it gives up, no reason to add
> another one.

Sure, but seem to you consider all EKEYREJECTED errors are transient.
But this is just not correct. There is also the case where the key is
not correct on the first connect attempt for example, or the ctrl key
gets updated but not the host key.

> Just don't return false based on the status if it is a transient
> authentication error.

Not all authentication errors are transient.

> The patch just needs to be modified from
> ??? if (status < 0)
> to
> ??? if (status < 0 && status != -EKEYREJECTED Plus a comment that explains
> it.

This is not correct. This patch is not following the specification
already and I don't think we should bend the rules too much. Hence why I
limited the reconnect attempt on auth errors to 3 attempts.