[PATCH 1/1] nfs-utils: nfsdctl: dont ignore rdma listener return

[PATCH 1/1] nfs-utils: nfsdctl: dont ignore rdma listener return

[Olga Kornievskaia]

Don't ignore return code of adding rdma listener. If nfs.conf has asked
for "rdma=y" but adding the listener fails, don't ignore the failure.
Note in soft-rdma-provider environment (such as soft iwarp, soft roce),
when no address-constraints are used, an "any" listener is created and
rdma-enabling is done independently.

Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed")
Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
---
 utils/nfsdctl/nfsdctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
index 05fecc71..244910ef 100644
--- a/utils/nfsdctl/nfsdctl.c
+++ b/utils/nfsdctl/nfsdctl.c
@@ -1388,7 +1388,7 @@ static int configure_listeners(void)
 			if (tcp)
 				ret = add_listener("tcp", n->field, port);
 			if (rdma)
-				add_listener("rdma", n->field, rdma_port);
+				ret = add_listener("rdma", n->field, rdma_port);
 			if (ret)
 				return ret;
 		}
@@ -1398,7 +1398,7 @@ static int configure_listeners(void)
 		if (tcp)
 			ret = add_listener("tcp", "", port);
 		if (rdma)
-			add_listener("rdma", "", rdma_port);
+			ret = add_listener("rdma", "", rdma_port);
 	}
 	return ret;
 }
-- 
2.47.1

Re: [PATCH 1/1] nfs-utils: nfsdctl: dont ignore rdma listener return

[Chuck Lever]

On 2/13/25 10:47 AM, Olga Kornievskaia wrote:
> Don't ignore return code of adding rdma listener. If nfs.conf has asked
> for "rdma=y" but adding the listener fails, don't ignore the failure.
> Note in soft-rdma-provider environment (such as soft iwarp, soft roce),
> when no address-constraints are used, an "any" listener is created and
> rdma-enabling is done independently.

This behavior is confusing... I suggest that an nfs.conf man page
update accompany the below code change.

Reviewed-by: Chuck Lever <chuck.lever@oracle.com>


> Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed")
> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> ---
>  utils/nfsdctl/nfsdctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> index 05fecc71..244910ef 100644
> --- a/utils/nfsdctl/nfsdctl.c
> +++ b/utils/nfsdctl/nfsdctl.c
> @@ -1388,7 +1388,7 @@ static int configure_listeners(void)
>  			if (tcp)
>  				ret = add_listener("tcp", n->field, port);
>  			if (rdma)
> -				add_listener("rdma", n->field, rdma_port);
> +				ret = add_listener("rdma", n->field, rdma_port);
>  			if (ret)
>  				return ret;
>  		}
> @@ -1398,7 +1398,7 @@ static int configure_listeners(void)
>  		if (tcp)
>  			ret = add_listener("tcp", "", port);
>  		if (rdma)
> -			add_listener("rdma", "", rdma_port);
> +			ret = add_listener("rdma", "", rdma_port);
>  	}
>  	return ret;
>  }


-- 
Chuck Lever

Re: [PATCH 1/1] nfs-utils: nfsdctl: dont ignore rdma listener return

[Olga Kornievskaia]

On Thu, Feb 13, 2025 at 11:01 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 2/13/25 10:47 AM, Olga Kornievskaia wrote:
> > Don't ignore return code of adding rdma listener. If nfs.conf has asked
> > for "rdma=y" but adding the listener fails, don't ignore the failure.
> > Note in soft-rdma-provider environment (such as soft iwarp, soft roce),
> > when no address-constraints are used, an "any" listener is created and
> > rdma-enabling is done independently.
>
> This behavior is confusing... I suggest that an nfs.conf man page
> update accompany the below code change.

Do you find only the rdma=y soft-rdma case confusing, or do you find
that when listeners fail and we shouldn't start knfsd threads in
general confusing?

It was always the case that if rdma=y is done, then any listener
created for it does not check whether or not the underlying interface
is already rdma-enabled. This hasn't changed. Nor does this patch
change it.

> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
>
>
> > Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed")
> > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> > ---
> >  utils/nfsdctl/nfsdctl.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> > index 05fecc71..244910ef 100644
> > --- a/utils/nfsdctl/nfsdctl.c
> > +++ b/utils/nfsdctl/nfsdctl.c
> > @@ -1388,7 +1388,7 @@ static int configure_listeners(void)
> >                       if (tcp)
> >                               ret = add_listener("tcp", n->field, port);
> >                       if (rdma)
> > -                             add_listener("rdma", n->field, rdma_port);
> > +                             ret = add_listener("rdma", n->field, rdma_port);
> >                       if (ret)
> >                               return ret;
> >               }
> > @@ -1398,7 +1398,7 @@ static int configure_listeners(void)
> >               if (tcp)
> >                       ret = add_listener("tcp", "", port);
> >               if (rdma)
> > -                     add_listener("rdma", "", rdma_port);
> > +                     ret = add_listener("rdma", "", rdma_port);
> >       }
> >       return ret;
> >  }
>
>
> --
> Chuck Lever
>

Re: [PATCH 1/1] nfs-utils: nfsdctl: dont ignore rdma listener return

[Chuck Lever]

On 2/13/25 12:30 PM, Olga Kornievskaia wrote:
> On Thu, Feb 13, 2025 at 11:01 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 2/13/25 10:47 AM, Olga Kornievskaia wrote:
>>> Don't ignore return code of adding rdma listener. If nfs.conf has asked
>>> for "rdma=y" but adding the listener fails, don't ignore the failure.
>>> Note in soft-rdma-provider environment (such as soft iwarp, soft roce),
>>> when no address-constraints are used, an "any" listener is created and
>>> rdma-enabling is done independently.
>>
>> This behavior is confusing... I suggest that an nfs.conf man page
>> update accompany the below code change.
> 
> Do you find only the rdma=y soft-rdma case confusing, or do you find
> that when listeners fail and we shouldn't start knfsd threads in
> general confusing?
> 
> It was always the case that if rdma=y is done, then any listener
> created for it does not check whether or not the underlying interface
> is already rdma-enabled. This hasn't changed. Nor does this patch
> change it.

Not saying the patch changes the behavior. But you have to admit the
behavior is surprising and needs clear documentation.


>> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
>>
>>
>>> Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed")
>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
>>> ---
>>>  utils/nfsdctl/nfsdctl.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
>>> index 05fecc71..244910ef 100644
>>> --- a/utils/nfsdctl/nfsdctl.c
>>> +++ b/utils/nfsdctl/nfsdctl.c
>>> @@ -1388,7 +1388,7 @@ static int configure_listeners(void)
>>>                       if (tcp)
>>>                               ret = add_listener("tcp", n->field, port);
>>>                       if (rdma)
>>> -                             add_listener("rdma", n->field, rdma_port);
>>> +                             ret = add_listener("rdma", n->field, rdma_port);
>>>                       if (ret)
>>>                               return ret;
>>>               }
>>> @@ -1398,7 +1398,7 @@ static int configure_listeners(void)
>>>               if (tcp)
>>>                       ret = add_listener("tcp", "", port);
>>>               if (rdma)
>>> -                     add_listener("rdma", "", rdma_port);
>>> +                     ret = add_listener("rdma", "", rdma_port);
>>>       }
>>>       return ret;
>>>  }
>>
>>
>> --
>> Chuck Lever
>>
> 


-- 
Chuck Lever

Re: [PATCH 1/1] nfs-utils: nfsdctl: dont ignore rdma listener return

[Olga Kornievskaia]

On Fri, Feb 14, 2025 at 9:24 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 2/13/25 12:30 PM, Olga Kornievskaia wrote:
> > On Thu, Feb 13, 2025 at 11:01 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >> On 2/13/25 10:47 AM, Olga Kornievskaia wrote:
> >>> Don't ignore return code of adding rdma listener. If nfs.conf has asked
> >>> for "rdma=y" but adding the listener fails, don't ignore the failure.
> >>> Note in soft-rdma-provider environment (such as soft iwarp, soft roce),
> >>> when no address-constraints are used, an "any" listener is created and
> >>> rdma-enabling is done independently.
> >>
> >> This behavior is confusing... I suggest that an nfs.conf man page
> >> update accompany the below code change.
> >
> > Do you find only the rdma=y soft-rdma case confusing, or do you find
> > that when listeners fail and we shouldn't start knfsd threads in
> > general confusing?
> >
> > It was always the case that if rdma=y is done, then any listener
> > created for it does not check whether or not the underlying interface
> > is already rdma-enabled. This hasn't changed. Nor does this patch
> > change it.
>
> Not saying the patch changes the behavior. But you have to admit the
> behavior is surprising and needs clear documentation.

Sure we can document the behavior of the any listener on a soft rdma
interface as it's used by the knfsd. But is it guaranteed not to
change, as the behaviour is controlled by the RDMA core not NFS?


> >> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> >>
> >>
> >>> Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed")
> >>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> >>> ---
> >>>  utils/nfsdctl/nfsdctl.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> >>> index 05fecc71..244910ef 100644
> >>> --- a/utils/nfsdctl/nfsdctl.c
> >>> +++ b/utils/nfsdctl/nfsdctl.c
> >>> @@ -1388,7 +1388,7 @@ static int configure_listeners(void)
> >>>                       if (tcp)
> >>>                               ret = add_listener("tcp", n->field, port);
> >>>                       if (rdma)
> >>> -                             add_listener("rdma", n->field, rdma_port);
> >>> +                             ret = add_listener("rdma", n->field, rdma_port);
> >>>                       if (ret)
> >>>                               return ret;
> >>>               }
> >>> @@ -1398,7 +1398,7 @@ static int configure_listeners(void)
> >>>               if (tcp)
> >>>                       ret = add_listener("tcp", "", port);
> >>>               if (rdma)
> >>> -                     add_listener("rdma", "", rdma_port);
> >>> +                     ret = add_listener("rdma", "", rdma_port);
> >>>       }
> >>>       return ret;
> >>>  }
> >>
> >>
> >> --
> >> Chuck Lever
> >>
> >
>
>
> --
> Chuck Lever
>

Re: [PATCH 1/1] nfs-utils: nfsdctl: dont ignore rdma listener return

[Chuck Lever]

On 2/14/25 10:38 AM, Olga Kornievskaia wrote:
> On Fri, Feb 14, 2025 at 9:24 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 2/13/25 12:30 PM, Olga Kornievskaia wrote:
>>> On Thu, Feb 13, 2025 at 11:01 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>
>>>> On 2/13/25 10:47 AM, Olga Kornievskaia wrote:
>>>>> Don't ignore return code of adding rdma listener. If nfs.conf has asked
>>>>> for "rdma=y" but adding the listener fails, don't ignore the failure.
>>>>> Note in soft-rdma-provider environment (such as soft iwarp, soft roce),
>>>>> when no address-constraints are used, an "any" listener is created and
>>>>> rdma-enabling is done independently.
>>>>
>>>> This behavior is confusing... I suggest that an nfs.conf man page
>>>> update accompany the below code change.
>>>
>>> Do you find only the rdma=y soft-rdma case confusing, or do you find
>>> that when listeners fail and we shouldn't start knfsd threads in
>>> general confusing?
>>>
>>> It was always the case that if rdma=y is done, then any listener
>>> created for it does not check whether or not the underlying interface
>>> is already rdma-enabled. This hasn't changed. Nor does this patch
>>> change it.
>>
>> Not saying the patch changes the behavior. But you have to admit the
>> behavior is surprising and needs clear documentation.
> 
> Sure we can document the behavior of the any listener on a soft rdma
> interface as it's used by the knfsd. But is it guaranteed not to
> change, as the behaviour is controlled by the RDMA core not NFS?

I'm talking about documenting the opposite. Something like:

When "host=" is present, the network interface named by this
configuration setting must be present (and when "rdma=y", that
device must be RDMA-enabled) in order for server start-up to
succeed.


>>>> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
>>>>
>>>>
>>>>> Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed")
>>>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
>>>>> ---
>>>>>  utils/nfsdctl/nfsdctl.c | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
>>>>> index 05fecc71..244910ef 100644
>>>>> --- a/utils/nfsdctl/nfsdctl.c
>>>>> +++ b/utils/nfsdctl/nfsdctl.c
>>>>> @@ -1388,7 +1388,7 @@ static int configure_listeners(void)
>>>>>                       if (tcp)
>>>>>                               ret = add_listener("tcp", n->field, port);
>>>>>                       if (rdma)
>>>>> -                             add_listener("rdma", n->field, rdma_port);
>>>>> +                             ret = add_listener("rdma", n->field, rdma_port);
>>>>>                       if (ret)
>>>>>                               return ret;
>>>>>               }
>>>>> @@ -1398,7 +1398,7 @@ static int configure_listeners(void)
>>>>>               if (tcp)
>>>>>                       ret = add_listener("tcp", "", port);
>>>>>               if (rdma)
>>>>> -                     add_listener("rdma", "", rdma_port);
>>>>> +                     ret = add_listener("rdma", "", rdma_port);
>>>>>       }
>>>>>       return ret;
>>>>>  }
>>>>
>>>>
>>>> --
>>>> Chuck Lever
>>>>
>>>
>>
>>
>> --
>> Chuck Lever
>>


-- 
Chuck Lever

Re: [PATCH 1/1] nfs-utils: nfsdctl: dont ignore rdma listener return

[Olga Kornievskaia]

On Fri, Feb 14, 2025 at 10:43 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 2/14/25 10:38 AM, Olga Kornievskaia wrote:
> > On Fri, Feb 14, 2025 at 9:24 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >> On 2/13/25 12:30 PM, Olga Kornievskaia wrote:
> >>> On Thu, Feb 13, 2025 at 11:01 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>
> >>>> On 2/13/25 10:47 AM, Olga Kornievskaia wrote:
> >>>>> Don't ignore return code of adding rdma listener. If nfs.conf has asked
> >>>>> for "rdma=y" but adding the listener fails, don't ignore the failure.
> >>>>> Note in soft-rdma-provider environment (such as soft iwarp, soft roce),
> >>>>> when no address-constraints are used, an "any" listener is created and
> >>>>> rdma-enabling is done independently.
> >>>>
> >>>> This behavior is confusing... I suggest that an nfs.conf man page
> >>>> update accompany the below code change.
> >>>
> >>> Do you find only the rdma=y soft-rdma case confusing, or do you find
> >>> that when listeners fail and we shouldn't start knfsd threads in
> >>> general confusing?
> >>>
> >>> It was always the case that if rdma=y is done, then any listener
> >>> created for it does not check whether or not the underlying interface
> >>> is already rdma-enabled. This hasn't changed. Nor does this patch
> >>> change it.
> >>
> >> Not saying the patch changes the behavior. But you have to admit the
> >> behavior is surprising and needs clear documentation.
> >
> > Sure we can document the behavior of the any listener on a soft rdma
> > interface as it's used by the knfsd. But is it guaranteed not to
> > change, as the behaviour is controlled by the RDMA core not NFS?
>
> I'm talking about documenting the opposite. Something like:
>
> When "host=" is present, the network interface named by this
> configuration setting must be present (and when "rdma=y", that
> device must be RDMA-enabled) in order for server start-up to
> succeed.

Ah that case. Yes, I agree.

> >>>> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> >>>>
> >>>>
> >>>>> Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed")
> >>>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> >>>>> ---
> >>>>>  utils/nfsdctl/nfsdctl.c | 4 ++--
> >>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> >>>>> index 05fecc71..244910ef 100644
> >>>>> --- a/utils/nfsdctl/nfsdctl.c
> >>>>> +++ b/utils/nfsdctl/nfsdctl.c
> >>>>> @@ -1388,7 +1388,7 @@ static int configure_listeners(void)
> >>>>>                       if (tcp)
> >>>>>                               ret = add_listener("tcp", n->field, port);
> >>>>>                       if (rdma)
> >>>>> -                             add_listener("rdma", n->field, rdma_port);
> >>>>> +                             ret = add_listener("rdma", n->field, rdma_port);
> >>>>>                       if (ret)
> >>>>>                               return ret;
> >>>>>               }
> >>>>> @@ -1398,7 +1398,7 @@ static int configure_listeners(void)
> >>>>>               if (tcp)
> >>>>>                       ret = add_listener("tcp", "", port);
> >>>>>               if (rdma)
> >>>>> -                     add_listener("rdma", "", rdma_port);
> >>>>> +                     ret = add_listener("rdma", "", rdma_port);
> >>>>>       }
> >>>>>       return ret;
> >>>>>  }
> >>>>
> >>>>
> >>>> --
> >>>> Chuck Lever
> >>>>
> >>>
> >>
> >>
> >> --
> >> Chuck Lever
> >>
>
>
> --
> Chuck Lever

Re: [PATCH 1/1] nfs-utils: nfsdctl: dont ignore rdma listener return

[Jeff Layton]

On Thu, 2025-02-13 at 10:47 -0500, Olga Kornievskaia wrote:
> Don't ignore return code of adding rdma listener. If nfs.conf has asked
> for "rdma=y" but adding the listener fails, don't ignore the failure.
> Note in soft-rdma-provider environment (such as soft iwarp, soft roce),
> when no address-constraints are used, an "any" listener is created and
> rdma-enabling is done independently.
> 
> Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed")
> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> ---
>  utils/nfsdctl/nfsdctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> index 05fecc71..244910ef 100644
> --- a/utils/nfsdctl/nfsdctl.c
> +++ b/utils/nfsdctl/nfsdctl.c
> @@ -1388,7 +1388,7 @@ static int configure_listeners(void)
>  			if (tcp)
>  				ret = add_listener("tcp", n->field, port);
>  			if (rdma)
> -				add_listener("rdma", n->field, rdma_port);
> +				ret = add_listener("rdma", n->field, rdma_port);
>  			if (ret)
>  				return ret;
>  		}
> @@ -1398,7 +1398,7 @@ static int configure_listeners(void)
>  		if (tcp)
>  			ret = add_listener("tcp", "", port);
>  		if (rdma)
> -			add_listener("rdma", "", rdma_port);
> +			ret = add_listener("rdma", "", rdma_port);
>  	}
>  	return ret;
>  }


add_listener() is just adding the given listener to the nfsd_sockets
array in memory. It's not consulting the kernel at that point. That
won't happen until set_listeners() is called.

It does make sense to check the return code though, just in case we
pass it some bogus arguments somehow. So...

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/1] nfs-utils: nfsdctl: dont ignore rdma listener return

[Steve Dickson]

On 2/13/25 10:47 AM, Olga Kornievskaia wrote:
> Don't ignore return code of adding rdma listener. If nfs.conf has asked
> for "rdma=y" but adding the listener fails, don't ignore the failure.
> Note in soft-rdma-provider environment (such as soft iwarp, soft roce),
> when no address-constraints are used, an "any" listener is created and
> rdma-enabling is done independently.
> 
> Fixes: e3b72007ab31 ("nfs-utils: nfsdctl: cleanup listeners if some failed")
> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
Committed... (tag: nfs-utils-2-8-3-rc7)

steved.

> ---
>   utils/nfsdctl/nfsdctl.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> index 05fecc71..244910ef 100644
> --- a/utils/nfsdctl/nfsdctl.c
> +++ b/utils/nfsdctl/nfsdctl.c
> @@ -1388,7 +1388,7 @@ static int configure_listeners(void)
>   			if (tcp)
>   				ret = add_listener("tcp", n->field, port);
>   			if (rdma)
> -				add_listener("rdma", n->field, rdma_port);
> +				ret = add_listener("rdma", n->field, rdma_port);
>   			if (ret)
>   				return ret;
>   		}
> @@ -1398,7 +1398,7 @@ static int configure_listeners(void)
>   		if (tcp)
>   			ret = add_listener("tcp", "", port);
>   		if (rdma)
> -			add_listener("rdma", "", rdma_port);
> +			ret = add_listener("rdma", "", rdma_port);
>   	}
>   	return ret;
>   }