On 8/18/25 3:04 PM, Olga Kornievskaia wrote:
> On Mon, Aug 18, 2025 at 2:55 PM Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
>>
>> On Mon, Aug 18, 2025 at 2:48 PM Chuck Lever <chuck.lever@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>> ---
>>>> 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.
>> 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
