[PATCHSET 0/6] xfsprogs: random fixes

[PATCHSET 0/6] xfsprogs: random fixes

[Darrick J. Wong]

Hi all,

Here are various fixes for the new code added in the 5.19 kernel.
They're pretty much all nrext64 fixes.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes
---
 copy/xfs_copy.c |    2 +-
 libxfs/util.c   |    9 ++++++---
 mkfs/xfs_mkfs.c |    2 +-
 repair/dinode.c |   39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 47 insertions(+), 5 deletions(-)

[PATCH 1/6] xfs_copy: don't use cached buffer reads until after libxfs_mount

[Darrick J. Wong]

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

I accidentally tried to xfs_copy an ext4 filesystem, but instead of
rejecting the filesystem, the program instead crashed.  I figured out
that zeroing the superblock was enough to trigger this:

# dd if=/dev/zero of=/dev/sda bs=1024k count=1
# xfs_copy  /dev/sda /dev/sdb
Floating point exception

The exact crash happens in this line from libxfs_getbuf_flags, which is
called from the main() routine of xfs_copy:

	if (btp == btp->bt_mount->m_ddev_targp) {
		(*bpp)->b_pag = xfs_perag_get(btp->bt_mount,
				xfs_daddr_to_agno(btp->bt_mount, blkno));

The problem here is that the uncached read filled the incore superblock
with zeroes, which means mbuf.sb_agblocks is zero.  This causes a
division by zero in xfs_daddr_to_agno, thereby crashing the program.

In commit f8b581d6, we made it so that xfs_buf structures contain a
passive reference to the associated perag structure.  That commit
assumes that no program would try a cached buffer read until the buffer
cache is fully set up, which is true throughout xfsprogs... except for
the beginning of xfs_copy.  For whatever reason, it attempts an uncached
read of the superblock to figure out the real superblock size, then
performs a *cached* read with the proper buffer length and verifier.
The cached read crashes the program.

Fix the problem by changing the (second) cached read into an uncached read.

Fixes: f8b581d6 ("libxfs: actually make buffers track the per-ag structures")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 copy/xfs_copy.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
index 41f594bd..79f65946 100644
--- a/copy/xfs_copy.c
+++ b/copy/xfs_copy.c
@@ -748,7 +748,7 @@ main(int argc, char **argv)
 	/* Do it again, now with proper length and verifier */
 	libxfs_buf_relse(sbp);
 
-	error = -libxfs_buf_read(mbuf.m_ddev_targp, XFS_SB_DADDR,
+	error = -libxfs_buf_read_uncached(mbuf.m_ddev_targp, XFS_SB_DADDR,
 			1 << (sb->sb_sectlog - BBSHIFT), 0, &sbp,
 			&xfs_sb_buf_ops);
 	if (error) {

[PATCH 2/6] xfs_repair: clear DIFLAG2_NREXT64 when filesystem doesn't support nrext64

[Darrick J. Wong]

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

Clear the nrext64 inode flag if the filesystem doesn't have the nrext64
feature enabled in the superblock.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/dinode.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)


diff --git a/repair/dinode.c b/repair/dinode.c
index 00de31fb..547c5833 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2690,6 +2690,25 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 			}
 		}
 
+		if (xfs_dinode_has_large_extent_counts(dino) &&
+		    !xfs_has_large_extent_counts(mp)) {
+			if (!uncertain) {
+				do_warn(
+	_("inode %" PRIu64 " is marked large extent counts but file system does not support large extent counts\n"),
+					lino);
+			}
+			flags2 &= ~XFS_DIFLAG2_NREXT64;
+
+			if (no_modify) {
+				do_warn(_("would zero extent counts.\n"));
+			} else {
+				do_warn(_("zeroing extent counts.\n"));
+				dino->di_nextents = 0;
+				dino->di_anextents = 0;
+				*dirty = 1;
+			}
+		}
+
 		if (!verify_mode && flags2 != be64_to_cpu(dino->di_flags2)) {
 			if (!no_modify) {
 				do_warn(_("fixing bad flags2.\n"));

[PATCH 3/6] xfs_repair: detect and fix padding fields that changed with nrext64

[Darrick J. Wong]

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

Detect incorrectly set padding fields when large extent counts are
enabled or disabled on v3 inodes.

Found by fuzzing v3.flags2 = zeroes with xfs/374 and an nrext64=1
filesystem.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/dinode.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)


diff --git a/repair/dinode.c b/repair/dinode.c
index 547c5833..1c92bfbd 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2709,6 +2709,26 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 			}
 		}
 
+		if (xfs_dinode_has_large_extent_counts(dino)) {
+			if (dino->di_nrext64_pad) {
+				if (!no_modify) {
+					do_warn(_("fixing bad nrext64_pad.\n"));
+					dino->di_nrext64_pad = 0;
+					*dirty = 1;
+				} else
+					do_warn(_("would fix bad nrext64_pad.\n"));
+			}
+		} else if (dino->di_version >= 3) {
+			if (dino->di_v3_pad) {
+				if (!no_modify) {
+					do_warn(_("fixing bad v3_pad.\n"));
+					dino->di_v3_pad = 0;
+					*dirty = 1;
+				} else
+					do_warn(_("would fix bad v3_pad.\n"));
+			}
+		}
+
 		if (!verify_mode && flags2 != be64_to_cpu(dino->di_flags2)) {
 			if (!no_modify) {
 				do_warn(_("fixing bad flags2.\n"));

[PATCH 4/6] mkfs: preserve DIFLAG2_NREXT64 when setting other inode attributes

[Darrick J. Wong]

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

Preserve the state of the NREXT64 inode flag when we're changing the
other flags2 fields.  This is only vital for the kernel version of this
function, but we should keep these in sync.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 libxfs/util.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/libxfs/util.c b/libxfs/util.c
index ef01fcf8..d2389198 100644
--- a/libxfs/util.c
+++ b/libxfs/util.c
@@ -198,7 +198,8 @@ xfs_flags2diflags2(
 {
 	uint64_t		di_flags2 =
 		(ip->i_diflags2 & (XFS_DIFLAG2_REFLINK |
-				   XFS_DIFLAG2_BIGTIME));
+				   XFS_DIFLAG2_BIGTIME |
+				   XFS_DIFLAG2_NREXT64));
 
 	if (xflags & FS_XFLAG_DAX)
 		di_flags2 |= XFS_DIFLAG2_DAX;

[PATCH 5/6] mkfs: document the large extent count switch in the --help screen

[Darrick J. Wong]

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

We should document this feature in the --help screen for mkfs.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 mkfs/xfs_mkfs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index a4705ee4..db322b3a 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -952,7 +952,7 @@ usage( void )
 			    sectsize=num\n\
 /* force overwrite */	[-f]\n\
 /* inode size */	[-i perblock=n|size=num,maxpct=n,attr=0|1|2,\n\
-			    projid32bit=0|1,sparse=0|1]\n\
+			    projid32bit=0|1,sparse=0|1,nrext64=0|1]\n\
 /* no discard */	[-K]\n\
 /* log subvol */	[-l agnum=n,internal,size=num,logdev=xxx,version=n\n\
 			    sunit=value|su=num,sectsize=num,lazy-count=0|1]\n\

[PATCH 6/6] mkfs: always use new_diflags2 to initialize new inodes

[Darrick J. Wong]

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

The new_diflags2 field that's set in the inode geometry represent
features that we want enabled for /all/ newly created inodes.
Unfortunately, mkfs doesn't do that because xfs_flags2diflags2 doesn't
read new_diflags2.  Change the new_diflags2 logic to match the kernel.

Without this fix, the root directory gets created without the
DIFLAG2_NREXT64 iflag set, but files created by a protofile /do/ have it
turned on.

This wasn't an issue with DIFLAG2_BIGTIME because xfs_trans_log_inode
quietly turns that on whenever possible.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 libxfs/util.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)


diff --git a/libxfs/util.c b/libxfs/util.c
index d2389198..5d2383e9 100644
--- a/libxfs/util.c
+++ b/libxfs/util.c
@@ -286,8 +286,10 @@ libxfs_init_new_inode(
 
 	if (xfs_has_v3inodes(ip->i_mount)) {
 		VFS_I(ip)->i_version = 1;
-		ip->i_diflags2 = pip ? ip->i_mount->m_ino_geo.new_diflags2 :
-				xfs_flags2diflags2(ip, fsx->fsx_xflags);
+		ip->i_diflags2 = ip->i_mount->m_ino_geo.new_diflags2;
+		if (!pip)
+			ip->i_diflags2 = xfs_flags2diflags2(ip,
+							fsx->fsx_xflags);
 		ip->i_crtime = VFS_I(ip)->i_mtime; /* struct copy */
 		ip->i_cowextsize = pip ? 0 : fsx->fsx_cowextsize;
 	}

Re: [PATCH 1/6] xfs_copy: don't use cached buffer reads until after libxfs_mount

[Dave Chinner]

On Tue, Jun 28, 2022 at 01:49:20PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> I accidentally tried to xfs_copy an ext4 filesystem, but instead of
> rejecting the filesystem, the program instead crashed.  I figured out
> that zeroing the superblock was enough to trigger this:
> 
> # dd if=/dev/zero of=/dev/sda bs=1024k count=1
> # xfs_copy  /dev/sda /dev/sdb
> Floating point exception
> 
> The exact crash happens in this line from libxfs_getbuf_flags, which is
> called from the main() routine of xfs_copy:
> 
> 	if (btp == btp->bt_mount->m_ddev_targp) {
> 		(*bpp)->b_pag = xfs_perag_get(btp->bt_mount,
> 				xfs_daddr_to_agno(btp->bt_mount, blkno));
> 
> The problem here is that the uncached read filled the incore superblock
> with zeroes, which means mbuf.sb_agblocks is zero.  This causes a
> division by zero in xfs_daddr_to_agno, thereby crashing the program.
> 
> In commit f8b581d6, we made it so that xfs_buf structures contain a
> passive reference to the associated perag structure.  That commit
> assumes that no program would try a cached buffer read until the buffer
> cache is fully set up, which is true throughout xfsprogs... except for
> the beginning of xfs_copy.  For whatever reason, it attempts an uncached
> read of the superblock to figure out the real superblock size, then
> performs a *cached* read with the proper buffer length and verifier.
> The cached read crashes the program.
> 
> Fix the problem by changing the (second) cached read into an uncached read.
> 
> Fixes: f8b581d6 ("libxfs: actually make buffers track the per-ag structures")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  copy/xfs_copy.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
> index 41f594bd..79f65946 100644
> --- a/copy/xfs_copy.c
> +++ b/copy/xfs_copy.c
> @@ -748,7 +748,7 @@ main(int argc, char **argv)
>  	/* Do it again, now with proper length and verifier */
>  	libxfs_buf_relse(sbp);
>  
> -	error = -libxfs_buf_read(mbuf.m_ddev_targp, XFS_SB_DADDR,
> +	error = -libxfs_buf_read_uncached(mbuf.m_ddev_targp, XFS_SB_DADDR,
>  			1 << (sb->sb_sectlog - BBSHIFT), 0, &sbp,
>  			&xfs_sb_buf_ops);
>  	if (error) {

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/6] xfs_repair: clear DIFLAG2_NREXT64 when filesystem doesn't support nrext64

[Dave Chinner]

On Tue, Jun 28, 2022 at 01:49:25PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Clear the nrext64 inode flag if the filesystem doesn't have the nrext64
> feature enabled in the superblock.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  repair/dinode.c |   19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 00de31fb..547c5833 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2690,6 +2690,25 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  			}
>  		}
>  
> +		if (xfs_dinode_has_large_extent_counts(dino) &&
> +		    !xfs_has_large_extent_counts(mp)) {
> +			if (!uncertain) {
> +				do_warn(
> +	_("inode %" PRIu64 " is marked large extent counts but file system does not support large extent counts\n"),
> +					lino);
> +			}
> +			flags2 &= ~XFS_DIFLAG2_NREXT64;
> +
> +			if (no_modify) {
> +				do_warn(_("would zero extent counts.\n"));
> +			} else {
> +				do_warn(_("zeroing extent counts.\n"));
> +				dino->di_nextents = 0;
> +				dino->di_anextents = 0;
> +				*dirty = 1;

Is that necessary? If the existing extent counts are within the
bounds of the old extent fields, then shouldn't we just rewrite the
current values into the old format rather than trashing all the
data/xattrs on the inode?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/6] xfs_repair: detect and fix padding fields that changed with nrext64

[Dave Chinner]

On Tue, Jun 28, 2022 at 01:49:31PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Detect incorrectly set padding fields when large extent counts are
> enabled or disabled on v3 inodes.
> 
> Found by fuzzing v3.flags2 = zeroes with xfs/374 and an nrext64=1
> filesystem.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  repair/dinode.c |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/6] mkfs: preserve DIFLAG2_NREXT64 when setting other inode attributes

[Dave Chinner]

On Tue, Jun 28, 2022 at 01:49:36PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Preserve the state of the NREXT64 inode flag when we're changing the
> other flags2 fields.  This is only vital for the kernel version of this
> function, but we should keep these in sync.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  libxfs/util.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/libxfs/util.c b/libxfs/util.c
> index ef01fcf8..d2389198 100644
> --- a/libxfs/util.c
> +++ b/libxfs/util.c
> @@ -198,7 +198,8 @@ xfs_flags2diflags2(
>  {
>  	uint64_t		di_flags2 =
>  		(ip->i_diflags2 & (XFS_DIFLAG2_REFLINK |
> -				   XFS_DIFLAG2_BIGTIME));
> +				   XFS_DIFLAG2_BIGTIME |
> +				   XFS_DIFLAG2_NREXT64));

*nod*

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/6] mkfs: document the large extent count switch in the --help screen

[Dave Chinner]

On Tue, Jun 28, 2022 at 01:49:42PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> We should document this feature in the --help screen for mkfs.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  mkfs/xfs_mkfs.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index a4705ee4..db322b3a 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -952,7 +952,7 @@ usage( void )
>  			    sectsize=num\n\
>  /* force overwrite */	[-f]\n\
>  /* inode size */	[-i perblock=n|size=num,maxpct=n,attr=0|1|2,\n\
> -			    projid32bit=0|1,sparse=0|1]\n\
> +			    projid32bit=0|1,sparse=0|1,nrext64=0|1]\n\
>  /* no discard */	[-K]\n\
>  /* log subvol */	[-l agnum=n,internal,size=num,logdev=xxx,version=n\n\
>  			    sunit=value|su=num,sectsize=num,lazy-count=0|1]\n\

LGTM

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 6/6] mkfs: always use new_diflags2 to initialize new inodes

[Dave Chinner]

On Tue, Jun 28, 2022 at 01:49:47PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The new_diflags2 field that's set in the inode geometry represent
> features that we want enabled for /all/ newly created inodes.
> Unfortunately, mkfs doesn't do that because xfs_flags2diflags2 doesn't
> read new_diflags2.  Change the new_diflags2 logic to match the kernel.
> 
> Without this fix, the root directory gets created without the
> DIFLAG2_NREXT64 iflag set, but files created by a protofile /do/ have it
> turned on.
> 
> This wasn't an issue with DIFLAG2_BIGTIME because xfs_trans_log_inode
> quietly turns that on whenever possible.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  libxfs/util.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/libxfs/util.c b/libxfs/util.c
> index d2389198..5d2383e9 100644
> --- a/libxfs/util.c
> +++ b/libxfs/util.c
> @@ -286,8 +286,10 @@ libxfs_init_new_inode(
>  
>  	if (xfs_has_v3inodes(ip->i_mount)) {
>  		VFS_I(ip)->i_version = 1;
> -		ip->i_diflags2 = pip ? ip->i_mount->m_ino_geo.new_diflags2 :
> -				xfs_flags2diflags2(ip, fsx->fsx_xflags);
> +		ip->i_diflags2 = ip->i_mount->m_ino_geo.new_diflags2;
> +		if (!pip)
> +			ip->i_diflags2 = xfs_flags2diflags2(ip,
> +							fsx->fsx_xflags);
>  		ip->i_crtime = VFS_I(ip)->i_mtime; /* struct copy */
>  		ip->i_cowextsize = pip ? 0 : fsx->fsx_cowextsize;
>  	}

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/6] xfs_copy: don't use cached buffer reads until after libxfs_mount

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 2/6] xfs_repair: clear DIFLAG2_NREXT64 when filesystem doesn't support nrext64

[Darrick J. Wong]

On Wed, Jun 29, 2022 at 08:58:57AM +1000, Dave Chinner wrote:
> On Tue, Jun 28, 2022 at 01:49:25PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Clear the nrext64 inode flag if the filesystem doesn't have the nrext64
> > feature enabled in the superblock.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  repair/dinode.c |   19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > 
> > diff --git a/repair/dinode.c b/repair/dinode.c
> > index 00de31fb..547c5833 100644
> > --- a/repair/dinode.c
> > +++ b/repair/dinode.c
> > @@ -2690,6 +2690,25 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
> >  			}
> >  		}
> >  
> > +		if (xfs_dinode_has_large_extent_counts(dino) &&
> > +		    !xfs_has_large_extent_counts(mp)) {
> > +			if (!uncertain) {
> > +				do_warn(
> > +	_("inode %" PRIu64 " is marked large extent counts but file system does not support large extent counts\n"),
> > +					lino);
> > +			}
> > +			flags2 &= ~XFS_DIFLAG2_NREXT64;
> > +
> > +			if (no_modify) {
> > +				do_warn(_("would zero extent counts.\n"));
> > +			} else {
> > +				do_warn(_("zeroing extent counts.\n"));
> > +				dino->di_nextents = 0;
> > +				dino->di_anextents = 0;
> > +				*dirty = 1;
> 
> Is that necessary? If the existing extent counts are within the
> bounds of the old extent fields, then shouldn't we just rewrite the
> current values into the old format rather than trashing all the
> data/xattrs on the inode?

It's hard to know what to do here -- we haven't actually checked the
forks yet, so we don't know if the dinode flag was set but the !nrext64
extent count fields are ok so all we have to do is clear the dinode
flag; or if the dinode flag was set, the nrext64 extent count fields are
correct and must be moved to the !nrext64 fields; or what?

I guess I could just leave the extent count fields as-is and let the
process_*_fork functions deal with it.

--D

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

Re: [PATCH 2/6] xfs_repair: clear DIFLAG2_NREXT64 when filesystem doesn't support nrext64

[Dave Chinner]

On Wed, Jun 29, 2022 at 04:10:47PM -0700, Darrick J. Wong wrote:
> On Wed, Jun 29, 2022 at 08:58:57AM +1000, Dave Chinner wrote:
> > On Tue, Jun 28, 2022 at 01:49:25PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Clear the nrext64 inode flag if the filesystem doesn't have the nrext64
> > > feature enabled in the superblock.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  repair/dinode.c |   19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > 
> > > diff --git a/repair/dinode.c b/repair/dinode.c
> > > index 00de31fb..547c5833 100644
> > > --- a/repair/dinode.c
> > > +++ b/repair/dinode.c
> > > @@ -2690,6 +2690,25 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
> > >  			}
> > >  		}
> > >  
> > > +		if (xfs_dinode_has_large_extent_counts(dino) &&
> > > +		    !xfs_has_large_extent_counts(mp)) {
> > > +			if (!uncertain) {
> > > +				do_warn(
> > > +	_("inode %" PRIu64 " is marked large extent counts but file system does not support large extent counts\n"),
> > > +					lino);
> > > +			}
> > > +			flags2 &= ~XFS_DIFLAG2_NREXT64;
> > > +
> > > +			if (no_modify) {
> > > +				do_warn(_("would zero extent counts.\n"));
> > > +			} else {
> > > +				do_warn(_("zeroing extent counts.\n"));
> > > +				dino->di_nextents = 0;
> > > +				dino->di_anextents = 0;
> > > +				*dirty = 1;
> > 
> > Is that necessary? If the existing extent counts are within the
> > bounds of the old extent fields, then shouldn't we just rewrite the
> > current values into the old format rather than trashing all the
> > data/xattrs on the inode?
> 
> It's hard to know what to do here -- we haven't actually checked the
> forks yet, so we don't know if the dinode flag was set but the !nrext64
> extent count fields are ok so all we have to do is clear the dinode
> flag; or if the dinode flag was set, the nrext64 extent count fields are
> correct and must be moved to the !nrext64 fields; or what?
> 
> I guess I could just leave the extent count fields as-is and let the
> process_*_fork functions deal with it.

Can we check it -after- the inode has otherwise been validated
and do the conversion then?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/6] xfs_repair: clear DIFLAG2_NREXT64 when filesystem doesn't support nrext64

[Darrick J. Wong]

On Fri, Jul 01, 2022 at 08:51:45AM +1000, Dave Chinner wrote:
> On Wed, Jun 29, 2022 at 04:10:47PM -0700, Darrick J. Wong wrote:
> > On Wed, Jun 29, 2022 at 08:58:57AM +1000, Dave Chinner wrote:
> > > On Tue, Jun 28, 2022 at 01:49:25PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Clear the nrext64 inode flag if the filesystem doesn't have the nrext64
> > > > feature enabled in the superblock.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  repair/dinode.c |   19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > > 
> > > > 
> > > > diff --git a/repair/dinode.c b/repair/dinode.c
> > > > index 00de31fb..547c5833 100644
> > > > --- a/repair/dinode.c
> > > > +++ b/repair/dinode.c
> > > > @@ -2690,6 +2690,25 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
> > > >  			}
> > > >  		}
> > > >  
> > > > +		if (xfs_dinode_has_large_extent_counts(dino) &&
> > > > +		    !xfs_has_large_extent_counts(mp)) {
> > > > +			if (!uncertain) {
> > > > +				do_warn(
> > > > +	_("inode %" PRIu64 " is marked large extent counts but file system does not support large extent counts\n"),
> > > > +					lino);
> > > > +			}
> > > > +			flags2 &= ~XFS_DIFLAG2_NREXT64;
> > > > +
> > > > +			if (no_modify) {
> > > > +				do_warn(_("would zero extent counts.\n"));
> > > > +			} else {
> > > > +				do_warn(_("zeroing extent counts.\n"));
> > > > +				dino->di_nextents = 0;
> > > > +				dino->di_anextents = 0;
> > > > +				*dirty = 1;
> > > 
> > > Is that necessary? If the existing extent counts are within the
> > > bounds of the old extent fields, then shouldn't we just rewrite the
> > > current values into the old format rather than trashing all the
> > > data/xattrs on the inode?
> > 
> > It's hard to know what to do here -- we haven't actually checked the
> > forks yet, so we don't know if the dinode flag was set but the !nrext64
> > extent count fields are ok so all we have to do is clear the dinode
> > flag; or if the dinode flag was set, the nrext64 extent count fields are
> > correct and must be moved to the !nrext64 fields; or what?
> > 
> > I guess I could just leave the extent count fields as-is and let the
> > process_*_fork functions deal with it.
> 
> Can we check it -after- the inode has otherwise been validated
> and do the conversion then?

process_inode_{data,attr}_fork compute the correct extent counts and
process_inode_blocks_and_extents writes them into the appropriate fields
in the xfs_dinode structure, so there's no need to convert anything.

This chunk comes before all that, so all we have to do here is set the
DIFLAG2 state as desired.  Zeroing the extent fields was unnecessary.

--D

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

[PATCH v2 2/6] xfs_repair: clear DIFLAG2_NREXT64 when filesystem doesn't support nrext64

[Darrick J. Wong]

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

Clear the nrext64 inode flag if the filesystem doesn't have the nrext64
feature enabled in the superblock.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v2: remove unnecessary clearing of extent counters, we reset them anyway
---
 repair/dinode.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/repair/dinode.c b/repair/dinode.c
index 00de31fb..7610cd45 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2690,6 +2690,19 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 			}
 		}
 
+		if (xfs_dinode_has_large_extent_counts(dino) &&
+		    !xfs_has_large_extent_counts(mp)) {
+			if (!uncertain) {
+				do_warn(
+	_("inode %" PRIu64 " is marked large extent counts but file system does not support large extent counts\n"),
+					lino);
+			}
+			flags2 &= ~XFS_DIFLAG2_NREXT64;
+
+			if (!no_modify)
+				*dirty = 1;
+		}
+
 		if (!verify_mode && flags2 != be64_to_cpu(dino->di_flags2)) {
 			if (!no_modify) {
 				do_warn(_("fixing bad flags2.\n"));

Re: [PATCH v2 2/6] xfs_repair: clear DIFLAG2_NREXT64 when filesystem doesn't support nrext64

[Dave Chinner]

On Thu, Jun 30, 2022 at 05:12:01PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Clear the nrext64 inode flag if the filesystem doesn't have the nrext64
> feature enabled in the superblock.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> v2: remove unnecessary clearing of extent counters, we reset them anyway
> ---
>  repair/dinode.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 00de31fb..7610cd45 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2690,6 +2690,19 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  			}
>  		}
>  
> +		if (xfs_dinode_has_large_extent_counts(dino) &&
> +		    !xfs_has_large_extent_counts(mp)) {
> +			if (!uncertain) {
> +				do_warn(
> +	_("inode %" PRIu64 " is marked large extent counts but file system does not support large extent counts\n"),
> +					lino);
> +			}
> +			flags2 &= ~XFS_DIFLAG2_NREXT64;
> +
> +			if (!no_modify)
> +				*dirty = 1;
> +		}
> +
>  		if (!verify_mode && flags2 != be64_to_cpu(dino->di_flags2)) {
>  			if (!no_modify) {
>  				do_warn(_("fixing bad flags2.\n"));
> 

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com