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

Jan Kara
2025-10-10T13:51:15Z

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