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

[Christoph Hellwig <hch@lst.de>]

On Tue, Feb 20, 2024 at 10:21:12AM +1100, Dave Chinner wrote:
> 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.....

This area is rather subtle.

For XFS_TRANS_SB_FDBLOCKS, xfs_trans_mod_sb expects enough t_blk_res to
be held to at least balance out the t_fdblocks_delta value, i.e.
xfs_trans_unreserve_and_mod_sb always starts out with a positive value
due to the t_blk_res, and then decrements the actually used block
allocation in t_fdblocks_delta, and then still must end up with 0 or a
positive value, and if a positive value is left it "unreserves" the
reservation per the function name.  Same for the rtextent version.

So we should be fine here, but the code could really use documentation,
a few more asserts and a slightly different structure that makes this
more obvious.  I'll throw in a patch for that.