On Sat, Sep 6, 2025 at 7:01 AM NeilBrown <neilb@xxxxxxxxxxx> wrote:
>
> From: NeilBrown <neil@xxxxxxxxxx>
>
> Also rename user_path_locked_at() to user_path_removing_at()
>
> Add done_path_removing() to clean up after these calls.
>
> The only credible need for a locked positive dentry is to remove it, so
> make that explicit in the name.
That's a pretty bold statement...
I generally like the done_ abstraction that could be also used as a guard
cleanup helper.
The problem I have with this is that {kern,done}_path_removing rhymes with
{kern,done}_path_create, while in fact they are very different.
What is the motivation for the function rename (you did not specify it)?
Is it just because done_path_locked() sounds weird or something else?
I wonder if using guard semantics could be the better choice if
looking to clarify the code.
Thanks,
Amir.
>
> Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> ---
> Documentation/filesystems/porting.rst | 10 ++++++++++
> drivers/base/devtmpfs.c | 12 ++++--------
> fs/bcachefs/fs-ioctl.c | 6 ++----
> fs/namei.c | 23 +++++++++++++++++------
> include/linux/namei.h | 5 +++--
> 5 files changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
> index 85f590254f07..defbae457310 100644
> --- a/Documentation/filesystems/porting.rst
> +++ b/Documentation/filesystems/porting.rst
> @@ -1285,3 +1285,13 @@ rather than a VMA, as the VMA at this stage is not yet valid.
> The vm_area_desc provides the minimum required information for a filesystem
> to initialise state upon memory mapping of a file-backed region, and output
> parameters for the file system to set this state.
> +
> +---
> +
> +**mandatory**
> +
> +kern_path_locked and user_path_locked_at() are renamed to
> +kern_path_removing() and user_path_removing_at() and should only
> +be used when removing a name. done_path_removing() should be called
> +after removal.
> +
> diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
> index 31bfb3194b4c..26d0beead1f0 100644
> --- a/drivers/base/devtmpfs.c
> +++ b/drivers/base/devtmpfs.c
> @@ -256,7 +256,7 @@ static int dev_rmdir(const char *name)
> struct dentry *dentry;
> int err;
>
> - dentry = kern_path_locked(name, &parent);
> + dentry = kern_path_removing(name, &parent);
> if (IS_ERR(dentry))
> return PTR_ERR(dentry);
> if (d_inode(dentry)->i_private == &thread)
> @@ -265,9 +265,7 @@ static int dev_rmdir(const char *name)
> else
> err = -EPERM;
>
> - dput(dentry);
> - inode_unlock(d_inode(parent.dentry));
> - path_put(&parent);
> + done_path_removing(dentry, &parent);
> return err;
> }
>
> @@ -325,7 +323,7 @@ static int handle_remove(const char *nodename, struct device *dev)
> int deleted = 0;
> int err = 0;
>
> - dentry = kern_path_locked(nodename, &parent);
> + dentry = kern_path_removing(nodename, &parent);
> if (IS_ERR(dentry))
> return PTR_ERR(dentry);
>
> @@ -349,10 +347,8 @@ static int handle_remove(const char *nodename, struct device *dev)
> if (!err || err == -ENOENT)
> deleted = 1;
> }
> - dput(dentry);
> - inode_unlock(d_inode(parent.dentry));
> + done_path_removing(dentry, &parent);
>
> - path_put(&parent);
> if (deleted && strchr(nodename, '/'))
> delete_path(nodename);
> return err;
> diff --git a/fs/bcachefs/fs-ioctl.c b/fs/bcachefs/fs-ioctl.c
> index 4e72e654da96..9446cefbe249 100644
> --- a/fs/bcachefs/fs-ioctl.c
> +++ b/fs/bcachefs/fs-ioctl.c
> @@ -334,7 +334,7 @@ static long bch2_ioctl_subvolume_destroy(struct bch_fs *c, struct file *filp,
> if (arg.flags)
> return -EINVAL;
>
> - victim = user_path_locked_at(arg.dirfd, name, &path);
> + victim = user_path_removing_at(arg.dirfd, name, &path);
> if (IS_ERR(victim))
> return PTR_ERR(victim);
>
> @@ -351,9 +351,7 @@ static long bch2_ioctl_subvolume_destroy(struct bch_fs *c, struct file *filp,
> d_invalidate(victim);
> }
> err:
> - inode_unlock(dir);
> - dput(victim);
> - path_put(&path);
> + done_path_removing(victim, &path);
> return ret;
> }
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 104015f302a7..c750820b27b9 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2757,7 +2757,8 @@ static int filename_parentat(int dfd, struct filename *name,
> }
>
> /* does lookup, returns the object with parent locked */
> -static struct dentry *__kern_path_locked(int dfd, struct filename *name, struct path *path)
> +static struct dentry *__kern_path_removing(int dfd, struct filename *name,
> + struct path *path)
> {
> struct path parent_path __free(path_put) = {};
> struct dentry *d;
> @@ -2815,24 +2816,34 @@ struct dentry *kern_path_parent(const char *name, struct path *path)
> return d;
> }
>
> -struct dentry *kern_path_locked(const char *name, struct path *path)
> +struct dentry *kern_path_removing(const char *name, struct path *path)
> {
> struct filename *filename = getname_kernel(name);
> - struct dentry *res = __kern_path_locked(AT_FDCWD, filename, path);
> + struct dentry *res = __kern_path_removing(AT_FDCWD, filename, path);
>
> putname(filename);
> return res;
> }
>
> -struct dentry *user_path_locked_at(int dfd, const char __user *name, struct path *path)
> +void done_path_removing(struct dentry *dentry, struct path *path)
> +{
> + if (!IS_ERR(dentry)) {
> + inode_unlock(path->dentry->d_inode);
> + dput(dentry);
> + path_put(path);
> + }
> +}
> +EXPORT_SYMBOL(done_path_removing);
> +
> +struct dentry *user_path_removing_at(int dfd, const char __user *name, struct path *path)
> {
> struct filename *filename = getname(name);
> - struct dentry *res = __kern_path_locked(dfd, filename, path);
> + struct dentry *res = __kern_path_removing(dfd, filename, path);
>
> putname(filename);
> return res;
> }
> -EXPORT_SYMBOL(user_path_locked_at);
> +EXPORT_SYMBOL(user_path_removing_at);
>
> int kern_path(const char *name, unsigned int flags, struct path *path)
> {
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 1d5038c21c20..37568f8055f9 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -62,8 +62,9 @@ struct dentry *kern_path_parent(const char *name, struct path *parent);
> extern struct dentry *kern_path_create(int, const char *, struct path *, unsigned int);
> extern struct dentry *user_path_create(int, const char __user *, struct path *, unsigned int);
> extern void done_path_create(struct path *, struct dentry *);
> -extern struct dentry *kern_path_locked(const char *, struct path *);
> -extern struct dentry *user_path_locked_at(int , const char __user *, struct path *);
> +extern struct dentry *kern_path_removing(const char *, struct path *);
> +extern struct dentry *user_path_removing_at(int , const char __user *, struct path *);
> +void done_path_removing(struct dentry *dentry, struct path *path);
> int vfs_path_parent_lookup(struct filename *filename, unsigned int flags,
> struct path *parent, struct qstr *last, int *type,
> const struct path *root);
> --
> 2.50.0.107.gf914562f5916.dirty
>
