[xfstests PATCH v2 0/5] overlay: add overlay filesystem dirs check

[xfstests PATCH v2 0/5] overlay: add overlay filesystem dirs check

[zhangyi (F)]

Hi, All:

This patchset implement filesystem check for overlay filesystem, base on
my "overlay: add fsck.overlay basic tests" patchset and works only if
fsck.overlay[1] exists (otherwise not run).

Patch 1 improve and fix _is_mounted helper in common/rc.
Patch 2 hook fsck helper to _check_test_fs & _check_scratch_fs, and
introduce helpers for optionally scratch dirs.
Patch 3~5 modify overlay test cases to do fs check correctly.

Hi, Eryu:

The patch 5/5 conflict with Amir's patchset, let me know if you want me
to rebase it.

Thanks,
Yi.

[1] https://github.com/hisilicon/overlayfs-progs

Changes since v1:
- Details comments in patch 1/5.
- Improve _overlay_check_scratch_dirs to accept extra options.
- Fix basic filesystem mounted check in _overlay_check_fs.
- Improve overlay/044 in patch 5/5 to add index=on option.


zhangyi (F) (5):
  common/rc: improve mounted check helper
  overlay: hook filesystem check helper
  overlay/003: fix fs check failure
  overlay: skip check for tests finished with corrupt filesystem
  overlay: correct scratch dirs check

 common/overlay    | 128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 common/rc         |  29 +++++--------
 tests/overlay/003 |   1 -
 tests/overlay/005 |   6 ++-
 tests/overlay/010 |   6 ++-
 tests/overlay/014 |  10 ++++-
 tests/overlay/019 |   2 +-
 tests/overlay/031 |   2 +-
 tests/overlay/035 |   6 ++-
 tests/overlay/036 |   5 ++-
 tests/overlay/037 |   6 ++-
 tests/overlay/038 |  10 ++++-
 tests/overlay/041 |  10 ++++-
 tests/overlay/043 |   6 ++-
 tests/overlay/044 |  13 +++++-
 15 files changed, 209 insertions(+), 31 deletions(-)

-- 
2.5.0

[xfstests PATCH v2 1/5] common/rc: improve mounted check helper

[zhangyi (F)]

Modify _is_mounted() to accept a dir and fstype as input, and check
whether this dir is a specified type of mount point.

This patch also fix the problem of missing fstype check:

For example:
  Base mounted filesystem:
    /dev/sda2 on /boot type ext4 (rw,relatime,data=ordered)

  FSTYPE=xfs
  mountpoint=`_is_mounted /dev/sda1`
  echo "$mountpoint"

  Output: /boot

This patch remove the useless error message, return empty if no
valid mount point.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 common/rc | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/common/rc b/common/rc
index 2e3a83a..3351f00 100644
--- a/common/rc
+++ b/common/rc
@@ -2372,27 +2372,22 @@ _scratch_mkfs_richacl()
 	esac
 }
 
-# check that a FS on a device is mounted
+# check that a FS on a device is mounted or a dir is a mount point
 # if so, return mount point
 #
 _is_mounted()
 {
-    if [ $# -ne 1 ]
-    then
-	echo "Usage: _is_mounted device" 1>&2
-	exit 1
-    fi
+	if [ $# -lt 1 ]; then
+		echo "Usage: _is_mounted <device|mountpoint> [fstype]" 1>&2
+		exit 1
+	fi
 
-    device=$1
+	local name=$1
+	local fstype=${2-$FSTYP}
 
-    if _mount | grep "$device " | $AWK_PROG -v pattern="type $FSTYP" '
-        pattern        { print $3 ; exit 0 }
-        END            { exit 1 }
-    '
-    then
-        echo "_is_mounted: $device is not a mounted $FSTYP FS"
-        exit 1
-    fi
+	_mount | grep "$name " | $AWK_PROG -v pattern="type $fstype" '
+		$0 ~ pattern	{ print $3 }
+	'
 }
 
 # remount a FS to a new mode (ro or rw)
-- 
2.5.0

Re: [xfstests PATCH v2 1/5] common/rc: improve mounted check helper

[Eryu Guan]

On Wed, Jan 31, 2018 at 06:27:55PM +0800, zhangyi (F) wrote:
> Modify _is_mounted() to accept a dir and fstype as input, and check
> whether this dir is a specified type of mount point.
> 
> This patch also fix the problem of missing fstype check:
> 
> For example:
>   Base mounted filesystem:
>     /dev/sda2 on /boot type ext4 (rw,relatime,data=ordered)
> 
>   FSTYPE=xfs
>   mountpoint=`_is_mounted /dev/sda1`
>   echo "$mountpoint"
> 
>   Output: /boot
> 
> This patch remove the useless error message, return empty if no
> valid mount point.
> 
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  common/rc | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 2e3a83a..3351f00 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2372,27 +2372,22 @@ _scratch_mkfs_richacl()
>  	esac
>  }
>  
> -# check that a FS on a device is mounted
> +# check that a FS on a device is mounted or a dir is a mount point

Hmm, I'm thinking about introducing a new _is_dir_mountpoint() function
for overlay use, and rename _is_mounted to _is_dev_mounted, so we don't
overload _is_mounted.

Further, I think we could take use of findmnt instead of parsing the
output from mount, which could be hard to do correctly. e.g.

# check if the given device is mounted, if so, return mount point
_is_dev_mounted()
{
	local dev=$1
	local fstype=${2:-$FSTYP}

	if [ $# -lt 1 ]; then
		echo "Usage: _is_dev_mounted <device> [fstype]" >&2
		exit 1
	fi
	findmnt -rncv -S $dev -t $fstype -o TARGET | head -1
}

And similarly

# check if the given dir is a mount point, if so, return mount point
_is_dir_mountpoint()
{
	local dir=$1
	local fstype=${2:-$FSTYP}
	...
	findmnt -rncv -T $dir -t $fstype -o TARGET | head -1
}

Note that I just slightly tested it, not sure if it'll work as expected
fully. (And please introduce _is_dir_mountpoint along with its first
usage, i.e. fold it to patch 2/5, if it works as expected.)

Thanks,
Eryu

>  # if so, return mount point
>  #
>  _is_mounted()
>  {
> -    if [ $# -ne 1 ]
> -    then
> -	echo "Usage: _is_mounted device" 1>&2
> -	exit 1
> -    fi
> +	if [ $# -lt 1 ]; then
> +		echo "Usage: _is_mounted <device|mountpoint> [fstype]" 1>&2
> +		exit 1
> +	fi
>  
> -    device=$1
> +	local name=$1
> +	local fstype=${2-$FSTYP}
>  
> -    if _mount | grep "$device " | $AWK_PROG -v pattern="type $FSTYP" '
> -        pattern        { print $3 ; exit 0 }
> -        END            { exit 1 }
> -    '
> -    then
> -        echo "_is_mounted: $device is not a mounted $FSTYP FS"
> -        exit 1
> -    fi
> +	_mount | grep "$name " | $AWK_PROG -v pattern="type $fstype" '
> +		$0 ~ pattern	{ print $3 }
> +	'
>  }
>  
>  # remount a FS to a new mode (ro or rw)
> -- 
> 2.5.0
> 

[xfstests PATCH v2 2/5] overlay: hook filesystem check helper

[zhangyi (F)]

Hook filesystem check helper to _check_test_fs and _check_scratch_fs for
constants underlying dirs of overlay filesystem, and introduce scratch
check helpers for optionally lower/upper/work dirs. These helpers works
only if fsck.overlay exists.

[ _check_test_fs/_check_scratch_fs part picked from Amir's patch, thanks ]

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 common/overlay | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 common/rc      |   4 +-
 2 files changed, 130 insertions(+), 2 deletions(-)

diff --git a/common/overlay b/common/overlay
index d741a7e..0e45ddd 100644
--- a/common/overlay
+++ b/common/overlay
@@ -152,6 +152,14 @@ _require_scratch_overlay_feature()
 	_scratch_unmount
 }
 
+# Require the same scratch device as _require_scratch, but do not check
+# the constants OVL_LOWER/OVL_UPPER/OVL_WORK dirs, should use together
+# with optionally lower/upper/work dirs and do check explicitly after test.
+_require_overlay_scratch_dirs()
+{
+	_require_scratch_nocheck
+}
+
 # Helper function to check underlying dirs of overlay filesystem
 _overlay_fsck_dirs()
 {
@@ -165,3 +173,123 @@ _overlay_fsck_dirs()
 	$FSCK_OVERLAY_PROG -o lowerdir=$lowerdir -o upperdir=$upperdir \
 			   -o workdir=$workdir $*
 }
+
+_overlay_check_dirs()
+{
+	local lowerdir=$1
+	local upperdir=$2
+	local workdir=$3
+	local err=0
+
+	_overlay_fsck_dirs $* $FSCK_OPTIONS >>$tmp.fsck 2>&1
+	if [ $? -ne 0 ]; then
+		_log_err "_overlay_check_fs: overlayfs on $lowerdir,$upperdir,$workdir is inconsistent"
+
+		echo "*** fsck.overlay output ***"	>>$seqres.full
+		cat $tmp.fsck				>>$seqres.full
+		echo "*** end fsck.overlay output"	>>$seqres.full
+
+		echo "*** mount output ***"		>>$seqres.full
+		_mount					>>$seqres.full
+		echo "*** end mount output"		>>$seqres.full
+
+		err=1
+	fi
+	rm -f $tmp.fsck
+
+	return $err
+}
+
+# Check the same mnt/dev of _check_overlay_scratch_fs, but check optionally
+# lower/upper/work dirs of overlay filesystem, should use together with
+# _require_overlay_scratch_dirs
+_overlay_check_scratch_dirs()
+{
+	local lowerdir=$1
+	local upperdir=$2
+	local workdir=$3
+	shift 3
+
+	# Need to umount overlay for scratch dir check
+	local ovl_mounted=`_is_mounted $SCRATCH_MNT`
+	[ -z "$ovl_mounted" ] || $UMOUNT_PROG $SCRATCH_MNT
+
+	# Check dirs with extra overlay options
+	_overlay_check_dirs $lowerdir $upperdir $workdir $*
+	local ret=$?
+
+	if [ $ret -eq 0 -a -n "$ovl_mounted" ]; then
+		# overlay was mounted, remount with extra mount options
+		_overlay_scratch_mount_dirs $lowerdir $upperdir \
+					    $workdir $*
+		ret=$?
+	fi
+
+	return $ret
+}
+
+_overlay_check_fs()
+{
+	# Aligns arguments for _overlay_base_mount
+	local ovl_mnt=$1
+	shift 1
+
+	local base_dev=$3
+	local base_mnt=$4
+
+	[ "$FSTYP" = overlay ] || return 0
+
+	# Base fs needs to be mounted to check overlay dirs
+	local base_fstype=""
+	local ovl_mounted=""
+
+	[ -z "$base_dev" ] || \
+		base_fstype=`_fs_type $base_dev`
+
+	# If base fstype is set, base fs is mounted, mount otherwise
+	if [ -z "$base_fstype" ]; then
+		_overlay_base_mount $*
+	else
+		# Need to umount overlay for dir check
+		ovl_mounted=`_is_mounted $ovl_mnt`
+		[ -z "$ovl_mounted" ] || $UMOUNT_PROG $ovl_mnt
+	fi
+
+	_overlay_check_dirs $base_mnt/$OVL_LOWER $base_mnt/$OVL_UPPER \
+			    $base_mnt/$OVL_WORK
+	local ret=$?
+
+	if [ -z "$base_fstype" ]; then
+		_overlay_base_unmount "$base_dev" "$base_mnt"
+	elif [ $ret -eq 0 -a -n "$ovl_mounted" ]; then
+		# overlay was mounted, remount besides extra mount options
+		_overlay_mount $base_mnt $ovl_mnt
+		ret=$?
+	fi
+
+	if [ $ret != 0 ]; then
+		status=1
+		if [ "$iam" != "check" ]; then
+			exit 1
+		fi
+		return 1
+	fi
+
+	return 0
+}
+
+_check_overlay_test_fs()
+{
+	_overlay_check_fs "$TEST_DIR" \
+		OVL_BASE_TEST_DEV OVL_BASE_TEST_DIR \
+		"$OVL_BASE_TEST_DEV" "$OVL_BASE_TEST_DIR" \
+		$TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS
+}
+
+_check_overlay_scratch_fs()
+{
+	_overlay_check_fs "$SCRATCH_MNT" \
+		OVL_BASE_SCRATCH_DEV OVL_BASE_SCRATCH_MNT \
+		"$OVL_BASE_SCRATCH_DEV" "$OVL_BASE_SCRATCH_MNT" \
+		$OVL_BASE_MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS
+}
diff --git a/common/rc b/common/rc
index 3351f00..7b84bb5 100644
--- a/common/rc
+++ b/common/rc
@@ -2585,7 +2585,7 @@ _check_test_fs()
 	# no way to check consistency for GlusterFS
 	;;
     overlay)
-	# no way to check consistency for overlay
+	_check_overlay_test_fs
 	;;
     pvfs2)
 	;;
@@ -2643,7 +2643,7 @@ _check_scratch_fs()
 	# no way to check consistency for GlusterFS
 	;;
     overlay)
-	# no way to check consistency for overlay
+	_check_overlay_scratch_fs
 	;;
     pvfs2)
 	;;
-- 
2.5.0

Re: [xfstests PATCH v2 2/5] overlay: hook filesystem check helper

[Eryu Guan]

On Wed, Jan 31, 2018 at 06:27:56PM +0800, zhangyi (F) wrote:
> Hook filesystem check helper to _check_test_fs and _check_scratch_fs for
> constants underlying dirs of overlay filesystem, and introduce scratch
> check helpers for optionally lower/upper/work dirs. These helpers works
> only if fsck.overlay exists.
> 
> [ _check_test_fs/_check_scratch_fs part picked from Amir's patch, thanks ]
> 
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> ---
>  common/overlay | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  common/rc      |   4 +-
>  2 files changed, 130 insertions(+), 2 deletions(-)
> 
> diff --git a/common/overlay b/common/overlay
> index d741a7e..0e45ddd 100644
> --- a/common/overlay
> +++ b/common/overlay
> @@ -152,6 +152,14 @@ _require_scratch_overlay_feature()
>  	_scratch_unmount
>  }
>  
> +# Require the same scratch device as _require_scratch, but do not check
> +# the constants OVL_LOWER/OVL_UPPER/OVL_WORK dirs, should use together
> +# with optionally lower/upper/work dirs and do check explicitly after test.
> +_require_overlay_scratch_dirs()
> +{
> +	_require_scratch_nocheck
> +}
> +

After looking at previous review comments, I know that this new function
was suggested by Amir, but I don't think we really need it, IMHO it just
adds another layer and complexity (sorry again on the late review..).
I'd just call _require_scratch_nocheck in tests with proper comments (as
we use multiple lower layers and the default _check_overlay_scratch_fs
just can't handle it).

>  # Helper function to check underlying dirs of overlay filesystem
>  _overlay_fsck_dirs()
>  {
> @@ -165,3 +173,123 @@ _overlay_fsck_dirs()
>  	$FSCK_OVERLAY_PROG -o lowerdir=$lowerdir -o upperdir=$upperdir \
>  			   -o workdir=$workdir $*
>  }
> +
> +_overlay_check_dirs()
> +{
> +	local lowerdir=$1
> +	local upperdir=$2
> +	local workdir=$3
> +	local err=0
> +
> +	_overlay_fsck_dirs $* $FSCK_OPTIONS >>$tmp.fsck 2>&1
> +	if [ $? -ne 0 ]; then
> +		_log_err "_overlay_check_fs: overlayfs on $lowerdir,$upperdir,$workdir is inconsistent"
> +
> +		echo "*** fsck.overlay output ***"	>>$seqres.full
> +		cat $tmp.fsck				>>$seqres.full
> +		echo "*** end fsck.overlay output"	>>$seqres.full
> +
> +		echo "*** mount output ***"		>>$seqres.full
> +		_mount					>>$seqres.full
> +		echo "*** end mount output"		>>$seqres.full
> +
> +		err=1
> +	fi
> +	rm -f $tmp.fsck
> +
> +	return $err
> +}
> +
> +# Check the same mnt/dev of _check_overlay_scratch_fs, but check optionally
> +# lower/upper/work dirs of overlay filesystem, should use together with
> +# _require_overlay_scratch_dirs

So the last sentence of above comments made me confused, why should we
use it together with _require_overlay_scratch_dirs and how? That's my
first impression reading these comments..

> +_overlay_check_scratch_dirs()
> +{
> +	local lowerdir=$1
> +	local upperdir=$2
> +	local workdir=$3
> +	shift 3
> +
> +	# Need to umount overlay for scratch dir check
> +	local ovl_mounted=`_is_mounted $SCRATCH_MNT`
> +	[ -z "$ovl_mounted" ] || $UMOUNT_PROG $SCRATCH_MNT
> +
> +	# Check dirs with extra overlay options
> +	_overlay_check_dirs $lowerdir $upperdir $workdir $*
> +	local ret=$?
> +
> +	if [ $ret -eq 0 -a -n "$ovl_mounted" ]; then
> +		# overlay was mounted, remount with extra mount options
> +		_overlay_scratch_mount_dirs $lowerdir $upperdir \
> +					    $workdir $*
> +		ret=$?
> +	fi
> +
> +	return $ret
> +}
> +
> +_overlay_check_fs()
> +{
> +	# Aligns arguments for _overlay_base_mount
> +	local ovl_mnt=$1
> +	shift 1
> +
> +	local base_dev=$3
> +	local base_mnt=$4

I think we need more comments on the arguments.

> +
> +	[ "$FSTYP" = overlay ] || return 0
> +
> +	# Base fs needs to be mounted to check overlay dirs
> +	local base_fstype=""
> +	local ovl_mounted=""
> +
> +	[ -z "$base_dev" ] || \
> +		base_fstype=`_fs_type $base_dev`
> +
> +	# If base fstype is set, base fs is mounted, mount otherwise

This comment is not clear enough, I think it's better to explain why we
do things differently here not what we do in the code.

> +	if [ -z "$base_fstype" ]; then

Need to check if "$base_dev" is empty or not, i.e. if we're using legacy
overlay setup or overlay with base devices:

	if [ -n "$base_dev" -a -z "$base_fstype" ]; then

Otherwise we call into _overlay_base_mount wrongly here when testing
with legacy overlay setup, and check prints weired messages (because
$base_dev is empty):

...
OVL_BASE_TEST_DEV=/mnt/ovl/test is mounted but not on OVL_BASE_TEST_DIR=-o - aborting
Already mounted result:
/mnt/ovl/test /mnt/testarea/test
overlay/006 1s ... 0s
OVL_BASE_SCRATCH_DEV=/mnt/ovl/scratch is mounted but not on OVL_BASE_SCRATCH_MNT=-o - aborting
Already mounted result:
/mnt/ovl/scratch /mnt/testarea/scratch
Ran: overlay/006
Passed all 1 tests

> +		_overlay_base_mount $*
> +	else
> +		# Need to umount overlay for dir check
> +		ovl_mounted=`_is_mounted $ovl_mnt`
> +		[ -z "$ovl_mounted" ] || $UMOUNT_PROG $ovl_mnt
> +	fi
> +
> +	_overlay_check_dirs $base_mnt/$OVL_LOWER $base_mnt/$OVL_UPPER \
> +			    $base_mnt/$OVL_WORK
> +	local ret=$?
> +
> +	if [ -z "$base_fstype" ]; then
> +		_overlay_base_unmount "$base_dev" "$base_mnt"

Looks like we need to check $base_dev too here.

> +	elif [ $ret -eq 0 -a -n "$ovl_mounted" ]; then
> +		# overlay was mounted, remount besides extra mount options
> +		_overlay_mount $base_mnt $ovl_mnt
> +		ret=$?
> +	fi
> +
> +	if [ $ret != 0 ]; then
> +		status=1
> +		if [ "$iam" != "check" ]; then
> +			exit 1
> +		fi
> +		return 1
> +	fi
> +
> +	return 0
> +}
> +
> +_check_overlay_test_fs()
> +{
> +	_overlay_check_fs "$TEST_DIR" \
> +		OVL_BASE_TEST_DEV OVL_BASE_TEST_DIR \
> +		"$OVL_BASE_TEST_DEV" "$OVL_BASE_TEST_DIR" \
> +		$TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS

Using $TEST_FS_MOUNT_OPTS doesn't look correct to me, the mount options
provided here are meant for mounting base test device, and
TEST_FS_MOUNT_OPTS is meant for mounting overlay. (I know that you're
copying from _overlay_base_test_mount(), and I think that's a bug in the
existing code.)

The problem is that both TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS should be
set to OVERLAY_MOUNT_OPTIONS if it's not empty. But currently only
MOUNT_OPTIONS is set in common/config::_mount_opts, TEST_FS_MOUNT_OPTS
isn't set in common/config::_test_mount_opts.

But, as mentioned above, this is a different issue, using
OVL_BASE_MOUNT_OPTIONS for both _check_overlay_test|scratch_fs should be
fine for now. But if you can fix the bug too in next version of this
patchset, it'd be great!

Thanks,
Eryu

> +}
> +
> +_check_overlay_scratch_fs()
> +{
> +	_overlay_check_fs "$SCRATCH_MNT" \
> +		OVL_BASE_SCRATCH_DEV OVL_BASE_SCRATCH_MNT \
> +		"$OVL_BASE_SCRATCH_DEV" "$OVL_BASE_SCRATCH_MNT" \
> +		$OVL_BASE_MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS
> +}
> diff --git a/common/rc b/common/rc
> index 3351f00..7b84bb5 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2585,7 +2585,7 @@ _check_test_fs()
>  	# no way to check consistency for GlusterFS
>  	;;
>      overlay)
> -	# no way to check consistency for overlay
> +	_check_overlay_test_fs
>  	;;
>      pvfs2)
>  	;;
> @@ -2643,7 +2643,7 @@ _check_scratch_fs()
>  	# no way to check consistency for GlusterFS
>  	;;
>      overlay)
> -	# no way to check consistency for overlay
> +	_check_overlay_scratch_fs
>  	;;
>      pvfs2)
>  	;;
> -- 
> 2.5.0
> 

Re: [xfstests PATCH v2 2/5] overlay: hook filesystem check helper

[Amir Goldstein]

On Tue, Feb 6, 2018 at 5:53 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Wed, Jan 31, 2018 at 06:27:56PM +0800, zhangyi (F) wrote:
>> Hook filesystem check helper to _check_test_fs and _check_scratch_fs for
>> constants underlying dirs of overlay filesystem, and introduce scratch
>> check helpers for optionally lower/upper/work dirs. These helpers works
>> only if fsck.overlay exists.
>>
>> [ _check_test_fs/_check_scratch_fs part picked from Amir's patch, thanks ]
>>
>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>> ---
>>  common/overlay | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  common/rc      |   4 +-
>>  2 files changed, 130 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/overlay b/common/overlay
>> index d741a7e..0e45ddd 100644
>> --- a/common/overlay
>> +++ b/common/overlay
>> @@ -152,6 +152,14 @@ _require_scratch_overlay_feature()
>>       _scratch_unmount
>>  }
>>
>> +# Require the same scratch device as _require_scratch, but do not check
>> +# the constants OVL_LOWER/OVL_UPPER/OVL_WORK dirs, should use together
>> +# with optionally lower/upper/work dirs and do check explicitly after test.
>> +_require_overlay_scratch_dirs()
>> +{
>> +     _require_scratch_nocheck
>> +}
>> +
>
> After looking at previous review comments, I know that this new function
> was suggested by Amir, but I don't think we really need it, IMHO it just
> adds another layer and complexity (sorry again on the late review..).
> I'd just call _require_scratch_nocheck in tests with proper comments (as
> we use multiple lower layers and the default _check_overlay_scratch_fs
> just can't handle it).
>
>>  # Helper function to check underlying dirs of overlay filesystem
>>  _overlay_fsck_dirs()
>>  {
>> @@ -165,3 +173,123 @@ _overlay_fsck_dirs()
>>       $FSCK_OVERLAY_PROG -o lowerdir=$lowerdir -o upperdir=$upperdir \
>>                          -o workdir=$workdir $*
>>  }
>> +
>> +_overlay_check_dirs()
>> +{
>> +     local lowerdir=$1
>> +     local upperdir=$2
>> +     local workdir=$3
>> +     local err=0
>> +
>> +     _overlay_fsck_dirs $* $FSCK_OPTIONS >>$tmp.fsck 2>&1
>> +     if [ $? -ne 0 ]; then
>> +             _log_err "_overlay_check_fs: overlayfs on $lowerdir,$upperdir,$workdir is inconsistent"
>> +
>> +             echo "*** fsck.overlay output ***"      >>$seqres.full
>> +             cat $tmp.fsck                           >>$seqres.full
>> +             echo "*** end fsck.overlay output"      >>$seqres.full
>> +
>> +             echo "*** mount output ***"             >>$seqres.full
>> +             _mount                                  >>$seqres.full
>> +             echo "*** end mount output"             >>$seqres.full
>> +
>> +             err=1
>> +     fi
>> +     rm -f $tmp.fsck
>> +
>> +     return $err
>> +}
>> +
>> +# Check the same mnt/dev of _check_overlay_scratch_fs, but check optionally
>> +# lower/upper/work dirs of overlay filesystem, should use together with
>> +# _require_overlay_scratch_dirs
>
> So the last sentence of above comments made me confused, why should we
> use it together with _require_overlay_scratch_dirs and how? That's my
> first impression reading these comments..
>
>> +_overlay_check_scratch_dirs()
>> +{
>> +     local lowerdir=$1
>> +     local upperdir=$2
>> +     local workdir=$3
>> +     shift 3
>> +
>> +     # Need to umount overlay for scratch dir check
>> +     local ovl_mounted=`_is_mounted $SCRATCH_MNT`
>> +     [ -z "$ovl_mounted" ] || $UMOUNT_PROG $SCRATCH_MNT
>> +
>> +     # Check dirs with extra overlay options
>> +     _overlay_check_dirs $lowerdir $upperdir $workdir $*
>> +     local ret=$?
>> +
>> +     if [ $ret -eq 0 -a -n "$ovl_mounted" ]; then
>> +             # overlay was mounted, remount with extra mount options
>> +             _overlay_scratch_mount_dirs $lowerdir $upperdir \
>> +                                         $workdir $*
>> +             ret=$?
>> +     fi
>> +
>> +     return $ret
>> +}
>> +
>> +_overlay_check_fs()
>> +{
>> +     # Aligns arguments for _overlay_base_mount
>> +     local ovl_mnt=$1
>> +     shift 1
>> +
>> +     local base_dev=$3
>> +     local base_mnt=$4
>
> I think we need more comments on the arguments.
>
>> +
>> +     [ "$FSTYP" = overlay ] || return 0
>> +
>> +     # Base fs needs to be mounted to check overlay dirs
>> +     local base_fstype=""
>> +     local ovl_mounted=""
>> +
>> +     [ -z "$base_dev" ] || \
>> +             base_fstype=`_fs_type $base_dev`
>> +
>> +     # If base fstype is set, base fs is mounted, mount otherwise
>
> This comment is not clear enough, I think it's better to explain why we
> do things differently here not what we do in the code.
>
>> +     if [ -z "$base_fstype" ]; then
>
> Need to check if "$base_dev" is empty or not, i.e. if we're using legacy
> overlay setup or overlay with base devices:
>
>         if [ -n "$base_dev" -a -z "$base_fstype" ]; then
>
> Otherwise we call into _overlay_base_mount wrongly here when testing
> with legacy overlay setup, and check prints weired messages (because
> $base_dev is empty):
>
> ...
> OVL_BASE_TEST_DEV=/mnt/ovl/test is mounted but not on OVL_BASE_TEST_DIR=-o - aborting
> Already mounted result:
> /mnt/ovl/test /mnt/testarea/test
> overlay/006 1s ... 0s
> OVL_BASE_SCRATCH_DEV=/mnt/ovl/scratch is mounted but not on OVL_BASE_SCRATCH_MNT=-o - aborting
> Already mounted result:
> /mnt/ovl/scratch /mnt/testarea/scratch
> Ran: overlay/006
> Passed all 1 tests
>
>> +             _overlay_base_mount $*
>> +     else
>> +             # Need to umount overlay for dir check
>> +             ovl_mounted=`_is_mounted $ovl_mnt`
>> +             [ -z "$ovl_mounted" ] || $UMOUNT_PROG $ovl_mnt
>> +     fi
>> +
>> +     _overlay_check_dirs $base_mnt/$OVL_LOWER $base_mnt/$OVL_UPPER \
>> +                         $base_mnt/$OVL_WORK
>> +     local ret=$?
>> +
>> +     if [ -z "$base_fstype" ]; then
>> +             _overlay_base_unmount "$base_dev" "$base_mnt"
>
> Looks like we need to check $base_dev too here.
>
>> +     elif [ $ret -eq 0 -a -n "$ovl_mounted" ]; then
>> +             # overlay was mounted, remount besides extra mount options
>> +             _overlay_mount $base_mnt $ovl_mnt
>> +             ret=$?
>> +     fi
>> +
>> +     if [ $ret != 0 ]; then
>> +             status=1
>> +             if [ "$iam" != "check" ]; then
>> +                     exit 1
>> +             fi
>> +             return 1
>> +     fi
>> +
>> +     return 0
>> +}
>> +
>> +_check_overlay_test_fs()
>> +{
>> +     _overlay_check_fs "$TEST_DIR" \
>> +             OVL_BASE_TEST_DEV OVL_BASE_TEST_DIR \
>> +             "$OVL_BASE_TEST_DEV" "$OVL_BASE_TEST_DIR" \
>> +             $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS
>
> Using $TEST_FS_MOUNT_OPTS doesn't look correct to me, the mount options
> provided here are meant for mounting base test device, and
> TEST_FS_MOUNT_OPTS is meant for mounting overlay. (I know that you're
> copying from _overlay_base_test_mount(), and I think that's a bug in the
> existing code.)
>
> The problem is that both TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS should be
> set to OVERLAY_MOUNT_OPTIONS if it's not empty. But currently only
> MOUNT_OPTIONS is set in common/config::_mount_opts, TEST_FS_MOUNT_OPTS
> isn't set in common/config::_test_mount_opts.
>
> But, as mentioned above, this is a different issue, using
> OVL_BASE_MOUNT_OPTIONS for both _check_overlay_test|scratch_fs should be
> fine for now. But if you can fix the bug too in next version of this
> patchset, it'd be great!
>

FWIW, I think your analysis is correct in the sense that the code
looks bad and smells
bad (my original code), but I am not sure there is an actual bug here
(or in original code).
You say that TEST_FS_MOUNT_OPTS is meant to mount overlay, but it is never used
for mounting overlay. See the last section of README.overlay - it
explains what the
different mount options are used for in -overlay run.
AFAICT, there is only a missing feature - not being able to configure
different overlay
mount options for scratch and test.

Can you point to a bug?

Thanks,
Amir.

Re: [xfstests PATCH v2 2/5] overlay: hook filesystem check helper

[Eryu Guan]

On Tue, Feb 06, 2018 at 10:34:54PM +0200, Amir Goldstein wrote:
> On Tue, Feb 6, 2018 at 5:53 PM, Eryu Guan <eguan@redhat.com> wrote:
> > On Wed, Jan 31, 2018 at 06:27:56PM +0800, zhangyi (F) wrote:
> >> Hook filesystem check helper to _check_test_fs and _check_scratch_fs for
> >> constants underlying dirs of overlay filesystem, and introduce scratch
> >> check helpers for optionally lower/upper/work dirs. These helpers works
> >> only if fsck.overlay exists.
> >>
> >> [ _check_test_fs/_check_scratch_fs part picked from Amir's patch, thanks ]
> >>
> >> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> >> ---
> >>  common/overlay | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  common/rc      |   4 +-
> >>  2 files changed, 130 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/common/overlay b/common/overlay
> >> index d741a7e..0e45ddd 100644
> >> --- a/common/overlay
> >> +++ b/common/overlay
> >> @@ -152,6 +152,14 @@ _require_scratch_overlay_feature()
> >>       _scratch_unmount
> >>  }
> >>
> >> +# Require the same scratch device as _require_scratch, but do not check
> >> +# the constants OVL_LOWER/OVL_UPPER/OVL_WORK dirs, should use together
> >> +# with optionally lower/upper/work dirs and do check explicitly after test.
> >> +_require_overlay_scratch_dirs()
> >> +{
> >> +     _require_scratch_nocheck
> >> +}
> >> +
> >
> > After looking at previous review comments, I know that this new function
> > was suggested by Amir, but I don't think we really need it, IMHO it just
> > adds another layer and complexity (sorry again on the late review..).
> > I'd just call _require_scratch_nocheck in tests with proper comments (as
> > we use multiple lower layers and the default _check_overlay_scratch_fs
> > just can't handle it).
> >
> >>  # Helper function to check underlying dirs of overlay filesystem
> >>  _overlay_fsck_dirs()
> >>  {
> >> @@ -165,3 +173,123 @@ _overlay_fsck_dirs()
> >>       $FSCK_OVERLAY_PROG -o lowerdir=$lowerdir -o upperdir=$upperdir \
> >>                          -o workdir=$workdir $*
> >>  }
> >> +
> >> +_overlay_check_dirs()
> >> +{
> >> +     local lowerdir=$1
> >> +     local upperdir=$2
> >> +     local workdir=$3
> >> +     local err=0
> >> +
> >> +     _overlay_fsck_dirs $* $FSCK_OPTIONS >>$tmp.fsck 2>&1
> >> +     if [ $? -ne 0 ]; then
> >> +             _log_err "_overlay_check_fs: overlayfs on $lowerdir,$upperdir,$workdir is inconsistent"
> >> +
> >> +             echo "*** fsck.overlay output ***"      >>$seqres.full
> >> +             cat $tmp.fsck                           >>$seqres.full
> >> +             echo "*** end fsck.overlay output"      >>$seqres.full
> >> +
> >> +             echo "*** mount output ***"             >>$seqres.full
> >> +             _mount                                  >>$seqres.full
> >> +             echo "*** end mount output"             >>$seqres.full
> >> +
> >> +             err=1
> >> +     fi
> >> +     rm -f $tmp.fsck
> >> +
> >> +     return $err
> >> +}
> >> +
> >> +# Check the same mnt/dev of _check_overlay_scratch_fs, but check optionally
> >> +# lower/upper/work dirs of overlay filesystem, should use together with
> >> +# _require_overlay_scratch_dirs
> >
> > So the last sentence of above comments made me confused, why should we
> > use it together with _require_overlay_scratch_dirs and how? That's my
> > first impression reading these comments..
> >
> >> +_overlay_check_scratch_dirs()
> >> +{
> >> +     local lowerdir=$1
> >> +     local upperdir=$2
> >> +     local workdir=$3
> >> +     shift 3
> >> +
> >> +     # Need to umount overlay for scratch dir check
> >> +     local ovl_mounted=`_is_mounted $SCRATCH_MNT`
> >> +     [ -z "$ovl_mounted" ] || $UMOUNT_PROG $SCRATCH_MNT
> >> +
> >> +     # Check dirs with extra overlay options
> >> +     _overlay_check_dirs $lowerdir $upperdir $workdir $*
> >> +     local ret=$?
> >> +
> >> +     if [ $ret -eq 0 -a -n "$ovl_mounted" ]; then
> >> +             # overlay was mounted, remount with extra mount options
> >> +             _overlay_scratch_mount_dirs $lowerdir $upperdir \
> >> +                                         $workdir $*
> >> +             ret=$?
> >> +     fi
> >> +
> >> +     return $ret
> >> +}
> >> +
> >> +_overlay_check_fs()
> >> +{
> >> +     # Aligns arguments for _overlay_base_mount
> >> +     local ovl_mnt=$1
> >> +     shift 1
> >> +
> >> +     local base_dev=$3
> >> +     local base_mnt=$4
> >
> > I think we need more comments on the arguments.
> >
> >> +
> >> +     [ "$FSTYP" = overlay ] || return 0
> >> +
> >> +     # Base fs needs to be mounted to check overlay dirs
> >> +     local base_fstype=""
> >> +     local ovl_mounted=""
> >> +
> >> +     [ -z "$base_dev" ] || \
> >> +             base_fstype=`_fs_type $base_dev`
> >> +
> >> +     # If base fstype is set, base fs is mounted, mount otherwise
> >
> > This comment is not clear enough, I think it's better to explain why we
> > do things differently here not what we do in the code.
> >
> >> +     if [ -z "$base_fstype" ]; then
> >
> > Need to check if "$base_dev" is empty or not, i.e. if we're using legacy
> > overlay setup or overlay with base devices:
> >
> >         if [ -n "$base_dev" -a -z "$base_fstype" ]; then
> >
> > Otherwise we call into _overlay_base_mount wrongly here when testing
> > with legacy overlay setup, and check prints weired messages (because
> > $base_dev is empty):
> >
> > ...
> > OVL_BASE_TEST_DEV=/mnt/ovl/test is mounted but not on OVL_BASE_TEST_DIR=-o - aborting
> > Already mounted result:
> > /mnt/ovl/test /mnt/testarea/test
> > overlay/006 1s ... 0s
> > OVL_BASE_SCRATCH_DEV=/mnt/ovl/scratch is mounted but not on OVL_BASE_SCRATCH_MNT=-o - aborting
> > Already mounted result:
> > /mnt/ovl/scratch /mnt/testarea/scratch
> > Ran: overlay/006
> > Passed all 1 tests
> >
> >> +             _overlay_base_mount $*
> >> +     else
> >> +             # Need to umount overlay for dir check
> >> +             ovl_mounted=`_is_mounted $ovl_mnt`
> >> +             [ -z "$ovl_mounted" ] || $UMOUNT_PROG $ovl_mnt
> >> +     fi
> >> +
> >> +     _overlay_check_dirs $base_mnt/$OVL_LOWER $base_mnt/$OVL_UPPER \
> >> +                         $base_mnt/$OVL_WORK
> >> +     local ret=$?
> >> +
> >> +     if [ -z "$base_fstype" ]; then
> >> +             _overlay_base_unmount "$base_dev" "$base_mnt"
> >
> > Looks like we need to check $base_dev too here.
> >
> >> +     elif [ $ret -eq 0 -a -n "$ovl_mounted" ]; then
> >> +             # overlay was mounted, remount besides extra mount options
> >> +             _overlay_mount $base_mnt $ovl_mnt
> >> +             ret=$?
> >> +     fi
> >> +
> >> +     if [ $ret != 0 ]; then
> >> +             status=1
> >> +             if [ "$iam" != "check" ]; then
> >> +                     exit 1
> >> +             fi
> >> +             return 1
> >> +     fi
> >> +
> >> +     return 0
> >> +}
> >> +
> >> +_check_overlay_test_fs()
> >> +{
> >> +     _overlay_check_fs "$TEST_DIR" \
> >> +             OVL_BASE_TEST_DEV OVL_BASE_TEST_DIR \
> >> +             "$OVL_BASE_TEST_DEV" "$OVL_BASE_TEST_DIR" \
> >> +             $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS
> >
> > Using $TEST_FS_MOUNT_OPTS doesn't look correct to me, the mount options
> > provided here are meant for mounting base test device, and
> > TEST_FS_MOUNT_OPTS is meant for mounting overlay. (I know that you're
> > copying from _overlay_base_test_mount(), and I think that's a bug in the
> > existing code.)
> >
> > The problem is that both TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS should be
> > set to OVERLAY_MOUNT_OPTIONS if it's not empty. But currently only
> > MOUNT_OPTIONS is set in common/config::_mount_opts, TEST_FS_MOUNT_OPTS
> > isn't set in common/config::_test_mount_opts.
> >
> > But, as mentioned above, this is a different issue, using
> > OVL_BASE_MOUNT_OPTIONS for both _check_overlay_test|scratch_fs should be
> > fine for now. But if you can fix the bug too in next version of this
> > patchset, it'd be great!
> >
> 
> FWIW, I think your analysis is correct in the sense that the code
> looks bad and smells
> bad (my original code), but I am not sure there is an actual bug here
> (or in original code).

Maybe "bug" is a bit strong here, and I didn't notice it either in my
review..

> You say that TEST_FS_MOUNT_OPTS is meant to mount overlay, but it is never used
> for mounting overlay. See the last section of README.overlay - it
> explains what the
> different mount options are used for in -overlay run.
> AFAICT, there is only a missing feature - not being able to configure
> different overlay
> mount options for scratch and test.

Yeah, a "missing feature" might be more accurate :) I think we just need
a "OVL_BASE_MOUNT_OPTIONS" counter part for TEST_FS_MOUNT_OPTS, and do
all necessary setups as what we did for MOUNT_OPTIONS and
OVL_BASE_MOUNT_OPTIONS.

Thanks,
Eryu

Re: [xfstests PATCH v2 2/5] overlay: hook filesystem check helper

[zhangyi (F)]

On 2018/2/6 23:53, Eryu Guan Wrote:
> On Wed, Jan 31, 2018 at 06:27:56PM +0800, zhangyi (F) wrote:
>> Hook filesystem check helper to _check_test_fs and _check_scratch_fs for
>> constants underlying dirs of overlay filesystem, and introduce scratch
>> check helpers for optionally lower/upper/work dirs. These helpers works
>> only if fsck.overlay exists.
>>
>> [ _check_test_fs/_check_scratch_fs part picked from Amir's patch, thanks ]
>>
[..]
>> +_check_overlay_test_fs()
>> +{
>> +	_overlay_check_fs "$TEST_DIR" \
>> +		OVL_BASE_TEST_DEV OVL_BASE_TEST_DIR \
>> +		"$OVL_BASE_TEST_DEV" "$OVL_BASE_TEST_DIR" \
>> +		$TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS
> 
> Using $TEST_FS_MOUNT_OPTS doesn't look correct to me, the mount options
> provided here are meant for mounting base test device, and
> TEST_FS_MOUNT_OPTS is meant for mounting overlay. (I know that you're
> copying from _overlay_base_test_mount(), and I think that's a bug in the
> existing code.)
> 
> The problem is that both TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS should be
> set to OVERLAY_MOUNT_OPTIONS if it's not empty. But currently only
> MOUNT_OPTIONS is set in common/config::_mount_opts, TEST_FS_MOUNT_OPTS
> isn't set in common/config::_test_mount_opts.
> 
Sorry, I was a little confuse that it conflict with comments in README.overlay.

IIUC, XXX_MOUNT_OPTIONS act as an "default mount options" when user not specify
MOUNT_OPTIONS explicity, so we set MOUNT_OPTIONS and TEST_FS_MOUNT_OPTS to
XXX_MOUNT_OPTIONS in _mount_opts and _test_mount_opts for other filesystems.

But overlayfs is different, OVERLAY_MOUNT_OPTIONS is used for overlayfs mount
options, so we first set OVL_BASE_MOUNT_OPTIONS to MOUNT_OPTIONS and then set
MOUNT_OPTIONS to OVERLAY_MOUNT_OPTIONS. So MOUNT_OPTIONS=$OVERLAY_MOUNT_OPTIONS
in _mount_opts is not correct? (It seem that it will not lead to mistack because
"8df8ad0c overlay: fix _overlay_config_override of MOUNT_OPTIONS").

Do you think we shoud use OVL_BASE_MOUNT_OPTIONS instead of OVERLAY_MOUNT_OPTIONS
as "default mount options" and set MOUNT_OPTIONS and TEST_FS_MOUNT_OPTS in
_mount_opts and _test_mount_opts if user not set either of them ?

Thanks,
Yi.
> But, as mentioned above, this is a different issue, using
> OVL_BASE_MOUNT_OPTIONS for both _check_overlay_test|scratch_fs should be
> fine for now. But if you can fix the bug too in next version of this
> patchset, it'd be great!
> 
> Thanks,
> Eryu
> 
>> +}
>> +
>> +_check_overlay_scratch_fs()
>> +{
>> +	_overlay_check_fs "$SCRATCH_MNT" \
>> +		OVL_BASE_SCRATCH_DEV OVL_BASE_SCRATCH_MNT \
>> +		"$OVL_BASE_SCRATCH_DEV" "$OVL_BASE_SCRATCH_MNT" \
>> +		$OVL_BASE_MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS
>> +}
>> diff --git a/common/rc b/common/rc
>> index 3351f00..7b84bb5 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -2585,7 +2585,7 @@ _check_test_fs()
>>  	# no way to check consistency for GlusterFS
>>  	;;
>>      overlay)
>> -	# no way to check consistency for overlay
>> +	_check_overlay_test_fs
>>  	;;
>>      pvfs2)
>>  	;;
>> @@ -2643,7 +2643,7 @@ _check_scratch_fs()
>>  	# no way to check consistency for GlusterFS
>>  	;;
>>      overlay)
>> -	# no way to check consistency for overlay
>> +	_check_overlay_scratch_fs
>>  	;;
>>      pvfs2)
>>  	;;
>> -- 
>> 2.5.0
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

[xfstests PATCH v2 3/5] overlay/003: fix fs check failure

[zhangyi (F)]

_check_overlay_scratch_fs() will check lowerdir of overlay filesystem,
this case remove this directory after test will lead to check failure,
and it is not really necessary to remove this directory, so keep this
directory.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/003 | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/overlay/003 b/tests/overlay/003
index f980edb..154531e 100755
--- a/tests/overlay/003
+++ b/tests/overlay/003
@@ -92,7 +92,6 @@ ls ${SCRATCH_MNT}/
 # unmount overlayfs but not base fs
 $UMOUNT_PROG $SCRATCH_MNT
 
-rm -rf $lowerdir
 echo "Silence is golden"
 # success, all done
 status=0
-- 
2.5.0

[xfstests PATCH v2 4/5] overlay: skip check for tests finished with corrupt filesystem

[zhangyi (F)]

No post-test check of the overlay dirs is required if case leaves
corrupt filesystem after test. We shoud use _require_scratch_nocheck()
instead of _require_scratch() in these cases.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/019 | 2 +-
 tests/overlay/031 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/overlay/019 b/tests/overlay/019
index 3e2bc4e..575aaf7 100755
--- a/tests/overlay/019
+++ b/tests/overlay/019
@@ -46,7 +46,7 @@ rm -f $seqres.full
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+_require_scratch_nocheck
 
 # Remove all files from previous tests
 _scratch_mkfs
diff --git a/tests/overlay/031 b/tests/overlay/031
index 186b409..90a06af 100755
--- a/tests/overlay/031
+++ b/tests/overlay/031
@@ -67,7 +67,7 @@ rm -f $seqres.full
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+_require_scratch_nocheck
 
 # remove all files from previous runs
 _scratch_mkfs
-- 
2.5.0

[xfstests PATCH v2 5/5] overlay: correct scratch dirs check

[zhangyi (F)]

Tests that use _overlay_scratch_mount_dirs instead of _scratch_mount
should use _require_overlay_scratch_dirs instead of _require_scratch.
Those tests can optionally call _overlay_check_scratch_dirs at the end
of the test or before _scratch_umount to umount $SCRATCH_MNT and
run the checker.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/005 |  6 +++++-
 tests/overlay/010 |  6 +++++-
 tests/overlay/014 | 10 +++++++++-
 tests/overlay/035 |  6 +++++-
 tests/overlay/036 |  5 ++++-
 tests/overlay/037 |  6 +++++-
 tests/overlay/038 | 10 +++++++++-
 tests/overlay/041 | 10 +++++++++-
 tests/overlay/043 |  6 +++++-
 tests/overlay/044 | 13 +++++++++++--
 10 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/tests/overlay/005 b/tests/overlay/005
index 17992a3..f502d3f 100755
--- a/tests/overlay/005
+++ b/tests/overlay/005
@@ -54,7 +54,7 @@ rm -f $seqres.full
 # Modify as appropriate.
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+_require_overlay_scratch_dirs
 _require_loop
 
 # Remove all files from previous tests
@@ -102,6 +102,10 @@ $XFS_IO_PROG -f -c "o" ${SCRATCH_MNT}/test_file \
 # unmount overlayfs
 $UMOUNT_PROG $SCRATCH_MNT
 
+# We called _require_overlay_scratch_dirs instead of _require_scratch,
+# do check optionally dirs after test
+_overlay_check_scratch_dirs $lowerd $upperd $workd
+
 # unmount undelying xfs, this tiggers panic if memleak happens
 $UMOUNT_PROG ${OVL_BASE_SCRATCH_MNT}/uppermnt
 $UMOUNT_PROG ${OVL_BASE_SCRATCH_MNT}/lowermnt
diff --git a/tests/overlay/010 b/tests/overlay/010
index f55ebec..f3b50d7 100755
--- a/tests/overlay/010
+++ b/tests/overlay/010
@@ -48,7 +48,7 @@ rm -f $seqres.full
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+_require_overlay_scratch_dirs
 
 # Remove all files from previous tests
 _scratch_mkfs
@@ -70,6 +70,10 @@ mknod $lowerdir2/testdir/a c 0 0
 _overlay_scratch_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
 rm -rf $SCRATCH_MNT/testdir
 
+# We called _require_overlay_scratch_dirs instead of _require_scratch,
+# do check optionally dirs after test
+_overlay_check_scratch_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
+
 # success, all done
 echo "Silence is golden"
 status=0
diff --git a/tests/overlay/014 b/tests/overlay/014
index 9f308d3..069d8b6 100755
--- a/tests/overlay/014
+++ b/tests/overlay/014
@@ -53,7 +53,7 @@ rm -f $seqres.full
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+_require_overlay_scratch_dirs
 
 # Remove all files from previous tests
 _scratch_mkfs
@@ -78,6 +78,10 @@ mkdir -p $SCRATCH_MNT/testdir/visibledir
 # unmount overlayfs but not base fs
 $UMOUNT_PROG $SCRATCH_MNT
 
+# We called _require_overlay_scratch_dirs instead of _require_scratch,
+# do check optionally dirs after each test
+_overlay_check_scratch_dirs $lowerdir1 $lowerdir2 $workdir2
+
 # mount overlay again, with lowerdir1 and lowerdir2 as multiple lowerdirs,
 # and create a new file in testdir, triggers copyup from lowerdir,
 # copyup should not copy overlayfs private xattr
@@ -90,6 +94,10 @@ $UMOUNT_PROG $SCRATCH_MNT
 _overlay_scratch_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
 ls $SCRATCH_MNT/testdir
 
+# We called _require_overlay_scratch_dirs instead of _require_scratch,
+# do check optionally dirs after each test
+_overlay_check_scratch_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
+
 # success, all done
 status=0
 exit
diff --git a/tests/overlay/035 b/tests/overlay/035
index 0544774..bc73013 100755
--- a/tests/overlay/035
+++ b/tests/overlay/035
@@ -51,7 +51,7 @@ rm -f $seqres.full
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+_require_overlay_scratch_dirs
 _require_chattr i
 
 # Remove all files from previous tests
@@ -81,6 +81,10 @@ _overlay_scratch_mount_dirs $lowerdir2 $upperdir $workdir
 touch $SCRATCH_MNT/bar 2>&1 | _filter_scratch
 _scratch_remount rw 2>&1 | _filter_ro_mount
 
+# We called _require_overlay_scratch_dirs instead of _require_scratch,
+# do check optionally dirs after test
+_overlay_check_scratch_dirs $lowerdir2 $upperdir $workdir
+
 # success, all done
 status=0
 exit
diff --git a/tests/overlay/036 b/tests/overlay/036
index e0c13ae..ca5adbd 100755
--- a/tests/overlay/036
+++ b/tests/overlay/036
@@ -69,7 +69,7 @@ rm -f $seqres.full
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+_require_overlay_scratch_dirs
 _require_scratch_feature index
 
 # Remove all files from previous tests
@@ -110,6 +110,9 @@ _overlay_mount_dirs $lowerdir2 $upperdir $workdir2 \
 _overlay_mount_dirs $lowerdir2 $upperdir2 $workdir \
 	    overlay3 $SCRATCH_MNT -oindex=on 2>&1 | _filter_busy_mount
 
+# We called _require_overlay_scratch_dirs instead of _require_scratch,
+# but which will not lead to inconsistency after this mount test,
+# so do not check optionally dirs after test
 
 # success, all done
 status=0
diff --git a/tests/overlay/037 b/tests/overlay/037
index 6710dda..60290a1 100755
--- a/tests/overlay/037
+++ b/tests/overlay/037
@@ -55,7 +55,7 @@ rm -f $seqres.full
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+_require_overlay_scratch_dirs
 _require_scratch_feature index
 
 # Remove all files from previous tests
@@ -87,6 +87,10 @@ $UMOUNT_PROG $SCRATCH_MNT 2>/dev/null
 # Mount overlay with original lowerdir, upperdir, workdir and index=on - expect success
 _overlay_scratch_mount_dirs $lowerdir $upperdir $workdir -oindex=on
 
+# We called _require_overlay_scratch_dirs instead of _require_scratch,
+# but which will not lead to inconsistency after this mount test,
+# so do not check optionally dirs after test
+
 # success, all done
 status=0
 exit
diff --git a/tests/overlay/038 b/tests/overlay/038
index bd87156..aa20cbc 100755
--- a/tests/overlay/038
+++ b/tests/overlay/038
@@ -46,7 +46,7 @@ _cleanup()
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+_require_overlay_scratch_dirs
 _require_attrs
 _require_test_program "t_dir_type"
 
@@ -161,6 +161,10 @@ subdir_d=$($here/src/t_dir_type $pure_lower_dir $pure_lower_subdir_st_ino)
 [[ $subdir_d == "subdir d" ]] || \
 	echo "Merged dir: Invalid d_ino reported for subdir"
 
+# We called _require_overlay_scratch_dirs instead of _require_scratch,
+# do check after each test
+_check_scratch_fs
+
 _scratch_unmount
 
 # Verify pure lower residing in dir which has another lower layer
@@ -202,6 +206,10 @@ subdir_d=$($here/src/t_dir_type $pure_lower_dir $pure_lower_subdir_st_ino)
 [[ $subdir_d == "subdir d" ]] || \
 	echo "Pure lower in dir which has another lower layer: Invalid d_ino reported for subdir"
 
+# We called _require_overlay_scratch_dirs instead of _require_scratch,
+# do check optionally dirs after each test
+_overlay_check_scratch_dirs "$middir:$lowerdir" $upperdir $workdir
+
 echo "Silence is golden"
 status=0
 exit
diff --git a/tests/overlay/041 b/tests/overlay/041
index 4152107..607908a 100755
--- a/tests/overlay/041
+++ b/tests/overlay/041
@@ -48,7 +48,7 @@ _cleanup()
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+_require_overlay_scratch_dirs
 _require_test
 _require_attrs
 _require_test_program "t_dir_type"
@@ -167,6 +167,10 @@ subdir_d=$($here/src/t_dir_type $pure_lower_dir $pure_lower_subdir_st_ino)
 [[ $subdir_d == "subdir d" ]] || \
 	echo "Merged dir: Invalid d_ino reported for subdir"
 
+# We called _require_overlay_scratch_dirs instead of _require_scratch,
+# do check optionally dirs after each test
+_overlay_check_scratch_dirs $lowerdir $upperdir $workdir
+
 _scratch_unmount
 
 # Verify pure lower residing in dir which has another lower layer
@@ -208,6 +212,10 @@ subdir_d=$($here/src/t_dir_type $pure_lower_dir $pure_lower_subdir_st_ino)
 [[ $subdir_d == "subdir d" ]] || \
 	echo "Pure lower in dir which has another lower layer: Invalid d_ino reported for subdir"
 
+# We called _require_overlay_scratch_dirs instead of _require_scratch,
+# do check optionally dirs after each test
+_overlay_check_scratch_dirs "$middir:$lowerdir" $upperdir $workdir
+
 echo "Silence is golden"
 status=0
 exit
diff --git a/tests/overlay/043 b/tests/overlay/043
index 858b6a9..76e0dd9 100755
--- a/tests/overlay/043
+++ b/tests/overlay/043
@@ -56,7 +56,7 @@ _cleanup()
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+_require_overlay_scratch_dirs
 _require_test
 _require_test_program "af_unix"
 _require_test_program "t_dir_type"
@@ -155,6 +155,10 @@ _overlay_scratch_mount_dirs $lowerdir $upperdir $workdir \
 # Compare inode numbers before/after mount cycle
 check_inode_numbers $testdir $tmp.after_move $tmp.after_cycle
 
+# We called _require_overlay_scratch_dirs instead of _require_scratch,
+# do check optionally dirs after test
+_overlay_check_scratch_dirs $lowerdir $upperdir $workdir
+
 echo "Silence is golden"
 status=0
 exit
diff --git a/tests/overlay/044 b/tests/overlay/044
index 9c0ff04..65591f9 100755
--- a/tests/overlay/044
+++ b/tests/overlay/044
@@ -49,7 +49,7 @@ _cleanup()
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+_require_overlay_scratch_dirs
 _require_test
 _require_scratch_feature index
 _require_test_program "t_dir_type"
@@ -122,8 +122,13 @@ echo "== After write one =="
 cat $FILES
 check_ino_nlink $SCRATCH_MNT $tmp.before $tmp.after_one
 
-# Verify that the hardlinks survive a mount cycle
 $UMOUNT_PROG $SCRATCH_MNT
+
+# We called _require_overlay_scratch_dirs instead of _require_scratch,
+# do check optionally dirs after each test
+_overlay_check_scratch_dirs $lowerdir $upperdir $workdir "-o index=on"
+
+# Verify that the hardlinks survive a mount cycle
 _overlay_scratch_mount_dirs $lowerdir $upperdir $workdir \
 			    $OVERLAY_MOUNT_OPTIONS
 
@@ -141,5 +146,9 @@ echo "== After write two =="
 cat $FILES
 check_ino_nlink $SCRATCH_MNT $tmp.after_one $tmp.after_two
 
+# We called _require_overlay_scratch_dirs instead of _require_scratch,
+# do check optionally dirs after test
+_overlay_check_scratch_dirs $lowerdir $upperdir $workdir
+
 status=0
 exit
-- 
2.5.0

Re: [xfstests PATCH v2 5/5] overlay: correct scratch dirs check

[Amir Goldstein]

On Wed, Jan 31, 2018 at 12:27 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> Tests that use _overlay_scratch_mount_dirs instead of _scratch_mount
> should use _require_overlay_scratch_dirs instead of _require_scratch.
> Those tests can optionally call _overlay_check_scratch_dirs at the end
> of the test or before _scratch_umount to umount $SCRATCH_MNT and
> run the checker.
>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  tests/overlay/005 |  6 +++++-
>  tests/overlay/010 |  6 +++++-
>  tests/overlay/014 | 10 +++++++++-
>  tests/overlay/035 |  6 +++++-
>  tests/overlay/036 |  5 ++++-
>  tests/overlay/037 |  6 +++++-
>  tests/overlay/038 | 10 +++++++++-
>  tests/overlay/041 | 10 +++++++++-
>  tests/overlay/043 |  6 +++++-
>  tests/overlay/044 | 13 +++++++++++--
>  10 files changed, 67 insertions(+), 11 deletions(-)
>
> diff --git a/tests/overlay/005 b/tests/overlay/005
> index 17992a3..f502d3f 100755
> --- a/tests/overlay/005
> +++ b/tests/overlay/005
> @@ -54,7 +54,7 @@ rm -f $seqres.full
>  # Modify as appropriate.
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>  _require_loop
>
>  # Remove all files from previous tests
> @@ -102,6 +102,10 @@ $XFS_IO_PROG -f -c "o" ${SCRATCH_MNT}/test_file \
>  # unmount overlayfs
>  $UMOUNT_PROG $SCRATCH_MNT
>
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after test
> +_overlay_check_scratch_dirs $lowerd $upperd $workd
> +
>  # unmount undelying xfs, this tiggers panic if memleak happens
>  $UMOUNT_PROG ${OVL_BASE_SCRATCH_MNT}/uppermnt
>  $UMOUNT_PROG ${OVL_BASE_SCRATCH_MNT}/lowermnt
> diff --git a/tests/overlay/010 b/tests/overlay/010
> index f55ebec..f3b50d7 100755
> --- a/tests/overlay/010
> +++ b/tests/overlay/010
> @@ -48,7 +48,7 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>
>  # Remove all files from previous tests
>  _scratch_mkfs
> @@ -70,6 +70,10 @@ mknod $lowerdir2/testdir/a c 0 0
>  _overlay_scratch_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
>  rm -rf $SCRATCH_MNT/testdir
>
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after test
> +_overlay_check_scratch_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
> +
>  # success, all done
>  echo "Silence is golden"
>  status=0
> diff --git a/tests/overlay/014 b/tests/overlay/014
> index 9f308d3..069d8b6 100755
> --- a/tests/overlay/014
> +++ b/tests/overlay/014
> @@ -53,7 +53,7 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>
>  # Remove all files from previous tests
>  _scratch_mkfs
> @@ -78,6 +78,10 @@ mkdir -p $SCRATCH_MNT/testdir/visibledir
>  # unmount overlayfs but not base fs
>  $UMOUNT_PROG $SCRATCH_MNT
>
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after each test
> +_overlay_check_scratch_dirs $lowerdir1 $lowerdir2 $workdir2
> +
>  # mount overlay again, with lowerdir1 and lowerdir2 as multiple lowerdirs,
>  # and create a new file in testdir, triggers copyup from lowerdir,
>  # copyup should not copy overlayfs private xattr
> @@ -90,6 +94,10 @@ $UMOUNT_PROG $SCRATCH_MNT
>  _overlay_scratch_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
>  ls $SCRATCH_MNT/testdir
>
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after each test
> +_overlay_check_scratch_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
> +
>  # success, all done
>  status=0
>  exit
> diff --git a/tests/overlay/035 b/tests/overlay/035
> index 0544774..bc73013 100755
> --- a/tests/overlay/035
> +++ b/tests/overlay/035
> @@ -51,7 +51,7 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>  _require_chattr i
>
>  # Remove all files from previous tests
> @@ -81,6 +81,10 @@ _overlay_scratch_mount_dirs $lowerdir2 $upperdir $workdir
>  touch $SCRATCH_MNT/bar 2>&1 | _filter_scratch
>  _scratch_remount rw 2>&1 | _filter_ro_mount
>
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after test
> +_overlay_check_scratch_dirs $lowerdir2 $upperdir $workdir
> +
>  # success, all done
>  status=0
>  exit
> diff --git a/tests/overlay/036 b/tests/overlay/036
> index e0c13ae..ca5adbd 100755
> --- a/tests/overlay/036
> +++ b/tests/overlay/036
> @@ -69,7 +69,7 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>  _require_scratch_feature index
>
>  # Remove all files from previous tests
> @@ -110,6 +110,9 @@ _overlay_mount_dirs $lowerdir2 $upperdir $workdir2 \
>  _overlay_mount_dirs $lowerdir2 $upperdir2 $workdir \
>             overlay3 $SCRATCH_MNT -oindex=on 2>&1 | _filter_busy_mount
>
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# but which will not lead to inconsistency after this mount test,
> +# so do not check optionally dirs after test

What? Why are we not calling check here?
We should call check with the first _overlay_mount_dirs parameters
($lowerdir $upperdir $workdir)

>
>  # success, all done
>  status=0
> diff --git a/tests/overlay/037 b/tests/overlay/037
> index 6710dda..60290a1 100755
> --- a/tests/overlay/037
> +++ b/tests/overlay/037
> @@ -55,7 +55,7 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>  _require_scratch_feature index
>
>  # Remove all files from previous tests
> @@ -87,6 +87,10 @@ $UMOUNT_PROG $SCRATCH_MNT 2>/dev/null
>  # Mount overlay with original lowerdir, upperdir, workdir and index=on - expect success
>  _overlay_scratch_mount_dirs $lowerdir $upperdir $workdir -oindex=on
>
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# but which will not lead to inconsistency after this mount test,
> +# so do not check optionally dirs after test
> +

Again, why not checking here?
We should call check with the first/last _overlay_mount_dirs parameters
($lowerdir $upperdir $workdir -oindex=on)

>  # success, all done
>  status=0
>  exit
> diff --git a/tests/overlay/038 b/tests/overlay/038
> index bd87156..aa20cbc 100755
> --- a/tests/overlay/038
> +++ b/tests/overlay/038
> @@ -46,7 +46,7 @@ _cleanup()
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>  _require_attrs
>  _require_test_program "t_dir_type"
>
> @@ -161,6 +161,10 @@ subdir_d=$($here/src/t_dir_type $pure_lower_dir $pure_lower_subdir_st_ino)
>  [[ $subdir_d == "subdir d" ]] || \
>         echo "Merged dir: Invalid d_ino reported for subdir"
>
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check after each test
> +_check_scratch_fs
> +
>  _scratch_unmount
>
>  # Verify pure lower residing in dir which has another lower layer
> @@ -202,6 +206,10 @@ subdir_d=$($here/src/t_dir_type $pure_lower_dir $pure_lower_subdir_st_ino)
>  [[ $subdir_d == "subdir d" ]] || \
>         echo "Pure lower in dir which has another lower layer: Invalid d_ino reported for subdir"
>
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after each test
> +_overlay_check_scratch_dirs "$middir:$lowerdir" $upperdir $workdir
> +

-o index=on here as well

Thanks,
Amir.

Re: [xfstests PATCH v2 5/5] overlay: correct scratch dirs check

[Eryu Guan]

On Wed, Jan 31, 2018 at 06:27:59PM +0800, zhangyi (F) wrote:
> Tests that use _overlay_scratch_mount_dirs instead of _scratch_mount
> should use _require_overlay_scratch_dirs instead of _require_scratch.

This commit log just describes what to do but not why, I guess it's
because these tests are either mounting with multiple lower dirs or
mounting with non-default lower/upper/work dir, so
_check_overlay_scratch_fs won't handle these cases correctly, we need to
call _overlay_check_scratch_dirs with the correct arguments.

> Those tests can optionally call _overlay_check_scratch_dirs at the end
> of the test or before _scratch_umount to umount $SCRATCH_MNT and
> run the checker.
> 
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  tests/overlay/005 |  6 +++++-
>  tests/overlay/010 |  6 +++++-
>  tests/overlay/014 | 10 +++++++++-
>  tests/overlay/035 |  6 +++++-
>  tests/overlay/036 |  5 ++++-
>  tests/overlay/037 |  6 +++++-
>  tests/overlay/038 | 10 +++++++++-
>  tests/overlay/041 | 10 +++++++++-
>  tests/overlay/043 |  6 +++++-
>  tests/overlay/044 | 13 +++++++++++--
>  10 files changed, 67 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/overlay/005 b/tests/overlay/005
> index 17992a3..f502d3f 100755
> --- a/tests/overlay/005
> +++ b/tests/overlay/005
> @@ -54,7 +54,7 @@ rm -f $seqres.full
>  # Modify as appropriate.
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs

As mentioned in previous reply, just use _require_scratch_nocheck for
such cases with comments why.

>  _require_loop
>  
>  # Remove all files from previous tests
> @@ -102,6 +102,10 @@ $XFS_IO_PROG -f -c "o" ${SCRATCH_MNT}/test_file \
>  # unmount overlayfs
>  $UMOUNT_PROG $SCRATCH_MNT
>  
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after test

Such comments (and similar comments below) don't provide any additional
information. And with proper comments for _require_scratch_nocheck, I
think we could remove the comments here.

> +_overlay_check_scratch_dirs $lowerd $upperd $workd
> +
>  # unmount undelying xfs, this tiggers panic if memleak happens
>  $UMOUNT_PROG ${OVL_BASE_SCRATCH_MNT}/uppermnt
>  $UMOUNT_PROG ${OVL_BASE_SCRATCH_MNT}/lowermnt
> diff --git a/tests/overlay/010 b/tests/overlay/010
> index f55ebec..f3b50d7 100755
> --- a/tests/overlay/010
> +++ b/tests/overlay/010
> @@ -48,7 +48,7 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>  
>  # Remove all files from previous tests
>  _scratch_mkfs
> @@ -70,6 +70,10 @@ mknod $lowerdir2/testdir/a c 0 0
>  _overlay_scratch_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
>  rm -rf $SCRATCH_MNT/testdir
>  
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after test
> +_overlay_check_scratch_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
> +
>  # success, all done
>  echo "Silence is golden"
>  status=0
> diff --git a/tests/overlay/014 b/tests/overlay/014
> index 9f308d3..069d8b6 100755
> --- a/tests/overlay/014
> +++ b/tests/overlay/014
> @@ -53,7 +53,7 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>  
>  # Remove all files from previous tests
>  _scratch_mkfs
> @@ -78,6 +78,10 @@ mkdir -p $SCRATCH_MNT/testdir/visibledir
>  # unmount overlayfs but not base fs
>  $UMOUNT_PROG $SCRATCH_MNT
>  
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after each test
> +_overlay_check_scratch_dirs $lowerdir1 $lowerdir2 $workdir2
> +
>  # mount overlay again, with lowerdir1 and lowerdir2 as multiple lowerdirs,
>  # and create a new file in testdir, triggers copyup from lowerdir,
>  # copyup should not copy overlayfs private xattr
> @@ -90,6 +94,10 @@ $UMOUNT_PROG $SCRATCH_MNT
>  _overlay_scratch_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
>  ls $SCRATCH_MNT/testdir
>  
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after each test
> +_overlay_check_scratch_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
> +
>  # success, all done
>  status=0
>  exit
> diff --git a/tests/overlay/035 b/tests/overlay/035
> index 0544774..bc73013 100755
> --- a/tests/overlay/035
> +++ b/tests/overlay/035
> @@ -51,7 +51,7 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>  _require_chattr i
>  
>  # Remove all files from previous tests
> @@ -81,6 +81,10 @@ _overlay_scratch_mount_dirs $lowerdir2 $upperdir $workdir
>  touch $SCRATCH_MNT/bar 2>&1 | _filter_scratch
>  _scratch_remount rw 2>&1 | _filter_ro_mount
>  
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after test
> +_overlay_check_scratch_dirs $lowerdir2 $upperdir $workdir
> +
>  # success, all done
>  status=0
>  exit
> diff --git a/tests/overlay/036 b/tests/overlay/036
> index e0c13ae..ca5adbd 100755
> --- a/tests/overlay/036
> +++ b/tests/overlay/036
> @@ -69,7 +69,7 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>  _require_scratch_feature index
>  
>  # Remove all files from previous tests
> @@ -110,6 +110,9 @@ _overlay_mount_dirs $lowerdir2 $upperdir $workdir2 \
>  _overlay_mount_dirs $lowerdir2 $upperdir2 $workdir \
>  	    overlay3 $SCRATCH_MNT -oindex=on 2>&1 | _filter_busy_mount
>  
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# but which will not lead to inconsistency after this mount test,
> +# so do not check optionally dirs after test

It's not clear where does the inconsistency come from and why it's
expected (as we don't do fsck after test).

>  
>  # success, all done
>  status=0
> diff --git a/tests/overlay/037 b/tests/overlay/037
> index 6710dda..60290a1 100755
> --- a/tests/overlay/037
> +++ b/tests/overlay/037
> @@ -55,7 +55,7 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>  _require_scratch_feature index
>  
>  # Remove all files from previous tests
> @@ -87,6 +87,10 @@ $UMOUNT_PROG $SCRATCH_MNT 2>/dev/null
>  # Mount overlay with original lowerdir, upperdir, workdir and index=on - expect success
>  _overlay_scratch_mount_dirs $lowerdir $upperdir $workdir -oindex=on
>  
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# but which will not lead to inconsistency after this mount test,
> +# so do not check optionally dirs after test
> +

Same here.

Thanks,
Eryu

>  # success, all done
>  status=0
>  exit
> diff --git a/tests/overlay/038 b/tests/overlay/038
> index bd87156..aa20cbc 100755
> --- a/tests/overlay/038
> +++ b/tests/overlay/038
> @@ -46,7 +46,7 @@ _cleanup()
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>  _require_attrs
>  _require_test_program "t_dir_type"
>  
> @@ -161,6 +161,10 @@ subdir_d=$($here/src/t_dir_type $pure_lower_dir $pure_lower_subdir_st_ino)
>  [[ $subdir_d == "subdir d" ]] || \
>  	echo "Merged dir: Invalid d_ino reported for subdir"
>  
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check after each test
> +_check_scratch_fs
> +
>  _scratch_unmount
>  
>  # Verify pure lower residing in dir which has another lower layer
> @@ -202,6 +206,10 @@ subdir_d=$($here/src/t_dir_type $pure_lower_dir $pure_lower_subdir_st_ino)
>  [[ $subdir_d == "subdir d" ]] || \
>  	echo "Pure lower in dir which has another lower layer: Invalid d_ino reported for subdir"
>  
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after each test
> +_overlay_check_scratch_dirs "$middir:$lowerdir" $upperdir $workdir
> +
>  echo "Silence is golden"
>  status=0
>  exit
> diff --git a/tests/overlay/041 b/tests/overlay/041
> index 4152107..607908a 100755
> --- a/tests/overlay/041
> +++ b/tests/overlay/041
> @@ -48,7 +48,7 @@ _cleanup()
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>  _require_test
>  _require_attrs
>  _require_test_program "t_dir_type"
> @@ -167,6 +167,10 @@ subdir_d=$($here/src/t_dir_type $pure_lower_dir $pure_lower_subdir_st_ino)
>  [[ $subdir_d == "subdir d" ]] || \
>  	echo "Merged dir: Invalid d_ino reported for subdir"
>  
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after each test
> +_overlay_check_scratch_dirs $lowerdir $upperdir $workdir
> +
>  _scratch_unmount
>  
>  # Verify pure lower residing in dir which has another lower layer
> @@ -208,6 +212,10 @@ subdir_d=$($here/src/t_dir_type $pure_lower_dir $pure_lower_subdir_st_ino)
>  [[ $subdir_d == "subdir d" ]] || \
>  	echo "Pure lower in dir which has another lower layer: Invalid d_ino reported for subdir"
>  
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after each test
> +_overlay_check_scratch_dirs "$middir:$lowerdir" $upperdir $workdir
> +
>  echo "Silence is golden"
>  status=0
>  exit
> diff --git a/tests/overlay/043 b/tests/overlay/043
> index 858b6a9..76e0dd9 100755
> --- a/tests/overlay/043
> +++ b/tests/overlay/043
> @@ -56,7 +56,7 @@ _cleanup()
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>  _require_test
>  _require_test_program "af_unix"
>  _require_test_program "t_dir_type"
> @@ -155,6 +155,10 @@ _overlay_scratch_mount_dirs $lowerdir $upperdir $workdir \
>  # Compare inode numbers before/after mount cycle
>  check_inode_numbers $testdir $tmp.after_move $tmp.after_cycle
>  
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after test
> +_overlay_check_scratch_dirs $lowerdir $upperdir $workdir
> +
>  echo "Silence is golden"
>  status=0
>  exit
> diff --git a/tests/overlay/044 b/tests/overlay/044
> index 9c0ff04..65591f9 100755
> --- a/tests/overlay/044
> +++ b/tests/overlay/044
> @@ -49,7 +49,7 @@ _cleanup()
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +_require_overlay_scratch_dirs
>  _require_test
>  _require_scratch_feature index
>  _require_test_program "t_dir_type"
> @@ -122,8 +122,13 @@ echo "== After write one =="
>  cat $FILES
>  check_ino_nlink $SCRATCH_MNT $tmp.before $tmp.after_one
>  
> -# Verify that the hardlinks survive a mount cycle
>  $UMOUNT_PROG $SCRATCH_MNT
> +
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after each test
> +_overlay_check_scratch_dirs $lowerdir $upperdir $workdir "-o index=on"
> +
> +# Verify that the hardlinks survive a mount cycle
>  _overlay_scratch_mount_dirs $lowerdir $upperdir $workdir \
>  			    $OVERLAY_MOUNT_OPTIONS
>  
> @@ -141,5 +146,9 @@ echo "== After write two =="
>  cat $FILES
>  check_ino_nlink $SCRATCH_MNT $tmp.after_one $tmp.after_two
>  
> +# We called _require_overlay_scratch_dirs instead of _require_scratch,
> +# do check optionally dirs after test
> +_overlay_check_scratch_dirs $lowerdir $upperdir $workdir
> +
>  status=0
>  exit
> -- 
> 2.5.0
> 

Re: [xfstests PATCH v2 0/5] overlay: add overlay filesystem dirs check

[Eryu Guan]

On Wed, Jan 31, 2018 at 06:27:54PM +0800, zhangyi (F) wrote:
> Hi, All:
> 
> This patchset implement filesystem check for overlay filesystem, base on
> my "overlay: add fsck.overlay basic tests" patchset and works only if
> fsck.overlay[1] exists (otherwise not run).
> 
> Patch 1 improve and fix _is_mounted helper in common/rc.
> Patch 2 hook fsck helper to _check_test_fs & _check_scratch_fs, and
> introduce helpers for optionally scratch dirs.
> Patch 3~5 modify overlay test cases to do fs check correctly.

Thanks a lot for all the patches and revisions! I just went through this
patchset and had additional comments. I'm sorry again for being late on
review..

> 
> Hi, Eryu:
> 
> The patch 5/5 conflict with Amir's patchset, let me know if you want me
> to rebase it.

Yes, please, rebasing against current master would be great, thanks!

Eryu

> 
> Thanks,
> Yi.
> 
> [1] https://github.com/hisilicon/overlayfs-progs
> 
> Changes since v1:
> - Details comments in patch 1/5.
> - Improve _overlay_check_scratch_dirs to accept extra options.
> - Fix basic filesystem mounted check in _overlay_check_fs.
> - Improve overlay/044 in patch 5/5 to add index=on option.
> 
> 
> zhangyi (F) (5):
>   common/rc: improve mounted check helper
>   overlay: hook filesystem check helper
>   overlay/003: fix fs check failure
>   overlay: skip check for tests finished with corrupt filesystem
>   overlay: correct scratch dirs check
> 
>  common/overlay    | 128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  common/rc         |  29 +++++--------
>  tests/overlay/003 |   1 -
>  tests/overlay/005 |   6 ++-
>  tests/overlay/010 |   6 ++-
>  tests/overlay/014 |  10 ++++-
>  tests/overlay/019 |   2 +-
>  tests/overlay/031 |   2 +-
>  tests/overlay/035 |   6 ++-
>  tests/overlay/036 |   5 ++-
>  tests/overlay/037 |   6 ++-
>  tests/overlay/038 |  10 ++++-
>  tests/overlay/041 |  10 ++++-
>  tests/overlay/043 |   6 ++-
>  tests/overlay/044 |  13 +++++-
>  15 files changed, 209 insertions(+), 31 deletions(-)
> 
> -- 
> 2.5.0
>