On Tue, Nov 29, 2022 at 02:01:31PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> Hoist these multiline conditionals into separate static inline helpers
> to improve readability and set the stage for corruption fixes that will
> be introduced in the next patch.
>
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_refcount.c | 126 +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 110 insertions(+), 16 deletions(-)
Looks OK. Minor nit below.
Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
>
>
> diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> index 3f34bafe18dd..8c68994d09f3 100644
> --- a/fs/xfs/libxfs/xfs_refcount.c
> +++ b/fs/xfs/libxfs/xfs_refcount.c
> @@ -815,11 +815,116 @@ xfs_refcount_find_right_extents(
> /* Is this extent valid? */
> static inline bool
> xfs_refc_valid(
> - struct xfs_refcount_irec *rc)
> + const struct xfs_refcount_irec *rc)
> {
> return rc->rc_startblock != NULLAGBLOCK;
> }
>
> +static inline bool
> +xfs_refc_want_merge_center(
> + const struct xfs_refcount_irec *left,
> + const struct xfs_refcount_irec *cleft,
> + const struct xfs_refcount_irec *cright,
> + const struct xfs_refcount_irec *right,
> + bool cleft_is_cright,
> + enum xfs_refc_adjust_op adjust,
> + unsigned long long *ulenp)
> +{
> + unsigned long long ulen = left->rc_blockcount;
> +
> + /*
> + * To merge with a center record, both shoulder records must be
> + * adjacent to the record we want to adjust. This is only true if
> + * find_left and find_right made all four records valid.
> + */
> + if (!xfs_refc_valid(left) || !xfs_refc_valid(right) ||
> + !xfs_refc_valid(cleft) || !xfs_refc_valid(cright))
> + return false;
> +
> + /* There must only be one record for the entire range. */
> + if (!cleft_is_cright)
> + return false;
> +
> + /* The shoulder record refcounts must match the new refcount. */
> + if (left->rc_refcount != cleft->rc_refcount + adjust)
> + return false;
> + if (right->rc_refcount != cleft->rc_refcount + adjust)
> + return false;
> +
> + /*
> + * The new record cannot exceed the max length. The funny computation
> + * of ulen avoids casting.
> + */
> + ulen += cleft->rc_blockcount + right->rc_blockcount;
> + if (ulen >= MAXREFCEXTLEN)
> + return false;
The comment took me a bit of spelunking to decipher what the "funny
computation" was. Better to spell it out directly (catch u32
overflows) than just hint that there's somethign special about it.
Say:
/*
* The new record cannot exceed the max length. ulen is a
* ULL as the individual record block counts can be up to
* (u32 - 1) in length hence we need to catch u32 addition
* overflows here.
*/
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
