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

[Jan Kara]

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@xxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								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@xxxxxxxx>
SUSE Labs, CR