Received-SPF: pass (google.com: domain of linux-nfs+bounces-121-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From: thfeathers <thfeathers@sina.cn>
Precedence: bulk
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH] SUNRPC: _xprt_switch_find_current_entry return xprt with condition find_active
Date: Tue, 28 Nov 2023 11:41:21 +0800
Message-Id: <609DC0E2-4262-43A3-A9CF-54AEC3FD5383@sina.cn>
References: <CAN-5tyHKddhV4OKL+ZhKKTXwoPiSug6rzRtv=Fq9KsY2wH0iPw@mail.gmail.com>
Cc: NeilBrown <neilb@suse.de>, 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
In-Reply-To: <CAN-5tyHKddhV4OKL+ZhKKTXwoPiSug6rzRtv=Fq9KsY2wH0iPw@mail.gmail.com>
To: Olga Kornievskaia <aglo@umich.edu>


> =E5=9C=A8 2023=E5=B9=B411=E6=9C=8828=E6=97=A5=EF=BC=8C06:26=EF=BC=8COlga K=
ornievskaia <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.de=
> 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 not=
 need
>>=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