[PATCHES][RFC][CFT] rpc_pipefs cleanups

[PATCHES][RFC][CFT] rpc_pipefs cleanups

[Al Viro]

	Another series pulled out of tree-in-dcache pile; it massages
rpc_pipefs to use saner primitives.  Lives in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.rpc_pipe
6.16-rc1-based, 17 commits, -314 lines of code...

Individual patches in followups.

Folks, please test and review.  In various forms this has sat in my tree
for more than a year and I'd rather get that logjam dealt with.

Overview:

	Prep/infrastructure (#1 is shared with #work.simple_recursive_removal)

1) simple_recursive_removal(): saner interaction with fsnotify
	fsnotify events are triggered before the callback, unlike in real
unlink(2)/rmdir(2) syscalls.  Obviously matters only in case when callback
is non-empty, which excludes most of the current users in the kernel.

2) new helper: simple_start_creating()
	Set the things up for kernel-initiated creation of object in a
tree-in-dcache filesystem.  With respect to locking it's an equivalent of
filename_create() - we either get a negative dentry with locked parent,
or ERR_PTR() and no locks taken.
	tracefs and debugfs had that open-coded as part of their object
creation machinery; switched to calling new helper.

      rpc_pipefs proper:

3) rpc_pipe: clean failure exits in fill_super
	->kill_sb() will be called immediately after a failure
return anyway, so we don't need to bother with notifier chain and
rpc_gssd_dummy_depopulate().  What's more, rpc_gssd_dummy_populate()
failure exits do not need to bother with __rpc_depopulate() - anything
added to the tree will be taken out by ->kill_sb().

4) rpc_{rmdir_,}depopulate(): use simple_recursive_removal() instead
	no need to give an exact list of objects to be remove when it's
simply every child of the victim directory.

5) rpc_unlink(): use simple_recursive_removal()
	Previous commit was dealing with directories; this one is for
named pipes (i.e. the thing rpc_pipefs is used for).  Note that the
callback of simple_recursive_removal() is called with the parent locked;
the victim isn't locked by the caller.

6) rpc_populate(): lift cleanup into callers
	rpc_populate() is called either from fill_super (where we
don't need to remove any files on failure - rpc_kill_sb() will take
them all out anyway) or from rpc_mkdir_populate(), where we need to
remove the directory we'd been trying to populate along with whatever
we'd put into it before we failed.  Simpler to combine that into
simple_recursive_removal() there.

7) rpc_unlink(): saner calling conventions
	* pass it pipe instead of pipe->dentry
	* zero pipe->dentry afterwards
	* it always returns 0; why bother?
	
8) rpc_mkpipe_dentry(): saner calling conventions
	Instead of returning a dentry or ERR_PTR(-E...), return 0 and
store dentry into pipe->dentry on success and return -E... on failure.
Callers are happier that way...

9) rpc_pipe: don't overdo directory locking
	Don't try to hold directories locked more than VFS requires;
lock just before getting a child to be made positive (using
simple_start_creating()) and unlock as soon as the child is created.
There's no benefit in keeping the parent locked while populating the
child - it won't stop dcache lookups anyway.

10) rpc_pipe: saner primitive for creating subdirectories
	All users of __rpc_mkdir() have the same form - start_creating(),
followed, in case of success, by __rpc_mkdir() and unlocking parent.
Combine that into a single helper, expanding __rpc_mkdir() into it, along
with the call of __rpc_create_common() in it.
	Don't mess with d_drop() + d_add() - just d_instantiate()
and be done with that.	The reason __rpc_create_common() goes for that
dance is that dentry it gets might or might not be hashed; here we know
it's hashed.

11) rpc_pipe: saner primitive for creating regular files
	rpc_new_file(); similar to rpc_new_dir(), except that here we
pass file_operations as well.  Callers don't care about dentry, just
return 0 or -E...

12) rpc_mkpipe_dentry(): switch to start_creating()
	... and make sure we set the rpc_pipe-private part of inode up
before attaching it to dentry.

13) rpc_gssd_dummy_populate(): don't bother with rpc_populate()
	Just have it create gssd (in root), clntXX in gssd, then info
and gssd in clntXX - all with explicit
rpc_new_dir()/rpc_new_file()/rpc_mkpipe_dentry().

14) rpc_pipe: expand the calls of rpc_mkdir_populate()
	... and get rid of convoluted callbacks.

15) rpc_new_dir(): the last argument is always NULL
	All callers except the one in rpc_populate() pass explicit NULL
there; rpc_populate() passes its last argument ('private') instead,
but in the only call of rpc_populate() that creates any subdirectories
(when creating fixed subdirectories of root) private itself is NULL.

16) rpc_create_client_dir(): don't bother with rpc_populate()
	not for a single file...

17) rpc_create_client_dir(): return 0 or -E...
	Callers couldn't care less which dentry did we get - anything
valid is treated as success.

[PATCH 01/17] simple_recursive_removal(): saner interaction with fsnotify

[Al Viro]

Make it match the real unlink(2)/rmdir(2) - notify *after* the
operation.  And use fsnotify_delete() instead of messing with
fsnotify_unlink()/fsnotify_rmdir().

Currently the only caller that cares is the one in debugfs, and
there the order matching the normal syscalls makes more sense;
it'll get more serious for users introduced later in the series.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/libfs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 9ea0ecc325a8..42e226af6095 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -628,12 +628,9 @@ void simple_recursive_removal(struct dentry *dentry,
 			inode_lock(inode);
 			if (simple_positive(victim)) {
 				d_invalidate(victim);	// avoid lost mounts
-				if (d_is_dir(victim))
-					fsnotify_rmdir(inode, victim);
-				else
-					fsnotify_unlink(inode, victim);
 				if (callback)
 					callback(victim);
+				fsnotify_delete(inode, d_inode(victim), victim);
 				dput(victim);		// unpin it
 			}
 			if (victim == dentry) {
-- 
2.39.5

[PATCH 02/17] new helper: simple_start_creating()

[Al Viro]

Set the things up for kernel-initiated creation of object in
a tree-in-dcache filesystem.  With respect to locking it's
an equivalent of filename_create() - we either get a negative
dentry with locked parent, or ERR_PTR() and no locks taken.

tracefs and debugfs had that open-coded as part of their
object creation machinery; switched to calling new helper.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/debugfs/inode.c | 21 ++-------------------
 fs/libfs.c         | 25 +++++++++++++++++++++++++
 fs/tracefs/inode.c | 15 ++-------------
 include/linux/fs.h |  1 +
 4 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 30c4944e1862..08638e39bd12 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -384,26 +384,9 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
 	if (!parent)
 		parent = debugfs_mount->mnt_root;
 
-	inode_lock(d_inode(parent));
-	if (unlikely(IS_DEADDIR(d_inode(parent))))
-		dentry = ERR_PTR(-ENOENT);
-	else
-		dentry = lookup_noperm(&QSTR(name), parent);
-	if (!IS_ERR(dentry) && d_really_is_positive(dentry)) {
-		if (d_is_dir(dentry))
-			pr_err("Directory '%s' with parent '%s' already present!\n",
-			       name, parent->d_name.name);
-		else
-			pr_err("File '%s' in directory '%s' already present!\n",
-			       name, parent->d_name.name);
-		dput(dentry);
-		dentry = ERR_PTR(-EEXIST);
-	}
-
-	if (IS_ERR(dentry)) {
-		inode_unlock(d_inode(parent));
+	dentry = simple_start_creating(parent, name);
+	if (IS_ERR(dentry))
 		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
-	}
 
 	return dentry;
 }
diff --git a/fs/libfs.c b/fs/libfs.c
index 42e226af6095..833ad5ed10f5 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -2260,3 +2260,28 @@ void stashed_dentry_prune(struct dentry *dentry)
 	 */
 	cmpxchg(stashed, dentry, NULL);
 }
+
+/* parent must be held exclusive */
+struct dentry *simple_start_creating(struct dentry *parent, const char *name)
+{
+	struct dentry *dentry;
+	struct inode *dir = d_inode(parent);
+
+	inode_lock(dir);
+	if (unlikely(IS_DEADDIR(dir))) {
+		inode_unlock(dir);
+		return ERR_PTR(-ENOENT);
+	}
+	dentry = lookup_noperm(&QSTR(name), parent);
+	if (IS_ERR(dentry)) {
+		inode_unlock(dir);
+		return dentry;
+	}
+	if (dentry->d_inode) {
+		dput(dentry);
+		inode_unlock(dir);
+		return ERR_PTR(-EEXIST);
+	}
+	return dentry;
+}
+EXPORT_SYMBOL(simple_start_creating);
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index a3fd3cc591bd..4e5d091e9263 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -551,20 +551,9 @@ struct dentry *tracefs_start_creating(const char *name, struct dentry *parent)
 	if (!parent)
 		parent = tracefs_mount->mnt_root;
 
-	inode_lock(d_inode(parent));
-	if (unlikely(IS_DEADDIR(d_inode(parent))))
-		dentry = ERR_PTR(-ENOENT);
-	else
-		dentry = lookup_noperm(&QSTR(name), parent);
-	if (!IS_ERR(dentry) && d_inode(dentry)) {
-		dput(dentry);
-		dentry = ERR_PTR(-EEXIST);
-	}
-
-	if (IS_ERR(dentry)) {
-		inode_unlock(d_inode(parent));
+	dentry = simple_start_creating(parent, name);
+	if (IS_ERR(dentry))
 		simple_release_fs(&tracefs_mount, &tracefs_mount_count);
-	}
 
 	return dentry;
 }
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 96c7925a6551..9f75f8836bbd 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3619,6 +3619,7 @@ extern int simple_fill_super(struct super_block *, unsigned long,
 			     const struct tree_descr *);
 extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
 extern void simple_release_fs(struct vfsmount **mount, int *count);
+struct dentry *simple_start_creating(struct dentry *, const char *);
 
 extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
 			loff_t *ppos, const void *from, size_t available);
-- 
2.39.5

[PATCH 03/17] rpc_pipe: clean failure exits in fill_super

[Al Viro]

->kill_sb() will be called immediately after a failure return
anyway, so we don't need to bother with notifier chain and
rpc_gssd_dummy_depopulate().  What's more, rpc_gssd_dummy_populate()
failure exits do not need to bother with __rpc_depopulate() -
anything added to the tree will be taken out by ->kill_sb().

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/sunrpc/rpc_pipe.c | 67 +++++++++++--------------------------------
 1 file changed, 16 insertions(+), 51 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 98f78cd55905..580e078e49a3 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -1292,7 +1292,7 @@ static const struct rpc_filelist gssd_dummy_info_file[] = {
  * Create a dummy set of directories and a pipe that gssd can hold open to
  * indicate that it is up and running.
  */
-static struct dentry *
+static int
 rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data)
 {
 	int ret = 0;
@@ -1303,58 +1303,37 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data)
 	/* We should never get this far if "gssd" doesn't exist */
 	gssd_dentry = try_lookup_noperm(&QSTR(files[RPCAUTH_gssd].name), root);
 	if (!gssd_dentry)
-		return ERR_PTR(-ENOENT);
+		return -ENOENT;
 
 	ret = rpc_populate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1, NULL);
 	if (ret) {
-		pipe_dentry = ERR_PTR(ret);
-		goto out;
+		dput(gssd_dentry);
+		return ret;
 	}
 
 	clnt_dentry = try_lookup_noperm(&QSTR(gssd_dummy_clnt_dir[0].name),
 					  gssd_dentry);
-	if (!clnt_dentry) {
-		__rpc_depopulate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1);
-		pipe_dentry = ERR_PTR(-ENOENT);
-		goto out;
-	}
+	dput(gssd_dentry);
+	if (!clnt_dentry)
+		return -ENOENT;
 
 	ret = rpc_populate(clnt_dentry, gssd_dummy_info_file, 0, 1, NULL);
 	if (ret) {
-		__rpc_depopulate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1);
-		pipe_dentry = ERR_PTR(ret);
-		goto out;
+		dput(clnt_dentry);
+		return ret;
 	}
-
 	pipe_dentry = rpc_mkpipe_dentry(clnt_dentry, "gssd", NULL, pipe_data);
-	if (IS_ERR(pipe_dentry)) {
-		__rpc_depopulate(clnt_dentry, gssd_dummy_info_file, 0, 1);
-		__rpc_depopulate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1);
-	}
-out:
 	dput(clnt_dentry);
-	dput(gssd_dentry);
-	return pipe_dentry;
-}
-
-static void
-rpc_gssd_dummy_depopulate(struct dentry *pipe_dentry)
-{
-	struct dentry *clnt_dir = pipe_dentry->d_parent;
-	struct dentry *gssd_dir = clnt_dir->d_parent;
-
-	dget(pipe_dentry);
-	__rpc_rmpipe(d_inode(clnt_dir), pipe_dentry);
-	__rpc_depopulate(clnt_dir, gssd_dummy_info_file, 0, 1);
-	__rpc_depopulate(gssd_dir, gssd_dummy_clnt_dir, 0, 1);
-	dput(pipe_dentry);
+	if (IS_ERR(pipe_dentry))
+		ret = PTR_ERR(pipe_dentry);
+	return ret;
 }
 
 static int
 rpc_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	struct inode *inode;
-	struct dentry *root, *gssd_dentry;
+	struct dentry *root;
 	struct net *net = sb->s_fs_info;
 	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
 	int err;
@@ -1373,11 +1352,9 @@ rpc_fill_super(struct super_block *sb, struct fs_context *fc)
 	if (rpc_populate(root, files, RPCAUTH_lockd, RPCAUTH_RootEOF, NULL))
 		return -ENOMEM;
 
-	gssd_dentry = rpc_gssd_dummy_populate(root, sn->gssd_dummy);
-	if (IS_ERR(gssd_dentry)) {
-		__rpc_depopulate(root, files, RPCAUTH_lockd, RPCAUTH_RootEOF);
-		return PTR_ERR(gssd_dentry);
-	}
+	err = rpc_gssd_dummy_populate(root, sn->gssd_dummy);
+	if (err)
+		return err;
 
 	dprintk("RPC:       sending pipefs MOUNT notification for net %x%s\n",
 		net->ns.inum, NET_NAME(net));
@@ -1386,18 +1363,6 @@ rpc_fill_super(struct super_block *sb, struct fs_context *fc)
 	err = blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
 					   RPC_PIPEFS_MOUNT,
 					   sb);
-	if (err)
-		goto err_depopulate;
-	mutex_unlock(&sn->pipefs_sb_lock);
-	return 0;
-
-err_depopulate:
-	rpc_gssd_dummy_depopulate(gssd_dentry);
-	blocking_notifier_call_chain(&rpc_pipefs_notifier_list,
-					   RPC_PIPEFS_UMOUNT,
-					   sb);
-	sn->pipefs_sb = NULL;
-	__rpc_depopulate(root, files, RPCAUTH_lockd, RPCAUTH_RootEOF);
 	mutex_unlock(&sn->pipefs_sb_lock);
 	return err;
 }
-- 
2.39.5

[PATCH 04/17] rpc_{rmdir_,}depopulate(): use simple_recursive_removal() instead

[Al Viro]

no need to give an exact list of objects to be remove when it's
simply every child of the victim directory.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/sunrpc/rpc_pipe.c | 44 +++----------------------------------------
 1 file changed, 3 insertions(+), 41 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 580e078e49a3..9571cbd91305 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -678,17 +678,6 @@ static void __rpc_depopulate(struct dentry *parent,
 	}
 }
 
-static void rpc_depopulate(struct dentry *parent,
-			   const struct rpc_filelist *files,
-			   int start, int eof)
-{
-	struct inode *dir = d_inode(parent);
-
-	inode_lock_nested(dir, I_MUTEX_CHILD);
-	__rpc_depopulate(parent, files, start, eof);
-	inode_unlock(dir);
-}
-
 static int rpc_populate(struct dentry *parent,
 			const struct rpc_filelist *files,
 			int start, int eof,
@@ -762,24 +751,6 @@ static struct dentry *rpc_mkdir_populate(struct dentry *parent,
 	goto out;
 }
 
-static int rpc_rmdir_depopulate(struct dentry *dentry,
-		void (*depopulate)(struct dentry *))
-{
-	struct dentry *parent;
-	struct inode *dir;
-	int error;
-
-	parent = dget_parent(dentry);
-	dir = d_inode(parent);
-	inode_lock_nested(dir, I_MUTEX_PARENT);
-	if (depopulate != NULL)
-		depopulate(dentry);
-	error = __rpc_rmdir(dir, dentry);
-	inode_unlock(dir);
-	dput(parent);
-	return error;
-}
-
 /**
  * rpc_mkpipe_dentry - make an rpc_pipefs file for kernel<->userspace
  *		       communication
@@ -1030,11 +1001,6 @@ static int rpc_clntdir_populate(struct dentry *dentry, void *private)
 			    private);
 }
 
-static void rpc_clntdir_depopulate(struct dentry *dentry)
-{
-	rpc_depopulate(dentry, authfiles, RPCAUTH_info, RPCAUTH_EOF);
-}
-
 /**
  * rpc_create_client_dir - Create a new rpc_client directory in rpc_pipefs
  * @dentry: the parent of new directory
@@ -1073,7 +1039,8 @@ int rpc_remove_client_dir(struct rpc_clnt *rpc_client)
 		return 0;
 	rpc_destroy_pipe_dir_objects(&rpc_client->cl_pipedir_objects);
 	rpc_client->cl_pipedir_objects.pdh_dentry = NULL;
-	return rpc_rmdir_depopulate(dentry, rpc_clntdir_depopulate);
+	simple_recursive_removal(dentry, NULL);
+	return 0;
 }
 
 static const struct rpc_filelist cache_pipefs_files[3] = {
@@ -1101,11 +1068,6 @@ static int rpc_cachedir_populate(struct dentry *dentry, void *private)
 			    private);
 }
 
-static void rpc_cachedir_depopulate(struct dentry *dentry)
-{
-	rpc_depopulate(dentry, cache_pipefs_files, 0, 3);
-}
-
 struct dentry *rpc_create_cache_dir(struct dentry *parent, const char *name,
 				    umode_t umode, struct cache_detail *cd)
 {
@@ -1115,7 +1077,7 @@ struct dentry *rpc_create_cache_dir(struct dentry *parent, const char *name,
 
 void rpc_remove_cache_dir(struct dentry *dentry)
 {
-	rpc_rmdir_depopulate(dentry, rpc_cachedir_depopulate);
+	simple_recursive_removal(dentry, NULL);
 }
 
 /*
-- 
2.39.5

[PATCH 05/17] rpc_unlink(): use simple_recursive_removal()

[Al Viro]

note that the callback of simple_recursive_removal() is called with
the parent locked; the victim isn't locked by the caller.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/sunrpc/rpc_pipe.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 9571cbd91305..67621a94f67b 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -168,8 +168,9 @@ rpc_inode_setowner(struct inode *inode, void *private)
 }
 
 static void
-rpc_close_pipes(struct inode *inode)
+rpc_close_pipes(struct dentry *dentry)
 {
+	struct inode *inode = dentry->d_inode;
 	struct rpc_pipe *pipe = RPC_I(inode)->pipe;
 	int need_release;
 	LIST_HEAD(free_list);
@@ -619,14 +620,6 @@ static int __rpc_unlink(struct inode *dir, struct dentry *dentry)
 	return ret;
 }
 
-static int __rpc_rmpipe(struct inode *dir, struct dentry *dentry)
-{
-	struct inode *inode = d_inode(dentry);
-
-	rpc_close_pipes(inode);
-	return __rpc_unlink(dir, dentry);
-}
-
 static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
 					  const char *name)
 {
@@ -814,17 +807,8 @@ EXPORT_SYMBOL_GPL(rpc_mkpipe_dentry);
 int
 rpc_unlink(struct dentry *dentry)
 {
-	struct dentry *parent;
-	struct inode *dir;
-	int error = 0;
-
-	parent = dget_parent(dentry);
-	dir = d_inode(parent);
-	inode_lock_nested(dir, I_MUTEX_PARENT);
-	error = __rpc_rmpipe(dir, dentry);
-	inode_unlock(dir);
-	dput(parent);
-	return error;
+	simple_recursive_removal(dentry, rpc_close_pipes);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(rpc_unlink);
 
-- 
2.39.5

[PATCH 06/17] rpc_populate(): lift cleanup into callers

[Al Viro]

rpc_populate() is called either from fill_super (where we don't
need to remove any files on failure - rpc_kill_sb() will take
them all out anyway) or from rpc_mkdir_populate(), where we need
to remove the directory we'd been trying to populate along with
whatever we'd put into it before we failed.  Simpler to combine
that into simple_recursive_removal() there.

Note that rpc_pipe is overlocking directories quite a bit -
locked parent is no obstacle to finding a child in dcache, so
keeping it locked won't prevent userland observing a partially
built subtree.

All we need is to follow minimal VFS requirements; it's not
as if clients used directory locking for exclusion - tree
changes are serialized, but that's done on ->pipefs_sb_lock.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/sunrpc/rpc_pipe.c | 71 +++----------------------------------------
 1 file changed, 5 insertions(+), 66 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 67621a94f67b..46fa00ac5e0e 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -594,32 +594,6 @@ static int __rpc_mkpipe_dentry(struct inode *dir, struct dentry *dentry,
 	return 0;
 }
 
-static int __rpc_rmdir(struct inode *dir, struct dentry *dentry)
-{
-	int ret;
-
-	dget(dentry);
-	ret = simple_rmdir(dir, dentry);
-	d_drop(dentry);
-	if (!ret)
-		fsnotify_rmdir(dir, dentry);
-	dput(dentry);
-	return ret;
-}
-
-static int __rpc_unlink(struct inode *dir, struct dentry *dentry)
-{
-	int ret;
-
-	dget(dentry);
-	ret = simple_unlink(dir, dentry);
-	d_drop(dentry);
-	if (!ret)
-		fsnotify_unlink(dir, dentry);
-	dput(dentry);
-	return ret;
-}
-
 static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
 					  const char *name)
 {
@@ -636,41 +610,6 @@ static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
 	return ERR_PTR(-EEXIST);
 }
 
-/*
- * FIXME: This probably has races.
- */
-static void __rpc_depopulate(struct dentry *parent,
-			     const struct rpc_filelist *files,
-			     int start, int eof)
-{
-	struct inode *dir = d_inode(parent);
-	struct dentry *dentry;
-	struct qstr name;
-	int i;
-
-	for (i = start; i < eof; i++) {
-		name.name = files[i].name;
-		name.len = strlen(files[i].name);
-		dentry = try_lookup_noperm(&name, parent);
-
-		if (dentry == NULL)
-			continue;
-		if (d_really_is_negative(dentry))
-			goto next;
-		switch (d_inode(dentry)->i_mode & S_IFMT) {
-			default:
-				BUG();
-			case S_IFREG:
-				__rpc_unlink(dir, dentry);
-				break;
-			case S_IFDIR:
-				__rpc_rmdir(dir, dentry);
-		}
-next:
-		dput(dentry);
-	}
-}
-
 static int rpc_populate(struct dentry *parent,
 			const struct rpc_filelist *files,
 			int start, int eof,
@@ -707,7 +646,6 @@ static int rpc_populate(struct dentry *parent,
 	inode_unlock(dir);
 	return 0;
 out_bad:
-	__rpc_depopulate(parent, files, start, eof);
 	inode_unlock(dir);
 	printk(KERN_WARNING "%s: %s failed to populate directory %pd\n",
 			__FILE__, __func__, parent);
@@ -731,14 +669,15 @@ static struct dentry *rpc_mkdir_populate(struct dentry *parent,
 		goto out_err;
 	if (populate != NULL) {
 		error = populate(dentry, args_populate);
-		if (error)
-			goto err_rmdir;
+		if (error) {
+			inode_unlock(dir);
+			simple_recursive_removal(dentry, NULL);
+			return ERR_PTR(error);
+		}
 	}
 out:
 	inode_unlock(dir);
 	return dentry;
-err_rmdir:
-	__rpc_rmdir(dir, dentry);
 out_err:
 	dentry = ERR_PTR(error);
 	goto out;
-- 
2.39.5

[PATCH 07/17] rpc_unlink(): saner calling conventions

[Al Viro]

1) pass it pipe instead of pipe->dentry
2) zero pipe->dentry afterwards
3) it always returns 0; why bother?

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/nfs/blocklayout/rpc_pipefs.c    | 12 ++----------
 fs/nfs/nfs4idmap.c                 |  6 +-----
 fs/nfsd/nfs4recover.c              | 12 ++----------
 include/linux/sunrpc/rpc_pipe_fs.h |  2 +-
 net/sunrpc/auth_gss/auth_gss.c     |  6 +-----
 net/sunrpc/rpc_pipe.c              | 12 +++++++-----
 6 files changed, 14 insertions(+), 36 deletions(-)

diff --git a/fs/nfs/blocklayout/rpc_pipefs.c b/fs/nfs/blocklayout/rpc_pipefs.c
index d8d50a88de04..25d429e44eb4 100644
--- a/fs/nfs/blocklayout/rpc_pipefs.c
+++ b/fs/nfs/blocklayout/rpc_pipefs.c
@@ -154,13 +154,6 @@ static struct dentry *nfs4blocklayout_register_sb(struct super_block *sb,
 	return dentry;
 }
 
-static void nfs4blocklayout_unregister_sb(struct super_block *sb,
-					  struct rpc_pipe *pipe)
-{
-	if (pipe->dentry)
-		rpc_unlink(pipe->dentry);
-}
-
 static int rpc_pipefs_event(struct notifier_block *nb, unsigned long event,
 			   void *ptr)
 {
@@ -188,8 +181,7 @@ static int rpc_pipefs_event(struct notifier_block *nb, unsigned long event,
 		nn->bl_device_pipe->dentry = dentry;
 		break;
 	case RPC_PIPEFS_UMOUNT:
-		if (nn->bl_device_pipe->dentry)
-			nfs4blocklayout_unregister_sb(sb, nn->bl_device_pipe);
+		rpc_unlink(nn->bl_device_pipe);
 		break;
 	default:
 		ret = -ENOTSUPP;
@@ -224,7 +216,7 @@ static void nfs4blocklayout_unregister_net(struct net *net,
 
 	pipefs_sb = rpc_get_sb_net(net);
 	if (pipefs_sb) {
-		nfs4blocklayout_unregister_sb(pipefs_sb, pipe);
+		rpc_unlink(pipe);
 		rpc_put_sb_net(net);
 	}
 }
diff --git a/fs/nfs/nfs4idmap.c b/fs/nfs/nfs4idmap.c
index 25a7c771cfd8..adc03232b851 100644
--- a/fs/nfs/nfs4idmap.c
+++ b/fs/nfs/nfs4idmap.c
@@ -424,12 +424,8 @@ static void nfs_idmap_pipe_destroy(struct dentry *dir,
 		struct rpc_pipe_dir_object *pdo)
 {
 	struct idmap *idmap = pdo->pdo_data;
-	struct rpc_pipe *pipe = idmap->idmap_pipe;
 
-	if (pipe->dentry) {
-		rpc_unlink(pipe->dentry);
-		pipe->dentry = NULL;
-	}
+	rpc_unlink(idmap->idmap_pipe);
 }
 
 static int nfs_idmap_pipe_create(struct dentry *dir,
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 82785db730d9..bbd29b3b573f 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -963,13 +963,6 @@ nfsd4_cld_register_sb(struct super_block *sb, struct rpc_pipe *pipe)
 	return dentry;
 }
 
-static void
-nfsd4_cld_unregister_sb(struct rpc_pipe *pipe)
-{
-	if (pipe->dentry)
-		rpc_unlink(pipe->dentry);
-}
-
 static struct dentry *
 nfsd4_cld_register_net(struct net *net, struct rpc_pipe *pipe)
 {
@@ -991,7 +984,7 @@ nfsd4_cld_unregister_net(struct net *net, struct rpc_pipe *pipe)
 
 	sb = rpc_get_sb_net(net);
 	if (sb) {
-		nfsd4_cld_unregister_sb(pipe);
+		rpc_unlink(pipe);
 		rpc_put_sb_net(net);
 	}
 }
@@ -2142,8 +2135,7 @@ rpc_pipefs_event(struct notifier_block *nb, unsigned long event, void *ptr)
 		cn->cn_pipe->dentry = dentry;
 		break;
 	case RPC_PIPEFS_UMOUNT:
-		if (cn->cn_pipe->dentry)
-			nfsd4_cld_unregister_sb(cn->cn_pipe);
+		rpc_unlink(cn->cn_pipe);
 		break;
 	default:
 		ret = -ENOTSUPP;
diff --git a/include/linux/sunrpc/rpc_pipe_fs.h b/include/linux/sunrpc/rpc_pipe_fs.h
index 3b35b6f6533a..a8c0a500d55c 100644
--- a/include/linux/sunrpc/rpc_pipe_fs.h
+++ b/include/linux/sunrpc/rpc_pipe_fs.h
@@ -129,7 +129,7 @@ struct rpc_pipe *rpc_mkpipe_data(const struct rpc_pipe_ops *ops, int flags);
 void rpc_destroy_pipe_data(struct rpc_pipe *pipe);
 extern struct dentry *rpc_mkpipe_dentry(struct dentry *, const char *, void *,
 					struct rpc_pipe *);
-extern int rpc_unlink(struct dentry *);
+extern void rpc_unlink(struct rpc_pipe *);
 extern int register_rpc_pipefs(void);
 extern void unregister_rpc_pipefs(void);
 
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 0fa244f16876..f2a44d589cfb 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -887,12 +887,8 @@ static void gss_pipe_dentry_destroy(struct dentry *dir,
 		struct rpc_pipe_dir_object *pdo)
 {
 	struct gss_pipe *gss_pipe = pdo->pdo_data;
-	struct rpc_pipe *pipe = gss_pipe->pipe;
 
-	if (pipe->dentry != NULL) {
-		rpc_unlink(pipe->dentry);
-		pipe->dentry = NULL;
-	}
+	rpc_unlink(gss_pipe->pipe);
 }
 
 static int gss_pipe_dentry_create(struct dentry *dir,
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 46fa00ac5e0e..2046582c4f35 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -737,17 +737,19 @@ EXPORT_SYMBOL_GPL(rpc_mkpipe_dentry);
 
 /**
  * rpc_unlink - remove a pipe
- * @dentry: dentry for the pipe, as returned from rpc_mkpipe
+ * @pipe: the pipe to be removed
  *
  * After this call, lookups will no longer find the pipe, and any
  * attempts to read or write using preexisting opens of the pipe will
  * return -EPIPE.
  */
-int
-rpc_unlink(struct dentry *dentry)
+void
+rpc_unlink(struct rpc_pipe *pipe)
 {
-	simple_recursive_removal(dentry, rpc_close_pipes);
-	return 0;
+	if (pipe->dentry) {
+		simple_recursive_removal(pipe->dentry, rpc_close_pipes);
+		pipe->dentry = NULL;
+	}
 }
 EXPORT_SYMBOL_GPL(rpc_unlink);
 
-- 
2.39.5

[PATCH 08/17] rpc_mkpipe_dentry(): saner calling conventions

[Al Viro]

Instead of returning a dentry or ERR_PTR(-E...), return 0 and store
dentry into pipe->dentry on success and return -E... on failure.

Callers are happier that way...

NOTE: dummy rpc_pipe is getting ->dentry set; we never access that,
since we
	1) never call rpc_unlink() for it (dentry is taken out by
->kill_sb())
	2) never call rpc_queue_upcall() for it (writing to that
sucker fails; no downcalls are ever submitted, so no replies are
going to arrive)
IOW, having that ->dentry set (and left dangling) is harmless,
if ugly; cleaner solution will take more massage.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/nfs/blocklayout/rpc_pipefs.c    | 41 ++++++++++++------------------
 fs/nfs/nfs4idmap.c                 |  8 +-----
 fs/nfsd/nfs4recover.c              | 37 ++++++++++-----------------
 include/linux/sunrpc/rpc_pipe_fs.h |  2 +-
 net/sunrpc/auth_gss/auth_gss.c     |  7 +----
 net/sunrpc/rpc_pipe.c              | 29 +++++++++------------
 6 files changed, 45 insertions(+), 79 deletions(-)

diff --git a/fs/nfs/blocklayout/rpc_pipefs.c b/fs/nfs/blocklayout/rpc_pipefs.c
index 25d429e44eb4..d526f5ba7887 100644
--- a/fs/nfs/blocklayout/rpc_pipefs.c
+++ b/fs/nfs/blocklayout/rpc_pipefs.c
@@ -141,17 +141,18 @@ static const struct rpc_pipe_ops bl_upcall_ops = {
 	.destroy_msg	= bl_pipe_destroy_msg,
 };
 
-static struct dentry *nfs4blocklayout_register_sb(struct super_block *sb,
+static int nfs4blocklayout_register_sb(struct super_block *sb,
 					    struct rpc_pipe *pipe)
 {
-	struct dentry *dir, *dentry;
+	struct dentry *dir;
+	int err;
 
 	dir = rpc_d_lookup_sb(sb, NFS_PIPE_DIRNAME);
 	if (dir == NULL)
-		return ERR_PTR(-ENOENT);
-	dentry = rpc_mkpipe_dentry(dir, "blocklayout", NULL, pipe);
+		return -ENOENT;
+	err = rpc_mkpipe_dentry(dir, "blocklayout", NULL, pipe);
 	dput(dir);
-	return dentry;
+	return err;
 }
 
 static int rpc_pipefs_event(struct notifier_block *nb, unsigned long event,
@@ -160,7 +161,6 @@ static int rpc_pipefs_event(struct notifier_block *nb, unsigned long event,
 	struct super_block *sb = ptr;
 	struct net *net = sb->s_fs_info;
 	struct nfs_net *nn = net_generic(net, nfs_net_id);
-	struct dentry *dentry;
 	int ret = 0;
 
 	if (!try_module_get(THIS_MODULE))
@@ -173,12 +173,7 @@ static int rpc_pipefs_event(struct notifier_block *nb, unsigned long event,
 
 	switch (event) {
 	case RPC_PIPEFS_MOUNT:
-		dentry = nfs4blocklayout_register_sb(sb, nn->bl_device_pipe);
-		if (IS_ERR(dentry)) {
-			ret = PTR_ERR(dentry);
-			break;
-		}
-		nn->bl_device_pipe->dentry = dentry;
+		ret = nfs4blocklayout_register_sb(sb, nn->bl_device_pipe);
 		break;
 	case RPC_PIPEFS_UMOUNT:
 		rpc_unlink(nn->bl_device_pipe);
@@ -195,18 +190,17 @@ static struct notifier_block nfs4blocklayout_block = {
 	.notifier_call = rpc_pipefs_event,
 };
 
-static struct dentry *nfs4blocklayout_register_net(struct net *net,
-						   struct rpc_pipe *pipe)
+static int nfs4blocklayout_register_net(struct net *net, struct rpc_pipe *pipe)
 {
 	struct super_block *pipefs_sb;
-	struct dentry *dentry;
+	int ret;
 
 	pipefs_sb = rpc_get_sb_net(net);
 	if (!pipefs_sb)
-		return NULL;
-	dentry = nfs4blocklayout_register_sb(pipefs_sb, pipe);
+		return 0;
+	ret = nfs4blocklayout_register_sb(pipefs_sb, pipe);
 	rpc_put_sb_net(net);
-	return dentry;
+	return ret;
 }
 
 static void nfs4blocklayout_unregister_net(struct net *net,
@@ -224,20 +218,17 @@ static void nfs4blocklayout_unregister_net(struct net *net,
 static int nfs4blocklayout_net_init(struct net *net)
 {
 	struct nfs_net *nn = net_generic(net, nfs_net_id);
-	struct dentry *dentry;
+	int err;
 
 	mutex_init(&nn->bl_mutex);
 	init_waitqueue_head(&nn->bl_wq);
 	nn->bl_device_pipe = rpc_mkpipe_data(&bl_upcall_ops, 0);
 	if (IS_ERR(nn->bl_device_pipe))
 		return PTR_ERR(nn->bl_device_pipe);
-	dentry = nfs4blocklayout_register_net(net, nn->bl_device_pipe);
-	if (IS_ERR(dentry)) {
+	err = nfs4blocklayout_register_net(net, nn->bl_device_pipe);
+	if (unlikely(err))
 		rpc_destroy_pipe_data(nn->bl_device_pipe);
-		return PTR_ERR(dentry);
-	}
-	nn->bl_device_pipe->dentry = dentry;
-	return 0;
+	return err;
 }
 
 static void nfs4blocklayout_net_exit(struct net *net)
diff --git a/fs/nfs/nfs4idmap.c b/fs/nfs/nfs4idmap.c
index adc03232b851..00932500fce4 100644
--- a/fs/nfs/nfs4idmap.c
+++ b/fs/nfs/nfs4idmap.c
@@ -432,14 +432,8 @@ static int nfs_idmap_pipe_create(struct dentry *dir,
 		struct rpc_pipe_dir_object *pdo)
 {
 	struct idmap *idmap = pdo->pdo_data;
-	struct rpc_pipe *pipe = idmap->idmap_pipe;
-	struct dentry *dentry;
 
-	dentry = rpc_mkpipe_dentry(dir, "idmap", idmap, pipe);
-	if (IS_ERR(dentry))
-		return PTR_ERR(dentry);
-	pipe->dentry = dentry;
-	return 0;
+	return rpc_mkpipe_dentry(dir, "idmap", idmap, idmap->idmap_pipe);
 }
 
 static const struct rpc_pipe_dir_object_ops nfs_idmap_pipe_dir_object_ops = {
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index bbd29b3b573f..2231192ec33f 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -950,31 +950,32 @@ static const struct rpc_pipe_ops cld_upcall_ops = {
 	.destroy_msg	= cld_pipe_destroy_msg,
 };
 
-static struct dentry *
+static int
 nfsd4_cld_register_sb(struct super_block *sb, struct rpc_pipe *pipe)
 {
-	struct dentry *dir, *dentry;
+	struct dentry *dir;
+	int err;
 
 	dir = rpc_d_lookup_sb(sb, NFSD_PIPE_DIR);
 	if (dir == NULL)
-		return ERR_PTR(-ENOENT);
-	dentry = rpc_mkpipe_dentry(dir, NFSD_CLD_PIPE, NULL, pipe);
+		return -ENOENT;
+	err = rpc_mkpipe_dentry(dir, NFSD_CLD_PIPE, NULL, pipe);
 	dput(dir);
-	return dentry;
+	return err;
 }
 
-static struct dentry *
+static int
 nfsd4_cld_register_net(struct net *net, struct rpc_pipe *pipe)
 {
 	struct super_block *sb;
-	struct dentry *dentry;
+	int err;
 
 	sb = rpc_get_sb_net(net);
 	if (!sb)
-		return NULL;
-	dentry = nfsd4_cld_register_sb(sb, pipe);
+		return 0;
+	err = nfsd4_cld_register_sb(sb, pipe);
 	rpc_put_sb_net(net);
-	return dentry;
+	return err;
 }
 
 static void
@@ -994,7 +995,6 @@ static int
 __nfsd4_init_cld_pipe(struct net *net)
 {
 	int ret;
-	struct dentry *dentry;
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 	struct cld_net *cn;
 
@@ -1015,13 +1015,10 @@ __nfsd4_init_cld_pipe(struct net *net)
 	spin_lock_init(&cn->cn_lock);
 	INIT_LIST_HEAD(&cn->cn_list);
 
-	dentry = nfsd4_cld_register_net(net, cn->cn_pipe);
-	if (IS_ERR(dentry)) {
-		ret = PTR_ERR(dentry);
+	ret = nfsd4_cld_register_net(net, cn->cn_pipe);
+	if (unlikely(ret))
 		goto err_destroy_data;
-	}
 
-	cn->cn_pipe->dentry = dentry;
 #ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
 	cn->cn_has_legacy = false;
 #endif
@@ -2114,7 +2111,6 @@ rpc_pipefs_event(struct notifier_block *nb, unsigned long event, void *ptr)
 	struct net *net = sb->s_fs_info;
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 	struct cld_net *cn = nn->cld_net;
-	struct dentry *dentry;
 	int ret = 0;
 
 	if (!try_module_get(THIS_MODULE))
@@ -2127,12 +2123,7 @@ rpc_pipefs_event(struct notifier_block *nb, unsigned long event, void *ptr)
 
 	switch (event) {
 	case RPC_PIPEFS_MOUNT:
-		dentry = nfsd4_cld_register_sb(sb, cn->cn_pipe);
-		if (IS_ERR(dentry)) {
-			ret = PTR_ERR(dentry);
-			break;
-		}
-		cn->cn_pipe->dentry = dentry;
+		ret = nfsd4_cld_register_sb(sb, cn->cn_pipe);
 		break;
 	case RPC_PIPEFS_UMOUNT:
 		rpc_unlink(cn->cn_pipe);
diff --git a/include/linux/sunrpc/rpc_pipe_fs.h b/include/linux/sunrpc/rpc_pipe_fs.h
index a8c0a500d55c..8cc3a5df9801 100644
--- a/include/linux/sunrpc/rpc_pipe_fs.h
+++ b/include/linux/sunrpc/rpc_pipe_fs.h
@@ -127,7 +127,7 @@ extern void rpc_remove_cache_dir(struct dentry *);
 
 struct rpc_pipe *rpc_mkpipe_data(const struct rpc_pipe_ops *ops, int flags);
 void rpc_destroy_pipe_data(struct rpc_pipe *pipe);
-extern struct dentry *rpc_mkpipe_dentry(struct dentry *, const char *, void *,
+extern int rpc_mkpipe_dentry(struct dentry *, const char *, void *,
 					struct rpc_pipe *);
 extern void rpc_unlink(struct rpc_pipe *);
 extern int register_rpc_pipefs(void);
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index f2a44d589cfb..6c23d46a1dcc 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -895,13 +895,8 @@ static int gss_pipe_dentry_create(struct dentry *dir,
 		struct rpc_pipe_dir_object *pdo)
 {
 	struct gss_pipe *p = pdo->pdo_data;
-	struct dentry *dentry;
 
-	dentry = rpc_mkpipe_dentry(dir, p->name, p->clnt, p->pipe);
-	if (IS_ERR(dentry))
-		return PTR_ERR(dentry);
-	p->pipe->dentry = dentry;
-	return 0;
+	return rpc_mkpipe_dentry(dir, p->name, p->clnt, p->pipe);
 }
 
 static const struct rpc_pipe_dir_object_ops gss_pipe_dir_object_ops = {
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 2046582c4f35..dac1c35a642f 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -702,7 +702,7 @@ static struct dentry *rpc_mkdir_populate(struct dentry *parent,
  * The @private argument passed here will be available to all these methods
  * from the file pointer, via RPC_I(file_inode(file))->private.
  */
-struct dentry *rpc_mkpipe_dentry(struct dentry *parent, const char *name,
+int rpc_mkpipe_dentry(struct dentry *parent, const char *name,
 				 void *private, struct rpc_pipe *pipe)
 {
 	struct dentry *dentry;
@@ -717,21 +717,19 @@ struct dentry *rpc_mkpipe_dentry(struct dentry *parent, const char *name,
 
 	inode_lock_nested(dir, I_MUTEX_PARENT);
 	dentry = __rpc_lookup_create_exclusive(parent, name);
-	if (IS_ERR(dentry))
-		goto out;
+	if (IS_ERR(dentry)) {
+		inode_unlock(dir);
+		return PTR_ERR(dentry);
+	}
 	err = __rpc_mkpipe_dentry(dir, dentry, umode, &rpc_pipe_fops,
 				  private, pipe);
-	if (err)
-		goto out_err;
-out:
+	if (unlikely(err))
+		pr_warn("%s() failed to create pipe %pd/%s (errno = %d)\n",
+			__func__, parent, name, err);
+	else
+		pipe->dentry = dentry;
 	inode_unlock(dir);
-	return dentry;
-out_err:
-	dentry = ERR_PTR(err);
-	printk(KERN_WARNING "%s: %s() failed to create pipe %pd/%s (errno = %d)\n",
-			__FILE__, __func__, parent, name,
-			err);
-	goto out;
+	return err;
 }
 EXPORT_SYMBOL_GPL(rpc_mkpipe_dentry);
 
@@ -1185,7 +1183,6 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data)
 	int ret = 0;
 	struct dentry *gssd_dentry;
 	struct dentry *clnt_dentry = NULL;
-	struct dentry *pipe_dentry = NULL;
 
 	/* We should never get this far if "gssd" doesn't exist */
 	gssd_dentry = try_lookup_noperm(&QSTR(files[RPCAUTH_gssd].name), root);
@@ -1209,10 +1206,8 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data)
 		dput(clnt_dentry);
 		return ret;
 	}
-	pipe_dentry = rpc_mkpipe_dentry(clnt_dentry, "gssd", NULL, pipe_data);
+	ret = rpc_mkpipe_dentry(clnt_dentry, "gssd", NULL, pipe_data);
 	dput(clnt_dentry);
-	if (IS_ERR(pipe_dentry))
-		ret = PTR_ERR(pipe_dentry);
 	return ret;
 }
 
-- 
2.39.5

[PATCH 09/17] rpc_pipe: don't overdo directory locking

[Al Viro]

Don't try to hold directories locked more than VFS requires;
lock just before getting a child to be made positive (using
simple_start_creating()) and unlock as soon as the child is
created.  There's no benefit in keeping the parent locked
while populating the child - it won't stop dcache lookups anyway.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/sunrpc/rpc_pipe.c | 44 +++++++++----------------------------------
 1 file changed, 9 insertions(+), 35 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index dac1c35a642f..a61c1173738c 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -594,22 +594,6 @@ static int __rpc_mkpipe_dentry(struct inode *dir, struct dentry *dentry,
 	return 0;
 }
 
-static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
-					  const char *name)
-{
-	struct qstr q = QSTR(name);
-	struct dentry *dentry = try_lookup_noperm(&q, parent);
-	if (!dentry) {
-		dentry = d_alloc(parent, &q);
-		if (!dentry)
-			return ERR_PTR(-ENOMEM);
-	}
-	if (d_really_is_negative(dentry))
-		return dentry;
-	dput(dentry);
-	return ERR_PTR(-EEXIST);
-}
-
 static int rpc_populate(struct dentry *parent,
 			const struct rpc_filelist *files,
 			int start, int eof,
@@ -619,9 +603,8 @@ static int rpc_populate(struct dentry *parent,
 	struct dentry *dentry;
 	int i, err;
 
-	inode_lock(dir);
 	for (i = start; i < eof; i++) {
-		dentry = __rpc_lookup_create_exclusive(parent, files[i].name);
+		dentry = simple_start_creating(parent, files[i].name);
 		err = PTR_ERR(dentry);
 		if (IS_ERR(dentry))
 			goto out_bad;
@@ -633,20 +616,20 @@ static int rpc_populate(struct dentry *parent,
 						files[i].mode,
 						files[i].i_fop,
 						private);
+				inode_unlock(dir);
 				break;
 			case S_IFDIR:
 				err = __rpc_mkdir(dir, dentry,
 						files[i].mode,
 						NULL,
 						private);
+				inode_unlock(dir);
 		}
 		if (err != 0)
 			goto out_bad;
 	}
-	inode_unlock(dir);
 	return 0;
 out_bad:
-	inode_unlock(dir);
 	printk(KERN_WARNING "%s: %s failed to populate directory %pd\n",
 			__FILE__, __func__, parent);
 	return err;
@@ -660,27 +643,21 @@ static struct dentry *rpc_mkdir_populate(struct dentry *parent,
 	struct inode *dir = d_inode(parent);
 	int error;
 
-	inode_lock_nested(dir, I_MUTEX_PARENT);
-	dentry = __rpc_lookup_create_exclusive(parent, name);
+	dentry = simple_start_creating(parent, name);
 	if (IS_ERR(dentry))
-		goto out;
+		return dentry;
 	error = __rpc_mkdir(dir, dentry, mode, NULL, private);
+	inode_unlock(dir);
 	if (error != 0)
-		goto out_err;
+		return ERR_PTR(error);
 	if (populate != NULL) {
 		error = populate(dentry, args_populate);
 		if (error) {
-			inode_unlock(dir);
 			simple_recursive_removal(dentry, NULL);
 			return ERR_PTR(error);
 		}
 	}
-out:
-	inode_unlock(dir);
 	return dentry;
-out_err:
-	dentry = ERR_PTR(error);
-	goto out;
 }
 
 /**
@@ -715,12 +692,9 @@ int rpc_mkpipe_dentry(struct dentry *parent, const char *name,
 	if (pipe->ops->downcall == NULL)
 		umode &= ~0222;
 
-	inode_lock_nested(dir, I_MUTEX_PARENT);
-	dentry = __rpc_lookup_create_exclusive(parent, name);
-	if (IS_ERR(dentry)) {
-		inode_unlock(dir);
+	dentry = simple_start_creating(parent, name);
+	if (IS_ERR(dentry))
 		return PTR_ERR(dentry);
-	}
 	err = __rpc_mkpipe_dentry(dir, dentry, umode, &rpc_pipe_fops,
 				  private, pipe);
 	if (unlikely(err))
-- 
2.39.5

[PATCH 10/17] rpc_pipe: saner primitive for creating subdirectories

[Al Viro]

All users of __rpc_mkdir() have the same form - start_creating(),
followed, in case of success, by __rpc_mkdir() and unlocking parent.

Combine that into a single helper, expanding __rpc_mkdir() into it,
along with the call of __rpc_create_common() in it.

Don't mess with d_drop() + d_add() - just d_instantiate() and be
done with that.  The reason __rpc_create_common() goes for that
dance is that dentry it gets might or might not be hashed; here
we know it's hashed.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/sunrpc/rpc_pipe.c | 67 +++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 28 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index a61c1173738c..c3f269aadcaf 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -524,21 +524,6 @@ static int __rpc_create(struct inode *dir, struct dentry *dentry,
 	return 0;
 }
 
-static int __rpc_mkdir(struct inode *dir, struct dentry *dentry,
-		       umode_t mode,
-		       const struct file_operations *i_fop,
-		       void *private)
-{
-	int err;
-
-	err = __rpc_create_common(dir, dentry, S_IFDIR | mode, i_fop, private);
-	if (err)
-		return err;
-	inc_nlink(dir);
-	fsnotify_mkdir(dir, dentry);
-	return 0;
-}
-
 static void
 init_pipe(struct rpc_pipe *pipe)
 {
@@ -594,6 +579,35 @@ static int __rpc_mkpipe_dentry(struct inode *dir, struct dentry *dentry,
 	return 0;
 }
 
+static struct dentry *rpc_new_dir(struct dentry *parent,
+				  const char *name,
+				  umode_t mode,
+				  void *private)
+{
+	struct dentry *dentry = simple_start_creating(parent, name);
+	struct inode *dir = parent->d_inode;
+	struct inode *inode;
+
+	if (IS_ERR(dentry))
+		return dentry;
+
+	inode = rpc_get_inode(dir->i_sb, S_IFDIR | mode);
+	if (unlikely(!inode)) {
+		dput(dentry);
+		inode_unlock(dir);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	inode->i_ino = iunique(dir->i_sb, 100);
+	rpc_inode_setowner(inode, private);
+	inc_nlink(dir);
+	d_instantiate(dentry, inode);
+	fsnotify_mkdir(dir, dentry);
+	inode_unlock(dir);
+
+	return dentry;
+}
+
 static int rpc_populate(struct dentry *parent,
 			const struct rpc_filelist *files,
 			int start, int eof,
@@ -604,14 +618,14 @@ static int rpc_populate(struct dentry *parent,
 	int i, err;
 
 	for (i = start; i < eof; i++) {
-		dentry = simple_start_creating(parent, files[i].name);
-		err = PTR_ERR(dentry);
-		if (IS_ERR(dentry))
-			goto out_bad;
 		switch (files[i].mode & S_IFMT) {
 			default:
 				BUG();
 			case S_IFREG:
+				dentry = simple_start_creating(parent, files[i].name);
+				err = PTR_ERR(dentry);
+				if (IS_ERR(dentry))
+					goto out_bad;
 				err = __rpc_create(dir, dentry,
 						files[i].mode,
 						files[i].i_fop,
@@ -619,11 +633,13 @@ static int rpc_populate(struct dentry *parent,
 				inode_unlock(dir);
 				break;
 			case S_IFDIR:
-				err = __rpc_mkdir(dir, dentry,
+				dentry = rpc_new_dir(parent,
+						files[i].name,
 						files[i].mode,
-						NULL,
 						private);
-				inode_unlock(dir);
+				err = PTR_ERR(dentry);
+				if (IS_ERR(dentry))
+					goto out_bad;
 		}
 		if (err != 0)
 			goto out_bad;
@@ -640,16 +656,11 @@ static struct dentry *rpc_mkdir_populate(struct dentry *parent,
 		int (*populate)(struct dentry *, void *), void *args_populate)
 {
 	struct dentry *dentry;
-	struct inode *dir = d_inode(parent);
 	int error;
 
-	dentry = simple_start_creating(parent, name);
+	dentry = rpc_new_dir(parent, name, mode, private);
 	if (IS_ERR(dentry))
 		return dentry;
-	error = __rpc_mkdir(dir, dentry, mode, NULL, private);
-	inode_unlock(dir);
-	if (error != 0)
-		return ERR_PTR(error);
 	if (populate != NULL) {
 		error = populate(dentry, args_populate);
 		if (error) {
-- 
2.39.5

[PATCH 11/17] rpc_pipe: saner primitive for creating regular files

[Al Viro]

rpc_new_file(); similar to rpc_new_dir(), except that here we pass
file_operations as well.  Callers don't care about dentry, just
return 0 or -E...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/sunrpc/rpc_pipe.c | 61 +++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 25 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index c3f269aadcaf..8f88c75c9b30 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -510,20 +510,6 @@ static int __rpc_create_common(struct inode *dir, struct dentry *dentry,
 	return -ENOMEM;
 }
 
-static int __rpc_create(struct inode *dir, struct dentry *dentry,
-			umode_t mode,
-			const struct file_operations *i_fop,
-			void *private)
-{
-	int err;
-
-	err = __rpc_create_common(dir, dentry, S_IFREG | mode, i_fop, private);
-	if (err)
-		return err;
-	fsnotify_create(dir, dentry);
-	return 0;
-}
-
 static void
 init_pipe(struct rpc_pipe *pipe)
 {
@@ -579,6 +565,35 @@ static int __rpc_mkpipe_dentry(struct inode *dir, struct dentry *dentry,
 	return 0;
 }
 
+static int rpc_new_file(struct dentry *parent,
+			   const char *name,
+			   umode_t mode,
+			   const struct file_operations *i_fop,
+			   void *private)
+{
+	struct dentry *dentry = simple_start_creating(parent, name);
+	struct inode *dir = parent->d_inode;
+	struct inode *inode;
+
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
+
+	inode = rpc_get_inode(dir->i_sb, S_IFREG | mode);
+	if (unlikely(!inode)) {
+		dput(dentry);
+		inode_unlock(dir);
+		return -ENOMEM;
+	}
+	inode->i_ino = iunique(dir->i_sb, 100);
+	if (i_fop)
+		inode->i_fop = i_fop;
+	rpc_inode_setowner(inode, private);
+	d_instantiate(dentry, inode);
+	fsnotify_create(dir, dentry);
+	inode_unlock(dir);
+	return 0;
+}
+
 static struct dentry *rpc_new_dir(struct dentry *parent,
 				  const char *name,
 				  umode_t mode,
@@ -613,7 +628,6 @@ static int rpc_populate(struct dentry *parent,
 			int start, int eof,
 			void *private)
 {
-	struct inode *dir = d_inode(parent);
 	struct dentry *dentry;
 	int i, err;
 
@@ -622,27 +636,24 @@ static int rpc_populate(struct dentry *parent,
 			default:
 				BUG();
 			case S_IFREG:
-				dentry = simple_start_creating(parent, files[i].name);
-				err = PTR_ERR(dentry);
-				if (IS_ERR(dentry))
-					goto out_bad;
-				err = __rpc_create(dir, dentry,
+				err = rpc_new_file(parent,
+						files[i].name,
 						files[i].mode,
 						files[i].i_fop,
 						private);
-				inode_unlock(dir);
+				if (err)
+					goto out_bad;
 				break;
 			case S_IFDIR:
 				dentry = rpc_new_dir(parent,
 						files[i].name,
 						files[i].mode,
 						private);
-				err = PTR_ERR(dentry);
-				if (IS_ERR(dentry))
+				if (IS_ERR(dentry)) {
+					err = PTR_ERR(dentry);
 					goto out_bad;
+				}
 		}
-		if (err != 0)
-			goto out_bad;
 	}
 	return 0;
 out_bad:
-- 
2.39.5

[PATCH 12/17] rpc_mkpipe_dentry(): switch to start_creating()

[Al Viro]

... and make sure we set the rpc_pipe-private part of inode up before
attaching it to dentry.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/sunrpc/rpc_pipe.c | 83 +++++++++++++++----------------------------
 1 file changed, 29 insertions(+), 54 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 8f88c75c9b30..a52fe3bbf9dc 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -485,31 +485,6 @@ rpc_get_inode(struct super_block *sb, umode_t mode)
 	return inode;
 }
 
-static int __rpc_create_common(struct inode *dir, struct dentry *dentry,
-			       umode_t mode,
-			       const struct file_operations *i_fop,
-			       void *private)
-{
-	struct inode *inode;
-
-	d_drop(dentry);
-	inode = rpc_get_inode(dir->i_sb, mode);
-	if (!inode)
-		goto out_err;
-	inode->i_ino = iunique(dir->i_sb, 100);
-	if (i_fop)
-		inode->i_fop = i_fop;
-	if (private)
-		rpc_inode_setowner(inode, private);
-	d_add(dentry, inode);
-	return 0;
-out_err:
-	printk(KERN_WARNING "%s: %s failed to allocate inode for dentry %pd\n",
-			__FILE__, __func__, dentry);
-	dput(dentry);
-	return -ENOMEM;
-}
-
 static void
 init_pipe(struct rpc_pipe *pipe)
 {
@@ -546,25 +521,6 @@ struct rpc_pipe *rpc_mkpipe_data(const struct rpc_pipe_ops *ops, int flags)
 }
 EXPORT_SYMBOL_GPL(rpc_mkpipe_data);
 
-static int __rpc_mkpipe_dentry(struct inode *dir, struct dentry *dentry,
-			       umode_t mode,
-			       const struct file_operations *i_fop,
-			       void *private,
-			       struct rpc_pipe *pipe)
-{
-	struct rpc_inode *rpci;
-	int err;
-
-	err = __rpc_create_common(dir, dentry, S_IFIFO | mode, i_fop, private);
-	if (err)
-		return err;
-	rpci = RPC_I(d_inode(dentry));
-	rpci->private = private;
-	rpci->pipe = pipe;
-	fsnotify_create(dir, dentry);
-	return 0;
-}
-
 static int rpc_new_file(struct dentry *parent,
 			   const char *name,
 			   umode_t mode,
@@ -704,8 +660,10 @@ static struct dentry *rpc_mkdir_populate(struct dentry *parent,
 int rpc_mkpipe_dentry(struct dentry *parent, const char *name,
 				 void *private, struct rpc_pipe *pipe)
 {
-	struct dentry *dentry;
 	struct inode *dir = d_inode(parent);
+	struct dentry *dentry;
+	struct inode *inode;
+	struct rpc_inode *rpci;
 	umode_t umode = S_IFIFO | 0600;
 	int err;
 
@@ -715,16 +673,33 @@ int rpc_mkpipe_dentry(struct dentry *parent, const char *name,
 		umode &= ~0222;
 
 	dentry = simple_start_creating(parent, name);
-	if (IS_ERR(dentry))
-		return PTR_ERR(dentry);
-	err = __rpc_mkpipe_dentry(dir, dentry, umode, &rpc_pipe_fops,
-				  private, pipe);
-	if (unlikely(err))
-		pr_warn("%s() failed to create pipe %pd/%s (errno = %d)\n",
-			__func__, parent, name, err);
-	else
-		pipe->dentry = dentry;
+	if (IS_ERR(dentry)) {
+		err = PTR_ERR(dentry);
+		goto failed;
+	}
+
+	inode = rpc_get_inode(dir->i_sb, umode);
+	if (unlikely(!inode)) {
+		dput(dentry);
+		inode_unlock(dir);
+		err = -ENOMEM;
+		goto failed;
+	}
+	inode->i_ino = iunique(dir->i_sb, 100);
+	inode->i_fop = &rpc_pipe_fops;
+	rpci = RPC_I(inode);
+	rpci->private = private;
+	rpci->pipe = pipe;
+	rpc_inode_setowner(inode, private);
+	d_instantiate(dentry, inode);
+	pipe->dentry = dentry;
+	fsnotify_create(dir, dentry);
 	inode_unlock(dir);
+	return 0;
+
+failed:
+	pr_warn("%s() failed to create pipe %pd/%s (errno = %d)\n",
+			__func__, parent, name, err);
 	return err;
 }
 EXPORT_SYMBOL_GPL(rpc_mkpipe_dentry);
-- 
2.39.5

[PATCH 13/17] rpc_gssd_dummy_populate(): don't bother with rpc_populate()

[Al Viro]

Just have it create gssd (in root), clntXX in gssd, then info and gssd in clntXX
- all with explicit rpc_new_dir()/rpc_new_file()/rpc_mkpipe_dentry().

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/sunrpc/rpc_pipe.c | 55 +++++++++----------------------------------
 1 file changed, 11 insertions(+), 44 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index a52fe3bbf9dc..9051842228ec 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -997,7 +997,6 @@ enum {
 	RPCAUTH_nfsd4_cb,
 	RPCAUTH_cache,
 	RPCAUTH_nfsd,
-	RPCAUTH_gssd,
 	RPCAUTH_RootEOF
 };
 
@@ -1034,10 +1033,6 @@ static const struct rpc_filelist files[] = {
 		.name = "nfsd",
 		.mode = S_IFDIR | 0555,
 	},
-	[RPCAUTH_gssd] = {
-		.name = "gssd",
-		.mode = S_IFDIR | 0555,
-	},
 };
 
 /*
@@ -1097,13 +1092,6 @@ void rpc_put_sb_net(const struct net *net)
 }
 EXPORT_SYMBOL_GPL(rpc_put_sb_net);
 
-static const struct rpc_filelist gssd_dummy_clnt_dir[] = {
-	[0] = {
-		.name = "clntXX",
-		.mode = S_IFDIR | 0555,
-	},
-};
-
 static ssize_t
 dummy_downcall(struct file *filp, const char __user *src, size_t len)
 {
@@ -1132,14 +1120,6 @@ rpc_dummy_info_show(struct seq_file *m, void *v)
 }
 DEFINE_SHOW_ATTRIBUTE(rpc_dummy_info);
 
-static const struct rpc_filelist gssd_dummy_info_file[] = {
-	[0] = {
-		.name = "info",
-		.i_fop = &rpc_dummy_info_fops,
-		.mode = S_IFREG | 0400,
-	},
-};
-
 /**
  * rpc_gssd_dummy_populate - create a dummy gssd pipe
  * @root:	root of the rpc_pipefs filesystem
@@ -1151,35 +1131,22 @@ static const struct rpc_filelist gssd_dummy_info_file[] = {
 static int
 rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data)
 {
-	int ret = 0;
-	struct dentry *gssd_dentry;
-	struct dentry *clnt_dentry = NULL;
+	struct dentry *gssd_dentry, *clnt_dentry;
+	int err;
 
-	/* We should never get this far if "gssd" doesn't exist */
-	gssd_dentry = try_lookup_noperm(&QSTR(files[RPCAUTH_gssd].name), root);
-	if (!gssd_dentry)
+	gssd_dentry = rpc_new_dir(root, "gssd", 0555, NULL);
+	if (IS_ERR(gssd_dentry))
 		return -ENOENT;
 
-	ret = rpc_populate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1, NULL);
-	if (ret) {
-		dput(gssd_dentry);
-		return ret;
-	}
-
-	clnt_dentry = try_lookup_noperm(&QSTR(gssd_dummy_clnt_dir[0].name),
-					  gssd_dentry);
-	dput(gssd_dentry);
-	if (!clnt_dentry)
+	clnt_dentry = rpc_new_dir(gssd_dentry, "clntXX", 0555, NULL);
+	if (IS_ERR(clnt_dentry))
 		return -ENOENT;
 
-	ret = rpc_populate(clnt_dentry, gssd_dummy_info_file, 0, 1, NULL);
-	if (ret) {
-		dput(clnt_dentry);
-		return ret;
-	}
-	ret = rpc_mkpipe_dentry(clnt_dentry, "gssd", NULL, pipe_data);
-	dput(clnt_dentry);
-	return ret;
+	err = rpc_new_file(clnt_dentry, "info", 0400,
+				   &rpc_dummy_info_fops, NULL);
+	if (!err)
+		err = rpc_mkpipe_dentry(clnt_dentry, "gssd", NULL, pipe_data);
+	return err;
 }
 
 static int
-- 
2.39.5

[PATCH 14/17] rpc_pipe: expand the calls of rpc_mkdir_populate()

[Al Viro]

... and get rid of convoluted callbacks.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/sunrpc/rpc_pipe.c | 63 +++++++++++++++----------------------------
 1 file changed, 22 insertions(+), 41 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 9051842228ec..15ec770bb7fb 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -618,26 +618,6 @@ static int rpc_populate(struct dentry *parent,
 	return err;
 }
 
-static struct dentry *rpc_mkdir_populate(struct dentry *parent,
-		const char *name, umode_t mode, void *private,
-		int (*populate)(struct dentry *, void *), void *args_populate)
-{
-	struct dentry *dentry;
-	int error;
-
-	dentry = rpc_new_dir(parent, name, mode, private);
-	if (IS_ERR(dentry))
-		return dentry;
-	if (populate != NULL) {
-		error = populate(dentry, args_populate);
-		if (error) {
-			simple_recursive_removal(dentry, NULL);
-			return ERR_PTR(error);
-		}
-	}
-	return dentry;
-}
-
 /**
  * rpc_mkpipe_dentry - make an rpc_pipefs file for kernel<->userspace
  *		       communication
@@ -888,13 +868,6 @@ static const struct rpc_filelist authfiles[] = {
 	},
 };
 
-static int rpc_clntdir_populate(struct dentry *dentry, void *private)
-{
-	return rpc_populate(dentry,
-			    authfiles, RPCAUTH_info, RPCAUTH_EOF,
-			    private);
-}
-
 /**
  * rpc_create_client_dir - Create a new rpc_client directory in rpc_pipefs
  * @dentry: the parent of new directory
@@ -911,13 +884,19 @@ struct dentry *rpc_create_client_dir(struct dentry *dentry,
 				   struct rpc_clnt *rpc_client)
 {
 	struct dentry *ret;
+	int error;
 
-	ret = rpc_mkdir_populate(dentry, name, 0555, NULL,
-				 rpc_clntdir_populate, rpc_client);
-	if (!IS_ERR(ret)) {
-		rpc_client->cl_pipedir_objects.pdh_dentry = ret;
-		rpc_create_pipe_dir_objects(&rpc_client->cl_pipedir_objects);
+	ret = rpc_new_dir(dentry, name, 0555, NULL);
+	if (IS_ERR(ret))
+		return ret;
+	error = rpc_populate(ret, authfiles, RPCAUTH_info, RPCAUTH_EOF,
+		    rpc_client);
+	if (unlikely(error)) {
+		simple_recursive_removal(ret, NULL);
+		return ERR_PTR(error);
 	}
+	rpc_client->cl_pipedir_objects.pdh_dentry = ret;
+	rpc_create_pipe_dir_objects(&rpc_client->cl_pipedir_objects);
 	return ret;
 }
 
@@ -955,18 +934,20 @@ static const struct rpc_filelist cache_pipefs_files[3] = {
 	},
 };
 
-static int rpc_cachedir_populate(struct dentry *dentry, void *private)
-{
-	return rpc_populate(dentry,
-			    cache_pipefs_files, 0, 3,
-			    private);
-}
-
 struct dentry *rpc_create_cache_dir(struct dentry *parent, const char *name,
 				    umode_t umode, struct cache_detail *cd)
 {
-	return rpc_mkdir_populate(parent, name, umode, NULL,
-			rpc_cachedir_populate, cd);
+	struct dentry *dentry;
+
+	dentry = rpc_new_dir(parent, name, umode, NULL);
+	if (!IS_ERR(dentry)) {
+		int error = rpc_populate(dentry, cache_pipefs_files, 0, 3, cd);
+		if (error) {
+			simple_recursive_removal(dentry, NULL);
+			return ERR_PTR(error);
+		}
+	}
+	return dentry;
 }
 
 void rpc_remove_cache_dir(struct dentry *dentry)
-- 
2.39.5

[PATCH 15/17] rpc_new_dir(): the last argument is always NULL

[Al Viro]

All callers except the one in rpc_populate() pass explicit NULL
there; rpc_populate() passes its last argument ('private') instead,
but in the only call of rpc_populate() that creates any subdirectories
(when creating fixed subdirectories of root) private itself is NULL.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/sunrpc/rpc_pipe.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 15ec770bb7fb..c14425d2d0d3 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -552,8 +552,7 @@ static int rpc_new_file(struct dentry *parent,
 
 static struct dentry *rpc_new_dir(struct dentry *parent,
 				  const char *name,
-				  umode_t mode,
-				  void *private)
+				  umode_t mode)
 {
 	struct dentry *dentry = simple_start_creating(parent, name);
 	struct inode *dir = parent->d_inode;
@@ -570,7 +569,6 @@ static struct dentry *rpc_new_dir(struct dentry *parent,
 	}
 
 	inode->i_ino = iunique(dir->i_sb, 100);
-	rpc_inode_setowner(inode, private);
 	inc_nlink(dir);
 	d_instantiate(dentry, inode);
 	fsnotify_mkdir(dir, dentry);
@@ -603,8 +601,7 @@ static int rpc_populate(struct dentry *parent,
 			case S_IFDIR:
 				dentry = rpc_new_dir(parent,
 						files[i].name,
-						files[i].mode,
-						private);
+						files[i].mode);
 				if (IS_ERR(dentry)) {
 					err = PTR_ERR(dentry);
 					goto out_bad;
@@ -886,7 +883,7 @@ struct dentry *rpc_create_client_dir(struct dentry *dentry,
 	struct dentry *ret;
 	int error;
 
-	ret = rpc_new_dir(dentry, name, 0555, NULL);
+	ret = rpc_new_dir(dentry, name, 0555);
 	if (IS_ERR(ret))
 		return ret;
 	error = rpc_populate(ret, authfiles, RPCAUTH_info, RPCAUTH_EOF,
@@ -939,7 +936,7 @@ struct dentry *rpc_create_cache_dir(struct dentry *parent, const char *name,
 {
 	struct dentry *dentry;
 
-	dentry = rpc_new_dir(parent, name, umode, NULL);
+	dentry = rpc_new_dir(parent, name, umode);
 	if (!IS_ERR(dentry)) {
 		int error = rpc_populate(dentry, cache_pipefs_files, 0, 3, cd);
 		if (error) {
@@ -1115,11 +1112,11 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data)
 	struct dentry *gssd_dentry, *clnt_dentry;
 	int err;
 
-	gssd_dentry = rpc_new_dir(root, "gssd", 0555, NULL);
+	gssd_dentry = rpc_new_dir(root, "gssd", 0555);
 	if (IS_ERR(gssd_dentry))
 		return -ENOENT;
 
-	clnt_dentry = rpc_new_dir(gssd_dentry, "clntXX", 0555, NULL);
+	clnt_dentry = rpc_new_dir(gssd_dentry, "clntXX", 0555);
 	if (IS_ERR(clnt_dentry))
 		return -ENOENT;
 
-- 
2.39.5

[PATCH 16/17] rpc_create_client_dir(): don't bother with rpc_populate()

[Al Viro]

not for a single file...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/sunrpc/rpc_pipe.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index c14425d2d0d3..e4b53530eb1b 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -852,19 +852,6 @@ rpc_destroy_pipe_dir_objects(struct rpc_pipe_dir_head *pdh)
 		pdo->pdo_ops->destroy(dir, pdo);
 }
 
-enum {
-	RPCAUTH_info,
-	RPCAUTH_EOF
-};
-
-static const struct rpc_filelist authfiles[] = {
-	[RPCAUTH_info] = {
-		.name = "info",
-		.i_fop = &rpc_info_operations,
-		.mode = S_IFREG | 0400,
-	},
-};
-
 /**
  * rpc_create_client_dir - Create a new rpc_client directory in rpc_pipefs
  * @dentry: the parent of new directory
@@ -881,16 +868,18 @@ struct dentry *rpc_create_client_dir(struct dentry *dentry,
 				   struct rpc_clnt *rpc_client)
 {
 	struct dentry *ret;
-	int error;
+	int err;
 
 	ret = rpc_new_dir(dentry, name, 0555);
 	if (IS_ERR(ret))
 		return ret;
-	error = rpc_populate(ret, authfiles, RPCAUTH_info, RPCAUTH_EOF,
-		    rpc_client);
-	if (unlikely(error)) {
+	err = rpc_new_file(ret, "info", S_IFREG | 0400,
+			      &rpc_info_operations, rpc_client);
+	if (err) {
+		pr_warn("%s failed to populate directory %pd\n",
+				__func__, ret);
 		simple_recursive_removal(ret, NULL);
-		return ERR_PTR(error);
+		return ERR_PTR(err);
 	}
 	rpc_client->cl_pipedir_objects.pdh_dentry = ret;
 	rpc_create_pipe_dir_objects(&rpc_client->cl_pipedir_objects);
-- 
2.39.5

[PATCH 17/17] rpc_create_client_dir(): return 0 or -E...

[Al Viro]

Callers couldn't care less which dentry did we get - anything
valid is treated as success.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 include/linux/sunrpc/rpc_pipe_fs.h |  2 +-
 net/sunrpc/clnt.c                  | 36 ++++++++++++------------------
 net/sunrpc/rpc_pipe.c              | 12 +++++-----
 3 files changed, 21 insertions(+), 29 deletions(-)

diff --git a/include/linux/sunrpc/rpc_pipe_fs.h b/include/linux/sunrpc/rpc_pipe_fs.h
index 8cc3a5df9801..2cb406f8ff4e 100644
--- a/include/linux/sunrpc/rpc_pipe_fs.h
+++ b/include/linux/sunrpc/rpc_pipe_fs.h
@@ -98,7 +98,7 @@ static inline bool rpc_msg_is_inflight(const struct rpc_pipe_msg *msg) {
 }
 
 struct rpc_clnt;
-extern struct dentry *rpc_create_client_dir(struct dentry *, const char *, struct rpc_clnt *);
+extern int rpc_create_client_dir(struct dentry *, const char *, struct rpc_clnt *);
 extern int rpc_remove_client_dir(struct rpc_clnt *);
 
 extern void rpc_init_pipe_dir_head(struct rpc_pipe_dir_head *pdh);
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 21426c3049d3..8ca354ecfd02 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -112,47 +112,46 @@ static void rpc_clnt_remove_pipedir(struct rpc_clnt *clnt)
 	}
 }
 
-static struct dentry *rpc_setup_pipedir_sb(struct super_block *sb,
+static int rpc_setup_pipedir_sb(struct super_block *sb,
 				    struct rpc_clnt *clnt)
 {
 	static uint32_t clntid;
 	const char *dir_name = clnt->cl_program->pipe_dir_name;
 	char name[15];
-	struct dentry *dir, *dentry;
+	struct dentry *dir;
+	int err;
 
 	dir = rpc_d_lookup_sb(sb, dir_name);
 	if (dir == NULL) {
 		pr_info("RPC: pipefs directory doesn't exist: %s\n", dir_name);
-		return dir;
+		return -ENOENT;
 	}
 	for (;;) {
 		snprintf(name, sizeof(name), "clnt%x", (unsigned int)clntid++);
 		name[sizeof(name) - 1] = '\0';
-		dentry = rpc_create_client_dir(dir, name, clnt);
-		if (!IS_ERR(dentry))
+		err = rpc_create_client_dir(dir, name, clnt);
+		if (!err)
 			break;
-		if (dentry == ERR_PTR(-EEXIST))
+		if (err == -EEXIST)
 			continue;
 		printk(KERN_INFO "RPC: Couldn't create pipefs entry"
-				" %s/%s, error %ld\n",
-				dir_name, name, PTR_ERR(dentry));
+				" %s/%s, error %d\n",
+				dir_name, name, err);
 		break;
 	}
 	dput(dir);
-	return dentry;
+	return err;
 }
 
 static int
 rpc_setup_pipedir(struct super_block *pipefs_sb, struct rpc_clnt *clnt)
 {
-	struct dentry *dentry;
-
 	clnt->pipefs_sb = pipefs_sb;
 
 	if (clnt->cl_program->pipe_dir_name != NULL) {
-		dentry = rpc_setup_pipedir_sb(pipefs_sb, clnt);
-		if (IS_ERR(dentry))
-			return PTR_ERR(dentry);
+		int err = rpc_setup_pipedir_sb(pipefs_sb, clnt);
+		if (err && err != -ENOENT)
+			return err;
 	}
 	return 0;
 }
@@ -180,16 +179,9 @@ static int rpc_clnt_skip_event(struct rpc_clnt *clnt, unsigned long event)
 static int __rpc_clnt_handle_event(struct rpc_clnt *clnt, unsigned long event,
 				   struct super_block *sb)
 {
-	struct dentry *dentry;
-
 	switch (event) {
 	case RPC_PIPEFS_MOUNT:
-		dentry = rpc_setup_pipedir_sb(sb, clnt);
-		if (!dentry)
-			return -ENOENT;
-		if (IS_ERR(dentry))
-			return PTR_ERR(dentry);
-		break;
+		return rpc_setup_pipedir_sb(sb, clnt);
 	case RPC_PIPEFS_UMOUNT:
 		__rpc_clnt_remove_pipedir(clnt);
 		break;
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index e4b53530eb1b..a12ec709c445 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -863,27 +863,27 @@ rpc_destroy_pipe_dir_objects(struct rpc_pipe_dir_head *pdh)
  * information about the client, together with any "pipes" that may
  * later be created using rpc_mkpipe().
  */
-struct dentry *rpc_create_client_dir(struct dentry *dentry,
-				   const char *name,
-				   struct rpc_clnt *rpc_client)
+int rpc_create_client_dir(struct dentry *dentry,
+			   const char *name,
+			   struct rpc_clnt *rpc_client)
 {
 	struct dentry *ret;
 	int err;
 
 	ret = rpc_new_dir(dentry, name, 0555);
 	if (IS_ERR(ret))
-		return ret;
+		return PTR_ERR(ret);
 	err = rpc_new_file(ret, "info", S_IFREG | 0400,
 			      &rpc_info_operations, rpc_client);
 	if (err) {
 		pr_warn("%s failed to populate directory %pd\n",
 				__func__, ret);
 		simple_recursive_removal(ret, NULL);
-		return ERR_PTR(err);
+		return err;
 	}
 	rpc_client->cl_pipedir_objects.pdh_dentry = ret;
 	rpc_create_pipe_dir_objects(&rpc_client->cl_pipedir_objects);
-	return ret;
+	return 0;
 }
 
 /**
-- 
2.39.5

Re: [PATCH 01/17] simple_recursive_removal(): saner interaction with fsnotify

[Amir Goldstein]

On Fri, Jun 13, 2025 at 9:35 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Make it match the real unlink(2)/rmdir(2) - notify *after* the
> operation.  And use fsnotify_delete() instead of messing with
> fsnotify_unlink()/fsnotify_rmdir().
>
> Currently the only caller that cares is the one in debugfs, and
> there the order matching the normal syscalls makes more sense;
> it'll get more serious for users introduced later in the series.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Makes sense.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/libfs.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 9ea0ecc325a8..42e226af6095 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -628,12 +628,9 @@ void simple_recursive_removal(struct dentry *dentry,
>                         inode_lock(inode);
>                         if (simple_positive(victim)) {
>                                 d_invalidate(victim);   // avoid lost mounts
> -                               if (d_is_dir(victim))
> -                                       fsnotify_rmdir(inode, victim);
> -                               else
> -                                       fsnotify_unlink(inode, victim);
>                                 if (callback)
>                                         callback(victim);
> +                               fsnotify_delete(inode, d_inode(victim), victim);
>                                 dput(victim);           // unpin it
>                         }
>                         if (victim == dentry) {
> --
> 2.39.5
>
>

Re: [PATCH 02/17] new helper: simple_start_creating()

[Jeff Layton]

On Fri, 2025-06-13 at 08:34 +0100, Al Viro wrote:
> Set the things up for kernel-initiated creation of object in
> a tree-in-dcache filesystem.  With respect to locking it's
> an equivalent of filename_create() - we either get a negative
> dentry with locked parent, or ERR_PTR() and no locks taken.
> 
> tracefs and debugfs had that open-coded as part of their
> object creation machinery; switched to calling new helper.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/debugfs/inode.c | 21 ++-------------------
>  fs/libfs.c         | 25 +++++++++++++++++++++++++
>  fs/tracefs/inode.c | 15 ++-------------
>  include/linux/fs.h |  1 +
>  4 files changed, 30 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 30c4944e1862..08638e39bd12 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -384,26 +384,9 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
>  	if (!parent)
>  		parent = debugfs_mount->mnt_root;
>  
> -	inode_lock(d_inode(parent));
> -	if (unlikely(IS_DEADDIR(d_inode(parent))))
> -		dentry = ERR_PTR(-ENOENT);
> -	else
> -		dentry = lookup_noperm(&QSTR(name), parent);
> -	if (!IS_ERR(dentry) && d_really_is_positive(dentry)) {
> -		if (d_is_dir(dentry))
> -			pr_err("Directory '%s' with parent '%s' already present!\n",
> -			       name, parent->d_name.name);
> -		else
> -			pr_err("File '%s' in directory '%s' already present!\n",
> -			       name, parent->d_name.name);

Any chance we could keep a pr_err() for this case? I was doing some
debugfs work recently, and found it helpful.

> -		dput(dentry);
> -		dentry = ERR_PTR(-EEXIST);
> -	}
> -
> -	if (IS_ERR(dentry)) {
> -		inode_unlock(d_inode(parent));
> +	dentry = simple_start_creating(parent, name);
> +	if (IS_ERR(dentry))
>  		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
> -	}
>  
>  	return dentry;
>  }
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 42e226af6095..833ad5ed10f5 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -2260,3 +2260,28 @@ void stashed_dentry_prune(struct dentry *dentry)
>  	 */
>  	cmpxchg(stashed, dentry, NULL);
>  }
> +
> +/* parent must be held exclusive */
> +struct dentry *simple_start_creating(struct dentry *parent, const char *name)
> +{
> +	struct dentry *dentry;
> +	struct inode *dir = d_inode(parent);
> +
> +	inode_lock(dir);
> +	if (unlikely(IS_DEADDIR(dir))) {
> +		inode_unlock(dir);
> +		return ERR_PTR(-ENOENT);
> +	}
> +	dentry = lookup_noperm(&QSTR(name), parent);
> +	if (IS_ERR(dentry)) {
> +		inode_unlock(dir);
> +		return dentry;
> +	}
> +	if (dentry->d_inode) {
> +		dput(dentry);
> +		inode_unlock(dir);
> +		return ERR_PTR(-EEXIST);
> +	}
> +	return dentry;
> +}
> +EXPORT_SYMBOL(simple_start_creating);
> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> index a3fd3cc591bd..4e5d091e9263 100644
> --- a/fs/tracefs/inode.c
> +++ b/fs/tracefs/inode.c
> @@ -551,20 +551,9 @@ struct dentry *tracefs_start_creating(const char *name, struct dentry *parent)
>  	if (!parent)
>  		parent = tracefs_mount->mnt_root;
>  
> -	inode_lock(d_inode(parent));
> -	if (unlikely(IS_DEADDIR(d_inode(parent))))
> -		dentry = ERR_PTR(-ENOENT);
> -	else
> -		dentry = lookup_noperm(&QSTR(name), parent);
> -	if (!IS_ERR(dentry) && d_inode(dentry)) {
> -		dput(dentry);
> -		dentry = ERR_PTR(-EEXIST);
> -	}
> -
> -	if (IS_ERR(dentry)) {
> -		inode_unlock(d_inode(parent));
> +	dentry = simple_start_creating(parent, name);
> +	if (IS_ERR(dentry))
>  		simple_release_fs(&tracefs_mount, &tracefs_mount_count);
> -	}
>  
>  	return dentry;
>  }
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 96c7925a6551..9f75f8836bbd 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3619,6 +3619,7 @@ extern int simple_fill_super(struct super_block *, unsigned long,
>  			     const struct tree_descr *);
>  extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
>  extern void simple_release_fs(struct vfsmount **mount, int *count);
> +struct dentry *simple_start_creating(struct dentry *, const char *);
>  
>  extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
>  			loff_t *ppos, const void *from, size_t available);

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 12/17] rpc_mkpipe_dentry(): switch to start_creating()

[Jeff Layton]

On Fri, 2025-06-13 at 08:34 +0100, Al Viro wrote:
> ... and make sure we set the rpc_pipe-private part of inode up before
> attaching it to dentry.
> 

"rpc_pipe->private"

nit: subject should say  "...switch to simple_start_creating()".

> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  net/sunrpc/rpc_pipe.c | 83 +++++++++++++++----------------------------
>  1 file changed, 29 insertions(+), 54 deletions(-)
> 
> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index 8f88c75c9b30..a52fe3bbf9dc 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -485,31 +485,6 @@ rpc_get_inode(struct super_block *sb, umode_t mode)
>  	return inode;
>  }
>  
> -static int __rpc_create_common(struct inode *dir, struct dentry *dentry,
> -			       umode_t mode,
> -			       const struct file_operations *i_fop,
> -			       void *private)
> -{
> -	struct inode *inode;
> -
> -	d_drop(dentry);
> -	inode = rpc_get_inode(dir->i_sb, mode);
> -	if (!inode)
> -		goto out_err;
> -	inode->i_ino = iunique(dir->i_sb, 100);
> -	if (i_fop)
> -		inode->i_fop = i_fop;
> -	if (private)
> -		rpc_inode_setowner(inode, private);
> -	d_add(dentry, inode);
> -	return 0;
> -out_err:
> -	printk(KERN_WARNING "%s: %s failed to allocate inode for dentry %pd\n",
> -			__FILE__, __func__, dentry);
> -	dput(dentry);
> -	return -ENOMEM;
> -}
> -
>  static void
>  init_pipe(struct rpc_pipe *pipe)
>  {
> @@ -546,25 +521,6 @@ struct rpc_pipe *rpc_mkpipe_data(const struct rpc_pipe_ops *ops, int flags)
>  }
>  EXPORT_SYMBOL_GPL(rpc_mkpipe_data);
>  
> -static int __rpc_mkpipe_dentry(struct inode *dir, struct dentry *dentry,
> -			       umode_t mode,
> -			       const struct file_operations *i_fop,
> -			       void *private,
> -			       struct rpc_pipe *pipe)
> -{
> -	struct rpc_inode *rpci;
> -	int err;
> -
> -	err = __rpc_create_common(dir, dentry, S_IFIFO | mode, i_fop, private);
> -	if (err)
> -		return err;
> -	rpci = RPC_I(d_inode(dentry));
> -	rpci->private = private;
> -	rpci->pipe = pipe;
> -	fsnotify_create(dir, dentry);
> -	return 0;
> -}
> -
>  static int rpc_new_file(struct dentry *parent,
>  			   const char *name,
>  			   umode_t mode,
> @@ -704,8 +660,10 @@ static struct dentry *rpc_mkdir_populate(struct dentry *parent,
>  int rpc_mkpipe_dentry(struct dentry *parent, const char *name,
>  				 void *private, struct rpc_pipe *pipe)
>  {
> -	struct dentry *dentry;
>  	struct inode *dir = d_inode(parent);
> +	struct dentry *dentry;
> +	struct inode *inode;
> +	struct rpc_inode *rpci;
>  	umode_t umode = S_IFIFO | 0600;
>  	int err;
>  
> @@ -715,16 +673,33 @@ int rpc_mkpipe_dentry(struct dentry *parent, const char *name,
>  		umode &= ~0222;
>  
>  	dentry = simple_start_creating(parent, name);
> -	if (IS_ERR(dentry))
> -		return PTR_ERR(dentry);
> -	err = __rpc_mkpipe_dentry(dir, dentry, umode, &rpc_pipe_fops,
> -				  private, pipe);
> -	if (unlikely(err))
> -		pr_warn("%s() failed to create pipe %pd/%s (errno = %d)\n",
> -			__func__, parent, name, err);
> -	else
> -		pipe->dentry = dentry;
> +	if (IS_ERR(dentry)) {
> +		err = PTR_ERR(dentry);
> +		goto failed;
> +	}
> +
> +	inode = rpc_get_inode(dir->i_sb, umode);
> +	if (unlikely(!inode)) {
> +		dput(dentry);
> +		inode_unlock(dir);
> +		err = -ENOMEM;
> +		goto failed;
> +	}
> +	inode->i_ino = iunique(dir->i_sb, 100);
> +	inode->i_fop = &rpc_pipe_fops;
> +	rpci = RPC_I(inode);
> +	rpci->private = private;
> +	rpci->pipe = pipe;
> +	rpc_inode_setowner(inode, private);
> +	d_instantiate(dentry, inode);
> +	pipe->dentry = dentry;
> +	fsnotify_create(dir, dentry);
>  	inode_unlock(dir);
> +	return 0;
> +
> +failed:
> +	pr_warn("%s() failed to create pipe %pd/%s (errno = %d)\n",
> +			__func__, parent, name, err);
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(rpc_mkpipe_dentry);

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCHES][RFC][CFT] rpc_pipefs cleanups

[Jeff Layton]

On Fri, 2025-06-13 at 08:31 +0100, Al Viro wrote:
> 	Another series pulled out of tree-in-dcache pile; it massages
> rpc_pipefs to use saner primitives.  Lives in
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.rpc_pipe
> 6.16-rc1-based, 17 commits, -314 lines of code...
> 
> Individual patches in followups.
> 
> Folks, please test and review.  In various forms this has sat in my tree
> for more than a year and I'd rather get that logjam dealt with.
> 
> Overview:
> 
> 	Prep/infrastructure (#1 is shared with #work.simple_recursive_removal)
> 
> 1) simple_recursive_removal(): saner interaction with fsnotify
> 	fsnotify events are triggered before the callback, unlike in real
> unlink(2)/rmdir(2) syscalls.  Obviously matters only in case when callback
> is non-empty, which excludes most of the current users in the kernel.
> 
> 2) new helper: simple_start_creating()
> 	Set the things up for kernel-initiated creation of object in a
> tree-in-dcache filesystem.  With respect to locking it's an equivalent of
> filename_create() - we either get a negative dentry with locked parent,
> or ERR_PTR() and no locks taken.
> 	tracefs and debugfs had that open-coded as part of their object
> creation machinery; switched to calling new helper.
> 
>       rpc_pipefs proper:
> 
> 3) rpc_pipe: clean failure exits in fill_super
> 	->kill_sb() will be called immediately after a failure
> return anyway, so we don't need to bother with notifier chain and
> rpc_gssd_dummy_depopulate().  What's more, rpc_gssd_dummy_populate()
> failure exits do not need to bother with __rpc_depopulate() - anything
> added to the tree will be taken out by ->kill_sb().
> 
> 4) rpc_{rmdir_,}depopulate(): use simple_recursive_removal() instead
> 	no need to give an exact list of objects to be remove when it's
> simply every child of the victim directory.
> 
> 5) rpc_unlink(): use simple_recursive_removal()
> 	Previous commit was dealing with directories; this one is for
> named pipes (i.e. the thing rpc_pipefs is used for).  Note that the
> callback of simple_recursive_removal() is called with the parent locked;
> the victim isn't locked by the caller.
> 
> 6) rpc_populate(): lift cleanup into callers
> 	rpc_populate() is called either from fill_super (where we
> don't need to remove any files on failure - rpc_kill_sb() will take
> them all out anyway) or from rpc_mkdir_populate(), where we need to
> remove the directory we'd been trying to populate along with whatever
> we'd put into it before we failed.  Simpler to combine that into
> simple_recursive_removal() there.
> 
> 7) rpc_unlink(): saner calling conventions
> 	* pass it pipe instead of pipe->dentry
> 	* zero pipe->dentry afterwards
> 	* it always returns 0; why bother?
> 	
> 8) rpc_mkpipe_dentry(): saner calling conventions
> 	Instead of returning a dentry or ERR_PTR(-E...), return 0 and
> store dentry into pipe->dentry on success and return -E... on failure.
> Callers are happier that way...
> 
> 9) rpc_pipe: don't overdo directory locking
> 	Don't try to hold directories locked more than VFS requires;
> lock just before getting a child to be made positive (using
> simple_start_creating()) and unlock as soon as the child is created.
> There's no benefit in keeping the parent locked while populating the
> child - it won't stop dcache lookups anyway.
> 
> 10) rpc_pipe: saner primitive for creating subdirectories
> 	All users of __rpc_mkdir() have the same form - start_creating(),
> followed, in case of success, by __rpc_mkdir() and unlocking parent.
> Combine that into a single helper, expanding __rpc_mkdir() into it, along
> with the call of __rpc_create_common() in it.
> 	Don't mess with d_drop() + d_add() - just d_instantiate()
> and be done with that.	The reason __rpc_create_common() goes for that
> dance is that dentry it gets might or might not be hashed; here we know
> it's hashed.
> 
> 11) rpc_pipe: saner primitive for creating regular files
> 	rpc_new_file(); similar to rpc_new_dir(), except that here we
> pass file_operations as well.  Callers don't care about dentry, just
> return 0 or -E...
> 
> 12) rpc_mkpipe_dentry(): switch to start_creating()
> 	... and make sure we set the rpc_pipe-private part of inode up
> before attaching it to dentry.
> 
> 13) rpc_gssd_dummy_populate(): don't bother with rpc_populate()
> 	Just have it create gssd (in root), clntXX in gssd, then info
> and gssd in clntXX - all with explicit
> rpc_new_dir()/rpc_new_file()/rpc_mkpipe_dentry().
> 
> 14) rpc_pipe: expand the calls of rpc_mkdir_populate()
> 	... and get rid of convoluted callbacks.
> 
> 15) rpc_new_dir(): the last argument is always NULL
> 	All callers except the one in rpc_populate() pass explicit NULL
> there; rpc_populate() passes its last argument ('private') instead,
> but in the only call of rpc_populate() that creates any subdirectories
> (when creating fixed subdirectories of root) private itself is NULL.
> 
> 16) rpc_create_client_dir(): don't bother with rpc_populate()
> 	not for a single file...
> 
> 17) rpc_create_client_dir(): return 0 or -E...
> 	Callers couldn't care less which dentry did we get - anything
> valid is treated as success.

Aside from a couple of minor nits, this all looks great, Al.

Pity you don't use git format-patch --cover-letter or we'd have the
aggregate diffstat! I imagine this shrinks the rpc_pipefs code
significantly.

You can add this to the pile.

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

Re: [PATCH 02/17] new helper: simple_start_creating()

[Al Viro]

On Fri, Jun 13, 2025 at 02:31:56PM -0400, Jeff Layton wrote:
> > -	if (unlikely(IS_DEADDIR(d_inode(parent))))
> > -		dentry = ERR_PTR(-ENOENT);
> > -	else
> > -		dentry = lookup_noperm(&QSTR(name), parent);
> > -	if (!IS_ERR(dentry) && d_really_is_positive(dentry)) {
> > -		if (d_is_dir(dentry))
> > -			pr_err("Directory '%s' with parent '%s' already present!\n",
> > -			       name, parent->d_name.name);
> > -		else
> > -			pr_err("File '%s' in directory '%s' already present!\n",
> > -			       name, parent->d_name.name);
> 
> Any chance we could keep a pr_err() for this case? I was doing some
> debugfs work recently, and found it helpful.

Umm...  Not in simple_start_creating(), obviously, but...
Would something like
	dentry = simple_start_creating(parent, name);
        if (IS_ERR(dentry)) {
		if (dentry == ERR_PTR(-EEXIST))
			pr_err("'%s' already exists in '%pd'\n", name, parent);
		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
	}
work for you?

Re: [PATCH 02/17] new helper: simple_start_creating()

[Jeff Layton]

On Fri, 2025-06-13 at 23:36 +0100, Al Viro wrote:
> On Fri, Jun 13, 2025 at 02:31:56PM -0400, Jeff Layton wrote:
> > > -	if (unlikely(IS_DEADDIR(d_inode(parent))))
> > > -		dentry = ERR_PTR(-ENOENT);
> > > -	else
> > > -		dentry = lookup_noperm(&QSTR(name), parent);
> > > -	if (!IS_ERR(dentry) && d_really_is_positive(dentry)) {
> > > -		if (d_is_dir(dentry))
> > > -			pr_err("Directory '%s' with parent '%s' already present!\n",
> > > -			       name, parent->d_name.name);
> > > -		else
> > > -			pr_err("File '%s' in directory '%s' already present!\n",
> > > -			       name, parent->d_name.name);
> > 
> > Any chance we could keep a pr_err() for this case? I was doing some
> > debugfs work recently, and found it helpful.
> 
> Umm...  Not in simple_start_creating(), obviously, but...
> Would something like
> 	dentry = simple_start_creating(parent, name);
>         if (IS_ERR(dentry)) {
> 		if (dentry == ERR_PTR(-EEXIST))
> 			pr_err("'%s' already exists in '%pd'\n", name, parent);
> 		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
> 	}
> work for you?

That's exactly what I was thinking.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 12/17] rpc_mkpipe_dentry(): switch to start_creating()

[Al Viro]

On Fri, Jun 13, 2025 at 03:27:39PM -0400, Jeff Layton wrote:
> On Fri, 2025-06-13 at 08:34 +0100, Al Viro wrote:
> > ... and make sure we set the rpc_pipe-private part of inode up before
> > attaching it to dentry.
> > 
> 
> "rpc_pipe->private"

Nope; fs-private, if anything.  That, or rpc_pipefs-private...

> nit: subject should say  "...switch to simple_start_creating()".

D'oh...