Thanks for the feedback!
On Sun, Apr 26, 2020 at 2:29 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Fri, Apr 24, 2020 at 7:52 PM Arvind Raghavan
> <raghavan.arvind@xxxxxxxxx> wrote:
> >
> > This patch aims to add tests to the xfstest suite to check whether
> > renamed files recover after a crash. These test cases were generated by
> > CrashMonkey, a crash-consistency testing framework built at the SASLab
> > at UT Austin.
> >
> > This patch batches 37 tests into a single test, each of which creates a
> > file, renames it, syncs one of the files/parent directories, and ensures
> > that the synced file/dir is equivalent before and after a crash.
> >
> > This test takes around 12-15 seconds to run locally and is thus placed
> > in the 'quick' group.
> >
> > Signed-off-by: Arvind Raghavan <raghavan.arvind@xxxxxxxxx>
>
> Nice work. Some things to improve...
>
> > ---
> > tests/generic/484 | 179 ++++++++++++++++++++++++++++++++++++++++++
> > tests/generic/484.out | 2 +
> > tests/generic/group | 1 +
> > 3 files changed, 182 insertions(+)
> > create mode 100755 tests/generic/484
> > create mode 100644 tests/generic/484.out
> >
> > diff --git a/tests/generic/484 b/tests/generic/484
> > new file mode 100755
> > index 00000000..b27d828d
> > --- /dev/null
> > +++ b/tests/generic/484
> > @@ -0,0 +1,179 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2020 The University of Texas at Austin. All Rights Reserved.
> > +#
> > +# FS QA Test 484
> > +#
> > +# Test case created by CrashMonkey
> > +#
> > +# Test if we create a rename a file and persist it that the
> > +# appropriate files exist.
> > +#
> > +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
> > +
> > +# 256MB in byte
> > +fssize=$((2**20 * 256))
> > +
> > +# 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_sized $fssize >> $seqres.full 2>&1
>
> Why limit the fs size?
> In general this results is test coverage for non real life filesystems,
> so sub-optimal IMO.
It was an artifact from the originally Crashmonkey test harness, but
we can go ahead and remove it now.
> > +_require_metadata_journaling $SCRATCH_DEV
> > +_init_flakey
> > +
> > +stat_opt='-c %n - blocks: %b size: %s inode: %i links: %h'
> > +
> > +# Usage: general_stat [--data-only] file1 [--data-only] [file2] ..
> > +# If the --data-only flag precedes a file, then only that file's
> > +# data will be printed. If a file is synced, then general_stat
> > +# should provide identical output before and after a crash.
>
> This helper seems overly complicated for the use case in this
> test, but it is local to the test. I understand it is auto generated for
> other tests but that's not the way to go.
> I suggest you look into using existing FSSUM_PROG tool.
I agree that seems like a more elegant solution. We essentially
have four cases that we need to check:
(1) fsync file (meta + data)
(2) fdatasync file
(3) fsync dir
(4) fdatasync dir
FSSUM_PROG works great for (3) and (4), since it looks like its
intended to be used only on directories. md5sum works well for
(2), but it doesn't capture the metadata needed for (1).
FSSUM_PROG has a way to print a full "manifest", which contains
the metadata/data hashes for each file, so I believe we can
grep/cut it out of the manifest output to test for (1). I'll send
out a V2 patch if you think this seems like a reasonable
approach.
Also, see below for a note on FSSUM_PROG.
> > +general_stat() {
> > + local data_only="false"
> > + while (( "$#" )); do
> > + case $1 in
> > + --data-only)
> > + data_only="true"
> > + ;;
> > + *)
> > + local path="$1"
> > + echo "-- $path --"
> > + if [ ! -e "$path" ]; then
> > + echo "Doesn't exist!"
> > + elif [ "$data_only" = "true" ] && [ -d "$path" ]; then
> > + echo "Directory Data"
> > + [ -z "$(ls -A $path)" ] || ls -1 "$path" | sort
>
> [ -z "$(ls -A $path)" ] || seems to add nothing.
>
> > + elif [ "$data_only" = "true" ]; then
> > + echo "File Data"
> > + od "$path"
> > + elif [ -d "$path" ]; then
> > + echo "Directory Metadata"
> > + stat "$stat_opt" "$path"
> > + echo "Directory Data"
> > + [ -z "$(ls -A $path)" ] || ls -1 "$path" | sort
> > + else
> > + echo "File Metadata"
> > + stat "$stat_opt" "$path"
> > + echo "File Data"
> > + od "$path"
> > + fi
> > + data_only="false"
> > + ;;
> > + esac
> > + shift
> > + done
> > +}
> > +
> > +check_consistency()
> > +{
> > + before=$(general_stat "$@")
> > + _flakey_drop_and_remount
> > + after=$(general_stat "$@")
> > +
> > + if [ "$before" != "$after" ]; then
> > + echo -e "Before:\n$before"
> > + echo -e "After:\n$after"
> > + fi
> > +
> > + # Using _unmount_flakey nondeterministically fails here with
> > + # "target is busy". Calling 'sync' doesn't fix the issue. Lazy
> > + # unmount waits for lingering processes to exit.
>
> No way. None of the other flaky tests have this kludge.
> You must be doing something wrong, or there is another bug
> in the infrastructure that needs to be fixed instead of being papered over.
Yeah, this was a bug on my end. I've fixed it and removed all of
the lazy unmounts.
> > + $UMOUNT_PROG -l $SCRATCH_MNT
> > + _check_scratch_fs $FLAKEY_DEV
> > +}
> > +
> > +clean_dir()
> > +{
> > + _mount_flakey
> > + rm -rf $(find $SCRATCH_MNT/* | grep -v "lost+found")
>
> Use testdir=$SCRATCH_MNT/testdir for base of your test and you
> won't need this blacklisting of lost+found.
>
> > + $UMOUNT_PROG -l $SCRATCH_MNT
>
> And maybe this is what you are doing wrong??
> Why is this unmount lazy?
>
> > +}
> > +
> > +do_fsync_check() {
> > + local file="$1"
> > + local sync_op="$2"
> > +
> > + if [ $sync_op == "fdatasync" ]; then
> > + $XFS_IO_PROG -c "fdatasync" $file
>
> You don't really need to put this statement inside if/else
>
> > + check_consistency --data-only $file
>
> And you could probably also pass sync_op to check_consistency()
> instead of converting it here. In other words, this helper does not
> do much, so you could get rid of it. up to you.
>
> Thanks,
> Amir.
I believe there is a minor bug in the fssum program. Below are
the relevant lines from the help output.
usage: fssum <options> <path>
options:
...
-[ugoamcdxe]: specify which fields to include in checksum
calculation.
...
x : include xattrs
...
...
-x path : exclude path when building checksum
(multiple ok)
...
There seems to be a duplicate flag which prevents the user from
passing in -x to include the xattrs field in checksum
calculation. This can be fixed with a simple patch that changes
the latter flag to "-i path", and updates the documentation to
say "ignores path when building checksum". I've got such a commit
built locally than I can send out in a patch if you think it
makes sense. None of the tests in the repo use the flag as AFAIK,
but we will end up using it for the xattr tests.
