Re: [PATCH] btrfs: remove runtime tweakable feature sysfs interface

[Boris Burkov <boris@bur.io>]

On Fri, Aug 21, 2026 at 07:42:10PM +0930, Qu Wenruo wrote:
> There are 2 features that are marked runtime tweakable inside
> /sys/fs/btrfs/features/
> 
> - acl
>   Which is a mount option, and it will not show up in
>   /sys/fs/btrfs/<fsid>/features/ directory anyway.
> 
> - extended_iref
>   This feature can only be enabled, but not disabled at runtime.
>   Furthermore it's already the default behavior since 3.12.
> 
>   So it means this feature is always enabled and cannot be disabled for
>   modern btrfs.
> 
> So there is no need to maintain the ability to modify btrfs' runtime
> features through sysfs.
> 
> And furthermore, the existing btrfs_feature_attr_store() is race-prone,
> it relies on fs_info->transaction_kthread, but our sysfs interfaces are
> enabled before transaction_kthread.
> 
> Meaning at mount time a sysfs write can trigger NULL pointer dereference
> if the transaction_kthread is not yet initialized.
> The opposite is also possible during unmount.
> 
> Thankfully that race is not possible in the real world, as the only
> supported feature is already enabled.
> 
> But it also means we do not really need to keep the race-prone
> infrastructure, so just remove it completely, and make the per-module
> and per-mount features files to be completely read-only.
> 
> Even with the sysfs tweakable features removed, we can still enable
> extended_iref feature through ioctl.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

FYI, I have been gating some behaviors like dynamic/periodic reclaim on
sysfs files not in features/ and I imagine there are some more out there
(like bg_reclaim_threshold). That has two relevant implications:

- We do still do "runtime feature setting" not through ioctl/mount opt.
  If we want to converge on only ioctl for that, I am open to it. Mount
  options only end in tears in my experience (have to be very careful to
  handle all remount scenarios as has played out with free space tree
  and async discard at least).

- Some features are not at whole fs granularity (like above
  per-space-info features) so it doesn't make sense to have this generic
  features/ mechanism anyway.

So with all that said, I support getting rid of this, but I apologize if
my stuff makes life more complicated for our "what is enabled" model
even with this patch.

Reviewed-by: Boris Burkov <boris@bur.io>

> ---
>  fs/btrfs/sysfs.c | 121 ++---------------------------------------------
>  1 file changed, 4 insertions(+), 117 deletions(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 39cb01ee441a..1f78bb1cf813 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -83,8 +83,7 @@ struct raid_kobject {
>  #define BTRFS_FEAT_ATTR(_name, _feature_set, _feature_prefix, _feature_bit)  \
>  static struct btrfs_feature_attr btrfs_attr_features_##_name = {	     \
>  	.kobj_attr = __INIT_KOBJ_ATTR(_name, S_IRUGO,			     \
> -				      btrfs_feature_attr_show,		     \
> -				      btrfs_feature_attr_store),	     \
> +				      btrfs_feature_attr_show, NULL),	     \
>  	.feature_set	= _feature_set,					     \
>  	.feature_bit	= _feature_prefix ##_## _feature_bit,		     \
>  }
> @@ -130,132 +129,22 @@ static u64 get_features(struct btrfs_fs_info *fs_info,
>  		return btrfs_super_incompat_flags(disk_super);
>  }
>  
> -static void set_features(struct btrfs_fs_info *fs_info,
> -			 enum btrfs_feature_set set, u64 features)
> -{
> -	struct btrfs_super_block *disk_super = fs_info->super_copy;
> -	if (set == FEAT_COMPAT)
> -		btrfs_set_super_compat_flags(disk_super, features);
> -	else if (set == FEAT_COMPAT_RO)
> -		btrfs_set_super_compat_ro_flags(disk_super, features);
> -	else
> -		btrfs_set_super_incompat_flags(disk_super, features);
> -}
> -
> -static int can_modify_feature(struct btrfs_feature_attr *fa)
> -{
> -	int val = 0;
> -	u64 set, clear;
> -	switch (fa->feature_set) {
> -	case FEAT_COMPAT:
> -		set = BTRFS_FEATURE_COMPAT_SAFE_SET;
> -		clear = BTRFS_FEATURE_COMPAT_SAFE_CLEAR;
> -		break;
> -	case FEAT_COMPAT_RO:
> -		set = BTRFS_FEATURE_COMPAT_RO_SAFE_SET;
> -		clear = BTRFS_FEATURE_COMPAT_RO_SAFE_CLEAR;
> -		break;
> -	case FEAT_INCOMPAT:
> -		set = BTRFS_FEATURE_INCOMPAT_SAFE_SET;
> -		clear = BTRFS_FEATURE_INCOMPAT_SAFE_CLEAR;
> -		break;
> -	default:
> -		btrfs_warn(NULL, "sysfs: unknown feature set %d", fa->feature_set);
> -		return 0;
> -	}
> -
> -	if (set & fa->feature_bit)
> -		val |= 1;
> -	if (clear & fa->feature_bit)
> -		val |= 2;
> -
> -	return val;
> -}
> -
>  static ssize_t btrfs_feature_attr_show(struct kobject *kobj,
>  				       struct kobj_attribute *a, char *buf)
>  {
>  	int val = 0;
>  	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
>  	struct btrfs_feature_attr *fa = to_btrfs_feature_attr(a);
> +
>  	if (fs_info) {
>  		u64 features = get_features(fs_info, fa->feature_set);
>  		if (features & fa->feature_bit)
>  			val = 1;
> -	} else
> -		val = can_modify_feature(fa);
> +	}
>  
>  	return sysfs_emit(buf, "%d\n", val);
>  }
>  
> -static ssize_t btrfs_feature_attr_store(struct kobject *kobj,
> -					struct kobj_attribute *a,
> -					const char *buf, size_t count)
> -{
> -	struct btrfs_fs_info *fs_info;
> -	struct btrfs_feature_attr *fa = to_btrfs_feature_attr(a);
> -	u64 features, set, clear;
> -	unsigned long val;
> -	int ret;
> -
> -	fs_info = to_fs_info(kobj);
> -	if (!fs_info)
> -		return -EPERM;
> -
> -	if (sb_rdonly(fs_info->sb))
> -		return -EROFS;
> -
> -	ret = kstrtoul(skip_spaces(buf), 0, &val);
> -	if (ret)
> -		return ret;
> -
> -	if (fa->feature_set == FEAT_COMPAT) {
> -		set = BTRFS_FEATURE_COMPAT_SAFE_SET;
> -		clear = BTRFS_FEATURE_COMPAT_SAFE_CLEAR;
> -	} else if (fa->feature_set == FEAT_COMPAT_RO) {
> -		set = BTRFS_FEATURE_COMPAT_RO_SAFE_SET;
> -		clear = BTRFS_FEATURE_COMPAT_RO_SAFE_CLEAR;
> -	} else {
> -		set = BTRFS_FEATURE_INCOMPAT_SAFE_SET;
> -		clear = BTRFS_FEATURE_INCOMPAT_SAFE_CLEAR;
> -	}
> -
> -	features = get_features(fs_info, fa->feature_set);
> -
> -	/* Nothing to do */
> -	if ((val && (features & fa->feature_bit)) ||
> -	    (!val && !(features & fa->feature_bit)))
> -		return count;
> -
> -	if ((val && !(set & fa->feature_bit)) ||
> -	    (!val && !(clear & fa->feature_bit))) {
> -		btrfs_info(fs_info,
> -			"%sabling feature %s on mounted fs is not supported.",
> -			val ? "En" : "Dis", fa->kobj_attr.attr.name);
> -		return -EPERM;
> -	}
> -
> -	btrfs_info(fs_info, "%s %s feature flag",
> -		   val ? "Setting" : "Clearing", fa->kobj_attr.attr.name);
> -
> -	spin_lock(&fs_info->super_lock);
> -	features = get_features(fs_info, fa->feature_set);
> -	if (val)
> -		features |= fa->feature_bit;
> -	else
> -		features &= ~fa->feature_bit;
> -	set_features(fs_info, fa->feature_set, features);
> -	spin_unlock(&fs_info->super_lock);
> -
> -	/*
> -	 * We don't want to do full transaction commit from inside sysfs
> -	 */
> -	set_bit(BTRFS_FS_NEED_TRANS_COMMIT, &fs_info->flags);
> -	wake_up_process(fs_info->transaction_kthread);
> -
> -	return count;
> -}
> -
>  static umode_t btrfs_feature_visible(struct kobject *kobj,
>  				     struct attribute *attr, int unused)
>  {
> @@ -269,9 +158,7 @@ static umode_t btrfs_feature_visible(struct kobject *kobj,
>  		fa = attr_to_btrfs_feature_attr(attr);
>  		features = get_features(fs_info, fa->feature_set);
>  
> -		if (can_modify_feature(fa))
> -			mode |= S_IWUSR;
> -		else if (!(features & fa->feature_bit))
> +		if (!(features & fa->feature_bit))
>  			mode = 0;
>  	}
>  
> -- 
> 2.54.0
>