[0/5,v2] sunrpc: stop refcounting svc_serv

Message

NeilBrown Dec. 15, 2023, 12:56 a.m. UTC

I sent an earlier version of this series, got some feed back, revised
it, but never sent it again.  Sorry.

The main feedback was around the interaction between sunrpc and nfsd for
handling poolstats.  I have changed that so that nfsd tells sunrpc where
the svc_serv pointer lives, and where to find a mutex to protect it.
sunrpc then taks the mutex and accesses the pointer - if not NULL.  I
think this is nicer than the version that pass around funciton pointers.

This series is against nfsd-next

Thanks,
NeilBrown


 [PATCH 1/5] nfsd: call nfsd_last_thread() before final nfsd_put()
 [PATCH 2/5] svc: don't hold reference for poolstats, only mutex.
 [PATCH 3/5] nfsd: hold nfsd_mutex across entire netlink operation
 [PATCH 4/5] SUNRPC: discard sv_refcnt, and svc_get/svc_put
 [PATCH 5/5] nfsd: rename nfsd_last_thread() to nfsd_destroy_serv()

Comments

Jeff Layton Dec. 15, 2023, 10:59 a.m. UTC | #1

On Fri, 2023-12-15 at 11:56 +1100, NeilBrown wrote:
> I sent an earlier version of this series, got some feed back, revised
> it, but never sent it again.  Sorry.
> 
> The main feedback was around the interaction between sunrpc and nfsd for
> handling poolstats.  I have changed that so that nfsd tells sunrpc where
> the svc_serv pointer lives, and where to find a mutex to protect it.
> sunrpc then taks the mutex and accesses the pointer - if not NULL.  I
> think this is nicer than the version that pass around funciton pointers.
> 
> This series is against nfsd-next
> 
> Thanks,
> NeilBrown
> 
> 
>  [PATCH 1/5] nfsd: call nfsd_last_thread() before final nfsd_put()
>  [PATCH 2/5] svc: don't hold reference for poolstats, only mutex.
>  [PATCH 3/5] nfsd: hold nfsd_mutex across entire netlink operation
>  [PATCH 4/5] SUNRPC: discard sv_refcnt, and svc_get/svc_put
>  [PATCH 5/5] nfsd: rename nfsd_last_thread() to nfsd_destroy_serv()

I'm not sure patch #2 is better than the version with function pointers,
but it seems reasonable.

Note that patch #1 probably needs to go to v6.6 stable, and I think we
want #3 in v6.7 before it ships.

I think I sent this on the earlier set, but I'll send it again:

Reviewed-by: Jeff Layton <[e-mail]>

Chuck Lever Dec. 15, 2023, 2:19 p.m. UTC | #2

> On Dec 15, 2023, at 5:59 AM, Jeff Layton <[e-mail]> wrote:
> 
> On Fri, 2023-12-15 at 11:56 +1100, NeilBrown wrote:
>> I sent an earlier version of this series, got some feed back, revised
>> it, but never sent it again.  Sorry.
>> 
>> The main feedback was around the interaction between sunrpc and nfsd for
>> handling poolstats.  I have changed that so that nfsd tells sunrpc where
>> the svc_serv pointer lives, and where to find a mutex to protect it.
>> sunrpc then taks the mutex and accesses the pointer - if not NULL.  I
>> think this is nicer than the version that pass around funciton pointers.
>> 
>> This series is against nfsd-next
>> 
>> Thanks,
>> NeilBrown
>> 
>> 
>> [PATCH 1/5] nfsd: call nfsd_last_thread() before final nfsd_put()
>> [PATCH 2/5] svc: don't hold reference for poolstats, only mutex.
>> [PATCH 3/5] nfsd: hold nfsd_mutex across entire netlink operation
>> [PATCH 4/5] SUNRPC: discard sv_refcnt, and svc_get/svc_put
>> [PATCH 5/5] nfsd: rename nfsd_last_thread() to nfsd_destroy_serv()
> 
> I'm not sure patch #2 is better than the version with function pointers,
> but it seems reasonable.
> 
> Note that patch #1 probably needs to go to v6.6 stable, and I think we
> want #3 in v6.7 before it ships.

Remind me why #3 should go into v6.7-rc ? There's no Fixes tag on
that one.


> I think I sent this on the earlier set, but I'll send it again:
> 
> Reviewed-by: Jeff Layton <[e-mail]>

--
Chuck Lever

Jeff Layton Dec. 15, 2023, 2:26 p.m. UTC | #3

On Fri, 2023-12-15 at 14:19 +0000, Chuck Lever III wrote:
> > On Dec 15, 2023, at 5:59 AM, Jeff Layton <[e-mail]> wrote:
> > 
> > On Fri, 2023-12-15 at 11:56 +1100, NeilBrown wrote:
> > > I sent an earlier version of this series, got some feed back, revised
> > > it, but never sent it again.  Sorry.
> > > 
> > > The main feedback was around the interaction between sunrpc and nfsd for
> > > handling poolstats.  I have changed that so that nfsd tells sunrpc where
> > > the svc_serv pointer lives, and where to find a mutex to protect it.
> > > sunrpc then taks the mutex and accesses the pointer - if not NULL.  I
> > > think this is nicer than the version that pass around funciton pointers.
> > > 
> > > This series is against nfsd-next
> > > 
> > > Thanks,
> > > NeilBrown
> > > 
> > > 
> > > [PATCH 1/5] nfsd: call nfsd_last_thread() before final nfsd_put()
> > > [PATCH 2/5] svc: don't hold reference for poolstats, only mutex.
> > > [PATCH 3/5] nfsd: hold nfsd_mutex across entire netlink operation
> > > [PATCH 4/5] SUNRPC: discard sv_refcnt, and svc_get/svc_put
> > > [PATCH 5/5] nfsd: rename nfsd_last_thread() to nfsd_destroy_serv()
> > 
> > I'm not sure patch #2 is better than the version with function pointers,
> > but it seems reasonable.
> > 
> > Note that patch #1 probably needs to go to v6.6 stable, and I think we
> > want #3 in v6.7 before it ships.
> 
> Remind me why #3 should go into v6.7-rc ? There's no Fixes tag on
> that one.
> 
> 

It's the problem I noted to Lorenzo the other day:

   
https://lore.kernel.org/linux-nfs/[e-mail]/T/#u

Once you've dropped the nfsd_mutex, there is no guarantee that
nn->nfsd_serv will still be a valid pointer. Holding the mutex across
the operation (like Neil's patch does), should close the race.

Chuck Lever Dec. 15, 2023, 2:38 p.m. UTC | #4

> On Dec 15, 2023, at 9:26 AM, Jeff Layton <[e-mail]> wrote:
> 
> On Fri, 2023-12-15 at 14:19 +0000, Chuck Lever III wrote:
>>> On Dec 15, 2023, at 5:59 AM, Jeff Layton <[e-mail]> wrote:
>>> 
>>> On Fri, 2023-12-15 at 11:56 +1100, NeilBrown wrote:
>>>> I sent an earlier version of this series, got some feed back, revised
>>>> it, but never sent it again.  Sorry.
>>>> 
>>>> The main feedback was around the interaction between sunrpc and nfsd for
>>>> handling poolstats.  I have changed that so that nfsd tells sunrpc where
>>>> the svc_serv pointer lives, and where to find a mutex to protect it.
>>>> sunrpc then taks the mutex and accesses the pointer - if not NULL.  I
>>>> think this is nicer than the version that pass around funciton pointers.
>>>> 
>>>> This series is against nfsd-next
>>>> 
>>>> Thanks,
>>>> NeilBrown
>>>> 
>>>> 
>>>> [PATCH 1/5] nfsd: call nfsd_last_thread() before final nfsd_put()
>>>> [PATCH 2/5] svc: don't hold reference for poolstats, only mutex.
>>>> [PATCH 3/5] nfsd: hold nfsd_mutex across entire netlink operation
>>>> [PATCH 4/5] SUNRPC: discard sv_refcnt, and svc_get/svc_put
>>>> [PATCH 5/5] nfsd: rename nfsd_last_thread() to nfsd_destroy_serv()
>>> 
>>> I'm not sure patch #2 is better than the version with function pointers,
>>> but it seems reasonable.
>>> 
>>> Note that patch #1 probably needs to go to v6.6 stable, and I think we
>>> want #3 in v6.7 before it ships.
>> 
>> Remind me why #3 should go into v6.7-rc ? There's no Fixes tag on
>> that one.
>> 
>> 
> 
> It's the problem I noted to Lorenzo the other day:
> 
> 
> https://lore.kernel.org/linux-nfs/[e-mail]/T/#u
> 
> Once you've dropped the nfsd_mutex, there is no guarantee that
> nn->nfsd_serv will still be a valid pointer. Holding the mutex across
> the operation (like Neil's patch does), should close the race.

OK. I'll add:

  Fixes: bd9d6a3efa97 ("NFSD: add rpc_status netlink support")

I will apply 1/3 and 3/3 to v6.7-rc, and the others will go to
v6.8 (nfsd-next) once it is rebased on v6.7-rc7.

--
Chuck Lever

Chuck Lever Dec. 15, 2023, 3:37 p.m. UTC | #5

> On Dec 15, 2023, at 9:38 AM, Chuck Lever III <[e-mail]> wrote:
> 
>> On Dec 15, 2023, at 9:26 AM, Jeff Layton <[e-mail]> wrote:
>> 
>> On Fri, 2023-12-15 at 14:19 +0000, Chuck Lever III wrote:
>>>> On Dec 15, 2023, at 5:59 AM, Jeff Layton <[e-mail]> wrote:
>>>> 
>>>> On Fri, 2023-12-15 at 11:56 +1100, NeilBrown wrote:
>>>>> I sent an earlier version of this series, got some feed back, revised
>>>>> it, but never sent it again.  Sorry.
>>>>> 
>>>>> The main feedback was around the interaction between sunrpc and nfsd for
>>>>> handling poolstats.  I have changed that so that nfsd tells sunrpc where
>>>>> the svc_serv pointer lives, and where to find a mutex to protect it.
>>>>> sunrpc then taks the mutex and accesses the pointer - if not NULL.  I
>>>>> think this is nicer than the version that pass around funciton pointers.
>>>>> 
>>>>> This series is against nfsd-next
>>>>> 
>>>>> Thanks,
>>>>> NeilBrown
>>>>> 
>>>>> 
>>>>> [PATCH 1/5] nfsd: call nfsd_last_thread() before final nfsd_put()
>>>>> [PATCH 2/5] svc: don't hold reference for poolstats, only mutex.
>>>>> [PATCH 3/5] nfsd: hold nfsd_mutex across entire netlink operation
>>>>> [PATCH 4/5] SUNRPC: discard sv_refcnt, and svc_get/svc_put
>>>>> [PATCH 5/5] nfsd: rename nfsd_last_thread() to nfsd_destroy_serv()
>>>> 
>>>> I'm not sure patch #2 is better than the version with function pointers,
>>>> but it seems reasonable.
>>>> 
>>>> Note that patch #1 probably needs to go to v6.6 stable, and I think we
>>>> want #3 in v6.7 before it ships.
>>> 
>>> Remind me why #3 should go into v6.7-rc ? There's no Fixes tag on
>>> that one.
>>> 
>>> 
>> 
>> It's the problem I noted to Lorenzo the other day:
>> 
>> 
>> https://lore.kernel.org/linux-nfs/[e-mail]/T/#u
>> 
>> Once you've dropped the nfsd_mutex, there is no guarantee that
>> nn->nfsd_serv will still be a valid pointer. Holding the mutex across
>> the operation (like Neil's patch does), should close the race.
> 
> OK. I'll add:
> 
>  Fixes: bd9d6a3efa97 ("NFSD: add rpc_status netlink support")
> 
> I will apply 1/3 and 3/3 to v6.7-rc, and the others will go to
> v6.8 (nfsd-next) once it is rebased on v6.7-rc7.

Please check the two patches at the tip of the nfsd-fixes
branch here:

https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git

I plan to apply the other three in this series to nfsd-next.


--
Chuck Lever

Jeff Layton Dec. 15, 2023, 3:44 p.m. UTC | #6

On Fri, 2023-12-15 at 15:37 +0000, Chuck Lever III wrote:
> > On Dec 15, 2023, at 9:38 AM, Chuck Lever III <[e-mail]> wrote:
> > 
> > > On Dec 15, 2023, at 9:26 AM, Jeff Layton <[e-mail]> wrote:
> > > 
> > > On Fri, 2023-12-15 at 14:19 +0000, Chuck Lever III wrote:
> > > > > On Dec 15, 2023, at 5:59 AM, Jeff Layton <[e-mail]> wrote:
> > > > > 
> > > > > On Fri, 2023-12-15 at 11:56 +1100, NeilBrown wrote:
> > > > > > I sent an earlier version of this series, got some feed back, revised
> > > > > > it, but never sent it again.  Sorry.
> > > > > > 
> > > > > > The main feedback was around the interaction between sunrpc and nfsd for
> > > > > > handling poolstats.  I have changed that so that nfsd tells sunrpc where
> > > > > > the svc_serv pointer lives, and where to find a mutex to protect it.
> > > > > > sunrpc then taks the mutex and accesses the pointer - if not NULL.  I
> > > > > > think this is nicer than the version that pass around funciton pointers.
> > > > > > 
> > > > > > This series is against nfsd-next
> > > > > > 
> > > > > > Thanks,
> > > > > > NeilBrown
> > > > > > 
> > > > > > 
> > > > > > [PATCH 1/5] nfsd: call nfsd_last_thread() before final nfsd_put()
> > > > > > [PATCH 2/5] svc: don't hold reference for poolstats, only mutex.
> > > > > > [PATCH 3/5] nfsd: hold nfsd_mutex across entire netlink operation
> > > > > > [PATCH 4/5] SUNRPC: discard sv_refcnt, and svc_get/svc_put
> > > > > > [PATCH 5/5] nfsd: rename nfsd_last_thread() to nfsd_destroy_serv()
> > > > > 
> > > > > I'm not sure patch #2 is better than the version with function pointers,
> > > > > but it seems reasonable.
> > > > > 
> > > > > Note that patch #1 probably needs to go to v6.6 stable, and I think we
> > > > > want #3 in v6.7 before it ships.
> > > > 
> > > > Remind me why #3 should go into v6.7-rc ? There's no Fixes tag on
> > > > that one.
> > > > 
> > > > 
> > > 
> > > It's the problem I noted to Lorenzo the other day:
> > > 
> > > 
> > > https://lore.kernel.org/linux-nfs/[e-mail]/T/#u
> > > 
> > > Once you've dropped the nfsd_mutex, there is no guarantee that
> > > nn->nfsd_serv will still be a valid pointer. Holding the mutex across
> > > the operation (like Neil's patch does), should close the race.
> > 
> > OK. I'll add:
> > 
> >  Fixes: bd9d6a3efa97 ("NFSD: add rpc_status netlink support")
> > 
> > I will apply 1/3 and 3/3 to v6.7-rc, and the others will go to
> > v6.8 (nfsd-next) once it is rebased on v6.7-rc7.
> 
> Please check the two patches at the tip of the nfsd-fixes
> branch here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
> 
> I plan to apply the other three in this series to nfsd-next.
> 
> 

They look good to me.

Thanks,