Re: [PATCH CANDIDATE v6.1 1/5] xfs: hoist refcount record merge predicates

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

On Fri, Aug 04, 2023 at 01:12:19PM -0400, Theodore Ts'o wrote:
> From: "Darrick J. Wong" <djwong@kernel.org>
> 
> commit 9d720a5a658f5135861773f26e927449bef93d61 upstream.
> 
> 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@kernel.org>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Xiao Yang <yangx.jy@fujitsu.com>

For the whole series,
Acked-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_refcount.c | 129 ++++++++++++++++++++++++++++++-----
>  1 file changed, 113 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> index 3f34bafe18dd..4408893333a6 100644
> --- a/fs/xfs/libxfs/xfs_refcount.c
> +++ b/fs/xfs/libxfs/xfs_refcount.c
> @@ -815,11 +815,119 @@ 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.  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.
> +	 */
> +	ulen += cleft->rc_blockcount + right->rc_blockcount;
> +	if (ulen >= MAXREFCEXTLEN)
> +		return false;
> +
> +	*ulenp = ulen;
> +	return true;
> +}
> +
> +static inline bool
> +xfs_refc_want_merge_left(
> +	const struct xfs_refcount_irec	*left,
> +	const struct xfs_refcount_irec	*cleft,
> +	enum xfs_refc_adjust_op		adjust)
> +{
> +	unsigned long long		ulen = left->rc_blockcount;
> +
> +	/*
> +	 * For a left merge, the left shoulder record must be adjacent to the
> +	 * start of the range.  If this is true, find_left made left and cleft
> +	 * contain valid contents.
> +	 */
> +	if (!xfs_refc_valid(left) || !xfs_refc_valid(cleft))
> +		return false;
> +
> +	/* Left shoulder record refcount must match the new refcount. */
> +	if (left->rc_refcount != cleft->rc_refcount + adjust)
> +		return false;
> +
> +	/*
> +	 * 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.
> +	 */
> +	ulen += cleft->rc_blockcount;
> +	if (ulen >= MAXREFCEXTLEN)
> +		return false;
> +
> +	return true;
> +}
> +
> +static inline bool
> +xfs_refc_want_merge_right(
> +	const struct xfs_refcount_irec	*cright,
> +	const struct xfs_refcount_irec	*right,
> +	enum xfs_refc_adjust_op		adjust)
> +{
> +	unsigned long long		ulen = right->rc_blockcount;
> +
> +	/*
> +	 * For a right merge, the right shoulder record must be adjacent to the
> +	 * end of the range.  If this is true, find_right made cright and right
> +	 * contain valid contents.
> +	 */
> +	if (!xfs_refc_valid(right) || !xfs_refc_valid(cright))
> +		return false;
> +
> +	/* Right shoulder record refcount must match the new refcount. */
> +	if (right->rc_refcount != cright->rc_refcount + adjust)
> +		return false;
> +
> +	/*
> +	 * 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.
> +	 */
> +	ulen += cright->rc_blockcount;
> +	if (ulen >= MAXREFCEXTLEN)
> +		return false;
> +
> +	return true;
> +}
> +
>  /*
>   * Try to merge with any extents on the boundaries of the adjustment range.
>   */
> @@ -861,23 +969,15 @@ xfs_refcount_merge_extents(
>  		 (cleft.rc_blockcount == cright.rc_blockcount);
>  
>  	/* Try to merge left, cleft, and right.  cleft must == cright. */
> -	ulen = (unsigned long long)left.rc_blockcount + cleft.rc_blockcount +
> -			right.rc_blockcount;
> -	if (xfs_refc_valid(&left) && xfs_refc_valid(&right) &&
> -	    xfs_refc_valid(&cleft) && xfs_refc_valid(&cright) && cequal &&
> -	    left.rc_refcount == cleft.rc_refcount + adjust &&
> -	    right.rc_refcount == cleft.rc_refcount + adjust &&
> -	    ulen < MAXREFCEXTLEN) {
> +	if (xfs_refc_want_merge_center(&left, &cleft, &cright, &right, cequal,
> +				adjust, &ulen)) {
>  		*shape_changed = true;
>  		return xfs_refcount_merge_center_extents(cur, &left, &cleft,
>  				&right, ulen, aglen);
>  	}
>  
>  	/* Try to merge left and cleft. */
> -	ulen = (unsigned long long)left.rc_blockcount + cleft.rc_blockcount;
> -	if (xfs_refc_valid(&left) && xfs_refc_valid(&cleft) &&
> -	    left.rc_refcount == cleft.rc_refcount + adjust &&
> -	    ulen < MAXREFCEXTLEN) {
> +	if (xfs_refc_want_merge_left(&left, &cleft, adjust)) {
>  		*shape_changed = true;
>  		error = xfs_refcount_merge_left_extent(cur, &left, &cleft,
>  				agbno, aglen);
> @@ -893,10 +993,7 @@ xfs_refcount_merge_extents(
>  	}
>  
>  	/* Try to merge cright and right. */
> -	ulen = (unsigned long long)right.rc_blockcount + cright.rc_blockcount;
> -	if (xfs_refc_valid(&right) && xfs_refc_valid(&cright) &&
> -	    right.rc_refcount == cright.rc_refcount + adjust &&
> -	    ulen < MAXREFCEXTLEN) {
> +	if (xfs_refc_want_merge_right(&cright, &right, adjust)) {
>  		*shape_changed = true;
>  		return xfs_refcount_merge_right_extent(cur, &right, &cright,
>  				aglen);
> -- 
> 2.31.0
>