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

[David Sterba <dsterba@suse.cz>]

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.