[PATCHES] nfsctl fix and cleanups

[PATCHES] nfsctl fix and cleanups

[Al Viro]

	More stuff pulled out of tree-in-dcache pile, this time nfsctl.
The first one in the series is a fix for minor bogosity, the rest -
cleanups.  Elimination of more d_alloc_name() call sites on conversions
to simple_start_creating() is what got that into preparation parts of
tree-in-dcache...

Branch in -rc5-based, lives in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.nfsctl
individual patches in followups.  If nobody objects, into -next it goes...

Shortlog:
      nfsctl: symlink has no business bumping link count of parent directory
      nfsd_mkdir(): switch to simple_start_creating()
      _nfsd_symlink(): switch to simple_start_creating()
      nfsdfs_create_files(): switch to simple_start_creating()
      nfsd_get_inode(): lift setting ->i_{,f}op to callers.

Diffstat:
 fs/nfsd/nfsctl.c | 137 ++++++++++++++++++++-----------------------------------
 1 file changed, 49 insertions(+), 88 deletions(-)

[PATCH 1/5] nfsctl: symlink has no business bumping link count of parent directory

[Al Viro]

mkdir should incrment the parent's refcount; symlink should not.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/nfsd/nfsctl.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index bc6b776fc657..282b961d8788 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1181,7 +1181,6 @@ static int __nfsd_symlink(struct inode *dir, struct dentry *dentry,
 	inode->i_size = strlen(content);
 
 	d_add(dentry, inode);
-	inc_nlink(dir);
 	fsnotify_create(dir, dentry);
 	return 0;
 }
-- 
2.47.2

[PATCH 2/5] nfsd_mkdir(): switch to simple_start_creating()

[Al Viro]

... and fold __nfsd_mkdir() into it.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/nfsd/nfsctl.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 282b961d8788..6d60bc48f96e 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1128,43 +1128,30 @@ static struct inode *nfsd_get_inode(struct super_block *sb, umode_t mode)
 	return inode;
 }
 
-static int __nfsd_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode, struct nfsdfs_client *ncl)
+static struct dentry *nfsd_mkdir(struct dentry *parent, struct nfsdfs_client *ncl, char *name)
 {
+	struct inode *dir = parent->d_inode;
+	struct dentry *dentry;
 	struct inode *inode;
 
-	inode = nfsd_get_inode(dir->i_sb, mode);
+	inode = nfsd_get_inode(parent->d_sb, S_IFDIR | 0600);
 	if (!inode)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
+
+	dentry = simple_start_creating(parent, name);
+	if (IS_ERR(dentry)) {
+		iput(inode);
+		return dentry;
+	}
 	if (ncl) {
 		inode->i_private = ncl;
 		kref_get(&ncl->cl_ref);
 	}
-	d_add(dentry, inode);
+	d_instantiate(dentry, inode);
 	inc_nlink(dir);
 	fsnotify_mkdir(dir, dentry);
-	return 0;
-}
-
-static struct dentry *nfsd_mkdir(struct dentry *parent, struct nfsdfs_client *ncl, char *name)
-{
-	struct inode *dir = parent->d_inode;
-	struct dentry *dentry;
-	int ret = -ENOMEM;
-
-	inode_lock(dir);
-	dentry = d_alloc_name(parent, name);
-	if (!dentry)
-		goto out_err;
-	ret = __nfsd_mkdir(d_inode(parent), dentry, S_IFDIR | 0600, ncl);
-	if (ret)
-		goto out_err;
-out:
 	inode_unlock(dir);
 	return dentry;
-out_err:
-	dput(dentry);
-	dentry = ERR_PTR(ret);
-	goto out;
 }
 
 #if IS_ENABLED(CONFIG_SUNRPC_GSS)
-- 
2.47.2

[PATCH 3/5] _nfsd_symlink(): switch to simple_start_creating()

[Al Viro]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/nfsd/nfsctl.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 6d60bc48f96e..1b5e417784f6 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1155,23 +1155,6 @@ static struct dentry *nfsd_mkdir(struct dentry *parent, struct nfsdfs_client *nc
 }
 
 #if IS_ENABLED(CONFIG_SUNRPC_GSS)
-static int __nfsd_symlink(struct inode *dir, struct dentry *dentry,
-			  umode_t mode, const char *content)
-{
-	struct inode *inode;
-
-	inode = nfsd_get_inode(dir->i_sb, mode);
-	if (!inode)
-		return -ENOMEM;
-
-	inode->i_link = (char *)content;
-	inode->i_size = strlen(content);
-
-	d_add(dentry, inode);
-	fsnotify_create(dir, dentry);
-	return 0;
-}
-
 /*
  * @content is assumed to be a NUL-terminated string that lives
  * longer than the symlink itself.
@@ -1180,17 +1163,24 @@ static void _nfsd_symlink(struct dentry *parent, const char *name,
 			  const char *content)
 {
 	struct inode *dir = parent->d_inode;
+	struct inode *inode;
 	struct dentry *dentry;
-	int ret;
 
-	inode_lock(dir);
-	dentry = d_alloc_name(parent, name);
-	if (!dentry)
-		goto out;
-	ret = __nfsd_symlink(d_inode(parent), dentry, S_IFLNK | 0777, content);
-	if (ret)
-		dput(dentry);
-out:
+	inode = nfsd_get_inode(dir->i_sb, S_IFLNK | 0777);
+	if (!inode)
+		return;
+
+	dentry = simple_start_creating(parent, name);
+	if (IS_ERR(dentry)) {
+		iput(inode);
+		return;
+	}
+
+	inode->i_link = (char *)content;
+	inode->i_size = strlen(content);
+
+	d_instantiate(dentry, inode);
+	fsnotify_create(dir, dentry);
 	inode_unlock(dir);
 }
 #else
-- 
2.47.2

[PATCH 4/5] nfsdfs_create_files(): switch to simple_start_creating()

[Al Viro]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/nfsd/nfsctl.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 1b5e417784f6..6deabe359a80 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1216,40 +1216,34 @@ struct nfsdfs_client *get_nfsdfs_client(struct inode *inode)
 
 /* XXX: cut'n'paste from simple_fill_super; figure out if we could share
  * code instead. */
-static  int nfsdfs_create_files(struct dentry *root,
+static int nfsdfs_create_files(struct dentry *root,
 				const struct tree_descr *files,
 				struct nfsdfs_client *ncl,
 				struct dentry **fdentries)
 {
 	struct inode *dir = d_inode(root);
-	struct inode *inode;
 	struct dentry *dentry;
-	int i;
 
-	inode_lock(dir);
-	for (i = 0; files->name && files->name[0]; i++, files++) {
-		dentry = d_alloc_name(root, files->name);
-		if (!dentry)
-			goto out;
-		inode = nfsd_get_inode(d_inode(root)->i_sb,
-					S_IFREG | files->mode);
-		if (!inode) {
-			dput(dentry);
-			goto out;
+	for (int i = 0; files->name && files->name[0]; i++, files++) {
+		struct inode *inode = nfsd_get_inode(root->d_sb,
+						     S_IFREG | files->mode);
+		if (!inode)
+			return -ENOMEM;
+		dentry = simple_start_creating(root, files->name);
+		if (IS_ERR(dentry)) {
+			iput(inode);
+			return PTR_ERR(dentry);
 		}
 		kref_get(&ncl->cl_ref);
 		inode->i_fop = files->ops;
 		inode->i_private = ncl;
-		d_add(dentry, inode);
+		d_instantiate(dentry, inode);
 		fsnotify_create(dir, dentry);
 		if (fdentries)
 			fdentries[i] = dentry;
+		inode_unlock(dir);
 	}
-	inode_unlock(dir);
 	return 0;
-out:
-	inode_unlock(dir);
-	return -ENOMEM;
 }
 
 /* on success, returns positive number unique to that client. */
-- 
2.47.2

[PATCH 5/5] nfsd_get_inode(): lift setting ->i_{,f}op to callers.

[Al Viro]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/nfsd/nfsctl.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 6deabe359a80..c19a74a36c45 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1103,27 +1103,14 @@ static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
  *	populating the filesystem.
  */
 
-/* Basically copying rpc_get_inode. */
 static struct inode *nfsd_get_inode(struct super_block *sb, umode_t mode)
 {
 	struct inode *inode = new_inode(sb);
-	if (!inode)
-		return NULL;
-	/* Following advice from simple_fill_super documentation: */
-	inode->i_ino = iunique(sb, NFSD_MaxReserved);
-	inode->i_mode = mode;
-	simple_inode_init_ts(inode);
-	switch (mode & S_IFMT) {
-	case S_IFDIR:
-		inode->i_fop = &simple_dir_operations;
-		inode->i_op = &simple_dir_inode_operations;
-		inc_nlink(inode);
-		break;
-	case S_IFLNK:
-		inode->i_op = &simple_symlink_inode_operations;
-		break;
-	default:
-		break;
+	if (inode) {
+		/* Following advice from simple_fill_super documentation: */
+		inode->i_ino = iunique(sb, NFSD_MaxReserved);
+		inode->i_mode = mode;
+		simple_inode_init_ts(inode);
 	}
 	return inode;
 }
@@ -1143,6 +1130,9 @@ static struct dentry *nfsd_mkdir(struct dentry *parent, struct nfsdfs_client *nc
 		iput(inode);
 		return dentry;
 	}
+	inode->i_fop = &simple_dir_operations;
+	inode->i_op = &simple_dir_inode_operations;
+	inc_nlink(inode);
 	if (ncl) {
 		inode->i_private = ncl;
 		kref_get(&ncl->cl_ref);
@@ -1176,6 +1166,7 @@ static void _nfsd_symlink(struct dentry *parent, const char *name,
 		return;
 	}
 
+	inode->i_op = &simple_symlink_inode_operations;
 	inode->i_link = (char *)content;
 	inode->i_size = strlen(content);
 
-- 
2.47.2

Re: [PATCH 4/5] nfsdfs_create_files(): switch to simple_start_creating()

[NeilBrown]

>  		inode->i_private = ncl;
> -		d_add(dentry, inode);
> +		d_instantiate(dentry, inode);
>  		fsnotify_create(dir, dentry);
>  		if (fdentries)
>  			fdentries[i] = dentry;

I wonder if we should get rid of that if (fdentries) test one day.
fdentries is never NULL.

NeilBrown

Re: [PATCHES] nfsctl fix and cleanups

[NeilBrown]

On Fri, 12 Sep 2025, Al Viro wrote:
> 	More stuff pulled out of tree-in-dcache pile, this time nfsctl.
> The first one in the series is a fix for minor bogosity, the rest -
> cleanups.  Elimination of more d_alloc_name() call sites on conversions
> to simple_start_creating() is what got that into preparation parts of
> tree-in-dcache...
> 
> Branch in -rc5-based, lives in
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.nfsctl
> individual patches in followups.  If nobody objects, into -next it goes...
> 
> Shortlog:
>       nfsctl: symlink has no business bumping link count of parent directory
>       nfsd_mkdir(): switch to simple_start_creating()
>       _nfsd_symlink(): switch to simple_start_creating()
>       nfsdfs_create_files(): switch to simple_start_creating()
>       nfsd_get_inode(): lift setting ->i_{,f}op to callers.

All these look good to me.

Reviewed-by: NeilBrown <neil@brown.name>

Thanks,
NeilBrown


> 
> Diffstat:
>  fs/nfsd/nfsctl.c | 137 ++++++++++++++++++++-----------------------------------
>  1 file changed, 49 insertions(+), 88 deletions(-)
> 

Re: [PATCHES] nfsctl fix and cleanups

[Jeff Layton]

On Thu, 2025-09-11 at 23:44 +0100, Al Viro wrote:
> 	More stuff pulled out of tree-in-dcache pile, this time nfsctl.
> The first one in the series is a fix for minor bogosity, the rest -
> cleanups.  Elimination of more d_alloc_name() call sites on conversions
> to simple_start_creating() is what got that into preparation parts of
> tree-in-dcache...
> 
> Branch in -rc5-based, lives in
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.nfsctl
> individual patches in followups.  If nobody objects, into -next it goes...
> 

(cc'ing Chuck)

Ok, so you're planning to take this in via the vfs tree? Let us know if
you'd rather Chuck do it via nfsd tree.

> Shortlog:
>       nfsctl: symlink has no business bumping link count of parent directory
>       nfsd_mkdir(): switch to simple_start_creating()
>       _nfsd_symlink(): switch to simple_start_creating()
>       nfsdfs_create_files(): switch to simple_start_creating()
>       nfsd_get_inode(): lift setting ->i_{,f}op to callers.
> 
> Diffstat:
>  fs/nfsd/nfsctl.c | 137 ++++++++++++++++++++-----------------------------------
>  1 file changed, 49 insertions(+), 88 deletions(-)

Thanks Al. This all looks great to me.

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