Re: [PATCH 3/9] xfs: split xfs_mod_freecounter

[Dave Chinner <david@fromorbit.com>]

On Mon, Feb 19, 2024 at 07:34:44AM +0100, Christoph Hellwig wrote:
> xfs_mod_freecounter has two entirely separate code paths for adding or
> subtracting from the free counters.  Only the subtract case looks at the
> rsvd flag and can return an error.
> 
> Split xfs_mod_freecounter into separate helpers for subtracting or
> adding the freecounter, and remove all the impossible to reach error
> handling for the addition case.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

.....

> @@ -593,12 +593,10 @@ xfs_trans_unreserve_and_mod_sb(
>  	struct xfs_trans	*tp)
>  {
>  	struct xfs_mount	*mp = tp->t_mountp;
> -	bool			rsvd = (tp->t_flags & XFS_TRANS_RESERVE) != 0;
>  	int64_t			blkdelta = 0;
>  	int64_t			rtxdelta = 0;
>  	int64_t			idelta = 0;
>  	int64_t			ifreedelta = 0;
> -	int			error;
>  
>  	/* calculate deltas */
>  	if (tp->t_blk_res > 0)
> @@ -621,10 +619,8 @@ xfs_trans_unreserve_and_mod_sb(
>  	}
>  
>  	/* apply the per-cpu counters */
> -	if (blkdelta) {
> -		error = xfs_mod_fdblocks(mp, blkdelta, rsvd);
> -		ASSERT(!error);
> -	}
> +	if (blkdelta)
> +		xfs_add_fdblocks(mp, blkdelta);
>  
>  	if (idelta)
>  		percpu_counter_add_batch(&mp->m_icount, idelta,
> @@ -633,10 +629,8 @@ xfs_trans_unreserve_and_mod_sb(
>  	if (ifreedelta)
>  		percpu_counter_add(&mp->m_ifree, ifreedelta);
>  
> -	if (rtxdelta) {
> -		error = xfs_mod_frextents(mp, rtxdelta);
> -		ASSERT(!error);
> -	}
> +	if (rtxdelta)
> +		xfs_add_frextents(mp, rtxdelta);
>  
>  	if (!(tp->t_flags & XFS_TRANS_SB_DIRTY))
>  		return;

I don't think these hunks are correct. blkdelta and rtxdelta can be
negative - they are int64_t, and they are set via
xfs_trans_mod_sb(). e.g. in xfs_ag_resv_alloc_extent() we do:

	case XFS_AG_RESV_NONE:
                field = args->wasdel ? XFS_TRANS_SB_RES_FDBLOCKS :
                                       XFS_TRANS_SB_FDBLOCKS;
                xfs_trans_mod_sb(args->tp, field, -(int64_t)args->len);
                return;
        }

Which passes a negative delta to xfs_trans_mod_sb() and adds it to
tp->t_fdblocks_delta. So that field can hold a negative number, and
now we pass a negative int64_t to xfs_add_fdblocks() as an unsigned
uint64_t.....

While it might kinda work because of implicit overflow behaviour,
it won't account allow for that block usage to correctly account
for the reserve pool usage that it should have accounted for near
ENOSPC....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com