[PATCH] btrfs: deprecate check_int* feature

[PATCH] btrfs: deprecate check_int* feature

[Qu Wenruo]

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(+)

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;
-- 
2.41.0

Re: [PATCH] btrfs: deprecate check_int* feature

[Christoph Hellwig]

The subject reads odd, why not spell out check_integrity?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] btrfs: deprecate check_int* feature

[David Sterba]

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.