diff mbox series

[v2,04/18] btrfs: move space cache settings into open_ctree

Message ID c1f4384e79a163e4aef516472a8d6574dc54545d.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
Currently we pre-load the space cache settings in btrfs_parse_options,
however when we switch to the new mount API the mount option parsing
will happen before we have the super block loaded.  Add a helper to set
the appropriate options based on the fs settings, this will allow us to
have consistent free space cache settings.

This also folds in the space cache related decisions we make for subpage
sectorsize support, so all of this is done in one place.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/disk-io.c | 17 ++++++-----------
 fs/btrfs/super.c   | 44 +++++++++++++++++++++++++++++++-------------
 fs/btrfs/super.h   |  1 +
 3 files changed, 38 insertions(+), 24 deletions(-)

Comments

Anand Jain Nov. 15, 2023, 12:06 a.m. UTC | #1
> @@ -3287,6 +3287,12 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>   	fs_info->csums_per_leaf = BTRFS_MAX_ITEM_SIZE(fs_info) / fs_info->csum_size;
>   	fs_info->stripesize = stripesize;
>   
> +	/*
> +	 * Handle the space caching options appropriately now that we have the
> +	 * super loaded and validated.
> +	 */
> +	btrfs_set_free_space_cache_settings(fs_info);
> +
>   	ret = btrfs_parse_options(fs_info, options, sb->s_flags);
>   	if (ret)
>   		goto fail_alloc;
> @@ -3298,17 +3304,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>   	if (sectorsize < PAGE_SIZE) {
>   		struct btrfs_subpage_info *subpage_info;
>   
> -		/*
> -		 * V1 space cache has some hardcoded PAGE_SIZE usage, and is
> -		 * going to be deprecated.
> -		 *
> -		 * Force to use v2 cache for subpage case.
> -		 */
> -		btrfs_clear_opt(fs_info->mount_opt, SPACE_CACHE);
> -		btrfs_set_and_info(fs_info, FREE_SPACE_TREE,
> -			"forcing free space tree for sector size %u with page size %lu",
> -			sectorsize, PAGE_SIZE);
> -
>   		btrfs_warn(fs_info,
>   		"read-write for sector size %u with page size %lu is experimental",
>   			   sectorsize, PAGE_SIZE);
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 639601d346d0..aef7e67538a3 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -266,6 +266,31 @@ static bool check_options(struct btrfs_fs_info *info, unsigned long flags)
>   	return true;
>   }
>   
> +void btrfs_set_free_space_cache_settings(struct btrfs_fs_info *fs_info)
> +{
> +	if (btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE))
> +		btrfs_set_opt(fs_info->mount_opt, FREE_SPACE_TREE);
> +	else if (btrfs_free_space_cache_v1_active(fs_info)) {
> +		if (btrfs_is_zoned(fs_info)) {
> +			btrfs_info(fs_info,
> +			"zoned: clearing existing space cache");
> +			btrfs_set_super_cache_generation(fs_info->super_copy, 0);
> +		} else {
> +			btrfs_set_opt(fs_info->mount_opt, SPACE_CACHE);
> +		}
> +	}
> +
> +	if (fs_info->sectorsize < PAGE_SIZE) {
> +		btrfs_clear_opt(fs_info->mount_opt, SPACE_CACHE);
> +		if (!btrfs_test_opt(fs_info, FREE_SPACE_TREE)) {
> +			btrfs_info(fs_info,
> +				   "forcing free space tree for sector size %u with page size %lu",
> +				   fs_info->sectorsize, PAGE_SIZE);
> +			btrfs_set_opt(fs_info->mount_opt, FREE_SPACE_TREE);
> +		}
> +	}
> +}
> +
>   static int parse_rescue_options(struct btrfs_fs_info *info, const char *options)
>   {
>   	char *opts;
> @@ -345,18 +370,6 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>   	bool saved_compress_force;
>   	int no_compress = 0;
>   
> -	if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE))
> -		btrfs_set_opt(info->mount_opt, FREE_SPACE_TREE);
> -	else if (btrfs_free_space_cache_v1_active(info)) {
> -		if (btrfs_is_zoned(info)) {
> -			btrfs_info(info,
> -			"zoned: clearing existing space cache");
> -			btrfs_set_super_cache_generation(info->super_copy, 0);
> -		} else {
> -			btrfs_set_opt(info->mount_opt, SPACE_CACHE);
> -		}
> -	}
> -
>   	/*
>   	 * Even the options are empty, we still need to do extra check
>   	 * against new flags


btrfs_remount() calls btrfs_parse_options(), which previously handled
space cache/tree flags set/reset. However, with the move of this
functionality to a separate function, btrfs_set_free_space_cache_settings(),
the btrfs_remount() thread no longer has the space cache/tree flags
set/reset. The changelog provides no explanation for this change.
Could this be a bug?

Thanks, Anand
Josef Bacik Nov. 20, 2023, 9:54 p.m. UTC | #2
On Wed, Nov 15, 2023 at 08:06:02AM +0800, Anand Jain wrote:
> 
> > @@ -3287,6 +3287,12 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
> >   	fs_info->csums_per_leaf = BTRFS_MAX_ITEM_SIZE(fs_info) / fs_info->csum_size;
> >   	fs_info->stripesize = stripesize;
> > +	/*
> > +	 * Handle the space caching options appropriately now that we have the
> > +	 * super loaded and validated.
> > +	 */
> > +	btrfs_set_free_space_cache_settings(fs_info);
> > +
> >   	ret = btrfs_parse_options(fs_info, options, sb->s_flags);
> >   	if (ret)
> >   		goto fail_alloc;
> > @@ -3298,17 +3304,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
> >   	if (sectorsize < PAGE_SIZE) {
> >   		struct btrfs_subpage_info *subpage_info;
> > -		/*
> > -		 * V1 space cache has some hardcoded PAGE_SIZE usage, and is
> > -		 * going to be deprecated.
> > -		 *
> > -		 * Force to use v2 cache for subpage case.
> > -		 */
> > -		btrfs_clear_opt(fs_info->mount_opt, SPACE_CACHE);
> > -		btrfs_set_and_info(fs_info, FREE_SPACE_TREE,
> > -			"forcing free space tree for sector size %u with page size %lu",
> > -			sectorsize, PAGE_SIZE);
> > -
> >   		btrfs_warn(fs_info,
> >   		"read-write for sector size %u with page size %lu is experimental",
> >   			   sectorsize, PAGE_SIZE);
> > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > index 639601d346d0..aef7e67538a3 100644
> > --- a/fs/btrfs/super.c
> > +++ b/fs/btrfs/super.c
> > @@ -266,6 +266,31 @@ static bool check_options(struct btrfs_fs_info *info, unsigned long flags)
> >   	return true;
> >   }
> > +void btrfs_set_free_space_cache_settings(struct btrfs_fs_info *fs_info)
> > +{
> > +	if (btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE))
> > +		btrfs_set_opt(fs_info->mount_opt, FREE_SPACE_TREE);
> > +	else if (btrfs_free_space_cache_v1_active(fs_info)) {
> > +		if (btrfs_is_zoned(fs_info)) {
> > +			btrfs_info(fs_info,
> > +			"zoned: clearing existing space cache");
> > +			btrfs_set_super_cache_generation(fs_info->super_copy, 0);
> > +		} else {
> > +			btrfs_set_opt(fs_info->mount_opt, SPACE_CACHE);
> > +		}
> > +	}
> > +
> > +	if (fs_info->sectorsize < PAGE_SIZE) {
> > +		btrfs_clear_opt(fs_info->mount_opt, SPACE_CACHE);
> > +		if (!btrfs_test_opt(fs_info, FREE_SPACE_TREE)) {
> > +			btrfs_info(fs_info,
> > +				   "forcing free space tree for sector size %u with page size %lu",
> > +				   fs_info->sectorsize, PAGE_SIZE);
> > +			btrfs_set_opt(fs_info->mount_opt, FREE_SPACE_TREE);
> > +		}
> > +	}
> > +}
> > +
> >   static int parse_rescue_options(struct btrfs_fs_info *info, const char *options)
> >   {
> >   	char *opts;
> > @@ -345,18 +370,6 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
> >   	bool saved_compress_force;
> >   	int no_compress = 0;
> > -	if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE))
> > -		btrfs_set_opt(info->mount_opt, FREE_SPACE_TREE);
> > -	else if (btrfs_free_space_cache_v1_active(info)) {
> > -		if (btrfs_is_zoned(info)) {
> > -			btrfs_info(info,
> > -			"zoned: clearing existing space cache");
> > -			btrfs_set_super_cache_generation(info->super_copy, 0);
> > -		} else {
> > -			btrfs_set_opt(info->mount_opt, SPACE_CACHE);
> > -		}
> > -	}
> > -
> >   	/*
> >   	 * Even the options are empty, we still need to do extra check
> >   	 * against new flags
> 
> 
> btrfs_remount() calls btrfs_parse_options(), which previously handled
> space cache/tree flags set/reset. However, with the move of this
> functionality to a separate function, btrfs_set_free_space_cache_settings(),
> the btrfs_remount() thread no longer has the space cache/tree flags
> set/reset. The changelog provides no explanation for this change.
> Could this be a bug?
> 

All this bit does is set the mount_opt's based on the current state of the file
system, so if we're mounting with no "-o space_cache=*" option, we'll derive it
from wether FREE_SPACE_TREE is set or btrfs_free_space_cache_v1_active() returns
true.

So in your case if we mounted -o nospace_cache then we'll have cleared the
v1_active bit and this won't do anything and we'll get -o nospace_cache again.
The same goes for the free space tree, we're only allowed to change the disk
option for this in certain cases.  And in those cases it'll clear the compat
flag.

But this is definitely subtle, I'll expand the changelog.  Thanks,

Josef
diff mbox series

Patch

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 27bbe0164425..b486cbec492b 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3287,6 +3287,12 @@  int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	fs_info->csums_per_leaf = BTRFS_MAX_ITEM_SIZE(fs_info) / fs_info->csum_size;
 	fs_info->stripesize = stripesize;
 
+	/*
+	 * Handle the space caching options appropriately now that we have the
+	 * super loaded and validated.
+	 */
+	btrfs_set_free_space_cache_settings(fs_info);
+
 	ret = btrfs_parse_options(fs_info, options, sb->s_flags);
 	if (ret)
 		goto fail_alloc;
@@ -3298,17 +3304,6 @@  int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	if (sectorsize < PAGE_SIZE) {
 		struct btrfs_subpage_info *subpage_info;
 
-		/*
-		 * V1 space cache has some hardcoded PAGE_SIZE usage, and is
-		 * going to be deprecated.
-		 *
-		 * Force to use v2 cache for subpage case.
-		 */
-		btrfs_clear_opt(fs_info->mount_opt, SPACE_CACHE);
-		btrfs_set_and_info(fs_info, FREE_SPACE_TREE,
-			"forcing free space tree for sector size %u with page size %lu",
-			sectorsize, PAGE_SIZE);
-
 		btrfs_warn(fs_info,
 		"read-write for sector size %u with page size %lu is experimental",
 			   sectorsize, PAGE_SIZE);
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 639601d346d0..aef7e67538a3 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -266,6 +266,31 @@  static bool check_options(struct btrfs_fs_info *info, unsigned long flags)
 	return true;
 }
 
+void btrfs_set_free_space_cache_settings(struct btrfs_fs_info *fs_info)
+{
+	if (btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE))
+		btrfs_set_opt(fs_info->mount_opt, FREE_SPACE_TREE);
+	else if (btrfs_free_space_cache_v1_active(fs_info)) {
+		if (btrfs_is_zoned(fs_info)) {
+			btrfs_info(fs_info,
+			"zoned: clearing existing space cache");
+			btrfs_set_super_cache_generation(fs_info->super_copy, 0);
+		} else {
+			btrfs_set_opt(fs_info->mount_opt, SPACE_CACHE);
+		}
+	}
+
+	if (fs_info->sectorsize < PAGE_SIZE) {
+		btrfs_clear_opt(fs_info->mount_opt, SPACE_CACHE);
+		if (!btrfs_test_opt(fs_info, FREE_SPACE_TREE)) {
+			btrfs_info(fs_info,
+				   "forcing free space tree for sector size %u with page size %lu",
+				   fs_info->sectorsize, PAGE_SIZE);
+			btrfs_set_opt(fs_info->mount_opt, FREE_SPACE_TREE);
+		}
+	}
+}
+
 static int parse_rescue_options(struct btrfs_fs_info *info, const char *options)
 {
 	char *opts;
@@ -345,18 +370,6 @@  int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 	bool saved_compress_force;
 	int no_compress = 0;
 
-	if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE))
-		btrfs_set_opt(info->mount_opt, FREE_SPACE_TREE);
-	else if (btrfs_free_space_cache_v1_active(info)) {
-		if (btrfs_is_zoned(info)) {
-			btrfs_info(info,
-			"zoned: clearing existing space cache");
-			btrfs_set_super_cache_generation(info->super_copy, 0);
-		} else {
-			btrfs_set_opt(info->mount_opt, SPACE_CACHE);
-		}
-	}
-
 	/*
 	 * Even the options are empty, we still need to do extra check
 	 * against new flags
@@ -649,8 +662,13 @@  int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			 * compat_ro(FREE_SPACE_TREE) set, and we aren't going
 			 * to allow v1 to be set for extent tree v2, simply
 			 * ignore this setting if we're extent tree v2.
+			 *
+			 * For subpage blocksize we don't allow space cache v1,
+			 * and we'll turn on v2, so we can skip the settings
+			 * here as well.
 			 */
-			if (btrfs_fs_incompat(info, EXTENT_TREE_V2))
+			if (btrfs_fs_incompat(info, EXTENT_TREE_V2) ||
+			    info->sectorsize < PAGE_SIZE)
 				break;
 			if (token == Opt_space_cache ||
 			    strcmp(args[0].from, "v1") == 0) {
diff --git a/fs/btrfs/super.h b/fs/btrfs/super.h
index 8dbb909b364f..7c1cd7527e76 100644
--- a/fs/btrfs/super.h
+++ b/fs/btrfs/super.h
@@ -8,6 +8,7 @@  int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 int btrfs_sync_fs(struct super_block *sb, int wait);
 char *btrfs_get_subvol_name_from_objectid(struct btrfs_fs_info *fs_info,
 					  u64 subvol_objectid);
+void btrfs_set_free_space_cache_settings(struct btrfs_fs_info *fs_info);
 
 static inline struct btrfs_fs_info *btrfs_sb(struct super_block *sb)
 {