Re: [PATCH 1/6] xfs: check if_bytes under the ilock in xfs_reflink_end_cow_extent

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Mar 28, 2024 at 08:02:51AM +0100, Christoph Hellwig wrote:
> Accessing if_bytes without the ilock is racy.  Move the check a little
> further down into the ilock critical section.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_reflink.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 7da0e8f961d351..df632790a0a51c 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -731,12 +731,6 @@ xfs_reflink_end_cow_extent(
>  	int			nmaps;
>  	int			error;
>  
> -	/* No COW extents?  That's easy! */
> -	if (ifp->if_bytes == 0) {
> -		*offset_fsb = end_fsb;
> -		return 0;
> -	}

This unlocked access was supposed to short-circuit the case where
there's absolutely nothing in the cow fork at all, so that we don't have
to wait for a transaction and the ILOCK.  Is the unlocked access
causing problems?

> -
>  	resblks = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK);
>  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0,
>  			XFS_TRANS_RESERVE, &tp);
> @@ -751,6 +745,12 @@ xfs_reflink_end_cow_extent(
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, ip, 0);
>  
> +	/* No COW extents?  That's easy! */
> +	if (ifp->if_bytes == 0) {
> +		*offset_fsb = end_fsb;
> +		goto out_cancel;
> +	}

This is already taken care of by the clause that comes below
the end of this diff:

	/*
	 * In case of racing, overlapping AIO writes no COW extents might be
	 * left by the time I/O completes for the loser of the race.  In that
	 * case we are done.
	 */
	if (!xfs_iext_lookup_extent(ip, ifp, *offset_fsb, &icur, &got) ||
	    got.br_startoff >= end_fsb) {
		*offset_fsb = end_fsb;
		goto out_cancel;
	}

Since xfs_iext_lookup_extent will return false if the cow fork tree is
empty.

That said, I think the xfs_iext_count_may_overflow stuff is misplaced --
we should be querying the cow fork extent and bouncing out early before
we bother with checking/upgrading the nextents width.  If
xfs_iext_count_upgrade dirtied the transaction, the early bailout will
cause a shutdown.

(The iext upgrade only needs to happen after the bmapi_read.)

--D

> +
>  	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
>  			XFS_IEXT_REFLINK_END_COW_CNT);
>  	if (error == -EFBIG)
> -- 
> 2.39.2
> 
>