On Tue, Feb 20, 2024 at 05:08:58PM +0100, Christoph Hellwig wrote:
> On Tue, Feb 20, 2024 at 08:28:21AM +0100, Christoph Hellwig wrote:
> > 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.
>
> This is what I ended up with:
>
> ---
> From 22cba925f1f94b22cfa6143a814f1d14a3521621 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@xxxxxx>
> Date: Tue, 20 Feb 2024 08:35:27 +0100
> Subject: xfs: block deltas in xfs_trans_unreserve_and_mod_sb must be positive
>
> And to make that more clear, rearrange the code a bit and add asserts
> and a comment.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> 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;
> }
That seems reasonable - at least it documents the expectations.
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
