Re: [PATCH 03/10] xfs: block deltas in xfs_trans_unreserve_and_mod_sb must be positive

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

On Fri, Feb 23, 2024 at 08:14:59AM +0100, Christoph Hellwig wrote:
> And to make that more clear, rearrange the code a bit and add asserts
> and a comment.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I /think/ I grok this... :)
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_trans.c | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 12d45e93f07d50..befb508638ca1f 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -594,28 +594,38 @@ xfs_trans_unreserve_and_mod_sb(
>  {
>  	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			blkdelta = tp->t_blk_res;
> +	int64_t			rtxdelta = tp->t_rtx_res;
>  	int64_t			idelta = 0;
>  	int64_t			ifreedelta = 0;
>  	int			error;
>  
> -	/* calculate deltas */
> -	if (tp->t_blk_res > 0)
> -		blkdelta = tp->t_blk_res;
> -	if ((tp->t_fdblocks_delta != 0) &&
> -	    (xfs_has_lazysbcount(mp) ||
> -	     (tp->t_flags & XFS_TRANS_SB_DIRTY)))
> +	/*
> +	 * Calculate the deltas.
> +	 *
> +	 * t_fdblocks_delta and t_frextents_delta can be positive or negative:
> +	 *
> +	 *  - positive values indicate blocks freed in the transaction.
> +	 *  - negative values indicate blocks allocated in the transaction
> +	 *
> +	 * Negative values can only happen if the transaction has a block
> +	 * reservation that covers the allocated block.  The end result is
> +	 * that the calculated delta values must always be positive and we
> +	 * can only put back previous allocated or reserved blocks here.
> +	 */
> +	ASSERT(tp->t_blk_res || tp->t_fdblocks_delta >= 0);
> +	if (xfs_has_lazysbcount(mp) || (tp->t_flags & XFS_TRANS_SB_DIRTY)) {
>  	        blkdelta += tp->t_fdblocks_delta;
> +		ASSERT(blkdelta >= 0);
> +	}
>  
> -	if (tp->t_rtx_res > 0)
> -		rtxdelta = tp->t_rtx_res;
> -	if ((tp->t_frextents_delta != 0) &&
> -	    (tp->t_flags & XFS_TRANS_SB_DIRTY))
> +	ASSERT(tp->t_rtx_res || tp->t_frextents_delta >= 0);
> +	if (tp->t_flags & XFS_TRANS_SB_DIRTY) {
>  		rtxdelta += tp->t_frextents_delta;
> +		ASSERT(rtxdelta >= 0);
> +	}
>  
> -	if (xfs_has_lazysbcount(mp) ||
> -	     (tp->t_flags & XFS_TRANS_SB_DIRTY)) {
> +	if (xfs_has_lazysbcount(mp) || (tp->t_flags & XFS_TRANS_SB_DIRTY)) {
>  		idelta = tp->t_icount_delta;
>  		ifreedelta = tp->t_ifree_delta;
>  	}
> -- 
> 2.39.2
> 
>