Re: [PATCH] xfs: don't treat unknown di_flags[2] as corruption in scrub

[Eric Sandeen <sandeen@redhat.com>]

On 9/18/18 12:20 AM, Dave Chinner wrote:
> On Mon, Sep 17, 2018 at 09:41:35PM -0500, Eric Sandeen wrote:
>> xchk_inode_flags[2]() currently treats any di_flags[2] values that the
>> running kernel doesn't recognize as corruption, and calls
>> xchk_ino_set_corrupt() if they are set.  However, it's entirely possible
>> that these flags were set in some newer kernel and are quite valid,
>> but ignored in this kernel.
>>
>> (Validators don't care one bit about unknown di_flags[2].)
>>
>> Call xchk_ino_set_warning instead, because this may or may not actually
>> indicate a problem.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
>> index 5b3b177..e53ed83 100644
>> --- a/fs/xfs/scrub/inode.c
>> +++ b/fs/xfs/scrub/inode.c
>> @@ -126,8 +126,9 @@
>>  {
>>  	struct xfs_mount	*mp = sc->mp;
>>  
>> +	/* Unknown di_flags could simply be from newer kernel */
>>  	if (flags & ~XFS_DIFLAG_ANY)
>> -		goto bad;
>> +		xchk_ino_set_warning(sc, ino);
> 
> There's only one flag in that set, right?

Yes, (1 << 15).

> And we only need that flag
> for a future v2 inode features we add? i.e. any new feature will be
> on a v3 inode format because the v2 format is the legacy inode
> format and we're not developing new features for it.

Ok...

> [ There's also the minor issue that the remaining flag bit in
> di_flags is reserved for the "more flags" flag bit so that we know
> to grab flags from some other padding in the inode we redefined to
> hold more flags in the v2 inode format. But that's irrelevant now
> because it's a legacy format. ]
> 
> IOWs, I think the original code here is just fine because we're not
> going to add new v2 format inode features in the future.

Ok, if we're absolutely 100% sure that no future kernel will ever use
that flag, then yes, it's corruption if it's ever found to be set.
I wasn't quite ready to draw that line in the sand.

Should probably #define a new XFS_DIFLAG_NEVER or something then, to make
it crystal clear?

-Eric