Re: [PATCH 3/3] nfsd: fix management of listener transports

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2025-01-16 at 09:27 -0500, Chuck Lever wrote:
> On 1/15/25 6:24 PM, Olga Kornievskaia wrote:
> > When a particular listener is being removed we need to make sure
> > that we delete the entry from the list of permanent sockets
> > (sv_permsocks) as well as remove it from the listener transports
> > (sp_xprts). When adding back the leftover transports not being
> > removed we need to clear XPT_BUSY flag so that it can be used.
> > 
> > Fixes: 16a471177496 ("NFSD: add listener-{set,get} netlink command")
> > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> > ---
> >   fs/nfsd/nfsctl.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 95ea4393305b..3deedd511e83 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1988,7 +1988,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
> >   	/* Close the remaining sockets on the permsocks list */
> >   	while (!list_empty(&permsocks)) {
> >   		xprt = list_first_entry(&permsocks, struct svc_xprt, xpt_list);
> > -		list_move(&xprt->xpt_list, &serv->sv_permsocks);
> > +		list_del_init(&xprt->xpt_list);
> >   
> >   		/*
> >   		 * Newly-created sockets are born with the BUSY bit set. Clear
> > @@ -2000,6 +2000,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
> >   
> >   		set_bit(XPT_CLOSE, &xprt->xpt_flags);
> >   		spin_unlock_bh(&serv->sv_lock);
> > +		svc_xprt_dequeue_entry(xprt);
> 
> The kdoc comment in front of llist_del_entry() says:
> 
> + * The caller must ensure that nothing can race in and change the
> + * list while this is running.
> 
> In this caller, I don't see how such a race is prevented.
> 

This caller holds the nfsd_mutex, and prior to this, it does:

        /* For now, no removing old sockets while server is running */
        if (serv->sv_nrthreads && !list_empty(&permsocks)) {
                list_splice_init(&permsocks, &serv->sv_permsocks);
                spin_unlock_bh(&serv->sv_lock);
                err = -EBUSY;
                goto out_unlock_mtx;
        }

No nfsd threads are running and none can be started, so nothing is
processing the queue at this time. Some comments explaining that would
be a welcome addition here.

> 
> >   		svc_xprt_close(xprt);
> >   		spin_lock_bh(&serv->sv_lock);
> >   	}
> > @@ -2031,6 +2032,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
> >   
> >   		xprt = svc_find_listener(serv, xcl_name, net, sa);
> >   		if (xprt) {
> > +			clear_bit(XPT_BUSY, &xprt->xpt_flags);
> >   			svc_xprt_put(xprt);
> >   			continue;
> >   		}
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>