[PATCH] xfs: fix di_onlink checking for V1/V2 inodes

[PATCH] xfs: fix di_onlink checking for V1/V2 inodes

[Darrick J. Wong]

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

"KjellR" complained on IRC that an old V4 filesystem suddenly stopped
mounting after upgrading from 6.9.11 to 6.10.3, with the following splat
when trying to read the rt bitmap inode:

00000000: 49 4e 80 00 01 02 00 01 00 00 00 00 00 00 00 00  IN..............
00000010: 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00 00  ................
00000020: 00 00 00 00 00 00 00 00 43 d2 a9 da 21 0f d6 30  ........C...!..0
00000030: 43 d2 a9 da 21 0f d6 30 00 00 00 00 00 00 00 00  C...!..0........
00000040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
00000050: 00 00 00 02 00 00 00 00 00 00 00 04 00 00 00 00  ................
00000060: ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00  ................
00000070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................

As Dave Chinner points out, this is a V1 inode with both di_onlink and
di_nlink set to 1 and di_flushiter == 0.  In other words, this inode was
formatted this way by mkfs and hasn't been touched since then.

Back in the old days of xfsprogs 3.2.3, I observed that libxfs_ialloc
would set di_nlink, but if the filesystem didn't have NLINK, it would
then set di_version = 1.  libxfs_iflush_int later sees the V1 inode and
copies the value of di_nlink to di_onlink without zeroing di_onlink.

Eventually this filesystem must have been upgraded to support NLINK
because 6.10 doesn't support !NLINK filesystems, which is how we tripped
over this old behavior.  The filesystem doesn't have a realtime section,
so that's why the rtbitmap inode has never been touched.

Fix this by removing the di_onlink/di_nlink checking for all V1/V2
inodes because this is a muddy mess.  The V3 inode handling code has
always supported NLINK and written di_onlink==0 so keep that check.
The removal of the V1 inode handling code when we dropped support for
!NLINK obscured this old behavior.

Reported-by: kjell.m.randa@gmail.com
Fixes: 40cb8613d612 ("xfs: check unused nlink fields in the ondisk inode")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_inode_buf.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 513b50da6215..79babeac9d75 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -514,12 +514,18 @@ xfs_dinode_verify(
 			return __this_address;
 	}
 
-	if (dip->di_version > 1) {
+	/*
+	 * Historical note: xfsprogs in the 3.2 era set up its incore inodes to
+	 * have di_nlink track the link count, even if the actual filesystem
+	 * only supported V1 inodes (i.e. di_onlink).  When writing out the
+	 * ondisk inode, it would set both the ondisk di_nlink and di_onlink to
+	 * the the incore di_nlink value, which is why we cannot check for
+	 * di_nlink==0 on a V1 inode.  V2/3 inodes would get written out with
+	 * di_onlink==0, so we can check that.
+	 */
+	if (dip->di_version >= 2) {
 		if (dip->di_onlink)
 			return __this_address;
-	} else {
-		if (dip->di_nlink)
-			return __this_address;
 	}
 
 	/* don't allow invalid i_size */

Re: [PATCH] xfs: fix di_onlink checking for V1/V2 inodes

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] xfs: fix di_onlink checking for V1/V2 inodes

[Darrick J. Wong]

On Mon, Aug 12, 2024 at 10:46:59PM -0700, Christoph Hellwig wrote:
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Hey Chandan, I just realized this didn't get merged yet, even though
there's a user complaint about this.  Can we get this staged for 6.11,
please?

--D

Re: [PATCH] xfs: fix di_onlink checking for V1/V2 inodes

[Chandan Babu R]

On Wed, Aug 21, 2024 at 10:04:42 PM -0700, Darrick J. Wong wrote:
> On Mon, Aug 12, 2024 at 10:46:59PM -0700, Christoph Hellwig wrote:
>> Looks good:
>> 
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> Hey Chandan, I just realized this didn't get merged yet, even though
> there's a user complaint about this.  Can we get this staged for 6.11,
> please?

Sorry, I missed this patch. I have started running tests on "v6.11-rc4 +
<this patch>".  However, the earliest I could send the pull request is
sometime next week.

-- 
Chandan

Re: [PATCH] xfs: fix di_onlink checking for V1/V2 inodes

[Christoph Hellwig]

On Thu, Aug 22, 2024 at 10:55:05AM +0530, Chandan Babu R wrote:
> On Wed, Aug 21, 2024 at 10:04:42 PM -0700, Darrick J. Wong wrote:
> > On Mon, Aug 12, 2024 at 10:46:59PM -0700, Christoph Hellwig wrote:
> >> Looks good:
> >> 
> >> Reviewed-by: Christoph Hellwig <hch@lst.de>
> >
> > Hey Chandan, I just realized this didn't get merged yet, even though
> > there's a user complaint about this.  Can we get this staged for 6.11,
> > please?
> 
> Sorry, I missed this patch. I have started running tests on "v6.11-rc4 +
> <this patch>".  However, the earliest I could send the pull request is
> sometime next week.

FYI, we'll also need to to fix the new RT discard support as it
currently is broken when the RT device does not support discard.
Either my series or (maybe?) the patch Darrick just sent, I'm
going to give it a spin now.

Re: [PATCH] xfs: fix di_onlink checking for V1/V2 inodes

[Chandan Babu R]

On Wed, Aug 21, 2024 at 10:31:15 PM -0700, Christoph Hellwig wrote:
> On Thu, Aug 22, 2024 at 10:55:05AM +0530, Chandan Babu R wrote:
>> On Wed, Aug 21, 2024 at 10:04:42 PM -0700, Darrick J. Wong wrote:
>> > On Mon, Aug 12, 2024 at 10:46:59PM -0700, Christoph Hellwig wrote:
>> >> Looks good:
>> >> 
>> >> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> >
>> > Hey Chandan, I just realized this didn't get merged yet, even though
>> > there's a user complaint about this.  Can we get this staged for 6.11,
>> > please?
>> 
>> Sorry, I missed this patch. I have started running tests on "v6.11-rc4 +
>> <this patch>".  However, the earliest I could send the pull request is
>> sometime next week.
>
> FYI, we'll also need to to fix the new RT discard support as it
> currently is broken when the RT device does not support discard.
> Either my series or (maybe?) the patch Darrick just sent, I'm
> going to give it a spin now.

I will keep a watch on them and pick the one which gets reviewed.

-- 
Chandan

[PATCH] xfs: add a test for v1 inodes with nonzero nlink and onlink fields

[Darrick J. Wong]

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

Add a regression test for XFS V1 inodes that have both nlink and onlink
fields set to 1.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/1890     |   39 +++++++++++++++++++++++++++++++++++++++
 tests/xfs/1890.out |    7 +++++++
 2 files changed, 46 insertions(+)
 create mode 100755 tests/xfs/1890
 create mode 100644 tests/xfs/1890.out

diff --git a/tests/xfs/1890 b/tests/xfs/1890
new file mode 100755
index 0000000000..c3813fca67
--- /dev/null
+++ b/tests/xfs/1890
@@ -0,0 +1,39 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2024 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 1890
+#
+# Regression test for V1 inodes that have di_onlink and di_nlink set to 1.
+#
+. ./common/preamble
+_begin_fstest auto
+
+_fixed_by_kernel_commit XXXXXXXXXXXX \
+	"xfs: fix di_onlink checking for V1/V2 inodes",
+
+_require_scratch_nocheck	# we'll do our own checking
+_require_xfs_nocrc
+
+_scratch_mkfs -m crc=0 >> $seqres.full
+_scratch_xfs_db -x \
+	-c 'sb' \
+	-c 'addr rbmino' \
+	-c 'print core.nlinkv2 core.onlink' \
+	-c 'write -d core.version 1' \
+	-c 'write -d core.nlinkv1 1' \
+	-c 'print core.version core.nlinkv1'
+
+# repair doesn't flag this combination
+_scratch_xfs_repair -n &>> $seqres.full || echo "xfs_repair -n failed??"
+
+# Prior to kernel commit 40cb8613d612 ("xfs: check unused nlink fields in the
+# ondisk inode"), the kernel accepted V1 format inode where both the new and
+# old nlink fields are set to 1.  With that commit applied, it stopped
+# accepting that combination and will refuse to mount.  Hence we need the fix
+# mentioned above.
+_scratch_mount
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/1890.out b/tests/xfs/1890.out
new file mode 100644
index 0000000000..166ae90286
--- /dev/null
+++ b/tests/xfs/1890.out
@@ -0,0 +1,7 @@
+QA output created by 1890
+core.nlinkv2 = 1
+core.onlink = 0
+core.version = 1
+core.nlinkv1 = 1
+core.version = 1
+core.nlinkv1 = 1

Re: [PATCH] xfs: add a test for v1 inodes with nonzero nlink and onlink fields

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>