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

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

[NeilBrown]

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

[PATCH 1/5] nfsd: call nfsd_last_thread() before final nfsd_put()

[NeilBrown]

If write_ports_addfd or write_ports_addxprt fail, they call nfsd_put()
without calling nfsd_last_thread().  This leaves nn->nfsd_serv pointing
to a structure that has been freed.

So remove 'static' from nfsd_last_thread() and call it when the
nfsd_serv is about to be destroyed.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfsctl.c | 9 +++++++--
 fs/nfsd/nfsd.h   | 1 +
 fs/nfsd/nfssvc.c | 2 +-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index d6eeee149370..8c84e77a8892 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -709,8 +709,10 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
 
 	err = svc_addsock(nn->nfsd_serv, net, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
 
-	if (err >= 0 &&
-	    !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
+	if (err < 0 && !nn->nfsd_serv->sv_nrthreads && !nn->keep_active)
+		nfsd_last_thread(net);
+	else if (err >= 0 &&
+		 !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
 		svc_get(nn->nfsd_serv);
 
 	nfsd_put(net);
@@ -761,6 +763,9 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
 		svc_xprt_put(xprt);
 	}
 out_err:
+	if (!nn->nfsd_serv->sv_nrthreads && !nn->keep_active)
+		nfsd_last_thread(net);
+
 	nfsd_put(net);
 	return err;
 }
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index f5ff42f41ee7..3286ffacbc56 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -155,6 +155,7 @@ int nfsd_vers(struct nfsd_net *nn, int vers, enum vers_op change);
 int nfsd_minorversion(struct nfsd_net *nn, u32 minorversion, enum vers_op change);
 void nfsd_reset_versions(struct nfsd_net *nn);
 int nfsd_create_serv(struct net *net);
+void nfsd_last_thread(struct net *net);
 
 extern int nfsd_max_blksize;
 
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 66ca50b38b27..32d06249b3c0 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -541,7 +541,7 @@ static struct notifier_block nfsd_inet6addr_notifier = {
 /* Only used under nfsd_mutex, so this atomic may be overkill: */
 static atomic_t nfsd_notifier_refcount = ATOMIC_INIT(0);
 
-static void nfsd_last_thread(struct net *net)
+void nfsd_last_thread(struct net *net)
 {
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 	struct svc_serv *serv = nn->nfsd_serv;
-- 
2.43.0

[PATCH 2/5] svc: don't hold reference for poolstats, only mutex.

[NeilBrown]

A future patch will remove refcounting on svc_serv as it is of little
use.
It is currently used to keep the svc around while the pool_stats file is
open.
Change this to get the pointer, protected by the mutex, only in
seq_start, and the release the mutex in seq_stop.
This means that if the nfsd server is stopped and restarted while the
pool_stats file it open, then some pool stats info could be from the
first instance and some from the second.  This might appear odd, but is
unlikely to be a problem in practice.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/netns.h            |  4 +++-
 fs/nfsd/nfsctl.c           |  2 +-
 fs/nfsd/nfssvc.c           | 24 ++----------------------
 include/linux/sunrpc/svc.h |  8 +++++++-
 net/sunrpc/svc_xprt.c      | 32 +++++++++++++++++++++++---------
 5 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index ab303a8b77d5..16dbef245dbb 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -123,7 +123,9 @@ struct nfsd_net {
 	u32 clientid_counter;
 	u32 clverifier_counter;
 
-	struct svc_serv *nfsd_serv;
+	struct svc_info nfsd_info;
+#define nfsd_serv nfsd_info.serv
+
 	/* When a listening socket is added to nfsd, keep_active is set
 	 * and this justifies a reference on nfsd_serv.  This stops
 	 * nfsd_serv from being freed.  When the number of threads is
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 8c84e77a8892..2923da1537d2 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -183,7 +183,7 @@ static const struct file_operations pool_stats_operations = {
 	.open		= nfsd_pool_stats_open,
 	.read		= seq_read,
 	.llseek		= seq_lseek,
-	.release	= nfsd_pool_stats_release,
+	.release	= seq_release,
 };
 
 DEFINE_SHOW_ATTRIBUTE(nfsd_reply_cache_stats);
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 32d06249b3c0..6927edf932e9 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -684,6 +684,7 @@ int nfsd_create_serv(struct net *net)
 		return error;
 	}
 	spin_lock(&nfsd_notifier_lock);
+	nn->nfsd_info.mutex = &nfsd_mutex;
 	nn->nfsd_serv = serv;
 	spin_unlock(&nfsd_notifier_lock);
 
@@ -1083,28 +1084,7 @@ bool nfssvc_encode_voidres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
 
 int nfsd_pool_stats_open(struct inode *inode, struct file *file)
 {
-	int ret;
 	struct nfsd_net *nn = net_generic(inode->i_sb->s_fs_info, nfsd_net_id);
 
-	mutex_lock(&nfsd_mutex);
-	if (nn->nfsd_serv == NULL) {
-		mutex_unlock(&nfsd_mutex);
-		return -ENODEV;
-	}
-	svc_get(nn->nfsd_serv);
-	ret = svc_pool_stats_open(nn->nfsd_serv, file);
-	mutex_unlock(&nfsd_mutex);
-	return ret;
-}
-
-int nfsd_pool_stats_release(struct inode *inode, struct file *file)
-{
-	struct seq_file *seq = file->private_data;
-	struct svc_serv *serv = seq->private;
-	int ret = seq_release(inode, file);
-
-	mutex_lock(&nfsd_mutex);
-	svc_put(serv);
-	mutex_unlock(&nfsd_mutex);
-	return ret;
+	return svc_pool_stats_open(&nn->nfsd_info, file);
 }
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 544fcfe07479..3bea2840272d 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -97,6 +97,12 @@ struct svc_serv {
 #endif /* CONFIG_SUNRPC_BACKCHANNEL */
 };
 
+/* This is used by pool_stats to find and lock an svc */
+struct svc_info {
+	struct svc_serv		*serv;
+	struct mutex		*mutex;
+};
+
 /**
  * svc_get() - increment reference count on a SUNRPC serv
  * @serv:  the svc_serv to have count incremented
@@ -431,7 +437,7 @@ void		   svc_exit_thread(struct svc_rqst *);
 struct svc_serv *  svc_create_pooled(struct svc_program *, unsigned int,
 				     int (*threadfn)(void *data));
 int		   svc_set_num_threads(struct svc_serv *, struct svc_pool *, int);
-int		   svc_pool_stats_open(struct svc_serv *serv, struct file *file);
+int		   svc_pool_stats_open(struct svc_info *si, struct file *file);
 void		   svc_process(struct svc_rqst *rqstp);
 void		   svc_process_bc(struct rpc_rqst *req, struct svc_rqst *rqstp);
 int		   svc_register(const struct svc_serv *, struct net *, const int,
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index fee83d1024bc..dbb190606eec 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -1363,29 +1363,36 @@ int svc_xprt_names(struct svc_serv *serv, char *buf, const int buflen)
 }
 EXPORT_SYMBOL_GPL(svc_xprt_names);
 
-
 /*----------------------------------------------------------------------------*/
 
 static void *svc_pool_stats_start(struct seq_file *m, loff_t *pos)
 {
 	unsigned int pidx = (unsigned int)*pos;
-	struct svc_serv *serv = m->private;
+	struct svc_info *si = m->private;
 
 	dprintk("svc_pool_stats_start, *pidx=%u\n", pidx);
 
+	mutex_lock(si->mutex);
+
 	if (!pidx)
 		return SEQ_START_TOKEN;
-	return (pidx > serv->sv_nrpools ? NULL : &serv->sv_pools[pidx-1]);
+	if (!si->serv)
+		return NULL;
+	return pidx > si->serv->sv_nrpools ? NULL
+		: &si->serv->sv_pools[pidx - 1];
 }
 
 static void *svc_pool_stats_next(struct seq_file *m, void *p, loff_t *pos)
 {
 	struct svc_pool *pool = p;
-	struct svc_serv *serv = m->private;
+	struct svc_info *si = m->private;
+	struct svc_serv *serv = si->serv;
 
 	dprintk("svc_pool_stats_next, *pos=%llu\n", *pos);
 
-	if (p == SEQ_START_TOKEN) {
+	if (!serv) {
+		pool = NULL;
+	} else if (p == SEQ_START_TOKEN) {
 		pool = &serv->sv_pools[0];
 	} else {
 		unsigned int pidx = (pool - &serv->sv_pools[0]);
@@ -1400,6 +1407,9 @@ static void *svc_pool_stats_next(struct seq_file *m, void *p, loff_t *pos)
 
 static void svc_pool_stats_stop(struct seq_file *m, void *p)
 {
+	struct svc_info *si = m->private;
+
+	mutex_unlock(si->mutex);
 }
 
 static int svc_pool_stats_show(struct seq_file *m, void *p)
@@ -1427,14 +1437,18 @@ static const struct seq_operations svc_pool_stats_seq_ops = {
 	.show	= svc_pool_stats_show,
 };
 
-int svc_pool_stats_open(struct svc_serv *serv, struct file *file)
+int svc_pool_stats_open(struct svc_info *info, struct file *file)
 {
+	struct seq_file *seq;
 	int err;
 
 	err = seq_open(file, &svc_pool_stats_seq_ops);
-	if (!err)
-		((struct seq_file *) file->private_data)->private = serv;
-	return err;
+	if (err)
+		return err;
+	seq = file->private_data;
+	seq->private = info;
+
+	return 0;
 }
 EXPORT_SYMBOL(svc_pool_stats_open);
 
-- 
2.43.0

[PATCH 3/5] nfsd: hold nfsd_mutex across entire netlink operation

[NeilBrown]

Rather than using svc_get() and svc_put() to hold a stable reference to
the nfsd_svc for netlink lookups, simply hold the mutex for the entire
time.

The "entire" time isn't very long, and the mutex is not often contented.

This makes way for us to remove the refcounts of svc, which is more
confusing than useful.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfsctl.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 2923da1537d2..3368eb5342dc 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1521,11 +1521,10 @@ int nfsd_nl_rpc_status_get_start(struct netlink_callback *cb)
 	int ret = -ENODEV;
 
 	mutex_lock(&nfsd_mutex);
-	if (nn->nfsd_serv) {
-		svc_get(nn->nfsd_serv);
+	if (nn->nfsd_serv)
 		ret = 0;
-	}
-	mutex_unlock(&nfsd_mutex);
+	else
+		mutex_unlock(&nfsd_mutex);
 
 	return ret;
 }
@@ -1697,8 +1696,6 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
  */
 int nfsd_nl_rpc_status_get_done(struct netlink_callback *cb)
 {
-	mutex_lock(&nfsd_mutex);
-	nfsd_put(sock_net(cb->skb->sk));
 	mutex_unlock(&nfsd_mutex);
 
 	return 0;
-- 
2.43.0

[PATCH 4/5] SUNRPC: discard sv_refcnt, and svc_get/svc_put

[NeilBrown]

sv_refcnt is no longer useful.
lockd and nfs-cb only ever have the svc active when there are a non-zero
number of threads, so sv_refcnt mirrors sv_nrthreads.

nfsd also keeps the svc active between when a socket is added and when
the first thread is started, but we don't really need a refcount for
that.  We can simply not destroy the svc while there are any permanent
sockets attached.

So remove sv_refcnt and the get/put functions.
Instead of a final call to svc_put(), call svc_destroy() instead.
This is changed to also store NULL in the passed-in pointer to make it
easier to avoid use-after-free situations.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/lockd/svc.c             | 10 ++++------
 fs/nfs/callback.c          | 13 ++++++-------
 fs/nfsd/netns.h            |  7 -------
 fs/nfsd/nfsctl.c           | 15 ++++-----------
 fs/nfsd/nfsd.h             |  7 -------
 fs/nfsd/nfssvc.c           | 26 ++++----------------------
 include/linux/sunrpc/svc.h | 27 +--------------------------
 net/sunrpc/svc.c           | 13 ++++---------
 8 files changed, 23 insertions(+), 95 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 81be07c1d3d1..0d6cb3fdc0e1 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -345,10 +345,10 @@ static int lockd_get(void)
 
 	serv->sv_maxconn = nlm_max_connections;
 	error = svc_set_num_threads(serv, NULL, 1);
-	/* The thread now holds the only reference */
-	svc_put(serv);
-	if (error < 0)
+	if (error < 0) {
+		svc_destroy(&serv);
 		return error;
+	}
 
 	nlmsvc_serv = serv;
 	register_inetaddr_notifier(&lockd_inetaddr_notifier);
@@ -372,11 +372,9 @@ static void lockd_put(void)
 	unregister_inet6addr_notifier(&lockd_inet6addr_notifier);
 #endif
 
-	svc_get(nlmsvc_serv);
 	svc_set_num_threads(nlmsvc_serv, NULL, 0);
-	svc_put(nlmsvc_serv);
 	timer_delete_sync(&nlmsvc_retry);
-	nlmsvc_serv = NULL;
+	svc_destroy(&nlmsvc_serv);
 	dprintk("lockd_down: service destroyed\n");
 }
 
diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 4ffa1f469e90..760d27dd7225 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -187,7 +187,7 @@ static struct svc_serv *nfs_callback_create_svc(int minorversion)
 	 * Check whether we're already up and running.
 	 */
 	if (cb_info->serv)
-		return svc_get(cb_info->serv);
+		return cb_info->serv;
 
 	/*
 	 * Sanity check: if there's no task,
@@ -245,9 +245,10 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
 
 	cb_info->users++;
 err_net:
-	if (!cb_info->users)
-		cb_info->serv = NULL;
-	svc_put(serv);
+	if (!cb_info->users) {
+		svc_set_num_threads(cb_info->serv, NULL, 0);
+		svc_destroy(&cb_info->serv);
+	}
 err_create:
 	mutex_unlock(&nfs_callback_mutex);
 	return ret;
@@ -271,11 +272,9 @@ void nfs_callback_down(int minorversion, struct net *net)
 	nfs_callback_down_net(minorversion, serv, net);
 	cb_info->users--;
 	if (cb_info->users == 0) {
-		svc_get(serv);
 		svc_set_num_threads(serv, NULL, 0);
-		svc_put(serv);
 		dprintk("nfs_callback_down: service destroyed\n");
-		cb_info->serv = NULL;
+		svc_destroy(&cb_info->serv);
 	}
 	mutex_unlock(&nfs_callback_mutex);
 }
diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index 16dbef245dbb..74b4360779a1 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -126,13 +126,6 @@ struct nfsd_net {
 	struct svc_info nfsd_info;
 #define nfsd_serv nfsd_info.serv
 
-	/* When a listening socket is added to nfsd, keep_active is set
-	 * and this justifies a reference on nfsd_serv.  This stops
-	 * nfsd_serv from being freed.  When the number of threads is
-	 * set, keep_active is cleared and the reference is dropped.  So
-	 * when the last thread exits, the service will be destroyed.
-	 */
-	int keep_active;
 
 	/*
 	 * clientid and stateid data for construction of net unique COPY
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 3368eb5342dc..d0089cc5dc4c 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -709,13 +709,10 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
 
 	err = svc_addsock(nn->nfsd_serv, net, fd, buf, SIMPLE_TRANSACTION_LIMIT, cred);
 
-	if (err < 0 && !nn->nfsd_serv->sv_nrthreads && !nn->keep_active)
+	if (!nn->nfsd_serv->sv_nrthreads &&
+	    list_empty(&nn->nfsd_serv->sv_permsocks))
 		nfsd_last_thread(net);
-	else if (err >= 0 &&
-		 !nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
-		svc_get(nn->nfsd_serv);
 
-	nfsd_put(net);
 	return err;
 }
 
@@ -751,10 +748,6 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
 	if (err < 0 && err != -EAFNOSUPPORT)
 		goto out_close;
 
-	if (!nn->nfsd_serv->sv_nrthreads && !xchg(&nn->keep_active, 1))
-		svc_get(nn->nfsd_serv);
-
-	nfsd_put(net);
 	return 0;
 out_close:
 	xprt = svc_find_xprt(nn->nfsd_serv, transport, net, PF_INET, port);
@@ -763,10 +756,10 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
 		svc_xprt_put(xprt);
 	}
 out_err:
-	if (!nn->nfsd_serv->sv_nrthreads && !nn->keep_active)
+	if (!nn->nfsd_serv->sv_nrthreads &&
+	    list_empty(&nn->nfsd_serv->sv_permsocks))
 		nfsd_last_thread(net);
 
-	nfsd_put(net);
 	return err;
 }
 
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 3286ffacbc56..9ed0e08d16c2 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -113,13 +113,6 @@ int		nfsd_pool_stats_open(struct inode *, struct file *);
 int		nfsd_pool_stats_release(struct inode *, struct file *);
 void		nfsd_shutdown_threads(struct net *net);
 
-static inline void nfsd_put(struct net *net)
-{
-	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
-
-	svc_put(nn->nfsd_serv);
-}
-
 bool		i_am_nfsd(void);
 
 struct nfsdfs_client {
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 6927edf932e9..d670adfbc15b 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -59,15 +59,6 @@ static __be32			nfsd_init_request(struct svc_rqst *,
  * nfsd_mutex protects nn->nfsd_serv -- both the pointer itself and some members
  * of the svc_serv struct such as ->sv_temp_socks and ->sv_permsocks.
  *
- * If (out side the lock) nn->nfsd_serv is non-NULL, then it must point to a
- * properly initialised 'struct svc_serv' with ->sv_nrthreads > 0 (unless
- * nn->keep_active is set).  That number of nfsd threads must
- * exist and each must be listed in ->sp_all_threads in some entry of
- * ->sv_pools[].
- *
- * Each active thread holds a counted reference on nn->nfsd_serv, as does
- * the nn->keep_active flag and various transient calls to svc_get().
- *
  * Finally, the nfsd_mutex also protects some of the global variables that are
  * accessed when nfsd starts and that are settable via the write_* routines in
  * nfsctl.c. In particular:
@@ -572,6 +563,7 @@ void nfsd_last_thread(struct net *net)
 
 	nfsd_shutdown_net(net);
 	nfsd_export_flush(net);
+	svc_destroy(&serv);
 }
 
 void nfsd_reset_versions(struct nfsd_net *nn)
@@ -646,11 +638,9 @@ void nfsd_shutdown_threads(struct net *net)
 		return;
 	}
 
-	svc_get(serv);
 	/* Kill outstanding nfsd threads */
 	svc_set_num_threads(serv, NULL, 0);
 	nfsd_last_thread(net);
-	svc_put(serv);
 	mutex_unlock(&nfsd_mutex);
 }
 
@@ -666,10 +656,9 @@ int nfsd_create_serv(struct net *net)
 	struct svc_serv *serv;
 
 	WARN_ON(!mutex_is_locked(&nfsd_mutex));
-	if (nn->nfsd_serv) {
-		svc_get(nn->nfsd_serv);
+	if (nn->nfsd_serv)
 		return 0;
-	}
+
 	if (nfsd_max_blksize == 0)
 		nfsd_max_blksize = nfsd_get_default_max_blksize();
 	nfsd_reset_versions(nn);
@@ -680,7 +669,7 @@ int nfsd_create_serv(struct net *net)
 	serv->sv_maxconn = nn->max_connections;
 	error = svc_bind(serv, net);
 	if (error < 0) {
-		svc_put(serv);
+		svc_destroy(&serv);
 		return error;
 	}
 	spin_lock(&nfsd_notifier_lock);
@@ -764,7 +753,6 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
 		nthreads[0] = 1;
 
 	/* apply the new numbers */
-	svc_get(nn->nfsd_serv);
 	for (i = 0; i < n; i++) {
 		err = svc_set_num_threads(nn->nfsd_serv,
 					  &nn->nfsd_serv->sv_pools[i],
@@ -772,7 +760,6 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
 		if (err)
 			break;
 	}
-	svc_put(nn->nfsd_serv);
 	return err;
 }
 
@@ -814,13 +801,8 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred)
 		goto out_put;
 	error = serv->sv_nrthreads;
 out_put:
-	/* Threads now hold service active */
-	if (xchg(&nn->keep_active, 0))
-		svc_put(serv);
-
 	if (serv->sv_nrthreads == 0)
 		nfsd_last_thread(net);
-	svc_put(serv);
 out:
 	mutex_unlock(&nfsd_mutex);
 	return error;
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 3bea2840272d..8d7888234e9e 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -69,7 +69,6 @@ struct svc_serv {
 	struct svc_program *	sv_program;	/* RPC program */
 	struct svc_stat *	sv_stats;	/* RPC statistics */
 	spinlock_t		sv_lock;
-	struct kref		sv_refcnt;
 	unsigned int		sv_nrthreads;	/* # of server threads */
 	unsigned int		sv_maxconn;	/* max connections allowed or
 						 * '0' causing max to be based
@@ -103,31 +102,7 @@ struct svc_info {
 	struct mutex		*mutex;
 };
 
-/**
- * svc_get() - increment reference count on a SUNRPC serv
- * @serv:  the svc_serv to have count incremented
- *
- * Returns: the svc_serv that was passed in.
- */
-static inline struct svc_serv *svc_get(struct svc_serv *serv)
-{
-	kref_get(&serv->sv_refcnt);
-	return serv;
-}
-
-void svc_destroy(struct kref *);
-
-/**
- * svc_put - decrement reference count on a SUNRPC serv
- * @serv:  the svc_serv to have count decremented
- *
- * When the reference count reaches zero, svc_destroy()
- * is called to clean up and free the serv.
- */
-static inline void svc_put(struct svc_serv *serv)
-{
-	kref_put(&serv->sv_refcnt, svc_destroy);
-}
+void svc_destroy(struct svc_serv **svcp);
 
 /*
  * Maximum payload size supported by a kernel RPC server.
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index fa4e23fa0e09..eb5856e1351d 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -463,7 +463,6 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
 		return NULL;
 	serv->sv_name      = prog->pg_name;
 	serv->sv_program   = prog;
-	kref_init(&serv->sv_refcnt);
 	serv->sv_stats     = prog->pg_stats;
 	if (bufsize > RPCSVC_MAXPAYLOAD)
 		bufsize = RPCSVC_MAXPAYLOAD;
@@ -564,11 +563,13 @@ EXPORT_SYMBOL_GPL(svc_create_pooled);
  * protect sv_permsocks and sv_tempsocks.
  */
 void
-svc_destroy(struct kref *ref)
+svc_destroy(struct svc_serv **servp)
 {
-	struct svc_serv *serv = container_of(ref, struct svc_serv, sv_refcnt);
+	struct svc_serv *serv = *servp;
 	unsigned int i;
 
+	*servp = NULL;
+
 	dprintk("svc: svc_destroy(%s)\n", serv->sv_program->pg_name);
 	timer_shutdown_sync(&serv->sv_temptimer);
 
@@ -675,7 +676,6 @@ svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node)
 	if (!rqstp)
 		return ERR_PTR(-ENOMEM);
 
-	svc_get(serv);
 	spin_lock_bh(&serv->sv_lock);
 	serv->sv_nrthreads += 1;
 	spin_unlock_bh(&serv->sv_lock);
@@ -935,11 +935,6 @@ svc_exit_thread(struct svc_rqst *rqstp)
 
 	svc_rqst_free(rqstp);
 
-	svc_put(serv);
-	/* That svc_put() cannot be the last, because the thread
-	 * waiting for SP_VICTIM_REMAINS to clear must hold
-	 * a reference. So it is still safe to access pool.
-	 */
 	clear_and_wake_up_bit(SP_VICTIM_REMAINS, &pool->sp_flags);
 }
 EXPORT_SYMBOL_GPL(svc_exit_thread);
-- 
2.43.0

[PATCH 5/5] nfsd: rename nfsd_last_thread() to nfsd_destroy_serv()

[NeilBrown]

As this function now destroys the svc_serv, this is a better name.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfsctl.c | 4 ++--
 fs/nfsd/nfsd.h   | 2 +-
 fs/nfsd/nfssvc.c | 8 ++++----
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index d0089cc5dc4c..cca1dd7b8c55 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -711,7 +711,7 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
 
 	if (!nn->nfsd_serv->sv_nrthreads &&
 	    list_empty(&nn->nfsd_serv->sv_permsocks))
-		nfsd_last_thread(net);
+		nfsd_destroy_serv(net);
 
 	return err;
 }
@@ -758,7 +758,7 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
 out_err:
 	if (!nn->nfsd_serv->sv_nrthreads &&
 	    list_empty(&nn->nfsd_serv->sv_permsocks))
-		nfsd_last_thread(net);
+		nfsd_destroy_serv(net);
 
 	return err;
 }
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 9ed0e08d16c2..304e9728b929 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -148,7 +148,7 @@ int nfsd_vers(struct nfsd_net *nn, int vers, enum vers_op change);
 int nfsd_minorversion(struct nfsd_net *nn, u32 minorversion, enum vers_op change);
 void nfsd_reset_versions(struct nfsd_net *nn);
 int nfsd_create_serv(struct net *net);
-void nfsd_last_thread(struct net *net);
+void nfsd_destroy_serv(struct net *net);
 
 extern int nfsd_max_blksize;
 
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index d670adfbc15b..0916744eda83 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -532,7 +532,7 @@ static struct notifier_block nfsd_inet6addr_notifier = {
 /* Only used under nfsd_mutex, so this atomic may be overkill: */
 static atomic_t nfsd_notifier_refcount = ATOMIC_INIT(0);
 
-void nfsd_last_thread(struct net *net)
+void nfsd_destroy_serv(struct net *net)
 {
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 	struct svc_serv *serv = nn->nfsd_serv;
@@ -554,7 +554,7 @@ void nfsd_last_thread(struct net *net)
 	/*
 	 * write_ports can create the server without actually starting
 	 * any threads--if we get shut down before any threads are
-	 * started, then nfsd_last_thread will be run before any of this
+	 * started, then nfsd_destroy_serv will be run before any of this
 	 * other initialization has been done except the rpcb information.
 	 */
 	svc_rpcb_cleanup(serv, net);
@@ -640,7 +640,7 @@ void nfsd_shutdown_threads(struct net *net)
 
 	/* Kill outstanding nfsd threads */
 	svc_set_num_threads(serv, NULL, 0);
-	nfsd_last_thread(net);
+	nfsd_destroy_serv(net);
 	mutex_unlock(&nfsd_mutex);
 }
 
@@ -802,7 +802,7 @@ nfsd_svc(int nrservs, struct net *net, const struct cred *cred)
 	error = serv->sv_nrthreads;
 out_put:
 	if (serv->sv_nrthreads == 0)
-		nfsd_last_thread(net);
+		nfsd_destroy_serv(net);
 out:
 	mutex_unlock(&nfsd_mutex);
 	return error;
-- 
2.43.0

Re: [PATCH 0/5 v2] sunrpc: stop refcounting svc_serv

[Jeff Layton]

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 <jlayton@kernel.org>

Re: [PATCH 0/5 v2] sunrpc: stop refcounting svc_serv

[Chuck Lever III]

> On Dec 15, 2023, at 5:59 AM, Jeff Layton <jlayton@kernel.org> 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 <jlayton@kernel.org>

--
Chuck Lever

Re: [PATCH 0/5 v2] sunrpc: stop refcounting svc_serv

[Jeff Layton]

On Fri, 2023-12-15 at 14:19 +0000, Chuck Lever III wrote:
> > On Dec 15, 2023, at 5:59 AM, Jeff Layton <jlayton@kernel.org> 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/5d9bbb599569ce29f16e4e0eef6b291eda0f375b.camel@kernel.org/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.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 0/5 v2] sunrpc: stop refcounting svc_serv

[Chuck Lever III]

> On Dec 15, 2023, at 9:26 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Fri, 2023-12-15 at 14:19 +0000, Chuck Lever III wrote:
>>> On Dec 15, 2023, at 5:59 AM, Jeff Layton <jlayton@kernel.org> 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/5d9bbb599569ce29f16e4e0eef6b291eda0f375b.camel@kernel.org/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

Re: [PATCH 0/5 v2] sunrpc: stop refcounting svc_serv

[Chuck Lever III]

> On Dec 15, 2023, at 9:38 AM, Chuck Lever III <chuck.lever@oracle.com> wrote:
> 
>> On Dec 15, 2023, at 9:26 AM, Jeff Layton <jlayton@kernel.org> wrote:
>> 
>> On Fri, 2023-12-15 at 14:19 +0000, Chuck Lever III wrote:
>>>> On Dec 15, 2023, at 5:59 AM, Jeff Layton <jlayton@kernel.org> 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/5d9bbb599569ce29f16e4e0eef6b291eda0f375b.camel@kernel.org/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

Re: [PATCH 0/5 v2] sunrpc: stop refcounting svc_serv

[Jeff Layton]

On Fri, 2023-12-15 at 15:37 +0000, Chuck Lever III wrote:
> > On Dec 15, 2023, at 9:38 AM, Chuck Lever III <chuck.lever@oracle.com> wrote:
> > 
> > > On Dec 15, 2023, at 9:26 AM, Jeff Layton <jlayton@kernel.org> wrote:
> > > 
> > > On Fri, 2023-12-15 at 14:19 +0000, Chuck Lever III wrote:
> > > > > On Dec 15, 2023, at 5:59 AM, Jeff Layton <jlayton@kernel.org> 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/5d9bbb599569ce29f16e4e0eef6b291eda0f375b.camel@kernel.org/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,
-- 
Jeff Layton <jlayton@kernel.org>