Re: [PATCH v7 13/14] xfs: use the new ->i_state accessors

Jan Kara
2025-10-10T14:41:31Z

On Thu 09-10-25 09:59:27, Mateusz Guzik wrote:
> Change generated with coccinelle and fixed up by hand as appropriate.
>
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>

...

> @@ -2111,7 +2111,7 @@ xfs_rename_alloc_whiteout(
> */
> xfs_setup_iops(tmpfile);
> xfs_finish_inode_setup(tmpfile);
> - VFS_I(tmpfile)->i_state |= I_LINKABLE;
> + inode_state_set_raw(VFS_I(tmpfile), I_LINKABLE);
>
> *wip = tmpfile;
> return 0;
> @@ -2330,7 +2330,7 @@ xfs_rename(
> * flag from the inode so it doesn't accidentally get misused in
> * future.
> */
> - VFS_I(du_wip.ip)->i_state &= ~I_LINKABLE;
> + inode_state_clear_raw(VFS_I(du_wip.ip), I_LINKABLE);
> }
>
> out_commit:

These two accesses look fishy (not your fault but when we are doing this
i_state exercise better make sure all the places are correct before
papering over bugs with _raw function variant). How come they cannot race
with other i_state modifications and thus corrupt i_state?

> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index caff0125faea..ad94fbf55014 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1420,7 +1420,7 @@ xfs_setup_inode(
> bool is_meta = xfs_is_internal_inode(ip);
>
> inode->i_ino = ip->i_ino;
> - inode->i_state |= I_NEW;
> + inode_state_set_raw(inode, I_NEW);
>
> inode_sb_list_add(inode);
> /* make the inode look hashed for the writeback code */

Frankly, the XFS i_state handling is kind of messy and I suspect we should
be getting i_state == 0 here. But we need to confirm with XFS guys. I'm
poking into this because this is actually the only case where we need
inode_state_set_raw() or inode_state_clear_raw() outside of core VFS and
I'd like to get rid of these functions because IMHO they are actively
dangerous to use.

Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR