On Sat, Sep 6, 2025 at 11:30 AM NeilBrown <neilb@xxxxxxxxxxx> wrote:
>
> 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?
>
void done_path_create(struct path *path, struct dentry *dentry)
{
if (!IS_ERR(dentry))
dput(dentry);
inode_unlock(path->dentry->d_inode);
mnt_drop_write(path->mnt);
path_put(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);
}
}
They look pretty different and the difference does not look like
it is related to differences between creation and removal.
For example, I do not see mnt_want_write() in handle_remove()
so I wonder why that is.
IOW, maybe xxx_path_removing() is like xxx_path_create()
but it does not act this way.
If you could use xxx_path_removing() in do_unlink() do_rmdir()
it would certainly prove your point, but as it is, I don't think you can.
> >
> > 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.
>
>
That makes sense.
> >
> > 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.
>
Makes sense.
Thanks,
Amir.
