Received-SPF: pass (google.com: domain of linux-nfs+bounces-154-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "thfeathers" <thfeathers@sina.cn>
Cc: "Olga Kornievskaia" <aglo@umich.edu>,
 "Trond Myklebust" <trondmy@hammerspace.com>, jlayton@kernel.org,
 chuck.lever@oracle.com, tom@talpey.com, Dai.Ngo@oracle.com, kolga@netapp.com,
 linux-nfs@vger.kernel.org
Subject: Re: [PATCH] SUNRPC: _xprt_switch_find_current_entry return xprt with
 condition find_active
In-reply-to: <609DC0E2-4262-43A3-A9CF-54AEC3FD5383@sina.cn>
References:
 <CAN-5tyHKddhV4OKL+ZhKKTXwoPiSug6rzRtv=Fq9KsY2wH0iPw@mail.gmail.com>,
 <609DC0E2-4262-43A3-A9CF-54AEC3FD5383@sina.cn>
Date: Wed, 29 Nov 2023 09:45:36 +1100
Message-id: <170121153646.7109.4907528705695653693@noble.neil.brown.name>

On Tue, 28 Nov 2023, thfeathers wrote:
>=20
> > =E5=9C=A8 2023=E5=B9=B411=E6=9C=8828=E6=97=A5=EF=BC=8C06:26=EF=BC=8COlga =
Kornievskaia <aglo@umich.edu> =E5=86=99=E9=81=93=EF=BC=9A
> >=20
> > =EF=BB=BFOn Mon, Nov 27, 2023 at 11:31=E2=80=AFAM NeilBrown <neilb@suse.d=
e> wrote:
> >>=20
> >>> 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 ]
> >>>>=20
> >>>> current function always return a active xprt or NULL no matter what
> >>>> find_active
> >>>=20
> >>>=20
> >>> This patch clearly breaks xprt_switch_find_current_entry_offline().
> >>=20
> >> I think it actually fixes xprt_switch_find_current_entry_offline().
> >>=20
> >> Looking closely at _xprt_switch_find_current_entry:
> >>=20
> >>                if (found && ((find_active && xprt_is_active(pos)) ||
> >>                              (!find_active && xprt_is_active(pos))))
> >>=20
> >> and comparing with similar code in xprt_switch_find_next_entry:
> >>=20
> >>                if (found && ((check_active && xprt_is_active(pos)) ||
> >>                              (!check_active && !xprt_is_active(pos))))
> >>=20
> >> There is a difference in the number of '!'.  I suspect the former is
> >> wrong.
> >> If the former is correct, then "find_active" is irrelevant.
> >=20
> > 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 "!".
> >=20
> return a xprt active state EQUAL find_active
> maybe =E2=80=9C&&=E2=80=9D =E2=80=9C||=E2=80=9D =E2=80=9C!=E2=80=9Dlogic no=
t need

True - and equality test of the boolean values is perfectly correct.
Maybe it is simpler.  Maybe it is a bit less common so the meaning may
not be as immediately obvious to some readers.
I see benefits in both directions, so the choice would be up to the
people who work with the code most.

Thanks for reporting this error by the way.

NeilBrown


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