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

[Eric Sandeen <sandeen@sandeen.net>]

On 2/6/14, 12:43 AM, Dave Chinner wrote:
> 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 ...?

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

Thanks,
-Eric

> Cheers,
> 
> Dave.
> 

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