[PATCH] v3 inodes are only valid on crc-enabled filesystems

[PATCH] v3 inodes are only valid on crc-enabled filesystems

[Roger Willcocks]

Fix an xfs_repair regression reported by Leslie Rhorer where a bad
(v3) inode version number was not reset.

Signed-off-by: Roger Willcocks <roger@filmlight.ltd.uk>
---
db/check.c             |  2 +-
include/libxfs.h       |  2 +-
include/xfs_dinode.h   | 10 +++++++++-
libxfs/xfs_inode_buf.c |  2 +-
repair/dinode.c        |  7 +++----
repair/prefetch.c      |  2 +-
6 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/db/check.c b/db/check.c
index c4c972f..b5c3b8e 100644
--- a/db/check.c
+++ b/db/check.c
@@ -2637,7 +2637,7 @@ process_inode(
		error++;
		return;
	}
-	if (!XFS_DINODE_GOOD_VERSION(idic.di_version)) {
+	if (!xfs_dinode_good_version(mp, idic.di_version)) {
		if (isfree || v)
			dbprintf(_("bad version number %#x for inode %lld\n"),
				idic.di_version, ino);
diff --git a/include/libxfs.h b/include/libxfs.h
index 962e319..450a38f 100644
--- a/include/libxfs.h
+++ b/include/libxfs.h
@@ -51,7 +51,6 @@
#include <xfs/xfs_alloc_btree.h>
#include <xfs/xfs_ialloc_btree.h>
#include <xfs/xfs_attr_sf.h>
-#include <xfs/xfs_dinode.h>
#include <xfs/xfs_inode_fork.h>
#include <xfs/xfs_inode_buf.h>
#include <xfs/xfs_alloc.h>
@@ -279,6 +278,7 @@ extern void	libxfs_rtmount_destroy (xfs_mount_t *);
 */
#include <xfs/xfs_da_format.h>
#include <xfs/xfs_da_btree.h>
+#include <xfs/xfs_dinode.h>
#include <xfs/xfs_dir2.h>

/*
diff --git a/include/xfs_dinode.h b/include/xfs_dinode.h
index 623bbe8..2079061 100644
--- a/include/xfs_dinode.h
+++ b/include/xfs_dinode.h
@@ -19,7 +19,15 @@
#define	__XFS_DINODE_H__

#define	XFS_DINODE_MAGIC		0x494e	/* 'IN' */
-#define XFS_DINODE_GOOD_VERSION(v)	((v) >= 1 && (v) <= 3)
+
+static inline bool
+xfs_dinode_good_version(struct xfs_mount *mp, __u8 version)
+{
+	if (xfs_sb_version_hascrc(&mp->m_sb))
+		return version == 3;
+
+	return version == 1 || version == 2;
+}

typedef struct xfs_timestamp {
	__be32		t_sec;		/* timestamp seconds */
diff --git a/libxfs/xfs_inode_buf.c b/libxfs/xfs_inode_buf.c
index de16ed9..c79b90b 100644
--- a/libxfs/xfs_inode_buf.c
+++ b/libxfs/xfs_inode_buf.c
@@ -78,7 +78,7 @@ xfs_inode_buf_verify(
		dip = (struct xfs_dinode *)xfs_buf_offset(bp,
					(i << mp->m_sb.sb_inodelog));
		di_ok = dip->di_magic == cpu_to_be16(XFS_DINODE_MAGIC) &&
-			    XFS_DINODE_GOOD_VERSION(dip->di_version);
+			xfs_dinode_good_version(mp, dip->di_version);
		if (unlikely(XFS_TEST_ERROR(!di_ok, mp,
						XFS_ERRTAG_ITOBP_INOTOBP,
						XFS_RANDOM_ITOBP_INOTOBP))) {
diff --git a/repair/dinode.c b/repair/dinode.c
index 035212c..b76066b 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -129,7 +129,7 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num)
		dinoc->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
	}

-	if (!XFS_DINODE_GOOD_VERSION(dinoc->di_version) ||
+	if (!xfs_dinode_good_version(mp, dinoc->di_version) ||
	    (!fs_inode_nlink && dinoc->di_version > 1))  {
		__dirty_no_modify_ret(dirty);
		if (xfs_sb_version_hascrc(&mp->m_sb))
@@ -2331,9 +2331,8 @@ process_dinode_int(xfs_mount_t *mp,
		}
	}

-	if (!XFS_DINODE_GOOD_VERSION(dino->di_version) ||
-	    (!fs_inode_nlink && dino->di_version > 1) ||
-	    (xfs_sb_version_hascrc(&mp->m_sb) && dino->di_version < 3) )  {
+	if (!xfs_dinode_good_version(mp, dino->di_version) ||
+	    (!fs_inode_nlink && dino->di_version > 1)) {
		retval = 1;
		if (!uncertain)
			do_warn(_("bad version number 0x%x on inode %" PRIu64 "%c"),
diff --git a/repair/prefetch.c b/repair/prefetch.c
index 7ea0d36..3631ff5 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -419,7 +419,7 @@ pf_read_inode_dirs(
		if (be16_to_cpu(dino->di_magic) != XFS_DINODE_MAGIC)
			continue;

-		if (!XFS_DINODE_GOOD_VERSION(dino->di_version) ||
+		if (!xfs_dinode_good_version(mp, dino->di_version) ||
				(!fs_inode_nlink && dino->di_version > 1))
			continue;

-- 
2.5.0.rc0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] v3 inodes are only valid on crc-enabled filesystems

[Dave Chinner]

On Sun, Aug 16, 2015 at 12:34:24PM +0100, Roger Willcocks wrote:
> Fix an xfs_repair regression reported by Leslie Rhorer where a bad
> (v3) inode version number was not reset.
> 
> Signed-off-by: Roger Willcocks <roger@filmlight.ltd.uk>

Hi Roger,

Just a FYI - the patch has mangled whitespace:
in it.

> ---
> db/check.c             |  2 +-
> include/libxfs.h       |  2 +-
> include/xfs_dinode.h   | 10 +++++++++-
> libxfs/xfs_inode_buf.c |  2 +-
> repair/dinode.c        |  7 +++----
> repair/prefetch.c      |  2 +-
> 6 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/db/check.c b/db/check.c
> index c4c972f..b5c3b8e 100644
> --- a/db/check.c
> +++ b/db/check.c
> @@ -2637,7 +2637,7 @@ process_inode(
> 		error++;
> 		return;
> 	}
> -	if (!XFS_DINODE_GOOD_VERSION(idic.di_version)) {
> +	if (!xfs_dinode_good_version(mp, idic.di_version)) {
> 		if (isfree || v)
> 			dbprintf(_("bad version number %#x for inode %lld\n"),
> 				idic.di_version, ino);

All the patch hunks are missing the leading " " for non-modified
lines. I've fixed it manually, but can you make sure future patches
are not mangled?

> -- 
> 2.5.0.rc0

IMO, it's best not to use development versions of git for anything
other than testing.... ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH] v3 inodes are only valid on crc-enabled filesystems.

[Roger Willcocks]

Fix an xfs_repair regression reported by Leslie Rhorer where a bad
(v3) inode version number was not reset.

Signed-off-by: Roger Willcocks <roger@filmlight.ltd.uk>
---
db/check.c             | 2 +-
include/xfs_dinode.h   | 3 ++-
libxfs/xfs_inode_buf.c | 2 +-
repair/dinode.c        | 7 +++----
repair/prefetch.c      | 2 +-
5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/db/check.c b/db/check.c
index c4c972f..810fa55 100644
--- a/db/check.c
+++ b/db/check.c
@@ -2637,7 +2637,7 @@ process_inode(
		error++;
		return;
	}
-	if (!XFS_DINODE_GOOD_VERSION(idic.di_version)) {
+	if (!XFS_DINODE_GOOD_VERSION(&mp->m_sb, idic.di_version)) {
		if (isfree || v)
			dbprintf(_("bad version number %#x for inode %lld\n"),
				idic.di_version, ino);
diff --git a/include/xfs_dinode.h b/include/xfs_dinode.h
index 623bbe8..40700e6 100644
--- a/include/xfs_dinode.h
+++ b/include/xfs_dinode.h
@@ -19,7 +19,8 @@
#define	__XFS_DINODE_H__

#define	XFS_DINODE_MAGIC		0x494e	/* 'IN' */
-#define XFS_DINODE_GOOD_VERSION(v)	((v) >= 1 && (v) <= 3)
+#define XFS_DINODE_GOOD_VERSION(sb, v) \
+	(xfs_sb_version_hascrc(sb) ? ((v) == 3) : ((v) == 1 || (v) == 2))

typedef struct xfs_timestamp {
	__be32		t_sec;		/* timestamp seconds */
diff --git a/libxfs/xfs_inode_buf.c b/libxfs/xfs_inode_buf.c
index de16ed9..e9cc74c 100644
--- a/libxfs/xfs_inode_buf.c
+++ b/libxfs/xfs_inode_buf.c
@@ -78,7 +78,7 @@ xfs_inode_buf_verify(
		dip = (struct xfs_dinode *)xfs_buf_offset(bp,
					(i << mp->m_sb.sb_inodelog));
		di_ok = dip->di_magic == cpu_to_be16(XFS_DINODE_MAGIC) &&
-			    XFS_DINODE_GOOD_VERSION(dip->di_version);
+			XFS_DINODE_GOOD_VERSION(&mp->m_sb, dip->di_version);
		if (unlikely(XFS_TEST_ERROR(!di_ok, mp,
						XFS_ERRTAG_ITOBP_INOTOBP,
						XFS_RANDOM_ITOBP_INOTOBP))) {
diff --git a/repair/dinode.c b/repair/dinode.c
index 035212c..29a7a19 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -129,7 +129,7 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num)
		dinoc->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
	}

-	if (!XFS_DINODE_GOOD_VERSION(dinoc->di_version) ||
+	if (!XFS_DINODE_GOOD_VERSION(&mp->m_sb, dinoc->di_version) ||
	    (!fs_inode_nlink && dinoc->di_version > 1))  {
		__dirty_no_modify_ret(dirty);
		if (xfs_sb_version_hascrc(&mp->m_sb))
@@ -2331,9 +2331,8 @@ process_dinode_int(xfs_mount_t *mp,
		}
	}

-	if (!XFS_DINODE_GOOD_VERSION(dino->di_version) ||
-	    (!fs_inode_nlink && dino->di_version > 1) ||
-	    (xfs_sb_version_hascrc(&mp->m_sb) && dino->di_version < 3) )  {
+	if (!XFS_DINODE_GOOD_VERSION(&mp->m_sb, dino->di_version) ||
+	    (!fs_inode_nlink && dino->di_version > 1) ) {
		retval = 1;
		if (!uncertain)
			do_warn(_("bad version number 0x%x on inode %" PRIu64 "%c"),
diff --git a/repair/prefetch.c b/repair/prefetch.c
index 7ea0d36..ad7082c 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -419,7 +419,7 @@ pf_read_inode_dirs(
		if (be16_to_cpu(dino->di_magic) != XFS_DINODE_MAGIC)
			continue;

-		if (!XFS_DINODE_GOOD_VERSION(dino->di_version) ||
+		if (!XFS_DINODE_GOOD_VERSION(&mp->m_sb, dino->di_version) ||
				(!fs_inode_nlink && dino->di_version > 1))
			continue;

-- 
2.5.0.rc0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] v3 inodes are only valid on crc-enabled filesystems.

[Roger Willcocks]

With this patch, Leslie’s filesystem now reports a Metadata corruption as well as an invalid inode:

Phase 3 - for each AG...
        - scan and clear agi unlinked lists...
        - 00:41:25: scanning agi unlinked lists - 32 of 32 allocation groups done
        - process known inodes and perform inode discovery...
        - agno = 0
...
        - agno = 29
Metadata corruption detected at block 0x9e7833810/0x2000
        - agno = 11
bad version number 0x3 on inode 124656869424
bad version number 0x3 on inode 124656869424, resetting version number
        - agno = 12
        - agno = 13
        - agno = 14
        - 00:41:26: process known inodes and inode discovery - 42368 of 42368 inodes done

—
Roger


On 16 Aug 2015, at 01:14, Roger Willcocks <roger@filmlight.ltd.uk> wrote:

> Fix an xfs_repair regression reported by Leslie Rhorer where a bad
> (v3) inode version number was not reset.
> 
> Signed-off-by: Roger Willcocks <roger@filmlight.ltd.uk>
> ---
> db/check.c             | 2 +-
> include/xfs_dinode.h   | 3 ++-
> libxfs/xfs_inode_buf.c | 2 +-
> repair/dinode.c        | 7 +++----
> repair/prefetch.c      | 2 +-
> 5 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/db/check.c b/db/check.c
> index c4c972f..810fa55 100644
> --- a/db/check.c
> +++ b/db/check.c
> @@ -2637,7 +2637,7 @@ process_inode(
> 		error++;
> 		return;
> 	}
> -	if (!XFS_DINODE_GOOD_VERSION(idic.di_version)) {
> +	if (!XFS_DINODE_GOOD_VERSION(&mp->m_sb, idic.di_version)) {
> 		if (isfree || v)
> 			dbprintf(_("bad version number %#x for inode %lld\n"),
> 				idic.di_version, ino);
> diff --git a/include/xfs_dinode.h b/include/xfs_dinode.h
> index 623bbe8..40700e6 100644
> --- a/include/xfs_dinode.h
> +++ b/include/xfs_dinode.h
> @@ -19,7 +19,8 @@
> #define	__XFS_DINODE_H__
> 
> #define	XFS_DINODE_MAGIC		0x494e	/* 'IN' */
> -#define XFS_DINODE_GOOD_VERSION(v)	((v) >= 1 && (v) <= 3)
> +#define XFS_DINODE_GOOD_VERSION(sb, v) \
> +	(xfs_sb_version_hascrc(sb) ? ((v) == 3) : ((v) == 1 || (v) == 2))
> 
> typedef struct xfs_timestamp {
> 	__be32		t_sec;		/* timestamp seconds */
> diff --git a/libxfs/xfs_inode_buf.c b/libxfs/xfs_inode_buf.c
> index de16ed9..e9cc74c 100644
> --- a/libxfs/xfs_inode_buf.c
> +++ b/libxfs/xfs_inode_buf.c
> @@ -78,7 +78,7 @@ xfs_inode_buf_verify(
> 		dip = (struct xfs_dinode *)xfs_buf_offset(bp,
> 					(i << mp->m_sb.sb_inodelog));
> 		di_ok = dip->di_magic == cpu_to_be16(XFS_DINODE_MAGIC) &&
> -			    XFS_DINODE_GOOD_VERSION(dip->di_version);
> +			XFS_DINODE_GOOD_VERSION(&mp->m_sb, dip->di_version);
> 		if (unlikely(XFS_TEST_ERROR(!di_ok, mp,
> 						XFS_ERRTAG_ITOBP_INOTOBP,
> 						XFS_RANDOM_ITOBP_INOTOBP))) {
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 035212c..29a7a19 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -129,7 +129,7 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num)
> 		dinoc->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
> 	}
> 
> -	if (!XFS_DINODE_GOOD_VERSION(dinoc->di_version) ||
> +	if (!XFS_DINODE_GOOD_VERSION(&mp->m_sb, dinoc->di_version) ||
> 	    (!fs_inode_nlink && dinoc->di_version > 1))  {
> 		__dirty_no_modify_ret(dirty);
> 		if (xfs_sb_version_hascrc(&mp->m_sb))
> @@ -2331,9 +2331,8 @@ process_dinode_int(xfs_mount_t *mp,
> 		}
> 	}
> 
> -	if (!XFS_DINODE_GOOD_VERSION(dino->di_version) ||
> -	    (!fs_inode_nlink && dino->di_version > 1) ||
> -	    (xfs_sb_version_hascrc(&mp->m_sb) && dino->di_version < 3) )  {
> +	if (!XFS_DINODE_GOOD_VERSION(&mp->m_sb, dino->di_version) ||
> +	    (!fs_inode_nlink && dino->di_version > 1) ) {
> 		retval = 1;
> 		if (!uncertain)
> 			do_warn(_("bad version number 0x%x on inode %" PRIu64 "%c"),
> diff --git a/repair/prefetch.c b/repair/prefetch.c
> index 7ea0d36..ad7082c 100644
> --- a/repair/prefetch.c
> +++ b/repair/prefetch.c
> @@ -419,7 +419,7 @@ pf_read_inode_dirs(
> 		if (be16_to_cpu(dino->di_magic) != XFS_DINODE_MAGIC)
> 			continue;
> 
> -		if (!XFS_DINODE_GOOD_VERSION(dino->di_version) ||
> +		if (!XFS_DINODE_GOOD_VERSION(&mp->m_sb, dino->di_version) ||
> 				(!fs_inode_nlink && dino->di_version > 1))
> 			continue;
> 
> -- 
> 2.5.0.rc0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] v3 inodes are only valid on crc-enabled filesystems.

[Dave Chinner]

On Sun, Aug 16, 2015 at 01:14:55AM +0100, Roger Willcocks wrote:
> Fix an xfs_repair regression reported by Leslie Rhorer where a bad
> (v3) inode version number was not reset.
> 
> Signed-off-by: Roger Willcocks <roger@filmlight.ltd.uk>
> ---
> db/check.c             | 2 +-
> include/xfs_dinode.h   | 3 ++-
> libxfs/xfs_inode_buf.c | 2 +-
> repair/dinode.c        | 7 +++----
> repair/prefetch.c      | 2 +-
> 5 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/db/check.c b/db/check.c
> index c4c972f..810fa55 100644
> --- a/db/check.c
> +++ b/db/check.c
> @@ -2637,7 +2637,7 @@ process_inode(
> 		error++;
> 		return;
> 	}
> -	if (!XFS_DINODE_GOOD_VERSION(idic.di_version)) {
> +	if (!XFS_DINODE_GOOD_VERSION(&mp->m_sb, idic.di_version)) {
> 		if (isfree || v)
> 			dbprintf(_("bad version number %#x for inode %lld\n"),
> 				idic.di_version, ino);
> diff --git a/include/xfs_dinode.h b/include/xfs_dinode.h
> index 623bbe8..40700e6 100644
> --- a/include/xfs_dinode.h
> +++ b/include/xfs_dinode.h
> @@ -19,7 +19,8 @@
> #define	__XFS_DINODE_H__
> 
> #define	XFS_DINODE_MAGIC		0x494e	/* 'IN' */
> -#define XFS_DINODE_GOOD_VERSION(v)	((v) >= 1 && (v) <= 3)
> +#define XFS_DINODE_GOOD_VERSION(sb, v) \
> +	(xfs_sb_version_hascrc(sb) ? ((v) == 3) : ((v) == 1 || (v) == 2))

I'd make this a static inline function so it gets type checking and
it is easier to understand the logic at a glance. i.e. something
like:

static inline bool
xfs_dinode_good_version(struct xfs_mount *mp, __uint8_t version)
{
	if (version < 1 || version > 3)
		return false;
	if (xfs_sb_version_hascrc(&mp->m_sb)) {
		if (version != 3)
			return false;
	} else if (version > 2)
		return false;

	return true;
}

Otherwise looks fine.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] v3 inodes are only valid on crc-enabled filesystems.

[Roger Willcocks]

On 16 Aug 2015, at 01:42, Dave Chinner <david@fromorbit.com> wrote:

> On Sun, Aug 16, 2015 at 01:14:55AM +0100, Roger Willcocks wrote:
>> Fix an xfs_repair regression reported by Leslie Rhorer where a bad
>> (v3) inode version number was not reset.
>> 
>> Signed-off-by: Roger Willcocks <roger@filmlight.ltd.uk>
>> 
>> -#define XFS_DINODE_GOOD_VERSION(v)	((v) >= 1 && (v) <= 3)
>> +#define XFS_DINODE_GOOD_VERSION(sb, v) \
>> +	(xfs_sb_version_hascrc(sb) ? ((v) == 3) : ((v) == 1 || (v) == 2))
> 
> I'd make this a static inline function so it gets type checking and
> it is easier to understand the logic at a glance. i.e. something
> like:
> 
> static inline bool
> xfs_dinode_good_version(struct xfs_mount *mp, __uint8_t version)
> {
> 	if (version < 1 || version > 3)
> 		return false;
> 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> 		if (version != 3)
> 			return false;
> 	} else if (version > 2)
> 		return false;
> 
> 	return true;
> }
> 

include of xfs_dinode.h has to be moved to after the definition of
xfs_mount in libxfs.h, but that’s okay following the pattern set by
xfs_da_format.h.

Updated patch follows.

—
Roger

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs