On Fri, May 20, 2011 at 11:22:51AM -0700, haldar@xxxxxxxxxx wrote:
> From: Vivek Haldar <haldar@xxxxxxxxxx>
>
> Scenarios covered:
> 1. Fallocate X bytes
> 2. Write Y bytes
> 3. Truncate to Z bytes
> for various values of X, Y and Z, and check both file size and
> number of filesystem blocks used by the file after each of
> 1, 2 and 3.
What does this cover that 213 and 214 don't cover? 214
especially does falloc, pwrite and truncate combinations. e.g:
cho "=== falloc & read ==="
echo "=== falloc, write beginning, read ==="
echo "=== falloc, write middle, read ==="
echo "=== falloc, write end, read ==="
echo "=== falloc, write, sync, truncate, read ==="
echo "=== delalloc write 16k; fallocate same range ==="
Why write a new test, when there's already a test that does the same
sort of tests? If you need to expand the test coverage, modify the
existing test that covers those cases, don't write a whole new test.
Also, you can't rely on the number of filesystem blocks being used
being consistent across different filesystem types or even the same
filesystem width different block sizes so this check is no good.
.....
> +# fallocate, write and truncate with various options and checks.
> +_test()
> +{
> + falloc_size=${1} # bytes
> + falloc_keep_size=${2} # boolean
> + expected_size_after_falloc=${3}
> + expected_blocks_after_falloc=${4}
> + write_size=${5}
> + expected_size_after_write=${6}
> + expected_blocks_after_write=${7}
> + trunc_size=${8}
> + expected_size_after_trunc=${9}
> + expected_blocks_after_trunc=${10}
> + dont_delete_test_file=${11} # boolean
Oh, my eyes! They bleed! :(
Use tabs for indents, please. And any test that requires 11
parameters passed to it need a little bit of factoring, I think,
because I can't tell what the test is testing from reading the test
function call...
> +
> + echo "falloc/write/truncate test: \
> + falloc_size=${falloc_size} \
> + falloc_keep_size=${falloc_keep_size} \
> + expected_size_after_falloc=${expected_size_after_falloc} \
> + expected_blocks_after_falloc=${expected_blocks_after_falloc} \
> + write_size=${write_size} \
> + expected_size_after_write=${expected_size_after_write} \
> + expected_blocks_after_write=${expected_blocks_after_write} \
> + trunc_size=${trunc_size} \
> + expected_size_after_trunc=${expected_size_after_trunc} \
> + expected_blocks_after_trunc=${expected_blocks_after_trunc} \
> + dont_delete_test_file=${dont_delete_test_file}"
This is not necessary and is really ugly.
Before running the test, all that is needed is a simple echo
"running test XXX" so that the results in the output are easily
separated and you can look at the test script to find out which test
actually failed.
> +
> + if [ $dont_delete_test_file ] ; then
> + echo "Re-using test file."
> + else
> + rm -f $test_file
> + fi
Same again - no need for outputing status like this.
> +
> + # falloc
> + keep_size_flag=''
> + if $falloc_keep_size ; then
> + keep_size_flag='-k'
> + fi
> + if [ $falloc_size -gt 0 ]; then
> + ${XFS_IO_PROG} -F -f -d \
> + -c "falloc $keep_size_flag 0 $falloc_size" \
> + $test_file | _filter_xfs_io_unique
> + else
> + touch $test_file
> + fi
Why does this need to be in the test() function? Why not just do it
before calling the test? i.e.:
echo "test 1"
echo -n > $test_file
test .....
....
echo "test 2"
${XFS_IO_PROG} -F -f -d -c "falloc -k 0 20k" $test_file | filter
test....
> + # check sizes after falloc
> + size_after_falloc=`stat --format="%s" $test_file`
> + blocks_after_falloc=`du --block-size=4096 -s $test_file | cut -f 1`
Just dump the stat output in the golden image with an appropriate
filter....
> +
> + if [ $expected_size_after_falloc -ne $size_after_falloc ]; then
> + status=1
> + echo "Expected size after falloc to be $expected_size_after_falloc \
> + but it was actually $size_after_falloc."
> + exit ${status}
> + fi
And then you can drop this as the golden image match will fail if it
is incorrect. Results analysis does not belong in the xfstests test
code - that's what the golden image is for.
Here, i wrote this yesterday because Allison made exactly the same
mistakes as you have:
http://users.on.net/~david_chinner/blog/xfstests_and_golden_output.html
> + if [ $expected_blocks_after_falloc -ne $blocks_after_falloc ]; then
> + status=1
> + echo "Expected blocks after falloc to be $expected_blocks_after_falloc \
> + but it was actually $blocks_after_falloc."
> + exit ${status}
> + fi
Not valid.
> + # write
> + if [ $write_size -gt 0 ]; then
> + ${XFS_IO_PROG} -F -f -d \
> + -c "pwrite 0 $write_size" \
> + $test_file | _filter_xfs_io_unique
> + fi
> +
> + # check sizes after write
> + size_after_write=`stat --format="%s" $test_file`
> + blocks_after_write=`du --block-size=4096 -s $test_file | cut -f 1`
> +
> + if [ $expected_size_after_write -ne $size_after_write ]; then
> + status=1
> + echo "Expected size after write to be $expected_size_after_write \
> + but it was actually $size_after_write."
> + exit ${status}
> + fi
> +
> + if [ $expected_blocks_after_write -ne $blocks_after_write ]; then
> + status=1
> + echo "Expected blocks after write to be $expected_blocks_after_write \
> + but it was actually $blocks_after_write."
> + exit ${status}
> + fi
same comments.
> +
> + # trunc
> + if [ $trunc_size -gt 0 ]; then
> + ${XFS_IO_PROG} -F -f -d \
> + -c "truncate $trunc_size" \
> + $test_file | _filter_xfs_io_unique
> + fi
> +
> + # check sizes after trunc
> + size_after_trunc=`stat --format="%s" $test_file`
> + blocks_after_trunc=`du --block-size=4096 -s $test_file | cut -f 1`
> +
> + if [ $expected_size_after_trunc -ne $size_after_trunc ]; then
> + status=1
> + echo "Expected size after truncate to be $expected_size_after_trunc \
> + but it was actually $size_after_trunc."
> + exit ${status}
> + fi
> +
> + if [ $expected_blocks_after_trunc -ne $blocks_after_trunc ]; then
> + status=1
> + echo "Expected blocks after trunc to be $expected_blocks_after_trunc \
> + but it was actually $blocks_after_trunc."
> + exit ${status}
> + fi
same comments.
Realistically, I can rewrite each test case with a single xfs_io
command line. They are all variants of:
echo "test XXX"
${XFS_IO_PROG} -F -f -d \
-c "falloc -k 0 10k" \
-c stat
-c "pwrite 0 4k" \
-c stat
-c "t 8k" \
-c stat \
$test_file | filter
And use the golden image to do all the results checking. This is
what test 214 does, and other hole/unwritten/falloc/punch tests do
like 242 and 252.
> +}
> +
> +# Check that a file has the given bit pattern.
> +_test_file_contents()
> +{
> + file_name=${1}
> + offset=${2}
> + len=${3}
> + expected_pattern=${4}
> + expected_count=${5}
> +
> + count=`${XFS_IO_PROG} -F -f -d -c \
> + "pread -v $offset $len" $file_name \
> + | grep -w -c $expected_pattern`
> +
> + if [ $expected_count -ne $count ]; then
> + status=1
> + echo "Expected count $expected_count for $file_name at \
> + offset $offset for $len bytes for pattern $expected_pattern \
> + but count was $count."
> + exit ${status}
> + fi
> +}
see how 214 does the read checks by using the golden output....
> +
> +# Get standard environment, filters and checks.
> +. ./common.rc
> +. ./common.filter
> +
> +# Prerequisites for the test run.
> +_supported_fs generic
> +_supported_os Linux
> +_require_xfs_io_falloc
> +
> +# Remove any leftover files from last run.
> +rm -f ${TEST_DIR}/test*
> +
> +test_file=${TEST_DIR}/test
test files should always have their test number associated with
them. ie.. test.$seq
> +
> +# Generic test cleanup function.
> +_cleanup()
> +{
> + cd /
> + rm -f $tmp.*
> + rm -f $test_file
> +}
these (the includes through to cleanup functions) should all be at
the top of the test, not in the middle of the test code.
> +
> +# Begin test cases.
> +
> +# KEEP_SIZE
> +_test 10240 true 0 3 0 0 3 0 0 3
> +_test 8192 true 0 2 0 0 2 0 0 2
> +_test 4096 true 0 1 0 0 1 0 0 1
Can't say I'm able to easily work out what these are testing from
this.
> +
> +# !KEEP_SIZE
> +_test 10240 false 10240 3 0 10240 3 0 10240 3
> +_test 8192 false 8192 2 0 8192 2 0 8192 2
> +_test 4096 false 4096 1 0 4096 1 0 4096 1
> +_test_file_contents $test_file 1024 512 "00" 32 # check null.
> +
> +_test 10240 true 0 3 4096 4096 3 0 4096 3
> +_test 10240 true 0 3 8192 8192 3 0 8192 3
> +_test 10240 true 0 3 10240 10240 3 0 10240 3
> +
> +_test 16384 true 0 4 16384 16384 4 0 16384 4
> +
> +# just truncate tests, without falloc.
> +_test 0 false 0 0 8192 8192 2 8192 8192 2
> +_test 0 false 0 0 8192 8192 2 4096 4096 1
> +_test 0 false 0 0 8192 8192 2 16384 16384 2
> +_test_file_contents $test_file 8192 512 "00" 32 # check null.
> +_test_file_contents $test_file 4096 512 "cd" 32 # check non-null.
Those cases are covered in many other tests, and with much better
coverage. e.g. fsx.
> +# write beyond fallocate size and then truncate to the written size.
> +_test 8192 true 0 2 16384 16384 4 16384 16384 4
> +_test_file_contents $test_file 8192 512 "cd" 32 # check non-null.
> +
> +# fallocate, write, truncate below the written size, and then truncate up. Then
> +# try to read the previously written data to see whether it returns zero or the
> +# stale data.
> +_test 8192 true 0 2 16384 16384 4 12288 12288 3
> +_test 0 true 12288 3 0 12288 3 16384 16384 3 true
> +_test_file_contents $test_file 1024 512 "cd" 32 # check non-null.
> +_test_file_contents $test_file 12288 512 "00" 32 # check null.
> +
> +# fallocate, write with the fallocate size, write after the fallocate size, and
> +# then truncate to a size smaller than the fallocate size.
> +_test 8192 true 0 2 8192 8192 2 0 8192 2
> +_test 0 true 8192 2 12288 12288 3 4096 4096 1 true
These should really go in 214....
> +# The following reproduce an existing bug.
> +# See http://patchwork.ozlabs.org/patch/96080/
> +_test 10240 true 0 3 4096 4096 3 8192 8192 2
> +_test 10240 true 0 3 0 0 3 8192 8192 2
> +_test 16384 true 0 4 0 0 4 16384 16384 4
<sigh>
So all this is ends up in a test for something that hasn't really
been decided whether it's a bug or not or what the proper behaviour
is supposed to be? Please document exactly what behaviour your
expect in the test, not point to discussion thread that hasn't come
to any conclusions....
Why? Because I can't tell from these numbers what exactly it is
testing. Is it testing the truncate up case, the truncate down
case, which behaviour is it codifying as correct, is it consiÑtent
across filesystems, etc? Using an xfs_io command directly is self
documenting and so avoids much of this confusion....
---
FWIW, it may sound like I'm complaining a lot about recent patches
to add new tests to xfstests, but when:
- there is a lot of overlap with existing tests,
- the new tests simply appear to be standalone tests just
with an xfstest wrapper
- the tests do they own (complex) results analysis
- are hard to read
- are hard to verify correct
- will break when block sizes or underlying
filesystems change
The proposed test is unlikely to pass a review.
That's because I hold the filesystem test code to the same standard
as I do for kernel patches or userspace filesystem tools. Tests
need to be well written, understandable, follow the existing coding
conventions, fit into the test suite architecture correctly, etc.
i.e. I judge it on the same criteria that I'd judge any kernel patch
that is proposed.
Our kernel filesystems code is only as good as our test code, and if
we can't easily verify the test code is correct and maintain it that
way, then we simply cannot expect to maintain the kernel filesystem
to any decent level of reliability.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
