Re: [PATCH v7 17/20] nfsd: use SRCU to dereference nn->nfsd_serv

[Mike Snitzer <snitzer@kernel.org>]

On Wed, Jun 26, 2024 at 09:29:13AM +1000, NeilBrown wrote:
> On Tue, 25 Jun 2024, Jeff Layton wrote:
> > On Mon, 2024-06-24 at 12:27 -0400, Mike Snitzer wrote:
> > > Introduce nfsd_serv_get, nfsd_serv_put and nfsd_serv_sync and update
> > > the nfsd code to prevent nfsd_destroy_serv from destroying
> > > nn->nfsd_serv until all nfsd code is done with it (particularly the
> > > localio code that doesn't run in the context of nfsd's svc threads,
> > > nor does it take the nfsd_mutex).
> > > 
> > > Commit 83d5e5b0af90 ("dm: optimize use SRCU and RCU") provided a
> > > familiar well-worn pattern for how implement.
> > > 
> > > Suggested-by: NeilBrown <neilb@suse.de>
> > > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > > ---
> > >  fs/nfsd/filecache.c | 13 ++++++++---
> > >  fs/nfsd/netns.h     | 14 ++++++++++--
> > >  fs/nfsd/nfs4state.c | 25 ++++++++++++++-------
> > >  fs/nfsd/nfsctl.c    |  7 ++++--
> > >  fs/nfsd/nfssvc.c    | 54 ++++++++++++++++++++++++++++++++++++---------
> > >  5 files changed, 88 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > > index 99631fa56662..474b3a3af3fb 100644
> > > --- a/fs/nfsd/filecache.c
> > > +++ b/fs/nfsd/filecache.c
> > > @@ -413,12 +413,15 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose)
> > >  		struct nfsd_file *nf = list_first_entry(dispose,
> > >  						struct nfsd_file, nf_lru);
> > >  		struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
> > > +		int srcu_idx;
> > > +		struct svc_serv *serv = nfsd_serv_get(nn, &srcu_idx);
> > >  		struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> > >  
> > >  		spin_lock(&l->lock);
> > >  		list_move_tail(&nf->nf_lru, &l->freeme);
> > >  		spin_unlock(&l->lock);
> > > -		svc_wake_up(nn->nfsd_serv);
> > > +		svc_wake_up(serv);
> > > +		nfsd_serv_put(nn, srcu_idx);
> > >  	}
> > >  }
> > >  
> > > @@ -443,11 +446,15 @@ void nfsd_file_net_dispose(struct nfsd_net *nn)
> > >  		for (i = 0; i < 8 && !list_empty(&l->freeme); i++)
> > >  			list_move(l->freeme.next, &dispose);
> > >  		spin_unlock(&l->lock);
> > > -		if (!list_empty(&l->freeme))
> > > +		if (!list_empty(&l->freeme)) {
> > > +			int srcu_idx;
> > > +			struct svc_serv *serv = nfsd_serv_get(nn, &srcu_idx);
> > >  			/* Wake up another thread to share the work
> > >  			 * *before* doing any actual disposing.
> > >  			 */
> > > -			svc_wake_up(nn->nfsd_serv);
> > > +			svc_wake_up(serv);
> > > +			nfsd_serv_put(nn, srcu_idx);
> > > +		}
> > >  		nfsd_file_dispose_list(&dispose);
> > >  	}
> > >  }
> > > diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> > > index 0c5a1d97e4ac..92d0d0883f17 100644
> > > --- a/fs/nfsd/netns.h
> > > +++ b/fs/nfsd/netns.h
> > > @@ -139,8 +139,14 @@ struct nfsd_net {
> > >  	u32 clverifier_counter;
> > >  
> > >  	struct svc_info nfsd_info;
> > > -#define nfsd_serv nfsd_info.serv
> > > -
> > > +	/*
> > > +	 * The current 'nfsd_serv' at nfsd_info.serv.  Using 'void' rather than
> > > +	 * 'struct svc_serv' to guard against new code dereferencing nfsd_serv
> > > +	 * without using proper synchronization.
> > > +	 * Use nfsd_serv_get() or take nfsd_mutex to dereference.
> > > +	 */
> > > +	void __rcu *nfsd_serv;
> > > +	struct srcu_struct nfsd_serv_srcu;
> > 
> > I'm still not sold on the use of a void pointer here. It might protect
> > you from using nn->nfsd_serv directly, but if you do:
> > 
> >     struct svc_serv *serv = nn->nfsd_serv;
> > 
> > ...that will still work. If you really want to guard against direct
> > dereferencing, maybe it should be an unsigned long? Then you really
> > will have to cast to use it.
> 
> I agree.  The point of the "__rcu" attribute is that sparse will
> complain if you use the pointer without using srcu_dereference().
> And CONFIG_PROVE_RCU gives you run-time splats.
> We should use the tools we already have.

Good news, I've switched from using SRCU to a percpu-refcount.  So v8
will be considerably cleaner (less shotgun blast due to dropping the
excessive churn from SRCU prep and implementation).

SRCU would've enabled us to drop the client notifier spinlock, and
maybe more but certianly best to avoid all the SRCU changes.

Mike