[PATCH v3 0/4] btrfs: free space tree mounting fixes

[PATCH v3 0/4] 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 /proc/mounts with regards to the space_cache
options (space_cache, space_cache=v2, nospace_cache)

The third 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 fourth patch stops re-creating the free space objects when we
are not using space_cache=v1.

changes for v3:
Patch 1/4: Change failure to warning logging.
Patch 2/4: New; fixes mount option printing.
Patch 3/4: Fix orphan inode vs. delayed iput bug, change remove function
           to take inode as a sink.
Patch 4/4: No changes.

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.

Boris Burkov (4):
  btrfs: support remount of ro fs with free space tree
  btrfs: use sb state to print space_cache mount option
  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/disk-io.c          | 20 ++++++++++
 fs/btrfs/free-space-cache.c | 78 +++++++++++++++++++++++++++++++++++++
 fs/btrfs/free-space-cache.h |  5 +++
 fs/btrfs/free-space-tree.c  |  3 ++
 fs/btrfs/super.c            | 42 +++++++++++++++++---
 fs/btrfs/transaction.c      |  2 +
 7 files changed, 150 insertions(+), 42 deletions(-)

-- 
2.24.1

[PATCH v3 1/4] 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 silently 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 and the new mount is read-write. In that case, we can create
the free space tree without contending with the block groups changing
as we go. If the remount is ro->ro, rw->ro, or rw->rw, we will not
create the free space tree, and print a warning to dmesg so that this
failure is more visible.

References: https://github.com/btrfs/btrfs-todo/issues/5
Signed-off-by: Boris Burkov <boris@bur.io>
---
v3:
- change failure to warning in dmesg for consistency
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 | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 3ebe7240c63d..8dfd6089e31d 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,16 @@ 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 supported from read-only to read-write");
+			create_fst = false;
+		}
+	}
+
 	if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
 		goto out;
 
@@ -1924,6 +1936,21 @@ 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) {
+			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 2/4] btrfs: use sb state to print space_cache mount option

[Boris Burkov]

To make the contents of /proc/mounts better match the actual state of
the file system, base the display of the space cache mount options off
the contents of the super block rather than the last mount options
passed in. Since there are many scenarios where the mount will ignore a
space cache option, simply showing the passed in option is misleading.

For example, if we mount with -o remount,space_cache=v2 on a read-write
file system without an existing free space tree, we won't build a free
space tree, but /proc/mounts will read space_cache=v2 (until we mount
again and it goes away)

There is already mount logic based on the super block's cache_generation
and free space tree flag that helps decide a consistent setting for the
space cache options, so we just bring those further to the fore. For
free space tree, the flag is already consistent, so we just switch mount
option display to use it. cache_generation is not always reliably set
correctly, so we ensure that cache_generation > 0 iff the file system
is using space_cache v1. This requires committing a transaction on any
mount which changes whether we are using v1. (v1->nospace_cache, v1->v2,
nospace_cache->v1, v2->v1).

References: https://github.com/btrfs/btrfs-todo/issues/5
Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/disk-io.c          | 11 +++++++++++
 fs/btrfs/free-space-cache.c | 20 ++++++++++++++++++++
 fs/btrfs/free-space-cache.h |  2 ++
 fs/btrfs/super.c            | 15 ++++++++++-----
 fs/btrfs/transaction.c      |  2 ++
 5 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 71beb9493ab4..ade92e93e63f 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3335,6 +3335,17 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 		}
 	}
 
+	if ((bool)btrfs_test_opt(fs_info, SPACE_CACHE) !=
+	    btrfs_free_space_cache_v1_active(fs_info)) {
+		ret = btrfs_update_free_space_cache_v1_active(fs_info);
+		if (ret) {
+			btrfs_warn(fs_info,
+				   "failed to update free space cache status: %d", ret);
+			close_ctree(fs_info);
+			return ret;
+		}
+	}
+
 	down_read(&fs_info->cleanup_work_sem);
 	if ((ret = btrfs_orphan_cleanup(fs_info->fs_root)) ||
 	    (ret = btrfs_orphan_cleanup(fs_info->tree_root))) {
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 8759f5a1d6a0..25420d51039c 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -3995,6 +3995,26 @@ int btrfs_write_out_ino_cache(struct btrfs_root *root,
 	return ret;
 }
 
+bool btrfs_free_space_cache_v1_active(struct btrfs_fs_info *fs_info)
+{
+	return btrfs_super_cache_generation(fs_info->super_copy);
+}
+
+int btrfs_update_free_space_cache_v1_active(struct btrfs_fs_info *fs_info)
+{
+	struct btrfs_trans_handle *trans;
+
+	/*
+	 * update_super_roots will appropriately set
+	 * fs_info->super_copy->cache_generation based on the SPACE_CACHE
+	 * option, so all we have to do is trigger a transaction commit.
+	 */
+	trans = btrfs_start_transaction(fs_info->tree_root, 0);
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
+	return btrfs_commit_transaction(trans);
+}
+
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 /*
  * Use this if you need to make a bitmap or extent entry specifically, it
diff --git a/fs/btrfs/free-space-cache.h b/fs/btrfs/free-space-cache.h
index e3d5e0ad8f8e..5fbdbd2fe740 100644
--- a/fs/btrfs/free-space-cache.h
+++ b/fs/btrfs/free-space-cache.h
@@ -148,6 +148,8 @@ int btrfs_trim_block_group_bitmaps(struct btrfs_block_group *block_group,
 				   u64 *trimmed, u64 start, u64 end, u64 minlen,
 				   u64 maxlen, bool async);
 
+bool btrfs_free_space_cache_v1_active(struct btrfs_fs_info *fs_info);
+int btrfs_update_free_space_cache_v1_active(struct btrfs_fs_info *fs_info);
 /* Support functions for running our sanity tests */
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 int test_add_free_space_entry(struct btrfs_block_group *cache,
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 8dfd6089e31d..3dcb676fc50c 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -512,7 +512,6 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 {
 	substring_t args[MAX_OPT_ARGS];
 	char *p, *num;
-	u64 cache_gen;
 	int intarg;
 	int ret = 0;
 	char *compress_type;
@@ -522,10 +521,9 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 	bool saved_compress_force;
 	int no_compress = 0;
 
-	cache_gen = btrfs_super_cache_generation(info->super_copy);
 	if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE))
 		btrfs_set_opt(info->mount_opt, FREE_SPACE_TREE);
-	else if (cache_gen)
+	else if (btrfs_free_space_cache_v1_active(info))
 		btrfs_set_opt(info->mount_opt, SPACE_CACHE);
 
 	/*
@@ -1430,9 +1428,9 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
 		seq_puts(seq, ",discard=async");
 	if (!(info->sb->s_flags & SB_POSIXACL))
 		seq_puts(seq, ",noacl");
-	if (btrfs_test_opt(info, SPACE_CACHE))
+	if (btrfs_free_space_cache_v1_active(info))
 		seq_puts(seq, ",space_cache");
-	else if (btrfs_test_opt(info, FREE_SPACE_TREE))
+	else if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE))
 		seq_puts(seq, ",space_cache=v2");
 	else
 		seq_puts(seq, ",nospace_cache");
@@ -1870,6 +1868,13 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 		if (!sb_rdonly(sb) || *flags & SB_RDONLY) {
 			btrfs_warn(fs_info,
 				   "Remounting with free space tree only supported from read-only to read-write");
+			/*
+			 * if we aren't building the free space tree, reset
+			 * the space cache options to what they were before
+			 */
+			btrfs_clear_opt(fs_info->mount_opt, FREE_SPACE_TREE);
+			if (btrfs_free_space_cache_v1_active(fs_info))
+				btrfs_set_opt(fs_info->mount_opt, SPACE_CACHE);
 			create_fst = false;
 		}
 	}
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 52ada47aff50..7b4e5d031744 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1761,6 +1761,8 @@ static void update_super_roots(struct btrfs_fs_info *fs_info)
 	super->root_level = root_item->level;
 	if (btrfs_test_opt(fs_info, SPACE_CACHE))
 		super->cache_generation = root_item->generation;
+	else
+		super->cache_generation = 0;
 	if (test_bit(BTRFS_FS_UPDATE_UUID_TREE_GEN, &fs_info->flags))
 		super->uuid_tree_generation = root_item->generation;
 }
-- 
2.24.1

[PATCH v3 3/4] 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>
---
v3:
- pass in optional inode to btrfs_remove_free_space_inode, which fixes
  the bug of not issuing an iput for it in the bg delete case.
- fix bug where the orphan generated by fst creation could not be
  cleaned up, because delayed_iput had an outstanding reference
v2:
- remove_free_space_inode -> btrfs_remove_free_space_inode
- undo sinful whitespace change

 fs/btrfs/block-group.c      | 39 ++-----------------------
 fs/btrfs/disk-io.c          |  9 ++++++
 fs/btrfs/free-space-cache.c | 58 +++++++++++++++++++++++++++++++++++++
 fs/btrfs/free-space-cache.h |  3 ++
 fs/btrfs/free-space-tree.c  |  3 ++
 5 files changed, 75 insertions(+), 37 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 01e8ba1da1d3..717b3435c88e 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, inode, 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/disk-io.c b/fs/btrfs/disk-io.c
index ade92e93e63f..775d29565665 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3333,6 +3333,15 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 			close_ctree(fs_info);
 			return ret;
 		}
+		/*
+		 * Creating the free space tree creates inode orphan items and
+		 * delayed iputs when it deletes the free space inodes. Later in
+		 * open_ctree, we run btrfs_orphan_cleanup which tries to clean
+		 * up the orphan items. However, the outstanding references on
+		 * the inodes from the delayed iputs causes the cleanup to fail.
+		 * To fix it, force going through the delayed iputs here.
+		 */
+		btrfs_run_delayed_iputs(fs_info);
 	}
 
 	if ((bool)btrfs_test_opt(fs_info, SPACE_CACHE) !=
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 25420d51039c..6e1bbe87d734 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -207,6 +207,64 @@ int create_free_space_inode(struct btrfs_trans_handle *trans,
 					 ino, block_group->start);
 }
 
+/*
+ * inode is an optional sink: if it is NULL, btrfs_remove_free_space_inode
+ * handles lookup, otherwise it takes ownership and iputs the inode.
+ * Don't reuse an inode pointer after passing it into this function.
+ */
+int btrfs_remove_free_space_inode(struct btrfs_trans_handle *trans,
+				  struct inode *inode,
+				  struct btrfs_block_group *block_group)
+{
+	struct btrfs_path *path;
+	struct btrfs_key key;
+	int ret = 0;
+	path = btrfs_alloc_path();
+	if (!path)
+		return -ENOMEM;
+
+	if (!inode) {
+		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 the 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) {
+		if (ret > 0)
+			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 5fbdbd2fe740..0c01c53fce82 100644
--- a/fs/btrfs/free-space-cache.h
+++ b/fs/btrfs/free-space-cache.h
@@ -84,6 +84,9 @@ 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 inode *inode,
+				  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..d5926d36bf73 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, NULL, block_group);
 		if (ret)
 			goto abort;
 		node = rb_next(node);
-- 
2.24.1

[PATCH 4/4] 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 717b3435c88e..002b3b3d9a30 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 v3 0/4] btrfs: free space tree mounting fixes

[David Sterba]

On Thu, Sep 17, 2020 at 11:13:37AM -0700, Boris Burkov wrote:
> 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 /proc/mounts with regards to the space_cache
> options (space_cache, space_cache=v2, nospace_cache)
> 
> The third 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 fourth patch stops re-creating the free space objects when we
> are not using space_cache=v1.
> 
> changes for v3:
> Patch 1/4: Change failure to warning logging.
> Patch 2/4: New; fixes mount option printing.
> Patch 3/4: Fix orphan inode vs. delayed iput bug, change remove function
>            to take inode as a sink.
> Patch 4/4: No changes.
> 
> 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.
> 
> Boris Burkov (4):
>   btrfs: support remount of ro fs with free space tree
>   btrfs: use sb state to print space_cache mount option
>   btrfs: remove free space items when creating free space tree
>   btrfs: skip space_cache v1 setup when not using it

This passed fstests so I'll add it to for-next, there are still some
minor coding style issues to fix but nothing that would affect
functionality.

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

[Josef Bacik]

On 9/17/20 2:13 PM, Boris Burkov wrote:
> When a user attempts to remount a btrfs filesystem with
> 'mount -o remount,space_cache=v2', that operation silently 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 and the new mount is read-write. In that case, we can create
> the free space tree without contending with the block groups changing
> as we go. If the remount is ro->ro, rw->ro, or rw->rw, we will not
> create the free space tree, and print a warning to dmesg so that this
> failure is more visible.
> 
> References: https://github.com/btrfs/btrfs-todo/issues/5
> Signed-off-by: Boris Burkov <boris@bur.io>

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

Thanks,

Josef

Re: [PATCH 2/4] btrfs: use sb state to print space_cache mount option

[Josef Bacik]

On 9/17/20 2:13 PM, Boris Burkov wrote:
> To make the contents of /proc/mounts better match the actual state of
> the file system, base the display of the space cache mount options off
> the contents of the super block rather than the last mount options
> passed in. Since there are many scenarios where the mount will ignore a
> space cache option, simply showing the passed in option is misleading.
> 
> For example, if we mount with -o remount,space_cache=v2 on a read-write
> file system without an existing free space tree, we won't build a free
> space tree, but /proc/mounts will read space_cache=v2 (until we mount
> again and it goes away)
> 
> There is already mount logic based on the super block's cache_generation
> and free space tree flag that helps decide a consistent setting for the
> space cache options, so we just bring those further to the fore. For
> free space tree, the flag is already consistent, so we just switch mount
> option display to use it. cache_generation is not always reliably set
> correctly, so we ensure that cache_generation > 0 iff the file system
> is using space_cache v1. This requires committing a transaction on any
> mount which changes whether we are using v1. (v1->nospace_cache, v1->v2,
> nospace_cache->v1, v2->v1).
> 
> References: https://github.com/btrfs/btrfs-todo/issues/5
> Signed-off-by: Boris Burkov <boris@bur.io>

Dave already took this, but next time I'd prefer if we'd keep logical changes 
separate.  So one patch to change /proc/mounts, one patch to deal with clearing 
the free space generation field if we're not using it.

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

Thanks,

Josef

Re: [PATCH v3 3/4] btrfs: remove free space items when creating free space tree

[Josef Bacik]

On 9/17/20 2:13 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>

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

Thanks,

Josef

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

[Josef Bacik]

On 9/17/20 2:13 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 2/4] btrfs: use sb state to print space_cache mount option

[David Sterba]

On Mon, Sep 21, 2020 at 10:50:25AM -0400, Josef Bacik wrote:
> On 9/17/20 2:13 PM, Boris Burkov wrote:
> > To make the contents of /proc/mounts better match the actual state of
> > the file system, base the display of the space cache mount options off
> > the contents of the super block rather than the last mount options
> > passed in. Since there are many scenarios where the mount will ignore a
> > space cache option, simply showing the passed in option is misleading.
> > 
> > For example, if we mount with -o remount,space_cache=v2 on a read-write
> > file system without an existing free space tree, we won't build a free
> > space tree, but /proc/mounts will read space_cache=v2 (until we mount
> > again and it goes away)
> > 
> > There is already mount logic based on the super block's cache_generation
> > and free space tree flag that helps decide a consistent setting for the
> > space cache options, so we just bring those further to the fore. For
> > free space tree, the flag is already consistent, so we just switch mount
> > option display to use it. cache_generation is not always reliably set
> > correctly, so we ensure that cache_generation > 0 iff the file system
> > is using space_cache v1. This requires committing a transaction on any
> > mount which changes whether we are using v1. (v1->nospace_cache, v1->v2,
> > nospace_cache->v1, v2->v1).
> > 
> > References: https://github.com/btrfs/btrfs-todo/issues/5
> > Signed-off-by: Boris Burkov <boris@bur.io>
> 
> Dave already took this, but next time I'd prefer if we'd keep logical changes 
> separate.  So one patch to change /proc/mounts, one patch to deal with clearing 
> the free space generation field if we're not using it.

I haven't taken it yet, adding branches to for-next is only to get test
coverage, by 'taken' you can count addig it to misc-next.

Re: [PATCH v3 3/4] btrfs: remove free space items when creating free space tree

[David Sterba]

On Thu, Sep 17, 2020 at 11:13:40AM -0700, Boris Burkov wrote:
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3333,6 +3333,15 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>  			close_ctree(fs_info);
>  			return ret;
>  		}
> +		/*
> +		 * Creating the free space tree creates inode orphan items and
> +		 * delayed iputs when it deletes the free space inodes. Later in
> +		 * open_ctree, we run btrfs_orphan_cleanup which tries to clean
> +		 * up the orphan items. However, the outstanding references on
> +		 * the inodes from the delayed iputs causes the cleanup to fail.
> +		 * To fix it, force going through the delayed iputs here.
> +		 */
> +		btrfs_run_delayed_iputs(fs_info);

This is called from open_ctree, so this is mount context and the free
space tree creation is called before that. That will schedule all free
space inodes for deletion and waits here. This takes time proportional
to the filesystem size.

We've had reports that this takes a lot of time already, so I wonder if
the delayed iputs can be avoided here.

Re: [PATCH 2/4] btrfs: use sb state to print space_cache mount option

[Boris Burkov]

On Mon, Sep 21, 2020 at 07:04:05PM +0200, David Sterba wrote:
> On Mon, Sep 21, 2020 at 10:50:25AM -0400, Josef Bacik wrote:
> > On 9/17/20 2:13 PM, Boris Burkov wrote:
> > > To make the contents of /proc/mounts better match the actual state of
> > > the file system, base the display of the space cache mount options off
> > > the contents of the super block rather than the last mount options
> > > passed in. Since there are many scenarios where the mount will ignore a
> > > space cache option, simply showing the passed in option is misleading.
> > > 
> > > For example, if we mount with -o remount,space_cache=v2 on a read-write
> > > file system without an existing free space tree, we won't build a free
> > > space tree, but /proc/mounts will read space_cache=v2 (until we mount
> > > again and it goes away)
> > > 
> > > There is already mount logic based on the super block's cache_generation
> > > and free space tree flag that helps decide a consistent setting for the
> > > space cache options, so we just bring those further to the fore. For
> > > free space tree, the flag is already consistent, so we just switch mount
> > > option display to use it. cache_generation is not always reliably set
> > > correctly, so we ensure that cache_generation > 0 iff the file system
> > > is using space_cache v1. This requires committing a transaction on any
> > > mount which changes whether we are using v1. (v1->nospace_cache, v1->v2,
> > > nospace_cache->v1, v2->v1).
> > > 
> > > References: https://github.com/btrfs/btrfs-todo/issues/5
> > > Signed-off-by: Boris Burkov <boris@bur.io>
> > 
> > Dave already took this, but next time I'd prefer if we'd keep logical changes 
> > separate.  So one patch to change /proc/mounts, one patch to deal with clearing 
> > the free space generation field if we're not using it.
> 
> I haven't taken it yet, adding branches to for-next is only to get test
> coverage, by 'taken' you can count addig it to misc-next.

Would you guys like me to split it up? I felt it was worth keeping the
two changes together because changing the mount options without clearing
the generation will break /proc/mounts in cases that currently work OK.

e.g., I believe it will never show space_cache=v2 if you have the
space_cache if look at generation, and the generation has ever been set.

Re: [PATCH v3 3/4] btrfs: remove free space items when creating free space tree

[Boris Burkov]

On Mon, Sep 21, 2020 at 07:13:04PM +0200, David Sterba wrote:
> On Thu, Sep 17, 2020 at 11:13:40AM -0700, Boris Burkov wrote:
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -3333,6 +3333,15 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
> >  			close_ctree(fs_info);
> >  			return ret;
> >  		}
> > +		/*
> > +		 * Creating the free space tree creates inode orphan items and
> > +		 * delayed iputs when it deletes the free space inodes. Later in
> > +		 * open_ctree, we run btrfs_orphan_cleanup which tries to clean
> > +		 * up the orphan items. However, the outstanding references on
> > +		 * the inodes from the delayed iputs causes the cleanup to fail.
> > +		 * To fix it, force going through the delayed iputs here.
> > +		 */
> > +		btrfs_run_delayed_iputs(fs_info);
> 
> This is called from open_ctree, so this is mount context and the free
> space tree creation is called before that. That will schedule all free
> space inodes for deletion and waits here. This takes time proportional
> to the filesystem size.
> 
> We've had reports that this takes a lot of time already, so I wonder if
> the delayed iputs can be avoided here.

Good point. The way I see it, deleting the inodes creates legitimate
orphan items, and the delayed_iput doesn't go down until after
open_ctree, I assume for the reason you give. So to delete the free
space inodes without incurring the iput cost during mount, we could:

1. make orphan cleanup smart enough to tell the difference between
orphan items left over from an old mount, vs. orphan items created while
mounting.

2. move orphan cleanup to before free space tree creation/free space
inode deletion in open_ctree.

I'll try to think of a way to do 1. or think of some option 3, as 2.
seems a bit hacky/fragile.

While thinking about a legitimate case for orphans, I realized that
unconditionally failing on free space inode delete errors in
create_free_space_tree without handling ENOENT is almost certainly
wrong, so I will likely need to resend this patch anyway.

Re: [PATCH v3 3/4] btrfs: remove free space items when creating free space tree

[Josef Bacik]

On 9/21/20 1:13 PM, David Sterba wrote:
> On Thu, Sep 17, 2020 at 11:13:40AM -0700, Boris Burkov wrote:
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -3333,6 +3333,15 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>>   			close_ctree(fs_info);
>>   			return ret;
>>   		}
>> +		/*
>> +		 * Creating the free space tree creates inode orphan items and
>> +		 * delayed iputs when it deletes the free space inodes. Later in
>> +		 * open_ctree, we run btrfs_orphan_cleanup which tries to clean
>> +		 * up the orphan items. However, the outstanding references on
>> +		 * the inodes from the delayed iputs causes the cleanup to fail.
>> +		 * To fix it, force going through the delayed iputs here.
>> +		 */
>> +		btrfs_run_delayed_iputs(fs_info);
> 
> This is called from open_ctree, so this is mount context and the free
> space tree creation is called before that. That will schedule all free
> space inodes for deletion and waits here. This takes time proportional
> to the filesystem size.
> 
> We've had reports that this takes a lot of time already, so I wonder if
> the delayed iputs can be avoided here.
> 

Chris and I told him to do it this way.  If you have a giant FS the time is 
mostly going to be spent doing the free space tree, the iputs won't add much 
more time to that.  We have to do this so the orphan cleanup that follows 
doesn't screw up because we haven't put our inodes yet.  This is one time pain 
and avoids us having to figure out what to do about orphans we generate while 
mounting the file system.  Thanks,

Josef

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

[David Sterba]

On Thu, Sep 17, 2020 at 11:13:38AM -0700, Boris Burkov wrote:
>  #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,16 @@ 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 supported from read-only to read-write");

lowercase for start of the sting, unindent it so ends below 80 columns

> +			create_fst = false;
> +		}
> +	}
> +
>  	if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
>  		goto out;
>  
> @@ -1924,6 +1936,21 @@ 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) {
> +			ret = btrfs_create_free_space_tree(fs_info);
> +			if (ret) {
> +				btrfs_warn(fs_info,
> +					   "failed to create free space tree: %d", ret);

same

> +				goto restore;
> +			}
> +		}
> +
> +
>  		ret = btrfs_cleanup_fs_roots(fs_info);
>  		if (ret)
>  			goto restore;
> -- 
> 2.24.1

Re: [PATCH 2/4] btrfs: use sb state to print space_cache mount option

[David Sterba]

On Thu, Sep 17, 2020 at 11:13:39AM -0700, Boris Burkov wrote:
> @@ -1870,6 +1868,13 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
>  		if (!sb_rdonly(sb) || *flags & SB_RDONLY) {
>  			btrfs_warn(fs_info,
>  				   "Remounting with free space tree only supported from read-only to read-write");
> +			/*
> +			 * if we aren't building the free space tree, reset

First letter in comments should be uppercase, unless it's a name of
function or identifier

> +			 * the space cache options to what they were before
> +			 */
> +			btrfs_clear_opt(fs_info->mount_opt, FREE_SPACE_TREE);
> +			if (btrfs_free_space_cache_v1_active(fs_info))
> +				btrfs_set_opt(fs_info->mount_opt, SPACE_CACHE);
>  			create_fst = false;

Re: [PATCH v3 3/4] btrfs: remove free space items when creating free space tree

[David Sterba]

On Thu, Sep 17, 2020 at 11:13:40AM -0700, Boris Burkov wrote:
> --- a/fs/btrfs/free-space-cache.c
> +++ b/fs/btrfs/free-space-cache.c
> @@ -207,6 +207,64 @@ int create_free_space_inode(struct btrfs_trans_handle *trans,
>  					 ino, block_group->start);
>  }
>  
> +/*
> + * inode is an optional sink: if it is NULL, btrfs_remove_free_space_inode
> + * handles lookup, otherwise it takes ownership and iputs the inode.
> + * Don't reuse an inode pointer after passing it into this function.
> + */
> +int btrfs_remove_free_space_inode(struct btrfs_trans_handle *trans,
> +				  struct inode *inode,
> +				  struct btrfs_block_group *block_group)
> +{
> +	struct btrfs_path *path;
> +	struct btrfs_key key;
> +	int ret = 0;
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return -ENOMEM;
> +
> +	if (!inode) {
> +		inode = lookup_free_space_inode(block_group, path);
> +	}

No { } around single statement

> +	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 the 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);

This slightly overflows 80 but it's full one parameter so in this case
I'd rather wrap it (from -1). Otherwise, more than 80 is ok-ish if it's
the ); or end of an identifier that's half visible.


> +	if (ret) {
> +		if (ret > 0)
> +			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)
>  {