[PATCH 1/2] common/dump: do not override test cleanup trap

[PATCH 1/2] common/dump: do not override test cleanup trap

[Amir Goldstein]

Currently this doesn't matter for the two dump tests xfs/022 and xfs/068
because they do not have a _cleanup() routine and the override dump
_cleanup() routine does the generic cleanup as well.

Instead, call the _dump_cleanup explicitly from the trap in those tests.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 common/dump   | 5 +----
 tests/xfs/022 | 2 +-
 tests/xfs/068 | 2 +-
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/common/dump b/common/dump
index 4d1a1607..47d14601 100644
--- a/common/dump
+++ b/common/dump
@@ -45,9 +45,6 @@ session_label="stress_$seq"
 nobody=4 # define this uid/gid as a number
 do_quota_check=true # do quota check if quotas enabled
 
-# install our cleaner
-trap "_cleanup; exit \$status" 0 1 2 3 15
-
 # start inventory from a known base - move it aside for test
 for dir in /var/xfsdump/inventory /var/lib/xfsdump/inventory; do
     if [ -d $dir ]; then
@@ -227,7 +224,7 @@ _wipe_fs()
 # Cleanup created dirs and files
 # Called by trap
 #
-_cleanup()
+_dump_cleanup()
 {
     # Some tests include this before checking _supported_fs xfs
     # and the sleeps & checks here get annoying
diff --git a/tests/xfs/022 b/tests/xfs/022
index e1162798..f091b7c5 100755
--- a/tests/xfs/022
+++ b/tests/xfs/022
@@ -17,7 +17,7 @@ echo "QA output created by $seq"
 here=`pwd`
 tmp=/tmp/$$
 status=0	# success is the default!
-trap "rm -rf $tmp.*; exit \$status" 0 1 2 3 15
+trap "_dump_cleanup; exit \$status" 0 1 2 3 15
 
 . ./common/rc
 . ./common/dump
diff --git a/tests/xfs/068 b/tests/xfs/068
index c755bc3e..95a8cd12 100755
--- a/tests/xfs/068
+++ b/tests/xfs/068
@@ -18,7 +18,7 @@ echo "QA output created by $seq"
 here=`pwd`
 tmp=/tmp/$$
 status=0	# success is the default!
-trap "rm -rf $tmp.*; exit \$status" 0 1 2 3 15
+trap "_dump_cleanup; exit \$status" 0 1 2 3 15
 
 . ./common/rc
 . ./common/dump
-- 
2.17.1

[PATCH 2/2] xfs/068: Add fsstress generated file count to golden output

[Amir Goldstein]

This test has the number of files/dirs created by fsstress hardcoded
in golden output.

When fsstress is added new ops, the number of files/dirs created with
the same random seed changes and this regularly breaks this test.

So when new fsstress ops are added they should be either added to the
dump test blacklist or golden output of this test needs to be ammended
to reflect the change.

Since the golden output includes only the file count after dump/restore,
add also the file count before dump/restore so developers are less
likely to forget to check the validity of golden output before commiting
the change.

For some reason the file count reported by xfsrestore has one directory
more than the file count reported by 'find'. I did not investigate why
that is, but did verify that this was the same with the original test
fsstress ops (i.e. without the added ops
insert/mread/mwrite/aread/awrite/readv/writev).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 common/dump       | 7 +++++++
 tests/xfs/068     | 1 +
 tests/xfs/068.out | 1 +
 3 files changed, 9 insertions(+)

diff --git a/common/dump b/common/dump
index 47d14601..23f42216 100644
--- a/common/dump
+++ b/common/dump
@@ -1515,6 +1515,13 @@ _check_quota_file()
    _check_quota 'xfsdump_quotas' 'xfsdump_quotas_group' 'xfsdump_quotas_proj'
 }
 
+_count_dumpdir_files()
+{
+	local ndirs=$(find $dump_dir -type d | wc -l)
+	local nents=$(find $dump_dir | wc -l)
+
+	echo "Created $ndirs directories and $nents entries"
+}
 
 # make sure this script returns success
 /bin/true
diff --git a/tests/xfs/068 b/tests/xfs/068
index 95a8cd12..ffc293bd 100755
--- a/tests/xfs/068
+++ b/tests/xfs/068
@@ -28,6 +28,7 @@ _supported_fs xfs
 _supported_os Linux
 
 _create_dumpdir_stress_num 4096
+_count_dumpdir_files
 _do_dump_restore
 
 # success, all done
diff --git a/tests/xfs/068.out b/tests/xfs/068.out
index fa3a5523..61cbbfa4 100644
--- a/tests/xfs/068.out
+++ b/tests/xfs/068.out
@@ -4,6 +4,7 @@ Creating directory system to dump using fsstress.
 -----------------------------------------------
 fsstress : -f link=10 -f creat=10 -f mkdir=10 -f truncate=5 -f symlink=10
 -----------------------------------------------
+Created 382 directories and 1334 entries
 xfsdump|xfsrestore ...
 xfsdump  -s DUMP_SUBDIR - SCRATCH_MNT | xfsrestore  - RESTORE_DIR
 xfsrestore: using file dump (drive_simple) strategy
-- 
2.17.1

Re: [PATCH 2/2] xfs/068: Add fsstress generated file count to golden output

[Eryu Guan]

[I'm sorry that I've been absent from the discussion entirely.. I was
busy with other tasks recently.]

On Thu, Jan 24, 2019 at 10:33:10AM +0200, Amir Goldstein wrote:
> This test has the number of files/dirs created by fsstress hardcoded
> in golden output.

This leads me to wonder if we could remove the hardcoded dir/entries
number from the golden output and verify the counts on the fly? i.e. we
count the dir/entries numbers that fsstress created and compare them
with the numbers xfsrestore reports. So we don't have to worry about new
ops in fsstress and xfs configurations.

But for now, I'd like to take Zorro's v3 patch, which follows Dave's
suggestion. And I've verified that the restored dir/entires counts
matched what fsstress created (so previously added
insert/mread/mwrite etc. ops didn't break xfs/068 either).

> 
> When fsstress is added new ops, the number of files/dirs created with
> the same random seed changes and this regularly breaks this test.
> 
> So when new fsstress ops are added they should be either added to the
> dump test blacklist or golden output of this test needs to be ammended
> to reflect the change.
> 
> Since the golden output includes only the file count after dump/restore,
> add also the file count before dump/restore so developers are less
> likely to forget to check the validity of golden output before commiting
> the change.
> 
> For some reason the file count reported by xfsrestore has one directory
> more than the file count reported by 'find'. I did not investigate why

I haven't looked at the xfsrestore code yet, but I guess it's because
xfsrestore counts the '$SCRATCH_MNT/restoredir' as one entry processed
as well.

Thanks,
Eryu

> that is, but did verify that this was the same with the original test
> fsstress ops (i.e. without the added ops
> insert/mread/mwrite/aread/awrite/readv/writev).
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  common/dump       | 7 +++++++
>  tests/xfs/068     | 1 +
>  tests/xfs/068.out | 1 +
>  3 files changed, 9 insertions(+)
> 
> diff --git a/common/dump b/common/dump
> index 47d14601..23f42216 100644
> --- a/common/dump
> +++ b/common/dump
> @@ -1515,6 +1515,13 @@ _check_quota_file()
>     _check_quota 'xfsdump_quotas' 'xfsdump_quotas_group' 'xfsdump_quotas_proj'
>  }
>  
> +_count_dumpdir_files()
> +{
> +	local ndirs=$(find $dump_dir -type d | wc -l)
> +	local nents=$(find $dump_dir | wc -l)
> +
> +	echo "Created $ndirs directories and $nents entries"
> +}
>  
>  # make sure this script returns success
>  /bin/true
> diff --git a/tests/xfs/068 b/tests/xfs/068
> index 95a8cd12..ffc293bd 100755
> --- a/tests/xfs/068
> +++ b/tests/xfs/068
> @@ -28,6 +28,7 @@ _supported_fs xfs
>  _supported_os Linux
>  
>  _create_dumpdir_stress_num 4096
> +_count_dumpdir_files
>  _do_dump_restore
>  
>  # success, all done
> diff --git a/tests/xfs/068.out b/tests/xfs/068.out
> index fa3a5523..61cbbfa4 100644
> --- a/tests/xfs/068.out
> +++ b/tests/xfs/068.out
> @@ -4,6 +4,7 @@ Creating directory system to dump using fsstress.
>  -----------------------------------------------
>  fsstress : -f link=10 -f creat=10 -f mkdir=10 -f truncate=5 -f symlink=10
>  -----------------------------------------------
> +Created 382 directories and 1334 entries
>  xfsdump|xfsrestore ...
>  xfsdump  -s DUMP_SUBDIR - SCRATCH_MNT | xfsrestore  - RESTORE_DIR
>  xfsrestore: using file dump (drive_simple) strategy
> -- 
> 2.17.1
> 

Re: [PATCH 1/2] common/dump: do not override test cleanup trap

[Eryu Guan]

On Thu, Jan 24, 2019 at 10:33:09AM +0200, Amir Goldstein wrote:
> Currently this doesn't matter for the two dump tests xfs/022 and xfs/068
> because they do not have a _cleanup() routine and the override dump
> _cleanup() routine does the generic cleanup as well.
> 
> Instead, call the _dump_cleanup explicitly from the trap in those tests.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  common/dump   | 5 +----
>  tests/xfs/022 | 2 +-
>  tests/xfs/068 | 2 +-

I just scan all tests that source common/dump quickly and it seems we
have more tests to convert than xfs/{022,068}

eguan@desktop:~/workspace/src/xfstests$ grep trap `grep common/dump ./* -rI | grep xfs | sort | uniq | cut -d':' -f 1`
./tests/xfs/022:trap "rm -rf $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/023:trap "rm -rf $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/024:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/025:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/026:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/027:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/028:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/035:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/036:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/037:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/038:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/039:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/043:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/046:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/047:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/055:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/056:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/059:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/060:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/061:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/063:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/064:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/065:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/066:trap "_cleanup; exit \$status" 0 1 2 3 15
./tests/xfs/068:trap "rm -rf $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/266:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/267:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/268:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/281:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/282:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/283:trap "rm -f $tmp.*; exit \$status" 0 1 2 3 15
./tests/xfs/287:trap "_cleanup; exit \$status" 0 1 2 3 15
./tests/xfs/296:trap "_cleanup; exit \$status" 0 1 2 3 15
./tests/xfs/301:trap "_cleanup; exit \$status" 0 1 2 3 15
./tests/xfs/302:trap "_cleanup; exit \$status" 0 1 2 3 15

>  3 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/common/dump b/common/dump
> index 4d1a1607..47d14601 100644
> --- a/common/dump
> +++ b/common/dump
> @@ -45,9 +45,6 @@ session_label="stress_$seq"
>  nobody=4 # define this uid/gid as a number
>  do_quota_check=true # do quota check if quotas enabled
>  
> -# install our cleaner
> -trap "_cleanup; exit \$status" 0 1 2 3 15
> -
>  # start inventory from a known base - move it aside for test
>  for dir in /var/xfsdump/inventory /var/lib/xfsdump/inventory; do
>      if [ -d $dir ]; then
> @@ -227,7 +224,7 @@ _wipe_fs()
>  # Cleanup created dirs and files
>  # Called by trap
>  #
> -_cleanup()
> +_dump_cleanup()
>  {
>      # Some tests include this before checking _supported_fs xfs
>      # and the sleeps & checks here get annoying
> diff --git a/tests/xfs/022 b/tests/xfs/022
> index e1162798..f091b7c5 100755
> --- a/tests/xfs/022
> +++ b/tests/xfs/022
> @@ -17,7 +17,7 @@ echo "QA output created by $seq"
>  here=`pwd`
>  tmp=/tmp/$$
>  status=0	# success is the default!
> -trap "rm -rf $tmp.*; exit \$status" 0 1 2 3 15
> +trap "_dump_cleanup; exit \$status" 0 1 2 3 15

Let's follow the template and trap a generic _cleanup and call
_dump_cleanup there, and we could remove the following lines from
_dump_cleanup, because they've been done in generic _cleanup.

    cd $here                                                                                                                                                                                                         
    rm -f $tmp.*

Thanks,
Eryu

>  
>  . ./common/rc
>  . ./common/dump
> diff --git a/tests/xfs/068 b/tests/xfs/068
> index c755bc3e..95a8cd12 100755
> --- a/tests/xfs/068
> +++ b/tests/xfs/068
> @@ -18,7 +18,7 @@ echo "QA output created by $seq"
>  here=`pwd`
>  tmp=/tmp/$$
>  status=0	# success is the default!
> -trap "rm -rf $tmp.*; exit \$status" 0 1 2 3 15
> +trap "_dump_cleanup; exit \$status" 0 1 2 3 15
>  
>  . ./common/rc
>  . ./common/dump
> -- 
> 2.17.1
> 

Re: [PATCH 2/2] xfs/068: Add fsstress generated file count to golden output

[Amir Goldstein]

On Sun, Jan 27, 2019 at 7:02 AM Eryu Guan <guaneryu@gmail.com> wrote:
>
> [I'm sorry that I've been absent from the discussion entirely.. I was
> busy with other tasks recently.]
>
> On Thu, Jan 24, 2019 at 10:33:10AM +0200, Amir Goldstein wrote:
> > This test has the number of files/dirs created by fsstress hardcoded
> > in golden output.
>
> This leads me to wonder if we could remove the hardcoded dir/entries
> number from the golden output and verify the counts on the fly? i.e. we
> count the dir/entries numbers that fsstress created and compare them
> with the numbers xfsrestore reports. So we don't have to worry about new
> ops in fsstress and xfs configurations.
>

posted v2 with a slightly different approach:
count the dir/entries numbers that fsstress created and compare them
with the *actual* numbers xfsrestore created instead of the numbers that
xfsrestore reports.

Sure, this removes test coverage for the accuracy of the report, but
that was really never the intention of the test and the report is really
wrong! (off by one dir from actual restored).

If someone really cares about validating the accuracy of xfsrestore report
in that test, that by all means, let someone read into xfsrestore and figure
out what the one directory stands for and either fix xfsrestore or work that
quirk into the test. As can be inferred, that someone is not going to be me.

Thanks,
Amir.