Re: [PATCH] xfs: _{attr,data}_map_shared should take ILOCK_EXCL until iread_extents is completely done

[Christoph Hellwig <hch@infradead.org>]

On Mon, Apr 10, 2023 at 06:06:38PM -0700, Darrick J. Wong wrote:
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1171,6 +1171,8 @@ xfs_iread_extents(
>  		goto out;
>  	}
>  	ASSERT(ir.loaded == xfs_iext_count(ifp));
> +	smp_mb();
> +	ifp->if_needextents = 0;

Shouldn't this be a WRITE_ONCE?

> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 6b21760184d9..eadae924dc42 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -174,6 +174,8 @@ xfs_iformat_btree(
>  	int			level;
>  
>  	ifp = xfs_ifork_ptr(ip, whichfork);
> +	ifp->if_needextents = 1;

Same here.

>  	dfp = (xfs_bmdr_block_t *)XFS_DFORK_PTR(dip, whichfork);
>  	size = XFS_BMAP_BROOT_SPACE(mp, dfp);
>  	nrecs = be16_to_cpu(dfp->bb_numrecs);
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index d3943d6ad0b9..e69c78c35c96 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -24,6 +24,7 @@ struct xfs_ifork {
>  	xfs_extnum_t		if_nextents;	/* # of extents in this fork */
>  	short			if_broot_bytes;	/* bytes allocated for root */
>  	int8_t			if_format;	/* format of this fork */
> +	int8_t			if_needextents;	/* extents have not been read */
>  };

I don't think this should be signed.

>  static inline bool xfs_need_iread_extents(struct xfs_ifork *ifp)
>  {
> -	return ifp->if_format == XFS_DINODE_FMT_BTREE && ifp->if_height == 0;
> +	return ifp->if_format == XFS_DINODE_FMT_BTREE && ifp->if_needextents;

.. and this should use READ_ONCE and drop the if_format check given
that if_needextents is only set in xfs_iformat_btree.