[PATCH] xfs: limit superblock corruption errors to probable corruption

[Dave Chinner]

On Thu, Jan 30, 2014 at 03:54:16PM -0500, Brian Foster wrote:
> On 01/30/2014 03:30 PM, Eric Sandeen wrote:
> > On 1/30/14, 2:26 PM, Brian Foster wrote:
> >>> diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
> >>>> index 511cce9..b575317 100644
> >>>> --- a/fs/xfs/xfs_sb.c
> >>>> +++ b/fs/xfs/xfs_sb.c
> >>>> @@ -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);
> >>>> -	}
> >>>>  }
> >> ... 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
> >>
> > 
> > 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. ;)

And the conclusion to this discussion is ...?

Cheers,

Dave.
-- 
Dave Chinner
david at fromorbit.com