On Tue, Nov 14, 2023 at 02:32:16PM +0800, Anand Jain wrote:
>
> > +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.
I would not say it's cleaner code, it's doing something else than
before. I think we should keep the behaviour 1:1 unless there's a reason
for that, the changelog does not say.
