Re: [PATCH v4] btrfs-progs: fsfeatures: properly merge -O and -R options

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/10/7 16:34, Anand Jain wrote:
> On 06/10/2022 23:18, David Sterba wrote:
>> On Wed, Oct 05, 2022 at 09:48:07AM +0800, Qu Wenruo wrote:
>>> [BUG]
>>> Commit "btrfs-progs: prepare merging compat feature lists" tries to
>>> merged "-O" and "-R" options, as they don't correctly represents
>>> btrfs features.
>>>
>>> But that commit caused the following bug during mkfs for experimental
>>> build:
>>>
>>>    $ mkfs.btrfs -f -O block-group-tree  /dev/nvme0n1
>>>    btrfs-progs v5.19.1
>>>    See http://btrfs.wiki.kernel.org for more information.
>>>
>>>    ERROR: superblock magic doesn't match
>>>    ERROR: illegal nodesize 16384 (not equal to 4096 for mixed block
>>> group)
>>>
>>> [CAUSE]
>>> Currently btrfs_parse_fs_features() will return a u64, and reuse the
>>> same u64 for both incompat and compat RO flags for experimental branch.
>>>
>>> This can easily leads to conflicts, as
>>> BTRFS_FEATURE_INCOMPAT_MIXED_BLOCK_GROUP and
>>> BTRFS_FEATURE_COMPAT_RO_BLOCK_GROUP_TREE both share the same bit
>>> (1 << 2).
>>>
>>> Thus for above case, mkfs.btrfs believe it has set MIXED_BLOCK_GROUP
>>> feature, but what we really want is BLOCK_GROUP_TREE.
>>>
>>> [FIX]
>>> Instead of incorrectly re-using the same bits in btrfs_feature, split
>>> the old flags into 3 flags:
>>>
>>> - incompat_flag
>>> - compat_ro_flag
>>> - runtime_flag
>>>
>>> The first two flags are easy to understand, the corresponding flag of
>>> each feature.
>>> The last runtime_flag is to compensate features which doesn't have any
>
>   I read the commit 5ac6e02665a6 ("btrfs-progs: mkfs: add -R|--runtime-
>   features option") too. But I still can't comprehend the problem that
>   the runtime flags solved; because the -O option enables the same
>   runtime features.

The old parse_fs_features() can only return one u64, thus it can not
handle compat_ro flags.

Furthermore for quota it has no on-disk format at all, thus not a good
fit for the old parse_fs_features() code.

>
>>> on-disk flag set, like QUOTA and LIST_ALL.
>
>   LIST_ALL is not a (kernel) feature.

No one is saying it is. What's the problem?

The commit message clearly said "which doesn't have any on-disk format
set". And since "list-all" is a valid parameter for "-O"/"-R" option, we
just treat it as a feature.

There is no requirement to bind a "-O"/"-R" feature to any kernel feature.

>
>
>>> And since we're no longer using a single u64 as features, we have to
>>> introduce a new structure, btrfs_mkfs_features, to contain above 3
>>> flags.
>>>
>>> This also mean, things like default mkfs features must be converted to
>>> use the new structure, thus those old macros are all converted to
>>> const static structures:
>>>
>>> - BTRFS_MKFS_DEFAULT_FEATURES + BTRFS_MKFS_DEFAULT_RUNTIME_FEATURES
>>>    -> btrfs_mkfs_default_features
>>>
>>> - BTRFS_CONVERT_ALLOWED_FEATURES -> btrfs_convert_allowed_features
>>>
>>> And since we're using a structure, it's not longer as easy to implement
>>> a disallowed mask.
>>>
>>> Thus functions with @mask_disallowed are all changed to using
>>> an @allowed structure pointer (which can be NULL).
>>>
>>> Finally if we have experimental features enabled, all features can be
>>> specified by -O options, and we can output a unified feature list,
>>> instead of the old split ones.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>> Changelog:
>>> v2:
>>> - Fix convert test failure due to missing allowed features
>>>
>>> v3:
>>> - Fix a bug that we can not unset free-space-tree for non-experimental
>>>    build
>>>
>>> - Fix a bug that free-space-tree compat RO flags are not properly set
>>>    for non-experimental build
>>>
>>> v4:
>>> - Address David's concern of new BTRFS_FEATURE_GENERIC_* defines
>>>    By introducing a new btrfs_mkfs_features structure, so we don't need
>>>    extra re-definitions.
>>>
>>>    The amount of code change is still the same as v3, since we have a
>>>    larger interface change.
>>
>> Thanks, this version looks good to me and maybe even better than what I
>> intended to implement myself. The amount of changed lines is high but
>> the core changes are clear and the rest is API update.
>
>
>> Added to devel.
>
> Still, there is something to take care of, I rebase to devel.
>
>
> $ mkfs.btrfs -f -O extent-tree-v2 /dev/nvme0n1
>
> ERROR: superblock magic doesn't match
> NOTE: several default settings have changed in version 5.15, please make
> sure
>        this does not affect your deployments:
>        - DUP for metadata (-m dup)
>        - enabled no-holes (-O no-holes)
>        - enabled free-space-tree (-R free-space-tree)
>
> Segmentation fault (core dumped)
>
>
>
> static int cache_block_group(struct btrfs_root *root,
>                               struct btrfs_block_group *block_group)
> {
>          struct btrfs_path *path;
>          int ret;
>          struct btrfs_key key;
>          struct extent_buffer *leaf;
>          struct extent_io_tree *free_space_cache;
>          int slot;
>          u64 last;
>          u64 hole_size;
>
>          if (!block_group)
>                  return 0;
>
>          root = btrfs_extent_root(root->fs_info, 0);  <--- root is NULL.

I can definitely look into the situation.


>
>
>
>