On Thu, Jan 16, 2025 at 9:55 AM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>
> On 1/16/25 9:46 AM, Jeff Layton wrote:
> > 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@xxxxxxxxxx>
> >>> ---
> >>> 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.
>
> So this would also block incoming accepts on another (active) socket?
>
> Yeah, that's not obvious.
I read these 2 comments as "more comments are needed" but I'm failing
to see what is not obvious about knowing that nothing can be running
because in the beginning of the function nfsd_mutex was acquired? And
there is already a comment in the quoted code.
I have contemplated that instead of introducing a new function into
code that isn't NFS (ie llist.h), perhaps we re-write the
nfsd_nl_listener_set_doit() to remove all from the existing transports
from lists (permsocks and sp_xprts) and create all new instead? But it
does seem an overkill for simply needing to remove something from the
list instead.
> >>> 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;
> >>> }
> >>
> >>
> >
>
>
> --
> Chuck Lever
>
