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

[Dave Chinner <david@fromorbit.com>]

On Mon, Nov 05, 2018 at 02:16:57PM -0600, Jayashree Mohan wrote:
> Hi Dave,
> 
> Thanks for the comments. 
> 
> >> +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.
> 
> Sure, that was meant for the initial evaluation. I’ve removed it now.

Thanks!

> 
> > 
> >> +total_time=0
> >> +
> >> +_clean_dir()
> >> +{
> >> +	_mount_flakey
> >> +	rm -rf $SCRATCH_MNT/*
> >> +	sync
> >> +	_unmount_flakey
> >> +}
> > 
> > Why not just _scratch_mkfs?
> 
> I believe that to _scratch_mkfs, I must first _cleanup dm_flakey.
> If I replace the above snippet by 
> _cleanup
> _scratch_mkfs
> _init_flakey
> 
> The time taken for the test goes up by around 10 seconds (due to
> mkfs maybe). So I thought it was sufficient to remove the working
> directory.

Ok. Put that in a comment (comments are for explaining why, not how
or what). :)

Overall the new version looks a lot better. More comments
below.

> diff --git a/tests/generic/517-link b/tests/generic/517-link
> new file mode 100755
> index 0000000..7108300
> --- /dev/null
> +++ b/tests/generic/517-link

Please just use "517" as the test name. At some point it was though
that descriptive names for tests might be useful, but it's turned
out that's not the case and we have just continued to number them.

> @@ -0,0 +1,176 @@
> +#! /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.*
> +}
> +
> +# 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
> +
> +stat_opt='-c "blocks: %b size: %s inode: %i links: %h"'
> +before=""
> +after=""
> +
> +_clean_dir()
> +{
> +	_mount_flakey
> +	rm -rf $SCRATCH_MNT/*
> +	sync
> +	_unmount_flakey
> +}
> +
> +_check_consistency()
> +{
> +	_flakey_drop_and_remount | tee -a $seqres.full
> +	

Whitespace damage.

> +	if [ -f $1 ]; then
> +		after=`stat "$stat_opt" $1`
> +	fi
> +
> +	if [ "$before" != "$after" ] && [ $2 -ne 1 ]; then
> +		echo "Before: $before" 
> +		echo "After: $after"	

Whitespace.

Add this to your .vimrc:

" highlight whitespace damage
highlight RedundantSpaces ctermbg=red guibg=red
match RedundantSpaces /\s\+$\| \+\ze\t/


> +	fi
> +
> +	_unmount_flakey
> +	_check_scratch_fs $FLAKEY_DEV
> +	[ $? -ne 0 ] && _fatal "fsck failed"
> +}
> +
> +# create a hard link $2 to file $1, and fsync $3, followed by power-cut
> +test_link_fsync()
> +{
> +	local sibling=0
> +	before=""
> +	after=""
> +	src=$SCRATCH_MNT/$1
> +	dest=$SCRATCH_MNT/$2
> +	

Whitespace

> +	if [ "$3" == "./" ]; then
> +		fsync=$SCRATCH_MNT
> +	else
> +		fsync=$SCRATCH_MNT/$3
> +	fi

Why doesn't fsync of $SCRATCH_MNT/./ work?

> +
> +	echo -ne "\n=== link $src $dest  with fsync $fsync ===\n" | _filter_scratch
> +	_mount_flakey
> +	

Whitespace

> +	# now execute the workload
> +	mkdir -p "${src%/*}"

Please explain why this magic directory creation is needed - "now
execute the workload" isn't going to remind me what this does in a
couple of years time.

> +	mkdir -p "${dest%/*}" 

Whitespace

> +	touch $src
> +	ln $src $dest || _fail "Link creation failed"

No need for _fail here. The error message will end up in the output
and the test will fail the golden output match. That way it will
still run all the other tests.

> +	

Whitespace

> +	# If the file being persisted is a sibling, create it first
> +	if [ ! -f $fsync ]; then
> +		sibling=1
> +		touch $fsync
> +	fi
> +
> +	$XFS_IO_PROG -c "fsync" $fsync
> +		

Whitespace

> +	if [ $sibling -ne 1 ]; then
> +		before=`stat "$stat_opt" $src`
> +	fi
> +
> +	_check_consistency $src $sibling
> +	_clean_dir
> +}
> +
> +# create a hard link $2 to file $1, and sync, followed by power-cut
> +test_link_sync()
> +{
> +	src=$SCRATCH_MNT/$1
> +	dest=$SCRATCH_MNT/$2
> +	before=""
> +	after=""
> +	echo -ne "\n=== link $src $dest  with sync ===\n" | _filter_scratch
> +	_mount_flakey
> +	

Whitespace

> +	# now execute the workload
> +	mkdir -p "${src%/*}"
> +	mkdir -p "${dest%/*}" 

Whitespace

> +	touch $src
> +	ln $src $dest || _fail "Link creation failed"	

Whitespace

> +	sync
> +	before=`stat "$stat_opt" $src`
> +	

Whitespace

> +	_check_consistency $src 0
> +	_clean_dir
> +}
> +
> +
> +# Create different combinations to run the link test
> +# Group 0: Both files within root directory
> +file_names[0]="foo bar"
> +fsync_names[0]="./ foo bar"
> +
> +# Group 1: Create hard link in a sub directory
> +file_names[1]="foo A/bar"
> +fsync_names[1]="./ foo bar A A/bar A/foo"
> +
> +# Group 2: Create hard link in parent directory
> +file_names[2]="A/foo bar"
> +fsync_names[2]="./ foo bar A A/bar A/foo"
> +
> +# Group 3: Both files within a directory other than root
> +file_names[3]="A/foo A/bar"
> +fsync_names[3]="./ A A/bar A/foo"
> +
> +#Group 4: Exercise name reuse : Link file in sub-directory
> +file_names[4]="bar A/bar"
> +fsync_names[4]="./ foo bar A A/bar A/foo"
> +
> +#Group 5: Exercise name reuse : Link file in parent directory
> +file_names[5]="A/bar bar"
> +fsync_names[5]="./ foo bar A A/bar A/foo"
> +
> +for ((test_group=0; test_group<6; test_group++)); do
> +	for file in ${fsync_names[$test_group]}; do
> +		test_link_fsync ${file_names[$test_group]} $file
> +	done
> +	test_link_sync ${file_names[$test_group]}
> +done 

Whitespace

> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/517-link.out b/tests/generic/517-link.out
> new file mode 100644
> index 0000000..1f80b27
> --- /dev/null
> +++ b/tests/generic/517-link.out
> @@ -0,0 +1,75 @@
> +QA output created by 517-link
> +
> +=== link SCRATCH_MNT/foo SCRATCH_MNT/bar  with fsync SCRATCH_MNT ===
> +
> +=== link SCRATCH_MNT/foo SCRATCH_MNT/bar  with fsync SCRATCH_MNT/foo ===
> +
> +=== link SCRATCH_MNT/foo SCRATCH_MNT/bar  with fsync SCRATCH_MNT/bar ===
> +
> +=== link SCRATCH_MNT/foo SCRATCH_MNT/bar  with sync ===
> +
> +=== link SCRATCH_MNT/foo SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT ===
> +
> +=== link SCRATCH_MNT/foo SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT/foo ===
> +
> +=== link SCRATCH_MNT/foo SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT/bar ===
> +
> +=== link SCRATCH_MNT/foo SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT/A ===
> +
> +=== link SCRATCH_MNT/foo SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT/A/bar ===
> +
> +=== link SCRATCH_MNT/foo SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT/A/foo ===
> +
> +=== link SCRATCH_MNT/foo SCRATCH_MNT/A/bar  with sync ===
> +
> +=== link SCRATCH_MNT/A/foo SCRATCH_MNT/bar  with fsync SCRATCH_MNT ===
> +
> +=== link SCRATCH_MNT/A/foo SCRATCH_MNT/bar  with fsync SCRATCH_MNT/foo ===
> +
> +=== link SCRATCH_MNT/A/foo SCRATCH_MNT/bar  with fsync SCRATCH_MNT/bar ===
> +
> +=== link SCRATCH_MNT/A/foo SCRATCH_MNT/bar  with fsync SCRATCH_MNT/A ===
> +
> +=== link SCRATCH_MNT/A/foo SCRATCH_MNT/bar  with fsync SCRATCH_MNT/A/bar ===
> +
> +=== link SCRATCH_MNT/A/foo SCRATCH_MNT/bar  with fsync SCRATCH_MNT/A/foo ===
> +
> +=== link SCRATCH_MNT/A/foo SCRATCH_MNT/bar  with sync ===
> +
> +=== link SCRATCH_MNT/A/foo SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT ===
> +
> +=== link SCRATCH_MNT/A/foo SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT/A ===
> +
> +=== link SCRATCH_MNT/A/foo SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT/A/bar ===
> +
> +=== link SCRATCH_MNT/A/foo SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT/A/foo ===
> +
> +=== link SCRATCH_MNT/A/foo SCRATCH_MNT/A/bar  with sync ===
> +
> +=== link SCRATCH_MNT/bar SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT ===
> +
> +=== link SCRATCH_MNT/bar SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT/foo ===
> +
> +=== link SCRATCH_MNT/bar SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT/bar ===
> +
> +=== link SCRATCH_MNT/bar SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT/A ===
> +
> +=== link SCRATCH_MNT/bar SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT/A/bar ===
> +
> +=== link SCRATCH_MNT/bar SCRATCH_MNT/A/bar  with fsync SCRATCH_MNT/A/foo ===
> +
> +=== link SCRATCH_MNT/bar SCRATCH_MNT/A/bar  with sync ===
> +
> +=== link SCRATCH_MNT/A/bar SCRATCH_MNT/bar  with fsync SCRATCH_MNT ===
> +
> +=== link SCRATCH_MNT/A/bar SCRATCH_MNT/bar  with fsync SCRATCH_MNT/foo ===
> +
> +=== link SCRATCH_MNT/A/bar SCRATCH_MNT/bar  with fsync SCRATCH_MNT/bar ===
> +
> +=== link SCRATCH_MNT/A/bar SCRATCH_MNT/bar  with fsync SCRATCH_MNT/A ===
> +
> +=== link SCRATCH_MNT/A/bar SCRATCH_MNT/bar  with fsync SCRATCH_MNT/A/bar ===
> +
> +=== link SCRATCH_MNT/A/bar SCRATCH_MNT/bar  with fsync SCRATCH_MNT/A/foo ===
> +
> +=== link SCRATCH_MNT/A/bar SCRATCH_MNT/bar  with sync ===
> diff --git a/tests/generic/group b/tests/generic/group
> index 47de978..67e9108 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -519,3 +519,4 @@
>  514 auto quick clone
>  515 auto quick clone
>  516 auto quick dedupe clone
> +517-link quick log
> 
> 
> 

-- 
Dave Chinner
david@fromorbit.com