[PATCH 0/5] NFS: Clean up NFS version module loading

[PATCH 0/5] NFS: Clean up NFS version module loading

[Anna Schumaker]

From: Anna Schumaker <anna.schumaker@oracle.com>

I noticed that I initally coded this to use a linked list instead of
an array. This adds a lot of extra "search the list" work that we don't
need to do since we'll always know the (integer) NFS version number that
we are looking for.

While I'm here, I cleaned up the locking to use a reader-write lock
matching how the VFS does things. I also renamed get_nfs_version() to
find_nfs_version(), and implemented a new get_nfs_version() that
abstracts out taking the module reference and acts as the opposite of
put_nfs_version()

Anna


Anna Schumaker (5):
  NFS: Clean up locking the nfs_versions list
  NFS: Convert the NFS module list into an array
  NFS: Rename get_nfs_version() -> find_nfs_version()
  NFS: Clean up find_nfs_version()
  NFS: Implement get_nfs_version()

 fs/nfs/client.c     | 64 ++++++++++++++++++++++++---------------------
 fs/nfs/fs_context.c |  6 ++---
 fs/nfs/namespace.c  |  2 +-
 fs/nfs/nfs.h        |  4 +--
 4 files changed, 40 insertions(+), 36 deletions(-)

2.46.2

[PATCH 1/5] NFS: Clean up locking the nfs_versions list

[Anna Schumaker]

From: Anna Schumaker <anna.schumaker@oracle.com>

This patch replaces the nfs_version_mutex and nfs_version_lock with a
single RW lock that protects access to the nfs_versions list.

The mutex around request_module() seemed unnecessary to me, and I
couldn't find any other callers using a lock around calls to
request_module() when I looked.

At the same time, I saw fs/filesystems.c using a RW lock to protect
their filesystems list. This seems like a better idea than a spinlock to
me, so I'm also making that change while I'm here.

Signed-off-by: Anna Schumaker <anna.schumaker@oracle.com>
---
 fs/nfs/client.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index a1d21c4be0ac..15340df117a7 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -55,8 +55,7 @@
 #define NFSDBG_FACILITY		NFSDBG_CLIENT
 
 static DECLARE_WAIT_QUEUE_HEAD(nfs_client_active_wq);
-static DEFINE_SPINLOCK(nfs_version_lock);
-static DEFINE_MUTEX(nfs_version_mutex);
+static DEFINE_RWLOCK(nfs_version_lock);
 static LIST_HEAD(nfs_versions);
 
 /*
@@ -79,16 +78,16 @@ const struct rpc_program nfs_program = {
 static struct nfs_subversion *find_nfs_version(unsigned int version)
 {
 	struct nfs_subversion *nfs;
-	spin_lock(&nfs_version_lock);
+	read_lock(&nfs_version_lock);
 
 	list_for_each_entry(nfs, &nfs_versions, list) {
 		if (nfs->rpc_ops->version == version) {
-			spin_unlock(&nfs_version_lock);
+			read_unlock(&nfs_version_lock);
 			return nfs;
 		}
 	}
 
-	spin_unlock(&nfs_version_lock);
+	read_unlock(&nfs_version_lock);
 	return ERR_PTR(-EPROTONOSUPPORT);
 }
 
@@ -97,10 +96,8 @@ struct nfs_subversion *get_nfs_version(unsigned int version)
 	struct nfs_subversion *nfs = find_nfs_version(version);
 
 	if (IS_ERR(nfs)) {
-		mutex_lock(&nfs_version_mutex);
 		request_module("nfsv%d", version);
 		nfs = find_nfs_version(version);
-		mutex_unlock(&nfs_version_mutex);
 	}
 
 	if (!IS_ERR(nfs) && !try_module_get(nfs->owner))
@@ -115,23 +112,23 @@ void put_nfs_version(struct nfs_subversion *nfs)
 
 void register_nfs_version(struct nfs_subversion *nfs)
 {
-	spin_lock(&nfs_version_lock);
+	write_lock(&nfs_version_lock);
 
 	list_add(&nfs->list, &nfs_versions);
 	nfs_version[nfs->rpc_ops->version] = nfs->rpc_vers;
 
-	spin_unlock(&nfs_version_lock);
+	write_unlock(&nfs_version_lock);
 }
 EXPORT_SYMBOL_GPL(register_nfs_version);
 
 void unregister_nfs_version(struct nfs_subversion *nfs)
 {
-	spin_lock(&nfs_version_lock);
+	write_lock(&nfs_version_lock);
 
 	nfs_version[nfs->rpc_ops->version] = NULL;
 	list_del(&nfs->list);
 
-	spin_unlock(&nfs_version_lock);
+	write_unlock(&nfs_version_lock);
 }
 EXPORT_SYMBOL_GPL(unregister_nfs_version);
 
-- 
2.46.2

[PATCH 2/5] NFS: Convert the NFS module list into an array

[Anna Schumaker]

From: Anna Schumaker <anna.schumaker@oracle.com>

Using a linked list here seems unnecessarily complex, especially since
possible index values are '2', '3', and '4'. Let's just use an array for
direct access.

Signed-off-by: Anna Schumaker <anna.schumaker@oracle.com>
---
 fs/nfs/client.c | 26 ++++++++++++++------------
 fs/nfs/nfs.h    |  1 -
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 15340df117a7..f5c2be89797a 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -56,7 +56,12 @@
 
 static DECLARE_WAIT_QUEUE_HEAD(nfs_client_active_wq);
 static DEFINE_RWLOCK(nfs_version_lock);
-static LIST_HEAD(nfs_versions);
+
+static struct nfs_subversion *nfs_version_mods[5] = {
+	[2] = NULL,
+	[3] = NULL,
+	[4] = NULL,
+};
 
 /*
  * RPC cruft for NFS
@@ -78,17 +83,14 @@ const struct rpc_program nfs_program = {
 static struct nfs_subversion *find_nfs_version(unsigned int version)
 {
 	struct nfs_subversion *nfs;
+
 	read_lock(&nfs_version_lock);
-
-	list_for_each_entry(nfs, &nfs_versions, list) {
-		if (nfs->rpc_ops->version == version) {
-			read_unlock(&nfs_version_lock);
-			return nfs;
-		}
-	}
-
+	nfs = nfs_version_mods[version];
 	read_unlock(&nfs_version_lock);
-	return ERR_PTR(-EPROTONOSUPPORT);
+
+	if (nfs == NULL)
+		return ERR_PTR(-EPROTONOSUPPORT);
+	return nfs;
 }
 
 struct nfs_subversion *get_nfs_version(unsigned int version)
@@ -114,7 +116,7 @@ void register_nfs_version(struct nfs_subversion *nfs)
 {
 	write_lock(&nfs_version_lock);
 
-	list_add(&nfs->list, &nfs_versions);
+	nfs_version_mods[nfs->rpc_ops->version] = nfs;
 	nfs_version[nfs->rpc_ops->version] = nfs->rpc_vers;
 
 	write_unlock(&nfs_version_lock);
@@ -126,7 +128,7 @@ void unregister_nfs_version(struct nfs_subversion *nfs)
 	write_lock(&nfs_version_lock);
 
 	nfs_version[nfs->rpc_ops->version] = NULL;
-	list_del(&nfs->list);
+	nfs_version_mods[nfs->rpc_ops->version] = NULL;
 
 	write_unlock(&nfs_version_lock);
 }
diff --git a/fs/nfs/nfs.h b/fs/nfs/nfs.h
index 0d3ce0460e35..0329fc3023d0 100644
--- a/fs/nfs/nfs.h
+++ b/fs/nfs/nfs.h
@@ -19,7 +19,6 @@ struct nfs_subversion {
 	const struct nfs_rpc_ops *rpc_ops;	/* NFS operations */
 	const struct super_operations *sops;	/* NFS Super operations */
 	const struct xattr_handler * const *xattr;	/* NFS xattr handlers */
-	struct list_head list;		/* List of NFS versions */
 };
 
 struct nfs_subversion *get_nfs_version(unsigned int);
-- 
2.46.2

[PATCH 3/5] NFS: Rename get_nfs_version() -> find_nfs_version()

[Anna Schumaker]

From: Anna Schumaker <anna.schumaker@oracle.com>

We have a put_nfs_version() that handles refcounting on the nfs version
module, but get_nfs_version() does much more work to find a version
module based on version number. Let's change 'get' to 'find' to better
match what it's doing.

Signed-off-by: Anna Schumaker <anna.schumaker@oracle.com>
---
 fs/nfs/client.c     | 8 ++++----
 fs/nfs/fs_context.c | 2 +-
 fs/nfs/nfs.h        | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index f5c2be89797a..63620766f2a1 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -80,7 +80,7 @@ const struct rpc_program nfs_program = {
 	.pipe_dir_name		= NFS_PIPE_DIRNAME,
 };
 
-static struct nfs_subversion *find_nfs_version(unsigned int version)
+static struct nfs_subversion *__find_nfs_version(unsigned int version)
 {
 	struct nfs_subversion *nfs;
 
@@ -93,13 +93,13 @@ static struct nfs_subversion *find_nfs_version(unsigned int version)
 	return nfs;
 }
 
-struct nfs_subversion *get_nfs_version(unsigned int version)
+struct nfs_subversion *find_nfs_version(unsigned int version)
 {
-	struct nfs_subversion *nfs = find_nfs_version(version);
+	struct nfs_subversion *nfs = __find_nfs_version(version);
 
 	if (IS_ERR(nfs)) {
 		request_module("nfsv%d", version);
-		nfs = find_nfs_version(version);
+		nfs = __find_nfs_version(version);
 	}
 
 	if (!IS_ERR(nfs) && !try_module_get(nfs->owner))
diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index 7e000d782e28..d553daa4c09c 100644
--- a/fs/nfs/fs_context.c
+++ b/fs/nfs/fs_context.c
@@ -1467,7 +1467,7 @@ static int nfs_fs_context_validate(struct fs_context *fc)
 
 	/* Load the NFS protocol module if we haven't done so yet */
 	if (!ctx->nfs_mod) {
-		nfs_mod = get_nfs_version(ctx->version);
+		nfs_mod = find_nfs_version(ctx->version);
 		if (IS_ERR(nfs_mod)) {
 			ret = PTR_ERR(nfs_mod);
 			goto out_version_unavailable;
diff --git a/fs/nfs/nfs.h b/fs/nfs/nfs.h
index 0329fc3023d0..a30bf8ef79d7 100644
--- a/fs/nfs/nfs.h
+++ b/fs/nfs/nfs.h
@@ -21,7 +21,7 @@ struct nfs_subversion {
 	const struct xattr_handler * const *xattr;	/* NFS xattr handlers */
 };
 
-struct nfs_subversion *get_nfs_version(unsigned int);
+struct nfs_subversion *find_nfs_version(unsigned int);
 void put_nfs_version(struct nfs_subversion *);
 void register_nfs_version(struct nfs_subversion *);
 void unregister_nfs_version(struct nfs_subversion *);
-- 
2.46.2

[PATCH 4/5] NFS: Clean up find_nfs_version()

[Anna Schumaker]

From: Anna Schumaker <anna.schumaker@oracle.com>

It's good practice to check the return value of request_module() to see
if the module has been found. It's also a little easier to follow the
code if __find_nfs_version() doesn't attempt to convert NULL pointers
into -EPROTONOSUPPORT.

Signed-off-by: Anna Schumaker <anna.schumaker@oracle.com>
---
 fs/nfs/client.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 63620766f2a1..4fe5a635f329 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -87,9 +87,6 @@ static struct nfs_subversion *__find_nfs_version(unsigned int version)
 	read_lock(&nfs_version_lock);
 	nfs = nfs_version_mods[version];
 	read_unlock(&nfs_version_lock);
-
-	if (nfs == NULL)
-		return ERR_PTR(-EPROTONOSUPPORT);
 	return nfs;
 }
 
@@ -97,13 +94,15 @@ struct nfs_subversion *find_nfs_version(unsigned int version)
 {
 	struct nfs_subversion *nfs = __find_nfs_version(version);
 
-	if (IS_ERR(nfs)) {
-		request_module("nfsv%d", version);
+	if (!nfs && request_module("nfsv%d", version) == 0)
 		nfs = __find_nfs_version(version);
-	}
 
-	if (!IS_ERR(nfs) && !try_module_get(nfs->owner))
+	if (!nfs)
+		return ERR_PTR(-EPROTONOSUPPORT);
+
+	if (!try_module_get(nfs->owner))
 		return ERR_PTR(-EAGAIN);
+
 	return nfs;
 }
 
-- 
2.46.2

[PATCH 5/5] NFS: Implement get_nfs_version()

[Anna Schumaker]

From: Anna Schumaker <anna.schumaker@oracle.com>

This is a pair for put_nfs_version(), and is used for incrementing the
reference count on the nfs version module. I also updated the callers I
could find who had this hardcoded up until now.

Signed-off-by: Anna Schumaker <anna.schumaker@oracle.com>
---
 fs/nfs/client.c     | 10 ++++++++--
 fs/nfs/fs_context.c |  4 ++--
 fs/nfs/namespace.c  |  2 +-
 fs/nfs/nfs.h        |  1 +
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index 4fe5a635f329..68a88f29a1d6 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -100,12 +100,18 @@ struct nfs_subversion *find_nfs_version(unsigned int version)
 	if (!nfs)
 		return ERR_PTR(-EPROTONOSUPPORT);
 
-	if (!try_module_get(nfs->owner))
+	if (!get_nfs_version(nfs))
 		return ERR_PTR(-EAGAIN);
 
 	return nfs;
 }
 
+int get_nfs_version(struct nfs_subversion *nfs)
+{
+	return try_module_get(nfs->owner);
+}
+EXPORT_SYMBOL_GPL(get_nfs_version);
+
 void put_nfs_version(struct nfs_subversion *nfs)
 {
 	module_put(nfs->owner);
@@ -149,7 +155,7 @@ struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_init)
 
 	clp->cl_minorversion = cl_init->minorversion;
 	clp->cl_nfs_mod = cl_init->nfs_mod;
-	if (!try_module_get(clp->cl_nfs_mod->owner))
+	if (!get_nfs_version(clp->cl_nfs_mod))
 		goto error_dealloc;
 
 	clp->rpc_ops = clp->cl_nfs_mod->rpc_ops;
diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index d553daa4c09c..b069385eea17 100644
--- a/fs/nfs/fs_context.c
+++ b/fs/nfs/fs_context.c
@@ -1541,7 +1541,7 @@ static int nfs_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc)
 	}
 	nfs_copy_fh(ctx->mntfh, src->mntfh);
 
-	__module_get(ctx->nfs_mod->owner);
+	get_nfs_version(ctx->nfs_mod);
 	ctx->client_address		= NULL;
 	ctx->mount_server.hostname	= NULL;
 	ctx->nfs_server.export_path	= NULL;
@@ -1633,7 +1633,7 @@ static int nfs_init_fs_context(struct fs_context *fc)
 		}
 
 		ctx->nfs_mod = nfss->nfs_client->cl_nfs_mod;
-		__module_get(ctx->nfs_mod->owner);
+		get_nfs_version(ctx->nfs_mod);
 	} else {
 		/* defaults */
 		ctx->timeo		= NFS_UNSPEC_TIMEO;
diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
index e7494cdd957e..2d53574da605 100644
--- a/fs/nfs/namespace.c
+++ b/fs/nfs/namespace.c
@@ -182,7 +182,7 @@ struct vfsmount *nfs_d_automount(struct path *path)
 	ctx->version		= client->rpc_ops->version;
 	ctx->minorversion	= client->cl_minorversion;
 	ctx->nfs_mod		= client->cl_nfs_mod;
-	__module_get(ctx->nfs_mod->owner);
+	get_nfs_version(ctx->nfs_mod);
 
 	ret = client->rpc_ops->submount(fc, server);
 	if (ret < 0) {
diff --git a/fs/nfs/nfs.h b/fs/nfs/nfs.h
index a30bf8ef79d7..8a5f51be013a 100644
--- a/fs/nfs/nfs.h
+++ b/fs/nfs/nfs.h
@@ -22,6 +22,7 @@ struct nfs_subversion {
 };
 
 struct nfs_subversion *find_nfs_version(unsigned int);
+int get_nfs_version(struct nfs_subversion *);
 void put_nfs_version(struct nfs_subversion *);
 void register_nfs_version(struct nfs_subversion *);
 void unregister_nfs_version(struct nfs_subversion *);
-- 
2.46.2