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

[Brian Foster <bfoster@redhat.com>]

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. ;)

Brian

> -Eric
> 

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