Received-SPF: pass (google.com: domain of linux-nfs+bounces-112-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231127153959.2067-1-thfeathers@sina.cn> <aa9e250a966c47782f79d258ea9818ae4fcbdbc5.camel@hammerspace.com>
 <170112068218.7109.1172633879607916557@noble.neil.brown.name>
In-Reply-To: <170112068218.7109.1172633879607916557@noble.neil.brown.name>
From: Olga Kornievskaia <aglo@umich.edu>
Date: Mon, 27 Nov 2023 12:26:06 -1000
Message-ID: <CAN-5tyHKddhV4OKL+ZhKKTXwoPiSug6rzRtv=Fq9KsY2wH0iPw@mail.gmail.com>
Subject: Re: [PATCH] SUNRPC: _xprt_switch_find_current_entry return xprt with
 condition find_active
To: NeilBrown <neilb@suse.de>
Cc: Trond Myklebust <trondmy@hammerspace.com>, "jlayton@kernel.org" <jlayton@kernel.org>, 
	"thfeathers@sina.cn" <thfeathers@sina.cn>, "chuck.lever@oracle.com" <chuck.lever@oracle.com>, 
	"tom@talpey.com" <tom@talpey.com>, "Dai.Ngo@oracle.com" <Dai.Ngo@oracle.com>, 
	"kolga@netapp.com" <kolga@netapp.com>, "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Nov 27, 2023 at 11:31=E2=80=AFAM NeilBrown <neilb@suse.de> wrote:
>
> On Tue, 28 Nov 2023, Trond Myklebust wrote:
> > On Mon, 2023-11-27 at 23:39 +0800, jsq wrote:
> > > [You don't often get email from thfeathers@sina.cn. Learn why this is
> > > important at https://aka.ms/LearnAboutSenderIdentification ]
> > >
> > > current function always return a active xprt or NULL no matter what
> > > find_active
> >
> >
> > This patch clearly breaks xprt_switch_find_current_entry_offline().
>
> I think it actually fixes xprt_switch_find_current_entry_offline().
>
> Looking closely at _xprt_switch_find_current_entry:
>
>                 if (found && ((find_active && xprt_is_active(pos)) ||
>                               (!find_active && xprt_is_active(pos))))
>
> and comparing with similar code in xprt_switch_find_next_entry:
>
>                 if (found && ((check_active && xprt_is_active(pos)) ||
>                               (!check_active && !xprt_is_active(pos))))
>
> There is a difference in the number of '!'.  I suspect the former is
> wrong.
> If the former is correct, then "find_active" is irrelevant.

Thanks Neil for pointing it out. We need the "find_active", otherwise
as Trond pointed out it breaks the offline function. But I do believe
I missed the "!" in the logic. I believe the reason this hasn't caused
problems is because for the offline transports we never use the
xprt_iter_xprt(). We only iterate thru the get_next when we iterate
offline transports. But I should fix the function that adds the "!".

>
> NeilBrown
>
> > Furthermore, we do not accept patches without a real name on a Signed-
> > off-by: line.
> >
> > So NACK on two accounts.
> >
> > --
> > Trond Myklebust
> > Linux NFS client maintainer, Hammerspace
> > trond.myklebust@hammerspace.com
> >
> >
> >
>
>