Re: [PATCH v2 00/15] btrfs: make open_ctree() init/exit sequence strictly matched

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

On 2022/10/24 21:47, David Sterba wrote:
> On Wed, Oct 12, 2022 at 05:12:56PM +0800, Qu Wenruo wrote:
>> [Changelog]
>> v2:
>> - Rebased to latest misc-next
>>    Most conflicts comes from the new function btrfs_check_features().
>>
>>
>> Just like init_btrfs_fs(), open_ctree() also has tons of different
>> labels for its error handling.
>>
>> And unsurprisingly the error handling labels are not matched correctly,
>> e.g. we always call btrfs_mapping_tree_free() even we didn't reach
>> sys chunk array read.
>>
>> And every time we need to add some new function, it will be a disaster
>> just to understand where the new function should be put and how the
>> error handling should be done.
>>
>> This patchset will follow the init_btrfs_fs() method, by introducing
>> an open_ctree_seq[] array, which contains the following sections:
>>
>> - btree_inode init/exit
>> - super block read and verification
>> - mount options and features check
>> - workqueues init/exit
>> - chunk tree init/exit
>> - tree roots init/exit
>> - mount time check and various item load
>> - sysfs init/exit
>> - block group tree init/exit
>> - subvolume trees init/exit
>> - kthread init/exit
>> - qgroup init/exit
>>
>> The remaining part of open_ctree() is only less than 50 lines, and are
>> all related to the very end of the mount progress, including log-replay,
>> uuid tree check.
>
> I'm not sure it's a good idea to split the open_ctree to the sequence if
> initializers, some of the code looks like it's not isolated the same way
> as it was in the module init/exit. The readability is IMHO also worse,
> verifying that some parts depend on each other requires jumping in the
> file. Maybe some parts can be put into more helpers and we can make the
> exit sequence robust enough so we don't need tons of labels and the
> whole can be called regardless of from where it would be called.
>
> This is similar to the array based approach but keeps the code in one
> function. As it is implemented in this patchset I think it's taking it
> too far.

All right, then I'd manually fix the wrongly matched tags.

Thanks,
Qu