Re: [PATCH v3 3/5] btrfs-progs: separate block group tree from extent tree v2

[Qu Wenruo <wqu@suse.com>]

On 2022/9/2 17:21, David Sterba wrote:
> On Thu, Sep 01, 2022 at 08:15:07PM +0800, Qu Wenruo wrote:
>>>>> --- a/common/fsfeatures.c
>>>>> +++ b/common/fsfeatures.c
>>>>> @@ -172,6 +172,14 @@ static const struct btrfs_feature
>>>>> runtime_features[] = {
>>>>>            VERSION_TO_STRING2(safe, 4,9),
>>>>>            VERSION_TO_STRING2(default, 5,15),
>>>>>            .desc        = "free space tree (space_cache=v2)"
>>>>> +    }, {
>>>>> +        .name        = "block-group-tree",
>>>>> +        .flag        = BTRFS_RUNTIME_FEATURE_BLOCK_GROUP_TREE,
>>>>> +        .sysfs_name = "block_group_tree",
>>>>> +        VERSION_TO_STRING2(compat, 6,0),
>>>>> +        VERSION_NULL(safe),
>>>>> +        VERSION_NULL(default),
>>>>> +        .desc        = "block group tree to reduce mount time"
>>>>
>>>> Like explaining that this is a runtime feature and I have not noticed
>>>> until I tried to test it expecting to see it among the mkfs-time
>>>> features but there was nothing in 'mkfs.btrfs -O list-all'.
>>>>
>>>> This is a mkfs-time feature as it creates a fundamental on-disk
>>>> structure, basically a subset of extent tree.
>>>
>>> This comes to the decision to make bg-tree feature as a compat RO flag.
>>>
>>> As we didn't put free-space-tree into "-O" options, but "-R" options.
>>> So the same should be done for most compat RO flags.
>>>
>>> Furthermore I remember I discussed about this before, extent tree change
>>> should not need a full incompat flag, as pure read-only tools, like
>>> btrfs-fuse should still be able to read the subvolume/csum/chunk/root
>>> trees without any problem.
>>>
>>> So following above reasons, bg-tree is compat RO, and compat RO goes
>>> into "-R" options, I see no reason to put it into "-O" options.
>>
>> After more consideration, I believe we shouldn't split all the features
>> (including quota) between "-O" and "-R" options.
> 
> After reading your previous I got to the same conclusion.
> 
>> Firstly, although free space tree is compat RO (and a lot of future
>> features will also be compat RO), it's still a on-disk format change (a
>> new tree, some new keys).
>>
>> It's even a bigger change compared to NO_HOLES features.
>> No to mention the block group tree.
>>
>> Now we have a very bad split for -R and -O, some of them are on-disk
>> format change that is large enough, but still compat RO.
> 
> Agreed.
> 
>> Some of them should be compat RO, but still set as incompt flags.
>>
>> To me, end users should not really bother what the feature is
>> implemented, they only need to bother:
>>
>> - What the feature is doing
>> - What is the compatibility
>>     The incompat and compat RO doesn't make too much difference for most
>>     users, they just care about which kernel version is compatible.
>>
>> So from this point of view, -O/-R split it not really helpful from the
>> very beginning.
>>
>> It may make sense for quota, which is the only exception, it's supported
>> from the very beginning, without a compat RO/incompat flag.
>>
>> But for more and more features, -O/-R split doesn't make much sense.
> 
> Yeah, the free-space-tree is misplaced and I did not realize that back
> then. That something is possible to switch on at run time by a mount
> option should not be the only condition to put the option to the -R option.
> 
> Quota are maybe still a good example of the runtime feature, there's a
> command to enable and disable it. There are additional structures
> created or deleted but it's not something fundamental. The distinction
> in the options should hint at what's the type "what if I don't select
> this now, can I turn it on later?", perhaps documentation should be more
> explicit about that.

Quota tree is a special case, just because it's from day-one, thus no 
compat/compat ro/incompat flags needed at all.

To me, we can accept one exception.

> 
> For compatibility we need to keep free-space-tree under -R but we can
> add an alias to -O and everything of that sort add there too, like the
> block group tree.

That's simple, make -R deprecated, and treat -R just as -O internally, 
and put all features including quota into -O.

Of course, we may need some small changes, as now one fs feature needs 1 
or 0 compat/compat ro/incompat flags set.
But everything else, from the compat/safe/default string can be 
inherited from the existing format.

By this, we have the minimal code change, while still keeps the same 
compatibility (in fact, greatly enlarged -O options)

Thanks,
Qu