Open-coded accesses prevent asserting they are done correctly. One
obvious aspect is locking, but significantly more can checked. For
example it can be detected when the code is clearing flags which are
already missing, or is setting flags when it is illegal (e.g., I_FREEING
when ->i_count > 0).
In order to keep things manageable this patchset merely gets the thing
off the ground with only lockdep checks baked in.
Current consumers can be trivially converted.
Suppose flags I_A and I_B are to be handled.
If ->i_lock is held, then:
state = inode->i_state => state = inode_state_read(inode)
inode->i_state |= (I_A | I_B) => inode_state_set(inode, I_A | I_B)
inode->i_state &= ~(I_A | I_B) => inode_state_clear(inode, I_A | I_B)
inode->i_state = I_A | I_B => inode_state_assign(inode, I_A | I_B)
If ->i_lock is not held or only held conditionally:
state = inode->i_state => state = inode_state_read_once(inode)
inode->i_state |= (I_A | I_B) => inode_state_set_raw(inode, I_A | I_B)
inode->i_state &= ~(I_A | I_B) => inode_state_clear_raw(inode, I_A | I_B)
inode->i_state = I_A | I_B => inode_state_assign_raw(inode, I_A | I_B)
The "_once" vs "_raw" discrepancy stems from the read variant differing
by READ_ONCE as opposed to just lockdep checks.
Finally, if you want to atomically clear flags and set new ones, the
following:
state = inode->i_state;
state &= ~I_A;
state |= I_B;
inode->i_state = state;
turns into:
inode_state_replace(inode, I_A, I_B);
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
include/linux/fs.h | 78 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 76 insertions(+), 2 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b35014ba681b..909eb1e68637 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -759,7 +759,7 @@ enum inode_state_bits {
/* reserved wait address bit 3 */
};
-enum inode_state_flags_t {
+enum inode_state_flags_enum {
I_NEW = (1U << __I_NEW),
I_SYNC = (1U << __I_SYNC),
I_LRU_ISOLATING = (1U << __I_LRU_ISOLATING),
@@ -843,7 +843,7 @@ struct inode {
#endif
/* Misc */
- enum inode_state_flags_t i_state;
+ enum inode_state_flags_enum i_state;
/* 32-bit hole */
struct rw_semaphore i_rwsem;
@@ -902,6 +902,80 @@ struct inode {
void *i_private; /* fs or device private pointer */
} __randomize_layout;
+/*
+ * i_state handling
+ *
+ * We hide all of it behind helpers so that we can validate consumers.
+ */
+static inline enum inode_state_flags_enum inode_state_read_once(struct inode *inode)
+{
+ return READ_ONCE(inode->i_state);
+}
+
+static inline enum inode_state_flags_enum inode_state_read(struct inode *inode)
+{
+ lockdep_assert_held(&inode->i_lock);
+ return inode->i_state;
+}
+
+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);
+}
+
+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);
+}
+
+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);
+}
+
+static inline void inode_state_replace(struct inode *inode,
+ enum inode_state_flags_enum clearflags,
+ enum inode_state_flags_enum setflags)
+{
+ lockdep_assert_held(&inode->i_lock);
+ inode_state_replace_raw(inode, clearflags, setflags);
+}
+
static inline void inode_set_cached_link(struct inode *inode, char *link, int linklen)
{
VFS_WARN_ON_INODE(strlen(link) != linklen, inode);
--
2.34.1On 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.
> +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().
> +static inline void inode_state_replace(struct inode *inode,
> + enum inode_state_flags_enum clearflags,
> + enum inode_state_flags_enum setflags)
> +{
> + lockdep_assert_held(&inode->i_lock);
> + inode_state_replace_raw(inode, clearflags, setflags);
> +}
> +
> static inline void inode_set_cached_link(struct inode *inode, char *link, int linklen)
> {
> VFS_WARN_ON_INODE(strlen(link) != linklen, inode);
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
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.
On Fri 10-10-25 17:51:06, Mateusz Guzik wrote:
> 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.
OK, then we are on the same page wrt the final goal. I can bear the raw
variants in the tree for some time if that makes the whole transition
easier.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
On Thu 09-10-25 09:59:17, Mateusz Guzik wrote:
> Open-coded accesses prevent asserting they are done correctly. One
> obvious aspect is locking, but significantly more can checked. For
> example it can be detected when the code is clearing flags which are
> already missing, or is setting flags when it is illegal (e.g., I_FREEING
> when ->i_count > 0).
>
> In order to keep things manageable this patchset merely gets the thing
> off the ground with only lockdep checks baked in.
>
> Current consumers can be trivially converted.
>
> Suppose flags I_A and I_B are to be handled.
>
> If ->i_lock is held, then:
>
> state = inode->i_state => state = inode_state_read(inode)
> inode->i_state |= (I_A | I_B) => inode_state_set(inode, I_A | I_B)
> inode->i_state &= ~(I_A | I_B) => inode_state_clear(inode, I_A | I_B)
> inode->i_state = I_A | I_B => inode_state_assign(inode, I_A | I_B)
>
> If ->i_lock is not held or only held conditionally:
>
> state = inode->i_state => state = inode_state_read_once(inode)
> inode->i_state |= (I_A | I_B) => inode_state_set_raw(inode, I_A | I_B)
> inode->i_state &= ~(I_A | I_B) => inode_state_clear_raw(inode, I_A | I_B)
> inode->i_state = I_A | I_B => inode_state_assign_raw(inode, I_A | I_B)
>
> The "_once" vs "_raw" discrepancy stems from the read variant differing
> by READ_ONCE as opposed to just lockdep checks.
>
> Finally, if you want to atomically clear flags and set new ones, the
> following:
>
> state = inode->i_state;
> state &= ~I_A;
> state |= I_B;
> inode->i_state = state;
>
> turns into:
>
> inode_state_replace(inode, I_A, I_B);
>
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> include/linux/fs.h | 78 ++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 76 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b35014ba681b..909eb1e68637 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -759,7 +759,7 @@ enum inode_state_bits {
> /* reserved wait address bit 3 */
> };
>
> -enum inode_state_flags_t {
> +enum inode_state_flags_enum {
> I_NEW = (1U << __I_NEW),
> I_SYNC = (1U << __I_SYNC),
> I_LRU_ISOLATING = (1U << __I_LRU_ISOLATING),
> @@ -843,7 +843,7 @@ struct inode {
> #endif
>
> /* Misc */
> - enum inode_state_flags_t i_state;
> + enum inode_state_flags_enum i_state;
> /* 32-bit hole */
> struct rw_semaphore i_rwsem;
>
> @@ -902,6 +902,80 @@ struct inode {
> void *i_private; /* fs or device private pointer */
> } __randomize_layout;
>
> +/*
> + * i_state handling
> + *
> + * We hide all of it behind helpers so that we can validate consumers.
> + */
> +static inline enum inode_state_flags_enum inode_state_read_once(struct inode *inode)
> +{
> + return READ_ONCE(inode->i_state);
> +}
> +
> +static inline enum inode_state_flags_enum inode_state_read(struct inode *inode)
> +{
> + lockdep_assert_held(&inode->i_lock);
> + return inode->i_state;
> +}
> +
> +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);
> +}
> +
> +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);
> +}
> +
> +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);
> +}
> +
> +static inline void inode_state_replace(struct inode *inode,
> + enum inode_state_flags_enum clearflags,
> + enum inode_state_flags_enum setflags)
> +{
> + lockdep_assert_held(&inode->i_lock);
> + inode_state_replace_raw(inode, clearflags, setflags);
> +}
> +
> static inline void inode_set_cached_link(struct inode *inode, char *link, int linklen)
> {
> VFS_WARN_ON_INODE(strlen(link) != linklen, inode);
> --
> 2.34.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
© 2016 - 2025 Red Hat, Inc.