[PATCH 0/9] current series for verifier error differentiation

[PATCH 0/9] current series for verifier error differentiation

[Eric Sandeen]

This is the current series I have leading up to verifier error differentiation,
just resending them all.

As I send this, I remember that EILSEQ is giving us an unusual perror()
output, so the last patch which uses it may need to pick some (?)
other error code...

-Eric

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

[PATCH 1/9] xfs: skip verification on initial "guess" superblock read

[Eric Sandeen]

When xfs_readsb() does the very first read of the superblock,
it makes a guess at the length of the buffer, based on the
sector size of the underlying storage.  This may or may
not match the filesystem sector size in sb_sectsize, so
we can't i.e. do a CRC check on it; it might be too short.

In fact, mounting a filesystem with sb_sectsize larger
than the device sector size will cause a mount failure
if CRCs are enabled, because we are checksumming a length
which exceeds the buffer passed to it.

So always read twice; the first time we read with NULL
buffer ops to skip verification; then set the proper
read length, hook up the proper verifier, and give it
another go.

Once we are sure that we've got the right buffer length,
we can also use bp->b_length in the xfs_sb_read_verify,
rather than the less-trusted on-disk sectorsize for
secondary superblocks.  Before this we ran the risk of
passing junk to the crc32c routines, which didn't always
handle extreme values.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_mount.c |   24 ++++++++++++++++--------
 fs/xfs/xfs_sb.c    |    3 +--
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 02df7b4..f96c056 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -282,22 +282,29 @@ xfs_readsb(
 	struct xfs_sb	*sbp = &mp->m_sb;
 	int		error;
 	int		loud = !(flags & XFS_MFSI_QUIET);
+	const struct xfs_buf_ops *buf_ops;
 
 	ASSERT(mp->m_sb_bp == NULL);
 	ASSERT(mp->m_ddev_targp != NULL);
 
 	/*
+	 * For the initial read, we must guess at the sector
+	 * size based on the block device.  It's enough to
+	 * get the sb_sectsize out of the superblock and
+	 * then reread with the proper length.
+	 * We don't verify it yet, because it may not be complete.
+	 */
+	sector_size = xfs_getsize_buftarg(mp->m_ddev_targp);
+	buf_ops = NULL;
+
+	/*
 	 * Allocate a (locked) buffer to hold the superblock.
 	 * This will be kept around at all times to optimize
 	 * access to the superblock.
 	 */
-	sector_size = xfs_getsize_buftarg(mp->m_ddev_targp);
-
 reread:
 	bp = xfs_buf_read_uncached(mp->m_ddev_targp, XFS_SB_DADDR,
-				   BTOBB(sector_size), 0,
-				   loud ? &xfs_sb_buf_ops
-				        : &xfs_sb_quiet_buf_ops);
+				   BTOBB(sector_size), 0, buf_ops);
 	if (!bp) {
 		if (loud)
 			xfs_warn(mp, "SB buffer read failed");
@@ -328,12 +335,13 @@ reread:
 	}
 
 	/*
-	 * If device sector size is smaller than the superblock size,
-	 * re-read the superblock so the buffer is correctly sized.
+	 * Re-read the superblock so the buffer is correctly sized,
+	 * and properly verified.
 	 */
-	if (sector_size < sbp->sb_sectsize) {
+	if (buf_ops == NULL) {
 		xfs_buf_relse(bp);
 		sector_size = sbp->sb_sectsize;
+		buf_ops = loud ? &xfs_sb_buf_ops : &xfs_sb_quiet_buf_ops;
 		goto reread;
 	}
 
diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
index 5071ccb..359b19a 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -611,7 +611,7 @@ xfs_sb_read_verify(
 						XFS_SB_VERSION_5) ||
 	     dsb->sb_crc != 0)) {
 
-		if (!xfs_verify_cksum(bp->b_addr, be16_to_cpu(dsb->sb_sectsize),
+		if (!xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
 				      offsetof(struct xfs_sb, sb_crc))) {
 			/* Only fail bad secondaries on a known V5 filesystem */
 			if (bp->b_bn == XFS_SB_DADDR ||
@@ -644,7 +644,6 @@ xfs_sb_quiet_read_verify(
 {
 	struct xfs_dsb	*dsb = XFS_BUF_TO_SBP(bp);
 
-
 	if (dsb->sb_magicnum == cpu_to_be32(XFS_SB_MAGIC)) {
 		/* XFS filesystem, verify noisily! */
 		xfs_sb_read_verify(bp);
-- 
1.7.1

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

Re: [PATCH 1/9] xfs: skip verification on initial "guess" superblock read

[Dave Chinner]

On Tue, Feb 18, 2014 at 05:52:21PM -0600, Eric Sandeen wrote:
> When xfs_readsb() does the very first read of the superblock,
> it makes a guess at the length of the buffer, based on the
> sector size of the underlying storage.  This may or may
> not match the filesystem sector size in sb_sectsize, so
> we can't i.e. do a CRC check on it; it might be too short.
> 
> In fact, mounting a filesystem with sb_sectsize larger
> than the device sector size will cause a mount failure
> if CRCs are enabled, because we are checksumming a length
> which exceeds the buffer passed to it.
> 
> So always read twice; the first time we read with NULL
> buffer ops to skip verification; then set the proper
> read length, hook up the proper verifier, and give it
> another go.
> 
> Once we are sure that we've got the right buffer length,
> we can also use bp->b_length in the xfs_sb_read_verify,
> rather than the less-trusted on-disk sectorsize for
> secondary superblocks.  Before this we ran the risk of
> passing junk to the crc32c routines, which didn't always
> handle extreme values.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks good.

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


-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 2/9] xfs: limit superblock corruption errors to actual corruption

[Eric Sandeen]

Today, if

xfs_sb_read_verify
  xfs_sb_verify
    xfs_mount_validate_sb

detects superblock corruption, it'll be extremely noisy, dumping
2 stacks, 2 hexdumps, etc.

This is because we call XFS_CORRUPTION_ERROR in xfs_mount_validate_sb
as well as in xfs_sb_read_verify.

Also, *any* errors in xfs_mount_validate_sb which are not corruption
per se; things like too-big-blocksize, bad version, bad magic, v1 dirs,
rw-incompat etc - things which do not return EFSCORRUPTED - will
still do the whole XFS_CORRUPTION_ERROR spew when xfs_sb_read_verify
sees any error at all.  And it suggests to the user that they
should run xfs_repair, even if the root cause of the mount failure
is a simple incompatibility.

I'll submit that the probably-not-corrupted errors don't warrant
this much noise, so this patch removes the warning for anything
other than EFSCORRUPTED returns, and replaces the lower-level
XFS_CORRUPTION_ERROR with an xfs_notice().

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_sb.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
index 359b19a..1e11679 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -295,8 +295,7 @@ xfs_mount_validate_sb(
 	    sbp->sb_dblocks == 0					||
 	    sbp->sb_dblocks > XFS_MAX_DBLOCKS(sbp)			||
 	    sbp->sb_dblocks < XFS_MIN_DBLOCKS(sbp))) {
-		XFS_CORRUPTION_ERROR("SB sanity check failed",
-				XFS_ERRLEVEL_LOW, mp, sbp);
+		xfs_notice(mp, "SB sanity check failed");
 		return XFS_ERROR(EFSCORRUPTED);
 	}
 
@@ -625,7 +624,7 @@ xfs_sb_read_verify(
 
 out_error:
 	if (error) {
-		if (error != EWRONGFS)
+		if (error == EFSCORRUPTED)
 			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
 					     mp, bp->b_addr);
 		xfs_buf_ioerror(bp, error);
-- 
1.7.1

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

Re: [PATCH 2/9] xfs: limit superblock corruption errors to actual corruption

[Dave Chinner]

On Tue, Feb 18, 2014 at 05:52:22PM -0600, Eric Sandeen wrote:
> Today, if
> 
> xfs_sb_read_verify
>   xfs_sb_verify
>     xfs_mount_validate_sb
> 
> detects superblock corruption, it'll be extremely noisy, dumping
> 2 stacks, 2 hexdumps, etc.
> 
> This is because we call XFS_CORRUPTION_ERROR in xfs_mount_validate_sb
> as well as in xfs_sb_read_verify.
> 
> Also, *any* errors in xfs_mount_validate_sb which are not corruption
> per se; things like too-big-blocksize, bad version, bad magic, v1 dirs,
> rw-incompat etc - things which do not return EFSCORRUPTED - will
> still do the whole XFS_CORRUPTION_ERROR spew when xfs_sb_read_verify
> sees any error at all.  And it suggests to the user that they
> should run xfs_repair, even if the root cause of the mount failure
> is a simple incompatibility.
> 
> I'll submit that the probably-not-corrupted errors don't warrant
> this much noise, so this patch removes the warning for anything
> other than EFSCORRUPTED returns, and replaces the lower-level
> XFS_CORRUPTION_ERROR with an xfs_notice().
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Agreed. Looks good.

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

-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 3/9] xfs: skip pointless CRC updates after verifier failures

[Eric Sandeen]

Most write verifiers don't update CRCs after the verifier
has failed and the buffer has been marked in error.  These
two didn't, but should.

Add returns to the verifier failure block,
since the buffer won't be written anyway.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_alloc_btree.c  |    1 +
 fs/xfs/xfs_ialloc_btree.c |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_alloc_btree.c b/fs/xfs/xfs_alloc_btree.c
index 1308542..144d3b0 100644
--- a/fs/xfs/xfs_alloc_btree.c
+++ b/fs/xfs/xfs_alloc_btree.c
@@ -373,6 +373,7 @@ xfs_allocbt_write_verify(
 		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
 				     bp->b_target->bt_mount, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		return;
 	}
 	xfs_btree_sblock_calc_crc(bp);
 
diff --git a/fs/xfs/xfs_ialloc_btree.c b/fs/xfs/xfs_ialloc_btree.c
index c8fa5bb..0028c50 100644
--- a/fs/xfs/xfs_ialloc_btree.c
+++ b/fs/xfs/xfs_ialloc_btree.c
@@ -261,6 +261,7 @@ xfs_inobt_write_verify(
 		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
 				     bp->b_target->bt_mount, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		return;
 	}
 	xfs_btree_sblock_calc_crc(bp);
 
-- 
1.7.1

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

Re: [PATCH 3/9] xfs: skip pointless CRC updates after verifier failures

[Jeff Liu]

On 02/19 2014 07:52 AM, Eric Sandeen wrote:
> Most write verifiers don't update CRCs after the verifier
> has failed and the buffer has been marked in error.  These
> two didn't, but should.
> 
> Add returns to the verifier failure block,
> since the buffer won't be written anyway.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/xfs_alloc_btree.c  |    1 +
>  fs/xfs/xfs_ialloc_btree.c |    1 +
>  2 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/xfs/xfs_alloc_btree.c b/fs/xfs/xfs_alloc_btree.c
> index 1308542..144d3b0 100644
> --- a/fs/xfs/xfs_alloc_btree.c
> +++ b/fs/xfs/xfs_alloc_btree.c
> @@ -373,6 +373,7 @@ xfs_allocbt_write_verify(
>  		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
>  				     bp->b_target->bt_mount, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		return;
>  	}
>  	xfs_btree_sblock_calc_crc(bp);
>  
> diff --git a/fs/xfs/xfs_ialloc_btree.c b/fs/xfs/xfs_ialloc_btree.c
> index c8fa5bb..0028c50 100644
> --- a/fs/xfs/xfs_ialloc_btree.c
> +++ b/fs/xfs/xfs_ialloc_btree.c
> @@ -261,6 +261,7 @@ xfs_inobt_write_verify(
>  		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
>  				     bp->b_target->bt_mount, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		return;
>  	}
>  	xfs_btree_sblock_calc_crc(bp);

Looks good to me.

Reviewed-by: Jie Liu <jeff.liu@oracle.com>


Thanks,
-Jeff

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

[PATCH 4/9] xfs: Use defines for CRC offsets in all cases

[Eric Sandeen]

Some calls to crc functions used useful #defines,
others used awkward offsetof() constructs.

Switch them all to #define to make things a bit
cleaner.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_ag.h             |    6 ++++++
 fs/xfs/xfs_alloc.c          |   10 ++++------
 fs/xfs/xfs_dinode.h         |    2 ++
 fs/xfs/xfs_format.h         |    2 ++
 fs/xfs/xfs_ialloc.c         |    5 ++---
 fs/xfs/xfs_inode_buf.c      |    4 ++--
 fs/xfs/xfs_sb.c             |    5 ++---
 fs/xfs/xfs_sb.h             |    2 ++
 fs/xfs/xfs_symlink_remote.c |    5 ++---
 9 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_ag.h b/fs/xfs/xfs_ag.h
index 3fc1098..0fdd410 100644
--- a/fs/xfs/xfs_ag.h
+++ b/fs/xfs/xfs_ag.h
@@ -89,6 +89,8 @@ typedef struct xfs_agf {
 	/* structure must be padded to 64 bit alignment */
 } xfs_agf_t;
 
+#define XFS_AGF_CRC_OFF		offsetof(struct xfs_agf, agf_crc)
+
 #define	XFS_AGF_MAGICNUM	0x00000001
 #define	XFS_AGF_VERSIONNUM	0x00000002
 #define	XFS_AGF_SEQNO		0x00000004
@@ -167,6 +169,8 @@ typedef struct xfs_agi {
 	/* structure must be padded to 64 bit alignment */
 } xfs_agi_t;
 
+#define XFS_AGI_CRC_OFF		offsetof(struct xfs_agi, agi_crc)
+
 #define	XFS_AGI_MAGICNUM	0x00000001
 #define	XFS_AGI_VERSIONNUM	0x00000002
 #define	XFS_AGI_SEQNO		0x00000004
@@ -222,6 +226,8 @@ typedef struct xfs_agfl {
 	__be32		agfl_bno[];	/* actually XFS_AGFL_SIZE(mp) */
 } xfs_agfl_t;
 
+#define XFS_AGFL_CRC_OFF	offsetof(struct xfs_agfl, agfl_crc)
+
 /*
  * tags for inode radix tree
  */
diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
index 9eab2df..72ea855 100644
--- a/fs/xfs/xfs_alloc.c
+++ b/fs/xfs/xfs_alloc.c
@@ -486,7 +486,7 @@ xfs_agfl_read_verify(
 		return;
 
 	agfl_ok = xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-				   offsetof(struct xfs_agfl, agfl_crc));
+				   XFS_AGFL_CRC_OFF);
 
 	agfl_ok = agfl_ok && xfs_agfl_verify(bp);
 
@@ -516,8 +516,7 @@ xfs_agfl_write_verify(
 	if (bip)
 		XFS_BUF_TO_AGFL(bp)->agfl_lsn = cpu_to_be64(bip->bli_item.li_lsn);
 
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length),
-			 offsetof(struct xfs_agfl, agfl_crc));
+	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_AGFL_CRC_OFF);
 }
 
 const struct xfs_buf_ops xfs_agfl_buf_ops = {
@@ -2242,7 +2241,7 @@ xfs_agf_read_verify(
 
 	if (xfs_sb_version_hascrc(&mp->m_sb))
 		agf_ok = xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-					  offsetof(struct xfs_agf, agf_crc));
+					  XFS_AGF_CRC_OFF);
 
 	agf_ok = agf_ok && xfs_agf_verify(mp, bp);
 
@@ -2272,8 +2271,7 @@ xfs_agf_write_verify(
 	if (bip)
 		XFS_BUF_TO_AGF(bp)->agf_lsn = cpu_to_be64(bip->bli_item.li_lsn);
 
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length),
-			 offsetof(struct xfs_agf, agf_crc));
+	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_AGF_CRC_OFF);
 }
 
 const struct xfs_buf_ops xfs_agf_buf_ops = {
diff --git a/fs/xfs/xfs_dinode.h b/fs/xfs/xfs_dinode.h
index e5869b5..623bbe8 100644
--- a/fs/xfs/xfs_dinode.h
+++ b/fs/xfs/xfs_dinode.h
@@ -89,6 +89,8 @@ typedef struct xfs_dinode {
 	/* structure must be padded to 64 bit alignment */
 } xfs_dinode_t;
 
+#define XFS_DINODE_CRC_OFF	offsetof(struct xfs_dinode, di_crc)
+
 #define DI_MAX_FLUSH 0xffff
 
 /*
diff --git a/fs/xfs/xfs_format.h b/fs/xfs/xfs_format.h
index b6ab5a3..9898f31 100644
--- a/fs/xfs/xfs_format.h
+++ b/fs/xfs/xfs_format.h
@@ -145,6 +145,8 @@ struct xfs_dsymlink_hdr {
 	__be64	sl_lsn;
 };
 
+#define XFS_SYMLINK_CRC_OFF	offsetof(struct xfs_dsymlink_hdr, sl_crc)
+
 /*
  * The maximum pathlen is 1024 bytes. Since the minimum file system
  * blocksize is 512 bytes, we can get a max of 3 extents back from
diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
index 5d7f105..d79210b 100644
--- a/fs/xfs/xfs_ialloc.c
+++ b/fs/xfs/xfs_ialloc.c
@@ -1572,7 +1572,7 @@ xfs_agi_read_verify(
 
 	if (xfs_sb_version_hascrc(&mp->m_sb))
 		agi_ok = xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-					  offsetof(struct xfs_agi, agi_crc));
+					  XFS_AGI_CRC_OFF);
 	agi_ok = agi_ok && xfs_agi_verify(bp);
 
 	if (unlikely(XFS_TEST_ERROR(!agi_ok, mp, XFS_ERRTAG_IALLOC_READ_AGI,
@@ -1600,8 +1600,7 @@ xfs_agi_write_verify(
 
 	if (bip)
 		XFS_BUF_TO_AGI(bp)->agi_lsn = cpu_to_be64(bip->bli_item.li_lsn);
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length),
-			 offsetof(struct xfs_agi, agi_crc));
+	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_AGI_CRC_OFF);
 }
 
 const struct xfs_buf_ops xfs_agi_buf_ops = {
diff --git a/fs/xfs/xfs_inode_buf.c b/fs/xfs/xfs_inode_buf.c
index 4fc9f39..606b43a 100644
--- a/fs/xfs/xfs_inode_buf.c
+++ b/fs/xfs/xfs_inode_buf.c
@@ -306,7 +306,7 @@ xfs_dinode_verify(
 	if (!xfs_sb_version_hascrc(&mp->m_sb))
 		return false;
 	if (!xfs_verify_cksum((char *)dip, mp->m_sb.sb_inodesize,
-			      offsetof(struct xfs_dinode, di_crc)))
+			      XFS_DINODE_CRC_OFF))
 		return false;
 	if (be64_to_cpu(dip->di_ino) != ip->i_ino)
 		return false;
@@ -327,7 +327,7 @@ xfs_dinode_calc_crc(
 
 	ASSERT(xfs_sb_version_hascrc(&mp->m_sb));
 	crc = xfs_start_cksum((char *)dip, mp->m_sb.sb_inodesize,
-			      offsetof(struct xfs_dinode, di_crc));
+			      XFS_DINODE_CRC_OFF);
 	dip->di_crc = xfs_end_cksum(crc);
 }
 
diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
index 1e11679..1ea7c86 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -611,7 +611,7 @@ xfs_sb_read_verify(
 	     dsb->sb_crc != 0)) {
 
 		if (!xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-				      offsetof(struct xfs_sb, sb_crc))) {
+				      XFS_SB_CRC_OFF)) {
 			/* Only fail bad secondaries on a known V5 filesystem */
 			if (bp->b_bn == XFS_SB_DADDR ||
 			    xfs_sb_version_hascrc(&mp->m_sb)) {
@@ -674,8 +674,7 @@ xfs_sb_write_verify(
 	if (bip)
 		XFS_BUF_TO_SBP(bp)->sb_lsn = cpu_to_be64(bip->bli_item.li_lsn);
 
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length),
-			 offsetof(struct xfs_sb, sb_crc));
+	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_SB_CRC_OFF);
 }
 
 const struct xfs_buf_ops xfs_sb_buf_ops = {
diff --git a/fs/xfs/xfs_sb.h b/fs/xfs/xfs_sb.h
index 35061d4..f7b2fe7 100644
--- a/fs/xfs/xfs_sb.h
+++ b/fs/xfs/xfs_sb.h
@@ -182,6 +182,8 @@ typedef struct xfs_sb {
 	/* must be padded to 64 bit alignment */
 } xfs_sb_t;
 
+#define XFS_SB_CRC_OFF		offsetof(struct xfs_sb, sb_crc)
+
 /*
  * Superblock - on disk version.  Must match the in core version above.
  * Must be padded to 64 bit alignment.
diff --git a/fs/xfs/xfs_symlink_remote.c b/fs/xfs/xfs_symlink_remote.c
index bf59a2b..7a705a4 100644
--- a/fs/xfs/xfs_symlink_remote.c
+++ b/fs/xfs/xfs_symlink_remote.c
@@ -134,7 +134,7 @@ xfs_symlink_read_verify(
 		return;
 
 	if (!xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-				  offsetof(struct xfs_dsymlink_hdr, sl_crc)) ||
+				  XFS_SYMLINK_CRC_OFF) ||
 	    !xfs_symlink_verify(bp)) {
 		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
@@ -162,8 +162,7 @@ xfs_symlink_write_verify(
 		struct xfs_dsymlink_hdr *dsl = bp->b_addr;
 		dsl->sl_lsn = cpu_to_be64(bip->bli_item.li_lsn);
 	}
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length),
-			 offsetof(struct xfs_dsymlink_hdr, sl_crc));
+	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_SYMLINK_CRC_OFF);
 }
 
 const struct xfs_buf_ops xfs_symlink_buf_ops = {
-- 
1.7.1

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

Re: [PATCH 4/9] xfs: Use defines for CRC offsets in all cases

[Jeff Liu]

Hi Eric,

I read the previous comments from Dave about using defines for CRC offsets,
and with a grep search after applying this patch, looks there have another
two places maybe we should switch them to the macros as well:

fs/xfs/xfs_log.c:
Do we need a log record crc offset macros for offsetof(struct xlog_rec_header, h_crc))?

xfs_dinode.h:
we added the XFS_DINODE_CRC_OFF, just use it at below routine?

static inline uint xfs_dinode_size(int version)
{
        if (version == 3)
                return sizeof(struct xfs_dinode);
        return offsetof(struct xfs_dinode, di_crc);
}


Thanks,
-Jeff

On 02/19 2014 07:52 AM, Eric Sandeen wrote:
> Some calls to crc functions used useful #defines,
> others used awkward offsetof() constructs.
> 
> Switch them all to #define to make things a bit
> cleaner.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/xfs_ag.h             |    6 ++++++
>  fs/xfs/xfs_alloc.c          |   10 ++++------
>  fs/xfs/xfs_dinode.h         |    2 ++
>  fs/xfs/xfs_format.h         |    2 ++
>  fs/xfs/xfs_ialloc.c         |    5 ++---
>  fs/xfs/xfs_inode_buf.c      |    4 ++--
>  fs/xfs/xfs_sb.c             |    5 ++---
>  fs/xfs/xfs_sb.h             |    2 ++
>  fs/xfs/xfs_symlink_remote.c |    5 ++---
>  9 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ag.h b/fs/xfs/xfs_ag.h
> index 3fc1098..0fdd410 100644
> --- a/fs/xfs/xfs_ag.h
> +++ b/fs/xfs/xfs_ag.h
> @@ -89,6 +89,8 @@ typedef struct xfs_agf {
>  	/* structure must be padded to 64 bit alignment */
>  } xfs_agf_t;
>  
> +#define XFS_AGF_CRC_OFF		offsetof(struct xfs_agf, agf_crc)
> +
>  #define	XFS_AGF_MAGICNUM	0x00000001
>  #define	XFS_AGF_VERSIONNUM	0x00000002
>  #define	XFS_AGF_SEQNO		0x00000004
> @@ -167,6 +169,8 @@ typedef struct xfs_agi {
>  	/* structure must be padded to 64 bit alignment */
>  } xfs_agi_t;
>  
> +#define XFS_AGI_CRC_OFF		offsetof(struct xfs_agi, agi_crc)
> +
>  #define	XFS_AGI_MAGICNUM	0x00000001
>  #define	XFS_AGI_VERSIONNUM	0x00000002
>  #define	XFS_AGI_SEQNO		0x00000004
> @@ -222,6 +226,8 @@ typedef struct xfs_agfl {
>  	__be32		agfl_bno[];	/* actually XFS_AGFL_SIZE(mp) */
>  } xfs_agfl_t;
>  
> +#define XFS_AGFL_CRC_OFF	offsetof(struct xfs_agfl, agfl_crc)
> +
>  /*
>   * tags for inode radix tree
>   */
> diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
> index 9eab2df..72ea855 100644
> --- a/fs/xfs/xfs_alloc.c
> +++ b/fs/xfs/xfs_alloc.c
> @@ -486,7 +486,7 @@ xfs_agfl_read_verify(
>  		return;
>  
>  	agfl_ok = xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
> -				   offsetof(struct xfs_agfl, agfl_crc));
> +				   XFS_AGFL_CRC_OFF);
>  
>  	agfl_ok = agfl_ok && xfs_agfl_verify(bp);
>  
> @@ -516,8 +516,7 @@ xfs_agfl_write_verify(
>  	if (bip)
>  		XFS_BUF_TO_AGFL(bp)->agfl_lsn = cpu_to_be64(bip->bli_item.li_lsn);
>  
> -	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length),
> -			 offsetof(struct xfs_agfl, agfl_crc));
> +	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_AGFL_CRC_OFF);
>  }
>  
>  const struct xfs_buf_ops xfs_agfl_buf_ops = {
> @@ -2242,7 +2241,7 @@ xfs_agf_read_verify(
>  
>  	if (xfs_sb_version_hascrc(&mp->m_sb))
>  		agf_ok = xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
> -					  offsetof(struct xfs_agf, agf_crc));
> +					  XFS_AGF_CRC_OFF);
>  
>  	agf_ok = agf_ok && xfs_agf_verify(mp, bp);
>  
> @@ -2272,8 +2271,7 @@ xfs_agf_write_verify(
>  	if (bip)
>  		XFS_BUF_TO_AGF(bp)->agf_lsn = cpu_to_be64(bip->bli_item.li_lsn);
>  
> -	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length),
> -			 offsetof(struct xfs_agf, agf_crc));
> +	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_AGF_CRC_OFF);
>  }
>  
>  const struct xfs_buf_ops xfs_agf_buf_ops = {
> diff --git a/fs/xfs/xfs_dinode.h b/fs/xfs/xfs_dinode.h
> index e5869b5..623bbe8 100644
> --- a/fs/xfs/xfs_dinode.h
> +++ b/fs/xfs/xfs_dinode.h
> @@ -89,6 +89,8 @@ typedef struct xfs_dinode {
>  	/* structure must be padded to 64 bit alignment */
>  } xfs_dinode_t;
>  
> +#define XFS_DINODE_CRC_OFF	offsetof(struct xfs_dinode, di_crc)
> +
>  #define DI_MAX_FLUSH 0xffff
>  
>  /*
> diff --git a/fs/xfs/xfs_format.h b/fs/xfs/xfs_format.h
> index b6ab5a3..9898f31 100644
> --- a/fs/xfs/xfs_format.h
> +++ b/fs/xfs/xfs_format.h
> @@ -145,6 +145,8 @@ struct xfs_dsymlink_hdr {
>  	__be64	sl_lsn;
>  };
>  
> +#define XFS_SYMLINK_CRC_OFF	offsetof(struct xfs_dsymlink_hdr, sl_crc)
> +
>  /*
>   * The maximum pathlen is 1024 bytes. Since the minimum file system
>   * blocksize is 512 bytes, we can get a max of 3 extents back from
> diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
> index 5d7f105..d79210b 100644
> --- a/fs/xfs/xfs_ialloc.c
> +++ b/fs/xfs/xfs_ialloc.c
> @@ -1572,7 +1572,7 @@ xfs_agi_read_verify(
>  
>  	if (xfs_sb_version_hascrc(&mp->m_sb))
>  		agi_ok = xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
> -					  offsetof(struct xfs_agi, agi_crc));
> +					  XFS_AGI_CRC_OFF);
>  	agi_ok = agi_ok && xfs_agi_verify(bp);
>  
>  	if (unlikely(XFS_TEST_ERROR(!agi_ok, mp, XFS_ERRTAG_IALLOC_READ_AGI,
> @@ -1600,8 +1600,7 @@ xfs_agi_write_verify(
>  
>  	if (bip)
>  		XFS_BUF_TO_AGI(bp)->agi_lsn = cpu_to_be64(bip->bli_item.li_lsn);
> -	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length),
> -			 offsetof(struct xfs_agi, agi_crc));
> +	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_AGI_CRC_OFF);
>  }
>  
>  const struct xfs_buf_ops xfs_agi_buf_ops = {
> diff --git a/fs/xfs/xfs_inode_buf.c b/fs/xfs/xfs_inode_buf.c
> index 4fc9f39..606b43a 100644
> --- a/fs/xfs/xfs_inode_buf.c
> +++ b/fs/xfs/xfs_inode_buf.c
> @@ -306,7 +306,7 @@ xfs_dinode_verify(
>  	if (!xfs_sb_version_hascrc(&mp->m_sb))
>  		return false;
>  	if (!xfs_verify_cksum((char *)dip, mp->m_sb.sb_inodesize,
> -			      offsetof(struct xfs_dinode, di_crc)))
> +			      XFS_DINODE_CRC_OFF))
>  		return false;
>  	if (be64_to_cpu(dip->di_ino) != ip->i_ino)
>  		return false;
> @@ -327,7 +327,7 @@ xfs_dinode_calc_crc(
>  
>  	ASSERT(xfs_sb_version_hascrc(&mp->m_sb));
>  	crc = xfs_start_cksum((char *)dip, mp->m_sb.sb_inodesize,
> -			      offsetof(struct xfs_dinode, di_crc));
> +			      XFS_DINODE_CRC_OFF);
>  	dip->di_crc = xfs_end_cksum(crc);
>  }
>  
> diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
> index 1e11679..1ea7c86 100644
> --- a/fs/xfs/xfs_sb.c
> +++ b/fs/xfs/xfs_sb.c
> @@ -611,7 +611,7 @@ xfs_sb_read_verify(
>  	     dsb->sb_crc != 0)) {
>  
>  		if (!xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
> -				      offsetof(struct xfs_sb, sb_crc))) {
> +				      XFS_SB_CRC_OFF)) {
>  			/* Only fail bad secondaries on a known V5 filesystem */
>  			if (bp->b_bn == XFS_SB_DADDR ||
>  			    xfs_sb_version_hascrc(&mp->m_sb)) {
> @@ -674,8 +674,7 @@ xfs_sb_write_verify(
>  	if (bip)
>  		XFS_BUF_TO_SBP(bp)->sb_lsn = cpu_to_be64(bip->bli_item.li_lsn);
>  
> -	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length),
> -			 offsetof(struct xfs_sb, sb_crc));
> +	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_SB_CRC_OFF);
>  }
>  
>  const struct xfs_buf_ops xfs_sb_buf_ops = {
> diff --git a/fs/xfs/xfs_sb.h b/fs/xfs/xfs_sb.h
> index 35061d4..f7b2fe7 100644
> --- a/fs/xfs/xfs_sb.h
> +++ b/fs/xfs/xfs_sb.h
> @@ -182,6 +182,8 @@ typedef struct xfs_sb {
>  	/* must be padded to 64 bit alignment */
>  } xfs_sb_t;
>  
> +#define XFS_SB_CRC_OFF		offsetof(struct xfs_sb, sb_crc)
> +
>  /*
>   * Superblock - on disk version.  Must match the in core version above.
>   * Must be padded to 64 bit alignment.
> diff --git a/fs/xfs/xfs_symlink_remote.c b/fs/xfs/xfs_symlink_remote.c
> index bf59a2b..7a705a4 100644
> --- a/fs/xfs/xfs_symlink_remote.c
> +++ b/fs/xfs/xfs_symlink_remote.c
> @@ -134,7 +134,7 @@ xfs_symlink_read_verify(
>  		return;
>  
>  	if (!xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
> -				  offsetof(struct xfs_dsymlink_hdr, sl_crc)) ||
> +				  XFS_SYMLINK_CRC_OFF) ||
>  	    !xfs_symlink_verify(bp)) {
>  		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> @@ -162,8 +162,7 @@ xfs_symlink_write_verify(
>  		struct xfs_dsymlink_hdr *dsl = bp->b_addr;
>  		dsl->sl_lsn = cpu_to_be64(bip->bli_item.li_lsn);
>  	}
> -	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length),
> -			 offsetof(struct xfs_dsymlink_hdr, sl_crc));
> +	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_SYMLINK_CRC_OFF);
>  }
>  
>  const struct xfs_buf_ops xfs_symlink_buf_ops = {
> 

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

Re: [PATCH 4/9] xfs: Use defines for CRC offsets in all cases

[Dave Chinner]

On Wed, Feb 19, 2014 at 03:56:54PM +0800, Jeff Liu wrote:
> Hi Eric,
> 
> I read the previous comments from Dave about using defines for CRC offsets,
> and with a grep search after applying this patch, looks there have another
> two places maybe we should switch them to the macros as well:
> 
> fs/xfs/xfs_log.c:
> Do we need a log record crc offset macros for offsetof(struct xlog_rec_header, h_crc))?
> 
> xfs_dinode.h:
> we added the XFS_DINODE_CRC_OFF, just use it at below routine?
> 
> static inline uint xfs_dinode_size(int version)
> {
>         if (version == 3)
>                 return sizeof(struct xfs_dinode);
>         return offsetof(struct xfs_dinode, di_crc);
> }

No, that's a different case - it's not being used for determining
the offset of a CRC varaible - it's being used to calculate the size
of the version 2 inode core. Hence it should remain open coded like
because it has a different purpose in life....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 4/9] xfs: Use defines for CRC offsets in all cases

[Jeff Liu]

On 02/20 2014 08:27 AM, Dave Chinner wrote:
> On Wed, Feb 19, 2014 at 03:56:54PM +0800, Jeff Liu wrote:
>> Hi Eric,
>>
>> I read the previous comments from Dave about using defines for CRC offsets,
>> and with a grep search after applying this patch, looks there have another
>> two places maybe we should switch them to the macros as well:
>>
>> fs/xfs/xfs_log.c:
>> Do we need a log record crc offset macros for offsetof(struct xlog_rec_header, h_crc))?
>>
>> xfs_dinode.h:
>> we added the XFS_DINODE_CRC_OFF, just use it at below routine?
>>
>> static inline uint xfs_dinode_size(int version)
>> {
>>         if (version == 3)
>>                 return sizeof(struct xfs_dinode);
>>         return offsetof(struct xfs_dinode, di_crc);
>> }
> 
> No, that's a different case - it's not being used for determining
> the offset of a CRC varaible - it's being used to calculate the size
> of the version 2 inode core. Hence it should remain open coded like
> because it has a different purpose in life....

Thanks for the clarification, so we don't need that for the second inode
case, but the first case is used to determine the log record crc offset
to generate the crc for record header, shouldn't we make it consistent
with others?


Thanks,
-Jeff

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

Re: [PATCH 4/9] xfs: Use defines for CRC offsets in all cases

[Jeff Liu]

On 02/20 2014 08:27 AM, Dave Chinner wrote:
> On Wed, Feb 19, 2014 at 03:56:54PM +0800, Jeff Liu wrote:
>> Hi Eric,
>>
>> I read the previous comments from Dave about using defines for CRC offsets,
>> and with a grep search after applying this patch, looks there have another
>> two places maybe we should switch them to the macros as well:
>>
>> fs/xfs/xfs_log.c:
>> Do we need a log record crc offset macros for offsetof(struct xlog_rec_header, h_crc))?
>>
>> xfs_dinode.h:
>> we added the XFS_DINODE_CRC_OFF, just use it at below routine?
>>
>> static inline uint xfs_dinode_size(int version)
>> {
>>         if (version == 3)
>>                 return sizeof(struct xfs_dinode);
>>         return offsetof(struct xfs_dinode, di_crc);
>> }
> 
> No, that's a different case - it's not being used for determining
> the offset of a CRC varaible - it's being used to calculate the size
> of the version 2 inode core. Hence it should remain open coded like
> because it has a different purpose in life....

Thanks for the clarification, so we don't need that for the second inode
case, but the first case is used to determine the log record crc offset
to generate the crc for record header, shouldn't we make it consistent
with others?


Thanks,
-Jeff

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

Re: [PATCH 4/9] xfs: Use defines for CRC offsets in all cases

[Dave Chinner]

On Thu, Feb 20, 2014 at 05:41:02PM +0800, Jeff Liu wrote:
> 
> On 02/20 2014 08:27 AM, Dave Chinner wrote:
> > On Wed, Feb 19, 2014 at 03:56:54PM +0800, Jeff Liu wrote:
> >> Hi Eric,
> >>
> >> I read the previous comments from Dave about using defines for CRC offsets,
> >> and with a grep search after applying this patch, looks there have another
> >> two places maybe we should switch them to the macros as well:
> >>
> >> fs/xfs/xfs_log.c:
> >> Do we need a log record crc offset macros for offsetof(struct xlog_rec_header, h_crc))?
> >>
> >> xfs_dinode.h:
> >> we added the XFS_DINODE_CRC_OFF, just use it at below routine?
> >>
> >> static inline uint xfs_dinode_size(int version)
> >> {
> >>         if (version == 3)
> >>                 return sizeof(struct xfs_dinode);
> >>         return offsetof(struct xfs_dinode, di_crc);
> >> }
> > 
> > No, that's a different case - it's not being used for determining
> > the offset of a CRC varaible - it's being used to calculate the size
> > of the version 2 inode core. Hence it should remain open coded like
> > because it has a different purpose in life....
> 
> Thanks for the clarification, so we don't need that for the second inode
> case, but the first case is used to determine the log record crc offset
> to generate the crc for record header, shouldn't we make it consistent
> with others?

Probably should. Eric, can you send a separate patch that converts
the log crc fileds to use the same convention?

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 5/9] xfs: add helper for verifying checksums on xfs_bufs

[Eric Sandeen]

Many/most callers of xfs_verify_cksum() pass bp->b_addr and
BBTOB(bp->b_length) as the first 2 args.  Add a helper
which can just accept the bp and the crc offset, and work
it out on its own, for brevity.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_alloc.c          |    6 ++----
 fs/xfs/xfs_attr_leaf.c      |    3 +--
 fs/xfs/xfs_btree.c          |    8 ++++----
 fs/xfs/xfs_buf.h            |    7 +++++++
 fs/xfs/xfs_da_btree.c       |    3 +--
 fs/xfs/xfs_dir2_block.c     |    3 +--
 fs/xfs/xfs_dir2_data.c      |    3 +--
 fs/xfs/xfs_dir2_leaf.c      |    3 +--
 fs/xfs/xfs_dir2_node.c      |    3 +--
 fs/xfs/xfs_ialloc.c         |    4 ++--
 fs/xfs/xfs_linux.h          |    1 +
 fs/xfs/xfs_sb.c             |    3 +--
 fs/xfs/xfs_symlink_remote.c |    3 +--
 13 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
index 72ea855..5050c9a 100644
--- a/fs/xfs/xfs_alloc.c
+++ b/fs/xfs/xfs_alloc.c
@@ -485,8 +485,7 @@ xfs_agfl_read_verify(
 	if (!xfs_sb_version_hascrc(&mp->m_sb))
 		return;
 
-	agfl_ok = xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-				   XFS_AGFL_CRC_OFF);
+	agfl_ok = xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF);
 
 	agfl_ok = agfl_ok && xfs_agfl_verify(bp);
 
@@ -2240,8 +2239,7 @@ xfs_agf_read_verify(
 	int		agf_ok = 1;
 
 	if (xfs_sb_version_hascrc(&mp->m_sb))
-		agf_ok = xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-					  XFS_AGF_CRC_OFF);
+		agf_ok = xfs_buf_verify_cksum(bp, XFS_AGF_CRC_OFF);
 
 	agf_ok = agf_ok && xfs_agf_verify(mp, bp);
 
diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
index 7b126f4..a19a023 100644
--- a/fs/xfs/xfs_attr_leaf.c
+++ b/fs/xfs/xfs_attr_leaf.c
@@ -240,8 +240,7 @@ xfs_attr3_leaf_read_verify(
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
 	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-					  XFS_ATTR3_LEAF_CRC_OFF)) ||
+	     !xfs_buf_verify_cksum(bp, XFS_ATTR3_LEAF_CRC_OFF)) ||
 	    !xfs_attr3_leaf_verify(bp)) {
 		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
diff --git a/fs/xfs/xfs_btree.c b/fs/xfs/xfs_btree.c
index 9adaae4..4e8524d 100644
--- a/fs/xfs/xfs_btree.c
+++ b/fs/xfs/xfs_btree.c
@@ -243,8 +243,8 @@ xfs_btree_lblock_verify_crc(
 	struct xfs_buf		*bp)
 {
 	if (xfs_sb_version_hascrc(&bp->b_target->bt_mount->m_sb))
-		return xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-					XFS_BTREE_LBLOCK_CRC_OFF);
+		return xfs_buf_verify_cksum(bp, XFS_BTREE_LBLOCK_CRC_OFF);
+
 	return true;
 }
 
@@ -276,8 +276,8 @@ xfs_btree_sblock_verify_crc(
 	struct xfs_buf		*bp)
 {
 	if (xfs_sb_version_hascrc(&bp->b_target->bt_mount->m_sb))
-		return xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-					XFS_BTREE_SBLOCK_CRC_OFF);
+		return xfs_buf_verify_cksum(bp, XFS_BTREE_SBLOCK_CRC_OFF);
+
 	return true;
 }
 
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 9953395..5edcfba 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -369,6 +369,13 @@ static inline void xfs_buf_relse(xfs_buf_t *bp)
 	xfs_buf_rele(bp);
 }
 
+static inline int
+xfs_buf_verify_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
+{
+	return xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
+				cksum_offset);
+}
+
 /*
  *	Handling of buftargs.
  */
diff --git a/fs/xfs/xfs_da_btree.c b/fs/xfs/xfs_da_btree.c
index 796272a..6cece55 100644
--- a/fs/xfs/xfs_da_btree.c
+++ b/fs/xfs/xfs_da_btree.c
@@ -214,8 +214,7 @@ xfs_da3_node_read_verify(
 
 	switch (be16_to_cpu(info->magic)) {
 		case XFS_DA3_NODE_MAGIC:
-			if (!xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-					      XFS_DA3_NODE_CRC_OFF))
+			if (!xfs_buf_verify_cksum(bp, XFS_DA3_NODE_CRC_OFF))
 				break;
 			/* fall through */
 		case XFS_DA_NODE_MAGIC:
diff --git a/fs/xfs/xfs_dir2_block.c b/fs/xfs/xfs_dir2_block.c
index 90cdbf4..948dc39 100644
--- a/fs/xfs/xfs_dir2_block.c
+++ b/fs/xfs/xfs_dir2_block.c
@@ -90,8 +90,7 @@ xfs_dir3_block_read_verify(
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
 	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-					  XFS_DIR3_DATA_CRC_OFF)) ||
+	     !xfs_buf_verify_cksum(bp, XFS_DIR3_DATA_CRC_OFF)) ||
 	    !xfs_dir3_block_verify(bp)) {
 		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
diff --git a/fs/xfs/xfs_dir2_data.c b/fs/xfs/xfs_dir2_data.c
index 70acff4..1952f00 100644
--- a/fs/xfs/xfs_dir2_data.c
+++ b/fs/xfs/xfs_dir2_data.c
@@ -268,8 +268,7 @@ xfs_dir3_data_read_verify(
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
 	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-					  XFS_DIR3_DATA_CRC_OFF)) ||
+	     !xfs_buf_verify_cksum(bp, XFS_DIR3_DATA_CRC_OFF)) ||
 	    !xfs_dir3_data_verify(bp)) {
 		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
diff --git a/fs/xfs/xfs_dir2_leaf.c b/fs/xfs/xfs_dir2_leaf.c
index ae47ec6..1a412eb 100644
--- a/fs/xfs/xfs_dir2_leaf.c
+++ b/fs/xfs/xfs_dir2_leaf.c
@@ -180,8 +180,7 @@ __read_verify(
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
 	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-					  XFS_DIR3_LEAF_CRC_OFF)) ||
+	     !xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF)) ||
 	    !xfs_dir3_leaf_verify(bp, magic)) {
 		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
diff --git a/fs/xfs/xfs_dir2_node.c b/fs/xfs/xfs_dir2_node.c
index 48c7d18..875e7c0 100644
--- a/fs/xfs/xfs_dir2_node.c
+++ b/fs/xfs/xfs_dir2_node.c
@@ -116,8 +116,7 @@ xfs_dir3_free_read_verify(
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
 	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-					  XFS_DIR3_FREE_CRC_OFF)) ||
+	     !xfs_buf_verify_cksum(bp, XFS_DIR3_FREE_CRC_OFF)) ||
 	    !xfs_dir3_free_verify(bp)) {
 		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
index d79210b..d6a879d 100644
--- a/fs/xfs/xfs_ialloc.c
+++ b/fs/xfs/xfs_ialloc.c
@@ -1571,8 +1571,8 @@ xfs_agi_read_verify(
 	int		agi_ok = 1;
 
 	if (xfs_sb_version_hascrc(&mp->m_sb))
-		agi_ok = xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-					  XFS_AGI_CRC_OFF);
+		agi_ok = xfs_buf_verify_cksum(bp, XFS_AGI_CRC_OFF);
+
 	agi_ok = agi_ok && xfs_agi_verify(bp);
 
 	if (unlikely(XFS_TEST_ERROR(!agi_ok, mp, XFS_ERRTAG_IALLOC_READ_AGI,
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index f9bb590..e8fed74 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -119,6 +119,7 @@ typedef __uint64_t __psunsigned_t;
 #include "xfs_iops.h"
 #include "xfs_aops.h"
 #include "xfs_super.h"
+#include "xfs_cksum.h"
 #include "xfs_buf.h"
 #include "xfs_message.h"
 
diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
index 1ea7c86..36f287f 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -610,8 +610,7 @@ xfs_sb_read_verify(
 						XFS_SB_VERSION_5) ||
 	     dsb->sb_crc != 0)) {
 
-		if (!xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-				      XFS_SB_CRC_OFF)) {
+		if (!xfs_buf_verify_cksum(bp, XFS_SB_CRC_OFF)) {
 			/* Only fail bad secondaries on a known V5 filesystem */
 			if (bp->b_bn == XFS_SB_DADDR ||
 			    xfs_sb_version_hascrc(&mp->m_sb)) {
diff --git a/fs/xfs/xfs_symlink_remote.c b/fs/xfs/xfs_symlink_remote.c
index 7a705a4..b172955 100644
--- a/fs/xfs/xfs_symlink_remote.c
+++ b/fs/xfs/xfs_symlink_remote.c
@@ -133,8 +133,7 @@ xfs_symlink_read_verify(
 	if (!xfs_sb_version_hascrc(&mp->m_sb))
 		return;
 
-	if (!xfs_verify_cksum(bp->b_addr, BBTOB(bp->b_length),
-				  XFS_SYMLINK_CRC_OFF) ||
+	if (!xfs_buf_verify_cksum(bp, XFS_SYMLINK_CRC_OFF) ||
 	    !xfs_symlink_verify(bp)) {
 		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-- 
1.7.1

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

Re: [PATCH 5/9] xfs: add helper for verifying checksums on xfs_bufs

[Dave Chinner]

On Tue, Feb 18, 2014 at 05:52:25PM -0600, Eric Sandeen wrote:
> Many/most callers of xfs_verify_cksum() pass bp->b_addr and
> BBTOB(bp->b_length) as the first 2 args.  Add a helper
> which can just accept the bp and the crc offset, and work
> it out on its own, for brevity.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks good.

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

-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 6/9] xfs: add helper for updating checksums on xfs_bufs

[Eric Sandeen]

Many/most callers of xfs_update_cksum() pass bp->b_addr and
BBTOB(bp->b_length) as the first 2 args.  Add a helper
which can just accept the bp and the crc offset, and work
it out on its own, for brevity.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_alloc.c          |    4 ++--
 fs/xfs/xfs_attr_leaf.c      |    2 +-
 fs/xfs/xfs_btree.c          |    6 ++----
 fs/xfs/xfs_buf.h            |    7 +++++++
 fs/xfs/xfs_da_btree.c       |    2 +-
 fs/xfs/xfs_dir2_block.c     |    2 +-
 fs/xfs/xfs_dir2_data.c      |    2 +-
 fs/xfs/xfs_dir2_leaf.c      |    2 +-
 fs/xfs/xfs_dir2_node.c      |    2 +-
 fs/xfs/xfs_ialloc.c         |    2 +-
 fs/xfs/xfs_sb.c             |    2 +-
 fs/xfs/xfs_symlink_remote.c |    2 +-
 12 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
index 5050c9a..9c7cf3d 100644
--- a/fs/xfs/xfs_alloc.c
+++ b/fs/xfs/xfs_alloc.c
@@ -515,7 +515,7 @@ xfs_agfl_write_verify(
 	if (bip)
 		XFS_BUF_TO_AGFL(bp)->agfl_lsn = cpu_to_be64(bip->bli_item.li_lsn);
 
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_AGFL_CRC_OFF);
+	xfs_buf_update_cksum(bp, XFS_AGFL_CRC_OFF);
 }
 
 const struct xfs_buf_ops xfs_agfl_buf_ops = {
@@ -2269,7 +2269,7 @@ xfs_agf_write_verify(
 	if (bip)
 		XFS_BUF_TO_AGF(bp)->agf_lsn = cpu_to_be64(bip->bli_item.li_lsn);
 
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_AGF_CRC_OFF);
+	xfs_buf_update_cksum(bp, XFS_AGF_CRC_OFF);
 }
 
 const struct xfs_buf_ops xfs_agf_buf_ops = {
diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
index a19a023..b552378 100644
--- a/fs/xfs/xfs_attr_leaf.c
+++ b/fs/xfs/xfs_attr_leaf.c
@@ -224,7 +224,7 @@ xfs_attr3_leaf_write_verify(
 	if (bip)
 		hdr3->info.lsn = cpu_to_be64(bip->bli_item.li_lsn);
 
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_ATTR3_LEAF_CRC_OFF);
+	xfs_buf_update_cksum(bp, XFS_ATTR3_LEAF_CRC_OFF);
 }
 
 /*
diff --git a/fs/xfs/xfs_btree.c b/fs/xfs/xfs_btree.c
index 4e8524d..e80d59f 100644
--- a/fs/xfs/xfs_btree.c
+++ b/fs/xfs/xfs_btree.c
@@ -234,8 +234,7 @@ xfs_btree_lblock_calc_crc(
 		return;
 	if (bip)
 		block->bb_u.l.bb_lsn = cpu_to_be64(bip->bli_item.li_lsn);
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length),
-			 XFS_BTREE_LBLOCK_CRC_OFF);
+	xfs_buf_update_cksum(bp, XFS_BTREE_LBLOCK_CRC_OFF);
 }
 
 bool
@@ -267,8 +266,7 @@ xfs_btree_sblock_calc_crc(
 		return;
 	if (bip)
 		block->bb_u.s.bb_lsn = cpu_to_be64(bip->bli_item.li_lsn);
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length),
-			 XFS_BTREE_SBLOCK_CRC_OFF);
+	xfs_buf_update_cksum(bp, XFS_BTREE_SBLOCK_CRC_OFF);
 }
 
 bool
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 5edcfba..b8a3abf 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -376,6 +376,13 @@ xfs_buf_verify_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
 				cksum_offset);
 }
 
+static inline void
+xfs_buf_update_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
+{
+	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length),
+			 cksum_offset);
+}
+
 /*
  *	Handling of buftargs.
  */
diff --git a/fs/xfs/xfs_da_btree.c b/fs/xfs/xfs_da_btree.c
index 6cece55..75ef990 100644
--- a/fs/xfs/xfs_da_btree.c
+++ b/fs/xfs/xfs_da_btree.c
@@ -196,7 +196,7 @@ xfs_da3_node_write_verify(
 	if (bip)
 		hdr3->info.lsn = cpu_to_be64(bip->bli_item.li_lsn);
 
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_DA3_NODE_CRC_OFF);
+	xfs_buf_update_cksum(bp, XFS_DA3_NODE_CRC_OFF);
 }
 
 /*
diff --git a/fs/xfs/xfs_dir2_block.c b/fs/xfs/xfs_dir2_block.c
index 948dc39..724377e 100644
--- a/fs/xfs/xfs_dir2_block.c
+++ b/fs/xfs/xfs_dir2_block.c
@@ -117,7 +117,7 @@ xfs_dir3_block_write_verify(
 	if (bip)
 		hdr3->lsn = cpu_to_be64(bip->bli_item.li_lsn);
 
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_DIR3_DATA_CRC_OFF);
+	xfs_buf_update_cksum(bp, XFS_DIR3_DATA_CRC_OFF);
 }
 
 const struct xfs_buf_ops xfs_dir3_block_buf_ops = {
diff --git a/fs/xfs/xfs_dir2_data.c b/fs/xfs/xfs_dir2_data.c
index 1952f00..74ae85e 100644
--- a/fs/xfs/xfs_dir2_data.c
+++ b/fs/xfs/xfs_dir2_data.c
@@ -295,7 +295,7 @@ xfs_dir3_data_write_verify(
 	if (bip)
 		hdr3->lsn = cpu_to_be64(bip->bli_item.li_lsn);
 
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_DIR3_DATA_CRC_OFF);
+	xfs_buf_update_cksum(bp, XFS_DIR3_DATA_CRC_OFF);
 }
 
 const struct xfs_buf_ops xfs_dir3_data_buf_ops = {
diff --git a/fs/xfs/xfs_dir2_leaf.c b/fs/xfs/xfs_dir2_leaf.c
index 1a412eb..dffb61b 100644
--- a/fs/xfs/xfs_dir2_leaf.c
+++ b/fs/xfs/xfs_dir2_leaf.c
@@ -208,7 +208,7 @@ __write_verify(
 	if (bip)
 		hdr3->info.lsn = cpu_to_be64(bip->bli_item.li_lsn);
 
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_DIR3_LEAF_CRC_OFF);
+	xfs_buf_update_cksum(bp, XFS_DIR3_LEAF_CRC_OFF);
 }
 
 static void
diff --git a/fs/xfs/xfs_dir2_node.c b/fs/xfs/xfs_dir2_node.c
index 875e7c0..0904b20 100644
--- a/fs/xfs/xfs_dir2_node.c
+++ b/fs/xfs/xfs_dir2_node.c
@@ -143,7 +143,7 @@ xfs_dir3_free_write_verify(
 	if (bip)
 		hdr3->lsn = cpu_to_be64(bip->bli_item.li_lsn);
 
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_DIR3_FREE_CRC_OFF);
+	xfs_buf_update_cksum(bp, XFS_DIR3_FREE_CRC_OFF);
 }
 
 const struct xfs_buf_ops xfs_dir3_free_buf_ops = {
diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
index d6a879d..4657586 100644
--- a/fs/xfs/xfs_ialloc.c
+++ b/fs/xfs/xfs_ialloc.c
@@ -1600,7 +1600,7 @@ xfs_agi_write_verify(
 
 	if (bip)
 		XFS_BUF_TO_AGI(bp)->agi_lsn = cpu_to_be64(bip->bli_item.li_lsn);
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_AGI_CRC_OFF);
+	xfs_buf_update_cksum(bp, XFS_AGI_CRC_OFF);
 }
 
 const struct xfs_buf_ops xfs_agi_buf_ops = {
diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
index 36f287f..818359f 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -673,7 +673,7 @@ xfs_sb_write_verify(
 	if (bip)
 		XFS_BUF_TO_SBP(bp)->sb_lsn = cpu_to_be64(bip->bli_item.li_lsn);
 
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_SB_CRC_OFF);
+	xfs_buf_update_cksum(bp, XFS_SB_CRC_OFF);
 }
 
 const struct xfs_buf_ops xfs_sb_buf_ops = {
diff --git a/fs/xfs/xfs_symlink_remote.c b/fs/xfs/xfs_symlink_remote.c
index b172955..defa09f 100644
--- a/fs/xfs/xfs_symlink_remote.c
+++ b/fs/xfs/xfs_symlink_remote.c
@@ -161,7 +161,7 @@ xfs_symlink_write_verify(
 		struct xfs_dsymlink_hdr *dsl = bp->b_addr;
 		dsl->sl_lsn = cpu_to_be64(bip->bli_item.li_lsn);
 	}
-	xfs_update_cksum(bp->b_addr, BBTOB(bp->b_length), XFS_SYMLINK_CRC_OFF);
+	xfs_buf_update_cksum(bp, XFS_SYMLINK_CRC_OFF);
 }
 
 const struct xfs_buf_ops xfs_symlink_buf_ops = {
-- 
1.7.1

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

[PATCH 7/9] xfs: add xfs_verifier_error()

[Eric Sandeen]

We want to distinguish between corruption, CRC errors,
etc.  In addition, the full stack trace on verifier errors
seems less than helpful; it looks more like an oops than
corruption.

Create a new function to specifically alert the user to
verifier errors, which can differentiate between
EFSCORRUPTED and CRC mismatches.  It doesn't dump stack
unless the xfs error level is turned up high.

Define a new error message (EFSBADCRC) to clearly identify
CRC errors.  (Defined to EILSEQ, bad byte sequence)

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_error.c |   26 ++++++++++++++++++++++++++
 fs/xfs/xfs_error.h |    1 +
 fs/xfs/xfs_linux.h |    1 +
 3 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index 9995b80..8733c59 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -178,3 +178,29 @@ xfs_corruption_error(
 	xfs_error_report(tag, level, mp, filename, linenum, ra);
 	xfs_alert(mp, "Corruption detected. Unmount and run xfs_repair");
 }
+
+/*
+ * Warnings specifically for verifier errors.  Differentiate CRC vs. invalid
+ * values, and omit the stack trace unless the error level is tuned high.
+ */
+void
+xfs_verifier_error(
+	struct xfs_buf		*bp)
+{
+	struct xfs_mount *mp = bp->b_target->bt_mount;
+
+	xfs_alert(mp, "Metadata %s detected at %pF, block 0x%llx",
+		  bp->b_error == EFSBADCRC ? "CRC error" : "corruption",
+		  __return_address, bp->b_bn);
+
+	xfs_alert(mp, "Unmount and run xfs_repair");
+
+	/* XXX handle page-mapped buffers too? */
+	if (xfs_error_level >= XFS_ERRLEVEL_LOW && bp->b_addr) {
+		xfs_alert(mp, "First 64 bytes of corrupted metadata buffer:");
+		xfs_hex_dump(bp->b_addr, 64);
+	}
+
+	if (xfs_error_level >= XFS_ERRLEVEL_HIGH)
+		xfs_stack_trace();
+}
diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
index 079a367..c1c57d4 100644
--- a/fs/xfs/xfs_error.h
+++ b/fs/xfs/xfs_error.h
@@ -34,6 +34,7 @@ extern void xfs_error_report(const char *tag, int level, struct xfs_mount *mp,
 extern void xfs_corruption_error(const char *tag, int level,
 			struct xfs_mount *mp, void *p, const char *filename,
 			int linenum, inst_t *ra);
+extern void xfs_verifier_error(struct xfs_buf *bp);
 
 #define	XFS_ERROR_REPORT(e, lvl, mp)	\
 	xfs_error_report(e, lvl, mp, __FILE__, __LINE__, __return_address)
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index e8fed74..016ea8d 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -179,6 +179,7 @@ typedef __uint64_t __psunsigned_t;
 #define ENOATTR		ENODATA		/* Attribute not found */
 #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
 #define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
+#define EFSBADCRC	EILSEQ		/* Bad CRC detected */
 
 #define SYNCHRONIZE()	barrier()
 #define __return_address __builtin_return_address(0)
-- 
1.7.1

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

Re: [PATCH 7/9] xfs: add xfs_verifier_error()

[Dave Chinner]

On Tue, Feb 18, 2014 at 05:52:27PM -0600, Eric Sandeen wrote:
> We want to distinguish between corruption, CRC errors,
> etc.  In addition, the full stack trace on verifier errors
> seems less than helpful; it looks more like an oops than
> corruption.
> 
> Create a new function to specifically alert the user to
> verifier errors, which can differentiate between
> EFSCORRUPTED and CRC mismatches.  It doesn't dump stack
> unless the xfs error level is turned up high.
> 
> Define a new error message (EFSBADCRC) to clearly identify
> CRC errors.  (Defined to EILSEQ, bad byte sequence)

I think we decided the other best candidates were:

EBADE: Invalid exchange
EPROTO: Protocol error
EBADMSG: Bad message
EKEYREJECTED: Key was rejected by service

because of the silly glibc translation of EILSEQ:

EILSEQ: Invalid or incomplete multibyte or wide character

I'm leaning towards EBADMSG as the one to use here.

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/xfs_error.c |   26 ++++++++++++++++++++++++++
>  fs/xfs/xfs_error.h |    1 +
>  fs/xfs/xfs_linux.h |    1 +
>  3 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> index 9995b80..8733c59 100644
> --- a/fs/xfs/xfs_error.c
> +++ b/fs/xfs/xfs_error.c
> @@ -178,3 +178,29 @@ xfs_corruption_error(
>  	xfs_error_report(tag, level, mp, filename, linenum, ra);
>  	xfs_alert(mp, "Corruption detected. Unmount and run xfs_repair");
>  }
> +
> +/*
> + * Warnings specifically for verifier errors.  Differentiate CRC vs. invalid
> + * values, and omit the stack trace unless the error level is tuned high.
> + */
> +void
> +xfs_verifier_error(
> +	struct xfs_buf		*bp)
> +{
> +	struct xfs_mount *mp = bp->b_target->bt_mount;
> +
> +	xfs_alert(mp, "Metadata %s detected at %pF, block 0x%llx",
> +		  bp->b_error == EFSBADCRC ? "CRC error" : "corruption",
> +		  __return_address, bp->b_bn);
> +
> +	xfs_alert(mp, "Unmount and run xfs_repair");
> +
> +	/* XXX handle page-mapped buffers too? */
> +	if (xfs_error_level >= XFS_ERRLEVEL_LOW && bp->b_addr) {
> +		xfs_alert(mp, "First 64 bytes of corrupted metadata buffer:");
> +		xfs_hex_dump(bp->b_addr, 64);
> +	}

Just use xfs_buf_offset(bp, 0) here, and you don't have to care
about how the buffer is mapped or allocated.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 7/9 V2] xfs: add xfs_verifier_error()

[Eric Sandeen]

We want to distinguish between corruption, CRC errors,
etc.  In addition, the full stack trace on verifier errors
seems less than helpful; it looks more like an oops than
corruption.

Create a new function to specifically alert the user to
verifier errors, which can differentiate between
EFSCORRUPTED and CRC mismatches.  It doesn't dump stack
unless the xfs error level is turned up high.

Define a new error message (EFSBADCRC) to clearly identify
CRC errors.  (Defined to EILSEQ, bad byte sequence)

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: Switch to EBADMSG, use xfs_buf_offset()

diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index 9995b80..a8b2ecb 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -178,3 +178,28 @@ xfs_corruption_error(
 	xfs_error_report(tag, level, mp, filename, linenum, ra);
 	xfs_alert(mp, "Corruption detected. Unmount and run xfs_repair");
 }
+
+/*
+ * Warnings specifically for verifier errors.  Differentiate CRC vs. invalid
+ * values, and omit the stack trace unless the error level is tuned high.
+ */
+void
+xfs_verifier_error(
+	struct xfs_buf		*bp)
+{
+	struct xfs_mount *mp = bp->b_target->bt_mount;
+
+	xfs_alert(mp, "Metadata %s detected at %pF, block 0x%llx",
+		  bp->b_error == EFSBADCRC ? "CRC error" : "corruption",
+		  __return_address, bp->b_bn);
+
+	xfs_alert(mp, "Unmount and run xfs_repair");
+
+	if (xfs_error_level >= XFS_ERRLEVEL_LOW) {
+		xfs_alert(mp, "First 64 bytes of corrupted metadata buffer:");
+		xfs_hex_dump(xfs_buf_offset(bp, 0), 64);
+	}
+
+	if (xfs_error_level >= XFS_ERRLEVEL_HIGH)
+		xfs_stack_trace();
+}
diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
index 079a367..c1c57d4 100644
--- a/fs/xfs/xfs_error.h
+++ b/fs/xfs/xfs_error.h
@@ -34,6 +34,7 @@ extern void xfs_error_report(const char *tag, int level, struct xfs_mount *mp,
 extern void xfs_corruption_error(const char *tag, int level,
 			struct xfs_mount *mp, void *p, const char *filename,
 			int linenum, inst_t *ra);
+extern void xfs_verifier_error(struct xfs_buf *bp);
 
 #define	XFS_ERROR_REPORT(e, lvl, mp)	\
 	xfs_error_report(e, lvl, mp, __FILE__, __LINE__, __return_address)
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index e8fed74..825249d 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -179,6 +179,7 @@ typedef __uint64_t __psunsigned_t;
 #define ENOATTR		ENODATA		/* Attribute not found */
 #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
 #define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
+#define EFSBADCRC	EBADMSG		/* Bad CRC detected */
 
 #define SYNCHRONIZE()	barrier()
 #define __return_address __builtin_return_address(0)


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

Re: [PATCH 7/9 V2] xfs: add xfs_verifier_error()

[Dave Chinner]

On Wed, Feb 19, 2014 at 08:58:11PM -0600, Eric Sandeen wrote:
> We want to distinguish between corruption, CRC errors,
> etc.  In addition, the full stack trace on verifier errors
> seems less than helpful; it looks more like an oops than
> corruption.
> 
> Create a new function to specifically alert the user to
> verifier errors, which can differentiate between
> EFSCORRUPTED and CRC mismatches.  It doesn't dump stack
> unless the xfs error level is turned up high.
> 
> Define a new error message (EFSBADCRC) to clearly identify
> CRC errors.  (Defined to EILSEQ, bad byte sequence)

I've updated this to be EBADMSG, to match the code.

Otherwise, looks good.

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

-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 8/9] xfs: print useful caller information in xfs_error_report

[Eric Sandeen]

xfs_error_report used to just print the hex address of the caller;
%pF will give us something more human-readable.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_error.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index 8733c59..04d8e65 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -156,7 +156,7 @@ xfs_error_report(
 {
 	if (level <= xfs_error_level) {
 		xfs_alert_tag(mp, XFS_PTAG_ERROR_REPORT,
-		"Internal error %s at line %d of file %s.  Caller 0x%p",
+		"Internal error %s at line %d of file %s.  Caller %pF",
 			    tag, linenum, filename, ra);
 
 		xfs_stack_trace();
-- 
1.7.1

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

Re: [PATCH 8/9] xfs: print useful caller information in xfs_error_report

[Jeff Liu]

On 02/19 2014 07:52 AM, Eric Sandeen wrote:
> xfs_error_report used to just print the hex address of the caller;
> %pF will give us something more human-readable.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/xfs_error.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> index 8733c59..04d8e65 100644
> --- a/fs/xfs/xfs_error.c
> +++ b/fs/xfs/xfs_error.c
> @@ -156,7 +156,7 @@ xfs_error_report(
>  {
>  	if (level <= xfs_error_level) {
>  		xfs_alert_tag(mp, XFS_PTAG_ERROR_REPORT,
> -		"Internal error %s at line %d of file %s.  Caller 0x%p",
> +		"Internal error %s at line %d of file %s.  Caller %pF",
>  			    tag, linenum, filename, ra);
>  
>  		xfs_stack_trace();

Reviewed-by: Jie Liu <jeff.liu@oracle.com>

In comparison with the hex code, this is obviously more readable as below.

Patched:
-------
XFS (sda7): Internal error xfs_sb_read_verify at line 630 of file fs/xfs/xfs_sb.c.  Caller xfs_buf_iodone_work+0xa5/0xd0 [xfs]
<snip>
Call Trace:
[<ffffffff81741f15>] dump_stack+0x45/0x56
[<ffffffffa07a5a0b>] xfs_error_report+0x3b/0x40 [xfs]
[<ffffffffa07a26b5>] ? xfs_buf_iodone_work+0xa5/0xd0 [xfs]
[<ffffffffa07a5a65>] xfs_corruption_error+0x55/0x80 [xfs]
[<ffffffffa080bf23>] xfs_sb_read_verify+0x143/0x150 [xfs]
[<ffffffffa07a26b5>] ? xfs_buf_iodone_work+0xa5/0xd0 [xfs]
[<ffffffff8108744f>] ? process_one_work+0x1af/0x540
[<ffffffffa07a26b5>] xfs_buf_iodone_work+0xa5/0xd0 [xfs]
<snip>


Old:
----
[  235.506754] XFS (sda7): Internal error xfs_sb_read_verify at line 630 of file fs/xfs/xfs_sb.c.  Caller 0xffffffffa08aa6b5
<snip>
[  235.506796] Call Trace:
[  235.506802]  [<ffffffff81741f15>] dump_stack+0x45/0x56
[  235.506818]  [<ffffffffa08ada0b>] xfs_error_report+0x3b/0x40 [xfs]
[  235.506832]  [<ffffffffa08aa6b5>] ? xfs_buf_iodone_work+0xa5/0xd0 [xfs]
[  235.506845]  [<ffffffffa08ada65>] xfs_corruption_error+0x55/0x80 [xfs]
[  235.506871]  [<ffffffffa0913f23>] xfs_sb_read_verify+0x143/0x150 [xfs]
[  235.506885]  [<ffffffffa08aa6b5>] ? xfs_buf_iodone_work+0xa5/0xd0 [xfs]
[  235.506890]  [<ffffffff8108744f>] ? process_one_work+0x1af/0x540
[  235.506903]  [<ffffffffa08aa6b5>] xfs_buf_iodone_work+0xa5/0xd0 [xfs]
<snip>


Thanks,
-Jeff

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

[PATCH 9/9] xfs: modify verifiers to differentiate CRC from other errors

[Eric Sandeen]

Modify all read & write verifiers to differentiate
between CRC errors and other inconsistencies.

This sets the appropriate error number on bp->b_error,
and then calls xfs_verifier_error() if something went
wrong.  That function will issue the appropriate message
to the user.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_alloc.c          |   37 +++++++++++++++++--------------------
 fs/xfs/xfs_alloc_btree.c    |   15 ++++++++-------
 fs/xfs/xfs_attr_leaf.c      |   14 ++++++++------
 fs/xfs/xfs_attr_remote.c    |   15 ++++++---------
 fs/xfs/xfs_bmap_btree.c     |   16 ++++++++--------
 fs/xfs/xfs_da_btree.c       |   14 ++++++++------
 fs/xfs/xfs_dir2_block.c     |   14 ++++++++------
 fs/xfs/xfs_dir2_data.c      |   17 +++++++++--------
 fs/xfs/xfs_dir2_leaf.c      |   14 ++++++++------
 fs/xfs/xfs_dir2_node.c      |   14 ++++++++------
 fs/xfs/xfs_dquot_buf.c      |   11 +++++++----
 fs/xfs/xfs_ialloc.c         |   12 ++++++++----
 fs/xfs/xfs_ialloc_btree.c   |   15 ++++++++-------
 fs/xfs/xfs_inode_buf.c      |    3 +--
 fs/xfs/xfs_sb.c             |   10 ++++------
 fs/xfs/xfs_symlink_remote.c |   12 +++++++-----
 16 files changed, 123 insertions(+), 110 deletions(-)

diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
index 9c7cf3d..9a93601 100644
--- a/fs/xfs/xfs_alloc.c
+++ b/fs/xfs/xfs_alloc.c
@@ -474,7 +474,6 @@ xfs_agfl_read_verify(
 	struct xfs_buf	*bp)
 {
 	struct xfs_mount *mp = bp->b_target->bt_mount;
-	int		agfl_ok = 1;
 
 	/*
 	 * There is no verification of non-crc AGFLs because mkfs does not
@@ -485,14 +484,13 @@ xfs_agfl_read_verify(
 	if (!xfs_sb_version_hascrc(&mp->m_sb))
 		return;
 
-	agfl_ok = xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF);
-
-	agfl_ok = agfl_ok && xfs_agfl_verify(bp);
-
-	if (!agfl_ok) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (!xfs_buf_verify_cksum(bp, offsetof(struct xfs_agfl, agfl_crc)))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_agfl_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -507,8 +505,8 @@ xfs_agfl_write_verify(
 		return;
 
 	if (!xfs_agfl_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
@@ -2236,18 +2234,17 @@ xfs_agf_read_verify(
 	struct xfs_buf	*bp)
 {
 	struct xfs_mount *mp = bp->b_target->bt_mount;
-	int		agf_ok = 1;
-
-	if (xfs_sb_version_hascrc(&mp->m_sb))
-		agf_ok = xfs_buf_verify_cksum(bp, XFS_AGF_CRC_OFF);
 
-	agf_ok = agf_ok && xfs_agf_verify(mp, bp);
-
-	if (unlikely(XFS_TEST_ERROR(!agf_ok, mp, XFS_ERRTAG_ALLOC_READ_AGF,
-			XFS_RANDOM_ALLOC_READ_AGF))) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	    !xfs_buf_verify_cksum(bp, offsetof(struct xfs_agf, agf_crc)))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (XFS_TEST_ERROR(!xfs_agf_verify(mp, bp), mp,
+				XFS_ERRTAG_ALLOC_READ_AGF,
+				XFS_RANDOM_ALLOC_READ_AGF))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -2258,8 +2255,8 @@ xfs_agf_write_verify(
 	struct xfs_buf_log_item	*bip = bp->b_fspriv;
 
 	if (!xfs_agf_verify(mp, bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_alloc_btree.c b/fs/xfs/xfs_alloc_btree.c
index 144d3b0..cc1eadc 100644
--- a/fs/xfs/xfs_alloc_btree.c
+++ b/fs/xfs/xfs_alloc_btree.c
@@ -355,12 +355,14 @@ static void
 xfs_allocbt_read_verify(
 	struct xfs_buf	*bp)
 {
-	if (!(xfs_btree_sblock_verify_crc(bp) &&
-	      xfs_allocbt_verify(bp))) {
-		trace_xfs_btree_corrupt(bp, _RET_IP_);
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     bp->b_target->bt_mount, bp->b_addr);
+	if (!xfs_btree_sblock_verify_crc(bp))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_allocbt_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+
+	if (bp->b_error) {
+		trace_xfs_btree_corrupt(bp, _RET_IP_);
+		xfs_verifier_error(bp);
 	}
 }
 
@@ -370,9 +372,8 @@ xfs_allocbt_write_verify(
 {
 	if (!xfs_allocbt_verify(bp)) {
 		trace_xfs_btree_corrupt(bp, _RET_IP_);
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     bp->b_target->bt_mount, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 	xfs_btree_sblock_calc_crc(bp);
diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
index b552378..fe9587f 100644
--- a/fs/xfs/xfs_attr_leaf.c
+++ b/fs/xfs/xfs_attr_leaf.c
@@ -213,8 +213,8 @@ xfs_attr3_leaf_write_verify(
 	struct xfs_attr3_leaf_hdr *hdr3 = bp->b_addr;
 
 	if (!xfs_attr3_leaf_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
@@ -239,12 +239,14 @@ xfs_attr3_leaf_read_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
-	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_buf_verify_cksum(bp, XFS_ATTR3_LEAF_CRC_OFF)) ||
-	    !xfs_attr3_leaf_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	     !xfs_buf_verify_cksum(bp, XFS_ATTR3_LEAF_CRC_OFF))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_attr3_leaf_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 const struct xfs_buf_ops xfs_attr3_leaf_buf_ops = {
diff --git a/fs/xfs/xfs_attr_remote.c b/fs/xfs/xfs_attr_remote.c
index 5549d69..6e37823 100644
--- a/fs/xfs/xfs_attr_remote.c
+++ b/fs/xfs/xfs_attr_remote.c
@@ -125,7 +125,6 @@ xfs_attr3_rmt_read_verify(
 	struct xfs_mount *mp = bp->b_target->bt_mount;
 	char		*ptr;
 	int		len;
-	bool		corrupt = false;
 	xfs_daddr_t	bno;
 
 	/* no verification of non-crc buffers */
@@ -140,11 +139,11 @@ xfs_attr3_rmt_read_verify(
 	while (len > 0) {
 		if (!xfs_verify_cksum(ptr, XFS_LBSIZE(mp),
 				      XFS_ATTR3_RMT_CRC_OFF)) {
-			corrupt = true;
+			xfs_buf_ioerror(bp, EFSBADCRC);
 			break;
 		}
 		if (!xfs_attr3_rmt_verify(mp, ptr, XFS_LBSIZE(mp), bno)) {
-			corrupt = true;
+			xfs_buf_ioerror(bp, EFSCORRUPTED);
 			break;
 		}
 		len -= XFS_LBSIZE(mp);
@@ -152,10 +151,9 @@ xfs_attr3_rmt_read_verify(
 		bno += mp->m_bsize;
 	}
 
-	if (corrupt) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
-		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	} else
+	if (bp->b_error)
+		xfs_verifier_error(bp);
+	else
 		ASSERT(len == 0);
 }
 
@@ -180,9 +178,8 @@ xfs_attr3_rmt_write_verify(
 
 	while (len > 0) {
 		if (!xfs_attr3_rmt_verify(mp, ptr, XFS_LBSIZE(mp), bno)) {
-			XFS_CORRUPTION_ERROR(__func__,
-					    XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 			xfs_buf_ioerror(bp, EFSCORRUPTED);
+			xfs_verifier_error(bp);
 			return;
 		}
 		if (bip) {
diff --git a/fs/xfs/xfs_bmap_btree.c b/fs/xfs/xfs_bmap_btree.c
index 706bc3f..818d546 100644
--- a/fs/xfs/xfs_bmap_btree.c
+++ b/fs/xfs/xfs_bmap_btree.c
@@ -780,12 +780,14 @@ static void
 xfs_bmbt_read_verify(
 	struct xfs_buf	*bp)
 {
-	if (!(xfs_btree_lblock_verify_crc(bp) &&
-	      xfs_bmbt_verify(bp))) {
-		trace_xfs_btree_corrupt(bp, _RET_IP_);
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     bp->b_target->bt_mount, bp->b_addr);
+	if (!xfs_btree_lblock_verify_crc(bp))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_bmbt_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+
+	if (bp->b_error) {
+		trace_xfs_btree_corrupt(bp, _RET_IP_);
+		xfs_verifier_error(bp);
 	}
 }
 
@@ -794,11 +796,9 @@ xfs_bmbt_write_verify(
 	struct xfs_buf	*bp)
 {
 	if (!xfs_bmbt_verify(bp)) {
-		xfs_warn(bp->b_target->bt_mount, "bmbt daddr 0x%llx failed", bp->b_bn);
 		trace_xfs_btree_corrupt(bp, _RET_IP_);
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     bp->b_target->bt_mount, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 	xfs_btree_lblock_calc_crc(bp);
diff --git a/fs/xfs/xfs_da_btree.c b/fs/xfs/xfs_da_btree.c
index 75ef990..1f5af79 100644
--- a/fs/xfs/xfs_da_btree.c
+++ b/fs/xfs/xfs_da_btree.c
@@ -185,8 +185,8 @@ xfs_da3_node_write_verify(
 	struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
 
 	if (!xfs_da3_node_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
@@ -209,17 +209,20 @@ static void
 xfs_da3_node_read_verify(
 	struct xfs_buf		*bp)
 {
-	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_da_blkinfo	*info = bp->b_addr;
 
 	switch (be16_to_cpu(info->magic)) {
 		case XFS_DA3_NODE_MAGIC:
-			if (!xfs_buf_verify_cksum(bp, XFS_DA3_NODE_CRC_OFF))
+			if (!xfs_buf_verify_cksum(bp, XFS_DA3_NODE_CRC_OFF)) {
+				xfs_buf_ioerror(bp, EFSBADCRC);
 				break;
+			}
 			/* fall through */
 		case XFS_DA_NODE_MAGIC:
-			if (!xfs_da3_node_verify(bp))
+			if (!xfs_da3_node_verify(bp)) {
+				xfs_buf_ioerror(bp, EFSCORRUPTED);
 				break;
+			}
 			return;
 		case XFS_ATTR_LEAF_MAGIC:
 		case XFS_ATTR3_LEAF_MAGIC:
@@ -236,8 +239,7 @@ xfs_da3_node_read_verify(
 	}
 
 	/* corrupt block */
-	XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
-	xfs_buf_ioerror(bp, EFSCORRUPTED);
+	xfs_verifier_error(bp);
 }
 
 const struct xfs_buf_ops xfs_da3_node_buf_ops = {
diff --git a/fs/xfs/xfs_dir2_block.c b/fs/xfs/xfs_dir2_block.c
index 724377e..4f6a38c 100644
--- a/fs/xfs/xfs_dir2_block.c
+++ b/fs/xfs/xfs_dir2_block.c
@@ -89,12 +89,14 @@ xfs_dir3_block_read_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
-	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_buf_verify_cksum(bp, XFS_DIR3_DATA_CRC_OFF)) ||
-	    !xfs_dir3_block_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	     !xfs_buf_verify_cksum(bp, XFS_DIR3_DATA_CRC_OFF))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_dir3_block_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -106,8 +108,8 @@ xfs_dir3_block_write_verify(
 	struct xfs_dir3_blk_hdr	*hdr3 = bp->b_addr;
 
 	if (!xfs_dir3_block_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_dir2_data.c b/fs/xfs/xfs_dir2_data.c
index 74ae85e..afa4ad5 100644
--- a/fs/xfs/xfs_dir2_data.c
+++ b/fs/xfs/xfs_dir2_data.c
@@ -241,7 +241,6 @@ static void
 xfs_dir3_data_reada_verify(
 	struct xfs_buf		*bp)
 {
-	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_dir2_data_hdr *hdr = bp->b_addr;
 
 	switch (hdr->magic) {
@@ -255,8 +254,8 @@ xfs_dir3_data_reada_verify(
 		xfs_dir3_data_verify(bp);
 		return;
 	default:
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, hdr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		break;
 	}
 }
@@ -267,12 +266,14 @@ xfs_dir3_data_read_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
-	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_buf_verify_cksum(bp, XFS_DIR3_DATA_CRC_OFF)) ||
-	    !xfs_dir3_data_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	     !xfs_buf_verify_cksum(bp, XFS_DIR3_DATA_CRC_OFF))
+		 xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_dir3_data_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -284,8 +285,8 @@ xfs_dir3_data_write_verify(
 	struct xfs_dir3_blk_hdr	*hdr3 = bp->b_addr;
 
 	if (!xfs_dir3_data_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_dir2_leaf.c b/fs/xfs/xfs_dir2_leaf.c
index dffb61b..d36e97d 100644
--- a/fs/xfs/xfs_dir2_leaf.c
+++ b/fs/xfs/xfs_dir2_leaf.c
@@ -179,12 +179,14 @@ __read_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
-	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF)) ||
-	    !xfs_dir3_leaf_verify(bp, magic)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	     !xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_dir3_leaf_verify(bp, magic))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -197,8 +199,8 @@ __write_verify(
 	struct xfs_dir3_leaf_hdr *hdr3 = bp->b_addr;
 
 	if (!xfs_dir3_leaf_verify(bp, magic)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_dir2_node.c b/fs/xfs/xfs_dir2_node.c
index 0904b20..cb434d7 100644
--- a/fs/xfs/xfs_dir2_node.c
+++ b/fs/xfs/xfs_dir2_node.c
@@ -115,12 +115,14 @@ xfs_dir3_free_read_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
-	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_buf_verify_cksum(bp, XFS_DIR3_FREE_CRC_OFF)) ||
-	    !xfs_dir3_free_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	    !xfs_buf_verify_cksum(bp, XFS_DIR3_FREE_CRC_OFF))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_dir3_free_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -132,8 +134,8 @@ xfs_dir3_free_write_verify(
 	struct xfs_dir3_blk_hdr	*hdr3 = bp->b_addr;
 
 	if (!xfs_dir3_free_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_dquot_buf.c b/fs/xfs/xfs_dquot_buf.c
index d401457..610da81 100644
--- a/fs/xfs/xfs_dquot_buf.c
+++ b/fs/xfs/xfs_dquot_buf.c
@@ -257,10 +257,13 @@ xfs_dquot_buf_read_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
-	if (!xfs_dquot_buf_verify_crc(mp, bp) || !xfs_dquot_buf_verify(mp, bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (!xfs_dquot_buf_verify_crc(mp, bp))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_dquot_buf_verify(mp, bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 /*
@@ -275,8 +278,8 @@ xfs_dquot_buf_write_verify(
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
 	if (!xfs_dquot_buf_verify(mp, bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 }
diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
index 4657586..8aa720d 100644
--- a/fs/xfs/xfs_ialloc.c
+++ b/fs/xfs/xfs_ialloc.c
@@ -1573,13 +1573,17 @@ xfs_agi_read_verify(
 	if (xfs_sb_version_hascrc(&mp->m_sb))
 		agi_ok = xfs_buf_verify_cksum(bp, XFS_AGI_CRC_OFF);
 
+	if (!agi_ok)
+		xfs_buf_ioerror(bp, EFSBADCRC);
+
 	agi_ok = agi_ok && xfs_agi_verify(bp);
 
 	if (unlikely(XFS_TEST_ERROR(!agi_ok, mp, XFS_ERRTAG_IALLOC_READ_AGI,
-			XFS_RANDOM_IALLOC_READ_AGI))) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+			XFS_RANDOM_IALLOC_READ_AGI)))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -1590,8 +1594,8 @@ xfs_agi_write_verify(
 	struct xfs_buf_log_item	*bip = bp->b_fspriv;
 
 	if (!xfs_agi_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_ialloc_btree.c b/fs/xfs/xfs_ialloc_btree.c
index 0028c50..7e309b1 100644
--- a/fs/xfs/xfs_ialloc_btree.c
+++ b/fs/xfs/xfs_ialloc_btree.c
@@ -243,12 +243,14 @@ static void
 xfs_inobt_read_verify(
 	struct xfs_buf	*bp)
 {
-	if (!(xfs_btree_sblock_verify_crc(bp) &&
-	      xfs_inobt_verify(bp))) {
-		trace_xfs_btree_corrupt(bp, _RET_IP_);
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     bp->b_target->bt_mount, bp->b_addr);
+	if (!xfs_btree_sblock_verify_crc(bp))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_inobt_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+
+	if (bp->b_error) {
+		trace_xfs_btree_corrupt(bp, _RET_IP_);
+		xfs_verifier_error(bp);
 	}
 }
 
@@ -258,9 +260,8 @@ xfs_inobt_write_verify(
 {
 	if (!xfs_inobt_verify(bp)) {
 		trace_xfs_btree_corrupt(bp, _RET_IP_);
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     bp->b_target->bt_mount, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 	xfs_btree_sblock_calc_crc(bp);
diff --git a/fs/xfs/xfs_inode_buf.c b/fs/xfs/xfs_inode_buf.c
index 606b43a..24e9939 100644
--- a/fs/xfs/xfs_inode_buf.c
+++ b/fs/xfs/xfs_inode_buf.c
@@ -102,8 +102,7 @@ xfs_inode_buf_verify(
 			}
 
 			xfs_buf_ioerror(bp, EFSCORRUPTED);
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_HIGH,
-					     mp, dip);
+			xfs_verifier_error(bp);
 #ifdef DEBUG
 			xfs_alert(mp,
 				"bad inode magic/vsn daddr %lld #%d (magic=%x)",
diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
index 818359f..b134aa8 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -614,7 +614,7 @@ xfs_sb_read_verify(
 			/* Only fail bad secondaries on a known V5 filesystem */
 			if (bp->b_bn == XFS_SB_DADDR ||
 			    xfs_sb_version_hascrc(&mp->m_sb)) {
-				error = EFSCORRUPTED;
+				error = EFSBADCRC;
 				goto out_error;
 			}
 		}
@@ -623,10 +623,9 @@ xfs_sb_read_verify(
 
 out_error:
 	if (error) {
-		if (error == EFSCORRUPTED)
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-					     mp, bp->b_addr);
 		xfs_buf_ioerror(bp, error);
+		if (error == EFSCORRUPTED || error == EFSBADCRC)
+			xfs_verifier_error(bp);
 	}
 }
 
@@ -661,9 +660,8 @@ xfs_sb_write_verify(
 
 	error = xfs_sb_verify(bp, false);
 	if (error) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     mp, bp->b_addr);
 		xfs_buf_ioerror(bp, error);
+		xfs_verifier_error(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_symlink_remote.c b/fs/xfs/xfs_symlink_remote.c
index defa09f..9b32052 100644
--- a/fs/xfs/xfs_symlink_remote.c
+++ b/fs/xfs/xfs_symlink_remote.c
@@ -133,11 +133,13 @@ xfs_symlink_read_verify(
 	if (!xfs_sb_version_hascrc(&mp->m_sb))
 		return;
 
-	if (!xfs_buf_verify_cksum(bp, XFS_SYMLINK_CRC_OFF) ||
-	    !xfs_symlink_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (!xfs_buf_verify_cksum(bp, XFS_SYMLINK_CRC_OFF))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_symlink_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -152,8 +154,8 @@ xfs_symlink_write_verify(
 		return;
 
 	if (!xfs_symlink_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
-- 
1.7.1

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

Re: [PATCH 9/9] xfs: modify verifiers to differentiate CRC from other errors

[Brian Foster]

On 02/18/2014 06:52 PM, Eric Sandeen wrote:
> Modify all read & write verifiers to differentiate
> between CRC errors and other inconsistencies.
> 
> This sets the appropriate error number on bp->b_error,
> and then calls xfs_verifier_error() if something went
> wrong.  That function will issue the appropriate message
> to the user.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/xfs_alloc.c          |   37 +++++++++++++++++--------------------
>  fs/xfs/xfs_alloc_btree.c    |   15 ++++++++-------
>  fs/xfs/xfs_attr_leaf.c      |   14 ++++++++------
>  fs/xfs/xfs_attr_remote.c    |   15 ++++++---------
>  fs/xfs/xfs_bmap_btree.c     |   16 ++++++++--------
>  fs/xfs/xfs_da_btree.c       |   14 ++++++++------
>  fs/xfs/xfs_dir2_block.c     |   14 ++++++++------
>  fs/xfs/xfs_dir2_data.c      |   17 +++++++++--------
>  fs/xfs/xfs_dir2_leaf.c      |   14 ++++++++------
>  fs/xfs/xfs_dir2_node.c      |   14 ++++++++------
>  fs/xfs/xfs_dquot_buf.c      |   11 +++++++----
>  fs/xfs/xfs_ialloc.c         |   12 ++++++++----
>  fs/xfs/xfs_ialloc_btree.c   |   15 ++++++++-------
>  fs/xfs/xfs_inode_buf.c      |    3 +--
>  fs/xfs/xfs_sb.c             |   10 ++++------
>  fs/xfs/xfs_symlink_remote.c |   12 +++++++-----
>  16 files changed, 123 insertions(+), 110 deletions(-)
> 
> diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
> index 9c7cf3d..9a93601 100644
> --- a/fs/xfs/xfs_alloc.c
> +++ b/fs/xfs/xfs_alloc.c
> @@ -474,7 +474,6 @@ xfs_agfl_read_verify(
>  	struct xfs_buf	*bp)
>  {
>  	struct xfs_mount *mp = bp->b_target->bt_mount;
> -	int		agfl_ok = 1;
>  
>  	/*
>  	 * There is no verification of non-crc AGFLs because mkfs does not
> @@ -485,14 +484,13 @@ xfs_agfl_read_verify(
>  	if (!xfs_sb_version_hascrc(&mp->m_sb))
>  		return;
>  
> -	agfl_ok = xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF);
> -
> -	agfl_ok = agfl_ok && xfs_agfl_verify(bp);
> -
> -	if (!agfl_ok) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> +	if (!xfs_buf_verify_cksum(bp, offsetof(struct xfs_agfl, agfl_crc)))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (!xfs_agfl_verify(bp))

Obviously you added the CRC_OFF directives earlier in the set. It looks
like this patch squashed a couple of them (XFS_AGF_CRC_OFF as well).

>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> -	}
> +
> +	if (bp->b_error)
> +		xfs_verifier_error(bp);
>  }
>  
...
> diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
> index 4657586..8aa720d 100644
> --- a/fs/xfs/xfs_ialloc.c
> +++ b/fs/xfs/xfs_ialloc.c
> @@ -1573,13 +1573,17 @@ xfs_agi_read_verify(
>  	if (xfs_sb_version_hascrc(&mp->m_sb))
>  		agi_ok = xfs_buf_verify_cksum(bp, XFS_AGI_CRC_OFF);
>  
> +	if (!agi_ok)
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +
>  	agi_ok = agi_ok && xfs_agi_verify(bp);
>  
>  	if (unlikely(XFS_TEST_ERROR(!agi_ok, mp, XFS_ERRTAG_IALLOC_READ_AGI,
> -			XFS_RANDOM_IALLOC_READ_AGI))) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> +			XFS_RANDOM_IALLOC_READ_AGI)))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> -	}
> +
> +	if (bp->b_error)
> +		xfs_verifier_error(bp);
>  }

Any reason not to use the same if/else pattern here that the others are
now using (i.e., similar to xfs_agf_read_verify(), removing the need for
agi_ok)?

Brian

>  
>  static void
> @@ -1590,8 +1594,8 @@ xfs_agi_write_verify(
>  	struct xfs_buf_log_item	*bip = bp->b_fspriv;
>  
>  	if (!xfs_agi_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  
> diff --git a/fs/xfs/xfs_ialloc_btree.c b/fs/xfs/xfs_ialloc_btree.c
> index 0028c50..7e309b1 100644
> --- a/fs/xfs/xfs_ialloc_btree.c
> +++ b/fs/xfs/xfs_ialloc_btree.c
> @@ -243,12 +243,14 @@ static void
>  xfs_inobt_read_verify(
>  	struct xfs_buf	*bp)
>  {
> -	if (!(xfs_btree_sblock_verify_crc(bp) &&
> -	      xfs_inobt_verify(bp))) {
> -		trace_xfs_btree_corrupt(bp, _RET_IP_);
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -				     bp->b_target->bt_mount, bp->b_addr);
> +	if (!xfs_btree_sblock_verify_crc(bp))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (!xfs_inobt_verify(bp))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +
> +	if (bp->b_error) {
> +		trace_xfs_btree_corrupt(bp, _RET_IP_);
> +		xfs_verifier_error(bp);
>  	}
>  }
>  
> @@ -258,9 +260,8 @@ xfs_inobt_write_verify(
>  {
>  	if (!xfs_inobt_verify(bp)) {
>  		trace_xfs_btree_corrupt(bp, _RET_IP_);
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -				     bp->b_target->bt_mount, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  	xfs_btree_sblock_calc_crc(bp);
> diff --git a/fs/xfs/xfs_inode_buf.c b/fs/xfs/xfs_inode_buf.c
> index 606b43a..24e9939 100644
> --- a/fs/xfs/xfs_inode_buf.c
> +++ b/fs/xfs/xfs_inode_buf.c
> @@ -102,8 +102,7 @@ xfs_inode_buf_verify(
>  			}
>  
>  			xfs_buf_ioerror(bp, EFSCORRUPTED);
> -			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_HIGH,
> -					     mp, dip);
> +			xfs_verifier_error(bp);
>  #ifdef DEBUG
>  			xfs_alert(mp,
>  				"bad inode magic/vsn daddr %lld #%d (magic=%x)",
> diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
> index 818359f..b134aa8 100644
> --- a/fs/xfs/xfs_sb.c
> +++ b/fs/xfs/xfs_sb.c
> @@ -614,7 +614,7 @@ xfs_sb_read_verify(
>  			/* Only fail bad secondaries on a known V5 filesystem */
>  			if (bp->b_bn == XFS_SB_DADDR ||
>  			    xfs_sb_version_hascrc(&mp->m_sb)) {
> -				error = EFSCORRUPTED;
> +				error = EFSBADCRC;
>  				goto out_error;
>  			}
>  		}
> @@ -623,10 +623,9 @@ xfs_sb_read_verify(
>  
>  out_error:
>  	if (error) {
> -		if (error == EFSCORRUPTED)
> -			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -					     mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, error);
> +		if (error == EFSCORRUPTED || error == EFSBADCRC)
> +			xfs_verifier_error(bp);
>  	}
>  }
>  
> @@ -661,9 +660,8 @@ xfs_sb_write_verify(
>  
>  	error = xfs_sb_verify(bp, false);
>  	if (error) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -				     mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, error);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  
> diff --git a/fs/xfs/xfs_symlink_remote.c b/fs/xfs/xfs_symlink_remote.c
> index defa09f..9b32052 100644
> --- a/fs/xfs/xfs_symlink_remote.c
> +++ b/fs/xfs/xfs_symlink_remote.c
> @@ -133,11 +133,13 @@ xfs_symlink_read_verify(
>  	if (!xfs_sb_version_hascrc(&mp->m_sb))
>  		return;
>  
> -	if (!xfs_buf_verify_cksum(bp, XFS_SYMLINK_CRC_OFF) ||
> -	    !xfs_symlink_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> +	if (!xfs_buf_verify_cksum(bp, XFS_SYMLINK_CRC_OFF))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (!xfs_symlink_verify(bp))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> -	}
> +
> +	if (bp->b_error)
> +		xfs_verifier_error(bp);
>  }
>  
>  static void
> @@ -152,8 +154,8 @@ xfs_symlink_write_verify(
>  		return;
>  
>  	if (!xfs_symlink_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  
> 

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

Re: [PATCH 9/9] xfs: modify verifiers to differentiate CRC from other errors

[Eric Sandeen]

On 2/19/14, 8:01 AM, Brian Foster wrote:
> On 02/18/2014 06:52 PM, Eric Sandeen wrote:
>> Modify all read & write verifiers to differentiate
>> between CRC errors and other inconsistencies.
>>
>> This sets the appropriate error number on bp->b_error,
>> and then calls xfs_verifier_error() if something went
>> wrong.  That function will issue the appropriate message
>> to the user.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---

...

>> @@ -485,14 +484,13 @@ xfs_agfl_read_verify(
>>  	if (!xfs_sb_version_hascrc(&mp->m_sb))
>>  		return;
>>  
>> -	agfl_ok = xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF);
>> -
>> -	agfl_ok = agfl_ok && xfs_agfl_verify(bp);
>> -
>> -	if (!agfl_ok) {
>> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>> +	if (!xfs_buf_verify_cksum(bp, offsetof(struct xfs_agfl, agfl_crc)))
>> +		xfs_buf_ioerror(bp, EFSBADCRC);
>> +	else if (!xfs_agfl_verify(bp))
> 
> Obviously you added the CRC_OFF directives earlier in the set. It looks
> like this patch squashed a couple of them (XFS_AGF_CRC_OFF as well).

Whoops, no idea how that happened :/ Thanks.

>>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
>> -	}
>> +
>> +	if (bp->b_error)
>> +		xfs_verifier_error(bp);
>>  }
>>  
> ...
>> diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
>> index 4657586..8aa720d 100644
>> --- a/fs/xfs/xfs_ialloc.c
>> +++ b/fs/xfs/xfs_ialloc.c
>> @@ -1573,13 +1573,17 @@ xfs_agi_read_verify(
>>  	if (xfs_sb_version_hascrc(&mp->m_sb))
>>  		agi_ok = xfs_buf_verify_cksum(bp, XFS_AGI_CRC_OFF);
>>  
>> +	if (!agi_ok)
>> +		xfs_buf_ioerror(bp, EFSBADCRC);
>> +
>>  	agi_ok = agi_ok && xfs_agi_verify(bp);
>>  
>>  	if (unlikely(XFS_TEST_ERROR(!agi_ok, mp, XFS_ERRTAG_IALLOC_READ_AGI,
>> -			XFS_RANDOM_IALLOC_READ_AGI))) {
>> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>> +			XFS_RANDOM_IALLOC_READ_AGI)))
>>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
>> -	}
>> +
>> +	if (bp->b_error)
>> +		xfs_verifier_error(bp);
>>  }
> 
> Any reason not to use the same if/else pattern here that the others are
> now using (i.e., similar to xfs_agf_read_verify(), removing the need for
> agi_ok)?

Hm I was thinking it was the weird XFS_TEST_ERROR construction but
xfs_agf_read_verify has that too.  I'll take another look, thanks.

(TBH all these verifiers are so similar, I wish there were a way
to not do so much of what is essentially cut and paste with different
error tags & offsets...)

Thanks for the careful review,

-Eric

> Brian

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

[PATCH 9/9 V2] xfs: modify verifiers to differentiate CRC from other errors

[Eric Sandeen]

Modify all read & write verifiers to differentiate
between CRC errors and other inconsistencies.

This sets the appropriate error number on bp->b_error,
and then calls xfs_verifier_error() if something went
wrong.  That function will issue the appropriate message
to the user.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: Fix inexplicably lost XFS_*_CRC_OFF macros, and
rework xfs_agi_read_verify() to be similar to other
verifiers, per Brian's review

diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
index 9c7cf3d..c1cf6a3 100644
--- a/fs/xfs/xfs_alloc.c
+++ b/fs/xfs/xfs_alloc.c
@@ -474,7 +474,6 @@ xfs_agfl_read_verify(
 	struct xfs_buf	*bp)
 {
 	struct xfs_mount *mp = bp->b_target->bt_mount;
-	int		agfl_ok = 1;
 
 	/*
 	 * There is no verification of non-crc AGFLs because mkfs does not
@@ -485,14 +484,13 @@ xfs_agfl_read_verify(
 	if (!xfs_sb_version_hascrc(&mp->m_sb))
 		return;
 
-	agfl_ok = xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF);
-
-	agfl_ok = agfl_ok && xfs_agfl_verify(bp);
-
-	if (!agfl_ok) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (!xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_agfl_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -507,8 +505,8 @@ xfs_agfl_write_verify(
 		return;
 
 	if (!xfs_agfl_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
@@ -2236,18 +2234,17 @@ xfs_agf_read_verify(
 	struct xfs_buf	*bp)
 {
 	struct xfs_mount *mp = bp->b_target->bt_mount;
-	int		agf_ok = 1;
-
-	if (xfs_sb_version_hascrc(&mp->m_sb))
-		agf_ok = xfs_buf_verify_cksum(bp, XFS_AGF_CRC_OFF);
 
-	agf_ok = agf_ok && xfs_agf_verify(mp, bp);
-
-	if (unlikely(XFS_TEST_ERROR(!agf_ok, mp, XFS_ERRTAG_ALLOC_READ_AGF,
-			XFS_RANDOM_ALLOC_READ_AGF))) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	    !xfs_buf_verify_cksum(bp, XFS_AGF_CRC_OFF))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (XFS_TEST_ERROR(!xfs_agf_verify(mp, bp), mp,
+				XFS_ERRTAG_ALLOC_READ_AGF,
+				XFS_RANDOM_ALLOC_READ_AGF))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -2258,8 +2255,8 @@ xfs_agf_write_verify(
 	struct xfs_buf_log_item	*bip = bp->b_fspriv;
 
 	if (!xfs_agf_verify(mp, bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_alloc_btree.c b/fs/xfs/xfs_alloc_btree.c
index 144d3b0..cc1eadc 100644
--- a/fs/xfs/xfs_alloc_btree.c
+++ b/fs/xfs/xfs_alloc_btree.c
@@ -355,12 +355,14 @@ static void
 xfs_allocbt_read_verify(
 	struct xfs_buf	*bp)
 {
-	if (!(xfs_btree_sblock_verify_crc(bp) &&
-	      xfs_allocbt_verify(bp))) {
-		trace_xfs_btree_corrupt(bp, _RET_IP_);
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     bp->b_target->bt_mount, bp->b_addr);
+	if (!xfs_btree_sblock_verify_crc(bp))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_allocbt_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+
+	if (bp->b_error) {
+		trace_xfs_btree_corrupt(bp, _RET_IP_);
+		xfs_verifier_error(bp);
 	}
 }
 
@@ -370,9 +372,8 @@ xfs_allocbt_write_verify(
 {
 	if (!xfs_allocbt_verify(bp)) {
 		trace_xfs_btree_corrupt(bp, _RET_IP_);
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     bp->b_target->bt_mount, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 	xfs_btree_sblock_calc_crc(bp);
diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
index b552378..fe9587f 100644
--- a/fs/xfs/xfs_attr_leaf.c
+++ b/fs/xfs/xfs_attr_leaf.c
@@ -213,8 +213,8 @@ xfs_attr3_leaf_write_verify(
 	struct xfs_attr3_leaf_hdr *hdr3 = bp->b_addr;
 
 	if (!xfs_attr3_leaf_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
@@ -239,12 +239,14 @@ xfs_attr3_leaf_read_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
-	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_buf_verify_cksum(bp, XFS_ATTR3_LEAF_CRC_OFF)) ||
-	    !xfs_attr3_leaf_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	     !xfs_buf_verify_cksum(bp, XFS_ATTR3_LEAF_CRC_OFF))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_attr3_leaf_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 const struct xfs_buf_ops xfs_attr3_leaf_buf_ops = {
diff --git a/fs/xfs/xfs_attr_remote.c b/fs/xfs/xfs_attr_remote.c
index 5549d69..6e37823 100644
--- a/fs/xfs/xfs_attr_remote.c
+++ b/fs/xfs/xfs_attr_remote.c
@@ -125,7 +125,6 @@ xfs_attr3_rmt_read_verify(
 	struct xfs_mount *mp = bp->b_target->bt_mount;
 	char		*ptr;
 	int		len;
-	bool		corrupt = false;
 	xfs_daddr_t	bno;
 
 	/* no verification of non-crc buffers */
@@ -140,11 +139,11 @@ xfs_attr3_rmt_read_verify(
 	while (len > 0) {
 		if (!xfs_verify_cksum(ptr, XFS_LBSIZE(mp),
 				      XFS_ATTR3_RMT_CRC_OFF)) {
-			corrupt = true;
+			xfs_buf_ioerror(bp, EFSBADCRC);
 			break;
 		}
 		if (!xfs_attr3_rmt_verify(mp, ptr, XFS_LBSIZE(mp), bno)) {
-			corrupt = true;
+			xfs_buf_ioerror(bp, EFSCORRUPTED);
 			break;
 		}
 		len -= XFS_LBSIZE(mp);
@@ -152,10 +151,9 @@ xfs_attr3_rmt_read_verify(
 		bno += mp->m_bsize;
 	}
 
-	if (corrupt) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
-		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	} else
+	if (bp->b_error)
+		xfs_verifier_error(bp);
+	else
 		ASSERT(len == 0);
 }
 
@@ -180,9 +178,8 @@ xfs_attr3_rmt_write_verify(
 
 	while (len > 0) {
 		if (!xfs_attr3_rmt_verify(mp, ptr, XFS_LBSIZE(mp), bno)) {
-			XFS_CORRUPTION_ERROR(__func__,
-					    XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 			xfs_buf_ioerror(bp, EFSCORRUPTED);
+			xfs_verifier_error(bp);
 			return;
 		}
 		if (bip) {
diff --git a/fs/xfs/xfs_bmap_btree.c b/fs/xfs/xfs_bmap_btree.c
index 706bc3f..818d546 100644
--- a/fs/xfs/xfs_bmap_btree.c
+++ b/fs/xfs/xfs_bmap_btree.c
@@ -780,12 +780,14 @@ static void
 xfs_bmbt_read_verify(
 	struct xfs_buf	*bp)
 {
-	if (!(xfs_btree_lblock_verify_crc(bp) &&
-	      xfs_bmbt_verify(bp))) {
-		trace_xfs_btree_corrupt(bp, _RET_IP_);
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     bp->b_target->bt_mount, bp->b_addr);
+	if (!xfs_btree_lblock_verify_crc(bp))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_bmbt_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+
+	if (bp->b_error) {
+		trace_xfs_btree_corrupt(bp, _RET_IP_);
+		xfs_verifier_error(bp);
 	}
 }
 
@@ -794,11 +796,9 @@ xfs_bmbt_write_verify(
 	struct xfs_buf	*bp)
 {
 	if (!xfs_bmbt_verify(bp)) {
-		xfs_warn(bp->b_target->bt_mount, "bmbt daddr 0x%llx failed", bp->b_bn);
 		trace_xfs_btree_corrupt(bp, _RET_IP_);
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     bp->b_target->bt_mount, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 	xfs_btree_lblock_calc_crc(bp);
diff --git a/fs/xfs/xfs_da_btree.c b/fs/xfs/xfs_da_btree.c
index 75ef990..1f5af79 100644
--- a/fs/xfs/xfs_da_btree.c
+++ b/fs/xfs/xfs_da_btree.c
@@ -185,8 +185,8 @@ xfs_da3_node_write_verify(
 	struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
 
 	if (!xfs_da3_node_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
@@ -209,17 +209,20 @@ static void
 xfs_da3_node_read_verify(
 	struct xfs_buf		*bp)
 {
-	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_da_blkinfo	*info = bp->b_addr;
 
 	switch (be16_to_cpu(info->magic)) {
 		case XFS_DA3_NODE_MAGIC:
-			if (!xfs_buf_verify_cksum(bp, XFS_DA3_NODE_CRC_OFF))
+			if (!xfs_buf_verify_cksum(bp, XFS_DA3_NODE_CRC_OFF)) {
+				xfs_buf_ioerror(bp, EFSBADCRC);
 				break;
+			}
 			/* fall through */
 		case XFS_DA_NODE_MAGIC:
-			if (!xfs_da3_node_verify(bp))
+			if (!xfs_da3_node_verify(bp)) {
+				xfs_buf_ioerror(bp, EFSCORRUPTED);
 				break;
+			}
 			return;
 		case XFS_ATTR_LEAF_MAGIC:
 		case XFS_ATTR3_LEAF_MAGIC:
@@ -236,8 +239,7 @@ xfs_da3_node_read_verify(
 	}
 
 	/* corrupt block */
-	XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
-	xfs_buf_ioerror(bp, EFSCORRUPTED);
+	xfs_verifier_error(bp);
 }
 
 const struct xfs_buf_ops xfs_da3_node_buf_ops = {
diff --git a/fs/xfs/xfs_dir2_block.c b/fs/xfs/xfs_dir2_block.c
index 724377e..4f6a38c 100644
--- a/fs/xfs/xfs_dir2_block.c
+++ b/fs/xfs/xfs_dir2_block.c
@@ -89,12 +89,14 @@ xfs_dir3_block_read_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
-	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_buf_verify_cksum(bp, XFS_DIR3_DATA_CRC_OFF)) ||
-	    !xfs_dir3_block_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	     !xfs_buf_verify_cksum(bp, XFS_DIR3_DATA_CRC_OFF))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_dir3_block_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -106,8 +108,8 @@ xfs_dir3_block_write_verify(
 	struct xfs_dir3_blk_hdr	*hdr3 = bp->b_addr;
 
 	if (!xfs_dir3_block_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_dir2_data.c b/fs/xfs/xfs_dir2_data.c
index 74ae85e..afa4ad5 100644
--- a/fs/xfs/xfs_dir2_data.c
+++ b/fs/xfs/xfs_dir2_data.c
@@ -241,7 +241,6 @@ static void
 xfs_dir3_data_reada_verify(
 	struct xfs_buf		*bp)
 {
-	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_dir2_data_hdr *hdr = bp->b_addr;
 
 	switch (hdr->magic) {
@@ -255,8 +254,8 @@ xfs_dir3_data_reada_verify(
 		xfs_dir3_data_verify(bp);
 		return;
 	default:
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, hdr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		break;
 	}
 }
@@ -267,12 +266,14 @@ xfs_dir3_data_read_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
-	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_buf_verify_cksum(bp, XFS_DIR3_DATA_CRC_OFF)) ||
-	    !xfs_dir3_data_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	     !xfs_buf_verify_cksum(bp, XFS_DIR3_DATA_CRC_OFF))
+		 xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_dir3_data_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -284,8 +285,8 @@ xfs_dir3_data_write_verify(
 	struct xfs_dir3_blk_hdr	*hdr3 = bp->b_addr;
 
 	if (!xfs_dir3_data_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_dir2_leaf.c b/fs/xfs/xfs_dir2_leaf.c
index dffb61b..d36e97d 100644
--- a/fs/xfs/xfs_dir2_leaf.c
+++ b/fs/xfs/xfs_dir2_leaf.c
@@ -179,12 +179,14 @@ __read_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
-	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF)) ||
-	    !xfs_dir3_leaf_verify(bp, magic)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	     !xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_dir3_leaf_verify(bp, magic))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -197,8 +199,8 @@ __write_verify(
 	struct xfs_dir3_leaf_hdr *hdr3 = bp->b_addr;
 
 	if (!xfs_dir3_leaf_verify(bp, magic)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_dir2_node.c b/fs/xfs/xfs_dir2_node.c
index 0904b20..cb434d7 100644
--- a/fs/xfs/xfs_dir2_node.c
+++ b/fs/xfs/xfs_dir2_node.c
@@ -115,12 +115,14 @@ xfs_dir3_free_read_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
-	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
-	     !xfs_buf_verify_cksum(bp, XFS_DIR3_FREE_CRC_OFF)) ||
-	    !xfs_dir3_free_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	    !xfs_buf_verify_cksum(bp, XFS_DIR3_FREE_CRC_OFF))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_dir3_free_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -132,8 +134,8 @@ xfs_dir3_free_write_verify(
 	struct xfs_dir3_blk_hdr	*hdr3 = bp->b_addr;
 
 	if (!xfs_dir3_free_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_dquot_buf.c b/fs/xfs/xfs_dquot_buf.c
index d401457..610da81 100644
--- a/fs/xfs/xfs_dquot_buf.c
+++ b/fs/xfs/xfs_dquot_buf.c
@@ -257,10 +257,13 @@ xfs_dquot_buf_read_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
-	if (!xfs_dquot_buf_verify_crc(mp, bp) || !xfs_dquot_buf_verify(mp, bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (!xfs_dquot_buf_verify_crc(mp, bp))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_dquot_buf_verify(mp, bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 /*
@@ -275,8 +278,8 @@ xfs_dquot_buf_write_verify(
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
 	if (!xfs_dquot_buf_verify(mp, bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 }
diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
index 4657586..5959b3b 100644
--- a/fs/xfs/xfs_ialloc.c
+++ b/fs/xfs/xfs_ialloc.c
@@ -1568,18 +1568,17 @@ xfs_agi_read_verify(
 	struct xfs_buf	*bp)
 {
 	struct xfs_mount *mp = bp->b_target->bt_mount;
-	int		agi_ok = 1;
 
-	if (xfs_sb_version_hascrc(&mp->m_sb))
-		agi_ok = xfs_buf_verify_cksum(bp, XFS_AGI_CRC_OFF);
-
-	agi_ok = agi_ok && xfs_agi_verify(bp);
-
-	if (unlikely(XFS_TEST_ERROR(!agi_ok, mp, XFS_ERRTAG_IALLOC_READ_AGI,
-			XFS_RANDOM_IALLOC_READ_AGI))) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	    !xfs_buf_verify_cksum(bp, XFS_AGI_CRC_OFF))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (XFS_TEST_ERROR(!xfs_agi_verify(bp), mp,
+				XFS_ERRTAG_IALLOC_READ_AGI,
+				XFS_RANDOM_IALLOC_READ_AGI))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -1590,8 +1589,8 @@ xfs_agi_write_verify(
 	struct xfs_buf_log_item	*bip = bp->b_fspriv;
 
 	if (!xfs_agi_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_ialloc_btree.c b/fs/xfs/xfs_ialloc_btree.c
index 0028c50..7e309b1 100644
--- a/fs/xfs/xfs_ialloc_btree.c
+++ b/fs/xfs/xfs_ialloc_btree.c
@@ -243,12 +243,14 @@ static void
 xfs_inobt_read_verify(
 	struct xfs_buf	*bp)
 {
-	if (!(xfs_btree_sblock_verify_crc(bp) &&
-	      xfs_inobt_verify(bp))) {
-		trace_xfs_btree_corrupt(bp, _RET_IP_);
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     bp->b_target->bt_mount, bp->b_addr);
+	if (!xfs_btree_sblock_verify_crc(bp))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_inobt_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+
+	if (bp->b_error) {
+		trace_xfs_btree_corrupt(bp, _RET_IP_);
+		xfs_verifier_error(bp);
 	}
 }
 
@@ -258,9 +260,8 @@ xfs_inobt_write_verify(
 {
 	if (!xfs_inobt_verify(bp)) {
 		trace_xfs_btree_corrupt(bp, _RET_IP_);
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     bp->b_target->bt_mount, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 	xfs_btree_sblock_calc_crc(bp);
diff --git a/fs/xfs/xfs_inode_buf.c b/fs/xfs/xfs_inode_buf.c
index 606b43a..24e9939 100644
--- a/fs/xfs/xfs_inode_buf.c
+++ b/fs/xfs/xfs_inode_buf.c
@@ -102,8 +102,7 @@ xfs_inode_buf_verify(
 			}
 
 			xfs_buf_ioerror(bp, EFSCORRUPTED);
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_HIGH,
-					     mp, dip);
+			xfs_verifier_error(bp);
 #ifdef DEBUG
 			xfs_alert(mp,
 				"bad inode magic/vsn daddr %lld #%d (magic=%x)",
diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
index 818359f..b134aa8 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -614,7 +614,7 @@ xfs_sb_read_verify(
 			/* Only fail bad secondaries on a known V5 filesystem */
 			if (bp->b_bn == XFS_SB_DADDR ||
 			    xfs_sb_version_hascrc(&mp->m_sb)) {
-				error = EFSCORRUPTED;
+				error = EFSBADCRC;
 				goto out_error;
 			}
 		}
@@ -623,10 +623,9 @@ xfs_sb_read_verify(
 
 out_error:
 	if (error) {
-		if (error == EFSCORRUPTED)
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-					     mp, bp->b_addr);
 		xfs_buf_ioerror(bp, error);
+		if (error == EFSCORRUPTED || error == EFSBADCRC)
+			xfs_verifier_error(bp);
 	}
 }
 
@@ -661,9 +660,8 @@ xfs_sb_write_verify(
 
 	error = xfs_sb_verify(bp, false);
 	if (error) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-				     mp, bp->b_addr);
 		xfs_buf_ioerror(bp, error);
+		xfs_verifier_error(bp);
 		return;
 	}
 
diff --git a/fs/xfs/xfs_symlink_remote.c b/fs/xfs/xfs_symlink_remote.c
index defa09f..9b32052 100644
--- a/fs/xfs/xfs_symlink_remote.c
+++ b/fs/xfs/xfs_symlink_remote.c
@@ -133,11 +133,13 @@ xfs_symlink_read_verify(
 	if (!xfs_sb_version_hascrc(&mp->m_sb))
 		return;
 
-	if (!xfs_buf_verify_cksum(bp, XFS_SYMLINK_CRC_OFF) ||
-	    !xfs_symlink_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+	if (!xfs_buf_verify_cksum(bp, XFS_SYMLINK_CRC_OFF))
+		xfs_buf_ioerror(bp, EFSBADCRC);
+	else if (!xfs_symlink_verify(bp))
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
-	}
+
+	if (bp->b_error)
+		xfs_verifier_error(bp);
 }
 
 static void
@@ -152,8 +154,8 @@ xfs_symlink_write_verify(
 		return;
 
 	if (!xfs_symlink_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
 		xfs_buf_ioerror(bp, EFSCORRUPTED);
+		xfs_verifier_error(bp);
 		return;
 	}
 


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

Re: [PATCH 9/9 V2] xfs: modify verifiers to differentiate CRC from other errors

[Brian Foster]

On 02/19/2014 10:10 PM, Eric Sandeen wrote:
> Modify all read & write verifiers to differentiate
> between CRC errors and other inconsistencies.
> 
> This sets the appropriate error number on bp->b_error,
> and then calls xfs_verifier_error() if something went
> wrong.  That function will issue the appropriate message
> to the user.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 

Looks good to me, thanks Eric.

Reviewed-by: Brian Foster <bfoster@redhat.com>

> V2: Fix inexplicably lost XFS_*_CRC_OFF macros, and
> rework xfs_agi_read_verify() to be similar to other
> verifiers, per Brian's review
> 
> diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
> index 9c7cf3d..c1cf6a3 100644
> --- a/fs/xfs/xfs_alloc.c
> +++ b/fs/xfs/xfs_alloc.c
> @@ -474,7 +474,6 @@ xfs_agfl_read_verify(
>  	struct xfs_buf	*bp)
>  {
>  	struct xfs_mount *mp = bp->b_target->bt_mount;
> -	int		agfl_ok = 1;
>  
>  	/*
>  	 * There is no verification of non-crc AGFLs because mkfs does not
> @@ -485,14 +484,13 @@ xfs_agfl_read_verify(
>  	if (!xfs_sb_version_hascrc(&mp->m_sb))
>  		return;
>  
> -	agfl_ok = xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF);
> -
> -	agfl_ok = agfl_ok && xfs_agfl_verify(bp);
> -
> -	if (!agfl_ok) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> +	if (!xfs_buf_verify_cksum(bp, XFS_AGFL_CRC_OFF))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (!xfs_agfl_verify(bp))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> -	}
> +
> +	if (bp->b_error)
> +		xfs_verifier_error(bp);
>  }
>  
>  static void
> @@ -507,8 +505,8 @@ xfs_agfl_write_verify(
>  		return;
>  
>  	if (!xfs_agfl_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  
> @@ -2236,18 +2234,17 @@ xfs_agf_read_verify(
>  	struct xfs_buf	*bp)
>  {
>  	struct xfs_mount *mp = bp->b_target->bt_mount;
> -	int		agf_ok = 1;
> -
> -	if (xfs_sb_version_hascrc(&mp->m_sb))
> -		agf_ok = xfs_buf_verify_cksum(bp, XFS_AGF_CRC_OFF);
>  
> -	agf_ok = agf_ok && xfs_agf_verify(mp, bp);
> -
> -	if (unlikely(XFS_TEST_ERROR(!agf_ok, mp, XFS_ERRTAG_ALLOC_READ_AGF,
> -			XFS_RANDOM_ALLOC_READ_AGF))) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> +	if (xfs_sb_version_hascrc(&mp->m_sb) &&
> +	    !xfs_buf_verify_cksum(bp, XFS_AGF_CRC_OFF))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (XFS_TEST_ERROR(!xfs_agf_verify(mp, bp), mp,
> +				XFS_ERRTAG_ALLOC_READ_AGF,
> +				XFS_RANDOM_ALLOC_READ_AGF))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> -	}
> +
> +	if (bp->b_error)
> +		xfs_verifier_error(bp);
>  }
>  
>  static void
> @@ -2258,8 +2255,8 @@ xfs_agf_write_verify(
>  	struct xfs_buf_log_item	*bip = bp->b_fspriv;
>  
>  	if (!xfs_agf_verify(mp, bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  
> diff --git a/fs/xfs/xfs_alloc_btree.c b/fs/xfs/xfs_alloc_btree.c
> index 144d3b0..cc1eadc 100644
> --- a/fs/xfs/xfs_alloc_btree.c
> +++ b/fs/xfs/xfs_alloc_btree.c
> @@ -355,12 +355,14 @@ static void
>  xfs_allocbt_read_verify(
>  	struct xfs_buf	*bp)
>  {
> -	if (!(xfs_btree_sblock_verify_crc(bp) &&
> -	      xfs_allocbt_verify(bp))) {
> -		trace_xfs_btree_corrupt(bp, _RET_IP_);
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -				     bp->b_target->bt_mount, bp->b_addr);
> +	if (!xfs_btree_sblock_verify_crc(bp))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (!xfs_allocbt_verify(bp))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +
> +	if (bp->b_error) {
> +		trace_xfs_btree_corrupt(bp, _RET_IP_);
> +		xfs_verifier_error(bp);
>  	}
>  }
>  
> @@ -370,9 +372,8 @@ xfs_allocbt_write_verify(
>  {
>  	if (!xfs_allocbt_verify(bp)) {
>  		trace_xfs_btree_corrupt(bp, _RET_IP_);
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -				     bp->b_target->bt_mount, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  	xfs_btree_sblock_calc_crc(bp);
> diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
> index b552378..fe9587f 100644
> --- a/fs/xfs/xfs_attr_leaf.c
> +++ b/fs/xfs/xfs_attr_leaf.c
> @@ -213,8 +213,8 @@ xfs_attr3_leaf_write_verify(
>  	struct xfs_attr3_leaf_hdr *hdr3 = bp->b_addr;
>  
>  	if (!xfs_attr3_leaf_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  
> @@ -239,12 +239,14 @@ xfs_attr3_leaf_read_verify(
>  {
>  	struct xfs_mount	*mp = bp->b_target->bt_mount;
>  
> -	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
> -	     !xfs_buf_verify_cksum(bp, XFS_ATTR3_LEAF_CRC_OFF)) ||
> -	    !xfs_attr3_leaf_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> +	if (xfs_sb_version_hascrc(&mp->m_sb) &&
> +	     !xfs_buf_verify_cksum(bp, XFS_ATTR3_LEAF_CRC_OFF))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (!xfs_attr3_leaf_verify(bp))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> -	}
> +
> +	if (bp->b_error)
> +		xfs_verifier_error(bp);
>  }
>  
>  const struct xfs_buf_ops xfs_attr3_leaf_buf_ops = {
> diff --git a/fs/xfs/xfs_attr_remote.c b/fs/xfs/xfs_attr_remote.c
> index 5549d69..6e37823 100644
> --- a/fs/xfs/xfs_attr_remote.c
> +++ b/fs/xfs/xfs_attr_remote.c
> @@ -125,7 +125,6 @@ xfs_attr3_rmt_read_verify(
>  	struct xfs_mount *mp = bp->b_target->bt_mount;
>  	char		*ptr;
>  	int		len;
> -	bool		corrupt = false;
>  	xfs_daddr_t	bno;
>  
>  	/* no verification of non-crc buffers */
> @@ -140,11 +139,11 @@ xfs_attr3_rmt_read_verify(
>  	while (len > 0) {
>  		if (!xfs_verify_cksum(ptr, XFS_LBSIZE(mp),
>  				      XFS_ATTR3_RMT_CRC_OFF)) {
> -			corrupt = true;
> +			xfs_buf_ioerror(bp, EFSBADCRC);
>  			break;
>  		}
>  		if (!xfs_attr3_rmt_verify(mp, ptr, XFS_LBSIZE(mp), bno)) {
> -			corrupt = true;
> +			xfs_buf_ioerror(bp, EFSCORRUPTED);
>  			break;
>  		}
>  		len -= XFS_LBSIZE(mp);
> @@ -152,10 +151,9 @@ xfs_attr3_rmt_read_verify(
>  		bno += mp->m_bsize;
>  	}
>  
> -	if (corrupt) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> -		xfs_buf_ioerror(bp, EFSCORRUPTED);
> -	} else
> +	if (bp->b_error)
> +		xfs_verifier_error(bp);
> +	else
>  		ASSERT(len == 0);
>  }
>  
> @@ -180,9 +178,8 @@ xfs_attr3_rmt_write_verify(
>  
>  	while (len > 0) {
>  		if (!xfs_attr3_rmt_verify(mp, ptr, XFS_LBSIZE(mp), bno)) {
> -			XFS_CORRUPTION_ERROR(__func__,
> -					    XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  			xfs_buf_ioerror(bp, EFSCORRUPTED);
> +			xfs_verifier_error(bp);
>  			return;
>  		}
>  		if (bip) {
> diff --git a/fs/xfs/xfs_bmap_btree.c b/fs/xfs/xfs_bmap_btree.c
> index 706bc3f..818d546 100644
> --- a/fs/xfs/xfs_bmap_btree.c
> +++ b/fs/xfs/xfs_bmap_btree.c
> @@ -780,12 +780,14 @@ static void
>  xfs_bmbt_read_verify(
>  	struct xfs_buf	*bp)
>  {
> -	if (!(xfs_btree_lblock_verify_crc(bp) &&
> -	      xfs_bmbt_verify(bp))) {
> -		trace_xfs_btree_corrupt(bp, _RET_IP_);
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -				     bp->b_target->bt_mount, bp->b_addr);
> +	if (!xfs_btree_lblock_verify_crc(bp))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (!xfs_bmbt_verify(bp))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +
> +	if (bp->b_error) {
> +		trace_xfs_btree_corrupt(bp, _RET_IP_);
> +		xfs_verifier_error(bp);
>  	}
>  }
>  
> @@ -794,11 +796,9 @@ xfs_bmbt_write_verify(
>  	struct xfs_buf	*bp)
>  {
>  	if (!xfs_bmbt_verify(bp)) {
> -		xfs_warn(bp->b_target->bt_mount, "bmbt daddr 0x%llx failed", bp->b_bn);
>  		trace_xfs_btree_corrupt(bp, _RET_IP_);
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -				     bp->b_target->bt_mount, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  	xfs_btree_lblock_calc_crc(bp);
> diff --git a/fs/xfs/xfs_da_btree.c b/fs/xfs/xfs_da_btree.c
> index 75ef990..1f5af79 100644
> --- a/fs/xfs/xfs_da_btree.c
> +++ b/fs/xfs/xfs_da_btree.c
> @@ -185,8 +185,8 @@ xfs_da3_node_write_verify(
>  	struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
>  
>  	if (!xfs_da3_node_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  
> @@ -209,17 +209,20 @@ static void
>  xfs_da3_node_read_verify(
>  	struct xfs_buf		*bp)
>  {
> -	struct xfs_mount	*mp = bp->b_target->bt_mount;
>  	struct xfs_da_blkinfo	*info = bp->b_addr;
>  
>  	switch (be16_to_cpu(info->magic)) {
>  		case XFS_DA3_NODE_MAGIC:
> -			if (!xfs_buf_verify_cksum(bp, XFS_DA3_NODE_CRC_OFF))
> +			if (!xfs_buf_verify_cksum(bp, XFS_DA3_NODE_CRC_OFF)) {
> +				xfs_buf_ioerror(bp, EFSBADCRC);
>  				break;
> +			}
>  			/* fall through */
>  		case XFS_DA_NODE_MAGIC:
> -			if (!xfs_da3_node_verify(bp))
> +			if (!xfs_da3_node_verify(bp)) {
> +				xfs_buf_ioerror(bp, EFSCORRUPTED);
>  				break;
> +			}
>  			return;
>  		case XFS_ATTR_LEAF_MAGIC:
>  		case XFS_ATTR3_LEAF_MAGIC:
> @@ -236,8 +239,7 @@ xfs_da3_node_read_verify(
>  	}
>  
>  	/* corrupt block */
> -	XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> -	xfs_buf_ioerror(bp, EFSCORRUPTED);
> +	xfs_verifier_error(bp);
>  }
>  
>  const struct xfs_buf_ops xfs_da3_node_buf_ops = {
> diff --git a/fs/xfs/xfs_dir2_block.c b/fs/xfs/xfs_dir2_block.c
> index 724377e..4f6a38c 100644
> --- a/fs/xfs/xfs_dir2_block.c
> +++ b/fs/xfs/xfs_dir2_block.c
> @@ -89,12 +89,14 @@ xfs_dir3_block_read_verify(
>  {
>  	struct xfs_mount	*mp = bp->b_target->bt_mount;
>  
> -	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
> -	     !xfs_buf_verify_cksum(bp, XFS_DIR3_DATA_CRC_OFF)) ||
> -	    !xfs_dir3_block_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> +	if (xfs_sb_version_hascrc(&mp->m_sb) &&
> +	     !xfs_buf_verify_cksum(bp, XFS_DIR3_DATA_CRC_OFF))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (!xfs_dir3_block_verify(bp))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> -	}
> +
> +	if (bp->b_error)
> +		xfs_verifier_error(bp);
>  }
>  
>  static void
> @@ -106,8 +108,8 @@ xfs_dir3_block_write_verify(
>  	struct xfs_dir3_blk_hdr	*hdr3 = bp->b_addr;
>  
>  	if (!xfs_dir3_block_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  
> diff --git a/fs/xfs/xfs_dir2_data.c b/fs/xfs/xfs_dir2_data.c
> index 74ae85e..afa4ad5 100644
> --- a/fs/xfs/xfs_dir2_data.c
> +++ b/fs/xfs/xfs_dir2_data.c
> @@ -241,7 +241,6 @@ static void
>  xfs_dir3_data_reada_verify(
>  	struct xfs_buf		*bp)
>  {
> -	struct xfs_mount	*mp = bp->b_target->bt_mount;
>  	struct xfs_dir2_data_hdr *hdr = bp->b_addr;
>  
>  	switch (hdr->magic) {
> @@ -255,8 +254,8 @@ xfs_dir3_data_reada_verify(
>  		xfs_dir3_data_verify(bp);
>  		return;
>  	default:
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, hdr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		break;
>  	}
>  }
> @@ -267,12 +266,14 @@ xfs_dir3_data_read_verify(
>  {
>  	struct xfs_mount	*mp = bp->b_target->bt_mount;
>  
> -	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
> -	     !xfs_buf_verify_cksum(bp, XFS_DIR3_DATA_CRC_OFF)) ||
> -	    !xfs_dir3_data_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> +	if (xfs_sb_version_hascrc(&mp->m_sb) &&
> +	     !xfs_buf_verify_cksum(bp, XFS_DIR3_DATA_CRC_OFF))
> +		 xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (!xfs_dir3_data_verify(bp))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> -	}
> +
> +	if (bp->b_error)
> +		xfs_verifier_error(bp);
>  }
>  
>  static void
> @@ -284,8 +285,8 @@ xfs_dir3_data_write_verify(
>  	struct xfs_dir3_blk_hdr	*hdr3 = bp->b_addr;
>  
>  	if (!xfs_dir3_data_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  
> diff --git a/fs/xfs/xfs_dir2_leaf.c b/fs/xfs/xfs_dir2_leaf.c
> index dffb61b..d36e97d 100644
> --- a/fs/xfs/xfs_dir2_leaf.c
> +++ b/fs/xfs/xfs_dir2_leaf.c
> @@ -179,12 +179,14 @@ __read_verify(
>  {
>  	struct xfs_mount	*mp = bp->b_target->bt_mount;
>  
> -	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
> -	     !xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF)) ||
> -	    !xfs_dir3_leaf_verify(bp, magic)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> +	if (xfs_sb_version_hascrc(&mp->m_sb) &&
> +	     !xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (!xfs_dir3_leaf_verify(bp, magic))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> -	}
> +
> +	if (bp->b_error)
> +		xfs_verifier_error(bp);
>  }
>  
>  static void
> @@ -197,8 +199,8 @@ __write_verify(
>  	struct xfs_dir3_leaf_hdr *hdr3 = bp->b_addr;
>  
>  	if (!xfs_dir3_leaf_verify(bp, magic)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  
> diff --git a/fs/xfs/xfs_dir2_node.c b/fs/xfs/xfs_dir2_node.c
> index 0904b20..cb434d7 100644
> --- a/fs/xfs/xfs_dir2_node.c
> +++ b/fs/xfs/xfs_dir2_node.c
> @@ -115,12 +115,14 @@ xfs_dir3_free_read_verify(
>  {
>  	struct xfs_mount	*mp = bp->b_target->bt_mount;
>  
> -	if ((xfs_sb_version_hascrc(&mp->m_sb) &&
> -	     !xfs_buf_verify_cksum(bp, XFS_DIR3_FREE_CRC_OFF)) ||
> -	    !xfs_dir3_free_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> +	if (xfs_sb_version_hascrc(&mp->m_sb) &&
> +	    !xfs_buf_verify_cksum(bp, XFS_DIR3_FREE_CRC_OFF))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (!xfs_dir3_free_verify(bp))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> -	}
> +
> +	if (bp->b_error)
> +		xfs_verifier_error(bp);
>  }
>  
>  static void
> @@ -132,8 +134,8 @@ xfs_dir3_free_write_verify(
>  	struct xfs_dir3_blk_hdr	*hdr3 = bp->b_addr;
>  
>  	if (!xfs_dir3_free_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  
> diff --git a/fs/xfs/xfs_dquot_buf.c b/fs/xfs/xfs_dquot_buf.c
> index d401457..610da81 100644
> --- a/fs/xfs/xfs_dquot_buf.c
> +++ b/fs/xfs/xfs_dquot_buf.c
> @@ -257,10 +257,13 @@ xfs_dquot_buf_read_verify(
>  {
>  	struct xfs_mount	*mp = bp->b_target->bt_mount;
>  
> -	if (!xfs_dquot_buf_verify_crc(mp, bp) || !xfs_dquot_buf_verify(mp, bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> +	if (!xfs_dquot_buf_verify_crc(mp, bp))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (!xfs_dquot_buf_verify(mp, bp))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> -	}
> +
> +	if (bp->b_error)
> +		xfs_verifier_error(bp);
>  }
>  
>  /*
> @@ -275,8 +278,8 @@ xfs_dquot_buf_write_verify(
>  	struct xfs_mount	*mp = bp->b_target->bt_mount;
>  
>  	if (!xfs_dquot_buf_verify(mp, bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  }
> diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
> index 4657586..5959b3b 100644
> --- a/fs/xfs/xfs_ialloc.c
> +++ b/fs/xfs/xfs_ialloc.c
> @@ -1568,18 +1568,17 @@ xfs_agi_read_verify(
>  	struct xfs_buf	*bp)
>  {
>  	struct xfs_mount *mp = bp->b_target->bt_mount;
> -	int		agi_ok = 1;
>  
> -	if (xfs_sb_version_hascrc(&mp->m_sb))
> -		agi_ok = xfs_buf_verify_cksum(bp, XFS_AGI_CRC_OFF);
> -
> -	agi_ok = agi_ok && xfs_agi_verify(bp);
> -
> -	if (unlikely(XFS_TEST_ERROR(!agi_ok, mp, XFS_ERRTAG_IALLOC_READ_AGI,
> -			XFS_RANDOM_IALLOC_READ_AGI))) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> +	if (xfs_sb_version_hascrc(&mp->m_sb) &&
> +	    !xfs_buf_verify_cksum(bp, XFS_AGI_CRC_OFF))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (XFS_TEST_ERROR(!xfs_agi_verify(bp), mp,
> +				XFS_ERRTAG_IALLOC_READ_AGI,
> +				XFS_RANDOM_IALLOC_READ_AGI))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> -	}
> +
> +	if (bp->b_error)
> +		xfs_verifier_error(bp);
>  }
>  
>  static void
> @@ -1590,8 +1589,8 @@ xfs_agi_write_verify(
>  	struct xfs_buf_log_item	*bip = bp->b_fspriv;
>  
>  	if (!xfs_agi_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  
> diff --git a/fs/xfs/xfs_ialloc_btree.c b/fs/xfs/xfs_ialloc_btree.c
> index 0028c50..7e309b1 100644
> --- a/fs/xfs/xfs_ialloc_btree.c
> +++ b/fs/xfs/xfs_ialloc_btree.c
> @@ -243,12 +243,14 @@ static void
>  xfs_inobt_read_verify(
>  	struct xfs_buf	*bp)
>  {
> -	if (!(xfs_btree_sblock_verify_crc(bp) &&
> -	      xfs_inobt_verify(bp))) {
> -		trace_xfs_btree_corrupt(bp, _RET_IP_);
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -				     bp->b_target->bt_mount, bp->b_addr);
> +	if (!xfs_btree_sblock_verify_crc(bp))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (!xfs_inobt_verify(bp))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +
> +	if (bp->b_error) {
> +		trace_xfs_btree_corrupt(bp, _RET_IP_);
> +		xfs_verifier_error(bp);
>  	}
>  }
>  
> @@ -258,9 +260,8 @@ xfs_inobt_write_verify(
>  {
>  	if (!xfs_inobt_verify(bp)) {
>  		trace_xfs_btree_corrupt(bp, _RET_IP_);
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -				     bp->b_target->bt_mount, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  	xfs_btree_sblock_calc_crc(bp);
> diff --git a/fs/xfs/xfs_inode_buf.c b/fs/xfs/xfs_inode_buf.c
> index 606b43a..24e9939 100644
> --- a/fs/xfs/xfs_inode_buf.c
> +++ b/fs/xfs/xfs_inode_buf.c
> @@ -102,8 +102,7 @@ xfs_inode_buf_verify(
>  			}
>  
>  			xfs_buf_ioerror(bp, EFSCORRUPTED);
> -			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_HIGH,
> -					     mp, dip);
> +			xfs_verifier_error(bp);
>  #ifdef DEBUG
>  			xfs_alert(mp,
>  				"bad inode magic/vsn daddr %lld #%d (magic=%x)",
> diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
> index 818359f..b134aa8 100644
> --- a/fs/xfs/xfs_sb.c
> +++ b/fs/xfs/xfs_sb.c
> @@ -614,7 +614,7 @@ xfs_sb_read_verify(
>  			/* Only fail bad secondaries on a known V5 filesystem */
>  			if (bp->b_bn == XFS_SB_DADDR ||
>  			    xfs_sb_version_hascrc(&mp->m_sb)) {
> -				error = EFSCORRUPTED;
> +				error = EFSBADCRC;
>  				goto out_error;
>  			}
>  		}
> @@ -623,10 +623,9 @@ xfs_sb_read_verify(
>  
>  out_error:
>  	if (error) {
> -		if (error == EFSCORRUPTED)
> -			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -					     mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, error);
> +		if (error == EFSCORRUPTED || error == EFSBADCRC)
> +			xfs_verifier_error(bp);
>  	}
>  }
>  
> @@ -661,9 +660,8 @@ xfs_sb_write_verify(
>  
>  	error = xfs_sb_verify(bp, false);
>  	if (error) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
> -				     mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, error);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  
> diff --git a/fs/xfs/xfs_symlink_remote.c b/fs/xfs/xfs_symlink_remote.c
> index defa09f..9b32052 100644
> --- a/fs/xfs/xfs_symlink_remote.c
> +++ b/fs/xfs/xfs_symlink_remote.c
> @@ -133,11 +133,13 @@ xfs_symlink_read_verify(
>  	if (!xfs_sb_version_hascrc(&mp->m_sb))
>  		return;
>  
> -	if (!xfs_buf_verify_cksum(bp, XFS_SYMLINK_CRC_OFF) ||
> -	    !xfs_symlink_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
> +	if (!xfs_buf_verify_cksum(bp, XFS_SYMLINK_CRC_OFF))
> +		xfs_buf_ioerror(bp, EFSBADCRC);
> +	else if (!xfs_symlink_verify(bp))
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> -	}
> +
> +	if (bp->b_error)
> +		xfs_verifier_error(bp);
>  }
>  
>  static void
> @@ -152,8 +154,8 @@ xfs_symlink_write_verify(
>  		return;
>  
>  	if (!xfs_symlink_verify(bp)) {
> -		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
>  		xfs_buf_ioerror(bp, EFSCORRUPTED);
> +		xfs_verifier_error(bp);
>  		return;
>  	}
>  
> 
> 
> _______________________________________________
> 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 0/9] current series for verifier error differentiation

[Dave Chinner]

On Tue, Feb 18, 2014 at 05:52:20PM -0600, Eric Sandeen wrote:
> This is the current series I have leading up to verifier error differentiation,
> just resending them all.
> 
> As I send this, I remember that EILSEQ is giving us an unusual perror()
> output, so the last patch which uses it may need to pick some (?)
> other error code...

Eric, this leaks EBADMSG to userspace in xfs/005 with a corrupted
superblock:

$ diff -u tests/xfs/005.out /home/dave/src/xfstests-dev/results//xfs/005.out.bad
--- tests/xfs/005.out   2014-02-24 09:58:09.000000000 +1100
+++ /home/dave/src/xfstests-dev/results//xfs/005.out.bad
2014-02-27 19:40:56.000000000 +1100
@@ -1,4 +1,4 @@
 QA output created by 005
 wrote 4/4 bytes at offset 224
 XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-mount: Structure needs cleaning
+mount: Bad message

Dmesg contains this:

[ 8790.946956] XFS (vdb): Metadata CRC error detected at xfs_sb_read_verify+0x10c/0x140, block 0x0
[ 8790.949984] XFS (vdb): Unmount and run xfs_repair
[ 8790.951725] XFS (vdb): First 64 bytes of corrupted metadata buffer:
[ 8790.953883] ffff880003eea000: 58 46 53 42 00 00 10 00 00 00 00 00 00 2c 00 00  XFSB.........,..
[ 8790.956452] ffff880003eea010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[ 8790.958518] ffff880003eea020: 0b 21 13 2a 5b 00 40 59 bd 7c 2b d4 5b 6c a4 0a  .!.*[.@Y.|+.[l..
[ 8790.960487] ffff880003eea030: 00 00 00 00 00 20 00 04 00 00 00 00 00 00 00 40  ..... .........@

So that part is good. However, I suspect we should still be
returning EFSCORRUPTED to userspace. Most metadata buffer reads go
through xfs_trans_read_buf_map(), so there only a couple of places
we need to catch and convert the error being returned. The other two
I notice that are need conversion are xfs_buf_read() in
xfs_readlink_bmap(), and the cause of this regression which is
xfs_buf_read_uncached() in xfs_readsb()....

Can you look into writing up a followup patch that does this
catch-and-convert? It will also mean all the higher layers will
treat a CRC error as corrupt metadata, which is exactly what we want
it to do. :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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