On Wed, 10 Sep 2025, Amir Goldstein wrote:
> On Tue, Sep 9, 2025 at 6:48 AM NeilBrown <neilb@xxxxxxxxxxx> wrote:
> >
> > From: NeilBrown <neil@xxxxxxxxxx>
> >
> > kern_path_locked() is now only used to prepare for removing an object
> > from the filesystem (and that is the only credible reason for wanting a
> > positive locked dentry). Thus it corresponds to kern_path_create() and
> > so should have a corresponding name.
> >
> > Unfortunately the name "kern_path_create" is somewhat misleading as it
> > doesn't actually create anything. The recently added
> > simple_start_creating() provides a better pattern I believe. The
> > "start" can be matched with "end" to bracket the creating or removing.
> >
> > So this patch changes names:
> >
> > kern_path_locked -> start_removing_path
> > kern_path_create -> start_creating_path
> > user_path_create -> start_creating_user_path
> > user_path_locked_at -> start_removing_user_path_at
> > done_path_create -> end_creating_path
>
> This looks nice.
>
> With one comment below fixed feel free to add:
>
> Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
Thanks.
> > @@ -2770,15 +2771,25 @@ static struct dentry *__kern_path_locked(int dfd, struct filename *name, struct
> > return ERR_PTR(error);
> > if (unlikely(type != LAST_NORM))
> > return ERR_PTR(-EINVAL);
>
> This abnormal error handling pattern deserves a comment:
> /* don't fail immediately if it's r/o, at least try to report other errors */
Just like in start_creating_path(). Yes, that is fair. Done.
Thanks,
NeilBrown
>
> > + error = mnt_want_write(path->mnt);
> > inode_lock_nested(parent_path.dentry->d_inode, I_MUTEX_PARENT);
> > d = lookup_one_qstr_excl(&last, parent_path.dentry, 0);
> > - if (IS_ERR(d)) {
> > - inode_unlock(parent_path.dentry->d_inode);
> > - return d;
> > - }
> > + if (IS_ERR(d))
> > + goto unlock;
> > + if (error)
> > + goto fail;
> > path->dentry = no_free_ptr(parent_path.dentry);
> > path->mnt = no_free_ptr(parent_path.mnt);
> > return d;
> > +
> > +fail:
> > + dput(d);
> > + d = ERR_PTR(error);
> > +unlock:
> > + inode_unlock(parent_path.dentry->d_inode);
> > + if (!error)
> > + mnt_drop_write(path->mnt);
> > + return d;
> > }
