On Mon, Aug 18, 2025 at 4:47 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>
> 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@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) {
>
> 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().
Let me work on v2 for the above.
>
>
> >>>> 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
