[PATCH v2 0/3] btrfs: free space tree mounting fixes

[PATCH v2 0/3] btrfs: free space tree mounting fixes

[Boris Burkov]

A few fixes for issues with mounting the btrfs free space tree
(aka space_cache v2). These are not dependent, and are only related
loosely, in that they all apply to mounting the file system with
the free space tree.

The first patch fixes -o remount,space_cache=v2.

The second patch fixes the slight oversight of not cleaning up the
space cache free space object or free space inodes when migrating to
the free space tree.

The third patch stops re-creating the free space objects when we
are not using space_cache=v1.

changes for v2:
Patch 1/3: made remount _only_ work in ro->rw case, added comment.
Patch 2/3: added btrfs_ prefix to non-static function, removed bad
           whitespace.
Patch 3/3: new in v2, was part of Patch 2/2 in v1.

Boris Burkov (3):
  btrfs: support remount of ro fs with free space tree
  btrfs: remove free space items when creating free space tree
  btrfs: skip space_cache v1 setup when not using it

 fs/btrfs/block-group.c      | 42 ++++----------------------------
 fs/btrfs/free-space-cache.c | 48 +++++++++++++++++++++++++++++++++++++
 fs/btrfs/free-space-cache.h |  2 ++
 fs/btrfs/free-space-tree.c  |  3 +++
 fs/btrfs/super.c            | 29 ++++++++++++++++++++++
 5 files changed, 87 insertions(+), 37 deletions(-)

-- 
2.24.1

[PATCH v2 1/3] btrfs: support remount of ro fs with free space tree

[Boris Burkov]

When a user attempts to remount a btrfs filesystem with
'mount -o remount,space_cache=v2', that operation succeeds.
Unfortunately, this is misleading, because the remount does not create
the free space tree. /proc/mounts will incorrectly show space_cache=v2,
but on the next mount, the file system will revert to the old
space_cache.

For now, we handle only the easier case, where the existing mount is
read only. In that case, we can create the free space tree without
contending with the block groups changing as we go. If it is not read
only, we fail more explicitly so the user knows that the remount was not
successful, and we don't end up in a state where /proc/mounts is giving
misleading information. We also fail if the remount is read-only, since
we would not be able to create the free space tree in that case.

References: https://github.com/btrfs/btrfs-todo/issues/5
Signed-off-by: Boris Burkov <boris@bur.io>
---
v2:
- move creation down to ro->rw case
- error on all other remount cases
- add a comment to help future remount modifiers

 fs/btrfs/super.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 3ebe7240c63d..0a1b5f554c27 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -47,6 +47,7 @@
 #include "tests/btrfs-tests.h"
 #include "block-group.h"
 #include "discard.h"
+#include "free-space-tree.h"
 
 #include "qgroup.h"
 #define CREATE_TRACE_POINTS
@@ -1838,6 +1839,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 	u64 old_max_inline = fs_info->max_inline;
 	u32 old_thread_pool_size = fs_info->thread_pool_size;
 	u32 old_metadata_ratio = fs_info->metadata_ratio;
+	bool create_fst = false;
 	int ret;
 
 	sync_filesystem(sb);
@@ -1862,6 +1864,17 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 	btrfs_resize_thread_pool(fs_info,
 		fs_info->thread_pool_size, old_thread_pool_size);
 
+	if (btrfs_test_opt(fs_info, FREE_SPACE_TREE) &&
+	    !btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE)) {
+		create_fst = true;
+		if(!sb_rdonly(sb) || *flags & SB_RDONLY) {
+			btrfs_warn(fs_info,
+				   "Remounting with free space tree only allowed from read-only to read-write");
+			ret = -EINVAL;
+			goto restore;
+		}
+	}
+
 	if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
 		goto out;
 
@@ -1924,6 +1937,22 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 			goto restore;
 		}
 
+		/*
+		 * NOTE: when remounting with a change that does writes, don't
+		 * put it anywhere above this point, as we are not sure to be
+		 * safe to write until we pass the above checks.
+		 */
+		if (create_fst) {
+			btrfs_info(fs_info, "creating free space tree");
+			ret = btrfs_create_free_space_tree(fs_info);
+			if (ret) {
+				btrfs_warn(fs_info,
+					   "failed to create free space tree: %d", ret);
+				goto restore;
+			}
+		}
+
+
 		ret = btrfs_cleanup_fs_roots(fs_info);
 		if (ret)
 			goto restore;
-- 
2.24.1

[PATCH v2 2/3] btrfs: remove free space items when creating free space tree

[Boris Burkov]

When the file system transitions from space cache v1 to v2 it removes
the old cached data, but does not remove the FREE_SPACE items nor the
free space inodes they point to. This doesn't cause any issues besides
being a bit inefficient, since these items no longer do anything useful.

To fix it, as part of populating the free space tree, destroy each block
group's free space item and free space inode. This code is lifted from
the existing code for removing them when removing the block group.

References: https://github.com/btrfs/btrfs-todo/issues/5
Signed-off-by: Boris Burkov <boris@bur.io>
---
v2:
- remove_free_space_inode -> btrfs_remove_free_space_inode
- undo sinful whitespace change

 fs/btrfs/block-group.c      | 39 ++----------------------------
 fs/btrfs/free-space-cache.c | 48 +++++++++++++++++++++++++++++++++++++
 fs/btrfs/free-space-cache.h |  2 ++
 fs/btrfs/free-space-tree.c  |  3 +++
 4 files changed, 55 insertions(+), 37 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 01e8ba1da1d3..e5e5baad88d8 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -892,8 +892,6 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
 	struct btrfs_path *path;
 	struct btrfs_block_group *block_group;
 	struct btrfs_free_cluster *cluster;
-	struct btrfs_root *tree_root = fs_info->tree_root;
-	struct btrfs_key key;
 	struct inode *inode;
 	struct kobject *kobj = NULL;
 	int ret;
@@ -971,42 +969,9 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
 	spin_unlock(&trans->transaction->dirty_bgs_lock);
 	mutex_unlock(&trans->transaction->cache_write_mutex);
 
-	if (!IS_ERR(inode)) {
-		ret = btrfs_orphan_add(trans, BTRFS_I(inode));
-		if (ret) {
-			btrfs_add_delayed_iput(inode);
-			goto out;
-		}
-		clear_nlink(inode);
-		/* One for the block groups ref */
-		spin_lock(&block_group->lock);
-		if (block_group->iref) {
-			block_group->iref = 0;
-			block_group->inode = NULL;
-			spin_unlock(&block_group->lock);
-			iput(inode);
-		} else {
-			spin_unlock(&block_group->lock);
-		}
-		/* One for our lookup ref */
-		btrfs_add_delayed_iput(inode);
-	}
-
-	key.objectid = BTRFS_FREE_SPACE_OBJECTID;
-	key.type = 0;
-	key.offset = block_group->start;
-
-	ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1);
-	if (ret < 0)
+	ret = btrfs_remove_free_space_inode(trans, block_group);
+	if (ret)
 		goto out;
-	if (ret > 0)
-		btrfs_release_path(path);
-	if (ret == 0) {
-		ret = btrfs_del_item(trans, tree_root, path);
-		if (ret)
-			goto out;
-		btrfs_release_path(path);
-	}
 
 	spin_lock(&fs_info->block_group_cache_lock);
 	rb_erase(&block_group->cache_node,
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 8759f5a1d6a0..da6d436c58fa 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -207,6 +207,54 @@ int create_free_space_inode(struct btrfs_trans_handle *trans,
 					 ino, block_group->start);
 }
 
+int btrfs_remove_free_space_inode(struct btrfs_trans_handle *trans,
+				  struct btrfs_block_group *block_group)
+{
+	struct btrfs_path *path;
+	struct inode *inode;
+	struct btrfs_key key;
+	int ret = 0;
+	path = btrfs_alloc_path();
+	if (!path)
+		return -ENOMEM;
+
+	inode = lookup_free_space_inode(block_group, path);
+	if (IS_ERR(inode)) {
+		if (PTR_ERR(inode) != -ENOENT)
+			ret = PTR_ERR(inode);
+		goto out;
+	}
+	ret = btrfs_orphan_add(trans, BTRFS_I(inode));
+	if (ret) {
+		btrfs_add_delayed_iput(inode);
+		goto out;
+	}
+	clear_nlink(inode);
+	/* One for the block groups ref */
+	spin_lock(&block_group->lock);
+	if (block_group->iref) {
+		block_group->iref = 0;
+		block_group->inode = NULL;
+		spin_unlock(&block_group->lock);
+		iput(inode);
+	} else {
+		spin_unlock(&block_group->lock);
+	}
+	/* One for our lookup ref */
+	btrfs_add_delayed_iput(inode);
+
+	key.objectid = BTRFS_FREE_SPACE_OBJECTID;
+	key.type = 0;
+	key.offset = block_group->start;
+	ret = btrfs_search_slot(trans, trans->fs_info->tree_root, &key, path, -1, 1);
+	if (ret != 0)
+		goto out;
+	ret = btrfs_del_item(trans, trans->fs_info->tree_root, path);
+out:
+	btrfs_free_path(path);
+	return ret;
+}
+
 int btrfs_check_trunc_cache_free_space(struct btrfs_fs_info *fs_info,
 				       struct btrfs_block_rsv *rsv)
 {
diff --git a/fs/btrfs/free-space-cache.h b/fs/btrfs/free-space-cache.h
index e3d5e0ad8f8e..606eca93e43f 100644
--- a/fs/btrfs/free-space-cache.h
+++ b/fs/btrfs/free-space-cache.h
@@ -84,6 +84,8 @@ struct inode *lookup_free_space_inode(struct btrfs_block_group *block_group,
 int create_free_space_inode(struct btrfs_trans_handle *trans,
 			    struct btrfs_block_group *block_group,
 			    struct btrfs_path *path);
+int btrfs_remove_free_space_inode(struct btrfs_trans_handle *trans,
+				  struct btrfs_block_group *block_group);
 
 int btrfs_check_trunc_cache_free_space(struct btrfs_fs_info *fs_info,
 				       struct btrfs_block_rsv *rsv);
diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index 6b9faf3b0e96..7b6be89c1862 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -1165,6 +1165,9 @@ int btrfs_create_free_space_tree(struct btrfs_fs_info *fs_info)
 		block_group = rb_entry(node, struct btrfs_block_group,
 				       cache_node);
 		ret = populate_free_space_tree(trans, block_group);
+		if (ret)
+			goto abort;
+		ret = btrfs_remove_free_space_inode(trans, block_group);
 		if (ret)
 			goto abort;
 		node = rb_next(node);
-- 
2.24.1

[PATCH 3/3] btrfs: skip space_cache v1 setup when not using it

[Boris Burkov]

If we are not using space cache v1, we should not create the free space
object or free space inodes. This comes up when we delete the existing
free space objects/inodes when migrating to v2, only to see them get
recreated for every dirtied block group.

Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/block-group.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index e5e5baad88d8..b3502a887978 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -2308,6 +2308,9 @@ static int cache_save_setup(struct btrfs_block_group *block_group,
 	int retries = 0;
 	int ret = 0;
 
+	if (!btrfs_test_opt(fs_info, SPACE_CACHE))
+		return 0;
+
 	/*
 	 * If this block group is smaller than 100 megs don't bother caching the
 	 * block group.
-- 
2.24.1

Re: [PATCH v2 1/3] btrfs: support remount of ro fs with free space tree

[Josef Bacik]

On 9/9/20 5:45 PM, Boris Burkov wrote:
> When a user attempts to remount a btrfs filesystem with
> 'mount -o remount,space_cache=v2', that operation succeeds.
> Unfortunately, this is misleading, because the remount does not create
> the free space tree. /proc/mounts will incorrectly show space_cache=v2,
> but on the next mount, the file system will revert to the old
> space_cache.
> 
> For now, we handle only the easier case, where the existing mount is
> read only. In that case, we can create the free space tree without
> contending with the block groups changing as we go. If it is not read
> only, we fail more explicitly so the user knows that the remount was not
> successful, and we don't end up in a state where /proc/mounts is giving
> misleading information. We also fail if the remount is read-only, since
> we would not be able to create the free space tree in that case.
> 
> References: https://github.com/btrfs/btrfs-todo/issues/5
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
> v2:
> - move creation down to ro->rw case
> - error on all other remount cases
> - add a comment to help future remount modifiers
> 
>   fs/btrfs/super.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 3ebe7240c63d..0a1b5f554c27 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -47,6 +47,7 @@
>   #include "tests/btrfs-tests.h"
>   #include "block-group.h"
>   #include "discard.h"
> +#include "free-space-tree.h"
>   
>   #include "qgroup.h"
>   #define CREATE_TRACE_POINTS
> @@ -1838,6 +1839,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
>   	u64 old_max_inline = fs_info->max_inline;
>   	u32 old_thread_pool_size = fs_info->thread_pool_size;
>   	u32 old_metadata_ratio = fs_info->metadata_ratio;
> +	bool create_fst = false;
>   	int ret;
>   
>   	sync_filesystem(sb);
> @@ -1862,6 +1864,17 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
>   	btrfs_resize_thread_pool(fs_info,
>   		fs_info->thread_pool_size, old_thread_pool_size);
>   
> +	if (btrfs_test_opt(fs_info, FREE_SPACE_TREE) &&
> +	    !btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE)) {
> +		create_fst = true;
> +		if(!sb_rdonly(sb) || *flags & SB_RDONLY) {
> +			btrfs_warn(fs_info,
> +				   "Remounting with free space tree only allowed from read-only to read-write");
> +			ret = -EINVAL;
> +			goto restore;
> +		}
> +	}

This will bite us if we remount -o ro,noatime but had previous mounted with -o 
ro,space_cache=v2.  These checks need to be under

> +
>   	if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
>   		goto out;
>   

This part right here.  Put the check for remounting RO with space_cache=v2 in 
that part, and the check if we need to create the fst at all down where you 
create it.  Thanks,

Josef

Re: [PATCH v2 2/3] btrfs: remove free space items when creating free space tree

[Josef Bacik]

On 9/9/20 5:45 PM, Boris Burkov wrote:
> When the file system transitions from space cache v1 to v2 it removes
> the old cached data, but does not remove the FREE_SPACE items nor the
> free space inodes they point to. This doesn't cause any issues besides
> being a bit inefficient, since these items no longer do anything useful.
> 
> To fix it, as part of populating the free space tree, destroy each block
> group's free space item and free space inode. This code is lifted from
> the existing code for removing them when removing the block group.
> 
> References: https://github.com/btrfs/btrfs-todo/issues/5
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
> v2:
> - remove_free_space_inode -> btrfs_remove_free_space_inode
> - undo sinful whitespace change
> 
>   fs/btrfs/block-group.c      | 39 ++----------------------------
>   fs/btrfs/free-space-cache.c | 48 +++++++++++++++++++++++++++++++++++++
>   fs/btrfs/free-space-cache.h |  2 ++
>   fs/btrfs/free-space-tree.c  |  3 +++
>   4 files changed, 55 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 01e8ba1da1d3..e5e5baad88d8 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -892,8 +892,6 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
>   	struct btrfs_path *path;
>   	struct btrfs_block_group *block_group;
>   	struct btrfs_free_cluster *cluster;
> -	struct btrfs_root *tree_root = fs_info->tree_root;
> -	struct btrfs_key key;
>   	struct inode *inode;
>   	struct kobject *kobj = NULL;
>   	int ret;
> @@ -971,42 +969,9 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
>   	spin_unlock(&trans->transaction->dirty_bgs_lock);
>   	mutex_unlock(&trans->transaction->cache_write_mutex);
>   
> -	if (!IS_ERR(inode)) {
> -		ret = btrfs_orphan_add(trans, BTRFS_I(inode));
> -		if (ret) {
> -			btrfs_add_delayed_iput(inode);
> -			goto out;
> -		}
> -		clear_nlink(inode);
> -		/* One for the block groups ref */
> -		spin_lock(&block_group->lock);
> -		if (block_group->iref) {
> -			block_group->iref = 0;
> -			block_group->inode = NULL;
> -			spin_unlock(&block_group->lock);
> -			iput(inode);
> -		} else {
> -			spin_unlock(&block_group->lock);
> -		}
> -		/* One for our lookup ref */
> -		btrfs_add_delayed_iput(inode);
> -	}
> -
> -	key.objectid = BTRFS_FREE_SPACE_OBJECTID;
> -	key.type = 0;
> -	key.offset = block_group->start;
> -
> -	ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1);
> -	if (ret < 0)
> +	ret = btrfs_remove_free_space_inode(trans, block_group);
> +	if (ret)
>   		goto out;
> -	if (ret > 0)
> -		btrfs_release_path(path);
> -	if (ret == 0) {
> -		ret = btrfs_del_item(trans, tree_root, path);
> -		if (ret)
> -			goto out;
> -		btrfs_release_path(path);
> -	}
>   
>   	spin_lock(&fs_info->block_group_cache_lock);
>   	rb_erase(&block_group->cache_node,
> diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
> index 8759f5a1d6a0..da6d436c58fa 100644
> --- a/fs/btrfs/free-space-cache.c
> +++ b/fs/btrfs/free-space-cache.c
> @@ -207,6 +207,54 @@ int create_free_space_inode(struct btrfs_trans_handle *trans,
>   					 ino, block_group->start);
>   }
>   
> +int btrfs_remove_free_space_inode(struct btrfs_trans_handle *trans,
> +				  struct btrfs_block_group *block_group)
> +{
> +	struct btrfs_path *path;
> +	struct inode *inode;
> +	struct btrfs_key key;
> +	int ret = 0;
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return -ENOMEM;
> +
> +	inode = lookup_free_space_inode(block_group, path);
> +	if (IS_ERR(inode)) {
> +		if (PTR_ERR(inode) != -ENOENT)
> +			ret = PTR_ERR(inode);
> +		goto out;
> +	}
> +	ret = btrfs_orphan_add(trans, BTRFS_I(inode));
> +	if (ret) {
> +		btrfs_add_delayed_iput(inode);
> +		goto out;
> +	}
> +	clear_nlink(inode);
> +	/* One for the block groups ref */
> +	spin_lock(&block_group->lock);
> +	if (block_group->iref) {
> +		block_group->iref = 0;
> +		block_group->inode = NULL;
> +		spin_unlock(&block_group->lock);
> +		iput(inode);
> +	} else {
> +		spin_unlock(&block_group->lock);
> +	}
> +	/* One for our lookup ref */
> +	btrfs_add_delayed_iput(inode);
> +
> +	key.objectid = BTRFS_FREE_SPACE_OBJECTID;
> +	key.type = 0;
> +	key.offset = block_group->start;
> +	ret = btrfs_search_slot(trans, trans->fs_info->tree_root, &key, path, -1, 1);
> +	if (ret != 0)
> +		goto out;

This leaks the ret == 1 if we don't have a free space cache objectid.  This 
needs to be

if (ret) {
	if (ret > 0)
		ret = 0;
	goto out;
}

or something like that.  Thanks,

Josef

Re: [PATCH 3/3] btrfs: skip space_cache v1 setup when not using it

[Josef Bacik]

On 9/9/20 5:45 PM, Boris Burkov wrote:
> If we are not using space cache v1, we should not create the free space
> object or free space inodes. This comes up when we delete the existing
> free space objects/inodes when migrating to v2, only to see them get
> recreated for every dirtied block group.
> 
> Signed-off-by: Boris Burkov <boris@bur.io>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH v2 2/3] btrfs: remove free space items when creating free space tree

[Josef Bacik]

On 9/9/20 5:45 PM, Boris Burkov wrote:
> When the file system transitions from space cache v1 to v2 it removes
> the old cached data, but does not remove the FREE_SPACE items nor the
> free space inodes they point to. This doesn't cause any issues besides
> being a bit inefficient, since these items no longer do anything useful.
> 
> To fix it, as part of populating the free space tree, destroy each block
> group's free space item and free space inode. This code is lifted from
> the existing code for removing them when removing the block group.
> 
> References: https://github.com/btrfs/btrfs-todo/issues/5
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
> v2:
> - remove_free_space_inode -> btrfs_remove_free_space_inode
> - undo sinful whitespace change
> 
>   fs/btrfs/block-group.c      | 39 ++----------------------------
>   fs/btrfs/free-space-cache.c | 48 +++++++++++++++++++++++++++++++++++++
>   fs/btrfs/free-space-cache.h |  2 ++
>   fs/btrfs/free-space-tree.c  |  3 +++
>   4 files changed, 55 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 01e8ba1da1d3..e5e5baad88d8 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -892,8 +892,6 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
>   	struct btrfs_path *path;
>   	struct btrfs_block_group *block_group;
>   	struct btrfs_free_cluster *cluster;
> -	struct btrfs_root *tree_root = fs_info->tree_root;
> -	struct btrfs_key key;
>   	struct inode *inode;
>   	struct kobject *kobj = NULL;
>   	int ret;
> @@ -971,42 +969,9 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
>   	spin_unlock(&trans->transaction->dirty_bgs_lock);
>   	mutex_unlock(&trans->transaction->cache_write_mutex);
>   
> -	if (!IS_ERR(inode)) {
> -		ret = btrfs_orphan_add(trans, BTRFS_I(inode));
> -		if (ret) {
> -			btrfs_add_delayed_iput(inode);
> -			goto out;
> -		}
> -		clear_nlink(inode);
> -		/* One for the block groups ref */
> -		spin_lock(&block_group->lock);
> -		if (block_group->iref) {
> -			block_group->iref = 0;
> -			block_group->inode = NULL;
> -			spin_unlock(&block_group->lock);
> -			iput(inode);
> -		} else {
> -			spin_unlock(&block_group->lock);
> -		}
> -		/* One for our lookup ref */
> -		btrfs_add_delayed_iput(inode);
> -	}
> -
> -	key.objectid = BTRFS_FREE_SPACE_OBJECTID;
> -	key.type = 0;
> -	key.offset = block_group->start;
> -
> -	ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1);
> -	if (ret < 0)
> +	ret = btrfs_remove_free_space_inode(trans, block_group);
> +	if (ret)
>   		goto out;
> -	if (ret > 0)
> -		btrfs_release_path(path);
> -	if (ret == 0) {
> -		ret = btrfs_del_item(trans, tree_root, path);
> -		if (ret)
> -			goto out;
> -		btrfs_release_path(path);
> -	}
>   

Dave also noticed we're leaking the inode with these patches, that's because 
here you do the remove_free_space_inode thing instead, which is good, but we've 
already done our own lookup, and we're no longer dropping our reference here.  I 
would probalby make btrfs_remove_free_space_inode() take the inode itself so you 
don't have to do two lookups, and then add the lookup to create_free_space_tree 
in a helper that deal's with this.  Then btrfs_remove_free_space_inode() _only_ 
drops the ref for ->iref, and then the caller has to drop their own ref from 
their lookup.  Thanks,

Josef

Re: [PATCH v2 1/3] btrfs: support remount of ro fs with free space tree

[Boris Burkov]

On Thu, Sep 10, 2020 at 10:05:40AM -0400, Josef Bacik wrote:
> On 9/9/20 5:45 PM, Boris Burkov wrote:
> >When a user attempts to remount a btrfs filesystem with
> >'mount -o remount,space_cache=v2', that operation succeeds.
> >Unfortunately, this is misleading, because the remount does not create
> >the free space tree. /proc/mounts will incorrectly show space_cache=v2,
> >but on the next mount, the file system will revert to the old
> >space_cache.
> >
> >For now, we handle only the easier case, where the existing mount is
> >read only. In that case, we can create the free space tree without
> >contending with the block groups changing as we go. If it is not read
> >only, we fail more explicitly so the user knows that the remount was not
> >successful, and we don't end up in a state where /proc/mounts is giving
> >misleading information. We also fail if the remount is read-only, since
> >we would not be able to create the free space tree in that case.
> >
> >References: https://github.com/btrfs/btrfs-todo/issues/5
> >Signed-off-by: Boris Burkov <boris@bur.io>
> >---
> >v2:
> >- move creation down to ro->rw case
> >- error on all other remount cases
> >- add a comment to help future remount modifiers
> >
> >  fs/btrfs/super.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> >diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> >index 3ebe7240c63d..0a1b5f554c27 100644
> >--- a/fs/btrfs/super.c
> >+++ b/fs/btrfs/super.c
> >@@ -47,6 +47,7 @@
> >  #include "tests/btrfs-tests.h"
> >  #include "block-group.h"
> >  #include "discard.h"
> >+#include "free-space-tree.h"
> >  #include "qgroup.h"
> >  #define CREATE_TRACE_POINTS
> >@@ -1838,6 +1839,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
> >  	u64 old_max_inline = fs_info->max_inline;
> >  	u32 old_thread_pool_size = fs_info->thread_pool_size;
> >  	u32 old_metadata_ratio = fs_info->metadata_ratio;
> >+	bool create_fst = false;
> >  	int ret;
> >  	sync_filesystem(sb);
> >@@ -1862,6 +1864,17 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
> >  	btrfs_resize_thread_pool(fs_info,
> >  		fs_info->thread_pool_size, old_thread_pool_size);
> >+	if (btrfs_test_opt(fs_info, FREE_SPACE_TREE) &&
> >+	    !btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE)) {
> >+		create_fst = true;
> >+		if(!sb_rdonly(sb) || *flags & SB_RDONLY) {
> >+			btrfs_warn(fs_info,
> >+				   "Remounting with free space tree only allowed from read-only to read-write");
> >+			ret = -EINVAL;
> >+			goto restore;
> >+		}
> >+	}
> 
> This will bite us if we remount -o ro,noatime but had previous
> mounted with -o ro,space_cache=v2.  These checks need to be under
> 

I wanted this case to fail because I was thinking of this patch as
creating the invariant: "if remount -o space_cache=v2 succeeds, we
actually created the free space tree". However, that behavior is
inconsistent with the fact that 'mount -o ro,space_cache=v2' does
succeed while failing to create the tree.

I guess my question is would we rather have a ro mount that tries to
create the free space tree fail silently or noisily in both cases?

If we want to allow them to silently fail, I will also send a patch to
change the mount option reporting logic.

Thanks for the review!

> >+
> >  	if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
> >  		goto out;
> 
> This part right here.  Put the check for remounting RO with
> space_cache=v2 in that part, and the check if we need to create the
> fst at all down where you create it.  Thanks,
> 
> Josef