| Message ID | cover.1665565866.git.wqu@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: make open_ctree() init/exit sequence strictly matched | expand |
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.
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
[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. Also to do better testing, for DEBUG build there will be a new mount option, "fail_mount=%u" to allow open_ctree() to fail at certain stage of open_ctree_seq[]. Unfortunately since that mount option can only be parsed in open_ctree_features_init(), this means we can only fail after stage 2. But this should still provide much better testing coverage. Qu Wenruo (15): btrfs: initialize fs_info->sb at the very beginning of open_ctree() btrfs: remove @fs_devices argument from open_ctree() btrfs: extract btree inode init code into its own init/exit helpers btrfs: extract super block read code into its own init helper btrfs: extract mount options and features init code into its own init helper btrfs: move btrfs_init_workqueus() and btrfs_stop_all_workers() into open_ctree_seq[] btrfs: extract chunk tree read code into its own init/exit helpers btrfs: extract tree roots and zone info initialization into init/exit helpers btrfs: extract mount time checks and items load code into its init helper btrfs: extract sysfs init into its own helper btrfs: extra block groups read code into its own init/exit helpers btrfs: move the fs root related code into its own init/exit helpers btrfs: extract kthread code into its own init/exit helpers btrfs: move qgroup init/exit code into open_ctree_seq[] array btrfs: introduce a debug mount option to do error injection for each stage of open_ctree() fs/btrfs/ctree.h | 7 + fs/btrfs/disk-io.c | 611 +++++++++++++++++++++++++++++---------------- fs/btrfs/disk-io.h | 4 +- fs/btrfs/super.c | 18 +- 4 files changed, 418 insertions(+), 222 deletions(-)