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

Anand Jain
2023-11-14T06:32:35Z

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