xfs: limit superblock corruption errors to probable corruption

PATCH In review Eric Sandeen 4 participants 6 messages Jan 29, 2014 – Feb 7, 2014 XFS filesystem

lore

Share & tools

lore

Eric Sandeen

author

2014-01-29 05:11 UTC #0 lore raw

PATCH 1/1 xfs: limit superblock corruption errors to probable corruption

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 high-level XFS_CORRUPTION_ERROR which was firing for every error return except EWRONGFS.

It also adds one to the path which detects a failed checksum.

The idea is, if it's really _corruption_ we can call XFS_CORRUPTION_ERROR at the point of detection. More benign incompatibilities can do a little printk & fail the mount without so much drama.

Signed-off-by Eric Sandeen <[email protected]>

---

I could see an argument where we might still want the hexdump for things like bad magic - ok, just what *was* the magic? But I think we do need to reserve the oops-mimicing-backtraces for the most severe problems. Discuss. ;)

fs/xfs/xfs_sb.c +3 -5

index 511cce9..b575317 100644
@@ -617,6 +617,8 @@ 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)) {
+				XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
+						     mp, bp->b_addr);
 				error = EFSCORRUPTED;
 				goto out_error;
 			}

@@ -625,12 +627,8 @@ xfs_sb_read_verify(
 	error = xfs_sb_verify(bp, true);
 
 out_error:
-	if (error) {
-		if (error != EWRONGFS)
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-					     mp, bp->b_addr);
+	if (error)
 		xfs_buf_ioerror(bp, error);
-	}
 }
 
 /*

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Brian Foster

2014-01-30 20:26 UTC #1 lore raw

Quoted 32 lines · 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 high-level XFS_CORRUPTION_ERROR which was firing for every error return except EWRONGFS.
It also adds one to the path which detects a failed checksum.
The idea is, if it's really _corruption_ we can call XFS_CORRUPTION_ERROR at the point of detection. More benign incompatibilities can do a little printk & fail the mount without so much drama.

Quoted 8 lines

Signed-off-by Eric Sandeen <[email protected]>
---
I could see an argument where we might still want the hexdump for things like bad magic - ok, just what *was* the magic? But I think we do need to reserve the oops-mimicing-backtraces for the most severe problems. Discuss. ;)

This seems pretty reasonable to me, particularly if pretty much any error via the xfs_sb_verify() path dumps corruption noise...

Quoted 15 lines

fs/xfs/xfs_sb.c +2 -0

index 511cce9..b575317 100644
@@ -617,6 +617,8 @@ 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)) {
+				XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
+						     mp, bp->b_addr);
 				error = EFSCORRUPTED;
 				goto out_error;
 			}

@@ -625,12 +627,8 @@ xfs_sb_read_verify(
 	error = xfs_sb_verify(bp, true);

fs/xfs/xfs_sb.c xfs_sb_read_verify( · line 632

Quoted 10 lines

fs/xfs/xfs_sb.c +1 -5

 
 out_error:
-	if (error) {
-		if (error != EWRONGFS)
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-					     mp, bp->b_addr);
+	if (error)
 		xfs_buf_ioerror(bp, error);
-	}
 }

... but why not leave the corruption output here in out_error, change the check to (error == EFSCORRUPTED) and remove the now duplicate corruption message in xfs_mount_validate_sb() (or replace it with a warn/notice message)? This would catch the other EFSCORRUPTED returns in a consistent manner, including another potential duplicate in the write verifier. I guess we'd lose a little specificity between the crc failure and sb validation, but we could add a warn/notice for the former too.

Brian

Quoted 8 lines

fs/xfs/xfs_sb.c +0 -0
 /*
 
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

author

2014-01-30 20:30 UTC #2 lore raw

Quoted 33 lines · Brian Foster

Quoted 16 lines
fs/xfs/xfs_sb.c +0 -0
Quoted 15 lines
index 511cce9..b575317 100644
fs/xfs/xfs_sb.c +2 -0
@@ -617,6 +617,8 @@ 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)) {
+				XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
+						     mp, bp->b_addr);
 				error = EFSCORRUPTED;
 				goto out_error;
 			}
@@ -625,12 +627,8 @@ xfs_sb_read_verify(
 	error = xfs_sb_verify(bp, true);
 
Quoted 9 lines
fs/xfs/xfs_sb.c xfs_sb_read_verify( · line 632
Quoted 9 lines
fs/xfs/xfs_sb.c +1 -5
 out_error:
-	if (error) {
-		if (error != EWRONGFS)
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-					     mp, bp->b_addr);
+	if (error)
 		xfs_buf_ioerror(bp, error);
-	}
 }
... but why not leave the corruption output here in out_error, change the check to (error == EFSCORRUPTED) and remove the now duplicate corruption message in xfs_mount_validate_sb() (or replace it with a warn/notice message)? This would catch the other EFSCORRUPTED returns in a consistent manner, including another potential duplicate in the write verifier. I guess we'd lose a little specificity between the crc failure and sb validation, but we could add a warn/notice for the former too.

Quoted 2 lines

Brian

Well, I went back and forth on this. It's probably philosophical. ;)

Should we emit the corruption error at the point of corruption detection, or at a higher level? I guess my concern was that while *this* caller might catch the return & yell, if another caller got added it might not.

Putting it at the point of detection seemed foolproof in that regard.

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Brian Foster

2014-01-30 20:54 UTC #3 lore raw

Quoted 37 lines · Eric Sandeen

Quoted 33 lines · Brian Foster
Quoted 16 lines
fs/xfs/xfs_sb.c +0 -0
Quoted 15 lines
index 511cce9..b575317 100644
fs/xfs/xfs_sb.c +2 -0
@@ -617,6 +617,8 @@ 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)) {
+				XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
+						     mp, bp->b_addr);
 				error = EFSCORRUPTED;
 				goto out_error;
 			}
@@ -625,12 +627,8 @@ xfs_sb_read_verify(
 	error = xfs_sb_verify(bp, true);
 
Quoted 9 lines
fs/xfs/xfs_sb.c xfs_sb_read_verify( · line 632
Quoted 9 lines
fs/xfs/xfs_sb.c +1 -5
 out_error:
-	if (error) {
-		if (error != EWRONGFS)
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-					     mp, bp->b_addr);
+	if (error)
 		xfs_buf_ioerror(bp, error);
-	}
 }
... but why not leave the corruption output here in out_error, change the check to (error == EFSCORRUPTED) and remove the now duplicate corruption message in xfs_mount_validate_sb() (or replace it with a warn/notice message)? This would catch the other EFSCORRUPTED returns in a consistent manner, including another potential duplicate in the write verifier. I guess we'd lose a little specificity between the crc failure and sb validation, but we could add a warn/notice for the former too.
Quoted 2 lines
Brian

Quoted 8 lines

Well, I went back and forth on this. It's probably philosophical. ;)
Should we emit the corruption error at the point of corruption detection, or at a higher level? I guess my concern was that while *this* caller might catch the return & yell, if another caller got added it might not.
Putting it at the point of detection seemed foolproof in that regard.

Yeah, that makes sense too. If we were consistent, that model would suggest the write verifier corruption message could go and we'd embed corruption errors along with the other associated EFSCORRUPTED returns (at least where the resulting message is appropriate) in xfs_mount_validate_sb().

Either way seems reasonable to me. I guess if all the remaining situations are in fact real corruption situations, the point of detection approach is probably more resilient. It would still be nice to make the verifiers consistent in that though. ;)

Brian

Quoted 2 lines

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2014-02-06 06:43 UTC #4 lore raw

Quoted 53 lines · Brian Foster

Quoted 37 lines · Eric Sandeen
Quoted 33 lines · Brian Foster
Quoted 16 lines
fs/xfs/xfs_sb.c +0 -0
Quoted 15 lines
index 511cce9..b575317 100644
fs/xfs/xfs_sb.c +2 -0
@@ -617,6 +617,8 @@ 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)) {
+				XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
+						     mp, bp->b_addr);
 				error = EFSCORRUPTED;
 				goto out_error;
 			}
@@ -625,12 +627,8 @@ xfs_sb_read_verify(
 	error = xfs_sb_verify(bp, true);
 
Quoted 9 lines
fs/xfs/xfs_sb.c xfs_sb_read_verify( · line 632
Quoted 9 lines
fs/xfs/xfs_sb.c +1 -5
 out_error:
-	if (error) {
-		if (error != EWRONGFS)
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-					     mp, bp->b_addr);
+	if (error)
 		xfs_buf_ioerror(bp, error);
-	}
 }
... but why not leave the corruption output here in out_error, change the check to (error == EFSCORRUPTED) and remove the now duplicate corruption message in xfs_mount_validate_sb() (or replace it with a warn/notice message)? This would catch the other EFSCORRUPTED returns in a consistent manner, including another potential duplicate in the write verifier. I guess we'd lose a little specificity between the crc failure and sb validation, but we could add a warn/notice for the former too.
Quoted 2 lines
Brian
Quoted 8 lines
Well, I went back and forth on this. It's probably philosophical. ;)
Should we emit the corruption error at the point of corruption detection, or at a higher level? I guess my concern was that while *this* caller might catch the return & yell, if another caller got added it might not.
Putting it at the point of detection seemed foolproof in that regard.
Yeah, that makes sense too. If we were consistent, that model would suggest the write verifier corruption message could go and we'd embed corruption errors along with the other associated EFSCORRUPTED returns (at least where the resulting message is appropriate) in xfs_mount_validate_sb().

Quoted 4 lines

Either way seems reasonable to me. I guess if all the remaining situations are in fact real corruption situations, the point of detection approach is probably more resilient. It would still be nice to make the verifiers consistent in that though. ;)

And the conclusion to this discussion is ...?

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2014-02-07 04:23 UTC #5 lore raw

Quoted 54 lines · Dave Chinner

Quoted 53 lines · Brian Foster
Quoted 37 lines · Eric Sandeen
Quoted 33 lines · Brian Foster
Quoted 16 lines
fs/xfs/xfs_sb.c +0 -0
Quoted 15 lines
index 511cce9..b575317 100644
fs/xfs/xfs_sb.c +2 -0
@@ -617,6 +617,8 @@ 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)) {
+				XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
+						     mp, bp->b_addr);
 				error = EFSCORRUPTED;
 				goto out_error;
 			}
@@ -625,12 +627,8 @@ xfs_sb_read_verify(
 	error = xfs_sb_verify(bp, true);
 
Quoted 9 lines
fs/xfs/xfs_sb.c xfs_sb_read_verify( · line 632
Quoted 9 lines
fs/xfs/xfs_sb.c +1 -5
 out_error:
-	if (error) {
-		if (error != EWRONGFS)
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW,
-					     mp, bp->b_addr);
+	if (error)
 		xfs_buf_ioerror(bp, error);
-	}
 }
... but why not leave the corruption output here in out_error, change the check to (error == EFSCORRUPTED) and remove the now duplicate corruption message in xfs_mount_validate_sb() (or replace it with a warn/notice message)? This would catch the other EFSCORRUPTED returns in a consistent manner, including another potential duplicate in the write verifier. I guess we'd lose a little specificity between the crc failure and sb validation, but we could add a warn/notice for the former too.
Quoted 2 lines
Brian
Quoted 8 lines
Well, I went back and forth on this. It's probably philosophical. ;)
Should we emit the corruption error at the point of corruption detection, or at a higher level? I guess my concern was that while *this* caller might catch the return & yell, if another caller got added it might not.
Putting it at the point of detection seemed foolproof in that regard.
Yeah, that makes sense too. If we were consistent, that model would suggest the write verifier corruption message could go and we'd embed corruption errors along with the other associated EFSCORRUPTED returns (at least where the resulting message is appropriate) in xfs_mount_validate_sb().

Quoted 6 lines

Quoted 4 lines
Either way seems reasonable to me. I guess if all the remaining situations are in fact real corruption situations, the point of detection approach is probably more resilient. It would still be nice to make the verifiers consistent in that though. ;)
And the conclusion to this discussion is ...?

I think Brian has some valid points, I'll take another look at it.

Thanks,
-Eric

Quoted 4 lines

Cheers,
Dave.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs