Re: [PATCH 09/13] xfs_repair: better checking of v5 attributes

[Eric Sandeen <sandeen@sandeen.net>]

On 9/14/15 2:44 PM, Brian Foster wrote:
> On Wed, Sep 09, 2015 at 02:34:07PM -0500, Eric Sandeen wrote:
>> The commit:
>>
>> 0519f66 xfs_repair: better checking of v5 metadata fields
>>
>> added new corruption checks to dir2.c but missed the similar
>> code in attr_repair.c; add that here.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
>> ---
>>  repair/attr_repair.c |    9 +++++++++
>>  1 files changed, 9 insertions(+), 0 deletions(-)
>>
>> diff --git a/repair/attr_repair.c b/repair/attr_repair.c
>> index 2aafdf6..c8ba484 100644
>> --- a/repair/attr_repair.c
>> +++ b/repair/attr_repair.c
>> @@ -201,6 +201,15 @@ traverse_int_dablock(xfs_mount_t	*mp,
>>  			goto error_out;
>>  		}
>>  
>> +		/* corrupt node; rebuild the dir. */
>> +		if (bp->b_error == -EFSBADCRC || bp->b_error == -EFSCORRUPTED) {
>> +			libxfs_putbuf(bp);
>> +			do_warn(
>> +_("corrupt tree block %u for directory inode %" PRIu64 "\n"),
>> +				bno, da_cursor->ino);
>> +			goto error_out;
>> +		}
>> +
> 
> Hmm, well this certainly looks similar, but is it the right thing to do
> for xattrs? I haven't followed through how exactly directories are
> rebuilt, but there does appear to be a recovery path in the dir2
> context. A failure there simply puts the inode on a "bad" list to be
> rebuilt later, presumably from data collected from all of the inodes.
> 
> If we fail here, it looks like we just clear the attribute fork. So are
> we failing too hard, too soon here if a dablock crc happens to be
> incorrect?

Brian & I talked about this briefly on IRC.  The upshot:

attr checking already has many failure points, and if any one fails, the attr
may get nuked.

dir checking already has many failure points, and if any one fails, the dir
can get rebuilt.

All this patch does is add another check to several existing checks in the
attr code, and if it fails, whatever action was taken before for any other
error will also be taken for a bad CRC or a verifier failure.  So, this 
doesn't really introduce any new or more draconian behavior; it simply adds
one more check (the CRC, which had previously been ignored) to a host of other
verifications in this code, with the same results as before if this new check
fails.  So I think it's fine as it is.

Thanks,
-Eric

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