Re: [PATCH 06/10] xfs: cleanup fdblock/frextent accounting in xfs_bmap_del_extent_delay

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

On Fri, Feb 23, 2024 at 08:15:02AM +0100, Christoph Hellwig wrote:
> The code to account fdblocks and frextents in xfs_bmap_del_extent_delay
> is a bit weird in that it accounts frextents before the iext tree
> manipulations and fdblocks after it.  Given that the iext tree
> manipulations can fail currently that's not really a problem, but

                cannot?

If my correction ^^^ is correct, then:
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> still odd.  Move the frextent manipulation to the end, and use a
> fdblocks variable to account of the unconditional indirect blocks and
> the data blocks only freed for !RT.  This prepares for following
> updates in the area and already makes the code more readable.
> 
> Also remove the !isrt assert given that this code clearly handles
> rt extents correctly, and we'll soon reinstate delalloc support for
> RT inodes.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 95e93534cd1264..074d833e845af3 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4833,6 +4833,7 @@ xfs_bmap_del_extent_delay(
>  	xfs_fileoff_t		del_endoff, got_endoff;
>  	xfs_filblks_t		got_indlen, new_indlen, stolen;
>  	uint32_t		state = xfs_bmap_fork_to_state(whichfork);
> +	uint64_t		fdblocks;
>  	int			error = 0;
>  	bool			isrt;
>  
> @@ -4848,15 +4849,11 @@ xfs_bmap_del_extent_delay(
>  	ASSERT(got->br_startoff <= del->br_startoff);
>  	ASSERT(got_endoff >= del_endoff);
>  
> -	if (isrt)
> -		xfs_add_frextents(mp, xfs_rtb_to_rtx(mp, del->br_blockcount));
> -
>  	/*
>  	 * Update the inode delalloc counter now and wait to update the
>  	 * sb counters as we might have to borrow some blocks for the
>  	 * indirect block accounting.
>  	 */
> -	ASSERT(!isrt);
>  	error = xfs_quota_unreserve_blkres(ip, del->br_blockcount);
>  	if (error)
>  		return error;
> @@ -4933,12 +4930,15 @@ xfs_bmap_del_extent_delay(
>  
>  	ASSERT(da_old >= da_new);
>  	da_diff = da_old - da_new;
> -	if (!isrt)
> -		da_diff += del->br_blockcount;
> -	if (da_diff) {
> -		xfs_add_fdblocks(mp, da_diff);
> -		xfs_mod_delalloc(mp, -da_diff);
> -	}
> +	fdblocks = da_diff;
> +
> +	if (isrt)
> +		xfs_add_frextents(mp, xfs_rtb_to_rtx(mp, del->br_blockcount));
> +	else
> +		fdblocks += del->br_blockcount;
> +
> +	xfs_add_fdblocks(mp, fdblocks);
> +	xfs_mod_delalloc(mp, -(int64_t)fdblocks);
>  	return error;
>  }
>  
> -- 
> 2.39.2
> 
>