xfs
[Top] [All Lists]

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

To: Brian Foster <bfoster@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
Subject: Re: [PATCH] xfs: limit superblock corruption errors to probable corruption
From: Eric Sandeen <sandeen@xxxxxxxxxx>
Date: Thu, 30 Jan 2014 14:30:38 -0600
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <52EAB56D.2050203@xxxxxxxxxx>
References: <52E88D8B.90208@xxxxxxxxxx> <52EAB56D.2050203@xxxxxxxxxx>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.2.0
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.

-Eric

<Prev in Thread] Current Thread [Next in Thread>