On Mon, Aug 18, 2025 at 3:36 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>
> 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.
Question is: should nfsd have been registering rdma with rpcbind as well?
__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
