[PATCH 0/3] btrfs-progs: make subpage warnings more strict

[PATCH 0/3] btrfs-progs: make subpage warnings more strict

[Qu Wenruo]

For the incoming support of more page sizes for subpage RW mount, we
will require tree blocks to be nodesize aligned.

This patch prepare such restrict warnings for btrfs-check and update
self-tests to handle them.

Currently all convert/fsck/misc can pass except one unrelated
regression, fsck/025, which is caused by "btrfs-progs: Drop the type
check in init_alloc_chunk_ctl_policy_regular".

All fsck images except fsck/018 are newer enough to have all their tree
blocks nodesize aligned, so they won't cause any new warnings.
But still, for read-only tests, we will skip the subpage warnings as we
only want to ensure our writes from btrfs-progs won't cause new subpage
warnings.

Qu Wenruo (3):
  btrfs-progs: tests: also check subpage warning for type 2 test cases
  btrfs-progs: tests: don't check subpage related warnings for fsck type
    1 tests
  btrfs-progs: require full nodesize alignement for subpage support

 check/main.c        |  3 ++-
 check/mode-common.h | 22 +++++++++++++---------
 check/mode-lowmem.c |  3 ++-
 tests/common        | 16 +++++++++++++---
 tests/fsck-tests.sh |  9 ++++++++-
 5 files changed, 38 insertions(+), 15 deletions(-)

-- 
2.32.0

[PATCH 1/3] btrfs-progs: tests: also check subpage warning for type 2 test cases

[Qu Wenruo]

There are two types of test cases:

- Type 1 (without test.sh)
- Type 2 (test.sh, mostly will override check_image())

For Type 2 tests, we check subpage related warnings of btrfs-check, but
didn't check it for Type 1 test cases.

In fact, Type 1 test cases are more important, as they involve repair,
which can generate new tree blocks, and we want to make sure such new
tree blocks won't cause subpage related warnings.

This patch will add the extra check for Type 1 test cases.

And it will make sure the subpage related warnings are really from this
test case, to prevent false alerts.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/common | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tests/common b/tests/common
index 805a447c84ce..a6f75c7ce237 100644
--- a/tests/common
+++ b/tests/common
@@ -423,13 +423,23 @@ check_image()
 {
 	local image
 
+	tmp_output=$(mktemp --tmpdir btrfs-progs-test-check-image.XXXXXX)
+
 	image=$1
 	echo "testing image $(basename $image)" >> "$RESULTS"
-	"$TOP/btrfs" check "$image" >> "$RESULTS" 2>&1
+	"$TOP/btrfs" check "$image" &> "$tmp_output"
 	[ $? -eq 0 ] && _fail "btrfs check should have detected corruption"
 
+	cat "$tmp_output" >> "$RESULTS"
+	# Also make sure no subpage related warnings
+	check_test_results "$tmp_output" "$testname"
+
 	run_check "$TOP/btrfs" check --repair --force "$image"
-	run_check "$TOP/btrfs" check "$image"
+	run_check_stdout "$TOP/btrfs" check "$image" &> "$tmp_output"
+
+	# Also make sure no subpage related warnings for the repaired image
+	check_test_results "$tmp_output" "$testname"
+	rm -f "$tmp_output"
 }
 
 # Extract a usable image from packed formats
-- 
2.32.0

Re: [PATCH 1/3] btrfs-progs: tests: also check subpage warning for type 2 test cases

[David Sterba]

On Wed, Aug 18, 2021 at 02:44:18PM +0800, Qu Wenruo wrote:
> There are two types of test cases:
> 
> - Type 1 (without test.sh)
> - Type 2 (test.sh, mostly will override check_image())
> 
> For Type 2 tests, we check subpage related warnings of btrfs-check, but
> didn't check it for Type 1 test cases.
> 
> In fact, Type 1 test cases are more important, as they involve repair,
> which can generate new tree blocks, and we want to make sure such new
> tree blocks won't cause subpage related warnings.
> 
> This patch will add the extra check for Type 1 test cases.
> 
> And it will make sure the subpage related warnings are really from this
> test case, to prevent false alerts.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  tests/common | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/common b/tests/common
> index 805a447c84ce..a6f75c7ce237 100644
> --- a/tests/common
> +++ b/tests/common
> @@ -423,13 +423,23 @@ check_image()
>  {
>  	local image
>  
> +	tmp_output=$(mktemp --tmpdir btrfs-progs-test-check-image.XXXXXX)
> +
>  	image=$1
>  	echo "testing image $(basename $image)" >> "$RESULTS"
> -	"$TOP/btrfs" check "$image" >> "$RESULTS" 2>&1
> +	"$TOP/btrfs" check "$image" &> "$tmp_output"

So this captures the output but does not store it into the results file

>  	[ $? -eq 0 ] && _fail "btrfs check should have detected corruption"

and when this fails, we lack that information.
>  

> +	cat "$tmp_output" >> "$RESULTS"

So this should probably go before the 'check' call.

> +	# Also make sure no subpage related warnings
> +	check_test_results "$tmp_output" "$testname"
> +
>  	run_check "$TOP/btrfs" check --repair --force "$image"
> -	run_check "$TOP/btrfs" check "$image"
> +	run_check_stdout "$TOP/btrfs" check "$image" &> "$tmp_output"
> +
> +	# Also make sure no subpage related warnings for the repaired image
> +	check_test_results "$tmp_output" "$testname"
> +	rm -f "$tmp_output"
>  }
>  
>  # Extract a usable image from packed formats
> -- 
> 2.32.0

Re: [PATCH 1/3] btrfs-progs: tests: also check subpage warning for type 2 test cases

[David Sterba]

On Wed, Aug 18, 2021 at 02:44:18PM +0800, Qu Wenruo wrote:
> 
> And it will make sure the subpage related warnings are really from this
> test case, to prevent false alerts.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  tests/common | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/common b/tests/common
> index 805a447c84ce..a6f75c7ce237 100644
> --- a/tests/common
> +++ b/tests/common
> @@ -423,13 +423,23 @@ check_image()
>  {
>  	local image
>  
> +	tmp_output=$(mktemp --tmpdir btrfs-progs-test-check-image.XXXXXX)
> +
>  	image=$1
>  	echo "testing image $(basename $image)" >> "$RESULTS"
> -	"$TOP/btrfs" check "$image" >> "$RESULTS" 2>&1
> +	"$TOP/btrfs" check "$image" &> "$tmp_output"
>  	[ $? -eq 0 ] && _fail "btrfs check should have detected corruption"
>  
> +	cat "$tmp_output" >> "$RESULTS"
> +	# Also make sure no subpage related warnings
> +	check_test_results "$tmp_output" "$testname"
> +
>  	run_check "$TOP/btrfs" check --repair --force "$image"
> -	run_check "$TOP/btrfs" check "$image"
> +	run_check_stdout "$TOP/btrfs" check "$image" &> "$tmp_output"
> +
> +	# Also make sure no subpage related warnings for the repaired image
> +	check_test_results "$tmp_output" "$testname"
> +	rm -f "$tmp_output"
>  }

Applied with the following fixup

--- a/tests/common
+++ b/tests/common
@@ -422,15 +422,17 @@ check_dm_target_support()
 check_image()
 {
        local image
+       local tmp_output
 
        tmp_output=$(mktemp --tmpdir btrfs-progs-test-check-image.XXXXXX)
 
        image=$1
        echo "testing image $(basename $image)" >> "$RESULTS"
        "$TOP/btrfs" check "$image" &> "$tmp_output"
-       [ $? -eq 0 ] && _fail "btrfs check should have detected corruption"
-
+       ret=$?
        cat "$tmp_output" >> "$RESULTS"
+       [ "$ret" -eq 0 ] && _fail "btrfs check should have detected corruption"
+
        # Also make sure no subpage related warnings
        check_test_results "$tmp_output" "$testname"
 
@@ -439,7 +441,7 @@ check_image()
 
        # Also make sure no subpage related warnings for the repaired image
        check_test_results "$tmp_output" "$testname"
-       rm -f "$tmp_output"
+       rm -f -- "$tmp_output"
 }
 
 # Extract a usable image from packed formats

[PATCH 2/3] btrfs-progs: tests: don't check subpage related warnings for fsck type 1 tests

[Qu Wenruo]

For fsck tests, we check the subpage warnings for each type 1 test, but
such type 1 tests are mostly read-only tests, and one of the test will
trigger new subpage related warnings (fsck/018).

For subpage related warnings, what we really care are write operations,
including mkfs, btrfs-convert and repair, not those read-only tests.

So skip the subpage warning check for fsck type 1 tests to prevent false
alert of later more strict subpage warnings.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/fsck-tests.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tests/fsck-tests.sh b/tests/fsck-tests.sh
index ed18136f3ab9..70b307ab9629 100755
--- a/tests/fsck-tests.sh
+++ b/tests/fsck-tests.sh
@@ -64,7 +64,14 @@ run_one_test() {
 			fi
 			_fail "test failed for case $(basename $testname)"
 		fi
-		check_test_results "$RESULTS" "$testname"
+		# These tests have overriden check_image() and their
+		# images may have intentional unaligned metadata to trigger
+		# subpage warnings (like fsck/018), skip the check for their
+		# subpage warnings.
+		#
+		# We care subpage related warnings for write operations
+		# (mkfs/convert/repair), not those read-only checks on
+		# pre-crafted images.
 	else
 		# Type 1
 		check_all_images
-- 
2.32.0

[PATCH 3/3] btrfs-progs: require full nodesize alignement for subpage support

[Qu Wenruo]

For the incoming extra page size support for subpage (sectorsize <
PAGE_SIZE) cases, the support for metadata will be a critical point.

Currently for subpage support, we require 64K page size, so that no
matter whatever the nodesize is, it will be contained inside one page.
And we will reject any tree block which crosses page boundary.

But for other page size, especially 16K page size, we must support
nodesize differently.

For nodesize < PAGE_SIZE, we will have the same requirement (tree blocks
can't cross page boundary).
While for nodesize >= PAGE_SIZE, we will require the tree blocks to be
page aligned.

To support such feature, we will make btrfs-check to reports more
subpage related warnings for metadata.

This patch will report any tree block which is not nodesize aligned as a
warning.

Existing mkfs/convert has already make sure all new tree blocks are
nodesize aligned, this is just for older converted filesystems.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c        |  3 ++-
 check/mode-common.h | 22 +++++++++++++---------
 check/mode-lowmem.c |  3 ++-
 tests/common        |  2 +-
 4 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/check/main.c b/check/main.c
index a88518159830..a46fc9daf591 100644
--- a/check/main.c
+++ b/check/main.c
@@ -5410,7 +5410,8 @@ static int process_extent_item(struct btrfs_root *root,
 		return -EIO;
 	}
 	if (metadata)
-		btrfs_check_subpage_eb_alignment(key.objectid, num_bytes);
+		btrfs_check_subpage_eb_alignment(gfs_info, key.objectid,
+						 num_bytes);
 
 	memset(&tmpl, 0, sizeof(tmpl));
 	tmpl.start = key.objectid;
diff --git a/check/mode-common.h b/check/mode-common.h
index cdfb10d58cde..4a3f7741f084 100644
--- a/check/mode-common.h
+++ b/check/mode-common.h
@@ -175,20 +175,24 @@ static inline u32 btrfs_type_to_imode(u8 type)
 int get_extent_item_generation(u64 bytenr, u64 *gen_ret);
 
 /*
- * Check tree block alignment for future subpage support on 64K page system.
+ * Check tree block alignment for future subpage support.
  *
- * Subpage support on 64K page size require one eb to be completely contained
- * by a page. Not allowing a tree block to cross 64K page boundary.
+ * For subpage support, either nodesize is smaller than PAGE_SIZE, then tree
+ * block should not cross page boundary. (A)
+ * Or nodesize >= PAGE_SIZE, then it should be page aligned. (B)
  *
- * Since subpage support is still under development, this check only provides
- * warning.
+ * But here we have no idea the PAGE_SIZE could be, so here we play safe by
+ * requiring all tree blocks to be nodesize aligned.
+ *
+ * For 4K page size system, it always meets condtion (B), thus we don't need
+ * to bother that much.
  */
-static inline void btrfs_check_subpage_eb_alignment(u64 start, u32 len)
+static inline void btrfs_check_subpage_eb_alignment(struct btrfs_fs_info *info,
+						    u64 start, u32 len)
 {
-	if (start / BTRFS_MAX_METADATA_BLOCKSIZE !=
-	    (start + len - 1) / BTRFS_MAX_METADATA_BLOCKSIZE)
+	if (!IS_ALIGNED(start, info->nodesize))
 		warning(
-"tree block [%llu, %llu) crosses 64K page boundary, may cause problem for 64K page system",
+"tree block [%llu, %llu) is not nodesize aligned, may cause problem for 64K page system",
 			start, start + len);
 }
 
diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index 323e66bc4cb1..1116f9872039 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -4265,7 +4265,8 @@ static int check_extent_item(struct btrfs_path *path)
 		err |= CROSSING_STRIPE_BOUNDARY;
 	}
 	if (metadata)
-		btrfs_check_subpage_eb_alignment(key.objectid, nodesize);
+		btrfs_check_subpage_eb_alignment(gfs_info, key.objectid,
+						 nodesize);
 
 	ptr = (unsigned long)(ei + 1);
 
diff --git a/tests/common b/tests/common
index a6f75c7ce237..9dbebd33a9c8 100644
--- a/tests/common
+++ b/tests/common
@@ -850,7 +850,7 @@ check_test_results()
 	local testname="$2"
 
 	# Check subpage related warning
-	if grep -q "crrosses 64K page boundary" "$results"; then
+	if grep -q "not nodesize aligned" "$results"; then
 		_fail "found subpage related warning for case $testname"
 	fi
 }
-- 
2.32.0

Re: [PATCH 0/3] btrfs-progs: make subpage warnings more strict

[David Sterba]

On Wed, Aug 18, 2021 at 02:44:17PM +0800, Qu Wenruo wrote:
> For the incoming support of more page sizes for subpage RW mount, we
> will require tree blocks to be nodesize aligned.
> 
> This patch prepare such restrict warnings for btrfs-check and update
> self-tests to handle them.
> 
> Currently all convert/fsck/misc can pass except one unrelated
> regression, fsck/025, which is caused by "btrfs-progs: Drop the type
> check in init_alloc_chunk_ctl_policy_regular".

I've dropped the test 025 for now so we can do the tests.

> All fsck images except fsck/018 are newer enough to have all their tree
> blocks nodesize aligned, so they won't cause any new warnings.
> But still, for read-only tests, we will skip the subpage warnings as we
> only want to ensure our writes from btrfs-progs won't cause new subpage
> warnings.
> 
> Qu Wenruo (3):
>   btrfs-progs: tests: also check subpage warning for type 2 test cases
>   btrfs-progs: tests: don't check subpage related warnings for fsck type
>     1 tests
>   btrfs-progs: require full nodesize alignement for subpage support

Added to devel, thanks.