Re: [PATCH] fstest: CrashMonkey tests ported to xfstest

[Dave Chinner <david@fromorbit.com>]

On Sun, Nov 04, 2018 at 02:21:21PM -0600, Jayashree Mohan wrote:
> Hi Eryu and Filipe,
> 
> I have incorporated the coding style suggested, and renamed the cleanup function. However, creating a clean fs image after each sub test is resulting in about 10-15s of additional overhead overall. I instead clean up the working directory and unmount. The time for the tests varies between 12-15 seconds.
> 
> Please find the patch below
> 
> —
> 
> diff --git a/tests/generic/517-link b/tests/generic/517-link
> new file mode 100755
> index 0000000..ea5c5b7
> --- /dev/null
> +++ b/tests/generic/517-link
> @@ -0,0 +1,164 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2018 The University of Texas at Austin.  All Rights Reserved.
> +#
> +# FS QA Test 517-link
> +#
> +# Test case created by CrashMonkey
> +#
> +# Test if we create a hard link to a file and persist either of the files, all the names persist.
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	_cleanup_flakey
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +init_start_time=$(date +%s.%N)
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/dmflakey
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +_supported_fs generic
> +_supported_os Linux
> +_require_scratch_nocheck
> +_require_dm_target flakey
> +
> +# initialize scratch device
> +_scratch_mkfs >> $seqres.full 2>&1
> +_require_metadata_journaling $SCRATCH_DEV
> +_init_flakey
> +init_end_time=$(date +%s.%N)
> +init_time_elapsed=$(echo "$init_end_time - $init_start_time" | bc) 
> +echo "Initialization time = $init_time_elapsed" >> $seqres.full

No timing inside the test, please. The harness times the test
execution.

> +stat_opt='-c "blocks: %b size: %s inode: %i links: %h"'
> +test_num=0

Don't number tests. If we add an extra test, it will completely
break the golden output.

> +total_time=0
> +
> +_clean_dir()
> +{
> +	_mount_flakey
> +	rm -rf $SCRATCH_MNT/*
> +	sync
> +	_unmount_flakey
> +}

Why not just _scratch_mkfs?

> +
> +# create a hard link $2 to file $1, and fsync $3, followed by power-cut
> +test_link()
> +{
> +	test_num=$((test_num + 1))
> +	local start_time=$(date +%s.%N)
> +	local sibling=0
> +	local before=""
> +	local after=""
> +	echo -ne "\n=== Test $test_num :  link $1 $2 " | _filter_scratch
> +	_mount_flakey
> +	

Still whitespace damaged.

> +	# now execute the workload
> +	mkdir -p "${1%/*}" && mkdir -p "${2%/*}" && touch "$1"

One line each. 

> +	ln $1 $2

No checks for failure?

> +	
> +	if [ -z "$3" ]; then
> +		echo -ne " with sync ===\n"
> +		sync
> +		before=`stat "$stat_opt" $1`
> +	else
> +		echo " with fsync $3 ===" | _filter_scratch
> +		
> +		# If the file being persisted is a sibling, create it first
> +		if [ ! -f $3 ]; then
> +			sibling=1
> +			touch $3
> +		fi
> +
> +		$XFS_IO_PROG -c "fsync" $3
> +		
> +		if [ $sibling -ne 1 ]; then
> +			before=`stat "$stat_opt" $1`
> +		fi
> +	fi

To tell the truth, I'd consider these two separate functions -
test_link_sync() and test_link_fsync(). More below.


> +
> +	_flakey_drop_and_remount | tee -a $seqres.full
> +	
> +	if [ -f $1 ]; then
> +		after=`stat "$stat_opt" $1`
> +	fi
> +
> +	if [ "$before" != "$after" ] && [ $sibling -ne 1 ]; then
> +		echo "Before: $before" | tee -a $seqres.full
> +		echo "After: $after" | tee -a $seqres.full	
> +	fi

Why tee all the output to $seqres.full? Once the timing info is
removed, it dones't contain anything extra that the normal output
file...

> +
> +	_unmount_flakey
> +	_check_scratch_fs $FLAKEY_DEV
> +	[ $? -ne 0 ] && _fatal "fsck failed"
> +	end_time=$(date +%s.%N)
> +	time_elapsed=$(echo "$end_time - $start_time" | bc)
> +	echo " Elapsed time : $time_elapsed" >> $seqres.full
> +	total_time=$(echo "$total_time + $time_elapsed" | bc)
> +	echo " Total time : $total_time" >> $seqres.full
> +	_clean_dir
> +}
> +
> +# run the link test for different combinations
> +
> +test_start_time=$(date +%s.%N)
> +# Group 1: Both files within root directory
> +for file_name in $SCRATCH_MNT $SCRATCH_MNT/foo $SCRATCH_MNT/bar; do
> +	test_link $SCRATCH_MNT/foo $SCRATCH_MNT/bar $file_name
> +done
> +test_link $SCRATCH_MNT/foo $SCRATCH_MNT/bar 
> +
> +# Group 2: Create hard link in a sub directory
> +for file_name in $SCRATCH_MNT $SCRATCH_MNT/foo $SCRATCH_MNT/bar $SCRATCH_MNT/A $SCRATCH_MNT/A/bar $SCRATCH_MNT/A/foo; do
> +	test_link $SCRATCH_MNT/foo $SCRATCH_MNT/A/bar $file_name
> +done
> +test_link $SCRATCH_MNT/foo $SCRATCH_MNT/A/bar 

Lines too long.

Basically, this is a two dimensional array of filenames.
Make test_link_fsync() do:

	src=$SCRATCH_MNT/$1
	dst=$SCRATCH_MNT/$2
	fsync=$SCRATCH_MNT/$3

And define your fsync filenames and link destinations names like so
(can't remember bash array notation, so paraphrasing):

# group 1: Both files within root directory
fnames[1]="./ foo bar"
dnames[1]="foo bar"

# group 2: Create hard link in a sub directory
fnames[2]="./ foo bar A A/bar A/foo"
dnames[2]="foo A/bar"

#g3
fnames[3]="./ foo bar A A/bar A/foo"
dnames[3]="A/foo bar"

#g4
fnames[4]="./ A A/bar A/foo"
dnames[4]="A/foo A/bar"

#g5
fnames[5]="./ foo bar A A/bar A/foo"
dnames[5]="bar A/bar"

#g6
fnames[6]="./ foo bar A A/bar A/foo"
dnames[6]="A/bar bar"

for ((i=1; i<=6; i++)); do
	for f in $fnames[$i]; do
		test_link_fsync $dnames[$i] $f
	done
	test_link_sync $dnames[1]
done

And all this shouty, shouty noise goes away. Much easier to read,
much simpler to maintain.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com