Re: [PATCH] fs: support filename refcount without atomics

[Mateusz Guzik]

On Fri, Mar 7, 2025 at 5:26 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Fri, Mar 07, 2025 at 05:11:55PM +0100, Mateusz Guzik wrote:
> > +++ b/include/linux/fs.h
> > @@ -2765,11 +2765,19 @@ struct audit_names;
> >  struct filename {
> >       const char              *name;  /* pointer to actual string */
> >       const __user char       *uptr;  /* original userland pointer */
> > -     atomic_t                refcnt;
> > +     union {
> > +             atomic_t        refcnt_atomic;
> > +             int             refcnt;
> > +     };
> > +#ifdef CONFIG_DEBUG_VFS
> > +     struct task_struct      *owner;
> > +#endif
> > +     bool                    is_atomic;
> >       struct audit_names      *aname;
> >       const char              iname[];
> >  };
>
> 7 (or 3) byte hole; try to pad.
>
> Would it make more sense to put the bool between aname and iname where
> it will only take one byte instead of 8?

On the stock kernel there is already a 4 byte hole between the
refcount and aname, which is where is_atomic lands with debug
disabled. I.e. no size changes in production kernels with and without
the change.

However, now that you mention it the debug owner field is misplaced --
it should have landed *after* is_atomic. Maybe Christian will be happy
to just move it, otherwise I'm going to include this in a v2.

The iname field is expected to be aligned, so I don't believe
shuffling the is_atomic flag helps anyone:
static_assert(offsetof(struct filename, iname) % sizeof(long) == 0);

-- 
Mateusz Guzik <mjguzik gmail.com>