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@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.
> */
Done; thanks for the quick review!
--D
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
