Re: [PATCH 1/3] btrfs: Add incompat flags check for btrfs_check_super_valid()

[David Sterba <dsterba@suse.cz>]

On Fri, Apr 20, 2018 at 10:32:03PM +0800, Anand Jain wrote:
> 
> 
> On 04/19/2018 05:38 PM, Qu Wenruo wrote:
> > Although we have already checked incompat flags manually before really
> > mounting it, we could still enhance btrfs_check_super_valid() to check
> > incompat flags for later write time super block validation check.
> > 
> > This patch adds such incompat flags check for btrfs_check_super_valid(),
> > currently it won't be triggered, but provides the basis for later write
> > time check.
> > 
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > ---
> >   fs/btrfs/disk-io.c | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> > 
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index 60caa68c3618..ec123158f051 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
> >   		ret = -EINVAL;
> >   	}
> >   
> > +	/*
> > +	 * Before calling btrfs_check_super_valid() we have already checked
> > +	 * incompat flags. So if we developr new incompat flags, it's must be
> > +	 * some corruption.
> > +	 */
> > +	if (btrfs_super_incompat_flags(sb) & ~BTRFS_FEATURE_INCOMPAT_SUPP) {
> > +		btrfs_err(fs_info,
> > +		"corrupted incompat flags detected 0x%llx, supported 0x%llx",

> 2707         features = btrfs_super_incompat_flags(disk_super) &
> 2708                 ~BTRFS_FEATURE_INCOMPAT_SUPP;
> 2709         if (features) {
> 2710                 btrfs_err(fs_info,
> 2711                     "cannot mount because of unsupported optional features (%llx)",
> 2712                     features);
> 2713                 err = -EINVAL;
> 2714                 goto fail_alloc;
> 2715         }

So there's a "user experience" change, now that you pointed out the
other check. If a filesystem with incompat bits set will be mounted,
this will say 'you have corrupted filesystem', which is not IMO what we
want to tell.

Tough the extended btrfs_check_super_valid could catch the corrupted
incompat bits, what we need at mount time is wording from the 2nd
message ("cannot mount unsuppported features").

I think that there should be a separate function that does the
pre-commit checks, calls btrfs_check_super_valid and also validates the
incompat bit.