Re: [PATCH 1/2] xfs: hoist refcount record merge predicates

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

On Wed, Nov 30, 2022 at 09:35:31AM +1100, Dave Chinner wrote:
> On Tue, Nov 29, 2022 at 02:01:31PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > 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>
> > ---
> >  fs/xfs/libxfs/xfs_refcount.c |  126 +++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 110 insertions(+), 16 deletions(-)
> 
> Looks OK. Minor nit below.
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> > 
> > 
> > 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.
> 	 */

Done; thanks for the quick review!

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com