public inbox for linux-nfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] nfsd: provide proper locking for all write_ function
  2025-06-18 21:31 NeilBrown
@ 2025-06-18 21:31 ` NeilBrown
  2025-06-20 12:59   ` Jeff Layton
  0 siblings, 1 reply; 11+ messages in thread
From: NeilBrown @ 2025-06-18 21:31 UTC (permalink / raw)
  To: Chuck Lever, Jeff Layton
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Li Lingfeng

write_foo functions are called to handle IO to files in /proc/fs/nfsd/.
The can be called at any time and so generally need locking to ensure
they don't happen at an awkward time.

Many already take nfsd_mutex and check if nfsd_serv has been set.  This
ensures they only run when the server is fully configured.

write_filehandle() does *not* need locking.  It interacts with the
export table which is set up when the netns is set up, so it is always
valid and it has its own locking.  write_filehandle() is needed before
the nfs server is started so checking nfsd_serv would be wrong.

The remaining files which do not have any locking are
write_v4_end_grace(), write_unlock_ip(), and write_unlock_fs().
None of these make sense when the nfs server is not running and there is
evidence that write_v4_end_grace() can race with ->client_tracking_op
setup/shutdown and cause problems.

This patch adds locking to these three and ensures the "unlock"
functions abort if ->nfsd_serv is not set.

Reported-by: Li Lingfeng <lilingfeng3@huawei.com>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfsctl.c | 115 +++++++++++++++++++++++++++++++----------------
 1 file changed, 77 insertions(+), 38 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 3f3e9f6c4250..3710a1992d17 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -200,27 +200,18 @@ static inline struct net *netns(struct file *file)
 	return file_inode(file)->i_sb->s_fs_info;
 }
 
-/*
- * write_unlock_ip - Release all locks used by a client
- *
- * Experimental.
- *
- * Input:
- *			buf:	'\n'-terminated C string containing a
- *				presentation format IP address
- *			size:	length of C string in @buf
- * Output:
- *	On success:	returns zero if all specified locks were released;
- *			returns one if one or more locks were not released
- *	On error:	return code is negative errno value
- */
-static ssize_t write_unlock_ip(struct file *file, char *buf, size_t size)
+static ssize_t __write_unlock_ip(struct file *file, char *buf, size_t size)
 {
 	struct sockaddr_storage address;
 	struct sockaddr *sap = (struct sockaddr *)&address;
 	size_t salen = sizeof(address);
 	char *fo_path;
 	struct net *net = netns(file);
+	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
+
+	if (!nn->nfsd_serv)
+		/* There cannot be any files to unlock */
+		return -EINVAL;
 
 	/* sanity check */
 	if (size == 0)
@@ -241,24 +232,39 @@ static ssize_t write_unlock_ip(struct file *file, char *buf, size_t size)
 }
 
 /*
- * write_unlock_fs - Release all locks on a local file system
+ * write_unlock_ip - Release all locks used by a client
  *
  * Experimental.
  *
  * Input:
- *			buf:	'\n'-terminated C string containing the
- *				absolute pathname of a local file system
+ *			buf:	'\n'-terminated C string containing a
+ *				presentation format IP address
  *			size:	length of C string in @buf
  * Output:
  *	On success:	returns zero if all specified locks were released;
  *			returns one if one or more locks were not released
  *	On error:	return code is negative errno value
  */
-static ssize_t write_unlock_fs(struct file *file, char *buf, size_t size)
+static ssize_t write_unlock_ip(struct file *file, char *buf, size_t size)
+{
+	ssize_t rv;
+
+	mutex_lock(&nfsd_mutex);
+	rv = __write_unlock_ip(file, buf, size);
+	mutex_unlock(&nfsd_mutex);
+	return rv;
+}
+
+static ssize_t __write_unlock_fs(struct file *file, char *buf, size_t size)
 {
 	struct path path;
 	char *fo_path;
 	int error;
+	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
+
+	if (!nn->nfsd_serv)
+		/* There cannot be any files to unlock */
+		return -EINVAL;
 
 	/* sanity check */
 	if (size == 0)
@@ -291,6 +297,30 @@ static ssize_t write_unlock_fs(struct file *file, char *buf, size_t size)
 	return error;
 }
 
+/*
+ * write_unlock_fs - Release all locks on a local file system
+ *
+ * Experimental.
+ *
+ * Input:
+ *			buf:	'\n'-terminated C string containing the
+ *				absolute pathname of a local file system
+ *			size:	length of C string in @buf
+ * Output:
+ *	On success:	returns zero if all specified locks were released;
+ *			returns one if one or more locks were not released
+ *	On error:	return code is negative errno value
+ */
+static ssize_t write_unlock_fs(struct file *file, char *buf, size_t size)
+{
+	ssize_t rv;
+
+	mutex_lock(&nfsd_mutex);
+	rv = __write_unlock_fs(file, buf, size);
+	mutex_unlock(&nfsd_mutex);
+	return rv;
+}
+
 /*
  * write_filehandle - Get a variable-length NFS file handle by path
  *
@@ -1053,6 +1083,29 @@ static ssize_t write_recoverydir(struct file *file, char *buf, size_t size)
 }
 #endif
 
+static ssize_t __write_v4_end_grace(struct file *file, char *buf, size_t size)
+{
+	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
+
+	if (size > 0) {
+		switch(buf[0]) {
+		case 'Y':
+		case 'y':
+		case '1':
+			if (!nn->nfsd_serv)
+				return -EBUSY;
+			trace_nfsd_end_grace(netns(file));
+			nfsd4_end_grace(nn);
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%c\n",
+			 nn->grace_ended ? 'Y' : 'N');
+}
+
 /*
  * write_v4_end_grace - release grace period for nfsd's v4.x lock manager
  *
@@ -1075,27 +1128,13 @@ static ssize_t write_recoverydir(struct file *file, char *buf, size_t size)
  */
 static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
 {
-	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
-
-	if (size > 0) {
-		switch(buf[0]) {
-		case 'Y':
-		case 'y':
-		case '1':
-			if (!nn->nfsd_serv)
-				return -EBUSY;
-			trace_nfsd_end_grace(netns(file));
-			nfsd4_end_grace(nn);
-			break;
-		default:
-			return -EINVAL;
-		}
-	}
+	ssize_t rv;
 
-	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%c\n",
-			 nn->grace_ended ? 'Y' : 'N');
+	mutex_lock(&nfsd_mutex);
+	rv = __write_v4_end_grace(file, buf, size);
+	mutex_unlock(&nfsd_mutex);
+	return rv;
 }
-
 #endif
 
 /*----------------------------------------------------------------------------*/
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] nfsd: provide proper locking for all write_ function
  2025-06-18 21:31 ` [PATCH 1/3] nfsd: provide proper locking for all write_ function NeilBrown
@ 2025-06-20 12:59   ` Jeff Layton
  0 siblings, 0 replies; 11+ messages in thread
From: Jeff Layton @ 2025-06-20 12:59 UTC (permalink / raw)
  To: NeilBrown, Chuck Lever
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Li Lingfeng

On Thu, 2025-06-19 at 07:31 +1000, NeilBrown wrote:
> write_foo functions are called to handle IO to files in /proc/fs/nfsd/.
> The can be called at any time and so generally need locking to ensure
> they don't happen at an awkward time.
> 
> Many already take nfsd_mutex and check if nfsd_serv has been set.  This
> ensures they only run when the server is fully configured.
> 
> write_filehandle() does *not* need locking.  It interacts with the
> export table which is set up when the netns is set up, so it is always
> valid and it has its own locking.  write_filehandle() is needed before
> the nfs server is started so checking nfsd_serv would be wrong.
> 
> The remaining files which do not have any locking are
> write_v4_end_grace(), write_unlock_ip(), and write_unlock_fs().
> None of these make sense when the nfs server is not running and there is
> evidence that write_v4_end_grace() can race with ->client_tracking_op
> setup/shutdown and cause problems.
> 
> This patch adds locking to these three and ensures the "unlock"
> functions abort if ->nfsd_serv is not set.
> 
> Reported-by: Li Lingfeng <lilingfeng3@huawei.com>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfsctl.c | 115 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 77 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 3f3e9f6c4250..3710a1992d17 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -200,27 +200,18 @@ static inline struct net *netns(struct file *file)
>  	return file_inode(file)->i_sb->s_fs_info;
>  }
>  
> -/*
> - * write_unlock_ip - Release all locks used by a client
> - *
> - * Experimental.
> - *
> - * Input:
> - *			buf:	'\n'-terminated C string containing a
> - *				presentation format IP address
> - *			size:	length of C string in @buf
> - * Output:
> - *	On success:	returns zero if all specified locks were released;
> - *			returns one if one or more locks were not released
> - *	On error:	return code is negative errno value
> - */
> -static ssize_t write_unlock_ip(struct file *file, char *buf, size_t size)
> +static ssize_t __write_unlock_ip(struct file *file, char *buf, size_t size)
>  {
>  	struct sockaddr_storage address;
>  	struct sockaddr *sap = (struct sockaddr *)&address;
>  	size_t salen = sizeof(address);
>  	char *fo_path;
>  	struct net *net = netns(file);
> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +
> +	if (!nn->nfsd_serv)
> +		/* There cannot be any files to unlock */
> +		return -EINVAL;
>  
>  	/* sanity check */
>  	if (size == 0)
> @@ -241,24 +232,39 @@ static ssize_t write_unlock_ip(struct file *file, char *buf, size_t size)
>  }
>  
>  /*
> - * write_unlock_fs - Release all locks on a local file system
> + * write_unlock_ip - Release all locks used by a client
>   *
>   * Experimental.
>   *
>   * Input:
> - *			buf:	'\n'-terminated C string containing the
> - *				absolute pathname of a local file system
> + *			buf:	'\n'-terminated C string containing a
> + *				presentation format IP address
>   *			size:	length of C string in @buf
>   * Output:
>   *	On success:	returns zero if all specified locks were released;
>   *			returns one if one or more locks were not released
>   *	On error:	return code is negative errno value
>   */
> -static ssize_t write_unlock_fs(struct file *file, char *buf, size_t size)
> +static ssize_t write_unlock_ip(struct file *file, char *buf, size_t size)
> +{
> +	ssize_t rv;
> +
> +	mutex_lock(&nfsd_mutex);
> +	rv = __write_unlock_ip(file, buf, size);
> +	mutex_unlock(&nfsd_mutex);
> +	return rv;
> +}
> +
> +static ssize_t __write_unlock_fs(struct file *file, char *buf, size_t size)
>  {
>  	struct path path;
>  	char *fo_path;
>  	int error;
> +	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
> +
> +	if (!nn->nfsd_serv)
> +		/* There cannot be any files to unlock */
> +		return -EINVAL;
>  
>  	/* sanity check */
>  	if (size == 0)
> @@ -291,6 +297,30 @@ static ssize_t write_unlock_fs(struct file *file, char *buf, size_t size)
>  	return error;
>  }
>  
> +/*
> + * write_unlock_fs - Release all locks on a local file system
> + *
> + * Experimental.
> + *
> + * Input:
> + *			buf:	'\n'-terminated C string containing the
> + *				absolute pathname of a local file system
> + *			size:	length of C string in @buf
> + * Output:
> + *	On success:	returns zero if all specified locks were released;
> + *			returns one if one or more locks were not released
> + *	On error:	return code is negative errno value
> + */
> +static ssize_t write_unlock_fs(struct file *file, char *buf, size_t size)
> +{
> +	ssize_t rv;
> +
> +	mutex_lock(&nfsd_mutex);
> +	rv = __write_unlock_fs(file, buf, size);
> +	mutex_unlock(&nfsd_mutex);
> +	return rv;
> +}
> +
>  /*
>   * write_filehandle - Get a variable-length NFS file handle by path
>   *
> @@ -1053,6 +1083,29 @@ static ssize_t write_recoverydir(struct file *file, char *buf, size_t size)
>  }
>  #endif
>  
> +static ssize_t __write_v4_end_grace(struct file *file, char *buf, size_t size)
> +{
> +	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
> +
> +	if (size > 0) {
> +		switch(buf[0]) {
> +		case 'Y':
> +		case 'y':
> +		case '1':
> +			if (!nn->nfsd_serv)
> +				return -EBUSY;
> +			trace_nfsd_end_grace(netns(file));
> +			nfsd4_end_grace(nn);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%c\n",
> +			 nn->grace_ended ? 'Y' : 'N');
> +}
> +
>  /*
>   * write_v4_end_grace - release grace period for nfsd's v4.x lock manager
>   *
> @@ -1075,27 +1128,13 @@ static ssize_t write_recoverydir(struct file *file, char *buf, size_t size)
>   */
>  static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
>  {
> -	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
> -
> -	if (size > 0) {
> -		switch(buf[0]) {
> -		case 'Y':
> -		case 'y':
> -		case '1':
> -			if (!nn->nfsd_serv)
> -				return -EBUSY;
> -			trace_nfsd_end_grace(netns(file));
> -			nfsd4_end_grace(nn);
> -			break;
> -		default:
> -			return -EINVAL;
> -		}
> -	}
> +	ssize_t rv;
>  
> -	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%c\n",
> -			 nn->grace_ended ? 'Y' : 'N');
> +	mutex_lock(&nfsd_mutex);
> +	rv = __write_v4_end_grace(file, buf, size);
> +	mutex_unlock(&nfsd_mutex);
> +	return rv;
>  }
> -
>  #endif
>  
>  /*----------------------------------------------------------------------------*/

Reviewed-by: Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 0/3 RFC] improve some nfsd_mutex locking
@ 2025-06-20 23:33 NeilBrown
  2025-06-20 23:33 ` [PATCH 1/3] nfsd: provide proper locking for all write_ function NeilBrown
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: NeilBrown @ 2025-06-20 23:33 UTC (permalink / raw)
  To: Chuck Lever, Jeff Layton
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Li Lingfeng

The first patch hopefully fixes a bug with locking as reported by Li
Lingfeng: some write_foo functions aren't locked properly.

The other two improve the locking code, particulary so that we don't
need a global mutex to change per-netns data.

I've revised the locking to use guard(mutex) for (almost) all places
that the per-netfs mutex is used.  I think this is an improvement but
would like to know what others think.

I haven't changed _get/_put to _pin/_unpin as Chuck wondered about.  I'm
not against that (though get/put are widely understood) but nor am I
particularly for it yet.  Again, opinions are welcome.

NeilBrown

 [PATCH 1/3] nfsd: provide proper locking for all write_ function
 [PATCH 2/3] nfsd: use kref and new mutex for global config management
 [PATCH 3/3] nfsd: split nfsd_mutex into one mutex per net-namespace.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/3] nfsd: provide proper locking for all write_ function
  2025-06-20 23:33 [PATCH 0/3 RFC] improve some nfsd_mutex locking NeilBrown
@ 2025-06-20 23:33 ` NeilBrown
  2025-06-21  8:50   ` Li Lingfeng
  2025-06-20 23:33 ` [PATCH 2/3] nfsd: use kref and new mutex for global config management NeilBrown
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: NeilBrown @ 2025-06-20 23:33 UTC (permalink / raw)
  To: Chuck Lever, Jeff Layton
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Li Lingfeng

write_foo functions are called to handle IO to files in /proc/fs/nfsd/.
They can be called at any time and so generally need locking to ensure
they don't happen at an awkward time.

Many already take nfsd_mutex and check if nfsd_serv has been set.  This
ensures they only run when the server is fully configured.

write_filehandle() does *not* need locking.  It interacts with the
export table which is set up when the netns is set up, so it is always
valid and it has its own locking.  write_filehandle() is needed before
the nfs server is started so checking nfsd_serv would be wrong.

The remaining files which do not have any locking are
write_v4_end_grace(), write_unlock_ip(), and write_unlock_fs().
None of these make sense when the nfs server is not running and there is
evidence that write_v4_end_grace() can race with ->client_tracking_op
setup/shutdown and cause problems.

This patch adds locking to these three and ensures the "unlock"
functions abort if ->nfsd_serv is not set.  It uses
    guard(mutex)(&nfsd_mutex);
so there is no need to ensure we unlock on every patch.

Reported-by: Li Lingfeng <lilingfeng3@huawei.com>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfsctl.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 3f3e9f6c4250..0e7e89dc730b 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -221,6 +221,12 @@ static ssize_t write_unlock_ip(struct file *file, char *buf, size_t size)
 	size_t salen = sizeof(address);
 	char *fo_path;
 	struct net *net = netns(file);
+	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
+
+	guard(mutex)(&nfsd_mutex);
+	if (!nn->nfsd_serv)
+		/* There cannot be any files to unlock */
+		return -EINVAL;
 
 	/* sanity check */
 	if (size == 0)
@@ -259,6 +265,12 @@ static ssize_t write_unlock_fs(struct file *file, char *buf, size_t size)
 	struct path path;
 	char *fo_path;
 	int error;
+	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
+
+	guard(mutex)(&nfsd_mutex);
+	if (!nn->nfsd_serv)
+		/* There cannot be any files to unlock */
+		return -EINVAL;
 
 	/* sanity check */
 	if (size == 0)
@@ -1053,6 +1065,7 @@ static ssize_t write_recoverydir(struct file *file, char *buf, size_t size)
 }
 #endif
 
+
 /*
  * write_v4_end_grace - release grace period for nfsd's v4.x lock manager
  *
@@ -1077,6 +1090,7 @@ static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
 {
 	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
 
+	guard(mutex)(&nfsd_mutex);
 	if (size > 0) {
 		switch(buf[0]) {
 		case 'Y':
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] nfsd: use kref and new mutex for global config management
  2025-06-20 23:33 [PATCH 0/3 RFC] improve some nfsd_mutex locking NeilBrown
  2025-06-20 23:33 ` [PATCH 1/3] nfsd: provide proper locking for all write_ function NeilBrown
@ 2025-06-20 23:33 ` NeilBrown
  2025-06-20 23:33 ` [PATCH 3/3] nfsd: split nfsd_mutex into one mutex per net-namespace NeilBrown
  2025-06-21 15:21 ` [PATCH 0/3 RFC] improve some nfsd_mutex locking Chuck Lever
  3 siblings, 0 replies; 11+ messages in thread
From: NeilBrown @ 2025-06-20 23:33 UTC (permalink / raw)
  To: Chuck Lever, Jeff Layton
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Li Lingfeng

nfsd_mutex is used for two quite different things:
1/ it prevents races when start/stoping global resources:
   the filecache and the v4 state table
2/ it prevents races for per-netns config, typically
   ensure config changes are synchronised w.r.t. server
   startup/shutdown.

This patch splits out the first used.  A subsequent patch improves the
second.

"nfsd_users" is changed to a kref which is can be taken to delay
the shutdown of global services.  nfsd_startup_get(), it is succeeds,
ensure the global services will remain until nfsd_startup_put().

The new mutex, nfsd_startup_mutex, is only take for startup and
shutdown.  It is not needed to protect the kref.

The locking needed by nfsd_file_cache_purge() is now provided internally
by that function so calls don't need to be concerned.

This replaces NFSD_FILE_CACHE_UP which is effective just a flag which
says nfsd_users is non-zero.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/export.c    |  6 ------
 fs/nfsd/filecache.c | 31 +++++++++++--------------------
 fs/nfsd/nfsd.h      |  3 +++
 fs/nfsd/nfssvc.c    | 41 +++++++++++++++++++++++++++--------------
 4 files changed, 41 insertions(+), 40 deletions(-)

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index cadfc2bae60e..1ea3d72ef5c9 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -243,13 +243,7 @@ static struct cache_head *expkey_alloc(void)
 
 static void expkey_flush(void)
 {
-	/*
-	 * Take the nfsd_mutex here to ensure that the file cache is not
-	 * destroyed while we're in the middle of flushing.
-	 */
-	mutex_lock(&nfsd_mutex);
 	nfsd_file_cache_purge(current->nsproxy->net_ns);
-	mutex_unlock(&nfsd_mutex);
 }
 
 static const struct cache_detail svc_expkey_cache_template = {
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index e108b6c705b4..0a9116b7530c 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -50,8 +50,6 @@
 
 #define NFSD_LAUNDRETTE_DELAY		     (2 * HZ)
 
-#define NFSD_FILE_CACHE_UP		     (0)
-
 /* We only care about NFSD_MAY_READ/WRITE for this cache */
 #define NFSD_FILE_MAY_MASK	(NFSD_MAY_READ|NFSD_MAY_WRITE|NFSD_MAY_LOCALIO)
 
@@ -70,7 +68,6 @@ struct nfsd_fcache_disposal {
 static struct kmem_cache		*nfsd_file_slab;
 static struct kmem_cache		*nfsd_file_mark_slab;
 static struct list_lru			nfsd_file_lru;
-static unsigned long			nfsd_file_flags;
 static struct fsnotify_group		*nfsd_file_fsnotify_group;
 static struct delayed_work		nfsd_filecache_laundrette;
 static struct rhltable			nfsd_file_rhltable
@@ -112,9 +109,12 @@ static const struct rhashtable_params nfsd_file_rhash_params = {
 static void
 nfsd_file_schedule_laundrette(void)
 {
-	if (test_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags))
-		queue_delayed_work(system_unbound_wq, &nfsd_filecache_laundrette,
+	if (nfsd_startup_get()) {
+		queue_delayed_work(system_unbound_wq,
+				   &nfsd_filecache_laundrette,
 				   NFSD_LAUNDRETTE_DELAY);
+		nfsd_startup_put();
+	}
 }
 
 static void
@@ -795,10 +795,6 @@ nfsd_file_cache_init(void)
 {
 	int ret;
 
-	lockdep_assert_held(&nfsd_mutex);
-	if (test_and_set_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags) == 1)
-		return 0;
-
 	ret = rhltable_init(&nfsd_file_rhltable, &nfsd_file_rhash_params);
 	if (ret)
 		goto out;
@@ -853,8 +849,6 @@ nfsd_file_cache_init(void)
 
 	INIT_DELAYED_WORK(&nfsd_filecache_laundrette, nfsd_file_gc_worker);
 out:
-	if (ret)
-		clear_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags);
 	return ret;
 out_notifier:
 	lease_unregister_notifier(&nfsd_file_lease_notifier);
@@ -958,9 +952,10 @@ nfsd_file_cache_start_net(struct net *net)
 void
 nfsd_file_cache_purge(struct net *net)
 {
-	lockdep_assert_held(&nfsd_mutex);
-	if (test_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags) == 1)
+	if (nfsd_startup_get()) {
 		__nfsd_file_cache_purge(net);
+		nfsd_startup_put();
+	}
 }
 
 void
@@ -975,10 +970,6 @@ nfsd_file_cache_shutdown(void)
 {
 	int i;
 
-	lockdep_assert_held(&nfsd_mutex);
-	if (test_and_clear_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags) == 0)
-		return;
-
 	lease_unregister_notifier(&nfsd_file_lease_notifier);
 	shrinker_free(nfsd_file_shrinker);
 	/*
@@ -1347,8 +1338,7 @@ int nfsd_file_cache_stats_show(struct seq_file *m, void *v)
 	unsigned long lru = 0, total_age = 0;
 
 	/* Serialize with server shutdown */
-	mutex_lock(&nfsd_mutex);
-	if (test_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags) == 1) {
+	if (nfsd_startup_get()) {
 		struct bucket_table *tbl;
 		struct rhashtable *ht;
 
@@ -1360,8 +1350,9 @@ int nfsd_file_cache_stats_show(struct seq_file *m, void *v)
 		tbl = rht_dereference_rcu(ht->tbl, ht);
 		buckets = tbl->size;
 		rcu_read_unlock();
+
+		nfsd_startup_put();
 	}
-	mutex_unlock(&nfsd_mutex);
 
 	for_each_possible_cpu(i) {
 		hits += per_cpu(nfsd_file_cache_hits, i);
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 1bfd0b4e9af7..8ad9fcc23789 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -80,6 +80,9 @@ extern const struct svc_version	nfsd_version2, nfsd_version3, nfsd_version4;
 extern struct mutex		nfsd_mutex;
 extern atomic_t			nfsd_th_cnt;		/* number of available threads */
 
+bool nfsd_startup_get(void);
+void nfsd_startup_put(void);
+
 extern const struct seq_operations nfs_exports_op;
 
 /*
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 82b0111ac469..b2080e5a71e6 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -270,38 +270,51 @@ static int nfsd_init_socks(struct net *net, const struct cred *cred)
 	return 0;
 }
 
-static int nfsd_users = 0;
+static struct kref nfsd_users = KREF_INIT(0);
+static DEFINE_MUTEX(nfsd_startup_mutex);
 
 static int nfsd_startup_generic(void)
 {
-	int ret;
+	int ret = 0;
 
-	if (nfsd_users++)
+	if (kref_get_unless_zero(&nfsd_users))
 		return 0;
+	mutex_lock(&nfsd_startup_mutex);
+	if (kref_get_unless_zero(&nfsd_users))
+		goto out_unlock;
 
 	ret = nfsd_file_cache_init();
 	if (ret)
-		goto dec_users;
+		goto out_unlock;
 
 	ret = nfs4_state_start();
 	if (ret)
 		goto out_file_cache;
-	return 0;
+	kref_init(&nfsd_users);
+out_unlock:
+	mutex_unlock(&nfsd_startup_mutex);
+	return ret;
 
 out_file_cache:
 	nfsd_file_cache_shutdown();
-dec_users:
-	nfsd_users--;
-	return ret;
+	goto out_unlock;
 }
 
-static void nfsd_shutdown_generic(void)
+static void nfsd_shutdown_cb(struct kref *kref)
 {
-	if (--nfsd_users)
-		return;
-
 	nfs4_state_shutdown();
 	nfsd_file_cache_shutdown();
+	mutex_unlock(&nfsd_startup_mutex);
+}
+
+bool nfsd_startup_get(void)
+{
+	return kref_get_unless_zero(&nfsd_users);
+}
+
+void nfsd_startup_put(void)
+{
+	kref_put_mutex(&nfsd_users, nfsd_shutdown_cb, &nfsd_startup_mutex);
 }
 
 static bool nfsd_needs_lockd(struct nfsd_net *nn)
@@ -416,7 +429,7 @@ static int nfsd_startup_net(struct net *net, const struct cred *cred)
 		nn->lockd_up = false;
 	}
 out_socks:
-	nfsd_shutdown_generic();
+	nfsd_startup_put();
 	return ret;
 }
 
@@ -443,7 +456,7 @@ static void nfsd_shutdown_net(struct net *net)
 	percpu_ref_exit(&nn->nfsd_net_ref);
 
 	nn->nfsd_net_up = false;
-	nfsd_shutdown_generic();
+	nfsd_startup_put();
 }
 
 static DEFINE_SPINLOCK(nfsd_notifier_lock);
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] nfsd: split nfsd_mutex into one mutex per net-namespace.
  2025-06-20 23:33 [PATCH 0/3 RFC] improve some nfsd_mutex locking NeilBrown
  2025-06-20 23:33 ` [PATCH 1/3] nfsd: provide proper locking for all write_ function NeilBrown
  2025-06-20 23:33 ` [PATCH 2/3] nfsd: use kref and new mutex for global config management NeilBrown
@ 2025-06-20 23:33 ` NeilBrown
  2025-06-21 13:02   ` kernel test robot
  2025-06-21 15:21 ` [PATCH 0/3 RFC] improve some nfsd_mutex locking Chuck Lever
  3 siblings, 1 reply; 11+ messages in thread
From: NeilBrown @ 2025-06-20 23:33 UTC (permalink / raw)
  To: Chuck Lever, Jeff Layton
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Li Lingfeng

The remaining uses for nfsd_mutex are all to protect per-netns
resources.

This patch replaces the global mutex with one per netns.  The "svc_info"
struct now contains that mutex rather than a pointer to the mutex.

The use of guard(mutex)(...) is extended to all users of this lock.

Signed-off-by: NeilBrown <neil@brown.name>
---
 .../admin-guide/nfs/nfsd-admin-interfaces.rst |   4 +-
 fs/nfsd/netns.h                               |   1 +
 fs/nfsd/nfsctl.c                              | 158 +++++++-----------
 fs/nfsd/nfsd.h                                |   1 -
 fs/nfsd/nfssvc.c                              |  40 ++---
 include/linux/sunrpc/svc.h                    |   2 +-
 net/sunrpc/svc_xprt.c                         |   4 +-
 7 files changed, 73 insertions(+), 137 deletions(-)

diff --git a/Documentation/admin-guide/nfs/nfsd-admin-interfaces.rst b/Documentation/admin-guide/nfs/nfsd-admin-interfaces.rst
index c05926f79054..0d9c3392e1ed 100644
--- a/Documentation/admin-guide/nfs/nfsd-admin-interfaces.rst
+++ b/Documentation/admin-guide/nfs/nfsd-admin-interfaces.rst
@@ -37,4 +37,6 @@ Implementation notes
 
 Note that the rpc server requires the caller to serialize addition and
 removal of listening sockets, and startup and shutdown of the server.
-For nfsd this is done using nfsd_mutex.
+For nfsd this is done using nfsd_info.mutex in struct nfsd_net, which is
+called config_mutex.
+
diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index 3e2d0fde80a7..d05e3f405d2e 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -134,6 +134,7 @@ struct nfsd_net {
 
 	struct svc_info nfsd_info;
 #define nfsd_serv nfsd_info.serv
+#define config_mutex nfsd_info.mutex
 
 	struct percpu_ref nfsd_net_ref;
 	struct completion nfsd_net_confirm_done;
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 0e7e89dc730b..66c87f63a258 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -223,7 +223,7 @@ static ssize_t write_unlock_ip(struct file *file, char *buf, size_t size)
 	struct net *net = netns(file);
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
-	guard(mutex)(&nfsd_mutex);
+	guard(mutex)(&nn->config_mutex);
 	if (!nn->nfsd_serv)
 		/* There cannot be any files to unlock */
 		return -EINVAL;
@@ -267,7 +267,7 @@ static ssize_t write_unlock_fs(struct file *file, char *buf, size_t size)
 	int error;
 	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
 
-	guard(mutex)(&nfsd_mutex);
+	guard(mutex)(&nn->config_mutex);
 	if (!nn->nfsd_serv)
 		/* There cannot be any files to unlock */
 		return -EINVAL;
@@ -413,6 +413,7 @@ static ssize_t write_threads(struct file *file, char *buf, size_t size)
 	char *mesg = buf;
 	int rv;
 	struct net *net = netns(file);
+	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
 	if (size > 0) {
 		int newthreads;
@@ -422,9 +423,8 @@ static ssize_t write_threads(struct file *file, char *buf, size_t size)
 		if (newthreads < 0)
 			return -EINVAL;
 		trace_nfsd_ctl_threads(net, newthreads);
-		mutex_lock(&nfsd_mutex);
+		guard(mutex)(&nn->config_mutex);
 		rv = nfsd_svc(1, &newthreads, net, file->f_cred, NULL);
-		mutex_unlock(&nfsd_mutex);
 		if (rv < 0)
 			return rv;
 	} else
@@ -467,8 +467,10 @@ static ssize_t write_pool_threads(struct file *file, char *buf, size_t size)
 	int npools;
 	int *nthreads;
 	struct net *net = netns(file);
+	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
+
+	guard(mutex)(&nn->config_mutex);
 
-	mutex_lock(&nfsd_mutex);
 	npools = nfsd_nrpools(net);
 	if (npools == 0) {
 		/*
@@ -476,7 +478,6 @@ static ssize_t write_pool_threads(struct file *file, char *buf, size_t size)
 		 * writing to the threads file but NOT the pool_threads
 		 * file, sorry.  Report zero threads.
 		 */
-		mutex_unlock(&nfsd_mutex);
 		strcpy(buf, "0\n");
 		return strlen(buf);
 	}
@@ -526,7 +527,6 @@ static ssize_t write_pool_threads(struct file *file, char *buf, size_t size)
 	rv = mesg - buf;
 out_free:
 	kfree(nthreads);
-	mutex_unlock(&nfsd_mutex);
 	return rv;
 }
 
@@ -689,12 +689,10 @@ static ssize_t __write_versions(struct file *file, char *buf, size_t size)
  */
 static ssize_t write_versions(struct file *file, char *buf, size_t size)
 {
-	ssize_t rv;
+	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
 
-	mutex_lock(&nfsd_mutex);
-	rv = __write_versions(file, buf, size);
-	mutex_unlock(&nfsd_mutex);
-	return rv;
+	guard(mutex)(&nn->config_mutex);
+	return __write_versions(file, buf, size);
 }
 
 /*
@@ -848,15 +846,12 @@ static ssize_t __write_ports(struct file *file, char *buf, size_t size,
  */
 static ssize_t write_ports(struct file *file, char *buf, size_t size)
 {
-	ssize_t rv;
+	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
 
-	mutex_lock(&nfsd_mutex);
-	rv = __write_ports(file, buf, size, netns(file));
-	mutex_unlock(&nfsd_mutex);
-	return rv;
+	guard(mutex)(&nn->config_mutex);
+	return __write_ports(file, buf, size, netns(file));
 }
 
-
 int nfsd_max_blksize;
 
 /*
@@ -898,13 +893,11 @@ static ssize_t write_maxblksize(struct file *file, char *buf, size_t size)
 		bsize = max_t(int, bsize, 1024);
 		bsize = min_t(int, bsize, NFSSVC_MAXBLKSIZE);
 		bsize &= ~(1024-1);
-		mutex_lock(&nfsd_mutex);
-		if (nn->nfsd_serv) {
-			mutex_unlock(&nfsd_mutex);
+
+		guard(mutex)(&nn->config_mutex);
+		if (nn->nfsd_serv)
 			return -EBUSY;
-		}
 		nfsd_max_blksize = bsize;
-		mutex_unlock(&nfsd_mutex);
 	}
 
 	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%d\n",
@@ -951,12 +944,8 @@ static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,
 static ssize_t nfsd4_write_time(struct file *file, char *buf, size_t size,
 				time64_t *time, struct nfsd_net *nn)
 {
-	ssize_t rv;
-
-	mutex_lock(&nfsd_mutex);
-	rv = __nfsd4_write_time(file, buf, size, time, nn);
-	mutex_unlock(&nfsd_mutex);
-	return rv;
+	guard(mutex)(&nn->config_mutex);
+	return __nfsd4_write_time(file, buf, size, time, nn);
 }
 
 /*
@@ -1055,13 +1044,10 @@ static ssize_t __write_recoverydir(struct file *file, char *buf, size_t size,
  */
 static ssize_t write_recoverydir(struct file *file, char *buf, size_t size)
 {
-	ssize_t rv;
 	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
 
-	mutex_lock(&nfsd_mutex);
-	rv = __write_recoverydir(file, buf, size, nn);
-	mutex_unlock(&nfsd_mutex);
-	return rv;
+	guard(mutex)(&nn->config_mutex);
+	return __write_recoverydir(file, buf, size, nn);
 }
 #endif
 
@@ -1090,7 +1076,7 @@ static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
 {
 	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
 
-	guard(mutex)(&nfsd_mutex);
+	guard(mutex)(&nn->config_mutex);
 	if (size > 0) {
 		switch(buf[0]) {
 		case 'Y':
@@ -1525,17 +1511,13 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
 				  struct netlink_callback *cb)
 {
 	int i, ret, rqstp_index = 0;
-	struct nfsd_net *nn;
-
-	mutex_lock(&nfsd_mutex);
+	struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
 
-	nn = net_generic(sock_net(skb->sk), nfsd_net_id);
-	if (!nn->nfsd_serv) {
-		ret = -ENODEV;
-		goto out_unlock;
-	}
+	guard(mutex)(&nn->config_mutex);
+	if (!nn->nfsd_serv)
+		return -ENODEV;
 
-	rcu_read_lock();
+	guard(rcu)();
 
 	for (i = 0; i < nn->nfsd_serv->sv_nrpools; i++) {
 		struct svc_rqst *rqstp;
@@ -1601,17 +1583,13 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
 			ret = nfsd_genl_rpc_status_compose_msg(skb, cb,
 							       &genl_rqstp);
 			if (ret)
-				goto out;
+				return ret;
 		}
 	}
 
 	cb->args[0] = i;
 	cb->args[1] = rqstp_index;
 	ret = skb->len;
-out:
-	rcu_read_unlock();
-out_unlock:
-	mutex_unlock(&nfsd_mutex);
 
 	return ret;
 }
@@ -1640,14 +1618,12 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 			count++;
 	}
 
-	mutex_lock(&nfsd_mutex);
+	guard(mutex)(&nn->config_mutex);
 
 	nrpools = max(count, nfsd_nrpools(net));
 	nthreads = kcalloc(nrpools, sizeof(int), GFP_KERNEL);
-	if (!nthreads) {
-		ret = -ENOMEM;
-		goto out_unlock;
-	}
+	if (!nthreads)
+		return -ENOMEM;
 
 	i = 0;
 	nlmsg_for_each_attr(attr, info->nlhdr, GENL_HDRLEN, rem) {
@@ -1663,7 +1639,7 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 	    info->attrs[NFSD_A_SERVER_SCOPE]) {
 		ret = -EBUSY;
 		if (nn->nfsd_serv && nn->nfsd_serv->sv_nrthreads)
-			goto out_unlock;
+			goto out_free;
 
 		ret = -EINVAL;
 		attr = info->attrs[NFSD_A_SERVER_GRACETIME];
@@ -1671,7 +1647,7 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 			u32 gracetime = nla_get_u32(attr);
 
 			if (gracetime < 10 || gracetime > 3600)
-				goto out_unlock;
+				goto out_free;
 
 			nn->nfsd4_grace = gracetime;
 		}
@@ -1681,7 +1657,7 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 			u32 leasetime = nla_get_u32(attr);
 
 			if (leasetime < 10 || leasetime > 3600)
-				goto out_unlock;
+				goto out_free;
 
 			nn->nfsd4_lease = leasetime;
 		}
@@ -1694,8 +1670,7 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
 	ret = nfsd_svc(nrpools, nthreads, net, get_current_cred(), scope);
 	if (ret > 0)
 		ret = 0;
-out_unlock:
-	mutex_unlock(&nfsd_mutex);
+out_free:
 	kfree(nthreads);
 	return ret;
 }
@@ -1724,7 +1699,7 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
 		goto err_free_msg;
 	}
 
-	mutex_lock(&nfsd_mutex);
+	guard(mutex)(&nn->config_mutex);
 
 	err = nla_put_u32(skb, NFSD_A_SERVER_GRACETIME,
 			  nn->nfsd4_grace) ||
@@ -1733,7 +1708,7 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
 	      nla_put_string(skb, NFSD_A_SERVER_SCOPE,
 			  nn->nfsd_name);
 	if (err)
-		goto err_unlock;
+		goto err_free_msg;
 
 	if (nn->nfsd_serv) {
 		int i;
@@ -1744,22 +1719,18 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
 			err = nla_put_u32(skb, NFSD_A_SERVER_THREADS,
 					  sp->sp_nrthreads);
 			if (err)
-				goto err_unlock;
+				goto err_free_msg;
 		}
 	} else {
 		err = nla_put_u32(skb, NFSD_A_SERVER_THREADS, 0);
 		if (err)
-			goto err_unlock;
+			goto err_free_msg;
 	}
 
-	mutex_unlock(&nfsd_mutex);
-
 	genlmsg_end(skb, hdr);
 
 	return genlmsg_reply(skb, info);
 
-err_unlock:
-	mutex_unlock(&nfsd_mutex);
 err_free_msg:
 	nlmsg_free(skb);
 
@@ -1782,13 +1753,10 @@ int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
 	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_PROTO_VERSION))
 		return -EINVAL;
 
-	mutex_lock(&nfsd_mutex);
-
 	nn = net_generic(genl_info_net(info), nfsd_net_id);
-	if (nn->nfsd_serv) {
-		mutex_unlock(&nfsd_mutex);
+	guard(mutex)(&nn->config_mutex);
+	if (nn->nfsd_serv)
 		return -EBUSY;
-	}
 
 	/* clear current supported versions. */
 	nfsd_vers(nn, 2, NFSD_CLEAR);
@@ -1831,8 +1799,6 @@ int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
 		}
 	}
 
-	mutex_unlock(&nfsd_mutex);
-
 	return 0;
 }
 
@@ -1859,8 +1825,8 @@ int nfsd_nl_version_get_doit(struct sk_buff *skb, struct genl_info *info)
 		goto err_free_msg;
 	}
 
-	mutex_lock(&nfsd_mutex);
 	nn = net_generic(genl_info_net(info), nfsd_net_id);
+	guard(mutex)(&nn->config_mutex);
 
 	for (i = 2; i <= 4; i++) {
 		int j;
@@ -1882,13 +1848,13 @@ int nfsd_nl_version_get_doit(struct sk_buff *skb, struct genl_info *info)
 					      NFSD_A_SERVER_PROTO_VERSION);
 			if (!attr) {
 				err = -EINVAL;
-				goto err_nfsd_unlock;
+				goto err_free_msg;
 			}
 
 			if (nla_put_u32(skb, NFSD_A_VERSION_MAJOR, i) ||
 			    nla_put_u32(skb, NFSD_A_VERSION_MINOR, j)) {
 				err = -EINVAL;
-				goto err_nfsd_unlock;
+				goto err_free_msg;
 			}
 
 			/* Set the enabled flag if the version is enabled */
@@ -1896,20 +1862,17 @@ int nfsd_nl_version_get_doit(struct sk_buff *skb, struct genl_info *info)
 			    (i < 4 || nfsd_minorversion(nn, j, NFSD_TEST)) &&
 			    nla_put_flag(skb, NFSD_A_VERSION_ENABLED)) {
 				err = -EINVAL;
-				goto err_nfsd_unlock;
+				goto err_free_msg;
 			}
 
 			nla_nest_end(skb, attr);
 		}
 	}
 
-	mutex_unlock(&nfsd_mutex);
 	genlmsg_end(skb, hdr);
 
 	return genlmsg_reply(skb, info);
 
-err_nfsd_unlock:
-	mutex_unlock(&nfsd_mutex);
 err_free_msg:
 	nlmsg_free(skb);
 
@@ -1934,15 +1897,13 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
 	bool delete = false;
 	int err, rem;
 
-	mutex_lock(&nfsd_mutex);
+	nn = net_generic(net, nfsd_net_id);
+	guard(mutex)(&nn->config_mutex);
 
 	err = nfsd_create_serv(net);
-	if (err) {
-		mutex_unlock(&nfsd_mutex);
+	if (err)
 		return err;
-	}
 
-	nn = net_generic(net, nfsd_net_id);
 	serv = nn->nfsd_serv;
 
 	spin_lock_bh(&serv->sv_lock);
@@ -2003,10 +1964,8 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
 	spin_unlock_bh(&serv->sv_lock);
 
 	/* Do not remove listeners while there are active threads. */
-	if (serv->sv_nrthreads) {
-		err = -EBUSY;
-		goto out_unlock_mtx;
-	}
+	if (serv->sv_nrthreads)
+		return -EBUSY;
 
 	/*
 	 * Since we can't delete an arbitrary llist entry, destroy the
@@ -2057,9 +2016,6 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info)
 	if (!serv->sv_nrthreads && list_empty(&nn->nfsd_serv->sv_permsocks))
 		nfsd_destroy_serv(net);
 
-out_unlock_mtx:
-	mutex_unlock(&nfsd_mutex);
-
 	return err;
 }
 
@@ -2088,12 +2044,12 @@ int nfsd_nl_listener_get_doit(struct sk_buff *skb, struct genl_info *info)
 		goto err_free_msg;
 	}
 
-	mutex_lock(&nfsd_mutex);
 	nn = net_generic(genl_info_net(info), nfsd_net_id);
+	guard(mutex)(&nn->config_mutex);
 
 	/* no nfs server? Just send empty socket list */
 	if (!nn->nfsd_serv)
-		goto out_unlock_mtx;
+		goto out;
 
 	serv = nn->nfsd_serv;
 	spin_lock_bh(&serv->sv_lock);
@@ -2103,7 +2059,7 @@ int nfsd_nl_listener_get_doit(struct sk_buff *skb, struct genl_info *info)
 		attr = nla_nest_start(skb, NFSD_A_SERVER_SOCK_ADDR);
 		if (!attr) {
 			err = -EINVAL;
-			goto err_serv_unlock;
+			goto err;
 		}
 
 		if (nla_put_string(skb, NFSD_A_SOCK_TRANSPORT_NAME,
@@ -2112,21 +2068,19 @@ int nfsd_nl_listener_get_doit(struct sk_buff *skb, struct genl_info *info)
 			    sizeof(struct sockaddr_storage),
 			    &xprt->xpt_local)) {
 			err = -EINVAL;
-			goto err_serv_unlock;
+			goto err;
 		}
 
 		nla_nest_end(skb, attr);
 	}
 	spin_unlock_bh(&serv->sv_lock);
-out_unlock_mtx:
-	mutex_unlock(&nfsd_mutex);
+out:
 	genlmsg_end(skb, hdr);
 
 	return genlmsg_reply(skb, info);
 
-err_serv_unlock:
+err:
 	spin_unlock_bh(&serv->sv_lock);
-	mutex_unlock(&nfsd_mutex);
 err_free_msg:
 	nlmsg_free(skb);
 
@@ -2228,7 +2182,7 @@ static __net_init int nfsd_net_init(struct net *net)
 		nn->nfsd_versions[i] = nfsd_support_version(i);
 	for (i = 0; i < sizeof(nn->nfsd4_minorversions); i++)
 		nn->nfsd4_minorversions[i] = nfsd_support_version(4);
-	nn->nfsd_info.mutex = &nfsd_mutex;
+	mutex_init(&nn->config_mutex);
 	nn->nfsd_serv = NULL;
 	nfsd4_init_leases_net(nn);
 	get_random_bytes(&nn->siphash_key, sizeof(nn->siphash_key));
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 8ad9fcc23789..3cbca4d34f48 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -77,7 +77,6 @@ struct nfsd_genl_rqstp {
 
 extern struct svc_program	nfsd_programs[];
 extern const struct svc_version	nfsd_version2, nfsd_version3, nfsd_version4;
-extern struct mutex		nfsd_mutex;
 extern atomic_t			nfsd_th_cnt;		/* number of available threads */
 
 bool nfsd_startup_get(void);
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index b2080e5a71e6..f7fdb05203f2 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -56,20 +56,6 @@ static __be32			nfsd_init_request(struct svc_rqst *,
 						const struct svc_program *,
 						struct svc_process_info *);
 
-/*
- * 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.
- *
- * 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:
- *
- *	user_recovery_dirname
- *	user_lease_time
- *	nfsd_versions
- */
-DEFINE_MUTEX(nfsd_mutex);
-
 #if IS_ENABLED(CONFIG_NFS_LOCALIO)
 static const struct svc_version *localio_versions[] = {
 	[1] = &localio_version1,
@@ -239,14 +225,12 @@ static void nfsd_net_free(struct percpu_ref *ref)
 
 int nfsd_nrthreads(struct net *net)
 {
-	int rv = 0;
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
-	mutex_lock(&nfsd_mutex);
-	if (nn->nfsd_serv)
-		rv = nn->nfsd_serv->sv_nrthreads;
-	mutex_unlock(&nfsd_mutex);
-	return rv;
+	guard(mutex)(&nn->config_mutex);
+	if (!nn->nfsd_serv)
+		return 0;
+	return nn->nfsd_serv->sv_nrthreads;
 }
 
 static int nfsd_init_socks(struct net *net, const struct cred *cred)
@@ -522,7 +506,6 @@ static struct notifier_block nfsd_inet6addr_notifier = {
 };
 #endif
 
-/* Only used under nfsd_mutex, so this atomic may be overkill: */
 static atomic_t nfsd_notifier_refcount = ATOMIC_INIT(0);
 
 /**
@@ -534,7 +517,7 @@ 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);
+	lockdep_assert_held(&nn->config_mutex);
 
 	spin_lock(&nfsd_notifier_lock);
 	nn->nfsd_serv = NULL;
@@ -606,17 +589,14 @@ void nfsd_shutdown_threads(struct net *net)
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 	struct svc_serv *serv;
 
-	mutex_lock(&nfsd_mutex);
+	guard(mutex)(&nn->config_mutex);
 	serv = nn->nfsd_serv;
-	if (serv == NULL) {
-		mutex_unlock(&nfsd_mutex);
+	if (serv == NULL)
 		return;
-	}
 
 	/* Kill outstanding nfsd threads */
 	svc_set_num_threads(serv, NULL, 0);
 	nfsd_destroy_serv(net);
-	mutex_unlock(&nfsd_mutex);
 }
 
 struct svc_rqst *nfsd_current_rqst(void)
@@ -632,7 +612,7 @@ int nfsd_create_serv(struct net *net)
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 	struct svc_serv *serv;
 
-	WARN_ON(!mutex_is_locked(&nfsd_mutex));
+	WARN_ON(!mutex_is_locked(&nn->config_mutex));
 	if (nn->nfsd_serv)
 		return 0;
 
@@ -714,7 +694,7 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
 	int err = 0;
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
-	lockdep_assert_held(&nfsd_mutex);
+	lockdep_assert_held(&nn->config_mutex);
 
 	if (nn->nfsd_serv == NULL || n <= 0)
 		return 0;
@@ -787,7 +767,7 @@ nfsd_svc(int n, int *nthreads, struct net *net, const struct cred *cred, const c
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 	struct svc_serv *serv;
 
-	lockdep_assert_held(&nfsd_mutex);
+	lockdep_assert_held(&nn->config_mutex);
 
 	dprintk("nfsd: creating service\n");
 
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 48666b83fe68..a12fe99156ec 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -98,7 +98,7 @@ struct svc_serv {
 /* This is used by pool_stats to find and lock an svc */
 struct svc_info {
 	struct svc_serv		*serv;
-	struct mutex		*mutex;
+	struct mutex		mutex;
 };
 
 void svc_destroy(struct svc_serv **svcp);
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 8b1837228799..b8352b7d6860 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -1399,7 +1399,7 @@ static void *svc_pool_stats_start(struct seq_file *m, loff_t *pos)
 
 	dprintk("svc_pool_stats_start, *pidx=%u\n", pidx);
 
-	mutex_lock(si->mutex);
+	mutex_lock(&si->mutex);
 
 	if (!pidx)
 		return SEQ_START_TOKEN;
@@ -1436,7 +1436,7 @@ static void svc_pool_stats_stop(struct seq_file *m, void *p)
 {
 	struct svc_info *si = m->private;
 
-	mutex_unlock(si->mutex);
+	mutex_unlock(&si->mutex);
 }
 
 static int svc_pool_stats_show(struct seq_file *m, void *p)
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] nfsd: provide proper locking for all write_ function
  2025-06-20 23:33 ` [PATCH 1/3] nfsd: provide proper locking for all write_ function NeilBrown
@ 2025-06-21  8:50   ` Li Lingfeng
  0 siblings, 0 replies; 11+ messages in thread
From: Li Lingfeng @ 2025-06-21  8:50 UTC (permalink / raw)
  To: NeilBrown, Chuck Lever, Jeff Layton
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, yangerkun

Thank you for the patch, it did fix the issue.

在 2025/6/21 7:33, NeilBrown 写道:
> write_foo functions are called to handle IO to files in /proc/fs/nfsd/.
> They can be called at any time and so generally need locking to ensure
> they don't happen at an awkward time.
>
> Many already take nfsd_mutex and check if nfsd_serv has been set.  This
> ensures they only run when the server is fully configured.
>
> write_filehandle() does *not* need locking.  It interacts with the
> export table which is set up when the netns is set up, so it is always
> valid and it has its own locking.  write_filehandle() is needed before
> the nfs server is started so checking nfsd_serv would be wrong.
>
> The remaining files which do not have any locking are
> write_v4_end_grace(), write_unlock_ip(), and write_unlock_fs().
> None of these make sense when the nfs server is not running and there is
> evidence that write_v4_end_grace() can race with ->client_tracking_op
> setup/shutdown and cause problems.
>
> This patch adds locking to these three and ensures the "unlock"
> functions abort if ->nfsd_serv is not set.  It uses
>      guard(mutex)(&nfsd_mutex);
> so there is no need to ensure we unlock on every patch.
>
> Reported-by: Li Lingfeng <lilingfeng3@huawei.com>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>   fs/nfsd/nfsctl.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 3f3e9f6c4250..0e7e89dc730b 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -221,6 +221,12 @@ static ssize_t write_unlock_ip(struct file *file, char *buf, size_t size)
>   	size_t salen = sizeof(address);
>   	char *fo_path;
>   	struct net *net = netns(file);
> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +
> +	guard(mutex)(&nfsd_mutex);
> +	if (!nn->nfsd_serv)
> +		/* There cannot be any files to unlock */
> +		return -EINVAL;
>   
>   	/* sanity check */
>   	if (size == 0)
> @@ -259,6 +265,12 @@ static ssize_t write_unlock_fs(struct file *file, char *buf, size_t size)
>   	struct path path;
>   	char *fo_path;
>   	int error;
> +	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
> +
> +	guard(mutex)(&nfsd_mutex);
> +	if (!nn->nfsd_serv)
> +		/* There cannot be any files to unlock */
> +		return -EINVAL;
>   
>   	/* sanity check */
>   	if (size == 0)
> @@ -1053,6 +1065,7 @@ static ssize_t write_recoverydir(struct file *file, char *buf, size_t size)
>   }
>   #endif
>   
> +
>   /*
>    * write_v4_end_grace - release grace period for nfsd's v4.x lock manager
>    *
> @@ -1077,6 +1090,7 @@ static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
>   {
>   	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
>   
> +	guard(mutex)(&nfsd_mutex);
>   	if (size > 0) {
>   		switch(buf[0]) {
>   		case 'Y':
Tested-by: Li Lingfeng <lilingfeng3@huawei.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] nfsd: split nfsd_mutex into one mutex per net-namespace.
  2025-06-20 23:33 ` [PATCH 3/3] nfsd: split nfsd_mutex into one mutex per net-namespace NeilBrown
@ 2025-06-21 13:02   ` kernel test robot
  0 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2025-06-21 13:02 UTC (permalink / raw)
  To: NeilBrown, Chuck Lever, Jeff Layton
  Cc: llvm, oe-kbuild-all, linux-nfs, Olga Kornievskaia, Dai Ngo,
	Tom Talpey, Li Lingfeng

Hi NeilBrown,

kernel test robot noticed the following build errors:

[auto build test ERROR on brauner-vfs/vfs.all]
[also build test ERROR on trondmy-nfs/linux-next linus/master v6.16-rc2 next-20250620]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/NeilBrown/nfsd-provide-proper-locking-for-all-write_-function/20250621-073955
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
patch link:    https://lore.kernel.org/r/20250620233802.1453016-4-neil%40brown.name
patch subject: [PATCH 3/3] nfsd: split nfsd_mutex into one mutex per net-namespace.
config: i386-buildonly-randconfig-003-20250621 (https://download.01.org/0day-ci/archive/20250621/202506212005.LpUsPRa3-lkp@intel.com/config)
compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250621/202506212005.LpUsPRa3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506212005.LpUsPRa3-lkp@intel.com/

All errors (new ones prefixed by >>):

>> fs/nfsd/nfsctl.c:1699:3: error: cannot jump from this goto statement to its label
    1699 |                 goto err_free_msg;
         |                 ^
   fs/nfsd/nfsctl.c:1702:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
    1702 |         guard(mutex)(&nn->config_mutex);
         |         ^
   include/linux/cleanup.h:338:15: note: expanded from macro 'guard'
     338 |         CLASS(_name, __UNIQUE_ID(guard))
         |                      ^
   include/linux/compiler.h:166:29: note: expanded from macro '__UNIQUE_ID'
     166 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:45:1: note: expanded from here
      45 | __UNIQUE_ID_guard1097
         | ^
   fs/nfsd/nfsctl.c:1825:3: error: cannot jump from this goto statement to its label
    1825 |                 goto err_free_msg;
         |                 ^
   fs/nfsd/nfsctl.c:1829:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
    1829 |         guard(mutex)(&nn->config_mutex);
         |         ^
   include/linux/cleanup.h:338:15: note: expanded from macro 'guard'
     338 |         CLASS(_name, __UNIQUE_ID(guard))
         |                      ^
   include/linux/compiler.h:166:29: note: expanded from macro '__UNIQUE_ID'
     166 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:68:1: note: expanded from here
      68 | __UNIQUE_ID_guard1099
         | ^
   fs/nfsd/nfsctl.c:2044:3: error: cannot jump from this goto statement to its label
    2044 |                 goto err_free_msg;
         |                 ^
   fs/nfsd/nfsctl.c:2048:2: note: jump bypasses initialization of variable with __attribute__((cleanup))
    2048 |         guard(mutex)(&nn->config_mutex);
         |         ^
   include/linux/cleanup.h:338:15: note: expanded from macro 'guard'
     338 |         CLASS(_name, __UNIQUE_ID(guard))
         |                      ^
   include/linux/compiler.h:166:29: note: expanded from macro '__UNIQUE_ID'
     166 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:118:1: note: expanded from here
     118 | __UNIQUE_ID_guard1101
         | ^
   3 errors generated.


vim +1699 fs/nfsd/nfsctl.c

924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1677  
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1678  /**
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1679   * nfsd_nl_threads_get_doit - get the number of running threads
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1680   * @skb: reply buffer
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1681   * @info: netlink metadata and command arguments
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1682   *
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1683   * Return 0 on success or a negative errno.
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1684   */
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1685  int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1686  {
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1687  	struct net *net = genl_info_net(info);
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1688  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1689  	void *hdr;
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1690  	int err;
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1691  
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1692  	skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1693  	if (!skb)
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1694  		return -ENOMEM;
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1695  
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1696  	hdr = genlmsg_iput(skb, info);
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1697  	if (!hdr) {
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1698  		err = -EMSGSIZE;
924f4fb003ba11 Lorenzo Bianconi 2024-04-23 @1699  		goto err_free_msg;
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1700  	}
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1701  
40677c06f2c6a4 NeilBrown        2025-06-21  1702  	guard(mutex)(&nn->config_mutex);
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1703  
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1704  	err = nla_put_u32(skb, NFSD_A_SERVER_GRACETIME,
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1705  			  nn->nfsd4_grace) ||
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1706  	      nla_put_u32(skb, NFSD_A_SERVER_LEASETIME,
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1707  			  nn->nfsd4_lease) ||
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1708  	      nla_put_string(skb, NFSD_A_SERVER_SCOPE,
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1709  			  nn->nfsd_name);
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1710  	if (err)
40677c06f2c6a4 NeilBrown        2025-06-21  1711  		goto err_free_msg;
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1712  
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1713  	if (nn->nfsd_serv) {
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1714  		int i;
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1715  
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1716  		for (i = 0; i < nfsd_nrpools(net); ++i) {
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1717  			struct svc_pool *sp = &nn->nfsd_serv->sv_pools[i];
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1718  
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1719  			err = nla_put_u32(skb, NFSD_A_SERVER_THREADS,
60749cbe3d8ae5 NeilBrown        2024-07-15  1720  					  sp->sp_nrthreads);
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1721  			if (err)
40677c06f2c6a4 NeilBrown        2025-06-21  1722  				goto err_free_msg;
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1723  		}
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1724  	} else {
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1725  		err = nla_put_u32(skb, NFSD_A_SERVER_THREADS, 0);
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1726  		if (err)
40677c06f2c6a4 NeilBrown        2025-06-21  1727  			goto err_free_msg;
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1728  	}
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1729  
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1730  	genlmsg_end(skb, hdr);
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1731  
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1732  	return genlmsg_reply(skb, info);
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1733  
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1734  err_free_msg:
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1735  	nlmsg_free(skb);
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1736  
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1737  	return err;
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1738  }
924f4fb003ba11 Lorenzo Bianconi 2024-04-23  1739  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3 RFC] improve some nfsd_mutex locking
  2025-06-20 23:33 [PATCH 0/3 RFC] improve some nfsd_mutex locking NeilBrown
                   ` (2 preceding siblings ...)
  2025-06-20 23:33 ` [PATCH 3/3] nfsd: split nfsd_mutex into one mutex per net-namespace NeilBrown
@ 2025-06-21 15:21 ` Chuck Lever
  2025-06-21 17:48   ` Jeff Layton
  2025-06-23  2:47   ` NeilBrown
  3 siblings, 2 replies; 11+ messages in thread
From: Chuck Lever @ 2025-06-21 15:21 UTC (permalink / raw)
  To: NeilBrown, Jeff Layton
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Li Lingfeng

On 6/20/25 7:33 PM, NeilBrown wrote:
> The first patch hopefully fixes a bug with locking as reported by Li
> Lingfeng: some write_foo functions aren't locked properly.
> 
> The other two improve the locking code, particulary so that we don't
> need a global mutex to change per-netns data.
> 
> I've revised the locking to use guard(mutex) for (almost) all places
> that the per-netfs mutex is used.  I think this is an improvement but
> would like to know what others think.
> 
> I haven't changed _get/_put to _pin/_unpin as Chuck wondered about.  I'm
> not against that (though get/put are widely understood) but nor am I
> particularly for it yet.  Again, opinions are welcome.

I think of get and put as operations you do on an object. Saying

  nfsd_startup_get();

seems a little strange to me. As I said before, it seems like you
are protecting a critical section, not a particular object.


-- 
Chuck Lever

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3 RFC] improve some nfsd_mutex locking
  2025-06-21 15:21 ` [PATCH 0/3 RFC] improve some nfsd_mutex locking Chuck Lever
@ 2025-06-21 17:48   ` Jeff Layton
  2025-06-23  2:47   ` NeilBrown
  1 sibling, 0 replies; 11+ messages in thread
From: Jeff Layton @ 2025-06-21 17:48 UTC (permalink / raw)
  To: Chuck Lever, NeilBrown
  Cc: linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey, Li Lingfeng

On Sat, 2025-06-21 at 11:21 -0400, Chuck Lever wrote:
> On 6/20/25 7:33 PM, NeilBrown wrote:
> > The first patch hopefully fixes a bug with locking as reported by Li
> > Lingfeng: some write_foo functions aren't locked properly.
> > 
> > The other two improve the locking code, particulary so that we don't
> > need a global mutex to change per-netns data.
> > 
> > I've revised the locking to use guard(mutex) for (almost) all places
> > that the per-netfs mutex is used.  I think this is an improvement but
> > would like to know what others think.
> > 
> > I haven't changed _get/_put to _pin/_unpin as Chuck wondered about.  I'm
> > not against that (though get/put are widely understood) but nor am I
> > particularly for it yet.  Again, opinions are welcome.
> 
> I think of get and put as operations you do on an object. Saying
> 
>   nfsd_startup_get();
> 
> seems a little strange to me. As I said before, it seems like you
> are protecting a critical section, not a particular object.
> 

I think of it as taking a reference to the service being up and
running. Maybe nfsd_service_get/put() ?

-- 
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3 RFC] improve some nfsd_mutex locking
  2025-06-21 15:21 ` [PATCH 0/3 RFC] improve some nfsd_mutex locking Chuck Lever
  2025-06-21 17:48   ` Jeff Layton
@ 2025-06-23  2:47   ` NeilBrown
  1 sibling, 0 replies; 11+ messages in thread
From: NeilBrown @ 2025-06-23  2:47 UTC (permalink / raw)
  To: Chuck Lever
  Cc: Jeff Layton, linux-nfs, Olga Kornievskaia, Dai Ngo, Tom Talpey,
	Li Lingfeng

On Sun, 22 Jun 2025, Chuck Lever wrote:
> On 6/20/25 7:33 PM, NeilBrown wrote:
> > The first patch hopefully fixes a bug with locking as reported by Li
> > Lingfeng: some write_foo functions aren't locked properly.
> > 
> > The other two improve the locking code, particulary so that we don't
> > need a global mutex to change per-netns data.
> > 
> > I've revised the locking to use guard(mutex) for (almost) all places
> > that the per-netfs mutex is used.  I think this is an improvement but
> > would like to know what others think.
> > 
> > I haven't changed _get/_put to _pin/_unpin as Chuck wondered about.  I'm
> > not against that (though get/put are widely understood) but nor am I
> > particularly for it yet.  Again, opinions are welcome.
> 
> I think of get and put as operations you do on an object. Saying
> 
>   nfsd_startup_get();
> 
> seems a little strange to me. As I said before, it seems like you
> are protecting a critical section, not a particular object.

I agree it looks like a critical section.  That suggests lock/unlock
naming.
A "get" is somewhat like a read_trylock.  But a put isn't much like an
unlock.

But maybe I can side-step the whole issue.  I think it is reasonable to
move the nfsd_shutdown_generic() code in to nfsd_exit() which is called
at module_put time.  There is no real need for any this before then.
With that done the ref on the module that we already maintain correctly
will ensure no code can race with the cleanup in
nfsd_shutdown_generic().

Then we don't need nfsd_users as a counter - just a flag to say if
startup has completed yet.

This series does that.  It also delays the conversion to guard(mutex)
until a final patch so it can be reviewed separately - or rejected
separately.

Thanks,
NeilBrown

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-06-23  2:48 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-20 23:33 [PATCH 0/3 RFC] improve some nfsd_mutex locking NeilBrown
2025-06-20 23:33 ` [PATCH 1/3] nfsd: provide proper locking for all write_ function NeilBrown
2025-06-21  8:50   ` Li Lingfeng
2025-06-20 23:33 ` [PATCH 2/3] nfsd: use kref and new mutex for global config management NeilBrown
2025-06-20 23:33 ` [PATCH 3/3] nfsd: split nfsd_mutex into one mutex per net-namespace NeilBrown
2025-06-21 13:02   ` kernel test robot
2025-06-21 15:21 ` [PATCH 0/3 RFC] improve some nfsd_mutex locking Chuck Lever
2025-06-21 17:48   ` Jeff Layton
2025-06-23  2:47   ` NeilBrown
  -- strict thread matches above, loose matches on Subject: below --
2025-06-18 21:31 NeilBrown
2025-06-18 21:31 ` [PATCH 1/3] nfsd: provide proper locking for all write_ function NeilBrown
2025-06-20 12:59   ` Jeff Layton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox