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

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

On 8/18/25 3:36 PM, Chuck Lever 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) {

I still think this check is unnecessarily confusing.

Can you instead add an XPT_RPCB_UNREG_ON_CLOSE flag in the
svc_xprt::xpt_flags field? Check if that one flag is set here.

Set XPT_RPCB_UNREG_ON_CLOSE for TCP listener sockets and all UDP
sockets.

An additional clean-up might be to add an svc_rpcb_cleanup() call to
svc_xprt_destroy_all(), and remove svc_rpcb_cleanup() from the upper
layer callers to svc_xprt_destroy_all().


>>>> 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.
> 
> 
>>> 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