| Message ID | 166975929118.3768925.9568770405264708473.stgit@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: fix broken MAXREFCOUNT handling | expand |
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. */ Cheers, Dave.
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
On 2022/11/30 6:01, 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. Hi Darrick, It's a good refactoring. LGTM. Reviewed-by: Xiao Yang <yangx.jy@fujitsu.com> Best Regards, Xiao Yang
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; + + *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. The funny computation + * of ulen avoids casting. + */ + 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. The funny computation + * of ulen avoids casting. + */ + 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 +966,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 +990,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);