Re: [PATCH 1/1] nfsd: unregister with rpcbind when deleting a transport

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

On 8/19/25 11:14 AM, Olga Kornievskaia wrote:
> On Mon, Aug 18, 2025 at 3:36 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 8/18/25 3:04 PM, Olga Kornievskaia wrote:
>>> On Mon, Aug 18, 2025 at 2:55 PM Olga Kornievskaia <aglo@umich.edu> wrote:
>>>>
>>>> On Mon, Aug 18, 2025 at 2:48 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>
>>>>> Hi Olga -
>>>>>
>>>>> On 8/18/25 2:25 PM, Olga Kornievskaia wrote:
>>>>>> When a listener is added, a part of creation of transport also registers
>>>>>> program/port with rpcbind. However, when the listener is removed,
>>>>>> while transport goes away, rpcbind still has the entry for that
>>>>>> port/type.
>>>>>>
>>>>>> When deleting the transport, unregister with rpcbind when appropriate.
>>>>>
>>>>> The patch description needs to explain why this is needed. Did you
>>>>> mention to me there was a crash or other malfunction?
>>>>
>>>> Malfunction is that nfsdctl removed a listener (nfsdctl listener
>>>> -udp::2049)  but rpcinfo query still shows it (rpcinfo -p |grep -w
>>>> nfs).
>>>>
>>>>>> Fixes: d093c9089260 ("nfsd: fix management of listener transports")
>>>>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
>>>>>> ---
>>>>>>  net/sunrpc/svc_xprt.c | 17 +++++++++++++++++
>>>>>>  1 file changed, 17 insertions(+)
>>>>>>
>>>>>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>>>>>> index 8b1837228799..223737fac95d 100644
>>>>>> --- a/net/sunrpc/svc_xprt.c
>>>>>> +++ b/net/sunrpc/svc_xprt.c
>>>>>> @@ -1014,6 +1014,23 @@ static void svc_delete_xprt(struct svc_xprt *xprt)
>>>>>>       struct svc_serv *serv = xprt->xpt_server;
>>>>>>       struct svc_deferred_req *dr;
>>>>>>
>>>>>> +     /* unregister with rpcbind for when transport type is TCP or UDP.
>>>>>> +      * Only TCP and RDMA sockets are marked as LISTENER sockets, so
>>>>>> +      * check for UDP separately.
>>>>>> +      */
>>>>>> +     if ((test_bit(XPT_LISTENER, &xprt->xpt_flags) &&
>>>>>> +         xprt->xpt_class->xcl_ident != XPRT_TRANSPORT_RDMA) ||
>>>>>> +         xprt->xpt_class->xcl_ident == XPRT_TRANSPORT_UDP) {
>>>>>
>>>>> Now I thought that UDP also had a rpcbind registration ... ?
>>>>
>>>> Correct.
>>>>
>>>>> So I don't
>>>>> quite understand why gating the unregistration is necessary.
>>>>
>>>> We are sending unregister for when the transport xprt is of type
>>>> LISTENER (which covers TCP but not UDP) so to also send unregister for
>>>> UDP we need to check for it separately. RDMA listener transport is
>>>> also marked as listener but we do not want to trigger unregister here
>>>> because rpcbind knows nothing about rdma type.
>>
>> rpcbind is supposed to know about the "rdma" and "rdma6" netids. The
>> convention though is not to register them. Registering those transports
>> should work just fine.
> 
> Question is: should nfsd have been registering rdma with rpcbind as well?

Solaris doesn't register it's RDMA services, so I didn't implement it
for Linux. There's no reason we couldn't, though. But probably not worth
spending time on if there isn't a practical need for it.


> __svc_rpcb_register4() takes in: program (i'm assuming nfs, acl, etc),
> version, protocol, and port.  Protocol is supposed to be a number
> (below). I don't see how "rdma" can be specified as a protocol/type.
>         switch (protocol) {
>         case IPPROTO_UDP:
>                 netid = RPCBIND_NETID_UDP;
>                 break;
>         case IPPROTO_TCP:
>                 netid = RPCBIND_NETID_TCP;
>                 break;
>         default:
>                 return -ENOPROTOOPT;
> 
> We can register nfs, tcp, port 20049 but nothing that would indicate
> that it's rdma. I have grepped thru the rpcbind source code and didn't
> find occurrences of rdma.
> 
> 
>>>> Transports are not necessarily of listener type and thus we don't want
>>>> to trigger rpcbind unregister for that.
>>
>> Ah. Maybe svc_delete_xprt() is not the right place for unregistration.
>>
>> The "listener" check here doesn't seem like the correct approach, but
>> I don't know enough about how UDP set-up works to understand how that
>> transport is registered.
>>
>> A xpo_register and a xpo_unregister method can be added to the
>> svc_xprt_ops structure to partially handle the differences. The question
>> is where should those methods be called?
>>
>>
>>> I still feel that unregistering "all" with rpcbind in nfsctl after we
>>> call svc_xprt_destroy_all() seems cleaner (single call vs a call per
>>> registered transport). But this approach would go for when listeners
>>> are allowed to be deleted while the server is running. Perhaps both
>>> patches can be considered for inclusion.
>>
>> You and Jeff both seem to want to punt on this, but...
>>
>> People will see that a transport can be removed, but having to shut down
>> the whole NFS service to do that seems... unexpected and rather useless.
>> At the very least, it would indicate to me as a sysadmin that the
>> "remove transport" feature is not finished, and is thus unusable in its
>> current form.
>>
>> An alternative is to simply disable the "remove transport" API until it
>> can be implemented correctly.
>>
>>
>>>>>> +             struct svc_sock *svsk = container_of(xprt, struct svc_sock,
>>>>>> +                                                  sk_xprt);
>>>>>> +             struct socket *sock = svsk->sk_sock;
>>>>>> +
>>>>>> +             if (svc_register(serv, xprt->xpt_net, sock->sk->sk_family,
>>>>>> +                              sock->sk->sk_protocol, 0) < 0)
>>>>>> +                     pr_warn("failed to unregister %s with rpcbind\n",
>>>>>> +                             xprt->xpt_class->xcl_name);
>>>>>> +     }
>>>>>> +
>>>>>>       if (test_and_set_bit(XPT_DEAD, &xprt->xpt_flags))
>>>>>>               return;
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Chuck Lever
>>>>>
>>>>
>>>
>>
>>
>> --
>> Chuck Lever


-- 
Chuck Lever