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

[Chuck Lever <chuck.lever@oracle.com>]

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