[PATCH v2 0/4] overlay: enhance fsck.overlay test cases

[PATCH v2 0/4] overlay: enhance fsck.overlay test cases

[zhangyi (F)]

Hi all,

These are the second version of fsck.overlay[1] program related patch set
(the first one was posted about two mounths ago[2], sorry for the late).
I have fix all comments as Amir suggested and looks fine after test.

The first two patches correct the exit code and fix the tests failure
with master branch of fsck.overlay, and the last two patches were just
want to test stability and exception handling processes.

Hi Amir,

Patch 1 and 2 will fix the test failure you mentioned yesterday when you
run -g overlay/fsck with fsck.overlay's master branch. I am very grateful
if you can give a review.

Thanks,
Yi.

Changes from v1:
 - split patch 2 from patch 1 for readable.
 - move exit code definitions from common/config to common/overlay.
 - factor out a common function _run_check_fsck() to fsck and check
   return value.
 - fix some mistakes in patch 3 and 4.

[1] https://github.com/hisilicon/overlayfs-progs/commits/master
[2] https://www.spinics.net/lists/fstests/msg10267.html

zhangyi (F) (4):
  overlay: correct fsck.overlay exit code
  overlay: fix exit code for some fsck.overlay valid cases
  overlay: add fsck.overlay stress test
  overlay: add fsck.overlay exception tests

 common/overlay        |  37 ++++++++-
 tests/overlay/045     |  46 ++++++-----
 tests/overlay/046     |  49 ++++++------
 tests/overlay/056     |   9 +--
 tests/overlay/062     |  84 +++++++++++++++++++
 tests/overlay/062.out |   2 +
 tests/overlay/063     | 217 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/063.out |  10 +++
 tests/overlay/group   |   2 +
 9 files changed, 403 insertions(+), 53 deletions(-)
 create mode 100755 tests/overlay/062
 create mode 100644 tests/overlay/062.out
 create mode 100755 tests/overlay/063
 create mode 100644 tests/overlay/063.out

-- 
2.5.0

[PATCH v2 1/4] overlay: correct fsck.overlay exit code

[zhangyi (F)]

fsck.overlay should return correct exit code to show the file system
status after fsck, instead of return 0 means consistency and !0 means
inconsistency or something bad happened.

Fix the following three exit code after running fsck.overlay:

- Return FSCK_OK if the input file system is consistent,
- Return FSCK_NONDESTRUCT if the file system inconsistent errors
  corrected,
- Return FSCK_UNCORRECTED if the file system still have inconsistent
  errors.

This patch also add a helper function to run fsck.overlay and check
the return value is expected or not.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 common/overlay    | 29 +++++++++++++++++++++++++++++
 tests/overlay/045 | 27 +++++++++------------------
 tests/overlay/046 | 36 ++++++++++++------------------------
 tests/overlay/056 |  9 +++------
 4 files changed, 53 insertions(+), 48 deletions(-)

diff --git a/common/overlay b/common/overlay
index b526f24..4cc2829 100644
--- a/common/overlay
+++ b/common/overlay
@@ -12,6 +12,16 @@ export OVL_XATTR_NLINK="trusted.overlay.nlink"
 export OVL_XATTR_UPPER="trusted.overlay.upper"
 export OVL_XATTR_METACOPY="trusted.overlay.metacopy"
 
+# Export exit code used by fsck.overlay program
+export FSCK_OK=0
+export FSCK_NONDESTRUCT=1
+export FSCK_REBOOT=2
+export FSCK_UNCORRECTED=4
+export FSCK_ERROR=8
+export FSCK_USAGE=16
+export FSCK_CANCELED=32
+export FSCK_LIBRARY=128
+
 # helper function to do the actual overlayfs mount operation
 _overlay_mount_dirs()
 {
@@ -193,6 +203,25 @@ _overlay_fsck_dirs()
 			   -o workdir=$workdir $*
 }
 
+# Run fsck and check the return value
+_run_check_fsck()
+{
+	# The first arguments is the expected fsck program exit code, the
+	# remaining arguments are the input parameters of the fsck program.
+	local expect_ret=$1
+	local lowerdir=$2
+	local upperdir=$3
+	local workdir=$4
+	shift 4
+
+	_overlay_fsck_dirs $lowerdir $upperdir $workdir $* >> \
+			$seqres.full 2>&1
+	fsck_ret=$?
+
+	[[ "$fsck_ret" == "$expect_ret" ]] || \
+		echo "fsck return unexpected value:$expect_ret,$fsck_ret"
+}
+
 _overlay_check_dirs()
 {
 	local lowerdir=$1
diff --git a/tests/overlay/045 b/tests/overlay/045
index acc7087..ed23258 100755
--- a/tests/overlay/045
+++ b/tests/overlay/045
@@ -96,8 +96,7 @@ echo "+ Orphan whiteout"
 make_test_dirs
 make_whiteout $lowerdir/foo $upperdir/{foo,bar}
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT $lowerdir $upperdir $workdir -p
 ls $lowerdir
 ls $upperdir
 
@@ -107,8 +106,7 @@ make_test_dirs
 touch $lowerdir2/{foo,bar}
 make_whiteout $upperdir/foo $lowerdir/bar
 
-_overlay_fsck_dirs "$lowerdir:$lowerdir2" $upperdir $workdir -p >> \
-	 $seqres.full 2>&1 || echo "fsck should not fail"
+_run_check_fsck $FSCK_OK "$lowerdir:$lowerdir2" $upperdir $workdir -p
 check_whiteout $upperdir/foo $lowerdir/bar
 
 # Test orphan whiteout in opaque directory, should remove
@@ -119,8 +117,7 @@ touch $lowerdir/testdir/foo
 make_opaque_dir $upperdir/testdir
 make_whiteout $upperdir/testdir/foo
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT $lowerdir $upperdir $workdir -p
 ls $upperdir/testdir
 
 # Test orphan whiteout whose parent path is not an merged directory,
@@ -135,8 +132,7 @@ make_whiteout $lowerdir/testdir2
 make_opaque_dir $lowerdir/testdir3
 make_whiteout $upperdir/{testdir1/foo,/testdir2/foo,testdir3/foo,testdir4/foo}
 
-_overlay_fsck_dirs "$lowerdir:$lowerdir2" $upperdir $workdir -p >> \
-	$seqres.full 2>&1 || echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT "$lowerdir:$lowerdir2" $upperdir $workdir -p
 ls $upperdir/testdir1
 ls $upperdir/testdir2
 ls $upperdir/testdir3
@@ -150,8 +146,7 @@ touch $lowerdir/testdir/foo
 make_redirect_dir $upperdir/testdir "origin"
 make_whiteout $upperdir/testdir/foo
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT $lowerdir $upperdir $workdir -p
 ls $upperdir/testdir
 
 # Test valid whiteout in redirect directory cover file in lower
@@ -163,8 +158,7 @@ touch $lowerdir/origin/foo
 make_redirect_dir $upperdir/testdir "origin"
 make_whiteout $upperdir/testdir/foo
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p
 check_whiteout $upperdir/testdir/foo
 
 # Test valid whiteout covering lower target whose parent directory
@@ -177,8 +171,7 @@ make_redirect_dir $lowerdir/testdir "origin"
 mkdir -p $upperdir/testdir/subdir
 make_whiteout $upperdir/testdir/subdir/foo
 
-_overlay_fsck_dirs "$lowerdir:$lowerdir2" $upperdir $workdir -p \
-	>> $seqres.full 2>&1 || echo "fsck should not fail"
+_run_check_fsck $FSCK_OK "$lowerdir:$lowerdir2" $upperdir $workdir -p
 check_whiteout $upperdir/testdir/subdir/foo
 
 # Test invalid whiteout in opaque subdirectory in a redirect directory,
@@ -191,8 +184,7 @@ make_redirect_dir $upperdir/testdir "origin"
 make_opaque_dir $upperdir/testdir/subdir
 make_whiteout $upperdir/testdir/subdir/foo
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT $lowerdir $upperdir $workdir -p
 ls $upperdir/testdir/subdir
 
 # Test valid whiteout in reidrect subdirectory in a opaque directory
@@ -205,8 +197,7 @@ make_opaque_dir $upperdir/testdir
 make_redirect_dir $upperdir/testdir/subdir "/origin"
 make_whiteout $upperdir/testdir/subdir/foo
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-        echo "fsck should not fail"
+_run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p
 check_whiteout $upperdir/testdir/subdir/foo
 
 # success, all done
diff --git a/tests/overlay/046 b/tests/overlay/046
index 6338a38..21645c1 100755
--- a/tests/overlay/046
+++ b/tests/overlay/046
@@ -121,8 +121,7 @@ echo "+ Invalid redirect"
 make_test_dirs
 make_redirect_dir $upperdir/testdir "invalid"
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT $lowerdir $upperdir $workdir -p
 check_no_redirect $upperdir/testdir
 
 # Test invalid redirect xattr point to a file origin, should remove
@@ -131,8 +130,7 @@ make_test_dirs
 touch $lowerdir/origin
 make_redirect_dir $upperdir/testdir "origin"
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT $lowerdir $upperdir $workdir -p
 check_no_redirect $upperdir/testdir
 
 # Test valid redirect xattr point to a directory origin in the same directory,
@@ -143,8 +141,7 @@ mkdir $lowerdir/origin
 make_whiteout $upperdir/origin
 make_redirect_dir $upperdir/testdir "origin"
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p
 check_redirect $upperdir/testdir "origin"
 
 # Test valid redirect xattr point to a directory origin in different directories
@@ -155,8 +152,7 @@ mkdir $lowerdir/origin
 make_whiteout $upperdir/origin
 make_redirect_dir $upperdir/testdir1/testdir2 "/origin"
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p
 check_redirect $upperdir/testdir1/testdir2 "/origin"
 
 # Test valid redirect xattr but missing whiteout to cover lower target,
@@ -166,8 +162,7 @@ make_test_dirs
 mkdir $lowerdir/origin
 make_redirect_dir $upperdir/testdir "origin"
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT $lowerdir $upperdir $workdir -p
 check_redirect $upperdir/testdir "origin"
 check_whiteout $upperdir/origin
 
@@ -178,8 +173,7 @@ mkdir $lowerdir/{testdir1,testdir2}
 make_redirect_dir $upperdir/testdir1 "testdir2"
 make_redirect_dir $upperdir/testdir2 "testdir1"
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p
 check_redirect $upperdir/testdir1 "testdir2"
 check_redirect $upperdir/testdir2 "testdir1"
 
@@ -191,8 +185,7 @@ mkdir $lowerdir/testdir
 make_redirect_dir $upperdir/testdir "invalid"
 
 # Question get yes answer: Should set opaque dir ?
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -y >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT $lowerdir $upperdir $workdir -y
 check_no_redirect $upperdir/testdir
 check_opaque $upperdir/testdir
 
@@ -205,12 +198,10 @@ make_redirect_dir $lowerdir/testdir1 "origin"
 make_redirect_dir $lowerdir/testdir2 "origin"
 make_redirect_dir $upperdir/testdir3 "origin"
 
-_overlay_fsck_dirs "$lowerdir:$lowerdir2" $upperdir $workdir -p >> \
-	$seqres.full 2>&1 && echo "fsck should fail"
+_run_check_fsck $FSCK_UNCORRECTED "$lowerdir:$lowerdir2" $upperdir $workdir -p
 
 # Question get yes answer: Duplicate redirect directory, remove xattr ?
-_overlay_fsck_dirs "$lowerdir:$lowerdir2" $upperdir $workdir -y >> \
-	$seqres.full 2>&1 || echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT "$lowerdir:$lowerdir2" $upperdir $workdir -y
 redirect_1=`check_redirect $lowerdir/testdir1 "origin" 2>/dev/null`
 redirect_2=`check_redirect $lowerdir/testdir2 "origin" 2>/dev/null`
 [[ $redirect_1 == $redirect_2 ]] && echo "Redirect xattr incorrect"
@@ -223,12 +214,10 @@ make_test_dirs
 mkdir $lowerdir/origin $upperdir/origin
 make_redirect_dir $upperdir/testdir "origin"
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 && \
-	echo "fsck should fail"
+_run_check_fsck $FSCK_UNCORRECTED $lowerdir $upperdir $workdir -p
 
 # Question get yes answer: Duplicate redirect directory, remove xattr ?
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -y >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT $lowerdir $upperdir $workdir -y
 check_no_redirect $upperdir/testdir
 
 # Test duplicate redirect xattr with lower same name directory exists,
@@ -240,8 +229,7 @@ make_redirect_dir $upperdir/testdir "invalid"
 
 # Question one get yes answer: Duplicate redirect directory, remove xattr?
 # Question two get yes answer: Should set opaque dir ?
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -y >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT $lowerdir $upperdir $workdir -y
 check_no_redirect $upperdir/testdir
 check_opaque $upperdir/testdir
 
diff --git a/tests/overlay/056 b/tests/overlay/056
index 44ffb54..8679b55 100755
--- a/tests/overlay/056
+++ b/tests/overlay/056
@@ -96,8 +96,7 @@ $UMOUNT_PROG $SCRATCH_MNT
 remove_impure $upperdir/testdir1
 remove_impure $upperdir/testdir2
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT $lowerdir $upperdir $workdir -p
 check_impure $upperdir/testdir1
 check_impure $upperdir/testdir2
 
@@ -108,8 +107,7 @@ make_test_dirs
 mkdir $lowerdir/origin
 make_redirect_dir $upperdir/testdir/subdir "/origin"
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT $lowerdir $upperdir $workdir -p
 check_impure $upperdir/testdir
 
 # Test missing impure xattr in directory which has merge directories,
@@ -118,8 +116,7 @@ echo "+ Missing impure(3)"
 make_test_dirs
 mkdir $lowerdir/testdir $upperdir/testdir
 
-_overlay_fsck_dirs $lowerdir $upperdir $workdir -p >> $seqres.full 2>&1 || \
-	echo "fsck should not fail"
+_run_check_fsck $FSCK_NONDESTRUCT $lowerdir $upperdir $workdir -p
 check_impure $upperdir
 
 # success, all done
-- 
2.5.0

Re: [PATCH v2 1/4] overlay: correct fsck.overlay exit code

[Amir Goldstein]

On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>
> fsck.overlay should return correct exit code to show the file system
> status after fsck, instead of return 0 means consistency and !0 means
> inconsistency or something bad happened.
>
> Fix the following three exit code after running fsck.overlay:
>
> - Return FSCK_OK if the input file system is consistent,
> - Return FSCK_NONDESTRUCT if the file system inconsistent errors
>   corrected,
> - Return FSCK_UNCORRECTED if the file system still have inconsistent
>   errors.
>
> This patch also add a helper function to run fsck.overlay and check
> the return value is expected or not.
>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> ---
>  common/overlay    | 29 +++++++++++++++++++++++++++++
>  tests/overlay/045 | 27 +++++++++------------------
>  tests/overlay/046 | 36 ++++++++++++------------------------
>  tests/overlay/056 |  9 +++------
>  4 files changed, 53 insertions(+), 48 deletions(-)
>
> diff --git a/common/overlay b/common/overlay
> index b526f24..4cc2829 100644
> --- a/common/overlay
> +++ b/common/overlay
> @@ -12,6 +12,16 @@ export OVL_XATTR_NLINK="trusted.overlay.nlink"
>  export OVL_XATTR_UPPER="trusted.overlay.upper"
>  export OVL_XATTR_METACOPY="trusted.overlay.metacopy"
>
> +# Export exit code used by fsck.overlay program
> +export FSCK_OK=0
> +export FSCK_NONDESTRUCT=1
> +export FSCK_REBOOT=2
> +export FSCK_UNCORRECTED=4
> +export FSCK_ERROR=8
> +export FSCK_USAGE=16
> +export FSCK_CANCELED=32
> +export FSCK_LIBRARY=128
> +
>  # helper function to do the actual overlayfs mount operation
>  _overlay_mount_dirs()
>  {
> @@ -193,6 +203,25 @@ _overlay_fsck_dirs()
>                            -o workdir=$workdir $*
>  }
>
> +# Run fsck and check the return value
> +_run_check_fsck()
> +{
> +       # The first arguments is the expected fsck program exit code, the
> +       # remaining arguments are the input parameters of the fsck program.
> +       local expect_ret=$1
> +       local lowerdir=$2
> +       local upperdir=$3
> +       local workdir=$4
> +       shift 4
> +
> +       _overlay_fsck_dirs $lowerdir $upperdir $workdir $* >> \
> +                       $seqres.full 2>&1
> +       fsck_ret=$?
> +
> +       [[ "$fsck_ret" == "$expect_ret" ]] || \
> +               echo "fsck return unexpected value:$expect_ret,$fsck_ret"
> +}

Looks good.

Please consider the name _overlay_repair_dirs() with reference to
_repair_scratch_fs(), which is used for roughly the same purpose.

BTW, the tests generic/330 generic/332 when run with -overlay
over xfs with reflink support seem to call _repair_scratch_fs() which does:
        # Let's hope fsck -y suffices...
        fsck -t $FSTYP -y $SCRATCH_DEV 2>&1
        local res=$?
        case $res in
        0|1|2)
                res=0
                ;;
        *)
                _dump_err2 "fsck.$FSTYP failed, err=$res"

How come fsck.overlay is not complaining about missing arguments??

The rest of the generic tests that call _repair_scratch_fs() have
_require_dm_target() which _require_block_device(), so don't run with overlay.

If you do sort this out and add overlay support to
_repair_scratch_fs() its probably
worth replacing 0|1|2) with FSCK_ constants.

Thanks,
Amir.

Re: [PATCH v2 1/4] overlay: correct fsck.overlay exit code

[zhangyi (F)]

On 2018/10/16 17:45, Amir Goldstein Wrote:
> On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>>
>> fsck.overlay should return correct exit code to show the file system
>> status after fsck, instead of return 0 means consistency and !0 means
>> inconsistency or something bad happened.
>>
>> Fix the following three exit code after running fsck.overlay:
>>
>> - Return FSCK_OK if the input file system is consistent,
>> - Return FSCK_NONDESTRUCT if the file system inconsistent errors
>>   corrected,
>> - Return FSCK_UNCORRECTED if the file system still have inconsistent
>>   errors.
>>
>> This patch also add a helper function to run fsck.overlay and check
>> the return value is expected or not.
>>
>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>> ---
>>  common/overlay    | 29 +++++++++++++++++++++++++++++
>>  tests/overlay/045 | 27 +++++++++------------------
>>  tests/overlay/046 | 36 ++++++++++++------------------------
>>  tests/overlay/056 |  9 +++------
>>  4 files changed, 53 insertions(+), 48 deletions(-)
>>
>> diff --git a/common/overlay b/common/overlay
>> index b526f24..4cc2829 100644
>> --- a/common/overlay
>> +++ b/common/overlay
>> @@ -12,6 +12,16 @@ export OVL_XATTR_NLINK="trusted.overlay.nlink"
>>  export OVL_XATTR_UPPER="trusted.overlay.upper"
>>  export OVL_XATTR_METACOPY="trusted.overlay.metacopy"
>>
>> +# Export exit code used by fsck.overlay program
>> +export FSCK_OK=0
>> +export FSCK_NONDESTRUCT=1
>> +export FSCK_REBOOT=2
>> +export FSCK_UNCORRECTED=4
>> +export FSCK_ERROR=8
>> +export FSCK_USAGE=16
>> +export FSCK_CANCELED=32
>> +export FSCK_LIBRARY=128
>> +
>>  # helper function to do the actual overlayfs mount operation
>>  _overlay_mount_dirs()
>>  {
>> @@ -193,6 +203,25 @@ _overlay_fsck_dirs()
>>                            -o workdir=$workdir $*
>>  }
>>
>> +# Run fsck and check the return value
>> +_run_check_fsck()
>> +{
>> +       # The first arguments is the expected fsck program exit code, the
>> +       # remaining arguments are the input parameters of the fsck program.
>> +       local expect_ret=$1
>> +       local lowerdir=$2
>> +       local upperdir=$3
>> +       local workdir=$4
>> +       shift 4
>> +
>> +       _overlay_fsck_dirs $lowerdir $upperdir $workdir $* >> \
>> +                       $seqres.full 2>&1
>> +       fsck_ret=$?
>> +
>> +       [[ "$fsck_ret" == "$expect_ret" ]] || \
>> +               echo "fsck return unexpected value:$expect_ret,$fsck_ret"
>> +}
> 
> Looks good.
> 
> Please consider the name _overlay_repair_dirs() with reference to
> _repair_scratch_fs(), which is used for roughly the same purpose.
> 

_run_check_fsck() is helper used to test fsck and may expect to return
"error" exit code when we do exception tests(see patch 4), but
_repair_scratch_fs() always want to return "correct" exit code.

> BTW, the tests generic/330 generic/332 when run with -overlay
> over xfs with reflink support seem to call _repair_scratch_fs() which does:
>         # Let's hope fsck -y suffices...
>         fsck -t $FSTYP -y $SCRATCH_DEV 2>&1
>         local res=$?
>         case $res in
>         0|1|2)
>                 res=0
>                 ;;
>         *)
>                 _dump_err2 "fsck.$FSTYP failed, err=$res"
> 
> How come fsck.overlay is not complaining about missing arguments??
> 
> The rest of the generic tests that call _repair_scratch_fs() have
> _require_dm_target() which _require_block_device(), so don't run with overlay.
> 
> If you do sort this out and add overlay support to
> _repair_scratch_fs() its probably
> worth replacing 0|1|2) with FSCK_ constants.
> 

Thanks for pointing this out, I think we do something like below can fix
this problem, what do you think?

diff --git a/common/overlay b/common/overlay
index 2896594..b9fe1ee 100644
--- a/common/overlay
+++ b/common/overlay
@@ -226,6 +226,14 @@ _run_check_fsck()
                echo "fsck return unexpected value:$expect_ret,$fsck_ret"
 }

+_scratch_overlay_repair()
+{
+       _overlay_fsck_dirs $OVL_BASE_SCRATCH_MNT/$OVL_LOWER \
+                          $OVL_BASE_SCRATCH_MNT/$OVL_UPPER \
+                          $OVL_BASE_SCRATCH_MNT/$OVL_WORK \
+                          -y
+}
+
 _overlay_check_dirs()
 {
        local lowerdir=$1
diff --git a/common/rc b/common/rc
index 38d9de7..7127539 100644
--- a/common/rc
+++ b/common/rc
@@ -1100,6 +1100,18 @@ _repair_scratch_fs()
        fi
        return $res
         ;;
+    overlay)
+       _scratch_overlay_repair 2>&1
+       local res=$?
+       case $res in
+       $FSCK_OK|$FSCK_NONDESTRUCT)
+               res=0
+               ;;
+       *)
+               _dump_err2 "fsck.overlay failed, err=$res"
+       esac
+       return $res
+       ;;
     *)
         # Let's hope fsck -y suffices...
         fsck -t $FSTYP -y $SCRATCH_DEV 2>&1

Thanks,
Yi.

Re: [PATCH v2 1/4] overlay: correct fsck.overlay exit code

[Amir Goldstein]

On Thu, Oct 18, 2018 at 5:37 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>
> On 2018/10/16 17:45, Amir Goldstein Wrote:
> > On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
[...]
> >
> > Please consider the name _overlay_repair_dirs() with reference to
> > _repair_scratch_fs(), which is used for roughly the same purpose.
> >
>
> _run_check_fsck() is helper used to test fsck and may expect to return
> "error" exit code when we do exception tests(see patch 4), but
> _repair_scratch_fs() always want to return "correct" exit code.
>

Right. so perhaps we need _overlay_repair_dirs() as a convenience
helper for things like the stress test and also related to another
comment about expecting return 0 is too fragile.

> > BTW, the tests generic/330 generic/332 when run with -overlay
> > over xfs with reflink support seem to call _repair_scratch_fs() which does:
> >         # Let's hope fsck -y suffices...
> >         fsck -t $FSTYP -y $SCRATCH_DEV 2>&1
> >         local res=$?
> >         case $res in
> >         0|1|2)
> >                 res=0
> >                 ;;
> >         *)
> >                 _dump_err2 "fsck.$FSTYP failed, err=$res"
> >
> > How come fsck.overlay is not complaining about missing arguments??
> >
> > The rest of the generic tests that call _repair_scratch_fs() have
> > _require_dm_target() which _require_block_device(), so don't run with overlay.
> >
> > If you do sort this out and add overlay support to
> > _repair_scratch_fs() its probably
> > worth replacing 0|1|2) with FSCK_ constants.
> >
>
> Thanks for pointing this out, I think we do something like below can fix
> this problem, what do you think?

I am puzzled about why those tests do NOT fail when fsck.overlay is given
incorrect args??

>
> diff --git a/common/overlay b/common/overlay
> index 2896594..b9fe1ee 100644
> --- a/common/overlay
> +++ b/common/overlay
> @@ -226,6 +226,14 @@ _run_check_fsck()
>                 echo "fsck return unexpected value:$expect_ret,$fsck_ret"
>  }
>
> +_scratch_overlay_repair()
> +{
> +       _overlay_fsck_dirs $OVL_BASE_SCRATCH_MNT/$OVL_LOWER \
> +                          $OVL_BASE_SCRATCH_MNT/$OVL_UPPER \
> +                          $OVL_BASE_SCRATCH_MNT/$OVL_WORK \
> +                          -y
> +}
> +

All overlay "global" helpers should be prefixed _overlay_XXX
unless there is a good reason to break that rule (even though we did
not always abide by this rule in the past).

But anyway, I think that a more specific _overlay_repair_scratch_fs()
should be used to hide the details of "correct" error code from common/rc.

And it would probably be useful if it used a helper _overlay_repair_dirs()
that can be used from overlay specific tests (like stress test).

Thanks,
Amir.

Re: [PATCH v2 1/4] overlay: correct fsck.overlay exit code

[zhangyi]

On 2018/10/18 12:37, Amir Goldstein Wrote:

> On Thu, Oct 18, 2018 at 5:37 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>> On 2018/10/16 17:45, Amir Goldstein Wrote:
>>> On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
> [...]
>>> Please consider the name _overlay_repair_dirs() with reference to
>>> _repair_scratch_fs(), which is used for roughly the same purpose.
>>>
>> _run_check_fsck() is helper used to test fsck and may expect to return
>> "error" exit code when we do exception tests(see patch 4), but
>> _repair_scratch_fs() always want to return "correct" exit code.
>>
> Right. so perhaps we need _overlay_repair_dirs() as a convenience
> helper for things like the stress test and also related to another
> comment about expecting return 0 is too fragile.
>
>>> BTW, the tests generic/330 generic/332 when run with -overlay
>>> over xfs with reflink support seem to call _repair_scratch_fs() which does:
>>>          # Let's hope fsck -y suffices...
>>>          fsck -t $FSTYP -y $SCRATCH_DEV 2>&1
>>>          local res=$?
>>>          case $res in
>>>          0|1|2)
>>>                  res=0
>>>                  ;;
>>>          *)
>>>                  _dump_err2 "fsck.$FSTYP failed, err=$res"
>>>
>>> How come fsck.overlay is not complaining about missing arguments??
>>>
>>> The rest of the generic tests that call _repair_scratch_fs() have
>>> _require_dm_target() which _require_block_device(), so don't run with overlay.
>>>
>>> If you do sort this out and add overlay support to
>>> _repair_scratch_fs() its probably
>>> worth replacing 0|1|2) with FSCK_ constants.
>>>
>> Thanks for pointing this out, I think we do something like below can fix
>> this problem, what do you think?
> I am puzzled about why those tests do NOT fail when fsck.overlay is given
> incorrect args??
>
Oh, it's _dump_err2() helper's fault, it should be

--- a/common/rc
+++ b/common/rc
@@ -98,7 +98,7 @@ _dump_err_cont()
  _dump_err2()
  {
      _err_msg="$*"
-    >2& echo "$_err_msg"
+    >&2 echo "$_err_msg"
  }

Thanks,
Yi.

Re: [PATCH v2 1/4] overlay: correct fsck.overlay exit code

[Amir Goldstein]

On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>
> fsck.overlay should return correct exit code to show the file system
> status after fsck, instead of return 0 means consistency and !0 means
> inconsistency or something bad happened.
>
> Fix the following three exit code after running fsck.overlay:
>
> - Return FSCK_OK if the input file system is consistent,
> - Return FSCK_NONDESTRUCT if the file system inconsistent errors
>   corrected,
> - Return FSCK_UNCORRECTED if the file system still have inconsistent
>   errors.
>
> This patch also add a helper function to run fsck.overlay and check
> the return value is expected or not.
>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> ---
>  common/overlay    | 29 +++++++++++++++++++++++++++++
>  tests/overlay/045 | 27 +++++++++------------------
>  tests/overlay/046 | 36 ++++++++++++------------------------
>  tests/overlay/056 |  9 +++------
>  4 files changed, 53 insertions(+), 48 deletions(-)
>
> diff --git a/common/overlay b/common/overlay
> index b526f24..4cc2829 100644
> --- a/common/overlay
> +++ b/common/overlay
> @@ -12,6 +12,16 @@ export OVL_XATTR_NLINK="trusted.overlay.nlink"
>  export OVL_XATTR_UPPER="trusted.overlay.upper"
>  export OVL_XATTR_METACOPY="trusted.overlay.metacopy"
>
> +# Export exit code used by fsck.overlay program
> +export FSCK_OK=0
> +export FSCK_NONDESTRUCT=1
> +export FSCK_REBOOT=2
> +export FSCK_UNCORRECTED=4
> +export FSCK_ERROR=8
> +export FSCK_USAGE=16
> +export FSCK_CANCELED=32
> +export FSCK_LIBRARY=128
> +
>  # helper function to do the actual overlayfs mount operation
>  _overlay_mount_dirs()
>  {
> @@ -193,6 +203,25 @@ _overlay_fsck_dirs()
>                            -o workdir=$workdir $*
>  }
>
> +# Run fsck and check the return value
> +_run_check_fsck()
> +{
> +       # The first arguments is the expected fsck program exit code, the
> +       # remaining arguments are the input parameters of the fsck program.
> +       local expect_ret=$1
> +       local lowerdir=$2
> +       local upperdir=$3
> +       local workdir=$4
> +       shift 4
> +
> +       _overlay_fsck_dirs $lowerdir $upperdir $workdir $* >> \
> +                       $seqres.full 2>&1
> +       fsck_ret=$?
> +
> +       [[ "$fsck_ret" == "$expect_ret" ]] || \
> +               echo "fsck return unexpected value:$expect_ret,$fsck_ret"
> +}

Let's put some naming rules into effect here.
Helper should be prefixed _overlay.
_run_* to me implies run_check() wrapper.
So what's a better descriptive name for this helper? I donno, maybe
_overlay_fsck_expect() ?

Thanks,
Amir.

[PATCH v2 2/4] overlay: fix exit code for some fsck.overlay valid cases

[zhangyi (F)]

Some valid test cases about fsck.overlay may be not valid enough now,
they lose the impure xattr on the parent directory of the simluated
redirect directory, and lose the whiteout which use to cover the origin
lower object. Then fsck.overlay will fix these two inconsistency which
are not those test cases want to cover, thus it will lead to
fsck.overlay return FSCK_NONDESTRUCT instead of FSCK_OK. Fix these by
complement the missing overlay related features.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 tests/overlay/045 | 19 ++++++++++++++++---
 tests/overlay/046 | 13 +++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/tests/overlay/045 b/tests/overlay/045
index ed23258..ffca47f 100755
--- a/tests/overlay/045
+++ b/tests/overlay/045
@@ -37,6 +37,7 @@ _require_attrs
 _require_command "$FSCK_OVERLAY_PROG" fsck.overlay
 
 OVL_XATTR_OPAQUE_VAL=y
+OVL_XATTR_IMPURE_VAL=y
 
 # remove all files from previous tests
 _scratch_mkfs
@@ -69,6 +70,15 @@ make_opaque_dir()
 	$SETFATTR_PROG -n $OVL_XATTR_OPAQUE -v $OVL_XATTR_OPAQUE_VAL $target
 }
 
+# Create impure directories
+make_impure_dir()
+{
+	for dir in $*; do
+		mkdir -p $dir
+		$SETFATTR_PROG -n $OVL_XATTR_IMPURE -v $OVL_XATTR_IMPURE_VAL $dir
+	done
+}
+
 # Create a redirect directory
 make_redirect_dir()
 {
@@ -155,8 +165,9 @@ echo "+ Valid whiteout(2)"
 make_test_dirs
 mkdir $lowerdir/origin
 touch $lowerdir/origin/foo
+make_impure_dir $upperdir
 make_redirect_dir $upperdir/testdir "origin"
-make_whiteout $upperdir/testdir/foo
+make_whiteout $upperdir/origin $upperdir/testdir/foo
 
 _run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p
 check_whiteout $upperdir/testdir/foo
@@ -169,7 +180,8 @@ mkdir -p $lowerdir2/origin/subdir
 touch $lowerdir2/origin/subdir/foo
 make_redirect_dir $lowerdir/testdir "origin"
 mkdir -p $upperdir/testdir/subdir
-make_whiteout $upperdir/testdir/subdir/foo
+make_whiteout $lowerdir/origin $upperdir/testdir/subdir/foo
+make_impure_dir $upperdir/testdir $upperdir
 
 _run_check_fsck $FSCK_OK "$lowerdir:$lowerdir2" $upperdir $workdir -p
 check_whiteout $upperdir/testdir/subdir/foo
@@ -195,7 +207,8 @@ mkdir $lowerdir/origin
 touch $lowerdir/origin/foo
 make_opaque_dir $upperdir/testdir
 make_redirect_dir $upperdir/testdir/subdir "/origin"
-make_whiteout $upperdir/testdir/subdir/foo
+make_whiteout $upperdir/origin $upperdir/testdir/subdir/foo
+make_impure_dir $upperdir/testdir
 
 _run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p
 check_whiteout $upperdir/testdir/subdir/foo
diff --git a/tests/overlay/046 b/tests/overlay/046
index 21645c1..ea4b14f 100755
--- a/tests/overlay/046
+++ b/tests/overlay/046
@@ -40,6 +40,16 @@ _require_command "$FSCK_OVERLAY_PROG" fsck.overlay
 _scratch_mkfs
 
 OVL_XATTR_OPAQUE_VAL=y
+OVL_XATTR_IMPURE_VAL=y
+
+# Create impure directories
+make_impure_dir()
+{
+	for dir in $*; do
+		mkdir -p $dir
+		$SETFATTR_PROG -n $OVL_XATTR_IMPURE -v $OVL_XATTR_IMPURE_VAL $dir
+	done
+}
 
 # Create a redirect directory
 make_redirect_dir()
@@ -140,6 +150,7 @@ make_test_dirs
 mkdir $lowerdir/origin
 make_whiteout $upperdir/origin
 make_redirect_dir $upperdir/testdir "origin"
+make_impure_dir $upperdir
 
 _run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p
 check_redirect $upperdir/testdir "origin"
@@ -151,6 +162,7 @@ make_test_dirs
 mkdir $lowerdir/origin
 make_whiteout $upperdir/origin
 make_redirect_dir $upperdir/testdir1/testdir2 "/origin"
+make_impure_dir $upperdir/testdir1
 
 _run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p
 check_redirect $upperdir/testdir1/testdir2 "/origin"
@@ -172,6 +184,7 @@ make_test_dirs
 mkdir $lowerdir/{testdir1,testdir2}
 make_redirect_dir $upperdir/testdir1 "testdir2"
 make_redirect_dir $upperdir/testdir2 "testdir1"
+make_impure_dir $upperdir
 
 _run_check_fsck $FSCK_OK $lowerdir $upperdir $workdir -p
 check_redirect $upperdir/testdir1 "testdir2"
-- 
2.5.0

Re: [PATCH v2 2/4] overlay: fix exit code for some fsck.overlay valid cases

[Amir Goldstein]

On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>
> Some valid test cases about fsck.overlay may be not valid enough now,
> they lose the impure xattr on the parent directory of the simluated
> redirect directory, and lose the whiteout which use to cover the origin
> lower object. Then fsck.overlay will fix these two inconsistency which
> are not those test cases want to cover, thus it will lead to
> fsck.overlay return FSCK_NONDESTRUCT instead of FSCK_OK. Fix these by
> complement the missing overlay related features.
>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> ---

Ok. I think it's fine if we merge this fix now, but this way it is going
to be quite hard to maintain this test.

Imagine every time that you add another feature to fsck.overlay,
say "add overlay features xattr", fsck will start returning FSCK_NONDESTRUCT
and break this test.

Perhaps it would have been better to construct the test cases by:
- mount overlay
- create some copied up/ redirected  dirs and whiteouts
- umount overlay
- make minor modifications to upper/lower layer
- run fsck

Then you wouldn't need to worry about things like impure parent dir
and future overlay features.

I will leave it to you to decide if you want to fix this now or the
next time around...

Thanks,
Amir.

Re: [PATCH v2 2/4] overlay: fix exit code for some fsck.overlay valid cases

[zhangyi (F)]

On 2018/10/16 17:26, Amir Goldstein Wrote:
> On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>>
>> Some valid test cases about fsck.overlay may be not valid enough now,
>> they lose the impure xattr on the parent directory of the simluated
>> redirect directory, and lose the whiteout which use to cover the origin
>> lower object. Then fsck.overlay will fix these two inconsistency which
>> are not those test cases want to cover, thus it will lead to
>> fsck.overlay return FSCK_NONDESTRUCT instead of FSCK_OK. Fix these by
>> complement the missing overlay related features.
>>
>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>> ---
> 
> Ok. I think it's fine if we merge this fix now, but this way it is going
> to be quite hard to maintain this test.
> 
> Imagine every time that you add another feature to fsck.overlay,
> say "add overlay features xattr", fsck will start returning FSCK_NONDESTRUCT
> and break this test.
> 
> Perhaps it would have been better to construct the test cases by:
> - mount overlay
> - create some copied up/ redirected  dirs and whiteouts
> - umount overlay
> - make minor modifications to upper/lower layer
> - run fsck
> 
> Then you wouldn't need to worry about things like impure parent dir
> and future overlay features.
> 
> I will leave it to you to decide if you want to fix this now or the
> next time around...
> 

Indeed, I thought about this choice. If we create overlay on-disk features
(xattrs，whiteouts...) through overlayfs, the fsck tests results becomes
non-independent. It will depends on the kernel (overlayfs module) user are
running the test. Imaging if user want to test the latest fsck.overlay
on the old kernel which contains a compatible feature xattr fsck.overlay
know but the kernel don't, we will get the unexpected result. Or maybe
we can add some _require_xxx_feature() helper to enforce user doing test
on the kernel which supports the specified feature?

Thanks,
Yi.

Re: [PATCH v2 2/4] overlay: fix exit code for some fsck.overlay valid cases

[Amir Goldstein]

On Thu, Oct 18, 2018 at 6:43 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>
> On 2018/10/16 17:26, Amir Goldstein Wrote:
> > On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
> >>
> >> Some valid test cases about fsck.overlay may be not valid enough now,
> >> they lose the impure xattr on the parent directory of the simluated
> >> redirect directory, and lose the whiteout which use to cover the origin
> >> lower object. Then fsck.overlay will fix these two inconsistency which
> >> are not those test cases want to cover, thus it will lead to
> >> fsck.overlay return FSCK_NONDESTRUCT instead of FSCK_OK. Fix these by
> >> complement the missing overlay related features.
> >>
> >> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> >> ---
> >
> > Ok. I think it's fine if we merge this fix now, but this way it is going
> > to be quite hard to maintain this test.
> >
> > Imagine every time that you add another feature to fsck.overlay,
> > say "add overlay features xattr", fsck will start returning FSCK_NONDESTRUCT
> > and break this test.
> >
> > Perhaps it would have been better to construct the test cases by:
> > - mount overlay
> > - create some copied up/ redirected  dirs and whiteouts
> > - umount overlay
> > - make minor modifications to upper/lower layer
> > - run fsck
> >
> > Then you wouldn't need to worry about things like impure parent dir
> > and future overlay features.
> >
> > I will leave it to you to decide if you want to fix this now or the
> > next time around...
> >
>
> Indeed, I thought about this choice. If we create overlay on-disk features
> (xattrs，whiteouts...) through overlayfs, the fsck tests results becomes
> non-independent. It will depends on the kernel (overlayfs module) user are
> running the test. Imaging if user want to test the latest fsck.overlay
> on the old kernel which contains a compatible feature xattr fsck.overlay
> know but the kernel don't, we will get the unexpected result. Or maybe
> we can add some _require_xxx_feature() helper to enforce user doing test
> on the kernel which supports the specified feature?
>

I think the only sane choice is for this test to relax the expectation of 0
exit code to "correct" exit code (i.e. _overlay_repair_dirs()) for the "Valid"
test cases.

Maybe the only fsck run that we are fine with expecting 0 exit code is
-n run. As you can see this is common practice for e2fsck:
e2fsck -fn "${SCRATCH_DEV}" >> $seqres.full 2>&1 || _fail "fsck should not fail"

Thanks,
Amir.

Re: [PATCH v2 2/4] overlay: fix exit code for some fsck.overlay valid cases

[zhangyi (F)]

On 2018/10/18 12:44, Amir Goldstein Wrote:
> On Thu, Oct 18, 2018 at 6:43 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>>
>> On 2018/10/16 17:26, Amir Goldstein Wrote:
>>> On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>>
>>>> Some valid test cases about fsck.overlay may be not valid enough now,
>>>> they lose the impure xattr on the parent directory of the simluated
>>>> redirect directory, and lose the whiteout which use to cover the origin
>>>> lower object. Then fsck.overlay will fix these two inconsistency which
>>>> are not those test cases want to cover, thus it will lead to
>>>> fsck.overlay return FSCK_NONDESTRUCT instead of FSCK_OK. Fix these by
>>>> complement the missing overlay related features.
>>>>
>>>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>>>> ---
>>>
>>> Ok. I think it's fine if we merge this fix now, but this way it is going
>>> to be quite hard to maintain this test.
>>>
>>> Imagine every time that you add another feature to fsck.overlay,
>>> say "add overlay features xattr", fsck will start returning FSCK_NONDESTRUCT
>>> and break this test.
>>>
>>> Perhaps it would have been better to construct the test cases by:
>>> - mount overlay
>>> - create some copied up/ redirected  dirs and whiteouts
>>> - umount overlay
>>> - make minor modifications to upper/lower layer
>>> - run fsck
>>>
>>> Then you wouldn't need to worry about things like impure parent dir
>>> and future overlay features.
>>>
>>> I will leave it to you to decide if you want to fix this now or the
>>> next time around...
>>>
>>
>> Indeed, I thought about this choice. If we create overlay on-disk features
>> (xattrs，whiteouts...) through overlayfs, the fsck tests results becomes
>> non-independent. It will depends on the kernel (overlayfs module) user are
>> running the test. Imaging if user want to test the latest fsck.overlay
>> on the old kernel which contains a compatible feature xattr fsck.overlay
>> know but the kernel don't, we will get the unexpected result. Or maybe
>> we can add some _require_xxx_feature() helper to enforce user doing test
>> on the kernel which supports the specified feature?
>>
> 
> I think the only sane choice is for this test to relax the expectation of 0
> exit code to "correct" exit code (i.e. _overlay_repair_dirs()) for the "Valid"
> test cases.
> 
The meaning of the "valid" test cases is to make sure fsck.overlay will never
change the on-disk filesystem if the feature(xattr) we want to test is valid,
so the FSCK_OK and FSCK_NONDESTRUCT is totally different.

If we relax the expectation of 0(FSCK_OK) exit code, we couldn't distinguish
the fsck was changed the file system or not, if so, we also couldn't distinguish
it's caused by some bugs of fsck or the base dirs were not valid enough. Then
the "valid" test cases cannot catch fsck's fault accurately. So I think make
a valid enough overlay image manually now is still the best way.

I think maybe after we introduce "feature set" xattr, it will becomes much easier,
fsck.overlay will fix things according to feature set, and we create overlay image
through mkfs.overlay. So we could disable some irrelevant features to avoid
disturbing our tests. Is it fine?

Thanks,
Yi.

> Maybe the only fsck run that we are fine with expecting 0 exit code is
> -n run. As you can see this is common practice for e2fsck:
> e2fsck -fn "${SCRATCH_DEV}" >> $seqres.full 2>&1 || _fail "fsck should not fail"
> 
> Thanks,
> Amir.
> 
> .
> 

Re: [PATCH v2 2/4] overlay: fix exit code for some fsck.overlay valid cases

[Amir Goldstein]

On Fri, Oct 19, 2018 at 3:36 PM zhangyi (F) <yi.zhang@huawei.com> wrote:
>
> On 2018/10/18 12:44, Amir Goldstein Wrote:
> > On Thu, Oct 18, 2018 at 6:43 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
> >>
> >> On 2018/10/16 17:26, Amir Goldstein Wrote:
> >>> On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
> >>>>
> >>>> Some valid test cases about fsck.overlay may be not valid enough now,
> >>>> they lose the impure xattr on the parent directory of the simluated
> >>>> redirect directory, and lose the whiteout which use to cover the origin
> >>>> lower object. Then fsck.overlay will fix these two inconsistency which
> >>>> are not those test cases want to cover, thus it will lead to
> >>>> fsck.overlay return FSCK_NONDESTRUCT instead of FSCK_OK. Fix these by
> >>>> complement the missing overlay related features.
> >>>>
> >>>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> >>>> ---
> >>>
> >>> Ok. I think it's fine if we merge this fix now, but this way it is going
> >>> to be quite hard to maintain this test.
> >>>
> >>> Imagine every time that you add another feature to fsck.overlay,
> >>> say "add overlay features xattr", fsck will start returning FSCK_NONDESTRUCT
> >>> and break this test.
> >>>
> >>> Perhaps it would have been better to construct the test cases by:
> >>> - mount overlay
> >>> - create some copied up/ redirected  dirs and whiteouts
> >>> - umount overlay
> >>> - make minor modifications to upper/lower layer
> >>> - run fsck
> >>>
> >>> Then you wouldn't need to worry about things like impure parent dir
> >>> and future overlay features.
> >>>
> >>> I will leave it to you to decide if you want to fix this now or the
> >>> next time around...
> >>>
> >>
> >> Indeed, I thought about this choice. If we create overlay on-disk features
> >> (xattrs，whiteouts...) through overlayfs, the fsck tests results becomes
> >> non-independent. It will depends on the kernel (overlayfs module) user are
> >> running the test. Imaging if user want to test the latest fsck.overlay
> >> on the old kernel which contains a compatible feature xattr fsck.overlay
> >> know but the kernel don't, we will get the unexpected result. Or maybe
> >> we can add some _require_xxx_feature() helper to enforce user doing test
> >> on the kernel which supports the specified feature?
> >>
> >
> > I think the only sane choice is for this test to relax the expectation of 0
> > exit code to "correct" exit code (i.e. _overlay_repair_dirs()) for the "Valid"
> > test cases.
> >
> The meaning of the "valid" test cases is to make sure fsck.overlay will never
> change the on-disk filesystem if the feature(xattr) we want to test is valid,
> so the FSCK_OK and FSCK_NONDESTRUCT is totally different.
>
> If we relax the expectation of 0(FSCK_OK) exit code, we couldn't distinguish
> the fsck was changed the file system or not, if so, we also couldn't distinguish
> it's caused by some bugs of fsck or the base dirs were not valid enough. Then
> the "valid" test cases cannot catch fsck's fault accurately. So I think make
> a valid enough overlay image manually now is still the best way.
>
> I think maybe after we introduce "feature set" xattr, it will becomes much easier,
> fsck.overlay will fix things according to feature set, and we create overlay image
> through mkfs.overlay. So we could disable some irrelevant features to avoid
> disturbing our tests. Is it fine?
>

Its fine by me to re-open this for discussion that next time fsck.overlay
changes and breaks the test.

Thanks,
Amir.

[PATCH v2 3/4] overlay: add fsck.overlay stress test

[zhangyi (F)]

Introduce a test case for fsck.overlay which runs on the underlying
directories created by fsstress (contain a lot of fs objects) to
find potential stability issue.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 tests/overlay/062     | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/062.out |  2 ++
 tests/overlay/group   |  1 +
 3 files changed, 87 insertions(+)
 create mode 100755 tests/overlay/062
 create mode 100644 tests/overlay/062.out

diff --git a/tests/overlay/062 b/tests/overlay/062
new file mode 100755
index 0000000..f818562
--- /dev/null
+++ b/tests/overlay/062
@@ -0,0 +1,84 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2018 Huawei.  All Rights Reserved.
+#
+# FS QA Test No. 062
+#
+# Stress test: test fsck.overlay running on the underlying dirs
+# which were created by fsstress.
+#
+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()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/attr
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+_supported_fs overlay
+_supported_os Linux
+_require_scratch_nocheck
+_require_command "$FSCK_OVERLAY_PROG" fsck.overlay
+
+# remove all files from previous tests
+_scratch_mkfs
+
+# Create an underlying layer which contain a lot of random objects
+create_layer()
+{
+	for dir in $*; do
+		seed=$RANDOM
+		echo "create random file system objects in $dir with seed $seed" \
+			>> $seqres.full
+
+		$FSSTRESS_PROG -s $seed -d $dir -z \
+			-f creat=20 -f link=10 -f mkdir=20 -f mknod=10 \
+			-f rename=10 -f setxattr=10 -f symlink=10 -f write=10 \
+			-p 4 -n 500 -l50 > /dev/null 2>&1
+	done
+}
+
+
+# Create test directories
+lowerdir=$OVL_BASE_SCRATCH_MNT/$seq-ovl-lower
+lowerdir2=$OVL_BASE_SCRATCH_MNT/$seq-ovl-lower2
+upperdir=$OVL_BASE_SCRATCH_MNT/$seq-ovl-upper
+workdir=$OVL_BASE_SCRATCH_MNT/$seq-ovl-workdir
+
+make_test_dirs()
+{
+	rm -rf $lowerdir $lowerdir2 $upperdir $workdir
+	mkdir -p $lowerdir $lowerdir2 $upperdir $workdir
+}
+
+
+# Test stability, should not crash and should not fail on "yes" mode.
+make_test_dirs
+create_layer $lowerdir2 $lowerdir $upperdir
+
+_overlay_fsck_dirs "$lowerdir:$lowerdir2" $upperdir $workdir -y >> \
+	$seqres.full 2>&1
+
+[[ "$?" == "$FSCK_OK" || "$?" == "$FSCK_NONDESTRUCT" ]] || \
+	echo "fsck return unexpected $?"
+
+# success, all done
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/overlay/062.out b/tests/overlay/062.out
new file mode 100644
index 0000000..a1578f4
--- /dev/null
+++ b/tests/overlay/062.out
@@ -0,0 +1,2 @@
+QA output created by 062
+Silence is golden
diff --git a/tests/overlay/group b/tests/overlay/group
index ccc71f3..b308427 100644
--- a/tests/overlay/group
+++ b/tests/overlay/group
@@ -64,3 +64,4 @@
 059 auto quick copyup
 060 auto quick metacopy
 061 auto quick copyup
+062 auto stress fsck
-- 
2.5.0

Re: [PATCH v2 3/4] overlay: add fsck.overlay stress test

[Amir Goldstein]

On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>
> Introduce a test case for fsck.overlay which runs on the underlying
> directories created by fsstress (contain a lot of fs objects) to
> find potential stability issue.
>

Looks good.
Did it help you find any stability issues?

Thanks,
Amir.

Re: [PATCH v2 3/4] overlay: add fsck.overlay stress test

[zhangyi (F)]

On 2018/10/16 18:07, Amir Goldstein Wrote:
> On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>>
>> Introduce a test case for fsck.overlay which runs on the underlying
>> directories created by fsstress (contain a lot of fs objects) to
>> find potential stability issue.
>>
> 
> Looks good.
> Did it help you find any stability issues?
> 

Not yet, it looks fine everytime I run this test, maybe could help to
find in the future. :)

Thanks,
Yi.

[PATCH v2 4/4] overlay: add fsck.overlay exception tests

[zhangyi (F)]

Introduce some exception test cases for fsck.overlay which runs on
invalid/unnormal underlying dirs or invalid input parameters to test
exception handling processes.

This patch also change _overlay_fsck_dirs() helper function to be able
to receive empty underlying dir arguments, which is used by this test
to cover the case of incomplete underlying dirs.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 common/overlay        |   8 +-
 tests/overlay/063     | 217 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/063.out |  10 +++
 tests/overlay/group   |   1 +
 4 files changed, 234 insertions(+), 2 deletions(-)
 create mode 100755 tests/overlay/063
 create mode 100644 tests/overlay/063.out

diff --git a/common/overlay b/common/overlay
index 4cc2829..2896594 100644
--- a/common/overlay
+++ b/common/overlay
@@ -195,12 +195,16 @@ _overlay_fsck_dirs()
 	local lowerdir=$1
 	local upperdir=$2
 	local workdir=$3
+	local dirs=""
 	shift 3
 
 	[[ ! -x "$FSCK_OVERLAY_PROG" ]] && return 0
 
-	$FSCK_OVERLAY_PROG -o lowerdir=$lowerdir -o upperdir=$upperdir \
-			   -o workdir=$workdir $*
+	[[ -n "$lowerdir" ]] && dirs=$(echo -n "-o lowerdir=$lowerdir")
+	[[ -n "$upperdir" ]] && dirs=$(echo -n "$dirs -o upperdir=$upperdir")
+	[[ -n "$workdir" ]] && dirs=$(echo -n "$dirs -o workdir=$workdir")
+
+	$FSCK_OVERLAY_PROG $dirs $*
 }
 
 # Run fsck and check the return value
diff --git a/tests/overlay/063 b/tests/overlay/063
new file mode 100755
index 0000000..e2c771a
--- /dev/null
+++ b/tests/overlay/063
@@ -0,0 +1,217 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2018 Huawei.  All Rights Reserved.
+#
+# FS QA Test No. 063
+#
+# Exception test: test fsck.overlay runs on invalid underlying
+# dirs or with invalid input options.
+#
+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()
+{
+	cd /
+	$CHATTR_PROG -i $upperdir/testdir
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/attr
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+_supported_fs overlay
+_supported_os Linux
+_require_scratch_nocheck
+_require_attrs
+_require_command "$FSCK_OVERLAY_PROG" fsck.overlay
+_require_chattr i
+
+# remove all files from previous tests
+_scratch_mkfs
+
+# Create a whiteout
+make_whiteout()
+{
+	for arg in $*; do
+		mknod $arg c 0 0
+	done
+}
+
+# Create a redirect directory
+make_redirect_dir()
+{
+	local target=$1
+	local value=$2
+
+	mkdir -p $target
+	$SETFATTR_PROG -n $OVL_XATTR_REDIRECT -v $value $target
+}
+
+# Create test directories
+lowerdir=$OVL_BASE_SCRATCH_MNT/lower
+lowerdir2=$OVL_BASE_SCRATCH_MNT/lower2
+upperdir=$OVL_BASE_SCRATCH_MNT/upper
+workdir=$OVL_BASE_SCRATCH_MNT/workdir
+
+make_test_dirs()
+{
+	rm -rf $lowerdir $lowerdir2 $upperdir $workdir
+	mkdir -p $lowerdir $lowerdir2 $upperdir $workdir
+}
+
+# Test incomplete underlying dirs, should fail
+echo "+ Invalid input"
+make_test_dirs
+
+none_dir=""
+
+_run_check_fsck $FSCK_USAGE $lowerdir "$none_dir" "$none_dir"
+_run_check_fsck $FSCK_USAGE $lowerdir $upperdir "$none_dir"
+_run_check_fsck $FSCK_USAGE "$none_dir" $upperdir $workdir
+
+
+# Test invalid underlying dirs, should fail
+echo "+ Invalid input(2)"
+make_test_dirs
+
+invalid_dir=$OVL_BASE_SCRATCH_MNT/invalid_dir
+
+_run_check_fsck $FSCK_ERROR $lowerdir $upperdir $invalid_dir
+_run_check_fsck $FSCK_ERROR $lowerdir $invalid_dir $workdir
+_run_check_fsck $FSCK_ERROR $invalid_dir $upperdir $workdir
+
+
+# Test conflict input options, should fail
+echo "+ Invalid input(3)"
+make_test_dirs
+
+_run_check_fsck $FSCK_USAGE $lowerdir $upperdir $workdir -pn
+_run_check_fsck $FSCK_USAGE $lowerdir $upperdir $workdir -yn
+_run_check_fsck $FSCK_USAGE $lowerdir $upperdir $workdir -py
+_run_check_fsck $FSCK_USAGE $lowerdir $upperdir $workdir -pyn
+
+
+# Test upperdir and workdir belong to different base filesystems, should fail
+echo "+ Invalid workdir"
+make_test_dirs
+
+test_workdir=$OVL_BASE_TEST_DIR/work
+mkdir -p $test_workdir
+_run_check_fsck $FSCK_ERROR $lowerdir $upperdir $test_workdir
+rm -rf $test_workdir
+
+
+# Test upperdir is subdir of workdir and vice versa, should fail
+echo "+ Invalid workdir(2)"
+make_test_dirs
+
+test_workdir=$upperdir/work
+test_upperdir=$workdir/upper
+mkdir -p $test_workdir $test_upperdir
+
+_run_check_fsck $FSCK_ERROR $lowerdir $upperdir $test_workdir
+_run_check_fsck $FSCK_ERROR $lowerdir $test_upperdir $workdir
+
+
+# Test upper layer is read-only, should fail in "!no" mode, and should
+# return the real consistent result in "no" mode.
+echo "+ Upper read-only"
+make_test_dirs
+
+test_lowerdir=$OVL_BASE_TEST_DIR/lower
+mkdir -p $test_lowerdir
+
+# Let upper layer read-only
+$MOUNT_PROG -o remount,ro $OVL_BASE_SCRATCH_MNT > /dev/null 2>&1
+# Refuse to check read-only upper layer
+_run_check_fsck $FSCK_ERROR $test_lowerdir $upperdir $workdir
+_run_check_fsck $FSCK_ERROR $test_lowerdir $upperdir $workdir -p
+_run_check_fsck $FSCK_ERROR $test_lowerdir $upperdir $workdir -y
+# Allow to use "no" mode scan read-only upper layer
+_run_check_fsck $FSCK_OK $test_lowerdir $upperdir $workdir -n
+$MOUNT_PROG -o remount,rw $OVL_BASE_SCRATCH_MNT > /dev/null 2>&1
+
+# Make a simple inconsistency on the upper layer and expect return failure
+make_whiteout $upperdir/invalid
+$MOUNT_PROG -o remount,ro $OVL_BASE_SCRATCH_MNT > /dev/null 2>&1
+_run_check_fsck $FSCK_UNCORRECTED $test_lowerdir $upperdir $workdir -n
+$MOUNT_PROG -o remount,rw $OVL_BASE_SCRATCH_MNT > /dev/null 2>&1
+rm -rf $test_lowerdir
+
+
+# Test lower layer is read-only, should sacn each layer and return
+# the real consistent result.
+echo "+ Lower read-only"
+make_test_dirs
+
+test_lowerdir=$OVL_BASE_TEST_DIR/lower
+mkdir -p $test_lowerdir
+
+# Let lower layer read-only
+$MOUNT_PROG -o remount,ro $OVL_BASE_TEST_DIR > /dev/null 2>&1
+# Allow check read-only lower layers in all modes
+_run_check_fsck $FSCK_OK $test_lowerdir $upperdir $workdir -p
+$MOUNT_PROG -o remount,rw $OVL_BASE_TEST_DIR > /dev/null 2>&1
+
+# Make a simple inconsistency on the read-only lower layer and expect
+# return failure.
+make_whiteout $test_lowerdir/invalid
+$MOUNT_PROG -o remount,ro $OVL_BASE_TEST_DIR > /dev/null 2>&1
+_run_check_fsck $FSCK_UNCORRECTED $test_lowerdir $upperdir $workdir -p
+$MOUNT_PROG -o remount,rw $OVL_BASE_TEST_DIR > /dev/null 2>&1
+rm -rf $test_lowerdir
+
+
+# Test one of the lower layers is read-only, should sacn each layer and
+# return the real consistent result.
+echo "+ Lower read-only(2)"
+make_test_dirs
+
+test_lowerdir=$OVL_BASE_TEST_DIR/lower
+mkdir -p $test_lowerdir
+
+# Let lower layer read-only
+$MOUNT_PROG -o remount,ro $OVL_BASE_TEST_DIR > /dev/null 2>&1
+# Make a simple inconsistency on the bottom read-write lower layer
+# and expect return success (consistent middle read-only layer should
+# not affect the result).
+make_whiteout $lowerdir2/invalid
+_run_check_fsck $FSCK_NONDESTRUCT $test_lowerdir:$lowerdir2 $upperdir $workdir -p
+$MOUNT_PROG -o remount,rw $OVL_BASE_TEST_DIR > /dev/null 2>&1
+
+# Make a simple inconsistency on the middle read-only lower layer
+# and expect return failure.
+make_whiteout $test_lowerdir/invalid
+$MOUNT_PROG -o remount,ro $OVL_BASE_TEST_DIR > /dev/null 2>&1
+_run_check_fsck $FSCK_UNCORRECTED $test_lowerdir:$lowerdir2 $upperdir $workdir -p
+$MOUNT_PROG -o remount,rw $OVL_BASE_TEST_DIR > /dev/null 2>&1
+rm -rf $test_lowerdir
+
+
+# Test encounter error when try to fix some inconsistency, should fail
+echo "+ Encounter error"
+make_test_dirs
+
+# Make a simple inconsistency and set immutable flag to simulate fix error
+make_redirect_dir $upperdir/testdir "invalid"
+
+$CHATTR_PROG +i $upperdir/testdir
+_run_check_fsck $(($FSCK_UNCORRECTED+$FSCK_ERROR)) $lowerdir $upperdir $workdir -p
+$CHATTR_PROG -i $upperdir/testdir
+
+# success, all done
+status=0
+exit
diff --git a/tests/overlay/063.out b/tests/overlay/063.out
new file mode 100644
index 0000000..2c5dd37
--- /dev/null
+++ b/tests/overlay/063.out
@@ -0,0 +1,10 @@
+QA output created by 063
++ Invalid input
++ Invalid input(2)
++ Invalid input(3)
++ Invalid workdir
++ Invalid workdir(2)
++ Upper read-only
++ Lower read-only
++ Lower read-only(2)
++ Encounter error
diff --git a/tests/overlay/group b/tests/overlay/group
index b308427..668329d 100644
--- a/tests/overlay/group
+++ b/tests/overlay/group
@@ -65,3 +65,4 @@
 060 auto quick metacopy
 061 auto quick copyup
 062 auto stress fsck
+063 auto quick fsck
-- 
2.5.0

Re: [PATCH v2 4/4] overlay: add fsck.overlay exception tests

[Amir Goldstein]

On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>
> Introduce some exception test cases for fsck.overlay which runs on
> invalid/unnormal underlying dirs or invalid input parameters to test
> exception handling processes.
>
> This patch also change _overlay_fsck_dirs() helper function to be able
> to receive empty underlying dir arguments, which is used by this test
> to cover the case of incomplete underlying dirs.

I think this change is not needed and only adds noise (see below)

>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> ---
>  common/overlay        |   8 +-
>  tests/overlay/063     | 217 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/overlay/063.out |  10 +++
>  tests/overlay/group   |   1 +
>  4 files changed, 234 insertions(+), 2 deletions(-)
>  create mode 100755 tests/overlay/063
>  create mode 100644 tests/overlay/063.out
>
> diff --git a/common/overlay b/common/overlay
> index 4cc2829..2896594 100644
> --- a/common/overlay
> +++ b/common/overlay
> @@ -195,12 +195,16 @@ _overlay_fsck_dirs()
>         local lowerdir=$1
>         local upperdir=$2
>         local workdir=$3
> +       local dirs=""
>         shift 3
>
>         [[ ! -x "$FSCK_OVERLAY_PROG" ]] && return 0
>
> -       $FSCK_OVERLAY_PROG -o lowerdir=$lowerdir -o upperdir=$upperdir \
> -                          -o workdir=$workdir $*
> +       [[ -n "$lowerdir" ]] && dirs=$(echo -n "-o lowerdir=$lowerdir")
> +       [[ -n "$upperdir" ]] && dirs=$(echo -n "$dirs -o upperdir=$upperdir")
> +       [[ -n "$workdir" ]] && dirs=$(echo -n "$dirs -o workdir=$workdir")
> +

passing non last empty args in bash is asking for trouble to begin with.
the echo above is really unneeded, but anyway, this whole change gives
no benefit at all, because the only user calling with empty args is just better
off executing $FSCK_OVERLAY_PROG directly.
The test isn't going to run anyway if $FSCK_OVERLAY_PROG does not exist.

> +       $FSCK_OVERLAY_PROG $dirs $*
>  }
>
>  # Run fsck and check the return value
> diff --git a/tests/overlay/063 b/tests/overlay/063
> new file mode 100755
> index 0000000..e2c771a
> --- /dev/null
> +++ b/tests/overlay/063
> @@ -0,0 +1,217 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2018 Huawei.  All Rights Reserved.
> +#
> +# FS QA Test No. 063
> +#
> +# Exception test: test fsck.overlay runs on invalid underlying
> +# dirs or with invalid input options.
> +#
> +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()
> +{
> +       cd /
> +       $CHATTR_PROG -i $upperdir/testdir
> +       rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/attr
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +_supported_fs overlay
> +_supported_os Linux
> +_require_scratch_nocheck
> +_require_attrs
> +_require_command "$FSCK_OVERLAY_PROG" fsck.overlay
> +_require_chattr i
> +
> +# remove all files from previous tests
> +_scratch_mkfs
> +
> +# Create a whiteout
> +make_whiteout()
> +{
> +       for arg in $*; do
> +               mknod $arg c 0 0
> +       done
> +}
> +
> +# Create a redirect directory
> +make_redirect_dir()
> +{
> +       local target=$1
> +       local value=$2
> +
> +       mkdir -p $target
> +       $SETFATTR_PROG -n $OVL_XATTR_REDIRECT -v $value $target
> +}
> +
> +# Create test directories
> +lowerdir=$OVL_BASE_SCRATCH_MNT/lower
> +lowerdir2=$OVL_BASE_SCRATCH_MNT/lower2
> +upperdir=$OVL_BASE_SCRATCH_MNT/upper
> +workdir=$OVL_BASE_SCRATCH_MNT/workdir
> +
> +make_test_dirs()
> +{
> +       rm -rf $lowerdir $lowerdir2 $upperdir $workdir
> +       mkdir -p $lowerdir $lowerdir2 $upperdir $workdir
> +}
> +
> +# Test incomplete underlying dirs, should fail
> +echo "+ Invalid input"
> +make_test_dirs
> +
> +none_dir=""
> +
> +_run_check_fsck $FSCK_USAGE $lowerdir "$none_dir" "$none_dir"
> +_run_check_fsck $FSCK_USAGE $lowerdir $upperdir "$none_dir"
> +_run_check_fsck $FSCK_USAGE "$none_dir" $upperdir $workdir
> +
> +
> +# Test invalid underlying dirs, should fail
> +echo "+ Invalid input(2)"
> +make_test_dirs
> +
> +invalid_dir=$OVL_BASE_SCRATCH_MNT/invalid_dir
> +
> +_run_check_fsck $FSCK_ERROR $lowerdir $upperdir $invalid_dir
> +_run_check_fsck $FSCK_ERROR $lowerdir $invalid_dir $workdir
> +_run_check_fsck $FSCK_ERROR $invalid_dir $upperdir $workdir
> +
> +
> +# Test conflict input options, should fail
> +echo "+ Invalid input(3)"
> +make_test_dirs
> +
> +_run_check_fsck $FSCK_USAGE $lowerdir $upperdir $workdir -pn
> +_run_check_fsck $FSCK_USAGE $lowerdir $upperdir $workdir -yn
> +_run_check_fsck $FSCK_USAGE $lowerdir $upperdir $workdir -py
> +_run_check_fsck $FSCK_USAGE $lowerdir $upperdir $workdir -pyn
> +
> +
> +# Test upperdir and workdir belong to different base filesystems, should fail
> +echo "+ Invalid workdir"
> +make_test_dirs
> +
> +test_workdir=$OVL_BASE_TEST_DIR/work

When using test partition tests usually pick a more unique name,
e.g. work-$seq (even though it is not really unique among generic/$seq
and overlay/$seq).

> +mkdir -p $test_workdir
> +_run_check_fsck $FSCK_ERROR $lowerdir $upperdir $test_workdir
> +rm -rf $test_workdir

Similarly, tests usually clean dirs from test partition BEFORE using it
(maybe another test created a file named work?) and often leave it around
after the test, just so test partition gets aged and contains files.
But cleaning the test dir is fine as well.

> +
> +
> +# Test upperdir is subdir of workdir and vice versa, should fail
> +echo "+ Invalid workdir(2)"
> +make_test_dirs
> +
> +test_workdir=$upperdir/work
> +test_upperdir=$workdir/upper
> +mkdir -p $test_workdir $test_upperdir
> +
> +_run_check_fsck $FSCK_ERROR $lowerdir $upperdir $test_workdir
> +_run_check_fsck $FSCK_ERROR $lowerdir $test_upperdir $workdir
> +
> +
> +# Test upper layer is read-only, should fail in "!no" mode, and should
> +# return the real consistent result in "no" mode.
> +echo "+ Upper read-only"
> +make_test_dirs
> +
> +test_lowerdir=$OVL_BASE_TEST_DIR/lower

same comment as test workdir above.

> +mkdir -p $test_lowerdir
> +
> +# Let upper layer read-only
> +$MOUNT_PROG -o remount,ro $OVL_BASE_SCRATCH_MNT > /dev/null 2>&1
> +# Refuse to check read-only upper layer
> +_run_check_fsck $FSCK_ERROR $test_lowerdir $upperdir $workdir
> +_run_check_fsck $FSCK_ERROR $test_lowerdir $upperdir $workdir -p
> +_run_check_fsck $FSCK_ERROR $test_lowerdir $upperdir $workdir -y
> +# Allow to use "no" mode scan read-only upper layer
> +_run_check_fsck $FSCK_OK $test_lowerdir $upperdir $workdir -n
> +$MOUNT_PROG -o remount,rw $OVL_BASE_SCRATCH_MNT > /dev/null 2>&1

Problem: unless you add remount,rw for both base scratch and base test fs
in cleanup(), _check_filesystems() -> ... _overlay_check_fs() won't remount
it rw for the next test (in case _run_check_fsck has a fatal error).
IMO, it will be cleaner to fix that in _overlay_check_fs() and not just in
cleanup() of this test.
_check_generic_filesystem() will recover from a read-only scratch/test,
so it makes some sense that _overlay_check_fs() will provide a similar
guaranty and shouldn't be difficult at all to do:

Thanks,
Amir.

Re: [PATCH v2 0/4] overlay: enhance fsck.overlay test cases

[Amir Goldstein]

On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>
> Hi all,
>
> These are the second version of fsck.overlay[1] program related patch set
> (the first one was posted about two mounths ago[2], sorry for the late).

Oh right. I managed to forget about that...
Thanks for following up.

Amir.

Re: [PATCH v2 0/4] overlay: enhance fsck.overlay test cases

[Amir Goldstein]

On Tue, Oct 16, 2018 at 12:27 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
> >
> > Hi all,
> >
> > These are the second version of fsck.overlay[1] program related patch set
> > (the first one was posted about two mounths ago[2], sorry for the late).
>
> Oh right. I managed to forget about that...
> Thanks for following up.
>

Zhangyi,

For the next submission, please CC  linux-unionfs.
I bet folks are interested to know about progress with fsck.overlay.

And if you can provide a public branch with the tests, that would be nice.

Thanks,
Amir.

Re: [PATCH v2 0/4] overlay: enhance fsck.overlay test cases

[zhangyi (F)]

On 2018/10/16 20:39, Amir Goldstein Wrote:
> On Tue, Oct 16, 2018 at 12:27 PM Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>
>>> Hi all,
>>>
>>> These are the second version of fsck.overlay[1] program related patch set
>>> (the first one was posted about two mounths ago[2], sorry for the late).
>>
>> Oh right. I managed to forget about that...
>> Thanks for following up.
>>
> 
> Zhangyi,
> 
> For the next submission, please CC  linux-unionfs.
> I bet folks are interested to know about progress with fsck.overlay.
> 
> And if you can provide a public branch with the tests, that would be nice.
> 
Hi, Amir

Thanks for your comments, will do.

Thanks,
Yi.