public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] xfs: verifier modification series
@ 2014-02-10  2:15 Eric Sandeen
  2014-02-10  2:20 ` [PATCH 1/6] xfs: limit superblock corruption errors to actual corruption Eric Sandeen
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Eric Sandeen @ 2014-02-10  2:15 UTC (permalink / raw)
  To: xfs-oss

This is a handful of changes related to the verifiers.  The end goal is to differentiate CRC errors from other non-CRC verifier errors.

At this point it's somewhat RFC, if you don't like some of the changes made along the way, just speak up.

Thanks,
-Eric

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 1/6] xfs: limit superblock corruption errors to actual corruption
  2014-02-10  2:15 [PATCH 0/6] xfs: verifier modification series Eric Sandeen
@ 2014-02-10  2:20 ` Eric Sandeen
  2014-02-10  2:24 ` [PATCH 2/6] xfs: skip pointless CRC updates after verifier failures Eric Sandeen
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Eric Sandeen @ 2014-02-10  2:20 UTC (permalink / raw)
  To: Eric Sandeen, xfs-oss

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>
---

Thanks to Brian for pointing out this better solution to my
earlier patch.

 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 b7c9aea..efaef71 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 2/6] xfs: skip pointless CRC updates after verifier failures
  2014-02-10  2:15 [PATCH 0/6] xfs: verifier modification series Eric Sandeen
  2014-02-10  2:20 ` [PATCH 1/6] xfs: limit superblock corruption errors to actual corruption Eric Sandeen
@ 2014-02-10  2:24 ` Eric Sandeen
  2014-02-10  2:27 ` [PATCH 3/6] xfs: add helper for verifying checksums on xfs_bufs Eric Sandeen
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Eric Sandeen @ 2014-02-10  2:24 UTC (permalink / raw)
  To: Eric Sandeen, xfs-oss

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 3/6] xfs: add helper for verifying checksums on xfs_bufs
  2014-02-10  2:15 [PATCH 0/6] xfs: verifier modification series Eric Sandeen
  2014-02-10  2:20 ` [PATCH 1/6] xfs: limit superblock corruption errors to actual corruption Eric Sandeen
  2014-02-10  2:24 ` [PATCH 2/6] xfs: skip pointless CRC updates after verifier failures Eric Sandeen
@ 2014-02-10  2:27 ` Eric Sandeen
  2014-02-10  3:31   ` Dave Chinner
  2014-02-10  2:29 ` [PATCH 4/6] " Eric Sandeen
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Eric Sandeen @ 2014-02-10  2:27 UTC (permalink / raw)
  To: Eric Sandeen, xfs-oss

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>
---

I'm not wedded to this; seems helpful and cleaner, but
if there's a reason not to, *shrug*

 fs/xfs/xfs_alloc.c          |    7 +++----
 fs/xfs/xfs_attr_leaf.c      |    3 +--
 fs/xfs/xfs_btree.c          |    8 ++++----
 fs/xfs/xfs_cksum.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_symlink_remote.c |    2 +-
 11 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
index 9eab2df..ab33714 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),
-				   offsetof(struct xfs_agfl, agfl_crc));
+	agfl_ok = xfs_buf_verify_cksum(bp, offsetof(struct xfs_agfl, agfl_crc));
 
 	agfl_ok = agfl_ok && xfs_agfl_verify(bp);
 
@@ -2241,8 +2240,8 @@ 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),
-					  offsetof(struct xfs_agf, agf_crc));
+		agf_ok = xfs_buf_verify_cksum(bp,
+					      offsetof(struct xfs_agf, agf_crc));
 
 	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_cksum.h b/fs/xfs/xfs_cksum.h
index fad1676..f605d64 100644
--- a/fs/xfs/xfs_cksum.h
+++ b/fs/xfs/xfs_cksum.h
@@ -60,4 +60,11 @@ xfs_verify_cksum(char *buffer, size_t length, unsigned long cksum_offset)
 	return *(__le32 *)(buffer + cksum_offset) == xfs_end_cksum(crc);
 }
 
+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);
+}
+
 #endif /* _XFS_CKSUM_H */
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 5d7f105..7321e49 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),
-					  offsetof(struct xfs_agi, agi_crc));
+		agi_ok = xfs_buf_verify_cksum(bp,
+					offsetof(struct xfs_agi, agi_crc));
 	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_symlink_remote.c b/fs/xfs/xfs_symlink_remote.c
index bf59a2b..3f09957 100644
--- a/fs/xfs/xfs_symlink_remote.c
+++ b/fs/xfs/xfs_symlink_remote.c
@@ -133,7 +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),
+	if (!xfs_buf_verify_cksum(bp,
 				  offsetof(struct xfs_dsymlink_hdr, sl_crc)) ||
 	    !xfs_symlink_verify(bp)) {
 		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
-- 1.7.1

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 4/6] xfs: add helper for verifying checksums on xfs_bufs
  2014-02-10  2:15 [PATCH 0/6] xfs: verifier modification series Eric Sandeen
                   ` (2 preceding siblings ...)
  2014-02-10  2:27 ` [PATCH 3/6] xfs: add helper for verifying checksums on xfs_bufs Eric Sandeen
@ 2014-02-10  2:29 ` Eric Sandeen
  2014-02-10  3:33   ` Dave Chinner
  2014-02-10  2:33 ` [PATCH 5/6] xfs: add xfs_verifier_error() Eric Sandeen
  2014-02-10  2:37 ` [PATCH 6/6] xfs: modify verifiers to differentiate CRC from other errors Eric Sandeen
  5 siblings, 1 reply; 14+ messages in thread
From: Eric Sandeen @ 2014-02-10  2:29 UTC (permalink / raw)
  To: Eric Sandeen, xfs-oss

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          |    6 ++----
 fs/xfs/xfs_attr_leaf.c      |    2 +-
 fs/xfs/xfs_btree.c          |    6 ++----
 fs/xfs/xfs_cksum.h          |    6 ++++++
 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         |    4 ++--
 fs/xfs/xfs_sb.c             |    3 +--
 fs/xfs/xfs_symlink_remote.c |    3 +--
 12 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
index ab33714..72b9451 100644
--- a/fs/xfs/xfs_alloc.c
+++ b/fs/xfs/xfs_alloc.c
@@ -515,8 +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),
-			 offsetof(struct xfs_agfl, agfl_crc));
+	xfs_buf_update_cksum(bp, offsetof(struct xfs_agfl, agfl_crc));
 }
 
 const struct xfs_buf_ops xfs_agfl_buf_ops = {
@@ -2271,8 +2270,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_buf_update_cksum(bp, offsetof(struct xfs_agf, agf_crc));
 }
 
 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_cksum.h b/fs/xfs/xfs_cksum.h
index f605d64..7886d05 100644
--- a/fs/xfs/xfs_cksum.h
+++ b/fs/xfs/xfs_cksum.h
@@ -49,6 +49,12 @@ xfs_update_cksum(char *buffer, size_t length, unsigned long cksum_offset)
 	*(__le32 *)(buffer + cksum_offset) = xfs_end_cksum(crc);
 }
 
+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);
+}
 /*
  * Helper to verify the checksum for a buffer.
  */
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 7321e49..0f03405 100644
--- a/fs/xfs/xfs_ialloc.c
+++ b/fs/xfs/xfs_ialloc.c
@@ -1600,8 +1600,8 @@ 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_buf_update_cksum(bp, offsetof(struct xfs_agi, agi_crc));
 }
 
 const struct xfs_buf_ops xfs_agi_buf_ops = {
diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
index efaef71..7bbaf20 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -675,8 +675,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_buf_update_cksum(bp, offsetof(struct xfs_sb, sb_crc));
 }
 
 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 3f09957..7d53c5d 100644
--- a/fs/xfs/xfs_symlink_remote.c
+++ b/fs/xfs/xfs_symlink_remote.c
@@ -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_buf_update_cksum(bp, offsetof(struct xfs_dsymlink_hdr, sl_crc));
 }
 
 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 5/6] xfs: add xfs_verifier_error()
  2014-02-10  2:15 [PATCH 0/6] xfs: verifier modification series Eric Sandeen
                   ` (3 preceding siblings ...)
  2014-02-10  2:29 ` [PATCH 4/6] " Eric Sandeen
@ 2014-02-10  2:33 ` Eric Sandeen
  2014-02-10  3:43   ` Dave Chinner
  2014-02-10  2:37 ` [PATCH 6/6] xfs: modify verifiers to differentiate CRC from other errors Eric Sandeen
  5 siblings, 1 reply; 14+ messages in thread
From: Eric Sandeen @ 2014-02-10  2:33 UTC (permalink / raw)
  To: Eric Sandeen, xfs-oss

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 |   22 ++++++++++++++++++++++
 fs/xfs/xfs_error.h |    3 +++
 fs/xfs/xfs_linux.h |    1 +
 3 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index 9995b80..08d76f4 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -178,3 +178,25 @@ 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(
+	const char		*func,
+	struct xfs_buf		*bp)
+{
+	struct xfs_mount *mp = bp->b_target->bt_mount;
+
+	xfs_alert(mp,
+"%sCorruption detected in %s, block 0x%llx. Unmount and run xfs_repair",
+		  bp->b_error == EFSBADCRC ? "CRC " : "", func, bp->b_bn);
+
+        if (xfs_error_level >= XFS_ERRLEVEL_LOW)
+                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..f4315c2 100644
--- a/fs/xfs/xfs_error.h
+++ b/fs/xfs/xfs_error.h
@@ -34,12 +34,15 @@ 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(const char *func, struct xfs_buf *bp);
 
 #define	XFS_ERROR_REPORT(e, lvl, mp)	\
 	xfs_error_report(e, lvl, mp, __FILE__, __LINE__, __return_address)
 #define	XFS_CORRUPTION_ERROR(e, lvl, mp, mem)	\
 	xfs_corruption_error(e, lvl, mp, mem, \
 			     __FILE__, __LINE__, __return_address)
+#define	xfs_verifier_error(bp)	\
+	__xfs_verifier_error(__func__, bp)
 
 #define XFS_ERRLEVEL_OFF	0
 #define XFS_ERRLEVEL_LOW	1
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index f9bb590..283b98b 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -178,6 +178,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

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 6/6] xfs: modify verifiers to differentiate CRC from other errors
  2014-02-10  2:15 [PATCH 0/6] xfs: verifier modification series Eric Sandeen
                   ` (4 preceding siblings ...)
  2014-02-10  2:33 ` [PATCH 5/6] xfs: add xfs_verifier_error() Eric Sandeen
@ 2014-02-10  2:37 ` Eric Sandeen
  5 siblings, 0 replies; 14+ messages in thread
From: Eric Sandeen @ 2014-02-10  2:37 UTC (permalink / raw)
  To: Eric Sandeen, xfs-oss

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>
---

note: I thought about moving the xfs_buf_error() set
into the crc checkers or the basic verifiers, but then
it's not obvious to the reader who has set the errno.
So while this is more cut & paste it seems more readable.

I'm flexible though...

This is a big-ish patch and I've run it through xfstests but
it could use careful review - I'm still looking over it but
wanted to get this on the list to make sure it's headed in
the right direction.

 fs/xfs/xfs_alloc.c          |   38 +++++++++++++++++---------------------
 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         |   13 +++++++++----
 fs/xfs/xfs_ialloc_btree.c   |   15 ++++++++-------
 fs/xfs/xfs_inode_buf.c      |    3 +--
 fs/xfs/xfs_sb.c             |   11 ++++-------
 fs/xfs/xfs_symlink_remote.c |   12 +++++++-----
 16 files changed, 124 insertions(+), 112 deletions(-)

diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
index 72b9451..90e6997 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, offsetof(struct xfs_agfl, agfl_crc));
-
-	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,19 +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,
-					      offsetof(struct xfs_agf, agf_crc));
 
-	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
@@ -2259,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 0f03405..7d640bb 100644
--- a/fs/xfs/xfs_ialloc.c
+++ b/fs/xfs/xfs_ialloc.c
@@ -1573,13 +1573,18 @@ xfs_agi_read_verify(
 	if (xfs_sb_version_hascrc(&mp->m_sb))
 		agi_ok = xfs_buf_verify_cksum(bp,
 					offsetof(struct xfs_agi, agi_crc));
+
+	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 +1595,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 4fc9f39..b984419 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 7bbaf20..b838287 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -615,7 +615,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;
 			}
 		}
@@ -624,10 +624,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);
 	}
 }
 
@@ -643,7 +642,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);
@@ -663,9 +661,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 7d53c5d..8193665 100644
--- a/fs/xfs/xfs_symlink_remote.c
+++ b/fs/xfs/xfs_symlink_remote.c
@@ -134,11 +134,13 @@ xfs_symlink_read_verify(
 		return;
 
 	if (!xfs_buf_verify_cksum(bp,
-				  offsetof(struct xfs_dsymlink_hdr, sl_crc)) ||
-	    !xfs_symlink_verify(bp)) {
-		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, bp->b_addr);
+				  offsetof(struct xfs_dsymlink_hdr, sl_crc)))
+		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
@@ -153,8 +155,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

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* Re: [PATCH 3/6] xfs: add helper for verifying checksums on xfs_bufs
  2014-02-10  2:27 ` [PATCH 3/6] xfs: add helper for verifying checksums on xfs_bufs Eric Sandeen
@ 2014-02-10  3:31   ` Dave Chinner
  0 siblings, 0 replies; 14+ messages in thread
From: Dave Chinner @ 2014-02-10  3:31 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Eric Sandeen, xfs-oss

On Sun, Feb 09, 2014 at 08:27:19PM -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>
> ---
> 
> I'm not wedded to this; seems helpful and cleaner, but
> if there's a reason not to, *shrug*
> 
>  fs/xfs/xfs_alloc.c          |    7 +++----
>  fs/xfs/xfs_attr_leaf.c      |    3 +--
>  fs/xfs/xfs_btree.c          |    8 ++++----
>  fs/xfs/xfs_cksum.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_symlink_remote.c |    2 +-
>  11 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
> index 9eab2df..ab33714 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),
> -				   offsetof(struct xfs_agfl, agfl_crc));
> +	agfl_ok = xfs_buf_verify_cksum(bp, offsetof(struct xfs_agfl, agfl_crc));
					   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
vs

> +          !xfs_buf_verify_cksum(bp, XFS_ATTR3_LEAF_CRC_OFF)) ||
> 				       ^^^^^^^^^^^^^^^^^^^^^^

I think that we also be consistent with the way we specify
the offset of the CRC field. Some of the code uses a #define, and
some doesn't. My preference would be to use the #define format
everywhere as it makes the code briefer and easier to read and it
gets rid of the long lines and wrapping that is done in places.

> diff --git a/fs/xfs/xfs_cksum.h b/fs/xfs/xfs_cksum.h
> index fad1676..f605d64 100644
> --- a/fs/xfs/xfs_cksum.h
> +++ b/fs/xfs/xfs_cksum.h
> @@ -60,4 +60,11 @@ xfs_verify_cksum(char *buffer, size_t length, unsigned long cksum_offset)
>  	return *(__le32 *)(buffer + cksum_offset) == xfs_end_cksum(crc);
>  }
>  
> +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);
> +}
> +

This introduces a dependency between xfs_buf.h and xfs_cksum.h.
i.e. if we include xfs_cksum.h we now have to include xfs_buf.h
before it.

Even though it means we'll have to juggle header files in quite a
few files, I'd prefer that we swap the dependency order as
xfs_cksum.h is shared with userspace and used in places that don't
necessarily know (or should know) what an xfs_buf is....

Otherwise it's a good cleanup.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 4/6] xfs: add helper for verifying checksums on xfs_bufs
  2014-02-10  2:29 ` [PATCH 4/6] " Eric Sandeen
@ 2014-02-10  3:33   ` Dave Chinner
  2014-02-10  3:35     ` Eric Sandeen
  0 siblings, 1 reply; 14+ messages in thread
From: Dave Chinner @ 2014-02-10  3:33 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Eric Sandeen, xfs-oss

On Sun, Feb 09, 2014 at 08:29:47PM -0600, Eric Sandeen wrote:
> 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.

The title of the patch is the same as the previous one - I think you
forgot to "update" it?

Other than that, same comments as for the previous patch.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 4/6] xfs: add helper for verifying checksums on xfs_bufs
  2014-02-10  3:33   ` Dave Chinner
@ 2014-02-10  3:35     ` Eric Sandeen
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Sandeen @ 2014-02-10  3:35 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Eric Sandeen, xfs-oss

On 2/9/14, 9:33 PM, Dave Chinner wrote:
> On Sun, Feb 09, 2014 at 08:29:47PM -0600, Eric Sandeen wrote:
>> 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.
> 
> The title of the patch is the same as the previous one - I think you
> forgot to "update" it?

Bah, yes.  Probably doesn't really even have to be 2 patches.

> Other than that, same comments as for the previous patch.

ok, thx for the reviews.  Agree that the defines make it better.

-Eric

> Cheers,
> 
> Dave.
> 

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 5/6] xfs: add xfs_verifier_error()
  2014-02-10  2:33 ` [PATCH 5/6] xfs: add xfs_verifier_error() Eric Sandeen
@ 2014-02-10  3:43   ` Dave Chinner
  2014-02-10  4:16     ` Eric Sandeen
  0 siblings, 1 reply; 14+ messages in thread
From: Dave Chinner @ 2014-02-10  3:43 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Eric Sandeen, xfs-oss

On Sun, Feb 09, 2014 at 08:33:49PM -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)
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/xfs_error.c |   22 ++++++++++++++++++++++
>  fs/xfs/xfs_error.h |    3 +++
>  fs/xfs/xfs_linux.h |    1 +
>  3 files changed, 26 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> index 9995b80..08d76f4 100644
> --- a/fs/xfs/xfs_error.c
> +++ b/fs/xfs/xfs_error.c
> @@ -178,3 +178,25 @@ 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(
> +	const char		*func,
> +	struct xfs_buf		*bp)
> +{
> +	struct xfs_mount *mp = bp->b_target->bt_mount;
> +
> +	xfs_alert(mp,
> +"%sCorruption detected in %s, block 0x%llx. Unmount and run xfs_repair",
> +		  bp->b_error == EFSBADCRC ? "CRC " : "", func, bp->b_bn);

Perhaps if we do this:

	xfs_alert(mp,
"Metadata %s detected at %pF, block 0x%llx. Unmount and run xfs_repair",
		  bp->b_error == EFSBADCRC ? "CRC error"
					   : "corruption", _RET_IP_, bp->b_bn);

We'll get a symbol of the form caller_name+0xoffset similar to a
stack dump. That way if we have multiple calls to a
xfs_verifier_error() inside a single function we get something that
tells us which call detected the error...

Also, the use of _RET_IP_ gets rid of the need for the wrapper
macro....

i.e. we could replace all the XFS_WANT_CORRUPTED_RETURN() calls in
__xfs_dir3_data_check() with calls to xfs_verifier_error() so we can
determine exactly what corruption check failed...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 5/6] xfs: add xfs_verifier_error()
  2014-02-10  3:43   ` Dave Chinner
@ 2014-02-10  4:16     ` Eric Sandeen
  2014-02-10 11:10       ` Dave Chinner
  0 siblings, 1 reply; 14+ messages in thread
From: Eric Sandeen @ 2014-02-10  4:16 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Eric Sandeen, xfs-oss

On 2/9/14, 9:43 PM, Dave Chinner wrote:
> On Sun, Feb 09, 2014 at 08:33:49PM -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)
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  fs/xfs/xfs_error.c |   22 ++++++++++++++++++++++
>>  fs/xfs/xfs_error.h |    3 +++
>>  fs/xfs/xfs_linux.h |    1 +
>>  3 files changed, 26 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
>> index 9995b80..08d76f4 100644
>> --- a/fs/xfs/xfs_error.c
>> +++ b/fs/xfs/xfs_error.c
>> @@ -178,3 +178,25 @@ 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(
>> +	const char		*func,
>> +	struct xfs_buf		*bp)
>> +{
>> +	struct xfs_mount *mp = bp->b_target->bt_mount;
>> +
>> +	xfs_alert(mp,
>> +"%sCorruption detected in %s, block 0x%llx. Unmount and run xfs_repair",
>> +		  bp->b_error == EFSBADCRC ? "CRC " : "", func, bp->b_bn);
> 
> Perhaps if we do this:
> 
> 	xfs_alert(mp,
> "Metadata %s detected at %pF, block 0x%llx. Unmount and run xfs_repair",
> 		  bp->b_error == EFSBADCRC ? "CRC error"
> 					   : "corruption", _RET_IP_, bp->b_bn);
> 
> We'll get a symbol of the form caller_name+0xoffset similar to a
> stack dump. That way if we have multiple calls to a
> xfs_verifier_error() inside a single function we get something that
> tells us which call detected the error...

Hm, but the point of the switch based on error nrs was to require only
one call in each ->verifier, and ...

> Also, the use of _RET_IP_ gets rid of the need for the wrapper
> macro....

0x${SPLAT} is a lot less useful than i.e. "xfs_agi_read_verify"

Printing the _RET_IP_ requires disassembly of that particular build
to figure out where we went wrong, whereas printing __func__ makes it
clear on the initial read of the dmesg.

> i.e. we could replace all the XFS_WANT_CORRUPTED_RETURN() calls in
> __xfs_dir3_data_check() with calls to xfs_verifier_error() so we can
> determine exactly what corruption check failed...

Well, I'm sympathetic to that goal, but I wonder if we can't do both;
print in plain english which verifier went bad, and also (when warranted)
print lower level details in some other manner...?

-Eric

> Cheers,
> 
> Dave.
> 

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 5/6] xfs: add xfs_verifier_error()
  2014-02-10  4:16     ` Eric Sandeen
@ 2014-02-10 11:10       ` Dave Chinner
  2014-02-10 14:52         ` Eric Sandeen
  0 siblings, 1 reply; 14+ messages in thread
From: Dave Chinner @ 2014-02-10 11:10 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Eric Sandeen, xfs-oss

On Sun, Feb 09, 2014 at 10:16:20PM -0600, Eric Sandeen wrote:
> On 2/9/14, 9:43 PM, Dave Chinner wrote:
> > On Sun, Feb 09, 2014 at 08:33:49PM -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)
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>  fs/xfs/xfs_error.c |   22 ++++++++++++++++++++++
> >>  fs/xfs/xfs_error.h |    3 +++
> >>  fs/xfs/xfs_linux.h |    1 +
> >>  3 files changed, 26 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> >> index 9995b80..08d76f4 100644
> >> --- a/fs/xfs/xfs_error.c
> >> +++ b/fs/xfs/xfs_error.c
> >> @@ -178,3 +178,25 @@ 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(
> >> +	const char		*func,
> >> +	struct xfs_buf		*bp)
> >> +{
> >> +	struct xfs_mount *mp = bp->b_target->bt_mount;
> >> +
> >> +	xfs_alert(mp,
> >> +"%sCorruption detected in %s, block 0x%llx. Unmount and run xfs_repair",
> >> +		  bp->b_error == EFSBADCRC ? "CRC " : "", func, bp->b_bn);
> > 
> > Perhaps if we do this:
> > 
> > 	xfs_alert(mp,
> > "Metadata %s detected at %pF, block 0x%llx. Unmount and run xfs_repair",
> > 		  bp->b_error == EFSBADCRC ? "CRC error"
> > 					   : "corruption", _RET_IP_, bp->b_bn);
> > 
> > We'll get a symbol of the form caller_name+0xoffset similar to a
> > stack dump. That way if we have multiple calls to a
> > xfs_verifier_error() inside a single function we get something that
> > tells us which call detected the error...
> 
> Hm, but the point of the switch based on error nrs was to require only
> one call in each ->verifier, and ...

Right, that's the current usage of it because we are simply
returning true/false from the checking code. Determining the exact
error is the report is much more useful - let's not lose sight of
the end goal....

> > Also, the use of _RET_IP_ gets rid of the need for the wrapper
> > macro....
> 
> 0x${SPLAT} is a lot less useful than i.e. "xfs_agi_read_verify"

Note the format string I used: "%pF". That decodes the _RET_IP_
into the function name and offset from the start of the function.
i.e. it returns xfs_agi_read_verify+0x<splat>.

> Printing the _RET_IP_ requires disassembly of that particular build
> to figure out where we went wrong, whereas printing __func__ makes it
> clear on the initial read of the dmesg.

The function is already there in plain test. You only need to go to
tools if you want ot know the exact line it came from....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 5/6] xfs: add xfs_verifier_error()
  2014-02-10 11:10       ` Dave Chinner
@ 2014-02-10 14:52         ` Eric Sandeen
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Sandeen @ 2014-02-10 14:52 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Eric Sandeen, xfs-oss

On 2/10/14, 5:10 AM, Dave Chinner wrote:
> On Sun, Feb 09, 2014 at 10:16:20PM -0600, Eric Sandeen wrote:
>> On 2/9/14, 9:43 PM, Dave Chinner wrote:
>>> On Sun, Feb 09, 2014 at 08:33:49PM -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)
>>>>
>>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>>> ---
>>>>  fs/xfs/xfs_error.c |   22 ++++++++++++++++++++++
>>>>  fs/xfs/xfs_error.h |    3 +++
>>>>  fs/xfs/xfs_linux.h |    1 +
>>>>  3 files changed, 26 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
>>>> index 9995b80..08d76f4 100644
>>>> --- a/fs/xfs/xfs_error.c
>>>> +++ b/fs/xfs/xfs_error.c
>>>> @@ -178,3 +178,25 @@ 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(
>>>> +	const char		*func,
>>>> +	struct xfs_buf		*bp)
>>>> +{
>>>> +	struct xfs_mount *mp = bp->b_target->bt_mount;
>>>> +
>>>> +	xfs_alert(mp,
>>>> +"%sCorruption detected in %s, block 0x%llx. Unmount and run xfs_repair",
>>>> +		  bp->b_error == EFSBADCRC ? "CRC " : "", func, bp->b_bn);
>>>
>>> Perhaps if we do this:
>>>
>>> 	xfs_alert(mp,
>>> "Metadata %s detected at %pF, block 0x%llx. Unmount and run xfs_repair",
>>> 		  bp->b_error == EFSBADCRC ? "CRC error"
>>> 					   : "corruption", _RET_IP_, bp->b_bn);
>>>
>>> We'll get a symbol of the form caller_name+0xoffset similar to a
>>> stack dump. That way if we have multiple calls to a
>>> xfs_verifier_error() inside a single function we get something that
>>> tells us which call detected the error...
>>
>> Hm, but the point of the switch based on error nrs was to require only
>> one call in each ->verifier, and ...
> 
> Right, that's the current usage of it because we are simply
> returning true/false from the checking code. Determining the exact
> error is the report is much more useful - let's not lose sight of
> the end goal....
> 
>>> Also, the use of _RET_IP_ gets rid of the need for the wrapper
>>> macro....
>>
>> 0x${SPLAT} is a lot less useful than i.e. "xfs_agi_read_verify"
> 
> Note the format string I used: "%pF". That decodes the _RET_IP_
> into the function name and offset from the start of the function.
> i.e. it returns xfs_agi_read_verify+0x<splat>.

I forgot that it did this, TBH.  Ok, I'll rethink things a bit.

(although with multiple failure points in a verifier, +0x4a vs +0x5b
will still require some digging; a line number might be nice, but
then we'd need a wrapper again)

Thanks,
-Eric

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-02-10 14:52 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-10  2:15 [PATCH 0/6] xfs: verifier modification series Eric Sandeen
2014-02-10  2:20 ` [PATCH 1/6] xfs: limit superblock corruption errors to actual corruption Eric Sandeen
2014-02-10  2:24 ` [PATCH 2/6] xfs: skip pointless CRC updates after verifier failures Eric Sandeen
2014-02-10  2:27 ` [PATCH 3/6] xfs: add helper for verifying checksums on xfs_bufs Eric Sandeen
2014-02-10  3:31   ` Dave Chinner
2014-02-10  2:29 ` [PATCH 4/6] " Eric Sandeen
2014-02-10  3:33   ` Dave Chinner
2014-02-10  3:35     ` Eric Sandeen
2014-02-10  2:33 ` [PATCH 5/6] xfs: add xfs_verifier_error() Eric Sandeen
2014-02-10  3:43   ` Dave Chinner
2014-02-10  4:16     ` Eric Sandeen
2014-02-10 11:10       ` Dave Chinner
2014-02-10 14:52         ` Eric Sandeen
2014-02-10  2:37 ` [PATCH 6/6] xfs: modify verifiers to differentiate CRC from other errors Eric Sandeen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox