On 8/12/25 4:30 PM, Jeff Layton wrote:
> On Tue, 2025-08-12 at 16:00 -0400, Chuck Lever wrote:
>> On 8/12/25 3:57 PM, Jeff Layton wrote:
>>> On Tue, 2025-08-12 at 15:13 -0400, Olga Kornievskaia wrote:
>>>> On Tue, Aug 12, 2025 at 3:08 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>>>>>
>>>>> On 8/12/25 3:02 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.
>>>>>>
>>>>>> Removal of listeners works by first removing all transports and then
>>>>>> re-adding the ones that were not removed. In addition to destroying
>>>>>> all transports, now also call the function that unregisters everything
>>>>>> with the rpcbind. But we also then need to call the rpcbind setup
>>>>>> function before adding back new transports.
>>>>>
>>>>> Removing all rpcbind registrations and then re-adding them might
>>>>> cause an outage for clients that attempt to mount the server right
>>>>> at that moment.
>>>>
>>>> Ok I'll take a look at unregistering elsewhere. But to note, removing
>>>> a listener is only allowed when no threads are running. Thus no mounts
>>>> are possible.
>>>>
>>>
>>> Right, which is why I think this is fine.
>>
>> I think Olga's proposed solution is "fine for now" but IMO it adds a bit
>> of technical debt that we don't want, long term.
>>
>> A better solution is to make NFSD listener creation and destruction
>> complementary, if that's possible.
>>
>
> Agreed, but that's a bigger project. Note that the patch below is just
> adding rpcbind dereg/reg to what the netlink interface is already
> doing. It already blows away all of the listeners and recreates them
> when one is removed.
>
> Changing that is a bigger project, and at that point we might as well
> also allow the removal of listeners while the server is up. I'm not
> opposed to that, but it may be considerably larger change.
Change size doesn't worry me that much (it will need some strategy,
though).
But two practical questions, then:
1. Does Olga's fix need to be backported to one or more LTS kernels? If
it does, the narrower fix is probably better for now.
2. Is there a concern that the "larger" change will result in ABI
incompatibility?
IMO both of the current behaviors (replacing all registrations and
requiring the server to be shutdown) have negative impact on server
availability. Adding or removing additional listeners should be
seamless.
>>> There is a small chance a
>>> client might see the bogus rpcbind registration, but that's still
>>> better than the status quo.
>>>
>>>>>> Fixes: d093c9089260 ("nfsd: fix management of listener transports")
>>>>>> Signed-off-by: Olga Kornievskaia <okorniev@xxxxxxxxxx>
>>>>>> ---
>>>>>> fs/nfsd/nfsctl.c | 5 ++++-
>>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>>>>>> index 2909d70de559..99d06343117b 100644
>>>>>> --- a/fs/nfsd/nfsctl.c
>>>>>> +++ b/fs/nfsd/nfsctl.c
>>>>>> @@ -1998,8 +1998,11 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
>>>>>> * Since we can't delete an arbitrary llist entry, destroy the
>>>>>> * remaining listeners and recreate the list.
>>>>>> */
>>>>>> - if (delete)
>>>>>> + if (delete) {
>>>>>> svc_xprt_destroy_all(serv, net);
>>>>>> + svc_rpcb_cleanup(serv, net);
>>>>>> + svc_bind(serv, net);
>>>>>> + }
>>>>>>
>>>>>> /* walk list of addrs again, open any that still don't exist */
>>>>>> nlmsg_for_each_attr(attr, info->nlhdr, GENL_HDRLEN, rem) {
>>>>>
>>>>>
>>>>> --
>>>>> Chuck Lever
>>>>>
>>>
>>> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
>>
>
--
Chuck Lever
