Re: [PATCH v2 02/18] btrfs: split out the mount option validation code into its own helper

[Anand Jain <anand.jain@xxxxxxxxxx>]

> +static bool check_options(struct btrfs_fs_info *info, unsigned long flags)
> +{
> +	if (!(flags & SB_RDONLY) &&
> +	    (check_ro_option(info, BTRFS_MOUNT_NOLOGREPLAY, "nologreplay") ||
> +	     check_ro_option(info, BTRFS_MOUNT_IGNOREBADROOTS, "ignorebadroots") ||
> +	     check_ro_option(info, BTRFS_MOUNT_IGNOREDATACSUMS, "ignoredatacsums")))
> +		return false;
> +
> +	if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE) &&
> +	    !btrfs_test_opt(info, FREE_SPACE_TREE) &&
> +	    !btrfs_test_opt(info, CLEAR_CACHE)) {
> +		btrfs_err(info, "cannot disable free space tree");
> +		return false;
> +	}
> +	if (btrfs_fs_compat_ro(info, BLOCK_GROUP_TREE) &&
> +	     !btrfs_test_opt(info, FREE_SPACE_TREE)) {
> +		btrfs_err(info, "cannot disable free space tree with block-group-tree feature");
> +		return false;
> +	}
> +
> +	if (btrfs_check_mountopts_zoned(info))
> +		return false;
> +


<snip>

> -check:
> -	/* We're read-only, don't have to check. */
> -	if (new_flags & SB_RDONLY)
> -		goto out;
> -
> -	if (check_ro_option(info, BTRFS_MOUNT_NOLOGREPLAY, "nologreplay") ||
> -	    check_ro_option(info, BTRFS_MOUNT_IGNOREBADROOTS, "ignorebadroots") ||
> -	    check_ro_option(info, BTRFS_MOUNT_IGNOREDATACSUMS, "ignoredatacsums"))
> -		ret = -EINVAL;
>   out:
> -	if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE) &&
> -	    !btrfs_test_opt(info, FREE_SPACE_TREE) &&
> -	    !btrfs_test_opt(info, CLEAR_CACHE)) {
> -		btrfs_err(info, "cannot disable free space tree");
> +	if (!ret && !check_options(info, new_flags))
>   		ret = -EINVAL;
> -	}
> -	if (btrfs_fs_compat_ro(info, BLOCK_GROUP_TREE) &&
> -	     !btrfs_test_opt(info, FREE_SPACE_TREE)) {
> -		btrfs_err(info, "cannot disable free space tree with block-group-tree feature");
> -		ret = -EINVAL;
> -	}
> -	if (!ret)
> -		ret = btrfs_check_mountopts_zoned(info);
> -	if (!ret && !remounting) {
> -		if (btrfs_test_opt(info, SPACE_CACHE))
> -			btrfs_info(info, "disk space caching is enabled");
> -		if (btrfs_test_opt(info, FREE_SPACE_TREE))
> -			btrfs_info(info, "using free space tree");
> -	}
>   	return ret;
>   }


Before this patch, we verified all the above checks simultaneously.
Now, for each error, we return without checking the rest.
As a result, if there are multiple failures in the above checks,
we report them sequentially. This is not a bug, but the optimization
we had earlier has been traded off for cleaner code.
IMO it is good to keep the optimization.

Thanks, Anand