diff mbox series

[v2,08/10] generic/574: test multiple Merkle tree block sizes

Message ID 20221223010554.281679-9-ebiggers@kernel.org (mailing list archive)
State Superseded
Headers show
Series xfstests: update verity tests for non-4K block and page size | expand

Commit Message

Eric Biggers Dec. 23, 2022, 1:05 a.m. UTC 
From: Eric Biggers <ebiggers@google.com>

Instead of only testing 4K Merkle tree blocks, test FSV_BLOCK_SIZE, and
also other sizes if they happen to be supported.  This allows this test
to run in cases where it couldn't before and improves test coverage in
cases where it did run before.

Given that the list of Merkle tree block sizes that will actually work
is not fixed, this required reworking the test to not rely on the .out
file so heavily.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 tests/generic/574     | 177 ++++++++++++++++++++++++++----------------
 tests/generic/574.out |  83 ++------------------
 2 files changed, 114 insertions(+), 146 deletions(-)

Comments

Zorro Lang Dec. 25, 2022, 12:46 p.m. UTC | #1 
On Thu, Dec 22, 2022 at 05:05:52PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Instead of only testing 4K Merkle tree blocks, test FSV_BLOCK_SIZE, and
> also other sizes if they happen to be supported.  This allows this test
> to run in cases where it couldn't before and improves test coverage in
> cases where it did run before.
> 
> Given that the list of Merkle tree block sizes that will actually work
> is not fixed, this required reworking the test to not rely on the .out
> file so heavily.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  tests/generic/574     | 177 ++++++++++++++++++++++++++----------------
>  tests/generic/574.out |  83 ++------------------
>  2 files changed, 114 insertions(+), 146 deletions(-)
> 
> diff --git a/tests/generic/574 b/tests/generic/574
> index fd4488c9..8f7923ba 100755
> --- a/tests/generic/574
> +++ b/tests/generic/574
> @@ -37,18 +37,19 @@ fsv_file=$SCRATCH_MNT/file.fsv
>  
>  setup_zeroed_file()
>  {
> -	local len=$1
> -	local sparse=$2
> +	local block_size=$1
> +	local file_len=$2
> +	local sparse=$3
>  
>  	if $sparse; then
> -		dd if=/dev/zero of=$fsv_orig_file bs=1 count=0 seek=$len \
> +		dd if=/dev/zero of=$fsv_orig_file bs=1 count=0 seek=$file_len \
>  			status=none
>  	else
> -		head -c $len /dev/zero > $fsv_orig_file
> +		head -c $file_len /dev/zero > $fsv_orig_file
>  	fi
>  	cp $fsv_orig_file $fsv_file
> -	_fsv_enable $fsv_file
> -	md5sum $fsv_file |& _filter_scratch
> +	_fsv_enable $fsv_file --block-size=$block_size
> +	cmp $fsv_orig_file $fsv_file
>  }
>  
>  filter_sigbus()
> @@ -66,63 +67,84 @@ round_up_to_page_boundary()
>  
>  corruption_test()
>  {
> -	local file_len=$1
> -	local zap_offset=$2
> -	local zap_len=$3
> -	local is_merkle_tree=${4:-false} # if true, zap tree instead of data
> -	local use_sparse_file=${5:-false}
> +	local block_size=$1
> +	local file_len=$2
> +	local zap_offset=$3
> +	local zap_len=$4
> +	local is_merkle_tree=${5:-false} # if true, zap tree instead of data
> +	local use_sparse_file=${6:-false}
>  	local page_aligned_eof=$(round_up_to_page_boundary $file_len)
> -	local measurement
> +
> +	local paramstr="block_size=$block_size"
> +	paramstr+=", file_len=$file_len"
> +	paramstr+=", zap_offset=$zap_offset"
> +	paramstr+=", zap_len=$zap_len"
> +	paramstr+=", is_merkle_tree=$is_merkle_tree"
> +	paramstr+=", use_sparse_file=$use_sparse_file"
>  
>  	if $is_merkle_tree; then
>  		local corrupt_func=_fsv_scratch_corrupt_merkle_tree
>  	else
>  		local corrupt_func=_fsv_scratch_corrupt_bytes
>  	fi
> +	local fs_block_size=$(_get_block_size $SCRATCH_MNT)
>  
> -	local msg="Corruption test:"
> -	msg+=" file_len=$file_len"
> -	if $use_sparse_file; then
> -		msg+=" (sparse)"
> -	fi
> -	msg+=" zap_offset=$zap_offset"
> -	if $is_merkle_tree; then
> -		msg+=" (in Merkle tree)"
> -	fi
> -	msg+=" zap_len=$zap_len"
> +	rm -rf "${SCRATCH_MNT:?}"/*
> +	setup_zeroed_file $block_size $file_len $use_sparse_file
>  
> -	_fsv_scratch_begin_subtest "$msg"
> -	setup_zeroed_file $file_len $use_sparse_file
> -	cmp $fsv_file $fsv_orig_file
> -	echo "Corrupting bytes..."
> +	# Corrupt part of the file (data or Merkle tree).
>  	head -c $zap_len /dev/zero | tr '\0' X \
>  		| $corrupt_func $fsv_file $zap_offset
>  
> -	echo "Validating corruption (reading full file)..."
> +	# Reading the full file with buffered I/O should fail.
>  	_scratch_cycle_mount
> -	md5sum $fsv_file |& _filter_scratch
> +	if cat $fsv_file >/dev/null 2>$tmp.out; then
> +		echo "Unexpectedly was able to read full file ($paramstr)"
> +	elif ! grep -q 'Input/output error' $tmp.out; then
> +		echo "Wrong error reading full file ($paramstr):"
> +		cat $tmp.out
> +	fi
>  
> -	echo "Validating corruption (direct I/O)..."
> +	# Reading the full file with direct I/O should fail.
>  	_scratch_cycle_mount
> -	dd if=$fsv_file bs=$FSV_BLOCK_SIZE iflag=direct status=none \
> -		of=/dev/null |& _filter_scratch
> +	if dd if=$fsv_file bs=$fs_block_size iflag=direct status=none \
> +		of=/dev/null 2>$tmp.out
> +	then
> +		echo "Unexpectedly was able to read full file with DIO ($paramstr)"
> +	elif ! grep -q 'Input/output error' $tmp.out; then
> +		echo "Wrong error reading full file with DIO ($paramstr):"
> +		cat $tmp.out
> +	fi
>  
> -	if (( zap_offset < file_len )) && ! $is_merkle_tree; then
> -		echo "Validating corruption (reading just corrupted part)..."
> -		dd if=$fsv_file bs=1 skip=$zap_offset count=$zap_len \
> -			of=/dev/null status=none |& _filter_scratch
> +	# Reading just the corrupted part of the file should fail.
> +	if ! $is_merkle_tree; then
> +		if dd if=$fsv_file bs=1 skip=$zap_offset count=$zap_len \
> +			of=/dev/null status=none 2>$tmp.out; then
> +			echo "Unexpectedly was able to read corrupted part ($paramstr)"
> +		elif ! grep -q 'Input/output error' $tmp.out; then
> +			echo "Wrong error reading corrupted part ($paramstr):"
> +			cat $tmp.out
> +		fi
>  	fi
>  
> -	echo "Validating corruption (reading full file via mmap)..."
> +	# Reading the full file via mmap should fail.
>  	bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $fsv_file \
>  		-c 'mmap -r 0 $page_aligned_eof' \
> -		-c 'mread 0 $file_len'" |& filter_sigbus
> +		-c 'mread 0 $file_len'" >/dev/null 2>$tmp.out
> +	if ! grep -q 'Bus error' $tmp.out; then
> +		echo "Didn't see SIGBUS when reading file via mmap"

Hmm... will sigbus error really be output to stderr? From a testing, the
generic/574 fails as:

# ./check -s simpledev generic/574
SECTION       -- simpledev
FSTYP         -- ext4
PLATFORM      -- Linux/x86_64 xx-xxxxxx-xxx 6.1.0-rc3 #5 SMP PREEMPT_DYNAMIC Tue Nov  1 01:08:52 CST 2022
MKFS_OPTIONS  -- -F /dev/sdb
MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/sdb /mnt/scratch

generic/574       - output mismatch (see /root/git/xfstests/results//simpledev/generic/574.out.bad)
    --- tests/generic/574.out   2022-12-25 20:02:41.609104749 +0800
    +++ /root/git/xfstests/results//simpledev/generic/574.out.bad       2022-12-25 20:21:57.430719504 +0800
    @@ -1,6 +1,32 @@
     QA output created by 574
     
     # Testing block_size=FSV_BLOCK_SIZE
    +/root/git/xfstests/tests/generic/574: line 69: 1949533 Bus error               (core dumped) bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $fsv_file            -c 'mmap -r 0 $page_aligned_eof'               -c 'mread 0 $file_len'" > /dev/null 2> $tmp.out
    +Didn't see SIGBUS when reading file via mmap
    +/root/git/xfstests/tests/generic/574: line 69: 1949544 Bus error               (core dumped) bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $fsv_file                    -c 'mmap -r 0 $page_aligned_eof'                       -c 'mread $zap_offset $zap_len'" > /dev/null 2> $tmp.out
    +Didn't see SIGBUS when reading corrupted part via mmap
    ...
    (Run 'diff -u /root/git/xfstests/tests/generic/574.out /root/git/xfstests/results//simpledev/generic/574.out.bad'  to see the entire diff)
Ran: generic/574
Failures: generic/574
Failed 1 of 1 tests

Thanks,
Zorro

> +		cat $tmp.out
> +	fi
>  
> +	# Reading just the corrupted part via mmap should fail.
>  	if ! $is_merkle_tree; then
> -		echo "Validating corruption (reading just corrupted part via mmap)..."
>  		bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $fsv_file \
>  			-c 'mmap -r 0 $page_aligned_eof' \
> -			-c 'mread $zap_offset $zap_len'" |& filter_sigbus
> +			-c 'mread $zap_offset $zap_len'" >/dev/null 2>$tmp.out
> +		if ! grep -q 'Bus error' $tmp.out; then
> +			echo "Didn't see SIGBUS when reading corrupted part via mmap"
> +			cat $tmp.out
> +		fi
>  	fi
>  }
>  
> @@ -131,18 +153,18 @@ corruption_test()
>  # return zeros in the last block past EOF, regardless of the contents on
>  # disk. In the former, corruption should be detected and result in SIGBUS,
>  # while in the latter we would expect zeros past EOF, but no error.
> -corrupt_eof_block_test() {
> -	local file_len=$1
> -	local zap_len=$2
> +corrupt_eof_block_test()
> +{
> +	local block_size=$1
> +	local file_len=$2
> +	local zap_len=$3
>  	local page_aligned_eof=$(round_up_to_page_boundary $file_len)
> -	_fsv_scratch_begin_subtest "Corruption test: EOF block"
> -	setup_zeroed_file $file_len false
> -	cmp $fsv_file $fsv_orig_file
> -	echo "Corrupting bytes..."
> +
> +	rm -rf "${SCRATCH_MNT:?}"/*
> +	setup_zeroed_file $block_size $file_len false
>  	head -c $zap_len /dev/zero | tr '\0' X \
>  		| _fsv_scratch_corrupt_bytes $fsv_file $file_len
>  
> -	echo "Reading eof block via mmap into a temporary file..."
>  	bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $fsv_file \
>  		-c 'mmap -r 0 $page_aligned_eof' \
>  		-c 'mread -v $file_len $zap_len'" \
> @@ -153,30 +175,49 @@ corrupt_eof_block_test() {
>  		-c "mmap -r 0 $page_aligned_eof" \
>  		-c "mread -v $file_len $zap_len" >$tmp.eof_zero_read
>  
> -	echo "Checking for SIGBUS or zeros..."
> -	grep -q -e '^Bus error$' $tmp.eof_block_read \
> -		|| diff $tmp.eof_block_read $tmp.eof_zero_read \
> -		&& echo "OK"
> +	grep -q -e '^Bus error$' $tmp.eof_block_read || \
> +		diff $tmp.eof_block_read $tmp.eof_zero_read
>  }
>  
> -# Note: these tests just overwrite some bytes without checking their original
> -# values.  Therefore, make sure to overwrite at least 5 or so bytes, to make it
> -# nearly guaranteed that there will be a change -- even when the test file is
> -# encrypted due to the test_dummy_encryption mount option being specified.
> -
> -corruption_test 131072 0 5
> -corruption_test 131072 4091 5
> -corruption_test 131072 65536 65536
> -corruption_test 131072 131067 5
> -
> -corrupt_eof_block_test 130999 72
> -
> -# Merkle tree corruption.
> -corruption_test 200000 100 10 true
> +test_block_size()
> +{
> +	local block_size=$1
> +
> +	# Note: these tests just overwrite some bytes without checking their
> +	# original values.  Therefore, make sure to overwrite at least 5 or so
> +	# bytes, to make it nearly guaranteed that there will be a change --
> +	# even when the test file is encrypted due to the test_dummy_encryption
> +	# mount option being specified.
> +	corruption_test $block_size 131072 0 5
> +	corruption_test $block_size 131072 4091 5
> +	corruption_test $block_size 131072 65536 65536
> +	corruption_test $block_size 131072 131067 5
> +
> +	corrupt_eof_block_test $block_size 130999 72
> +
> +	# Merkle tree corruption.
> +	corruption_test $block_size 200000 100 10 true
> +
> +	# Sparse file.  Corrupting the Merkle tree should still cause reads to
> +	# fail, i.e. the filesystem must verify holes.
> +	corruption_test $block_size 200000 100 10 true true
> +}
>  
> -# Sparse file.  Corrupting the Merkle tree should still cause reads to fail,
> -# i.e. the filesystem must verify holes.
> -corruption_test 200000 100 10 true true
> +# Always test FSV_BLOCK_SIZE.  Also test some other block sizes if they happen
> +# to be supported.
> +_fsv_scratch_begin_subtest "Testing block_size=FSV_BLOCK_SIZE"
> +test_block_size $FSV_BLOCK_SIZE
> +for block_size in 1024 4096 16384 65536; do
> +	_fsv_scratch_begin_subtest "Testing block_size=$block_size if supported"
> +	if (( block_size == FSV_BLOCK_SIZE )); then
> +		continue # Skip redundant test case.
> +	fi
> +	if ! _fsv_can_enable $fsv_file --block-size=$block_size; then
> +		echo "block_size=$block_size is unsupported" >> $seqres.full
> +		continue
> +	fi
> +	test_block_size $block_size
> +done
>  
>  # success, all done
>  status=0
> diff --git a/tests/generic/574.out b/tests/generic/574.out
> index d40d1263..3ed57f1c 100644
> --- a/tests/generic/574.out
> +++ b/tests/generic/574.out
> @@ -1,84 +1,11 @@
>  QA output created by 574
>  
> -# Corruption test: file_len=131072 zap_offset=0 zap_len=5
> -0dfbe8aa4c20b52e1b8bf3cb6cbdf193  SCRATCH_MNT/file.fsv
> -Corrupting bytes...
> -Validating corruption (reading full file)...
> -md5sum: SCRATCH_MNT/file.fsv: Input/output error
> -Validating corruption (direct I/O)...
> -dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
> -Validating corruption (reading just corrupted part)...
> -dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
> -Validating corruption (reading full file via mmap)...
> -Bus error
> -Validating corruption (reading just corrupted part via mmap)...
> -Bus error
> +# Testing block_size=FSV_BLOCK_SIZE
>  
> -# Corruption test: file_len=131072 zap_offset=4091 zap_len=5
> -0dfbe8aa4c20b52e1b8bf3cb6cbdf193  SCRATCH_MNT/file.fsv
> -Corrupting bytes...
> -Validating corruption (reading full file)...
> -md5sum: SCRATCH_MNT/file.fsv: Input/output error
> -Validating corruption (direct I/O)...
> -dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
> -Validating corruption (reading just corrupted part)...
> -dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
> -Validating corruption (reading full file via mmap)...
> -Bus error
> -Validating corruption (reading just corrupted part via mmap)...
> -Bus error
> +# Testing block_size=1024 if supported
>  
> -# Corruption test: file_len=131072 zap_offset=65536 zap_len=65536
> -0dfbe8aa4c20b52e1b8bf3cb6cbdf193  SCRATCH_MNT/file.fsv
> -Corrupting bytes...
> -Validating corruption (reading full file)...
> -md5sum: SCRATCH_MNT/file.fsv: Input/output error
> -Validating corruption (direct I/O)...
> -dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
> -Validating corruption (reading just corrupted part)...
> -dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
> -Validating corruption (reading full file via mmap)...
> -Bus error
> -Validating corruption (reading just corrupted part via mmap)...
> -Bus error
> +# Testing block_size=4096 if supported
>  
> -# Corruption test: file_len=131072 zap_offset=131067 zap_len=5
> -0dfbe8aa4c20b52e1b8bf3cb6cbdf193  SCRATCH_MNT/file.fsv
> -Corrupting bytes...
> -Validating corruption (reading full file)...
> -md5sum: SCRATCH_MNT/file.fsv: Input/output error
> -Validating corruption (direct I/O)...
> -dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
> -Validating corruption (reading just corrupted part)...
> -dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
> -Validating corruption (reading full file via mmap)...
> -Bus error
> -Validating corruption (reading just corrupted part via mmap)...
> -Bus error
> +# Testing block_size=16384 if supported
>  
> -# Corruption test: EOF block
> -f5cca0d7fbb8b02bc6118a9954d5d306  SCRATCH_MNT/file.fsv
> -Corrupting bytes...
> -Reading eof block via mmap into a temporary file...
> -Checking for SIGBUS or zeros...
> -OK
> -
> -# Corruption test: file_len=200000 zap_offset=100 (in Merkle tree) zap_len=10
> -4a1e4325031b13f933ac4f1db9ecb63f  SCRATCH_MNT/file.fsv
> -Corrupting bytes...
> -Validating corruption (reading full file)...
> -md5sum: SCRATCH_MNT/file.fsv: Input/output error
> -Validating corruption (direct I/O)...
> -dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
> -Validating corruption (reading full file via mmap)...
> -Bus error
> -
> -# Corruption test: file_len=200000 (sparse) zap_offset=100 (in Merkle tree) zap_len=10
> -4a1e4325031b13f933ac4f1db9ecb63f  SCRATCH_MNT/file.fsv
> -Corrupting bytes...
> -Validating corruption (reading full file)...
> -md5sum: SCRATCH_MNT/file.fsv: Input/output error
> -Validating corruption (direct I/O)...
> -dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
> -Validating corruption (reading full file via mmap)...
> -Bus error
> +# Testing block_size=65536 if supported
> -- 
> 2.39.0
>
Eric Biggers Dec. 26, 2022, 5:21 a.m. UTC | #2 
On Sun, Dec 25, 2022 at 08:46:00PM +0800, Zorro Lang wrote:
> > +	# Reading the full file via mmap should fail.
> >  	bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $fsv_file \
> >  		-c 'mmap -r 0 $page_aligned_eof' \
> > -		-c 'mread 0 $file_len'" |& filter_sigbus
> > +		-c 'mread 0 $file_len'" >/dev/null 2>$tmp.out
> > +	if ! grep -q 'Bus error' $tmp.out; then
> > +		echo "Didn't see SIGBUS when reading file via mmap"
> 
> Hmm... will sigbus error really be output to stderr? From a testing, the
> generic/574 fails as:
> 
> # ./check -s simpledev generic/574
> SECTION       -- simpledev
> FSTYP         -- ext4
> PLATFORM      -- Linux/x86_64 xx-xxxxxx-xxx 6.1.0-rc3 #5 SMP PREEMPT_DYNAMIC Tue Nov  1 01:08:52 CST 2022
> MKFS_OPTIONS  -- -F /dev/sdb
> MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/sdb /mnt/scratch
> 
> generic/574       - output mismatch (see /root/git/xfstests/results//simpledev/generic/574.out.bad)
>     --- tests/generic/574.out   2022-12-25 20:02:41.609104749 +0800
>     +++ /root/git/xfstests/results//simpledev/generic/574.out.bad       2022-12-25 20:21:57.430719504 +0800
>     @@ -1,6 +1,32 @@
>      QA output created by 574
>      
>      # Testing block_size=FSV_BLOCK_SIZE
>     +/root/git/xfstests/tests/generic/574: line 69: 1949533 Bus error               (core dumped) bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $fsv_file            -c 'mmap -r 0 $page_aligned_eof'               -c 'mread 0 $file_len'" > /dev/null 2> $tmp.out
>     +Didn't see SIGBUS when reading file via mmap

This test passes for me both before and after this patch series.

Both before and after, the way this is supposed to work is that in:

	bash -c "trap '' SIGBUS; command_that_exits_with_sigbus"

... bash should print "Bus error" to stderr due to
'command_that_exits_with_sigbus'.  That "Bus error" is then redirected.  Before
it was redirected to a pipeline; after it is redirected to a file.

I think what's happening is that the version of bash your system has is not
forking before exec'ing 'command_that_exits_with_sigbus'.  As a result, "Bus
error" is printed by the *parent* bash process instead, skipping any redirection
in the shell script.

Apparently skipping fork is a real thing in bash, and different versions of bash
have had subtly different conditions for enabling it.  So this seems plausible.

Adding an extra command after 'command_that_exits_with_sigbus' should fix this:

	bash -c "trap '' SIGBUS; command_that_exits_with_sigbus; true"

The joy of working with a shell scripting system where everyone has different
versions of everything installed...

- Eric
Theodore Ts'o Dec. 28, 2022, 12:50 p.m. UTC | #3 
On Sun, Dec 25, 2022 at 09:21:34PM -0800, Eric Biggers wrote:
> The joy of working with a shell scripting system where everyone has different
> versions of everything installed...

And this is why some large companies are still stuck on Bash v3
because the SRE's are afraid that untold number of shell scripts will
break in subtle and entertaining ways, leading to customer outages...

      	 	    		       	       	  - Ted
Zorro Lang Dec. 29, 2022, 4:32 p.m. UTC | #4 
On Sun, Dec 25, 2022 at 09:21:34PM -0800, Eric Biggers wrote:
> On Sun, Dec 25, 2022 at 08:46:00PM +0800, Zorro Lang wrote:
> > > +	# Reading the full file via mmap should fail.
> > >  	bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $fsv_file \
> > >  		-c 'mmap -r 0 $page_aligned_eof' \
> > > -		-c 'mread 0 $file_len'" |& filter_sigbus
> > > +		-c 'mread 0 $file_len'" >/dev/null 2>$tmp.out
> > > +	if ! grep -q 'Bus error' $tmp.out; then
> > > +		echo "Didn't see SIGBUS when reading file via mmap"
> > 
> > Hmm... will sigbus error really be output to stderr? From a testing, the
> > generic/574 fails as:
> > 
> > # ./check -s simpledev generic/574
> > SECTION       -- simpledev
> > FSTYP         -- ext4
> > PLATFORM      -- Linux/x86_64 xx-xxxxxx-xxx 6.1.0-rc3 #5 SMP PREEMPT_DYNAMIC Tue Nov  1 01:08:52 CST 2022
> > MKFS_OPTIONS  -- -F /dev/sdb
> > MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/sdb /mnt/scratch
> > 
> > generic/574       - output mismatch (see /root/git/xfstests/results//simpledev/generic/574.out.bad)
> >     --- tests/generic/574.out   2022-12-25 20:02:41.609104749 +0800
> >     +++ /root/git/xfstests/results//simpledev/generic/574.out.bad       2022-12-25 20:21:57.430719504 +0800
> >     @@ -1,6 +1,32 @@
> >      QA output created by 574
> >      
> >      # Testing block_size=FSV_BLOCK_SIZE
> >     +/root/git/xfstests/tests/generic/574: line 69: 1949533 Bus error               (core dumped) bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $fsv_file            -c 'mmap -r 0 $page_aligned_eof'               -c 'mread 0 $file_len'" > /dev/null 2> $tmp.out
> >     +Didn't see SIGBUS when reading file via mmap
> 
> This test passes for me both before and after this patch series.
> 
> Both before and after, the way this is supposed to work is that in:
> 
> 	bash -c "trap '' SIGBUS; command_that_exits_with_sigbus"
> 
> ... bash should print "Bus error" to stderr due to
> 'command_that_exits_with_sigbus'.  That "Bus error" is then redirected.  Before
> it was redirected to a pipeline; after it is redirected to a file.
> 
> I think what's happening is that the version of bash your system has is not
> forking before exec'ing 'command_that_exits_with_sigbus'.  As a result, "Bus
> error" is printed by the *parent* bash process instead, skipping any redirection
> in the shell script.
> 
> Apparently skipping fork is a real thing in bash, and different versions of bash
> have had subtly different conditions for enabling it.  So this seems plausible.
> 
> Adding an extra command after 'command_that_exits_with_sigbus' should fix this:
> 
> 	bash -c "trap '' SIGBUS; command_that_exits_with_sigbus; true"

Thanks for this explanation, I think you're right!

I'm not sure if it's a bug of bash. If it's not a bug, I think we can do this
change (add a true) to avoid that failure. If it's a bug, hmmm..., I think we'd
better to avoid that failure too, due to we don't test for bash :-/

How about your resend this single patch (by version 2.1), to fix this problem.
Other patches looks good to me, I'd like to merge this patchset this weekend.

Thanks,
Zorro

> 
> The joy of working with a shell scripting system where everyone has different
> versions of everything installed...
> 
> - Eric
>
Eric Biggers Dec. 29, 2022, 11:47 p.m. UTC | #5 
On Fri, Dec 30, 2022 at 12:32:15AM +0800, Zorro Lang wrote:
> > 
> > This test passes for me both before and after this patch series.
> > 
> > Both before and after, the way this is supposed to work is that in:
> > 
> > 	bash -c "trap '' SIGBUS; command_that_exits_with_sigbus"
> > 
> > ... bash should print "Bus error" to stderr due to
> > 'command_that_exits_with_sigbus'.  That "Bus error" is then redirected.  Before
> > it was redirected to a pipeline; after it is redirected to a file.
> > 
> > I think what's happening is that the version of bash your system has is not
> > forking before exec'ing 'command_that_exits_with_sigbus'.  As a result, "Bus
> > error" is printed by the *parent* bash process instead, skipping any redirection
> > in the shell script.
> > 
> > Apparently skipping fork is a real thing in bash, and different versions of bash
> > have had subtly different conditions for enabling it.  So this seems plausible.
> > 
> > Adding an extra command after 'command_that_exits_with_sigbus' should fix this:
> > 
> > 	bash -c "trap '' SIGBUS; command_that_exits_with_sigbus; true"
> 
> Thanks for this explanation, I think you're right!
> 
> I'm not sure if it's a bug of bash. If it's not a bug, I think we can do this
> change (add a true) to avoid that failure. If it's a bug, hmmm..., I think we'd
> better to avoid that failure too, due to we don't test for bash :-/
> 
> How about your resend this single patch (by version 2.1), to fix this problem.
> Other patches looks good to me, I'd like to merge this patchset this weekend.
> 
> Thanks,
> Zorro

I expect that the bash developers wouldn't consider this to be a bug, since it
could only be fixed by removing the "skip fork" optimization entirely.

Anyway, the workaround I suggested is simple enough.

I just resent the whole series, since it's confusing to have new versions of
patches within existing series versions.  But only this one patch changed.

Thanks!

- Eric
diff mbox series

Patch

diff --git a/tests/generic/574 b/tests/generic/574
index fd4488c9..8f7923ba 100755
--- a/tests/generic/574
+++ b/tests/generic/574
@@ -37,18 +37,19 @@  fsv_file=$SCRATCH_MNT/file.fsv
 
 setup_zeroed_file()
 {
-	local len=$1
-	local sparse=$2
+	local block_size=$1
+	local file_len=$2
+	local sparse=$3
 
 	if $sparse; then
-		dd if=/dev/zero of=$fsv_orig_file bs=1 count=0 seek=$len \
+		dd if=/dev/zero of=$fsv_orig_file bs=1 count=0 seek=$file_len \
 			status=none
 	else
-		head -c $len /dev/zero > $fsv_orig_file
+		head -c $file_len /dev/zero > $fsv_orig_file
 	fi
 	cp $fsv_orig_file $fsv_file
-	_fsv_enable $fsv_file
-	md5sum $fsv_file |& _filter_scratch
+	_fsv_enable $fsv_file --block-size=$block_size
+	cmp $fsv_orig_file $fsv_file
 }
 
 filter_sigbus()
@@ -66,63 +67,84 @@  round_up_to_page_boundary()
 
 corruption_test()
 {
-	local file_len=$1
-	local zap_offset=$2
-	local zap_len=$3
-	local is_merkle_tree=${4:-false} # if true, zap tree instead of data
-	local use_sparse_file=${5:-false}
+	local block_size=$1
+	local file_len=$2
+	local zap_offset=$3
+	local zap_len=$4
+	local is_merkle_tree=${5:-false} # if true, zap tree instead of data
+	local use_sparse_file=${6:-false}
 	local page_aligned_eof=$(round_up_to_page_boundary $file_len)
-	local measurement
+
+	local paramstr="block_size=$block_size"
+	paramstr+=", file_len=$file_len"
+	paramstr+=", zap_offset=$zap_offset"
+	paramstr+=", zap_len=$zap_len"
+	paramstr+=", is_merkle_tree=$is_merkle_tree"
+	paramstr+=", use_sparse_file=$use_sparse_file"
 
 	if $is_merkle_tree; then
 		local corrupt_func=_fsv_scratch_corrupt_merkle_tree
 	else
 		local corrupt_func=_fsv_scratch_corrupt_bytes
 	fi
+	local fs_block_size=$(_get_block_size $SCRATCH_MNT)
 
-	local msg="Corruption test:"
-	msg+=" file_len=$file_len"
-	if $use_sparse_file; then
-		msg+=" (sparse)"
-	fi
-	msg+=" zap_offset=$zap_offset"
-	if $is_merkle_tree; then
-		msg+=" (in Merkle tree)"
-	fi
-	msg+=" zap_len=$zap_len"
+	rm -rf "${SCRATCH_MNT:?}"/*
+	setup_zeroed_file $block_size $file_len $use_sparse_file
 
-	_fsv_scratch_begin_subtest "$msg"
-	setup_zeroed_file $file_len $use_sparse_file
-	cmp $fsv_file $fsv_orig_file
-	echo "Corrupting bytes..."
+	# Corrupt part of the file (data or Merkle tree).
 	head -c $zap_len /dev/zero | tr '\0' X \
 		| $corrupt_func $fsv_file $zap_offset
 
-	echo "Validating corruption (reading full file)..."
+	# Reading the full file with buffered I/O should fail.
 	_scratch_cycle_mount
-	md5sum $fsv_file |& _filter_scratch
+	if cat $fsv_file >/dev/null 2>$tmp.out; then
+		echo "Unexpectedly was able to read full file ($paramstr)"
+	elif ! grep -q 'Input/output error' $tmp.out; then
+		echo "Wrong error reading full file ($paramstr):"
+		cat $tmp.out
+	fi
 
-	echo "Validating corruption (direct I/O)..."
+	# Reading the full file with direct I/O should fail.
 	_scratch_cycle_mount
-	dd if=$fsv_file bs=$FSV_BLOCK_SIZE iflag=direct status=none \
-		of=/dev/null |& _filter_scratch
+	if dd if=$fsv_file bs=$fs_block_size iflag=direct status=none \
+		of=/dev/null 2>$tmp.out
+	then
+		echo "Unexpectedly was able to read full file with DIO ($paramstr)"
+	elif ! grep -q 'Input/output error' $tmp.out; then
+		echo "Wrong error reading full file with DIO ($paramstr):"
+		cat $tmp.out
+	fi
 
-	if (( zap_offset < file_len )) && ! $is_merkle_tree; then
-		echo "Validating corruption (reading just corrupted part)..."
-		dd if=$fsv_file bs=1 skip=$zap_offset count=$zap_len \
-			of=/dev/null status=none |& _filter_scratch
+	# Reading just the corrupted part of the file should fail.
+	if ! $is_merkle_tree; then
+		if dd if=$fsv_file bs=1 skip=$zap_offset count=$zap_len \
+			of=/dev/null status=none 2>$tmp.out; then
+			echo "Unexpectedly was able to read corrupted part ($paramstr)"
+		elif ! grep -q 'Input/output error' $tmp.out; then
+			echo "Wrong error reading corrupted part ($paramstr):"
+			cat $tmp.out
+		fi
 	fi
 
-	echo "Validating corruption (reading full file via mmap)..."
+	# Reading the full file via mmap should fail.
 	bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $fsv_file \
 		-c 'mmap -r 0 $page_aligned_eof' \
-		-c 'mread 0 $file_len'" |& filter_sigbus
+		-c 'mread 0 $file_len'" >/dev/null 2>$tmp.out
+	if ! grep -q 'Bus error' $tmp.out; then
+		echo "Didn't see SIGBUS when reading file via mmap"
+		cat $tmp.out
+	fi
 
+	# Reading just the corrupted part via mmap should fail.
 	if ! $is_merkle_tree; then
-		echo "Validating corruption (reading just corrupted part via mmap)..."
 		bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $fsv_file \
 			-c 'mmap -r 0 $page_aligned_eof' \
-			-c 'mread $zap_offset $zap_len'" |& filter_sigbus
+			-c 'mread $zap_offset $zap_len'" >/dev/null 2>$tmp.out
+		if ! grep -q 'Bus error' $tmp.out; then
+			echo "Didn't see SIGBUS when reading corrupted part via mmap"
+			cat $tmp.out
+		fi
 	fi
 }
 
@@ -131,18 +153,18 @@  corruption_test()
 # return zeros in the last block past EOF, regardless of the contents on
 # disk. In the former, corruption should be detected and result in SIGBUS,
 # while in the latter we would expect zeros past EOF, but no error.
-corrupt_eof_block_test() {
-	local file_len=$1
-	local zap_len=$2
+corrupt_eof_block_test()
+{
+	local block_size=$1
+	local file_len=$2
+	local zap_len=$3
 	local page_aligned_eof=$(round_up_to_page_boundary $file_len)
-	_fsv_scratch_begin_subtest "Corruption test: EOF block"
-	setup_zeroed_file $file_len false
-	cmp $fsv_file $fsv_orig_file
-	echo "Corrupting bytes..."
+
+	rm -rf "${SCRATCH_MNT:?}"/*
+	setup_zeroed_file $block_size $file_len false
 	head -c $zap_len /dev/zero | tr '\0' X \
 		| _fsv_scratch_corrupt_bytes $fsv_file $file_len
 
-	echo "Reading eof block via mmap into a temporary file..."
 	bash -c "trap '' SIGBUS; $XFS_IO_PROG -r $fsv_file \
 		-c 'mmap -r 0 $page_aligned_eof' \
 		-c 'mread -v $file_len $zap_len'" \
@@ -153,30 +175,49 @@  corrupt_eof_block_test() {
 		-c "mmap -r 0 $page_aligned_eof" \
 		-c "mread -v $file_len $zap_len" >$tmp.eof_zero_read
 
-	echo "Checking for SIGBUS or zeros..."
-	grep -q -e '^Bus error$' $tmp.eof_block_read \
-		|| diff $tmp.eof_block_read $tmp.eof_zero_read \
-		&& echo "OK"
+	grep -q -e '^Bus error$' $tmp.eof_block_read || \
+		diff $tmp.eof_block_read $tmp.eof_zero_read
 }
 
-# Note: these tests just overwrite some bytes without checking their original
-# values.  Therefore, make sure to overwrite at least 5 or so bytes, to make it
-# nearly guaranteed that there will be a change -- even when the test file is
-# encrypted due to the test_dummy_encryption mount option being specified.
-
-corruption_test 131072 0 5
-corruption_test 131072 4091 5
-corruption_test 131072 65536 65536
-corruption_test 131072 131067 5
-
-corrupt_eof_block_test 130999 72
-
-# Merkle tree corruption.
-corruption_test 200000 100 10 true
+test_block_size()
+{
+	local block_size=$1
+
+	# Note: these tests just overwrite some bytes without checking their
+	# original values.  Therefore, make sure to overwrite at least 5 or so
+	# bytes, to make it nearly guaranteed that there will be a change --
+	# even when the test file is encrypted due to the test_dummy_encryption
+	# mount option being specified.
+	corruption_test $block_size 131072 0 5
+	corruption_test $block_size 131072 4091 5
+	corruption_test $block_size 131072 65536 65536
+	corruption_test $block_size 131072 131067 5
+
+	corrupt_eof_block_test $block_size 130999 72
+
+	# Merkle tree corruption.
+	corruption_test $block_size 200000 100 10 true
+
+	# Sparse file.  Corrupting the Merkle tree should still cause reads to
+	# fail, i.e. the filesystem must verify holes.
+	corruption_test $block_size 200000 100 10 true true
+}
 
-# Sparse file.  Corrupting the Merkle tree should still cause reads to fail,
-# i.e. the filesystem must verify holes.
-corruption_test 200000 100 10 true true
+# Always test FSV_BLOCK_SIZE.  Also test some other block sizes if they happen
+# to be supported.
+_fsv_scratch_begin_subtest "Testing block_size=FSV_BLOCK_SIZE"
+test_block_size $FSV_BLOCK_SIZE
+for block_size in 1024 4096 16384 65536; do
+	_fsv_scratch_begin_subtest "Testing block_size=$block_size if supported"
+	if (( block_size == FSV_BLOCK_SIZE )); then
+		continue # Skip redundant test case.
+	fi
+	if ! _fsv_can_enable $fsv_file --block-size=$block_size; then
+		echo "block_size=$block_size is unsupported" >> $seqres.full
+		continue
+	fi
+	test_block_size $block_size
+done
 
 # success, all done
 status=0
diff --git a/tests/generic/574.out b/tests/generic/574.out
index d40d1263..3ed57f1c 100644
--- a/tests/generic/574.out
+++ b/tests/generic/574.out
@@ -1,84 +1,11 @@ 
 QA output created by 574
 
-# Corruption test: file_len=131072 zap_offset=0 zap_len=5
-0dfbe8aa4c20b52e1b8bf3cb6cbdf193  SCRATCH_MNT/file.fsv
-Corrupting bytes...
-Validating corruption (reading full file)...
-md5sum: SCRATCH_MNT/file.fsv: Input/output error
-Validating corruption (direct I/O)...
-dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
-Validating corruption (reading just corrupted part)...
-dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
-Validating corruption (reading full file via mmap)...
-Bus error
-Validating corruption (reading just corrupted part via mmap)...
-Bus error
+# Testing block_size=FSV_BLOCK_SIZE
 
-# Corruption test: file_len=131072 zap_offset=4091 zap_len=5
-0dfbe8aa4c20b52e1b8bf3cb6cbdf193  SCRATCH_MNT/file.fsv
-Corrupting bytes...
-Validating corruption (reading full file)...
-md5sum: SCRATCH_MNT/file.fsv: Input/output error
-Validating corruption (direct I/O)...
-dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
-Validating corruption (reading just corrupted part)...
-dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
-Validating corruption (reading full file via mmap)...
-Bus error
-Validating corruption (reading just corrupted part via mmap)...
-Bus error
+# Testing block_size=1024 if supported
 
-# Corruption test: file_len=131072 zap_offset=65536 zap_len=65536
-0dfbe8aa4c20b52e1b8bf3cb6cbdf193  SCRATCH_MNT/file.fsv
-Corrupting bytes...
-Validating corruption (reading full file)...
-md5sum: SCRATCH_MNT/file.fsv: Input/output error
-Validating corruption (direct I/O)...
-dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
-Validating corruption (reading just corrupted part)...
-dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
-Validating corruption (reading full file via mmap)...
-Bus error
-Validating corruption (reading just corrupted part via mmap)...
-Bus error
+# Testing block_size=4096 if supported
 
-# Corruption test: file_len=131072 zap_offset=131067 zap_len=5
-0dfbe8aa4c20b52e1b8bf3cb6cbdf193  SCRATCH_MNT/file.fsv
-Corrupting bytes...
-Validating corruption (reading full file)...
-md5sum: SCRATCH_MNT/file.fsv: Input/output error
-Validating corruption (direct I/O)...
-dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
-Validating corruption (reading just corrupted part)...
-dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
-Validating corruption (reading full file via mmap)...
-Bus error
-Validating corruption (reading just corrupted part via mmap)...
-Bus error
+# Testing block_size=16384 if supported
 
-# Corruption test: EOF block
-f5cca0d7fbb8b02bc6118a9954d5d306  SCRATCH_MNT/file.fsv
-Corrupting bytes...
-Reading eof block via mmap into a temporary file...
-Checking for SIGBUS or zeros...
-OK
-
-# Corruption test: file_len=200000 zap_offset=100 (in Merkle tree) zap_len=10
-4a1e4325031b13f933ac4f1db9ecb63f  SCRATCH_MNT/file.fsv
-Corrupting bytes...
-Validating corruption (reading full file)...
-md5sum: SCRATCH_MNT/file.fsv: Input/output error
-Validating corruption (direct I/O)...
-dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
-Validating corruption (reading full file via mmap)...
-Bus error
-
-# Corruption test: file_len=200000 (sparse) zap_offset=100 (in Merkle tree) zap_len=10
-4a1e4325031b13f933ac4f1db9ecb63f  SCRATCH_MNT/file.fsv
-Corrupting bytes...
-Validating corruption (reading full file)...
-md5sum: SCRATCH_MNT/file.fsv: Input/output error
-Validating corruption (direct I/O)...
-dd: error reading 'SCRATCH_MNT/file.fsv': Input/output error
-Validating corruption (reading full file via mmap)...
-Bus error
+# Testing block_size=65536 if supported