[PATCH 5.10 0/2] Fix xfs/179 for 5.10 stable

[PATCH 5.10 0/2] Fix xfs/179 for 5.10 stable

[Guoqing Jiang]

Hi,

With the two patches applied, xfs/179 can pass in 5.10.188. Otherwise I got

[root@localhost xfstests]# ./check xfs/179
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 localhost 5.10.188-default #14 SMP Thu Aug 3 15:23:19 CST 2023
MKFS_OPTIONS  -- -f /dev/loop1
MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/loop1 /mnt/scratch

xfs/179 1s ... [failed, exit status 1]- output mismatch (see /root/xfstests/results//xfs/179.out.bad)
    --- tests/xfs/179.out	2023-07-13 16:12:27.000000000 +0800
    +++ /root/xfstests/results//xfs/179.out.bad	2023-08-03 16:55:38.173787911 +0800
    @@ -8,3 +8,5 @@
     Check scratch fs
     Remove reflinked files
     Check scratch fs
    +xfs_repair fails
    +(see /root/xfstests/results//xfs/179.full for details)
    ...
    (Run 'diff -u /root/xfstests/tests/xfs/179.out /root/xfstests/results//xfs/179.out.bad'  to see the entire diff)

HINT: You _MAY_ be missing kernel fix:
      b25d1984aa88 xfs: estimate post-merge refcounts correctly

Ran: xfs/179
Failures: xfs/179
Failed 1 of 1 tests

Please review if they are approriate for 5.10 stable.

Thanks,
Guoqing

Darrick J. Wong (2):
  xfs: hoist refcount record merge predicates
  xfs: estimate post-merge refcounts correctly

 fs/xfs/libxfs/xfs_refcount.c | 146 +++++++++++++++++++++++++++++++----
 1 file changed, 130 insertions(+), 16 deletions(-)

-- 
2.33.0

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

[Guoqing Jiang]

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>
Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 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 2076627243b0..9806942a13ea 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -781,11 +781,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.
  */
@@ -827,23 +935,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);
@@ -859,10 +959,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.33.0

[PATCH 5.10 2/2] xfs: estimate post-merge refcounts correctly

[Guoqing Jiang]

From: "Darrick J. Wong" <djwong@kernel.org>

commit b25d1984aa884fc91a73a5a407b9ac976d441e9b upstream.

Upon enabling fsdax + reflink for XFS, xfs/179 began to report refcount
metadata corruptions after being run.  Specifically, xfs_repair noticed
single-block refcount records that could be combined but had not been.

The root cause of this is improper MAXREFCOUNT edge case handling in
xfs_refcount_merge_extents.  When we're trying to find candidates for a
refcount btree record merge, we compute the refcount attribute of the
merged record, but we fail to account for the fact that once a record
hits rc_refcount == MAXREFCOUNT, it is pinned that way forever.  Hence
the computed refcount is wrong, and we fail to merge the extents.

Fix this by adjusting the merge predicates to compute the adjusted
refcount correctly.

Fixes: 3172725814f9 ("xfs: adjust refcount of an extent of blocks in refcount btree")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Xiao Yang <yangx.jy@fujitsu.com>
Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 fs/xfs/libxfs/xfs_refcount.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index 9806942a13ea..ba6ce17af405 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -786,6 +786,17 @@ xfs_refc_valid(
 	return rc->rc_startblock != NULLAGBLOCK;
 }
 
+static inline xfs_nlink_t
+xfs_refc_merge_refcount(
+	const struct xfs_refcount_irec	*irec,
+	enum xfs_refc_adjust_op		adjust)
+{
+	/* Once a record hits MAXREFCOUNT, it is pinned there forever */
+	if (irec->rc_refcount == MAXREFCOUNT)
+		return MAXREFCOUNT;
+	return irec->rc_refcount + adjust;
+}
+
 static inline bool
 xfs_refc_want_merge_center(
 	const struct xfs_refcount_irec	*left,
@@ -797,6 +808,7 @@ xfs_refc_want_merge_center(
 	unsigned long long		*ulenp)
 {
 	unsigned long long		ulen = left->rc_blockcount;
+	xfs_nlink_t			new_refcount;
 
 	/*
 	 * To merge with a center record, both shoulder records must be
@@ -812,9 +824,10 @@ xfs_refc_want_merge_center(
 		return false;
 
 	/* The shoulder record refcounts must match the new refcount. */
-	if (left->rc_refcount != cleft->rc_refcount + adjust)
+	new_refcount = xfs_refc_merge_refcount(cleft, adjust);
+	if (left->rc_refcount != new_refcount)
 		return false;
-	if (right->rc_refcount != cleft->rc_refcount + adjust)
+	if (right->rc_refcount != new_refcount)
 		return false;
 
 	/*
@@ -837,6 +850,7 @@ xfs_refc_want_merge_left(
 	enum xfs_refc_adjust_op		adjust)
 {
 	unsigned long long		ulen = left->rc_blockcount;
+	xfs_nlink_t			new_refcount;
 
 	/*
 	 * For a left merge, the left shoulder record must be adjacent to the
@@ -847,7 +861,8 @@ xfs_refc_want_merge_left(
 		return false;
 
 	/* Left shoulder record refcount must match the new refcount. */
-	if (left->rc_refcount != cleft->rc_refcount + adjust)
+	new_refcount = xfs_refc_merge_refcount(cleft, adjust);
+	if (left->rc_refcount != new_refcount)
 		return false;
 
 	/*
@@ -869,6 +884,7 @@ xfs_refc_want_merge_right(
 	enum xfs_refc_adjust_op		adjust)
 {
 	unsigned long long		ulen = right->rc_blockcount;
+	xfs_nlink_t			new_refcount;
 
 	/*
 	 * For a right merge, the right shoulder record must be adjacent to the
@@ -879,7 +895,8 @@ xfs_refc_want_merge_right(
 		return false;
 
 	/* Right shoulder record refcount must match the new refcount. */
-	if (right->rc_refcount != cright->rc_refcount + adjust)
+	new_refcount = xfs_refc_merge_refcount(cright, adjust);
+	if (right->rc_refcount != new_refcount)
 		return false;
 
 	/*
-- 
2.33.0

Re: [PATCH 5.10 0/2] Fix xfs/179 for 5.10 stable

[Darrick J. Wong]

On Thu, Aug 03, 2023 at 05:36:50PM +0800, Guoqing Jiang wrote:
> Hi,
> 
> With the two patches applied, xfs/179 can pass in 5.10.188. Otherwise I got
> 
> [root@localhost xfstests]# ./check xfs/179
> FSTYP         -- xfs (non-debug)
> PLATFORM      -- Linux/x86_64 localhost 5.10.188-default #14 SMP Thu Aug 3 15:23:19 CST 2023
> MKFS_OPTIONS  -- -f /dev/loop1
> MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/loop1 /mnt/scratch
> 
> xfs/179 1s ... [failed, exit status 1]- output mismatch (see /root/xfstests/results//xfs/179.out.bad)
>     --- tests/xfs/179.out	2023-07-13 16:12:27.000000000 +0800
>     +++ /root/xfstests/results//xfs/179.out.bad	2023-08-03 16:55:38.173787911 +0800
>     @@ -8,3 +8,5 @@
>      Check scratch fs
>      Remove reflinked files
>      Check scratch fs
>     +xfs_repair fails
>     +(see /root/xfstests/results//xfs/179.full for details)
>     ...
>     (Run 'diff -u /root/xfstests/tests/xfs/179.out /root/xfstests/results//xfs/179.out.bad'  to see the entire diff)
> 
> HINT: You _MAY_ be missing kernel fix:
>       b25d1984aa88 xfs: estimate post-merge refcounts correctly
> 
> Ran: xfs/179
> Failures: xfs/179
> Failed 1 of 1 tests
> 
> Please review if they are approriate for 5.10 stable.

Seems fine to me, but ... there is no maintainer for 5.10; is your
employer willing to support this LTS kernel?

--D

> Thanks,
> Guoqing
> 
> Darrick J. Wong (2):
>   xfs: hoist refcount record merge predicates
>   xfs: estimate post-merge refcounts correctly
> 
>  fs/xfs/libxfs/xfs_refcount.c | 146 +++++++++++++++++++++++++++++++----
>  1 file changed, 130 insertions(+), 16 deletions(-)
> 
> -- 
> 2.33.0
> 

Re: [PATCH 5.10 0/2] Fix xfs/179 for 5.10 stable

[Guoqing Jiang]

On 8/4/23 23:47, Darrick J. Wong wrote:
> On Thu, Aug 03, 2023 at 05:36:50PM +0800, Guoqing Jiang wrote:
>> Hi,
>>
>> With the two patches applied, xfs/179 can pass in 5.10.188. Otherwise I got
>>
>> [root@localhost xfstests]# ./check xfs/179
>> FSTYP         -- xfs (non-debug)
>> PLATFORM      -- Linux/x86_64 localhost 5.10.188-default #14 SMP Thu Aug 3 15:23:19 CST 2023
>> MKFS_OPTIONS  -- -f /dev/loop1
>> MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/loop1 /mnt/scratch
>>
>> xfs/179 1s ... [failed, exit status 1]- output mismatch (see /root/xfstests/results//xfs/179.out.bad)
>>      --- tests/xfs/179.out	2023-07-13 16:12:27.000000000 +0800
>>      +++ /root/xfstests/results//xfs/179.out.bad	2023-08-03 16:55:38.173787911 +0800
>>      @@ -8,3 +8,5 @@
>>       Check scratch fs
>>       Remove reflinked files
>>       Check scratch fs
>>      +xfs_repair fails
>>      +(see /root/xfstests/results//xfs/179.full for details)
>>      ...
>>      (Run 'diff -u /root/xfstests/tests/xfs/179.out /root/xfstests/results//xfs/179.out.bad'  to see the entire diff)
>>
>> HINT: You _MAY_ be missing kernel fix:
>>        b25d1984aa88 xfs: estimate post-merge refcounts correctly
>>
>> Ran: xfs/179
>> Failures: xfs/179
>> Failed 1 of 1 tests
>>
>> Please review if they are approriate for 5.10 stable.
> Seems fine to me, but ... there is no maintainer for 5.10; is your
> employer willing to support this LTS kernel?

Hi Darrick,

Thanks for your review! I think Amir is the maintainer for 5.10 😉. I 
can help
if needed since our kernel is heavily based on 5.10 stable. We also run 
tests
against 5.10 stable, that is why I send fixes patches for it.

Hi Greg,

Could you consider add the two to your list? Thank you!

Regards,
Guoqing

Re: [PATCH 5.10 0/2] Fix xfs/179 for 5.10 stable

[Greg KH]

On Mon, Aug 07, 2023 at 10:39:44AM +0800, Guoqing Jiang wrote:
> 
> 
> On 8/4/23 23:47, Darrick J. Wong wrote:
> > On Thu, Aug 03, 2023 at 05:36:50PM +0800, Guoqing Jiang wrote:
> > > Hi,
> > > 
> > > With the two patches applied, xfs/179 can pass in 5.10.188. Otherwise I got
> > > 
> > > [root@localhost xfstests]# ./check xfs/179
> > > FSTYP         -- xfs (non-debug)
> > > PLATFORM      -- Linux/x86_64 localhost 5.10.188-default #14 SMP Thu Aug 3 15:23:19 CST 2023
> > > MKFS_OPTIONS  -- -f /dev/loop1
> > > MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/loop1 /mnt/scratch
> > > 
> > > xfs/179 1s ... [failed, exit status 1]- output mismatch (see /root/xfstests/results//xfs/179.out.bad)
> > >      --- tests/xfs/179.out	2023-07-13 16:12:27.000000000 +0800
> > >      +++ /root/xfstests/results//xfs/179.out.bad	2023-08-03 16:55:38.173787911 +0800
> > >      @@ -8,3 +8,5 @@
> > >       Check scratch fs
> > >       Remove reflinked files
> > >       Check scratch fs
> > >      +xfs_repair fails
> > >      +(see /root/xfstests/results//xfs/179.full for details)
> > >      ...
> > >      (Run 'diff -u /root/xfstests/tests/xfs/179.out /root/xfstests/results//xfs/179.out.bad'  to see the entire diff)
> > > 
> > > HINT: You _MAY_ be missing kernel fix:
> > >        b25d1984aa88 xfs: estimate post-merge refcounts correctly
> > > 
> > > Ran: xfs/179
> > > Failures: xfs/179
> > > Failed 1 of 1 tests
> > > 
> > > Please review if they are approriate for 5.10 stable.
> > Seems fine to me, but ... there is no maintainer for 5.10; is your
> > employer willing to support this LTS kernel?
> 
> Hi Darrick,
> 
> Thanks for your review! I think Amir is the maintainer for 5.10 😉. I can
> help
> if needed since our kernel is heavily based on 5.10 stable. We also run
> tests
> against 5.10 stable, that is why I send fixes patches for it.
> 
> Hi Greg,
> 
> Could you consider add the two to your list? Thank you!

Sorry, but as these would only be in th 5.10.y release, and not in any
newer stable kernel, you would have a regression if you moved to a newer
stable kernel branch, right?

Because of that, no, I can't take this, nor should you want me to, as
you would have a regression if you upgraded, right?

I'll be glad to do so if we have backports for all relevant stable
kernels.

thanks,

greg k-h

Re: [PATCH 5.10 0/2] Fix xfs/179 for 5.10 stable

[Guoqing Jiang]

On 8/7/23 17:11, Greg KH wrote:
> On Mon, Aug 07, 2023 at 10:39:44AM +0800, Guoqing Jiang wrote:
>>
>> On 8/4/23 23:47, Darrick J. Wong wrote:
>>> On Thu, Aug 03, 2023 at 05:36:50PM +0800, Guoqing Jiang wrote:
>>>> Hi,
>>>>
>>>> With the two patches applied, xfs/179 can pass in 5.10.188. Otherwise I got
>>>>
>>>> [root@localhost xfstests]# ./check xfs/179
>>>> FSTYP         -- xfs (non-debug)
>>>> PLATFORM      -- Linux/x86_64 localhost 5.10.188-default #14 SMP Thu Aug 3 15:23:19 CST 2023
>>>> MKFS_OPTIONS  -- -f /dev/loop1
>>>> MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/loop1 /mnt/scratch
>>>>
>>>> xfs/179 1s ... [failed, exit status 1]- output mismatch (see /root/xfstests/results//xfs/179.out.bad)
>>>>       --- tests/xfs/179.out	2023-07-13 16:12:27.000000000 +0800
>>>>       +++ /root/xfstests/results//xfs/179.out.bad	2023-08-03 16:55:38.173787911 +0800
>>>>       @@ -8,3 +8,5 @@
>>>>        Check scratch fs
>>>>        Remove reflinked files
>>>>        Check scratch fs
>>>>       +xfs_repair fails
>>>>       +(see /root/xfstests/results//xfs/179.full for details)
>>>>       ...
>>>>       (Run 'diff -u /root/xfstests/tests/xfs/179.out /root/xfstests/results//xfs/179.out.bad'  to see the entire diff)
>>>>
>>>> HINT: You _MAY_ be missing kernel fix:
>>>>         b25d1984aa88 xfs: estimate post-merge refcounts correctly
>>>>
>>>> Ran: xfs/179
>>>> Failures: xfs/179
>>>> Failed 1 of 1 tests
>>>>
>>>> Please review if they are approriate for 5.10 stable.
>>> Seems fine to me, but ... there is no maintainer for 5.10; is your
>>> employer willing to support this LTS kernel?
>> Hi Darrick,
>>
>> Thanks for your review! I think Amir is the maintainer for 5.10 😉. I can
>> help
>> if needed since our kernel is heavily based on 5.10 stable. We also run
>> tests
>> against 5.10 stable, that is why I send fixes patches for it.
>>
>> Hi Greg,
>>
>> Could you consider add the two to your list? Thank you!
> Sorry, but as these would only be in th 5.10.y release, and not in any
> newer stable kernel, you would have a regression if you moved to a newer
> stable kernel branch, right?

Fair enough.

> Because of that, no, I can't take this, nor should you want me to, as
> you would have a regression if you upgraded, right?
>
> I'll be glad to do so if we have backports for all relevant stable
> kernels.
>

I found Ted had mentioned the two commits ([1]), so it probably make sense
to port them to 5.10 as well.

[1]. https://lore.kernel.org/linux-xfs/20230802031039.GC358316@mit.edu/

Thanks,
Guoqing