diff mbox series

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

Message ID f30ace3052a298c6536453fed66577c308c72d2f.1699470345.git.josef@toxicpanda.com (mailing list archive)
State New, archived
Headers show
Series btrfs: convert to the new mount API | expand

Commit Message

Josef Bacik Nov. 8, 2023, 7:08 p.m. UTC 
We're going to need to validate mount options after they're all parsed
with the new mount api, split this code out into its own helper so we
can use it when we swap over to the new mount api.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/super.c | 64 ++++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 29 deletions(-)

Comments

Anand Jain Nov. 14, 2023, 6:32 a.m. UTC | #1 
> +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
David Sterba Nov. 14, 2023, 5:35 p.m. UTC | #2 
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.
diff mbox series

Patch

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 6ecf78d09694..639601d346d0 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -233,6 +233,39 @@  static bool check_ro_option(struct btrfs_fs_info *fs_info, unsigned long opt,
 	return false;
 }
 
+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;
+
+	if (!test_bit(BTRFS_FS_STATE_REMOUNTING, &info->fs_state)) {
+		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 true;
+}
+
 static int parse_rescue_options(struct btrfs_fs_info *info, const char *options)
 {
 	char *opts;
@@ -311,7 +344,6 @@  int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 	int saved_compress_level;
 	bool saved_compress_force;
 	int no_compress = 0;
-	const bool remounting = test_bit(BTRFS_FS_STATE_REMOUNTING, &info->fs_state);
 
 	if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE))
 		btrfs_set_opt(info->mount_opt, FREE_SPACE_TREE);
@@ -330,7 +362,7 @@  int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 	 * against new flags
 	 */
 	if (!options)
-		goto check;
+		goto out;
 
 	while ((p = strsep(&options, ",")) != NULL) {
 		int token;
@@ -774,35 +806,9 @@  int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			break;
 		}
 	}
-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;
 }