On Sat, 06 Sep 2025, Amir Goldstein wrote:
> 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...
True - but it appears to be the case.
>
> 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.
As far as I can see the only difference is that one prepares to remove
an object and the other prepares to create an object - as reflected in
the names.
What other difference do you see?
>
> 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?
Making the name more specific discourages misuse. It also highlights
the similarity to kern_path_create (which obviously works because you
noticed it).
This also prepares readers for when they see my next series which adds
start_creating and start_removing (with _noperm and _killable options
and end_foo()) so that they will think "oh, this fits an established
pattern - good".
Note that I chose "end_" for symmetry with end_creating() in debugfs and
tracefs_end_creating() which already exist.
Maybe they should be changed to "done_" but I'm not so keen on done_ as
if there was an error then it wasn't "done". And end_ matches start_
better than done_. They mark the start and end of a code fragment which
tries to create or remove (or rename - I have a selection of
start_renaming options too).
(simple_start_creating() was introduced a few months ago and I'm basing
some of my naming choices on that).
Maybe kern_path_create() should be renamed to start_path_creating().
We don't really need the "kern" - that is the default.
>
> I wonder if using guard semantics could be the better choice if
> looking to clarify the code.
Al suggested in reply to a previous patch that RAII (aka guard etc)
should be added separately so it can be reviewed separately.
Thanks,
NeilBrown
>
> 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
> >
>
