[PATCH 0/7] btrfs-progs: various regression fixes

[PATCH 0/7] btrfs-progs: various regression fixes

[Josef Bacik]

Hello,

In trying to test my new extent tree v2 patches I noticed I had regressed a few
of these tests with my previous prep patches.  I'm not entirely sure how I
missed this before as I generally run the tests, but there are some clear things
that need fixing.  I've based these patches on the devel branch, with them in
place everything passes as it should.  Thanks,

Josef

Josef Bacik (7):
  btrfs-progs: check: fix check_global_roots_uptodate
  btrfs-progs: fuzz-tests: use --force for --init-csum-tree
  btrfs-progs: repair: bail if we find an unaligned extent
  btrfs-progs: properly populate missing trees
  btrfs-progs: don't check skip_csum_check if there's no fs_info
  btrfs-progs: do not try to load the free space tree if it's not
    enabled
  btrfs-progs: inspect-tree-stats: initialize the key properly

 check/main.c                                  | 34 +++++++++++++-
 cmds/inspect-tree-stats.c                     |  2 +-
 common/repair.c                               | 17 +++++--
 kernel-shared/disk-io.c                       | 47 ++++++++++++++++++-
 .../003-multi-check-unmounted/test.sh         |  2 +-
 5 files changed, 94 insertions(+), 8 deletions(-)

-- 
2.26.3

[PATCH 1/7] btrfs-progs: check: fix check_global_roots_uptodate

[Josef Bacik]

While running make test on other patches I noticed we are now
segfaulting on the fuzz tests.  This is because when I converted us to a
rb tree for the global roots we lost the ability to catch that there's
no extent root at all.  Before we'd populate a dummy
fs_info->extent_root with a not uptodate node, but now you simply don't
get an extent root in the rb_tree.  Fix the check_global_roots_uptodate
helper to count how many roots we find and make sure it matches the
number we expect.  If it doesn't then we can return -EIO.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 check/main.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/check/main.c b/check/main.c
index 8ccba447..abe208df 100644
--- a/check/main.c
+++ b/check/main.c
@@ -10207,6 +10207,8 @@ static int check_global_roots_uptodate(void)
 {
 	struct btrfs_root *root;
 	struct rb_node *n;
+	int found_csum = 0, found_extent = 0, found_fst = 0;
+	int ret = 0;
 
 	for (n = rb_first(&gfs_info->global_roots_tree); n; n = rb_next(n)) {
 		root = rb_entry(n, struct btrfs_root, rb_node);
@@ -10215,9 +10217,39 @@ static int check_global_roots_uptodate(void)
 			      root->root_key.objectid, root->root_key.offset);
 			return -EIO;
 		}
+		switch(root->root_key.objectid) {
+		case BTRFS_EXTENT_TREE_OBJECTID:
+			found_extent++;
+			break;
+		case BTRFS_CSUM_TREE_OBJECTID:
+			found_csum++;
+			break;
+		case BTRFS_FREE_SPACE_TREE_OBJECTID:
+			found_fst++;
+			break;
+		default:
+			break;
+		}
 	}
 
-	return 0;
+	if (found_extent != gfs_info->nr_global_roots) {
+		error("found %d extent roots, expected %llu\n", found_extent,
+		      gfs_info->nr_global_roots);
+		ret = -EIO;
+	}
+	if (found_csum != gfs_info->nr_global_roots) {
+		error("found %d csum roots, expected %llu\n", found_csum,
+		      gfs_info->nr_global_roots);
+		ret = -EIO;
+	}
+	if (!btrfs_fs_compat_ro(gfs_info, FREE_SPACE_TREE))
+		return ret;
+	if (found_fst != gfs_info->nr_global_roots) {
+		error("found %d free space roots, expected %llu\n", found_fst,
+		      gfs_info->nr_global_roots);
+		ret = -EIO;
+	}
+	return ret;
 }
 
 static const char * const cmd_check_usage[] = {
-- 
2.26.3

Re: [PATCH 1/7] btrfs-progs: check: fix check_global_roots_uptodate

[David Sterba]

On Tue, Feb 22, 2022 at 05:22:36PM -0500, Josef Bacik wrote:
> While running make test on other patches I noticed we are now
> segfaulting on the fuzz tests.  This is because when I converted us to a
> rb tree for the global roots we lost the ability to catch that there's
> no extent root at all.  Before we'd populate a dummy
> fs_info->extent_root with a not uptodate node, but now you simply don't
> get an extent root in the rb_tree.  Fix the check_global_roots_uptodate
> helper to count how many roots we find and make sure it matches the
> number we expect.  If it doesn't then we can return -EIO.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  check/main.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/check/main.c b/check/main.c
> index 8ccba447..abe208df 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -10207,6 +10207,8 @@ static int check_global_roots_uptodate(void)
>  {
>  	struct btrfs_root *root;
>  	struct rb_node *n;
> +	int found_csum = 0, found_extent = 0, found_fst = 0;
> +	int ret = 0;
>  
>  	for (n = rb_first(&gfs_info->global_roots_tree); n; n = rb_next(n)) {
>  		root = rb_entry(n, struct btrfs_root, rb_node);
> @@ -10215,9 +10217,39 @@ static int check_global_roots_uptodate(void)
>  			      root->root_key.objectid, root->root_key.offset);
>  			return -EIO;
>  		}
> +		switch(root->root_key.objectid) {
> +		case BTRFS_EXTENT_TREE_OBJECTID:
> +			found_extent++;
> +			break;
> +		case BTRFS_CSUM_TREE_OBJECTID:
> +			found_csum++;
> +			break;
> +		case BTRFS_FREE_SPACE_TREE_OBJECTID:
> +			found_fst++;
> +			break;
> +		default:
> +			break;
> +		}
>  	}
>  
> -	return 0;
> +	if (found_extent != gfs_info->nr_global_roots) {
> +		error("found %d extent roots, expected %llu\n", found_extent,

Reminder for the future, the strings for error() a warning() helpers do
not need the trailing \n.

[PATCH 2/7] btrfs-progs: fuzz-tests: use --force for --init-csum-tree

[Josef Bacik]

I noticed that 003-multi-check-unmounted was taking a long time, this is
because it was doing the 10 second countdown for each iteration of
--init-csum-tree.  Fix this by using --force to bypass the countdown.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 tests/fuzz-tests/003-multi-check-unmounted/test.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/fuzz-tests/003-multi-check-unmounted/test.sh b/tests/fuzz-tests/003-multi-check-unmounted/test.sh
index e93a20af..c6f9cc9f 100755
--- a/tests/fuzz-tests/003-multi-check-unmounted/test.sh
+++ b/tests/fuzz-tests/003-multi-check-unmounted/test.sh
@@ -14,7 +14,7 @@ check_image() {
 
 	image=$1
 	run_mayfail $TOP/btrfs check -s 1 "$image"
-	run_mayfail $TOP/btrfs check --init-csum-tree "$image"
+	run_mayfail $TOP/btrfs check --force --init-csum-tree "$image"
 	run_mayfail $TOP/btrfs check --repair --force --init-extent-tree "$image"
 	run_mayfail $TOP/btrfs check --repair --force --check-data-csum "$image"
 	run_mayfail $TOP/btrfs check --subvol-extents "$image"
-- 
2.26.3

[PATCH 3/7] btrfs-progs: repair: bail if we find an unaligned extent

[Josef Bacik]

The fuzz test 003 was infinite looping when I reworked the code to
re-calculate the used bytes for the superblock.  This is because fsck
wasn't properly fixing the bad extent before my change, it just happened
to error out nicely, whereas my change made it so we go the wrong bytes
used count and just infinite looped trying to fix the problem.

Fix this by sanity checking the extent when we try to re-calculate the
bytes_used.  This makes us no longer infinite loop so we can get through
the fuzz tests.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 common/repair.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/common/repair.c b/common/repair.c
index f8c3f89c..a73949b0 100644
--- a/common/repair.c
+++ b/common/repair.c
@@ -178,9 +178,11 @@ static int populate_used_from_extent_root(struct btrfs_root *root,
 		if (slot >= btrfs_header_nritems(leaf)) {
 			ret = btrfs_next_leaf(root, &path);
 			if (ret < 0)
-				return ret;
-			if (ret > 0)
 				break;
+			if (ret > 0) {
+				ret = 0;
+				break;
+			}
 			leaf = path.nodes[0];
 			slot = path.slots[0];
 		}
@@ -191,13 +193,20 @@ static int populate_used_from_extent_root(struct btrfs_root *root,
 		else if (key.type == BTRFS_METADATA_ITEM_KEY)
 			end = start + fs_info->nodesize - 1;
 
-		if (start != end)
+		if (start != end) {
+			if (!IS_ALIGNED(start, fs_info->sectorsize) ||
+			    !IS_ALIGNED(end + 1, fs_info->sectorsize)) {
+				fprintf(stderr, "unaligned value in the extent tree\n");
+				ret = -EINVAL;
+				break;
+			}
 			set_extent_dirty(io_tree, start, end);
+		}
 
 		path.slots[0]++;
 	}
 	btrfs_release_path(&path);
-	return 0;
+	return ret;
 }
 
 int btrfs_mark_used_blocks(struct btrfs_fs_info *fs_info,
-- 
2.26.3

[PATCH 4/7] btrfs-progs: properly populate missing trees

[Josef Bacik]

With my global roots prep patches I regressed us on handling the case
where we didn't find a root at all.  In this case we need to return an
error (prior we returned -ENOENT) or we need to populate a dummy tree if
we have OPEN_CTREE_PARTIAL set.  This fixes a segfault of fuzz test 006.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 kernel-shared/disk-io.c | 43 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
index e7eab1a1..1cd965aa 100644
--- a/kernel-shared/disk-io.c
+++ b/kernel-shared/disk-io.c
@@ -1059,6 +1059,7 @@ static int load_global_roots_objectid(struct btrfs_fs_info *fs_info,
 	struct btrfs_root *tree_root = fs_info->tree_root;
 	struct btrfs_root *root;
 	int ret;
+	int found = 0;
 	struct btrfs_key key = {
 		.objectid = objectid,
 		.type = BTRFS_ROOT_ITEM_KEY,
@@ -1124,9 +1125,51 @@ static int load_global_roots_objectid(struct btrfs_fs_info *fs_info,
 			break;
 		}
 
+		found++;
 		path->slots[0]++;
 	}
 	btrfs_release_path(path);
+
+	/*
+	 * We didn't find all of our roots, create empty ones if we have PARTIAL
+	 * set.
+	 */
+	if (!ret && found < fs_info->nr_global_roots) {
+		int i;
+
+		if (!(flags & OPEN_CTREE_PARTIAL)) {
+			error("could not setup %s tree", str);
+			return -EIO;
+		}
+
+		warning("could not setup %s tree, skipping it", str);
+		for (i = found; i < fs_info->nr_global_roots; i++) {
+			root = calloc(1, sizeof(*root));
+			if (!root) {
+				ret = -ENOMEM;
+				break;
+			}
+			btrfs_setup_root(root, fs_info, objectid);
+			root->root_key.objectid = objectid;
+			root->root_key.type = BTRFS_ROOT_ITEM_KEY;
+			root->root_key.offset = i;
+			root->track_dirty = 1;
+			root->node = btrfs_find_create_tree_block(fs_info, 0);
+			if (!root->node) {
+				free(root);
+				ret = -ENOMEM;
+				break;
+			}
+			clear_extent_buffer_uptodate(root->node);
+			ret = btrfs_global_root_insert(fs_info, root);
+			if (ret) {
+				free_extent_buffer(root->node);
+				free(root);
+				break;
+			}
+		}
+	}
+
 	return ret;
 }
 
-- 
2.26.3

Re: [PATCH 4/7] btrfs-progs: properly populate missing trees

[David Sterba]

On Tue, Feb 22, 2022 at 05:22:39PM -0500, Josef Bacik wrote:
> With my global roots prep patches I regressed us on handling the case
> where we didn't find a root at all.  In this case we need to return an
> error (prior we returned -ENOENT) or we need to populate a dummy tree if
> we have OPEN_CTREE_PARTIAL set.  This fixes a segfault of fuzz test 006.

This unfortunatelly breaks test fsck/001-bad-file-extent-bytenr, seems
that the image can't be properly restored before the test starts.

Re: [PATCH 4/7] btrfs-progs: properly populate missing trees

[David Sterba]

On Fri, Mar 04, 2022 at 11:18:29PM +0100, David Sterba wrote:
> On Tue, Feb 22, 2022 at 05:22:39PM -0500, Josef Bacik wrote:
> > With my global roots prep patches I regressed us on handling the case
> > where we didn't find a root at all.  In this case we need to return an
> > error (prior we returned -ENOENT) or we need to populate a dummy tree if
> > we have OPEN_CTREE_PARTIAL set.  This fixes a segfault of fuzz test 006.
> 
> This unfortunatelly breaks test fsck/001-bad-file-extent-bytenr, seems
> that the image can't be properly restored before the test starts.

Fixed by reordering patches.

[PATCH 5/7] btrfs-progs: don't check skip_csum_check if there's no fs_info

[Josef Bacik]

The chunk recover code passes in a buffer it allocates with metadata but
no fs_info, causing fuzz-test 008 to segfault.  Fix this test to only
check the flag if we have buf->fs_info set.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 kernel-shared/disk-io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
index 1cd965aa..637e8b00 100644
--- a/kernel-shared/disk-io.c
+++ b/kernel-shared/disk-io.c
@@ -174,7 +174,7 @@ static int __csum_tree_block_size(struct extent_buffer *buf, u16 csum_size,
 			result, len);
 
 	if (verify) {
-		if (buf->fs_info->skip_csum_check) {
+		if (buf->fs_info && buf->fs_info->skip_csum_check) {
 			/* printf("skip csum check for block %llu\n", buf->start); */
 		} else if (memcmp_extent_buffer(buf, result, 0, csum_size)) {
 			if (!silent) {
-- 
2.26.3

[PATCH 6/7] btrfs-progs: do not try to load the free space tree if it's not enabled

[Josef Bacik]

We were previously getting away with this because the
load_global_roots() treated ENOENT like everything was a-ok.  However
that was a bug and fixing that bug uncovered a problem where we were
unconditionally trying to load the free space tree.  Fix that by
skipping the load if we do not have the compat bit set.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 kernel-shared/disk-io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
index 637e8b00..f83ba884 100644
--- a/kernel-shared/disk-io.c
+++ b/kernel-shared/disk-io.c
@@ -1192,6 +1192,8 @@ static int load_global_roots(struct btrfs_fs_info *fs_info, unsigned flags)
 					 "csum");
 	if (ret)
 		goto out;
+	if (!btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE))
+		goto out;
 	ret = load_global_roots_objectid(fs_info, path,
 					 BTRFS_FREE_SPACE_TREE_OBJECTID, flags,
 					 "free space");
-- 
2.26.3

[PATCH 7/7] btrfs-progs: inspect-tree-stats: initialize the key properly

[Josef Bacik]

I started hitting a segfault on fuzz test 006 because we couldn't find
the extent root.  This is because the global root search stuff expects
the actual key to be setup properly, not just an objectid.  Fix this by
initializing the key properly so we can find the extent root and other
trees properly.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 cmds/inspect-tree-stats.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmds/inspect-tree-stats.c b/cmds/inspect-tree-stats.c
index 1fa35e7a..eeb57810 100644
--- a/cmds/inspect-tree-stats.c
+++ b/cmds/inspect-tree-stats.c
@@ -447,7 +447,7 @@ static const char * const cmd_inspect_tree_stats_usage[] = {
 static int cmd_inspect_tree_stats(const struct cmd_struct *cmd,
 				  int argc, char **argv)
 {
-	struct btrfs_key key;
+	struct btrfs_key key = { .type = BTRFS_ROOT_ITEM_KEY };
 	struct btrfs_root *root;
 	int opt;
 	int ret = 0;
-- 
2.26.3

Re: [PATCH 0/7] btrfs-progs: various regression fixes

[David Sterba]

On Tue, Feb 22, 2022 at 05:22:35PM -0500, Josef Bacik wrote:
> Hello,
> 
> In trying to test my new extent tree v2 patches I noticed I had regressed a few
> of these tests with my previous prep patches.  I'm not entirely sure how I
> missed this before as I generally run the tests, but there are some clear things
> that need fixing.  I've based these patches on the devel branch, with them in
> place everything passes as it should.  Thanks,
> 
> Josef
> 
> Josef Bacik (7):
>   btrfs-progs: check: fix check_global_roots_uptodate
>   btrfs-progs: fuzz-tests: use --force for --init-csum-tree
>   btrfs-progs: repair: bail if we find an unaligned extent
>   btrfs-progs: properly populate missing trees
>   btrfs-progs: don't check skip_csum_check if there's no fs_info
>   btrfs-progs: do not try to load the free space tree if it's not
>     enabled
>   btrfs-progs: inspect-tree-stats: initialize the key properly

Added to devel, thanks.