Re: [PATCH 3/6] ovl: reflow ovl_create_or_link()

[Amir Goldstein <amir73il@gmail.com>]

On Fri, Nov 14, 2025 at 1:00 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Fri, Nov 14, 2025 at 12:52:58PM +0100, Amir Goldstein wrote:
> > On Fri, Nov 14, 2025 at 11:15 AM Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > Reflow the creation routine in preparation of porting it to a guard.
> > >
> > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > ---
> > >  fs/overlayfs/dir.c | 23 +++++++++++++++--------
> > >  1 file changed, 15 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > > index a276eafb5e78..ff30a91e07f8 100644
> > > --- a/fs/overlayfs/dir.c
> > > +++ b/fs/overlayfs/dir.c
> > > @@ -644,14 +644,23 @@ static const struct cred *ovl_setup_cred_for_create(struct dentry *dentry,
> > >         return override_cred;
> > >  }
> > >
> > > +static int do_ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> > > +                                struct ovl_cattr *attr)
> >
> > Trying to avert the bikesheding over do_ovl_ helper name...
> >
> > > +{
> > > +       if (!ovl_dentry_is_whiteout(dentry))
> > > +               return ovl_create_upper(dentry, inode, attr);
> > > +
> > > +       return ovl_create_over_whiteout(dentry, inode, attr);
> > > +}
> > > +
> > >  static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> > >                               struct ovl_cattr *attr, bool origin)
> > >  {
> > >         int err;
> > > -       const struct cred *new_cred __free(put_cred) = NULL;
> > >         struct dentry *parent = dentry->d_parent;
> > >
> > >         scoped_class(override_creds_ovl, old_cred, dentry->d_sb) {
> > > +               const struct cred *new_cred __free(put_cred) = NULL;
> > >                 /*
> > >                  * When linking a file with copy up origin into a new parent, mark the
> > >                  * new parent dir "impure".
> > > @@ -662,7 +671,6 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> > >                                 return err;
> > >                 }
> > >
> > > -               if (!attr->hardlink) {
> > >                 /*
> > >                  * In the creation cases(create, mkdir, mknod, symlink),
> > >                  * ovl should transfer current's fs{u,g}id to underlying
> > > @@ -676,16 +684,15 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> > >                  * create a new inode, so just use the ovl mounter's
> > >                  * fs{u,g}id.
> > >                  */
> > > +
> > > +               if (attr->hardlink)
> > > +                       return do_ovl_create_or_link(dentry, inode, attr);
> > > +
> >
> > ^^^ This looks like an optimization (don't setup cred for hardlink).
> > Is it really an important optimization that is worth complicating the code flow?
>
> It elides a bunch of allocations and an rcu cycle from put_cred().
> So yes, I think it's worth it.

I have no doubt that ovl_setup_cred_for_create() has a price.
The question is whether hardlinking over ovl is an interesting use case
to optimize for.

Miklos? WDYT?

> You can always remove the special-case later yourself.

Sure. Just as we touch and improve the code, it's worth asking those
questions.

Thanks,
Amir.