[PATCH 0/8] tree reading cleanups in mount

[PATCH 0/8] tree reading cleanups in mount

[Nikolay Borisov]

Hello, 

Here is the second version of the tree reading code which gets executed during 
mount. This goes a bit further than the previous posting in that it not only 
introduces a new function but also refactors the code which decides which backup
root to use. Overall I think the semantics are now much cleaner and centralised
in a single function - init_tree_roots rather than split between mount and 
backup root write out. 

The series starts gradually by simplifying find_newest_super_backup and its
callers (patches 1-2). 

It then paves the way forward by introducing read_backup_rooti (patch 3) which
supersedes (patch 4) next_root_backup, the latter is then removed (patch 6). While
at it I also remove the unnecessary objectid mutex during mount (patch 5). 

The final 2 patches streamlines how btrfs_fs_info::backup_root_index is being
initialised. 

This patchset has been tested by simulating (via btrfs-corrupt-block) corruption
of the primary root and resorting to using usebackuproot mount option. I've also 
added a regression test to btrfs-progs that will follow shortly. 

Nikolay Borisov (8):
  btrfs: Cleanup and simplify find_newest_super_backup
  btrfs: Remove newest_gen argument from find_oldest_super_backup
  btrfs: Add read_backup_root
  btrfs: Factor out tree roots initialization during mount
  btrfs: Don't use objectid_mutex during mount
  btrfs: Remove unused next_root_backup function
  btrfs: Rename find_oldest_super_backup to init_backup_root_slot
  btrfs: Streamline btrfs_fs_info::backup_root_index semantics

 fs/btrfs/disk-io.c | 252 ++++++++++++++++++++-------------------------
 1 file changed, 113 insertions(+), 139 deletions(-)

-- 
2.17.1

[PATCH 1/8] btrfs: Cleanup and simplify find_newest_super_backup

[Nikolay Borisov]

Backup roots are always written in a circular manner. By definition we
can only ever have 1 backup root whose generation equals to that of the
superblock. Hence, the 'if' in the for loop will trigger at most once.
This is sufficient to return the newest backup root.

Furthermore thew newest_gen parameter is always set to the generation
of the superblock. This value can be obtained from the fs_info.

This patch removes the unnecessary code dealing with the wraparound
case and makes 'newest_gen' a local variable.
---
 fs/btrfs/disk-io.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index de5696023391..8b1f6385023d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1761,18 +1761,18 @@ static int transaction_kthread(void *arg)
 }
 
 /*
- * this will find the highest generation in the array of
+ * This will find the highest generation in the array of
  * root backups.  The index of the highest array is returned,
- * or -1 if we can't find anything.
+ * or -EINVAL if we can't find anything.
  *
  * We check to make sure the array is valid by comparing the
  * generation of the latest  root in the array with the generation
  * in the super block.  If they don't match we pitch it.
  */
-static int find_newest_super_backup(struct btrfs_fs_info *info, u64 newest_gen)
+static int find_newest_super_backup(struct btrfs_fs_info *info)
 {
+	u64 newest_gen = btrfs_super_generation(info->super_copy);
 	u64 cur;
-	int newest_index = -1;
 	struct btrfs_root_backup *root_backup;
 	int i;
 
@@ -1780,17 +1780,10 @@ static int find_newest_super_backup(struct btrfs_fs_info *info, u64 newest_gen)
 		root_backup = info->super_copy->super_roots + i;
 		cur = btrfs_backup_tree_root_gen(root_backup);
 		if (cur == newest_gen)
-			newest_index = i;
+			return i;
 	}
 
-	/* check to see if we actually wrapped around */
-	if (newest_index == BTRFS_NUM_BACKUP_ROOTS - 1) {
-		root_backup = info->super_copy->super_roots;
-		cur = btrfs_backup_tree_root_gen(root_backup);
-		if (cur == newest_gen)
-			newest_index = 0;
-	}
-	return newest_index;
+	return -EINVAL;
 }
 
 
@@ -1802,11 +1795,11 @@ static int find_newest_super_backup(struct btrfs_fs_info *info, u64 newest_gen)
 static void find_oldest_super_backup(struct btrfs_fs_info *info,
 				     u64 newest_gen)
 {
-	int newest_index = -1;
+	int newest_index;
 
-	newest_index = find_newest_super_backup(info, newest_gen);
+	newest_index = find_newest_super_backup(info);
 	/* if there was garbage in there, just move along */
-	if (newest_index == -1) {
+	if (newest_index == -EINVAL) {
 		info->backup_root_index = 0;
 	} else {
 		info->backup_root_index = (newest_index + 1) % BTRFS_NUM_BACKUP_ROOTS;
@@ -1923,9 +1916,7 @@ static noinline int next_root_backup(struct btrfs_fs_info *info,
 	int newest = *backup_index;
 
 	if (*num_backups_tried == 0) {
-		u64 gen = btrfs_super_generation(super);
-
-		newest = find_newest_super_backup(info, gen);
+		newest = find_newest_super_backup(info);
 		if (newest == -1)
 			return -1;
 
-- 
2.17.1

Re: [PATCH 1/8] btrfs: Cleanup and simplify find_newest_super_backup

[David Sterba]

On Tue, Oct 15, 2019 at 06:42:17PM +0300, Nikolay Borisov wrote:
> Backup roots are always written in a circular manner. By definition we
> can only ever have 1 backup root whose generation equals to that of the
> superblock. Hence, the 'if' in the for loop will trigger at most once.
> This is sufficient to return the newest backup root.
> 
> Furthermore thew newest_gen parameter is always set to the generation
> of the superblock. This value can be obtained from the fs_info.
> 
> This patch removes the unnecessary code dealing with the wraparound
> case and makes 'newest_gen' a local variable.

"This patch cannot be applied without an SOB line." --n. borisov

And the rest does not have it either, I'll add it to avoid pointless
resends.

Tip of the day: use 'git commit -save' for every commit, it adds SOB,
opens editor for changelog and also shows the the diff

[PATCH 2/8] btrfs: Remove newest_gen argument from find_oldest_super_backup

[Nikolay Borisov]

It's no longer needed following cleanups around find_newest_backup_root
---
 fs/btrfs/disk-io.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 8b1f6385023d..d51f76abde45 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1792,8 +1792,7 @@ static int find_newest_super_backup(struct btrfs_fs_info *info)
  * in the backup array.  This will set the backup_root_index
  * field in the fs_info struct
  */
-static void find_oldest_super_backup(struct btrfs_fs_info *info,
-				     u64 newest_gen)
+static void find_oldest_super_backup(struct btrfs_fs_info *info)
 {
 	int newest_index;
 
@@ -2831,8 +2830,7 @@ int __cold open_ctree(struct super_block *sb,
 	 * run through our array of backup supers and setup
 	 * our ring pointer to the oldest one
 	 */
-	generation = btrfs_super_generation(disk_super);
-	find_oldest_super_backup(fs_info, generation);
+	find_oldest_super_backup(fs_info);
 
 	/*
 	 * In the long term, we'll store the compression type in the super
-- 
2.17.1

[PATCH 3/8] btrfs: Add read_backup_root

[Nikolay Borisov]

This function will replace next_root_backup with a much saner/cleaner
interface.
---
 fs/btrfs/disk-io.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index d51f76abde45..9ed3b40aa10d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1899,6 +1899,50 @@ static void backup_super_roots(struct btrfs_fs_info *info)
 	       sizeof(*root_backup) * BTRFS_NUM_BACKUP_ROOTS);
 }
 
+/*
+ * read_backup_root - Reads a backup root based on the passed priority. Prio 0
+ * is the newest, prio 1/2/3 are 2nd newest/3rd newest/4th (oldest) backup roots
+ *
+ * fs_info - filesystem whose backup roots need to be read
+ * priority - priority of backup root required
+ *
+ * Returns backup root index on success and -EINVAL otherwise.
+ */
+static int read_backup_root(struct btrfs_fs_info *fs_info, u8 priority)
+{
+	int backup_index = find_newest_super_backup(fs_info);
+	struct btrfs_super_block *super = fs_info->super_copy;
+	struct btrfs_root_backup *root_backup;
+
+	if (priority < BTRFS_NUM_BACKUP_ROOTS && backup_index >= 0) {
+		if (priority == 0)
+			return backup_index;
+
+		backup_index = (backup_index + BTRFS_NUM_BACKUP_ROOTS - priority) %
+			BTRFS_NUM_BACKUP_ROOTS;
+	} else {
+		return -EINVAL;
+	}
+
+	root_backup = super->super_roots + backup_index;
+
+	btrfs_set_super_generation(super,
+				   btrfs_backup_tree_root_gen(root_backup));
+	btrfs_set_super_root(super, btrfs_backup_tree_root(root_backup));
+	btrfs_set_super_root_level(super,
+				   btrfs_backup_tree_root_level(root_backup));
+	btrfs_set_super_bytes_used(super, btrfs_backup_bytes_used(root_backup));
+
+	/*
+	 * fixme: the total bytes and num_devices need to match or we should
+	 * need a fsck
+	 */
+	btrfs_set_super_total_bytes(super, btrfs_backup_total_bytes(root_backup));
+	btrfs_set_super_num_devices(super, btrfs_backup_num_devices(root_backup));
+
+	return backup_index;
+}
+
 /*
  * this copies info out of the root backup array and back into
  * the in-memory super block.  It is meant to help iterate through
-- 
2.17.1

[PATCH 4/8] btrfs: Factor out tree roots initialization during mount

[Nikolay Borisov]

The code responsible for reading and initilizing tree roots is
scattered in open_ctree among 2 labels, emulating a loop. This is rather
confusing to reason about. Instead, factor the code in a new function,
init_tree_roots which implements the same logical flow.

There are a couple of notable differences, namely:

* Instead of using next_backup_root it's using the newly introduced
  read_backup_root.

* If read_backup_root returns an error init_tree_roots propagates the
  error and there is no special handling of that case e.g. the code jumps
  straight to 'fail_tree_roots' label. The old code, however, was
  (erroneously) jumping to 'fail_block_groups' label if next_backup_root
  did fail, this was unnecessary since the tree roots init logic doesn't
  modify the state of block groups.
---
 fs/btrfs/disk-io.c | 140 +++++++++++++++++++++++++++------------------
 1 file changed, 83 insertions(+), 57 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 9ed3b40aa10d..a82e3acca765 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2581,6 +2581,87 @@ static int btrfs_validate_write_super(struct btrfs_fs_info *fs_info,
 	return ret;
 }
 
+int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
+{
+	struct btrfs_super_block *sb = fs_info->super_copy;
+	struct btrfs_root *tree_root = fs_info->tree_root;
+	bool handle_error = false;
+	int ret = 0;
+	int i;
+
+	for (i = 0; i < BTRFS_NUM_BACKUP_ROOTS; i++) {
+		u64 generation;
+		int level;
+
+		if (handle_error) {
+			if (!IS_ERR(tree_root->node))
+				free_extent_buffer(tree_root->node);
+			tree_root->node = NULL;
+
+			if (!btrfs_test_opt(fs_info, USEBACKUPROOT))
+				break;
+
+			free_root_pointers(fs_info, 0);
+
+			/* don't use the log in recovery mode, it won't be valid */
+			btrfs_set_super_log_root(sb, 0);
+
+			/* we can't trust the free space cache either */
+			btrfs_set_opt(fs_info->mount_opt, CLEAR_CACHE);
+
+			ret = read_backup_root(fs_info, i);
+			if (ret < 0)
+				return ret;
+		}
+		generation = btrfs_super_generation(sb);
+		level = btrfs_super_root_level(sb);
+		tree_root->node = read_tree_block(fs_info, btrfs_super_root(sb),
+						  generation, level, NULL);
+		if (IS_ERR(tree_root->node) ||
+		    !extent_buffer_uptodate(tree_root->node)) {
+			handle_error = true;
+
+			if (IS_ERR(tree_root->node))
+				ret = PTR_ERR(tree_root->node);
+			else if (!extent_buffer_uptodate(tree_root->node))
+				ret = -EUCLEAN;
+
+			btrfs_warn(fs_info, "failed to read tree root");
+			continue;
+		}
+
+		btrfs_set_root_node(&tree_root->root_item, tree_root->node);
+		tree_root->commit_root = btrfs_root_node(tree_root);
+		btrfs_set_root_refs(&tree_root->root_item, 1);
+
+		mutex_lock(&tree_root->objectid_mutex);
+		ret = btrfs_find_highest_objectid(tree_root,
+						&tree_root->highest_objectid);
+		if (ret < 0) {
+			mutex_unlock(&tree_root->objectid_mutex);
+			handle_error = true;
+			continue;
+		}
+
+		ASSERT(tree_root->highest_objectid <= BTRFS_LAST_FREE_OBJECTID);
+		mutex_unlock(&tree_root->objectid_mutex);
+
+		ret = btrfs_read_roots(fs_info);
+		if (ret < 0) {
+			handle_error = true;
+			continue;
+		}
+
+		/* All successful */
+		fs_info->generation = generation;
+		fs_info->last_trans_committed = generation;
+		break;
+
+	}
+
+	return ret;
+}
+
 int __cold open_ctree(struct super_block *sb,
 	       struct btrfs_fs_devices *fs_devices,
 	       char *options)
@@ -2599,8 +2680,6 @@ int __cold open_ctree(struct super_block *sb,
 	struct btrfs_root *chunk_root;
 	int ret;
 	int err = -EINVAL;
-	int num_backups_tried = 0;
-	int backup_index = 0;
 	int clear_free_space_tree = 0;
 	int level;
 
@@ -3022,44 +3101,9 @@ int __cold open_ctree(struct super_block *sb,
 		goto fail_tree_roots;
 	}
 
-retry_root_backup:
-	generation = btrfs_super_generation(disk_super);
-	level = btrfs_super_root_level(disk_super);
-
-	tree_root->node = read_tree_block(fs_info,
-					  btrfs_super_root(disk_super),
-					  generation, level, NULL);
-	if (IS_ERR(tree_root->node) ||
-	    !extent_buffer_uptodate(tree_root->node)) {
-		btrfs_warn(fs_info, "failed to read tree root");
-		if (!IS_ERR(tree_root->node))
-			free_extent_buffer(tree_root->node);
-		tree_root->node = NULL;
-		goto recovery_tree_root;
-	}
-
-	btrfs_set_root_node(&tree_root->root_item, tree_root->node);
-	tree_root->commit_root = btrfs_root_node(tree_root);
-	btrfs_set_root_refs(&tree_root->root_item, 1);
-
-	mutex_lock(&tree_root->objectid_mutex);
-	ret = btrfs_find_highest_objectid(tree_root,
-					&tree_root->highest_objectid);
-	if (ret) {
-		mutex_unlock(&tree_root->objectid_mutex);
-		goto recovery_tree_root;
-	}
-
-	ASSERT(tree_root->highest_objectid <= BTRFS_LAST_FREE_OBJECTID);
-
-	mutex_unlock(&tree_root->objectid_mutex);
-
-	ret = btrfs_read_roots(fs_info);
+	ret = init_tree_roots(fs_info);
 	if (ret)
-		goto recovery_tree_root;
-
-	fs_info->generation = generation;
-	fs_info->last_trans_committed = generation;
+		goto fail_tree_roots;
 
 	ret = btrfs_verify_dev_extents(fs_info);
 	if (ret) {
@@ -3354,24 +3398,6 @@ int __cold open_ctree(struct super_block *sb,
 	btrfs_free_stripe_hash_table(fs_info);
 	btrfs_close_devices(fs_info->fs_devices);
 	return err;
-
-recovery_tree_root:
-	if (!btrfs_test_opt(fs_info, USEBACKUPROOT))
-		goto fail_tree_roots;
-
-	free_root_pointers(fs_info, false);
-
-	/* don't use the log in recovery mode, it won't be valid */
-	btrfs_set_super_log_root(disk_super, 0);
-
-	/* we can't trust the free space cache either */
-	btrfs_set_opt(fs_info->mount_opt, CLEAR_CACHE);
-
-	ret = next_root_backup(fs_info, fs_info->super_copy,
-			       &num_backups_tried, &backup_index);
-	if (ret == -1)
-		goto fail_block_groups;
-	goto retry_root_backup;
 }
 ALLOW_ERROR_INJECTION(open_ctree, ERRNO);
 
-- 
2.17.1

[PATCH 5/8] btrfs: Don't use objectid_mutex during mount

[Nikolay Borisov]

Since the filesystem is not well formed and no trees are loaded it's
pointless holding the objectid_mutex. Just remove its usage.
---
 fs/btrfs/disk-io.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index a82e3acca765..418619dfb76c 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2634,17 +2634,18 @@ int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
 		tree_root->commit_root = btrfs_root_node(tree_root);
 		btrfs_set_root_refs(&tree_root->root_item, 1);
 
-		mutex_lock(&tree_root->objectid_mutex);
+		/*
+		 * No need to hold btrfs_root::objectid_mutex since the fs
+		 * hasn't been fully initialised and we are the only user
+		 */
 		ret = btrfs_find_highest_objectid(tree_root,
 						&tree_root->highest_objectid);
 		if (ret < 0) {
-			mutex_unlock(&tree_root->objectid_mutex);
 			handle_error = true;
 			continue;
 		}
 
 		ASSERT(tree_root->highest_objectid <= BTRFS_LAST_FREE_OBJECTID);
-		mutex_unlock(&tree_root->objectid_mutex);
 
 		ret = btrfs_read_roots(fs_info);
 		if (ret < 0) {
-- 
2.17.1

[PATCH 6/8] btrfs: Remove unused next_root_backup function

[Nikolay Borisov]

This function is no longer used so just remove it
---
 fs/btrfs/disk-io.c | 50 ----------------------------------------------
 1 file changed, 50 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 418619dfb76c..bcb21a35d30c 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1943,56 +1943,6 @@ static int read_backup_root(struct btrfs_fs_info *fs_info, u8 priority)
 	return backup_index;
 }
 
-/*
- * this copies info out of the root backup array and back into
- * the in-memory super block.  It is meant to help iterate through
- * the array, so you send it the number of backups you've already
- * tried and the last backup index you used.
- *
- * this returns -1 when it has tried all the backups
- */
-static noinline int next_root_backup(struct btrfs_fs_info *info,
-				     struct btrfs_super_block *super,
-				     int *num_backups_tried, int *backup_index)
-{
-	struct btrfs_root_backup *root_backup;
-	int newest = *backup_index;
-
-	if (*num_backups_tried == 0) {
-		newest = find_newest_super_backup(info);
-		if (newest == -1)
-			return -1;
-
-		*backup_index = newest;
-		*num_backups_tried = 1;
-	} else if (*num_backups_tried == BTRFS_NUM_BACKUP_ROOTS) {
-		/* we've tried all the backups, all done */
-		return -1;
-	} else {
-		/* jump to the next oldest backup */
-		newest = (*backup_index + BTRFS_NUM_BACKUP_ROOTS - 1) %
-			BTRFS_NUM_BACKUP_ROOTS;
-		*backup_index = newest;
-		*num_backups_tried += 1;
-	}
-	root_backup = super->super_roots + newest;
-
-	btrfs_set_super_generation(super,
-				   btrfs_backup_tree_root_gen(root_backup));
-	btrfs_set_super_root(super, btrfs_backup_tree_root(root_backup));
-	btrfs_set_super_root_level(super,
-				   btrfs_backup_tree_root_level(root_backup));
-	btrfs_set_super_bytes_used(super, btrfs_backup_bytes_used(root_backup));
-
-	/*
-	 * fixme: the total bytes and num_devices need to match or we should
-	 * need a fsck
-	 */
-	btrfs_set_super_total_bytes(super, btrfs_backup_total_bytes(root_backup));
-	btrfs_set_super_num_devices(super, btrfs_backup_num_devices(root_backup));
-	return 0;
-}
-
 /* helper to cleanup workers */
 static void btrfs_stop_all_workers(struct btrfs_fs_info *fs_info)
 {
-- 
2.17.1

[PATCH 7/8] btrfs: Rename find_oldest_super_backup to init_backup_root_slot

[Nikolay Borisov]

The old name name was an awful misnomer because it didn't really find
the oldest super backup per se but rather its slot. For example if we
have:

slot0: gen - 2
slot1: gen - 1
slot2: gen
slot3: empty

init_backup_root_slot will return slot3 and not slot0.

The new name is more appropriate since the function doesn't care whether
there is a valid backup in the returned slot or not.
---
 fs/btrfs/disk-io.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index bcb21a35d30c..ac899fdb1414 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1786,13 +1786,11 @@ static int find_newest_super_backup(struct btrfs_fs_info *info)
 	return -EINVAL;
 }
 
-
 /*
- * find the oldest backup so we know where to store new entries
- * in the backup array.  This will set the backup_root_index
- * field in the fs_info struct
+ * Initialize backup_root_index with the next available slot, where subsequent
+ * transaction commit will store the back up root
  */
-static void find_oldest_super_backup(struct btrfs_fs_info *info)
+static void init_backup_root_slot(struct btrfs_fs_info *info)
 {
 	int newest_index;
 
@@ -2904,7 +2902,7 @@ int __cold open_ctree(struct super_block *sb,
 	 * run through our array of backup supers and setup
 	 * our ring pointer to the oldest one
 	 */
-	find_oldest_super_backup(fs_info);
+	init_backup_root_slot(fs_info);
 
 	/*
 	 * In the long term, we'll store the compression type in the super
-- 
2.17.1

[PATCH 8/8] btrfs: Streamline btrfs_fs_info::backup_root_index semantics

[Nikolay Borisov]

The backup_root_index member stores the index at which  the backup root
should be saved upon next transaction commit. However, there is a
small deviation from this behavior in the form of a check in
backup_super_roots which checks if current root generation equals to the
generation of the previous root. This can trigger in the following
scenario:

slot0: gen-2
slot1: gen-1
slot2: gen
slot3: unused

Now suppose slot3 (which is also the root specified in the super block)
is corrupted hence init_tree_roots chooses to use the backup root at
slot2, meaning read_backup_root will read slot2 and assign the
superblock generation to gen-1. Despite this backup_root_index will
point at slot3 because its init happens in init_backup_root_slot, long
before any parsing of the backup roots occur. Then on next transaction
start, gen-1 will be incremented by 1 making the root's generation
equal gen. Subsequently, on transaction commit the following check
triggers:

  if (btrfs_backup_tree_root_gen(root_backup) ==
           btrfs_header_generation(info->tree_root->node))

This causes the 'next_backup', which is the index at which the backup is
going to be written to, to set to last_backup, which will be slot2.

All of this is a very confusing way of expressing the following
invariant:

 Always write a backup root at the index following the last used backup
 root.

This commit streamlines this logic by setting backup_root_index to the
next index after the one used for mount.
---
 fs/btrfs/disk-io.c | 48 +++++++++-------------------------------------
 1 file changed, 9 insertions(+), 39 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index ac899fdb1414..e266949529fb 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1786,23 +1786,6 @@ static int find_newest_super_backup(struct btrfs_fs_info *info)
 	return -EINVAL;
 }
 
-/*
- * Initialize backup_root_index with the next available slot, where subsequent
- * transaction commit will store the back up root
- */
-static void init_backup_root_slot(struct btrfs_fs_info *info)
-{
-	int newest_index;
-
-	newest_index = find_newest_super_backup(info);
-	/* if there was garbage in there, just move along */
-	if (newest_index == -EINVAL) {
-		info->backup_root_index = 0;
-	} else {
-		info->backup_root_index = (newest_index + 1) % BTRFS_NUM_BACKUP_ROOTS;
-	}
-}
-
 /*
  * copy all the root pointers into the super backup array.
  * this will bump the backup pointer by one when it is
@@ -1810,22 +1793,8 @@ static void init_backup_root_slot(struct btrfs_fs_info *info)
  */
 static void backup_super_roots(struct btrfs_fs_info *info)
 {
-	int next_backup;
+	int next_backup = info->backup_root_index;
 	struct btrfs_root_backup *root_backup;
-	int last_backup;
-
-	next_backup = info->backup_root_index;
-	last_backup = (next_backup + BTRFS_NUM_BACKUP_ROOTS - 1) %
-		BTRFS_NUM_BACKUP_ROOTS;
-
-	/*
-	 * just overwrite the last backup if we're at the same generation
-	 * this happens only at umount
-	 */
-	root_backup = info->super_for_commit->super_roots + last_backup;
-	if (btrfs_backup_tree_root_gen(root_backup) ==
-	    btrfs_header_generation(info->tree_root->node))
-		next_backup = last_backup;
 
 	root_backup = info->super_for_commit->super_roots + next_backup;
 
@@ -2531,6 +2500,7 @@ static int btrfs_validate_write_super(struct btrfs_fs_info *fs_info,
 
 int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
 {
+	int backup_index = find_newest_super_backup(fs_info);
 	struct btrfs_super_block *sb = fs_info->super_copy;
 	struct btrfs_root *tree_root = fs_info->tree_root;
 	bool handle_error = false;
@@ -2557,7 +2527,7 @@ int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
 			/* we can't trust the free space cache either */
 			btrfs_set_opt(fs_info->mount_opt, CLEAR_CACHE);
 
-			ret = read_backup_root(fs_info, i);
+			ret = backup_index = read_backup_root(fs_info, i);
 			if (ret < 0)
 				return ret;
 		}
@@ -2604,6 +2574,12 @@ int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
 		/* All successful */
 		fs_info->generation = generation;
 		fs_info->last_trans_committed = generation;
+
+		/* Always begin writing backup roots after one being used */
+		if (backup_index < 0)
+			fs_info->backup_root_index = 0;
+		else
+			fs_info->backup_root_index = (backup_index + 1) % BTRFS_NUM_BACKUP_ROOTS;
 		break;
 
 	}
@@ -2898,12 +2874,6 @@ int __cold open_ctree(struct super_block *sb,
 	if (btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_ERROR)
 		set_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state);
 
-	/*
-	 * run through our array of backup supers and setup
-	 * our ring pointer to the oldest one
-	 */
-	init_backup_root_slot(fs_info);
-
 	/*
 	 * In the long term, we'll store the compression type in the super
 	 * block, and it'll be used for per file compression control.
-- 
2.17.1

Re: [PATCH 0/8] tree reading cleanups in mount

[David Sterba]

On Tue, Oct 15, 2019 at 06:42:16PM +0300, Nikolay Borisov wrote:
> Hello, 
> 
> Here is the second version of the tree reading code which gets executed during 
> mount. This goes a bit further than the previous posting in that it not only 
> introduces a new function but also refactors the code which decides which backup
> root to use. Overall I think the semantics are now much cleaner and centralised
> in a single function - init_tree_roots rather than split between mount and 
> backup root write out. 
> 
> The series starts gradually by simplifying find_newest_super_backup and its
> callers (patches 1-2). 
> 
> It then paves the way forward by introducing read_backup_rooti (patch 3) which
> supersedes (patch 4) next_root_backup, the latter is then removed (patch 6). While
> at it I also remove the unnecessary objectid mutex during mount (patch 5). 
> 
> The final 2 patches streamlines how btrfs_fs_info::backup_root_index is being
> initialised. 
> 
> This patchset has been tested by simulating (via btrfs-corrupt-block) corruption
> of the primary root and resorting to using usebackuproot mount option. I've also 
> added a regression test to btrfs-progs that will follow shortly. 
> 
> Nikolay Borisov (8):
>   btrfs: Cleanup and simplify find_newest_super_backup
>   btrfs: Remove newest_gen argument from find_oldest_super_backup
>   btrfs: Add read_backup_root
>   btrfs: Factor out tree roots initialization during mount
>   btrfs: Don't use objectid_mutex during mount
>   btrfs: Remove unused next_root_backup function
>   btrfs: Rename find_oldest_super_backup to init_backup_root_slot
>   btrfs: Streamline btrfs_fs_info::backup_root_index semantics

Moved from topic branch to misc-next. The cleaned up code looks much
better, thanks.