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.
