| Message ID | 20231127153959.2067-1-thfeathers@sina.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SUNRPC: _xprt_switch_find_current_entry return xprt with condition find_active | expand |
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(). Furthermore, we do not accept patches without a real name on a Signed- off-by: line. So NACK on two accounts.
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. 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 > > >
On Mon, Nov 27, 2023 at 11:31 AM 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 > > > > > > > >
> 在 2023年11月28日，06:26，Olga Kornievskaia <aglo@umich.edu> 写道： > > On Mon, Nov 27, 2023 at 11:31 AM 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 "!". > 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 >>> trond.myklebust@hammerspace.com >>> >>> >>> >> >>
On Tue, 28 Nov 2023, thfeathers wrote: > > > 在 2023年11月28日，06:26，Olga Kornievskaia <aglo@umich.edu> 写道： > > > > On Mon, Nov 27, 2023 at 11:31 AM 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 "!". > > > 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 > >>> trond.myklebust@hammerspace.com > >>> > >>> > >>> > >> > >> > >
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;