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

[Chuck Lever <chuck.lever@oracle.com>]

On 1/16/25 10:34 AM, Olga Kornievskaia wrote:
> On Thu, Jan 16, 2025 at 9:55 AM Chuck Lever <chuck.lever@oracle.com> 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@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.
>>
>> 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.

The patch appears to reviewers as a diff. There is no part of the
"For now, no removing" code that appears in this fix. Even when
going back to the source code and viewing the change in context,
the "For now" code block is far enough above the new dequeue_entry()
call site that it's simply not obvious what's going on.

A new comment here might read

	/* We've already corked new work above, so this is safe: */


> 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.

Or change the management of "permanent sockets" to use a different
data structure, possibly. The temporary sockets see high traffic and
benefit from the waitless list, but listeners can use something more
conventional, as long as the thread scheduling logic looks for work
there.


>>>>>              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
>>
> 


-- 
Chuck Lever