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

[Jeff Layton <jlayton@kernel.org>]

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.

>  
>  	/*
>  	 * clientid and stateid data for construction of net unique COPY
> @@ -225,6 +231,10 @@ struct nfsd_net {
>  extern bool nfsd_support_version(int vers);
>  extern void nfsd_netns_free_versions(struct nfsd_net *nn);
>  
> +extern struct svc_serv *nfsd_serv_get(struct nfsd_net *nn, int *srcu_idx);
> +extern void nfsd_serv_put(struct nfsd_net *nn, int srcu_idx);
> +extern void nfsd_serv_sync(struct nfsd_net *nn);
> +
>  extern unsigned int nfsd_net_id;
>  
>  void nfsd_copy_write_verifier(__be32 verf[2], struct nfsd_net *nn);
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index a20c2c9d7d45..8876810e569d 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1919,6 +1919,8 @@ static u32 nfsd4_get_drc_mem(struct nfsd4_channel_attrs *ca, struct nfsd_net *nn
>  	u32 num = ca->maxreqs;
>  	unsigned long avail, total_avail;
>  	unsigned int scale_factor;
> +	int srcu_idx;
> +	struct svc_serv *serv = nfsd_serv_get(nn, &srcu_idx);
>  
>  	spin_lock(&nfsd_drc_lock);
>  	if (nfsd_drc_max_mem > nfsd_drc_mem_used)
> @@ -1940,7 +1942,7 @@ static u32 nfsd4_get_drc_mem(struct nfsd4_channel_attrs *ca, struct nfsd_net *nn
>  	 * Give the client one slot even if that would require
>  	 * over-allocation--it is better than failure.
>  	 */
> -	scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads);
> +	scale_factor = max_t(unsigned int, 8, serv->sv_nrthreads);
>  
>  	avail = clamp_t(unsigned long, avail, slotsize,
>  			total_avail/scale_factor);
> @@ -1949,6 +1951,8 @@ static u32 nfsd4_get_drc_mem(struct nfsd4_channel_attrs *ca, struct nfsd_net *nn
>  	nfsd_drc_mem_used += num * slotsize;
>  	spin_unlock(&nfsd_drc_lock);
>  
> +	nfsd_serv_put(nn, srcu_idx);
> +
>  	return num;
>  }
>  
> @@ -3702,12 +3706,16 @@ nfsd4_replay_create_session(struct nfsd4_create_session *cr_ses,
>  
>  static __be32 check_forechannel_attrs(struct nfsd4_channel_attrs *ca, struct nfsd_net *nn)
>  {
> -	u32 maxrpc = nn->nfsd_serv->sv_max_mesg;
> +	int srcu_idx;
> +	struct svc_serv *serv = nfsd_serv_get(nn, &srcu_idx);
> +	u32 maxrpc = serv->sv_max_mesg;
> +	__be32 status = nfs_ok;
>  
> -	if (ca->maxreq_sz < NFSD_MIN_REQ_HDR_SEQ_SZ)
> -		return nfserr_toosmall;
> -	if (ca->maxresp_sz < NFSD_MIN_RESP_HDR_SEQ_SZ)
> -		return nfserr_toosmall;
> +	if (ca->maxreq_sz < NFSD_MIN_REQ_HDR_SEQ_SZ ||
> +	    ca->maxresp_sz < NFSD_MIN_RESP_HDR_SEQ_SZ) {
> +		status = nfserr_toosmall;
> +		goto out;
> +	}
>  	ca->headerpadsz = 0;
>  	ca->maxreq_sz = min_t(u32, ca->maxreq_sz, maxrpc);
>  	ca->maxresp_sz = min_t(u32, ca->maxresp_sz, maxrpc);
> @@ -3726,8 +3734,9 @@ static __be32 check_forechannel_attrs(struct nfsd4_channel_attrs *ca, struct nfs
>  	 * accounting is soft and provides no guarantees either way.
>  	 */
>  	ca->maxreqs = nfsd4_get_drc_mem(ca, nn);
> -
> -	return nfs_ok;
> +out:
> +	nfsd_serv_put(nn, srcu_idx);
> +	return status;
>  }
>  
>  /*
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 075ada559e18..d3eeb829bc9b 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1569,10 +1569,12 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
>  {
>  	struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
>  	int i, ret, rqstp_index = 0;
> +	int srcu_idx;
> +	struct svc_serv *serv = nfsd_serv_get(nn, &srcu_idx);
>  
>  	rcu_read_lock();
>  
> -	for (i = 0; i < nn->nfsd_serv->sv_nrpools; i++) {
> +	for (i = 0; i < serv->sv_nrpools; i++) {
>  		struct svc_rqst *rqstp;
>  
>  		if (i < cb->args[0]) /* already consumed */
> @@ -1580,7 +1582,7 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
>  
>  		rqstp_index = 0;
>  		list_for_each_entry_rcu(rqstp,
> -				&nn->nfsd_serv->sv_pools[i].sp_all_threads,
> +				&serv->sv_pools[i].sp_all_threads,
>  				rq_all) {
>  			struct nfsd_genl_rqstp genl_rqstp;
>  			unsigned int status_counter;
> @@ -1645,6 +1647,7 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
>  	ret = skb->len;
>  out:
>  	rcu_read_unlock();
> +	nfsd_serv_put(nn, srcu_idx);
>  
>  	return ret;
>  }
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index c16c7d630859..6f41fb832484 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -279,6 +279,26 @@ int nfsd_minorversion(struct nfsd_net *nn, u32 minorversion, enum vers_op change
>  	return 0;
>  }
>  
> +struct svc_serv *nfsd_serv_get(struct nfsd_net *nn, int *srcu_idx)
> +	__acquires(nn->nfsd_serv_srcu)
> +{
> +	*srcu_idx = srcu_read_lock(&nn->nfsd_serv_srcu);
> +
> +	return srcu_dereference(nn->nfsd_serv, &nn->nfsd_serv_srcu);
> +}
> +
> +void nfsd_serv_put(struct nfsd_net *nn, int srcu_idx)
> +	__releases(nn->nfsd_serv_srcu)
> +{
> +	srcu_read_unlock(&nn->nfsd_serv_srcu, srcu_idx);
> +}
> +
> +void nfsd_serv_sync(struct nfsd_net *nn)
> +{
> +	synchronize_srcu(&nn->nfsd_serv_srcu);
> +	synchronize_rcu_expedited();
> +}
> +
>  /*
>   * Maximum number of nfsd processes
>   */
> @@ -486,6 +506,7 @@ static void nfsd_shutdown_net(struct net *net)
>  		lockd_down(net);
>  		nn->lockd_up = false;
>  	}
> +	cleanup_srcu_struct(&nn->nfsd_serv_srcu);
>  #if IS_ENABLED(CONFIG_NFSD_LOCALIO)
>  	list_del_rcu(&nn->nfsd_uuid.list);
>  #endif
> @@ -493,6 +514,7 @@ static void nfsd_shutdown_net(struct net *net)
>  	nfsd_shutdown_generic();
>  }
>  
> +// FIXME: nfsd_serv_{get,put} should make it safe to eliminate nfsd_notifier_lock
>  static DEFINE_SPINLOCK(nfsd_notifier_lock);
>  static int nfsd_inetaddr_event(struct notifier_block *this, unsigned long event,
>  	void *ptr)
> @@ -502,20 +524,22 @@ static int nfsd_inetaddr_event(struct notifier_block *this, unsigned long event,
>  	struct net *net = dev_net(dev);
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>  	struct sockaddr_in sin;
> +	int srcu_idx;
> +	struct svc_serv *serv = nfsd_serv_get(nn, &srcu_idx);
>  
> -	if (event != NETDEV_DOWN || !nn->nfsd_serv)
> +	if (event != NETDEV_DOWN || !serv)
>  		goto out;
>  
>  	spin_lock(&nfsd_notifier_lock);
> -	if (nn->nfsd_serv) {
> +	if (serv) {
>  		dprintk("nfsd_inetaddr_event: removed %pI4\n", &ifa->ifa_local);
>  		sin.sin_family = AF_INET;
>  		sin.sin_addr.s_addr = ifa->ifa_local;
> -		svc_age_temp_xprts_now(nn->nfsd_serv, (struct sockaddr *)&sin);
> +		svc_age_temp_xprts_now(serv, (struct sockaddr *)&sin);
>  	}
>  	spin_unlock(&nfsd_notifier_lock);
> -
>  out:
> +	nfsd_serv_put(nn, srcu_idx);
>  	return NOTIFY_DONE;
>  }
>  
> @@ -532,22 +556,24 @@ static int nfsd_inet6addr_event(struct notifier_block *this,
>  	struct net *net = dev_net(dev);
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>  	struct sockaddr_in6 sin6;
> +	int srcu_idx;
> +	struct svc_serv *serv = nfsd_serv_get(nn, &srcu_idx);
>  
> -	if (event != NETDEV_DOWN || !nn->nfsd_serv)
> +	if (event != NETDEV_DOWN || !serv)
>  		goto out;
>  
>  	spin_lock(&nfsd_notifier_lock);
> -	if (nn->nfsd_serv) {
> +	if (serv) {
>  		dprintk("nfsd_inet6addr_event: removed %pI6\n", &ifa->addr);
>  		sin6.sin6_family = AF_INET6;
>  		sin6.sin6_addr = ifa->addr;
>  		if (ipv6_addr_type(&sin6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
>  			sin6.sin6_scope_id = ifa->idev->dev->ifindex;
> -		svc_age_temp_xprts_now(nn->nfsd_serv, (struct sockaddr *)&sin6);
> +		svc_age_temp_xprts_now(serv, (struct sockaddr *)&sin6);
>  	}
>  	spin_unlock(&nfsd_notifier_lock);
> -
>  out:
> +	nfsd_serv_put(nn, srcu_idx);
>  	return NOTIFY_DONE;
>  }
>  
> @@ -568,9 +594,12 @@ void nfsd_destroy_serv(struct net *net)
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>  	struct svc_serv *serv = nn->nfsd_serv;
>  
> +	lockdep_assert_held(&nfsd_mutex);
> +
>  	spin_lock(&nfsd_notifier_lock);
> -	nn->nfsd_serv = NULL;
> +	rcu_assign_pointer(nn->nfsd_serv, NULL);
>  	spin_unlock(&nfsd_notifier_lock);
> +	nfsd_serv_sync(nn);
>  
>  	/* check if the notifier still has clients */
>  	if (atomic_dec_return(&nfsd_notifier_refcount) == 0) {
> @@ -690,6 +719,10 @@ int nfsd_create_serv(struct net *net)
>  	if (nn->nfsd_serv)
>  		return 0;
>  
> +	error = init_srcu_struct(&nn->nfsd_serv_srcu);
> +	if (error)
> +		return error;
> +
>  	if (nfsd_max_blksize == 0)
>  		nfsd_max_blksize = nfsd_get_default_max_blksize();
>  	nfsd_reset_versions(nn);
> @@ -707,7 +740,8 @@ int nfsd_create_serv(struct net *net)
>  	}
>  	spin_lock(&nfsd_notifier_lock);
>  	nn->nfsd_info.mutex = &nfsd_mutex;
> -	nn->nfsd_serv = serv;
> +	nn->nfsd_info.serv = serv;
> +	rcu_assign_pointer(nn->nfsd_serv, nn->nfsd_info.serv);
>  	spin_unlock(&nfsd_notifier_lock);
>  
>  	set_max_drc();

-- 
Jeff Layton <jlayton@kernel.org>