current function always return a active xprt or NULL no matter what find_active
---
net/sunrpc/xprtmultipath.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/sunrpc/xprtmultipath.c b/net/sunrpc/xprtmultipath.c
index 701250b305db..94f3b5f444a1 100644
--- a/net/sunrpc/xprtmultipath.c
+++ b/net/sunrpc/xprtmultipath.c
@@ -283,8 +283,7 @@ struct rpc_xprt *_xprt_switch_find_current_entry(struct list_head *head,
list_for_each_entry_rcu(pos, head, xprt_switch) {
if (cur == pos)
found = true;
- if (found && ((find_active && xprt_is_active(pos)) ||
- (!find_active && xprt_is_active(pos))))
+ if (found && (find_active == xprt_is_active(pos)))
return pos;
}
return NULL;
--
2.43.0.windows.1
On Mon, 2023-11-27 at 23:39 +0800, jsq wrote:
> [You don't often get email from [email protected]. 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().
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
[email protected]
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 [email protected]. 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.
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
> [email protected]
>
>
>
On Mon, Nov 27, 2023 at 11:31 AM NeilBrown <[email protected]> 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 [email protected]. 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
> > [email protected]
> >
> >
> >
>
>
> 在 2023年11月28日，06:26，Olga Kornievskaia <[email protected]> 写道：
>
> On Mon, Nov 27, 2023 at 11:31 AM NeilBrown <[email protected]> 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 [email protected]. 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 "!".
>
return a xprt active state EQUAL find_active
maybe “&&” “||” “!”logic not need
>>
>> 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
>>> [email protected]
>>>
>>>
>>>
>>
>>
On Tue, 28 Nov 2023, thfeathers wrote:
>
> > 在 2023年11月28日，06:26，Olga Kornievskaia <[email protected]> 写道：
> >
> > On Mon, Nov 27, 2023 at 11:31 AM NeilBrown <[email protected]> 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 [email protected]. 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 "!".
> >
> return a xprt active state EQUAL find_active
> maybe “&&” “||” “!”logic not 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
> >>
> >> 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
> >>> [email protected]
> >>>
> >>>
> >>>
> >>
> >>
>
>