Re: [PATCH] btrfs: loosen the block-group-tree feature dependency check

[David Sterba <dsterba@suse.cz>]

On Wed, Sep 21, 2022 at 07:40:19AM +0800, Qu Wenruo wrote:
> On 2022/9/12 13:44, Qu Wenruo wrote:
> > [BUG]
> > When one user did a wrong try to clear block group tree, which can not
> > be done through mount option, by using "-o clear_cache,space_cache=v2",
> > it will cause the following error on a fs with block-group-tree feature:
> > 
> >   BTRFS info (device dm-1): force clearing of disk cache
> >   BTRFS info (device dm-1): using free space tree
> >   BTRFS info (device dm-1): clearing free space tree
> >   BTRFS info (device dm-1): clearing compat-ro feature flag for FREE_SPACE_TREE (0x1)
> >   BTRFS info (device dm-1): clearing compat-ro feature flag for FREE_SPACE_TREE_VALID (0x2)
> >   BTRFS error (device dm-1): block-group-tree feature requires fres-space-tree and no-holes
> >   BTRFS error (device dm-1): super block corruption detected before writing it to disk
> >   BTRFS: error (device dm-1) in write_all_supers:4318: errno=-117 Filesystem corrupted (unexpected superblock corruption detected)
> >   BTRFS warning (device dm-1: state E): Skipping commit of aborted transaction.
> > 
> > [CAUSE]
> > Although the dependency for block-group-tree feature is just an
> > artificial one (to reduce test matrix), we put the dependency check into
> > btrfs_validate_super().
> > 
> > This is too strict, and during space cache clearing, we will have a
> > window where free space tree is cleared, and we need to commit the super
> > block.
> > 
> > In that window, we had block group tree without v2 cache, and triggered
> > the artificial dependency check.
> > 
> > This is not necessary at all, especially for such a soft dependency.
> > 
> > [FIX]
> > Introduce a new helper, btrfs_check_features(), to do all the runtime
> > limitation checks, including:
> > 
> > - Unsupported incompat flags check
> > 
> > - Unsupported compat RO flags check
> > 
> > - Setting missing incompat flags
> > 
> > - Aritifical feature dependency checks
> >    Currently only block group tree will rely on this.
> > 
> > - Subpage runtime check for v1 cache
> > 
> > With this helper, we can move quite some checks from
> > open_ctree()/btrfs_remount() into it, and just call it after
> > btrfs_parse_options().
> > 
> > Now "-o clear_cache,space_cache=v2" will not trigger above error
> > anymore.
> > 
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Any feedback? I really hope this patch can be merged before we expose 
> the kernel support for block group tree.
> 
> Or clear_space_cache mount option can easily flip the fs to RO.

I'm aware of the patch, as it is technically a regression fix it can go
to any rc, I was processing other patches that must be merged before the
pull request. That was before rc6 and there will be rc7 so there's more
time to add this still to the first batch.