| Message ID | d1b4174922a786884a1a3bfdcbbc208925797f4b.1687773318.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: deprecate check_int* feature | expand |
The subject reads odd, why not spell out check_integrity?
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Mon, Jun 26, 2023 at 05:55:25PM +0800, Qu Wenruo wrote: > Check_int feature needs to be enabled at compile time and then enable > through mount option. > > Although it provides some unique feature which can not be provided by > any other sanity checks, it does not only have high CPU and memory > overhead, but also maintenance burden. > > For example, check_int is the only caller of btrfs_map_block() with > @need_raid_map = 0. > > Considering most btrfs developers are not even testing this feature, I'm > here to purpose the deprecation of this feature. I'm on the fence regarding the integrity checker. It once was very useful and discovered a very serious problem when metadata block could have been left uncommitted before the superblock due to missing serialization. I can't find the commit though. The whole idea of tracking the metadata blocks separately makes sense but it is true the memory and CPU overhead is high. The new approach by tree-checker is not the same but does not need to be compiled and enabled separately and works on all builds. Maintenance burden hasn't been high but there are quite a few changes in the interfaces, types or other cleanups that basically change the original code from 2011. I tried to build it and run some time ago, in combination with KASAN it produced lots of warnings (use-after-free) and ended by a crash at some point. So I doubt anybody is actively using it or maybe without the debugging functionality. The points for keeping it are not many, only the strength of the verification is the highest of what we have. But at a high runtime cost, no interest and unknown state regarding reliability of the implementation. In summary: Pro: - can verify metadata/data integrity in a broder scope than tree-checker Against: - maintenance burden (not high but still) - probably lots of unused code - unknown implementation status With my maintainer hat on I'd rather see it fixed and used more, but this requires somebody willing doing the work. Otherwise I'm personally not against removing it. Even if it's a feature for developers, a deprecation period makes to give people time to react. The closest target is 6.7, i.e. deprecation will start at 6.6. Possibly it could go to 6.5 just to give us a two release period.
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 8b1c1225245e..3eaa0775cef1 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -709,12 +709,16 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, break; #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY case Opt_check_integrity_including_extent_data: + btrfs_warn(info, + "check_int feature is marked deprecated and would be removed soon"); btrfs_info(info, "enabling check integrity including extent data"); btrfs_set_opt(info->mount_opt, CHECK_INTEGRITY_DATA); btrfs_set_opt(info->mount_opt, CHECK_INTEGRITY); break; case Opt_check_integrity: + btrfs_warn(info, + "check_int feature is marked deprecated and would be removed soon"); btrfs_info(info, "enabling check integrity"); btrfs_set_opt(info->mount_opt, CHECK_INTEGRITY); break; @@ -727,6 +731,8 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, goto out; } info->check_integrity_print_mask = intarg; + btrfs_warn(info, + "check_int feature is marked deprecated and would be removed soon"); btrfs_info(info, "check_integrity_print_mask 0x%x", info->check_integrity_print_mask); break;
Check_int feature needs to be enabled at compile time and then enable through mount option. Although it provides some unique feature which can not be provided by any other sanity checks, it does not only have high CPU and memory overhead, but also maintenance burden. For example, check_int is the only caller of btrfs_map_block() with @need_raid_map = 0. Considering most btrfs developers are not even testing this feature, I'm here to purpose the deprecation of this feature. For now only extra warning messages would be outputted, the feature itself would still work. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/super.c | 6 ++++++ 1 file changed, 6 insertions(+)