Re: [PATCH v7 03/14] fs: provide accessors for ->i_state

Mateusz Guzik
2025-10-10T15:51:20Z

On Fri, Oct 10, 2025 at 4:44 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 09-10-25 09:59:17, Mateusz Guzik wrote:
> > +static inline void inode_state_set_raw(struct inode *inode,
> > + enum inode_state_flags_enum flags)
> > +{
> > + WRITE_ONCE(inode->i_state, inode->i_state | flags);
> > +}
>
> I think this shouldn't really exist as it is dangerous to use and if we
> deal with XFS, nobody will actually need this function.
>

That's not strictly true, unless you mean code outside of fs/inode.c

First, something is still needed to clear out the state in
inode_init_always_gfp().

Afterwards there are few spots which further modify it without the
spinlock held (for example see insert_inode_locked4()).

My take on the situation is that the current I_NEW et al handling is
crap and the inode hash api is also crap.

For starters freshly allocated inodes should not be starting with 0,
but with I_NEW.

I can agree after the dust settles there should be no _raw thing for
filesystems to use, but getting there is beyond the scope of this
patchset.

> > +static inline void inode_state_set(struct inode *inode,
> > + enum inode_state_flags_enum flags)
> > +{
> > + lockdep_assert_held(&inode->i_lock);
> > + inode_state_set_raw(inode, flags);
> > +}
> > +
> > +static inline void inode_state_clear_raw(struct inode *inode,
> > + enum inode_state_flags_enum flags)
> > +{
> > + WRITE_ONCE(inode->i_state, inode->i_state & ~flags);
> > +}
>
> Ditto here.
>
> > +static inline void inode_state_clear(struct inode *inode,
> > + enum inode_state_flags_enum flags)
> > +{
> > + lockdep_assert_held(&inode->i_lock);
> > + inode_state_clear_raw(inode, flags);
> > +}
> > +
> > +static inline void inode_state_assign_raw(struct inode *inode,
> > + enum inode_state_flags_enum flags)
> > +{
> > + WRITE_ONCE(inode->i_state, flags);
> > +}
> > +
> > +static inline void inode_state_assign(struct inode *inode,
> > + enum inode_state_flags_enum flags)
> > +{
> > + lockdep_assert_held(&inode->i_lock);
> > + inode_state_assign_raw(inode, flags);
> > +}
> > +
> > +static inline void inode_state_replace_raw(struct inode *inode,
> > + enum inode_state_flags_enum clearflags,
> > + enum inode_state_flags_enum setflags)
> > +{
> > + enum inode_state_flags_enum flags;
> > + flags = inode->i_state;
> > + flags &= ~clearflags;
> > + flags |= setflags;
> > + inode_state_assign_raw(inode, flags);
> > +}
>
> Nobody needs this so I'd just provide inode_state_replace().
>

The unused _raw variants are provided for consistency for the time
being. I do expect some of them to die later.