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

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

[zhangyi (F)]

Hi Eryu and Amir:

I have handle comments from last iteration in patch {2,5}/5, please
review.

Thanks,
Yi.

Changes since v4:
- Clarify comments in patch 2/5, and move _overlay_check_scratch_dirs
  helper to patch 5/5.
- Modify input arguments of _overlay_fsck_dirs in _overlay_check_dirs.
- Modify OVL_FSCK_OPTIONS example in README.overlay.

Changes since v3:
- Move overlay checker after _unmount in overlay/{038,041}, and embed
  checker into unmount_dirs() in overlay/{051,053,055}.
- split out patch "overlay: correct test mount options"

Changes since v2:
- Rename _is_mounted to _is_dev_mounted and use findmnt instead of mount.
- Fix mistakes in patch 2/6 and introduce _is_dir_mountpoint helper, also
  introduce fsck options use for overlayfs.
- Add overlay/049 into patch 4/6 to skip fs check.
- Add nfs export cases into patch 5/6 to do correct dirs check.
- Add the "missing feature" of test mount options.

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.

----------

Background:

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).

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

zhangyi (F) (5):
  common/rc: improve dev 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

 README.overlay    |  10 ++++-
 common/config     |   4 ++
 common/overlay    | 128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 common/rc         |  47 +++++++++++---------
 common/xfs        |   2 +-
 tests/overlay/003 |   1 -
 tests/overlay/005 |   7 ++-
 tests/overlay/010 |   7 ++-
 tests/overlay/014 |  10 ++++-
 tests/overlay/019 |   2 +-
 tests/overlay/031 |   2 +-
 tests/overlay/035 |   7 ++-
 tests/overlay/036 |   6 ++-
 tests/overlay/037 |   7 ++-
 tests/overlay/038 |  10 ++++-
 tests/overlay/041 |  10 ++++-
 tests/overlay/043 |   7 ++-
 tests/overlay/044 |   8 +++-
 tests/overlay/049 |   2 +-
 tests/overlay/051 |  10 ++++-
 tests/overlay/053 |  10 ++++-
 tests/overlay/055 |  10 ++++-
 22 files changed, 263 insertions(+), 44 deletions(-)

-- 
2.5.0

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

[zhangyi (F)]

There is a problem of missing fstype check in _is_mounted() helper,
it will return the mountpoint if only the device arguments matches.

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 rename _is_mounted to _is_dev_mounted because it check
the given device only (not mount dir), and add an optional "fstype"
parameter, let user specify file system type instead of default
FSTYPE. Finally, use findmnt instead of mount to avoid complex
processing of mount info and fix this problem simply.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 common/rc  | 29 ++++++++++-------------------
 common/xfs |  2 +-
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/common/rc b/common/rc
index a68e6cc..1dae3bf 100644
--- a/common/rc
+++ b/common/rc
@@ -2381,27 +2381,18 @@ _scratch_mkfs_richacl()
 	esac
 }
 
-# check that a FS on a device is mounted
-# if so, return mount point
-#
-_is_mounted()
+# check if the given device is mounted, if so, return mount point
+_is_dev_mounted()
 {
-    if [ $# -ne 1 ]
-    then
-	echo "Usage: _is_mounted device" 1>&2
-	exit 1
-    fi
+	local dev=$1
+	local fstype=${2:-$FSTYP}
 
-    device=$1
+	if [ $# -lt 1 ]; then
+		echo "Usage: _is_dev_mounted <device> [fstype]" 1>&2
+		exit 1
+	fi
 
-    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
+	findmnt -rncv -S $dev -t $fstype -o TARGET | head -1
 }
 
 # remount a FS to a new mode (ro or rw)
@@ -2441,7 +2432,7 @@ _umount_or_remount_ro()
     fi
 
     device=$1
-    mountpoint=`_is_mounted $device`
+    mountpoint=`_is_dev_mounted $device`
 
     if [ $USE_REMOUNT -eq 0 ]; then
         $UMOUNT_PROG $device
diff --git a/common/xfs b/common/xfs
index 354f584..37cd80c 100644
--- a/common/xfs
+++ b/common/xfs
@@ -356,7 +356,7 @@ _check_xfs_filesystem()
 	ok=1
 
 	# Run online scrub if we can.
-	mntpt="$(_is_mounted $device)"
+	mntpt="$(_is_dev_mounted $device)"
 	if [ -n "$mntpt" ] && _supports_xfs_scrub "$mntpt" "$device"; then
 		"$XFS_SCRUB_PROG" $scrubflag -v -d -n $device > $tmp.scrub 2>&1
 		if [ $? -ne 0 ]; then
-- 
2.5.0

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

[zhangyi (F)]

Hook filesystem check helper to _check_test_fs and _check_scratch_fs
for checking consistency of underlying dirs of overlay filesystem.
These helpers works only if fsck.overlay exists.

This patch introduce OVERLAY_FSCK_OPTIONS use for check overlayfs like
OVERLAY_MOUNT_OPTIONS, and also introduce a mount point check helper in
common/rc to detect a dir is a mount point or not.

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

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 README.overlay | 10 ++++--
 common/config  |  4 +++
 common/overlay | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 common/rc      | 18 +++++++++--
 4 files changed, 127 insertions(+), 4 deletions(-)

diff --git a/README.overlay b/README.overlay
index dfb8234..3cbefab 100644
--- a/README.overlay
+++ b/README.overlay
@@ -22,6 +22,10 @@ the base fs should be pre-formatted before starting the -overlay run.
 An easy way to accomplish this is by running './check <some test>' once,
 before running './check -overlay'.
 
+'./check -overlay' support check overlay test and scratch dirs,
+OVERLAY_FSCK_OPTIONS should be set instead of FSCK_OPTIONS if fsck
+options need to given directly.
+
 Because of the lack of mkfs support, multi-section config files are only
 partly supported with './check -overlay'. Only multi-section files that
 do not change FSTYP and MKFS_OPTIONS can be safely used with -overlay.
@@ -40,7 +44,9 @@ run overlay tests on the same base fs, but with different mount options:
  MOUNT_OPTIONS="-o pquota"
  TEST_FS_MOUNT_OPTS="-o noatime"
  OVERLAY_MOUNT_OPTIONS="-o redirect_dir=off"
+ OVERLAY_FSCK_OPTIONS="-n redirect_dir=off"
 
 In the example above, MOUNT_OPTIONS will be used to mount the base scratch fs,
-TEST_FS_MOUNT_OPTS will be used to mount the base test fs and
-OVERLAY_MOUNT_OPTIONS will be used to mount both test and scratch overlays.
+TEST_FS_MOUNT_OPTS will be used to mount the base test fs,
+OVERLAY_MOUNT_OPTIONS will be used to mount both test and scratch overlay and
+OVERLAY_FSCK_OPTIONS will be used to check both test and scratch overlay.
diff --git a/common/config b/common/config
index 71115bd..20f0e5f 100644
--- a/common/config
+++ b/common/config
@@ -566,6 +566,10 @@ _overlay_config_override()
 	# Set SCRATCH vars to overlay base and mount dirs inside base fs
 	export SCRATCH_DEV="$OVL_BASE_SCRATCH_MNT"
 	export SCRATCH_MNT="$OVL_BASE_SCRATCH_MNT/$OVL_MNT"
+
+	# Set fsck options, use default if user not set directly.
+	export FSCK_OPTIONS="$OVERLAY_FSCK_OPTIONS"
+	[ -z "$FSCK_OPTIONS" ] && _fsck_opts
 }
 
 _overlay_config_restore()
diff --git a/common/overlay b/common/overlay
index e1b07f4..5d01e5e 100644
--- a/common/overlay
+++ b/common/overlay
@@ -190,3 +190,102 @@ _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
+	shift 3
+	local err=0
+
+	_overlay_fsck_dirs $lowerdir $upperdir $workdir \
+			   $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
+}
+
+_overlay_check_fs()
+{
+	# The first arguments is overlay mount point use for checking
+	# overlay filesystem is mounted or not, the remaining arquments
+	# use for mounting overlay base filesystem if it was not mounted.
+	# We shift one to 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_dev is set but base_fstype is empty, base fs is not
+	# mounted, we need to mount base fs. Otherwise, we need to
+	# check and umount overlayfs if it was mounted.
+	if [ -n "$base_dev" -a -z "$base_fstype" ]; then
+		_overlay_base_mount $*
+	else
+		# Check and umount overlay for dir check
+		ovl_mounted=`_is_dir_mountpoint $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 [ -n "$base_dev" -a -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 1dae3bf..9317674 100644
--- a/common/rc
+++ b/common/rc
@@ -2395,6 +2395,20 @@ _is_dev_mounted()
 	findmnt -rncv -S $dev -t $fstype -o TARGET | head -1
 }
 
+# check if the given dir is a mount point, if so, return mount point
+_is_dir_mountpoint()
+{
+	local dir=$1
+	local fstype=${2:-$FSTYP}
+
+	if [ $# -lt 1 ]; then
+		echo "Uasge: _is_dir_mountpoint <dir> [fstype]" 1>&2
+		exit 1
+	fi
+
+	findmnt -rncv -t $fstype -o TARGET $dir | head -1
+}
+
 # remount a FS to a new mode (ro or rw)
 #
 _remount()
@@ -2590,7 +2604,7 @@ _check_test_fs()
 	# no way to check consistency for GlusterFS
 	;;
     overlay)
-	# no way to check consistency for overlay
+	_check_overlay_test_fs
 	;;
     pvfs2)
 	;;
@@ -2648,7 +2662,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 v5 2/5] overlay: hook filesystem check helper

[Amir Goldstein]

On Thu, Mar 1, 2018 at 2:13 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> Hook filesystem check helper to _check_test_fs and _check_scratch_fs
> for checking consistency of underlying dirs of overlay filesystem.
> These helpers works only if fsck.overlay exists.
>
> This patch introduce OVERLAY_FSCK_OPTIONS use for check overlayfs like
> OVERLAY_MOUNT_OPTIONS, and also introduce a mount point check helper in
> common/rc to detect a dir is a mount point or not.
>
> [ _check_test_fs/_check_scratch_fs part picked from Amir's patch, thanks ]
>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> ---
>  README.overlay | 10 ++++--
>  common/config  |  4 +++
>  common/overlay | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  common/rc      | 18 +++++++++--
>  4 files changed, 127 insertions(+), 4 deletions(-)
>
> diff --git a/README.overlay b/README.overlay
> index dfb8234..3cbefab 100644
> --- a/README.overlay
> +++ b/README.overlay
> @@ -22,6 +22,10 @@ the base fs should be pre-formatted before starting the -overlay run.
>  An easy way to accomplish this is by running './check <some test>' once,
>  before running './check -overlay'.
>
> +'./check -overlay' support check overlay test and scratch dirs,
> +OVERLAY_FSCK_OPTIONS should be set instead of FSCK_OPTIONS if fsck
> +options need to given directly.
> +
>  Because of the lack of mkfs support, multi-section config files are only
>  partly supported with './check -overlay'. Only multi-section files that
>  do not change FSTYP and MKFS_OPTIONS can be safely used with -overlay.
> @@ -40,7 +44,9 @@ run overlay tests on the same base fs, but with different mount options:
>   MOUNT_OPTIONS="-o pquota"
>   TEST_FS_MOUNT_OPTS="-o noatime"
>   OVERLAY_MOUNT_OPTIONS="-o redirect_dir=off"
> + OVERLAY_FSCK_OPTIONS="-n redirect_dir=off"

typo for the phony options. I believe it is going to be:  "-n -o
redirect_dir=off"
>
>  In the example above, MOUNT_OPTIONS will be used to mount the base scratch fs,
> -TEST_FS_MOUNT_OPTS will be used to mount the base test fs and
> -OVERLAY_MOUNT_OPTIONS will be used to mount both test and scratch overlays.
> +TEST_FS_MOUNT_OPTS will be used to mount the base test fs,
> +OVERLAY_MOUNT_OPTIONS will be used to mount both test and scratch overlay and
> +OVERLAY_FSCK_OPTIONS will be used to check both test and scratch overlay.
> diff --git a/common/config b/common/config
> index 71115bd..20f0e5f 100644
> --- a/common/config
> +++ b/common/config
> @@ -566,6 +566,10 @@ _overlay_config_override()
>         # Set SCRATCH vars to overlay base and mount dirs inside base fs
>         export SCRATCH_DEV="$OVL_BASE_SCRATCH_MNT"
>         export SCRATCH_MNT="$OVL_BASE_SCRATCH_MNT/$OVL_MNT"
> +
> +       # Set fsck options, use default if user not set directly.
> +       export FSCK_OPTIONS="$OVERLAY_FSCK_OPTIONS"
> +       [ -z "$FSCK_OPTIONS" ] && _fsck_opts
>  }
>
>  _overlay_config_restore()
> diff --git a/common/overlay b/common/overlay
> index e1b07f4..5d01e5e 100644
> --- a/common/overlay
> +++ b/common/overlay
> @@ -190,3 +190,102 @@ _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
> +       shift 3
> +       local err=0
> +
> +       _overlay_fsck_dirs $lowerdir $upperdir $workdir \
> +                          $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
> +}
> +
> +_overlay_check_fs()
> +{
> +       # The first arguments is overlay mount point use for checking
> +       # overlay filesystem is mounted or not, the remaining arquments
> +       # use for mounting overlay base filesystem if it was not mounted.
> +       # We shift one to 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_dev is set but base_fstype is empty, base fs is not
> +       # mounted, we need to mount base fs. Otherwise, we need to
> +       # check and umount overlayfs if it was mounted.
> +       if [ -n "$base_dev" -a -z "$base_fstype" ]; then
> +               _overlay_base_mount $*
> +       else
> +               # Check and umount overlay for dir check
> +               ovl_mounted=`_is_dir_mountpoint $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 [ -n "$base_dev" -a -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 1dae3bf..9317674 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2395,6 +2395,20 @@ _is_dev_mounted()
>         findmnt -rncv -S $dev -t $fstype -o TARGET | head -1
>  }
>
> +# check if the given dir is a mount point, if so, return mount point
> +_is_dir_mountpoint()
> +{
> +       local dir=$1
> +       local fstype=${2:-$FSTYP}
> +
> +       if [ $# -lt 1 ]; then
> +               echo "Uasge: _is_dir_mountpoint <dir> [fstype]" 1>&2
> +               exit 1
> +       fi
> +
> +       findmnt -rncv -t $fstype -o TARGET $dir | head -1
> +}
> +
>  # remount a FS to a new mode (ro or rw)
>  #
>  _remount()
> @@ -2590,7 +2604,7 @@ _check_test_fs()
>         # no way to check consistency for GlusterFS
>         ;;
>      overlay)
> -       # no way to check consistency for overlay
> +       _check_overlay_test_fs
>         ;;
>      pvfs2)
>         ;;
> @@ -2648,7 +2662,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 v5 2/5] overlay: hook filesystem check helper

[zhangyi (F)]

On 2018/3/1 21:03, Amir Goldstein Wrote:
> On Thu, Mar 1, 2018 at 2:13 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> Hook filesystem check helper to _check_test_fs and _check_scratch_fs
>> for checking consistency of underlying dirs of overlay filesystem.
>> These helpers works only if fsck.overlay exists.
>>
>> This patch introduce OVERLAY_FSCK_OPTIONS use for check overlayfs like
>> OVERLAY_MOUNT_OPTIONS, and also introduce a mount point check helper in
>> common/rc to detect a dir is a mount point or not.
>>
>> [ _check_test_fs/_check_scratch_fs part picked from Amir's patch, thanks ]
>>
>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>> ---
>>  README.overlay | 10 ++++--
>>  common/config  |  4 +++
>>  common/overlay | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  common/rc      | 18 +++++++++--
>>  4 files changed, 127 insertions(+), 4 deletions(-)
>>
>> diff --git a/README.overlay b/README.overlay
>> index dfb8234..3cbefab 100644
>> --- a/README.overlay
>> +++ b/README.overlay
>> @@ -22,6 +22,10 @@ the base fs should be pre-formatted before starting the -overlay run.
>>  An easy way to accomplish this is by running './check <some test>' once,
>>  before running './check -overlay'.
>>
>> +'./check -overlay' support check overlay test and scratch dirs,
>> +OVERLAY_FSCK_OPTIONS should be set instead of FSCK_OPTIONS if fsck
>> +options need to given directly.
>> +
>>  Because of the lack of mkfs support, multi-section config files are only
>>  partly supported with './check -overlay'. Only multi-section files that
>>  do not change FSTYP and MKFS_OPTIONS can be safely used with -overlay.
>> @@ -40,7 +44,9 @@ run overlay tests on the same base fs, but with different mount options:
>>   MOUNT_OPTIONS="-o pquota"
>>   TEST_FS_MOUNT_OPTS="-o noatime"
>>   OVERLAY_MOUNT_OPTIONS="-o redirect_dir=off"
>> + OVERLAY_FSCK_OPTIONS="-n redirect_dir=off"
> 
> typo for the phony options. I believe it is going to be:  "-n -o
> redirect_dir=off"

Oh, sorry for the mistake, it is "-n -o redirect_dir=off"

Thanks,
Yi.

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

[Amir Goldstein]

On Thu, Mar 1, 2018 at 3:11 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> On 2018/3/1 21:03, Amir Goldstein Wrote:
>> On Thu, Mar 1, 2018 at 2:13 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>> Hook filesystem check helper to _check_test_fs and _check_scratch_fs
>>> for checking consistency of underlying dirs of overlay filesystem.
>>> These helpers works only if fsck.overlay exists.
>>>
>>> This patch introduce OVERLAY_FSCK_OPTIONS use for check overlayfs like
>>> OVERLAY_MOUNT_OPTIONS, and also introduce a mount point check helper in
>>> common/rc to detect a dir is a mount point or not.
>>>
>>> [ _check_test_fs/_check_scratch_fs part picked from Amir's patch, thanks ]
>>>
>>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>>> ---
>>>  README.overlay | 10 ++++--
>>>  common/config  |  4 +++
>>>  common/overlay | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  common/rc      | 18 +++++++++--
>>>  4 files changed, 127 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/README.overlay b/README.overlay
>>> index dfb8234..3cbefab 100644
>>> --- a/README.overlay
>>> +++ b/README.overlay
>>> @@ -22,6 +22,10 @@ the base fs should be pre-formatted before starting the -overlay run.
>>>  An easy way to accomplish this is by running './check <some test>' once,
>>>  before running './check -overlay'.
>>>
>>> +'./check -overlay' support check overlay test and scratch dirs,
>>> +OVERLAY_FSCK_OPTIONS should be set instead of FSCK_OPTIONS if fsck
>>> +options need to given directly.
>>> +
>>>  Because of the lack of mkfs support, multi-section config files are only
>>>  partly supported with './check -overlay'. Only multi-section files that
>>>  do not change FSTYP and MKFS_OPTIONS can be safely used with -overlay.
>>> @@ -40,7 +44,9 @@ run overlay tests on the same base fs, but with different mount options:
>>>   MOUNT_OPTIONS="-o pquota"
>>>   TEST_FS_MOUNT_OPTS="-o noatime"
>>>   OVERLAY_MOUNT_OPTIONS="-o redirect_dir=off"
>>> + OVERLAY_FSCK_OPTIONS="-n redirect_dir=off"
>>
>> typo for the phony options. I believe it is going to be:  "-n -o
>> redirect_dir=off"
>
> Oh, sorry for the mistake, it is "-n -o redirect_dir=off"
>

Zhangyi,

One thing that has occurred to me now, which should be fixed before
first release
is that fsck.overlay should return an error for unknown/unsupported options
(e.g. fsck.overlay -o blah doesn't complain).
If this doesn't happen for first release then in the future we will
not be able to
write test scripts to check if installed fsck.overlay version supports
feature X.

About the possible meaning of <feature>=on/off and the meaning of <defaults>
in the context of fsck.overlay, I think it is worth a separate
discussion, but the
way I see it:
- When fsck.overlay *supports* a <feature> it should be able to detect if that
  <feature> was ever enabled on overlay (e.g. known xattr exists or
index dir exists).
- When fsck.overlay detects an unknown <feature> it should abort
- -o <feature>=off means wipe all traces of <feature> from overlay and fix if
  necessary (e.g. make a copy of redirect dir before removing
redirect_dir xattr)
- -o <feature>=on means bring overlay to a "feature consistent state",
as if feature
  was enabled from day 1 (e.g. index all copies up lower hardlinks)
- If <feature>=[on/off] is not specified, then if <feature> is not
detected, it should
  not be explicitly enabled and if feature is detected it should be
made consistent

That last rule is certainly debatable, so maybe best if we leave that
decision to
admin via default policy configuration file.

Cheers,
Amir.

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

[zhangyi (F)]

On 2018/3/11 16:12, Amir Goldstein Wrote:
> On Thu, Mar 1, 2018 at 3:11 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> On 2018/3/1 21:03, Amir Goldstein Wrote:
>>> On Thu, Mar 1, 2018 at 2:13 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>> Hook filesystem check helper to _check_test_fs and _check_scratch_fs
>>>> for checking consistency of underlying dirs of overlay filesystem.
>>>> These helpers works only if fsck.overlay exists.
>>>>
>>>> This patch introduce OVERLAY_FSCK_OPTIONS use for check overlayfs like
>>>> OVERLAY_MOUNT_OPTIONS, and also introduce a mount point check helper in
>>>> common/rc to detect a dir is a mount point or not.
>>>>
>>>> [ _check_test_fs/_check_scratch_fs part picked from Amir's patch, thanks ]
>>>>
>>>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>>>> ---
>>>>  README.overlay | 10 ++++--
>>>>  common/config  |  4 +++
>>>>  common/overlay | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  common/rc      | 18 +++++++++--
>>>>  4 files changed, 127 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/README.overlay b/README.overlay
>>>> index dfb8234..3cbefab 100644
>>>> --- a/README.overlay
>>>> +++ b/README.overlay
>>>> @@ -22,6 +22,10 @@ the base fs should be pre-formatted before starting the -overlay run.
>>>>  An easy way to accomplish this is by running './check <some test>' once,
>>>>  before running './check -overlay'.
>>>>
>>>> +'./check -overlay' support check overlay test and scratch dirs,
>>>> +OVERLAY_FSCK_OPTIONS should be set instead of FSCK_OPTIONS if fsck
>>>> +options need to given directly.
>>>> +
>>>>  Because of the lack of mkfs support, multi-section config files are only
>>>>  partly supported with './check -overlay'. Only multi-section files that
>>>>  do not change FSTYP and MKFS_OPTIONS can be safely used with -overlay.
>>>> @@ -40,7 +44,9 @@ run overlay tests on the same base fs, but with different mount options:
>>>>   MOUNT_OPTIONS="-o pquota"
>>>>   TEST_FS_MOUNT_OPTS="-o noatime"
>>>>   OVERLAY_MOUNT_OPTIONS="-o redirect_dir=off"
>>>> + OVERLAY_FSCK_OPTIONS="-n redirect_dir=off"
>>>
>>> typo for the phony options. I believe it is going to be:  "-n -o
>>> redirect_dir=off"
>>
>> Oh, sorry for the mistake, it is "-n -o redirect_dir=off"
>>
> 
> Zhangyi,
> 
> One thing that has occurred to me now, which should be fixed before
> first release
> is that fsck.overlay should return an error for unknown/unsupported options
> (e.g. fsck.overlay -o blah doesn't complain).
> If this doesn't happen for first release then in the future we will
> not be able to
> write test scripts to check if installed fsck.overlay version supports
> feature X.
> 
Yes, It's time to impliment the feature/option set and do the corresponding
work for the already known features. I am writing the kernel side feature set
recent days, I will add these feature into fsck.overlay after kernel's work done.

BTW, the feature set in kernel do the following, some suggestions?
1) Add feature set into upper root xattr "trusted.overlay.feature_[compat|imcompat|rocompat]"
   if user give mount options "xxx=on" or some feature xattr/dir detected.
2) If unknown imcompat feature in xattr detected, fall back to try to ro-mount.
3) If unknown ro-compat featrure in xattr detected, mount fail.

> About the possible meaning of <feature>=on/off and the meaning of <defaults>
> in the context of fsck.overlay, I think it is worth a separate
> discussion, but the
> way I see it:
> - When fsck.overlay *supports* a <feature> it should be able to detect if that
>   <feature> was ever enabled on overlay (e.g. known xattr exists or
> index dir exists).

Agree, and we can also fix feature set when detect feature.

> - When fsck.overlay detects an unknown <feature> it should abort

Agree. A little more detail: if user specify upper and work dir, check imcompat
unknown feature; if not, check ro-imcompat unknown feature. If detected, fsck
about.

> - -o <feature>=off means wipe all traces of <feature> from overlay and fix if
>   necessary (e.g. make a copy of redirect dir before removing
> redirect_dir xattr)

Agree. But not sure it is easy to implement of making a copy of redirect dir
when redirect_dir=off. Because it modify the directory hierarchy, it may affect
the origin/nlink xattr in files/dirs in index dir when index=on or nfs_export=on,
maybe also affect some other feature future.

> - -o <feature>=on means bring overlay to a "feature consistent state",
> as if feature
>   was enabled from day 1 (e.g. index all copies up lower hardlinks)

Agree.

> - If <feature>=[on/off] is not specified, then if <feature> is not
> detected, it should
>   not be explicitly enabled and if feature is detected it should be
> made consistent

Agree.

> That last rule is certainly debatable, so maybe best if we leave that
> decision to
> admin via default policy configuration file.
> 

Something like /etc/mke2fs.conf but not for mkfs? Do you mean if feature
detected conflict with this config(eg: we detect redirect dir but "redirect_dir=off"
was setted in ths config file), fsck should check and fix fs according to this
config file?

I think it may confusion and conflict with mount options(eg: redirect_dir=on)
previous mount or next mount. And IIUC, there is no such feature config file for
ext4/xfs, fsck.ext4 will detect feature from super block and fix fs according to
this detected result. So I think it's better to detect feature feature by fsck
itself, keep features as it was.

Thanks,
Yi.

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

[Amir Goldstein]

On Mon, Mar 12, 2018 at 10:44 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> On 2018/3/11 16:12, Amir Goldstein Wrote:
>> On Thu, Mar 1, 2018 at 3:11 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>> On 2018/3/1 21:03, Amir Goldstein Wrote:
>>>> On Thu, Mar 1, 2018 at 2:13 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>>> Hook filesystem check helper to _check_test_fs and _check_scratch_fs
>>>>> for checking consistency of underlying dirs of overlay filesystem.
>>>>> These helpers works only if fsck.overlay exists.
>>>>>
>>>>> This patch introduce OVERLAY_FSCK_OPTIONS use for check overlayfs like
>>>>> OVERLAY_MOUNT_OPTIONS, and also introduce a mount point check helper in
>>>>> common/rc to detect a dir is a mount point or not.
>>>>>
>>>>> [ _check_test_fs/_check_scratch_fs part picked from Amir's patch, thanks ]
>>>>>
>>>>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>>>>> ---
>>>>>  README.overlay | 10 ++++--
>>>>>  common/config  |  4 +++
>>>>>  common/overlay | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  common/rc      | 18 +++++++++--
>>>>>  4 files changed, 127 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/README.overlay b/README.overlay
>>>>> index dfb8234..3cbefab 100644
>>>>> --- a/README.overlay
>>>>> +++ b/README.overlay
>>>>> @@ -22,6 +22,10 @@ the base fs should be pre-formatted before starting the -overlay run.
>>>>>  An easy way to accomplish this is by running './check <some test>' once,
>>>>>  before running './check -overlay'.
>>>>>
>>>>> +'./check -overlay' support check overlay test and scratch dirs,
>>>>> +OVERLAY_FSCK_OPTIONS should be set instead of FSCK_OPTIONS if fsck
>>>>> +options need to given directly.
>>>>> +
>>>>>  Because of the lack of mkfs support, multi-section config files are only
>>>>>  partly supported with './check -overlay'. Only multi-section files that
>>>>>  do not change FSTYP and MKFS_OPTIONS can be safely used with -overlay.
>>>>> @@ -40,7 +44,9 @@ run overlay tests on the same base fs, but with different mount options:
>>>>>   MOUNT_OPTIONS="-o pquota"
>>>>>   TEST_FS_MOUNT_OPTS="-o noatime"
>>>>>   OVERLAY_MOUNT_OPTIONS="-o redirect_dir=off"
>>>>> + OVERLAY_FSCK_OPTIONS="-n redirect_dir=off"
>>>>
>>>> typo for the phony options. I believe it is going to be:  "-n -o
>>>> redirect_dir=off"
>>>
>>> Oh, sorry for the mistake, it is "-n -o redirect_dir=off"
>>>
>>
>> Zhangyi,
>>
>> One thing that has occurred to me now, which should be fixed before
>> first release
>> is that fsck.overlay should return an error for unknown/unsupported options
>> (e.g. fsck.overlay -o blah doesn't complain).
>> If this doesn't happen for first release then in the future we will
>> not be able to
>> write test scripts to check if installed fsck.overlay version supports
>> feature X.
>>
> Yes, It's time to impliment the feature/option set and do the corresponding
> work for the already known features. I am writing the kernel side feature set
> recent days, I will add these feature into fsck.overlay after kernel's work done.
>
> BTW, the feature set in kernel do the following, some suggestions?
> 1) Add feature set into upper root xattr "trusted.overlay.feature_[compat|imcompat|rocompat]"
>    if user give mount options "xxx=on" or some feature xattr/dir detected.
> 2) If unknown imcompat feature in xattr detected, fall back to try to ro-mount.

fail mount for unknown incompat

> 3) If unknown ro-compat featrure in xattr detected, mount fail.

fallback to ro-mount (see ext4_feature_set_ok())

>
>> About the possible meaning of <feature>=on/off and the meaning of <defaults>
>> in the context of fsck.overlay, I think it is worth a separate
>> discussion, but the
>> way I see it:
>> - When fsck.overlay *supports* a <feature> it should be able to detect if that
>>   <feature> was ever enabled on overlay (e.g. known xattr exists or
>> index dir exists).
>
> Agree, and we can also fix feature set when detect feature.
>
>> - When fsck.overlay detects an unknown <feature> it should abort
>
> Agree. A little more detail: if user specify upper and work dir, check imcompat
> unknown feature; if not, check ro-imcompat unknown feature. If detected, fsck
> about.
>

No exactly. The difference between ext4 kernel driver and e2fsck -
ext4 *allows* to mount with unknown compat features
and *allows* to mount ro with unknonw ro-compat features.
e2fsck *aborts* for any unknown compat/rocompat/incomapt feature

Meaning that you MAY be able to mount new ext4 with old kernel,
but you CANNOT fsck new ext4 with old e2fsck

>> - -o <feature>=off means wipe all traces of <feature> from overlay and fix if
>>   necessary (e.g. make a copy of redirect dir before removing
>> redirect_dir xattr)
>
> Agree. But not sure it is easy to implement of making a copy of redirect dir
> when redirect_dir=off. Because it modify the directory hierarchy, it may affect
> the origin/nlink xattr in files/dirs in index dir when index=on or nfs_export=on,
> maybe also affect some other feature future.
>

This is why fsck MUST refuse to run if it detects an unknown feature, even
if that feature is "compatible" for rw mounting, fixing may break it.

And about disabling redirect_dir, yes, it means that all renamed directories
are replaced with pure opaque (not indexed) directories and all the content
from merged dir needs to be copied up recursively to the new directory.
This is not trivial and may be time consuming, but with underlying fs clone
support it is also not that bad.
In fact, if you try to 'mv dir newdir' on overlay without redirect_dir, the 'mv'
tool already does that fallback to create new dir and recursive rename.

>> - -o <feature>=on means bring overlay to a "feature consistent state",
>> as if feature
>>   was enabled from day 1 (e.g. index all copies up lower hardlinks)
>
> Agree.
>
>> - If <feature>=[on/off] is not specified, then if <feature> is not
>> detected, it should
>>   not be explicitly enabled and if feature is detected it should be
>> made consistent
>
> Agree.
>
>> That last rule is certainly debatable, so maybe best if we leave that
>> decision to
>> admin via default policy configuration file.
>>
>
> Something like /etc/mke2fs.conf but not for mkfs? Do you mean if feature
> detected conflict with this config(eg: we detect redirect dir but "redirect_dir=off"
> was setted in ths config file), fsck should check and fix fs according to this
> config file?
>
> I think it may confusion and conflict with mount options(eg: redirect_dir=on)
> previous mount or next mount. And IIUC, there is no such feature config file for
> ext4/xfs, fsck.ext4 will detect feature from super block and fix fs according to
> this detected result. So I think it's better to detect feature feature by fsck
> itself, keep features as it was.
>

I agree it is best to not ask user anything because user probably doesn't know
the answer anyway. If you follow the discussion that has started on adding a
config file for mkfs.xfs, you'll see it is mainly intended to be used
by distributions.
Imagine a disro that set the kernel config OVERLAY_FS_INDEX with the
intention that all overlays will be indexed. This distro may want to make the
default behavior of fsck.overlay to construct an index by default.

For you, the role of a config file is to reduce "bike shedding" - if there are
conflicting options about what should be done in case feature X is detected
or not detected and option X=on/off is not specified, instead of
debating what the
right thing to do by default, you delegate the decision to "config
file" and set the
hard coded default to what *you* think is the best behavior.

So my advise to you is to add a config file if and only if (or when) it
becomes beneficial to you for reconciling different opinions.

Cheers,
Amir.

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

[zhangyi (F)]

On 2018/3/12 17:19, Amir Goldstein Wrote:
> On Mon, Mar 12, 2018 at 10:44 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> On 2018/3/11 16:12, Amir Goldstein Wrote:
>>> On Thu, Mar 1, 2018 at 3:11 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>> On 2018/3/1 21:03, Amir Goldstein Wrote:
>>>>> On Thu, Mar 1, 2018 at 2:13 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>>>> Hook filesystem check helper to _check_test_fs and _check_scratch_fs
>>>>>> for checking consistency of underlying dirs of overlay filesystem.
>>>>>> These helpers works only if fsck.overlay exists.
>>>>>>
>>>>>> This patch introduce OVERLAY_FSCK_OPTIONS use for check overlayfs like
>>>>>> OVERLAY_MOUNT_OPTIONS, and also introduce a mount point check helper in
>>>>>> common/rc to detect a dir is a mount point or not.
>>>>>>
>>>>>> [ _check_test_fs/_check_scratch_fs part picked from Amir's patch, thanks ]
>>>>>>
>>>>>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>>>>>> ---
>>>>>>  README.overlay | 10 ++++--
>>>>>>  common/config  |  4 +++
>>>>>>  common/overlay | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  common/rc      | 18 +++++++++--
>>>>>>  4 files changed, 127 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/README.overlay b/README.overlay
>>>>>> index dfb8234..3cbefab 100644
>>>>>> --- a/README.overlay
>>>>>> +++ b/README.overlay
>>>>>> @@ -22,6 +22,10 @@ the base fs should be pre-formatted before starting the -overlay run.
>>>>>>  An easy way to accomplish this is by running './check <some test>' once,
>>>>>>  before running './check -overlay'.
>>>>>>
>>>>>> +'./check -overlay' support check overlay test and scratch dirs,
>>>>>> +OVERLAY_FSCK_OPTIONS should be set instead of FSCK_OPTIONS if fsck
>>>>>> +options need to given directly.
>>>>>> +
>>>>>>  Because of the lack of mkfs support, multi-section config files are only
>>>>>>  partly supported with './check -overlay'. Only multi-section files that
>>>>>>  do not change FSTYP and MKFS_OPTIONS can be safely used with -overlay.
>>>>>> @@ -40,7 +44,9 @@ run overlay tests on the same base fs, but with different mount options:
>>>>>>   MOUNT_OPTIONS="-o pquota"
>>>>>>   TEST_FS_MOUNT_OPTS="-o noatime"
>>>>>>   OVERLAY_MOUNT_OPTIONS="-o redirect_dir=off"
>>>>>> + OVERLAY_FSCK_OPTIONS="-n redirect_dir=off"
>>>>>
>>>>> typo for the phony options. I believe it is going to be:  "-n -o
>>>>> redirect_dir=off"
>>>>
>>>> Oh, sorry for the mistake, it is "-n -o redirect_dir=off"
>>>>
>>>
>>> Zhangyi,
>>>
>>> One thing that has occurred to me now, which should be fixed before
>>> first release
>>> is that fsck.overlay should return an error for unknown/unsupported options
>>> (e.g. fsck.overlay -o blah doesn't complain).
>>> If this doesn't happen for first release then in the future we will
>>> not be able to
>>> write test scripts to check if installed fsck.overlay version supports
>>> feature X.
>>>
>> Yes, It's time to impliment the feature/option set and do the corresponding
>> work for the already known features. I am writing the kernel side feature set
>> recent days, I will add these feature into fsck.overlay after kernel's work done.
>>
>> BTW, the feature set in kernel do the following, some suggestions?
>> 1) Add feature set into upper root xattr "trusted.overlay.feature_[compat|imcompat|rocompat]"
>>    if user give mount options "xxx=on" or some feature xattr/dir detected.
>> 2) If unknown imcompat feature in xattr detected, fall back to try to ro-mount.
> 
> fail mount for unknown incompat
> 
>> 3) If unknown ro-compat featrure in xattr detected, mount fail.
> 
> fallback to ro-mount (see ext4_feature_set_ok())
> 

You are right, I get it.

>>
>>> About the possible meaning of <feature>=on/off and the meaning of <defaults>
>>> in the context of fsck.overlay, I think it is worth a separate
>>> discussion, but the
>>> way I see it:
>>> - When fsck.overlay *supports* a <feature> it should be able to detect if that
>>>   <feature> was ever enabled on overlay (e.g. known xattr exists or
>>> index dir exists).
>>
>> Agree, and we can also fix feature set when detect feature.
>>
>>> - When fsck.overlay detects an unknown <feature> it should abort
>>
>> Agree. A little more detail: if user specify upper and work dir, check imcompat
>> unknown feature; if not, check ro-imcompat unknown feature. If detected, fsck
>> about.
>>
> 
> No exactly. The difference between ext4 kernel driver and e2fsck -
> ext4 *allows* to mount with unknown compat features
> and *allows* to mount ro with unknonw ro-compat features.
> e2fsck *aborts* for any unknown compat/rocompat/incomapt feature
> 
> Meaning that you MAY be able to mount new ext4 with old kernel,
> but you CANNOT fsck new ext4 with old e2fsck
> 

Yes, you are right, I see this check in e2fsck/unix.c.

>>> - -o <feature>=off means wipe all traces of <feature> from overlay and fix if
>>>   necessary (e.g. make a copy of redirect dir before removing
>>> redirect_dir xattr)
>>
>> Agree. But not sure it is easy to implement of making a copy of redirect dir
>> when redirect_dir=off. Because it modify the directory hierarchy, it may affect
>> the origin/nlink xattr in files/dirs in index dir when index=on or nfs_export=on,
>> maybe also affect some other feature future.
>>
> 
> This is why fsck MUST refuse to run if it detects an unknown feature, even
> if that feature is "compatible" for rw mounting, fixing may break it.
> 

Right.

> And about disabling redirect_dir, yes, it means that all renamed directories
> are replaced with pure opaque (not indexed) directories and all the content
> from merged dir needs to be copied up recursively to the new directory.
> This is not trivial and may be time consuming, but with underlying fs clone
> support it is also not that bad.
> In fact, if you try to 'mv dir newdir' on overlay without redirect_dir, the 'mv'
> tool already does that fallback to create new dir and recursive rename.
> 

Yes, we can do the same thing like "mv" does when overlayfs doesn't support
rename syscall for dir(redirect_dir=off).

>>> - -o <feature>=on means bring overlay to a "feature consistent state",
>>> as if feature
>>>   was enabled from day 1 (e.g. index all copies up lower hardlinks)
>>
>> Agree.
>>
>>> - If <feature>=[on/off] is not specified, then if <feature> is not
>>> detected, it should
>>>   not be explicitly enabled and if feature is detected it should be
>>> made consistent
>>
>> Agree.
>>
>>> That last rule is certainly debatable, so maybe best if we leave that
>>> decision to
>>> admin via default policy configuration file.
>>>
>>
>> Something like /etc/mke2fs.conf but not for mkfs? Do you mean if feature
>> detected conflict with this config(eg: we detect redirect dir but "redirect_dir=off"
>> was setted in ths config file), fsck should check and fix fs according to this
>> config file?
>>
>> I think it may confusion and conflict with mount options(eg: redirect_dir=on)
>> previous mount or next mount. And IIUC, there is no such feature config file for
>> ext4/xfs, fsck.ext4 will detect feature from super block and fix fs according to
>> this detected result. So I think it's better to detect feature feature by fsck
>> itself, keep features as it was.
>>
> 
> I agree it is best to not ask user anything because user probably doesn't know
> the answer anyway. If you follow the discussion that has started on adding a
> config file for mkfs.xfs, you'll see it is mainly intended to be used
> by distributions.
> Imagine a disro that set the kernel config OVERLAY_FS_INDEX with the
> intention that all overlays will be indexed. This distro may want to make the
> default behavior of fsck.overlay to construct an index by default.
> 
> For you, the role of a config file is to reduce "bike shedding" - if there are
> conflicting options about what should be done in case feature X is detected
> or not detected and option X=on/off is not specified, instead of
> debating what the
> right thing to do by default, you delegate the decision to "config
> file" and set the
> hard coded default to what *you* think is the best behavior.
> 
> So my advise to you is to add a config file if and only if (or when) it
> becomes beneficial to you for reconciling different opinions.
> 

Yes, I get your point, It's a good point to avoid debate, and seems no
apparent defects. I'd like to add this file if no different opinions.

Thanks,
Yi.

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

[Eryu Guan]

On Thu, Mar 01, 2018 at 03:03:55PM +0200, Amir Goldstein wrote:
> On Thu, Mar 1, 2018 at 2:13 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> > Hook filesystem check helper to _check_test_fs and _check_scratch_fs
> > for checking consistency of underlying dirs of overlay filesystem.
> > These helpers works only if fsck.overlay exists.
> >
> > This patch introduce OVERLAY_FSCK_OPTIONS use for check overlayfs like
> > OVERLAY_MOUNT_OPTIONS, and also introduce a mount point check helper in
> > common/rc to detect a dir is a mount point or not.
> >
> > [ _check_test_fs/_check_scratch_fs part picked from Amir's patch, thanks ]
> >
> > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> > ---
> >  README.overlay | 10 ++++--
> >  common/config  |  4 +++
> >  common/overlay | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  common/rc      | 18 +++++++++--
> >  4 files changed, 127 insertions(+), 4 deletions(-)
> >
> > diff --git a/README.overlay b/README.overlay
> > index dfb8234..3cbefab 100644
> > --- a/README.overlay
> > +++ b/README.overlay
> > @@ -22,6 +22,10 @@ the base fs should be pre-formatted before starting the -overlay run.
> >  An easy way to accomplish this is by running './check <some test>' once,
> >  before running './check -overlay'.
> >
> > +'./check -overlay' support check overlay test and scratch dirs,
> > +OVERLAY_FSCK_OPTIONS should be set instead of FSCK_OPTIONS if fsck
> > +options need to given directly.
> > +
> >  Because of the lack of mkfs support, multi-section config files are only
> >  partly supported with './check -overlay'. Only multi-section files that
> >  do not change FSTYP and MKFS_OPTIONS can be safely used with -overlay.
> > @@ -40,7 +44,9 @@ run overlay tests on the same base fs, but with different mount options:
> >   MOUNT_OPTIONS="-o pquota"
> >   TEST_FS_MOUNT_OPTS="-o noatime"
> >   OVERLAY_MOUNT_OPTIONS="-o redirect_dir=off"
> > + OVERLAY_FSCK_OPTIONS="-n redirect_dir=off"
> 
> typo for the phony options. I believe it is going to be:  "-n -o
> redirect_dir=off"

I can fix it on commit. Thanks for review!

Eryu

[xfstests PATCH v5 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 v5 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 +-
 tests/overlay/049 | 2 +-
 3 files changed, 3 insertions(+), 3 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
diff --git a/tests/overlay/049 b/tests/overlay/049
index ef00138..1dafc46 100755
--- a/tests/overlay/049
+++ b/tests/overlay/049
@@ -72,7 +72,7 @@ rm -f $seqres.full
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+_require_scratch_nocheck
 _require_scratch_feature redirect_dir
 
 # remove all files from previous runs
-- 
2.5.0

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

[zhangyi (F)]

Tests that use _overlay_scratch_mount_dirs instead of _scratch_mount
should use _require_scratch_nocheck instead of _require_scratch
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. So we
introduce _overlay_check_scratch_dirs helper and should call this
helper with the correct dir arguments for these non-default cases.

This patch modify these tests to optionally call
_overlay_check_scratch_dirs at the end of the test or after
_scratch_umount to mount base filesystem only and run the checker.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 common/overlay    | 29 +++++++++++++++++++++++++++++
 tests/overlay/005 |  7 ++++++-
 tests/overlay/010 |  7 ++++++-
 tests/overlay/014 | 10 +++++++++-
 tests/overlay/035 |  7 ++++++-
 tests/overlay/036 |  6 +++++-
 tests/overlay/037 |  7 ++++++-
 tests/overlay/038 | 10 +++++++++-
 tests/overlay/041 | 10 +++++++++-
 tests/overlay/043 |  7 ++++++-
 tests/overlay/044 |  8 +++++++-
 tests/overlay/051 | 10 ++++++++--
 tests/overlay/053 | 10 ++++++++--
 tests/overlay/055 | 10 ++++++++--
 14 files changed, 122 insertions(+), 16 deletions(-)

diff --git a/common/overlay b/common/overlay
index 5d01e5e..441827b 100644
--- a/common/overlay
+++ b/common/overlay
@@ -219,6 +219,35 @@ _overlay_check_dirs()
 	return $err
 }
 
+# Check the same mnt/dev of _check_overlay_scratch_fs but non-default
+# underlying scratch dirs of overlayfs, it needs lower/upper/work dirs
+# provided as arguments, and it's useful for non-default setups such
+# as multiple lower layers
+_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_dir_mountpoint $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()
 {
 	# The first arguments is overlay mount point use for checking
diff --git a/tests/overlay/005 b/tests/overlay/005
index 17992a3..87fc9bb 100755
--- a/tests/overlay/005
+++ b/tests/overlay/005
@@ -54,7 +54,9 @@ rm -f $seqres.full
 # Modify as appropriate.
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+# Use non-default scratch underlying overlay dirs, we need to check
+# them explicity after test.
+_require_scratch_nocheck
 _require_loop
 
 # Remove all files from previous tests
@@ -102,6 +104,9 @@ $XFS_IO_PROG -f -c "o" ${SCRATCH_MNT}/test_file \
 # unmount overlayfs
 $UMOUNT_PROG $SCRATCH_MNT
 
+# check overlayfs
+_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..32af89c 100755
--- a/tests/overlay/010
+++ b/tests/overlay/010
@@ -48,7 +48,9 @@ rm -f $seqres.full
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+# Use non-default scratch underlying overlay dirs, we need to check
+# them explicity after test.
+_require_scratch_nocheck
 
 # Remove all files from previous tests
 _scratch_mkfs
@@ -70,6 +72,9 @@ mknod $lowerdir2/testdir/a c 0 0
 _overlay_scratch_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
 rm -rf $SCRATCH_MNT/testdir
 
+# check overlayfs
+_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..67fad9f 100755
--- a/tests/overlay/014
+++ b/tests/overlay/014
@@ -53,7 +53,9 @@ rm -f $seqres.full
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+# Use non-default scratch underlying overlay dirs, we need to check
+# them explicity after test.
+_require_scratch_nocheck
 
 # Remove all files from previous tests
 _scratch_mkfs
@@ -78,6 +80,9 @@ mkdir -p $SCRATCH_MNT/testdir/visibledir
 # unmount overlayfs but not base fs
 $UMOUNT_PROG $SCRATCH_MNT
 
+# check overlayfs
+_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 +95,9 @@ $UMOUNT_PROG $SCRATCH_MNT
 _overlay_scratch_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
 ls $SCRATCH_MNT/testdir
 
+# check overlayfs
+_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..d1b2c19 100755
--- a/tests/overlay/035
+++ b/tests/overlay/035
@@ -51,7 +51,9 @@ rm -f $seqres.full
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+# Use non-default scratch underlying overlay dirs, we need to check
+# them explicity after test.
+_require_scratch_nocheck
 _require_chattr i
 
 # Remove all files from previous tests
@@ -81,6 +83,9 @@ _overlay_scratch_mount_dirs $lowerdir2 $upperdir $workdir
 touch $SCRATCH_MNT/bar 2>&1 | _filter_scratch
 _scratch_remount rw 2>&1 | _filter_ro_mount
 
+# check overlayfs
+_overlay_check_scratch_dirs $lowerdir2 $upperdir $workdir
+
 # success, all done
 status=0
 exit
diff --git a/tests/overlay/036 b/tests/overlay/036
index e04aaee..8d3ccf4 100755
--- a/tests/overlay/036
+++ b/tests/overlay/036
@@ -69,7 +69,9 @@ rm -f $seqres.full
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+# Use non-default scratch underlying overlay dirs, we need to check
+# them explicity after test.
+_require_scratch_nocheck
 _require_scratch_feature index
 
 # Remove all files from previous tests
@@ -110,6 +112,8 @@ _overlay_mount_dirs $lowerdir $upperdir $workdir2 \
 _overlay_mount_dirs $lowerdir2 $upperdir2 $workdir \
 	    overlay3 $SCRATCH_MNT -oindex=on 2>&1 | _filter_busy_mount
 
+# check overlayfs
+_overlay_check_scratch_dirs $lowerdir $upperdir $workdir
 
 # success, all done
 status=0
diff --git a/tests/overlay/037 b/tests/overlay/037
index 6710dda..4e2e5df 100755
--- a/tests/overlay/037
+++ b/tests/overlay/037
@@ -55,7 +55,9 @@ rm -f $seqres.full
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+# Use non-default scratch underlying overlay dirs, we need to check
+# them explicity after test.
+_require_scratch_nocheck
 _require_scratch_feature index
 
 # Remove all files from previous tests
@@ -87,6 +89,9 @@ $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
 
+# check overlayfs
+_overlay_check_scratch_dirs $lowerdir $upperdir $workdir -oindex=on
+
 # success, all done
 status=0
 exit
diff --git a/tests/overlay/038 b/tests/overlay/038
index bd87156..972a16e 100755
--- a/tests/overlay/038
+++ b/tests/overlay/038
@@ -46,7 +46,9 @@ _cleanup()
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+# Use non-default scratch underlying overlay dirs, we need to check
+# them explicity after test.
+_require_scratch_nocheck
 _require_attrs
 _require_test_program "t_dir_type"
 
@@ -163,6 +165,9 @@ subdir_d=$($here/src/t_dir_type $pure_lower_dir $pure_lower_subdir_st_ino)
 
 _scratch_unmount
 
+# check overlayfs
+_check_scratch_fs
+
 # Verify pure lower residing in dir which has another lower layer
 _scratch_mkfs
 
@@ -202,6 +207,9 @@ 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"
 
+# check overlayfs
+_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 11efacb..4e72348 100755
--- a/tests/overlay/041
+++ b/tests/overlay/041
@@ -48,7 +48,9 @@ _cleanup()
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+# Use non-default scratch underlying overlay dirs, we need to check
+# them explicity after test.
+_require_scratch_nocheck
 _require_test
 _require_attrs
 _require_test_program "t_dir_type"
@@ -168,6 +170,9 @@ subdir_d=$($here/src/t_dir_type $pure_lower_dir $pure_lower_subdir_st_ino)
 
 _scratch_unmount
 
+# check overlayfs
+_overlay_check_scratch_dirs $lowerdir $upperdir $workdir
+
 # Verify pure lower residing in dir which has another lower layer
 middir=$OVL_BASE_TEST_DIR/$seq-ovl-mid
 lowerdir=$OVL_BASE_TEST_DIR/$seq-ovl-lower
@@ -206,6 +211,9 @@ 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"
 
+# check overlayfs
+_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 699c4e1..46df686 100755
--- a/tests/overlay/043
+++ b/tests/overlay/043
@@ -56,7 +56,9 @@ _cleanup()
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+# Use non-default scratch underlying overlay dirs, we need to check
+# them explicity after test.
+_require_scratch_nocheck
 _require_test
 _require_test_program "af_unix"
 _require_test_program "t_dir_type"
@@ -153,6 +155,9 @@ _overlay_scratch_mount_dirs $lowerdir $upperdir $workdir
 # Compare inode numbers before/after mount cycle
 check_inode_numbers $testdir $tmp.after_move $tmp.after_cycle
 
+# check overlayfs
+_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 e57f6f7..2ab3035 100755
--- a/tests/overlay/044
+++ b/tests/overlay/044
@@ -49,7 +49,9 @@ _cleanup()
 # real QA test starts here
 _supported_fs overlay
 _supported_os Linux
-_require_scratch
+# Use non-default scratch underlying overlay dirs, we need to check
+# them explicity after test.
+_require_scratch_nocheck
 _require_test
 _require_scratch_feature index
 _require_test_program "t_dir_type"
@@ -122,6 +124,7 @@ check_ino_nlink $SCRATCH_MNT $tmp.before $tmp.after_one
 
 # Verify that the hardlinks survive a mount cycle
 $UMOUNT_PROG $SCRATCH_MNT
+_overlay_check_scratch_dirs $lowerdir $upperdir $workdir -o index=on
 _overlay_scratch_mount_dirs $lowerdir $upperdir $workdir -o index=on
 
 echo "== After mount cycle =="
@@ -138,5 +141,8 @@ echo "== After write two =="
 cat $FILES
 check_ino_nlink $SCRATCH_MNT $tmp.after_one $tmp.after_two
 
+# check overlayfs
+_overlay_check_scratch_dirs $lowerdir $upperdir $workdir -o index=on
+
 status=0
 exit
diff --git a/tests/overlay/051 b/tests/overlay/051
index ae7844d..04bd133 100755
--- a/tests/overlay/051
+++ b/tests/overlay/051
@@ -61,8 +61,10 @@ _cleanup()
 _supported_fs overlay
 _supported_os Linux
 _require_test
-_require_scratch
 _require_test_program "open_by_handle"
+# Use non-default scratch underlying overlay dirs, we need to check
+# them explicity after test.
+_require_scratch_nocheck
 # We need to require both features together, because nfs_export cannot
 # be enabled when index is disabled
 _require_scratch_overlay_features index nfs_export
@@ -122,10 +124,14 @@ mount_dirs()
 				-o "index=on,nfs_export=on"
 }
 
-# Unmount the overlay without unmounting base fs
+# Unmount the overlay without unmounting base fs and check the
+# underlying dirs
 unmount_dirs()
 {
 	$UMOUNT_PROG $SCRATCH_MNT
+
+	_overlay_check_scratch_dirs $middle:$lower $upper $work \
+				-o "index=on,nfs_export=on"
 }
 
 # Check non-stale file handles of lower/upper files and verify
diff --git a/tests/overlay/053 b/tests/overlay/053
index d4fdcde..d766cde 100755
--- a/tests/overlay/053
+++ b/tests/overlay/053
@@ -63,8 +63,10 @@ _cleanup()
 _supported_fs overlay
 _supported_os Linux
 _require_test
-_require_scratch
 _require_test_program "open_by_handle"
+# Use non-default scratch underlying overlay dirs, we need to check
+# them explicity after test.
+_require_scratch_nocheck
 # We need to require all features together, because nfs_export cannot
 # be enabled when index is disabled
 _require_scratch_overlay_features index nfs_export redirect_dir
@@ -118,10 +120,14 @@ mount_dirs()
 				-o "index=on,nfs_export=on,redirect_dir=on"
 }
 
-# Unmount the overlay without unmounting base fs
+# Unmount the overlay without unmounting base fs and check the
+# underlying dirs
 unmount_dirs()
 {
 	$UMOUNT_PROG $SCRATCH_MNT
+
+	_overlay_check_scratch_dirs $middle:$lower $upper $work \
+				-o "index=on,nfs_export=on,redirect_dir=on"
 }
 
 # Check non-stale file handles of lower/upper moved files
diff --git a/tests/overlay/055 b/tests/overlay/055
index 70fe6ac..02da6ef 100755
--- a/tests/overlay/055
+++ b/tests/overlay/055
@@ -67,8 +67,10 @@ _cleanup()
 _supported_fs overlay
 _supported_os Linux
 _require_test
-_require_scratch
 _require_test_program "open_by_handle"
+# Use non-default scratch underlying overlay dirs, we need to check
+# them explicity after test.
+_require_scratch_nocheck
 # We need to require all features together, because nfs_export cannot
 # be enabled when index is disabled
 _require_scratch_overlay_features index nfs_export redirect_dir
@@ -122,10 +124,14 @@ mount_dirs()
 				-o "index=on,nfs_export=on,redirect_dir=on"
 }
 
-# Unmount the overlay without unmounting base fs
+# Unmount the overlay without unmounting base fs and check the
+# underlying dirs
 unmount_dirs()
 {
 	$UMOUNT_PROG $SCRATCH_MNT
+
+	_overlay_check_scratch_dirs $middle:$lower $upper $work \
+				-o "index=on,nfs_export=on,redirect_dir=on"
 }
 
 # Check file handles of dir with ancestor under lower redirect
-- 
2.5.0

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

[Amir Goldstein]

On Thu, Mar 1, 2018 at 2:13 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> Tests that use _overlay_scratch_mount_dirs instead of _scratch_mount
> should use _require_scratch_nocheck instead of _require_scratch
> 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. So we
> introduce _overlay_check_scratch_dirs helper and should call this
> helper with the correct dir arguments for these non-default cases.
>
> This patch modify these tests to optionally call
> _overlay_check_scratch_dirs at the end of the test or after
> _scratch_umount to mount base filesystem only and run the checker.
>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Looks ok to me.

> ---
>  common/overlay    | 29 +++++++++++++++++++++++++++++
>  tests/overlay/005 |  7 ++++++-
>  tests/overlay/010 |  7 ++++++-
>  tests/overlay/014 | 10 +++++++++-
>  tests/overlay/035 |  7 ++++++-
>  tests/overlay/036 |  6 +++++-
>  tests/overlay/037 |  7 ++++++-
>  tests/overlay/038 | 10 +++++++++-
>  tests/overlay/041 | 10 +++++++++-
>  tests/overlay/043 |  7 ++++++-
>  tests/overlay/044 |  8 +++++++-
>  tests/overlay/051 | 10 ++++++++--
>  tests/overlay/053 | 10 ++++++++--
>  tests/overlay/055 | 10 ++++++++--
>  14 files changed, 122 insertions(+), 16 deletions(-)
>
> diff --git a/common/overlay b/common/overlay
> index 5d01e5e..441827b 100644
> --- a/common/overlay
> +++ b/common/overlay
> @@ -219,6 +219,35 @@ _overlay_check_dirs()
>         return $err
>  }
>
> +# Check the same mnt/dev of _check_overlay_scratch_fs but non-default
> +# underlying scratch dirs of overlayfs, it needs lower/upper/work dirs
> +# provided as arguments, and it's useful for non-default setups such
> +# as multiple lower layers
> +_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_dir_mountpoint $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()
>  {
>         # The first arguments is overlay mount point use for checking
> diff --git a/tests/overlay/005 b/tests/overlay/005
> index 17992a3..87fc9bb 100755
> --- a/tests/overlay/005
> +++ b/tests/overlay/005
> @@ -54,7 +54,9 @@ rm -f $seqres.full
>  # Modify as appropriate.
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +# Use non-default scratch underlying overlay dirs, we need to check
> +# them explicity after test.
> +_require_scratch_nocheck
>  _require_loop
>
>  # Remove all files from previous tests
> @@ -102,6 +104,9 @@ $XFS_IO_PROG -f -c "o" ${SCRATCH_MNT}/test_file \
>  # unmount overlayfs
>  $UMOUNT_PROG $SCRATCH_MNT
>
> +# check overlayfs
> +_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..32af89c 100755
> --- a/tests/overlay/010
> +++ b/tests/overlay/010
> @@ -48,7 +48,9 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +# Use non-default scratch underlying overlay dirs, we need to check
> +# them explicity after test.
> +_require_scratch_nocheck
>
>  # Remove all files from previous tests
>  _scratch_mkfs
> @@ -70,6 +72,9 @@ mknod $lowerdir2/testdir/a c 0 0
>  _overlay_scratch_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
>  rm -rf $SCRATCH_MNT/testdir
>
> +# check overlayfs
> +_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..67fad9f 100755
> --- a/tests/overlay/014
> +++ b/tests/overlay/014
> @@ -53,7 +53,9 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +# Use non-default scratch underlying overlay dirs, we need to check
> +# them explicity after test.
> +_require_scratch_nocheck
>
>  # Remove all files from previous tests
>  _scratch_mkfs
> @@ -78,6 +80,9 @@ mkdir -p $SCRATCH_MNT/testdir/visibledir
>  # unmount overlayfs but not base fs
>  $UMOUNT_PROG $SCRATCH_MNT
>
> +# check overlayfs
> +_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 +95,9 @@ $UMOUNT_PROG $SCRATCH_MNT
>  _overlay_scratch_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
>  ls $SCRATCH_MNT/testdir
>
> +# check overlayfs
> +_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..d1b2c19 100755
> --- a/tests/overlay/035
> +++ b/tests/overlay/035
> @@ -51,7 +51,9 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +# Use non-default scratch underlying overlay dirs, we need to check
> +# them explicity after test.
> +_require_scratch_nocheck
>  _require_chattr i
>
>  # Remove all files from previous tests
> @@ -81,6 +83,9 @@ _overlay_scratch_mount_dirs $lowerdir2 $upperdir $workdir
>  touch $SCRATCH_MNT/bar 2>&1 | _filter_scratch
>  _scratch_remount rw 2>&1 | _filter_ro_mount
>
> +# check overlayfs
> +_overlay_check_scratch_dirs $lowerdir2 $upperdir $workdir
> +
>  # success, all done
>  status=0
>  exit
> diff --git a/tests/overlay/036 b/tests/overlay/036
> index e04aaee..8d3ccf4 100755
> --- a/tests/overlay/036
> +++ b/tests/overlay/036
> @@ -69,7 +69,9 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +# Use non-default scratch underlying overlay dirs, we need to check
> +# them explicity after test.
> +_require_scratch_nocheck
>  _require_scratch_feature index
>
>  # Remove all files from previous tests
> @@ -110,6 +112,8 @@ _overlay_mount_dirs $lowerdir $upperdir $workdir2 \
>  _overlay_mount_dirs $lowerdir2 $upperdir2 $workdir \
>             overlay3 $SCRATCH_MNT -oindex=on 2>&1 | _filter_busy_mount
>
> +# check overlayfs
> +_overlay_check_scratch_dirs $lowerdir $upperdir $workdir
>
>  # success, all done
>  status=0
> diff --git a/tests/overlay/037 b/tests/overlay/037
> index 6710dda..4e2e5df 100755
> --- a/tests/overlay/037
> +++ b/tests/overlay/037
> @@ -55,7 +55,9 @@ rm -f $seqres.full
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +# Use non-default scratch underlying overlay dirs, we need to check
> +# them explicity after test.
> +_require_scratch_nocheck
>  _require_scratch_feature index
>
>  # Remove all files from previous tests
> @@ -87,6 +89,9 @@ $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
>
> +# check overlayfs
> +_overlay_check_scratch_dirs $lowerdir $upperdir $workdir -oindex=on
> +
>  # success, all done
>  status=0
>  exit
> diff --git a/tests/overlay/038 b/tests/overlay/038
> index bd87156..972a16e 100755
> --- a/tests/overlay/038
> +++ b/tests/overlay/038
> @@ -46,7 +46,9 @@ _cleanup()
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +# Use non-default scratch underlying overlay dirs, we need to check
> +# them explicity after test.
> +_require_scratch_nocheck
>  _require_attrs
>  _require_test_program "t_dir_type"
>
> @@ -163,6 +165,9 @@ subdir_d=$($here/src/t_dir_type $pure_lower_dir $pure_lower_subdir_st_ino)
>
>  _scratch_unmount
>
> +# check overlayfs
> +_check_scratch_fs
> +
>  # Verify pure lower residing in dir which has another lower layer
>  _scratch_mkfs
>
> @@ -202,6 +207,9 @@ 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"
>
> +# check overlayfs
> +_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 11efacb..4e72348 100755
> --- a/tests/overlay/041
> +++ b/tests/overlay/041
> @@ -48,7 +48,9 @@ _cleanup()
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +# Use non-default scratch underlying overlay dirs, we need to check
> +# them explicity after test.
> +_require_scratch_nocheck
>  _require_test
>  _require_attrs
>  _require_test_program "t_dir_type"
> @@ -168,6 +170,9 @@ subdir_d=$($here/src/t_dir_type $pure_lower_dir $pure_lower_subdir_st_ino)
>
>  _scratch_unmount
>
> +# check overlayfs
> +_overlay_check_scratch_dirs $lowerdir $upperdir $workdir
> +
>  # Verify pure lower residing in dir which has another lower layer
>  middir=$OVL_BASE_TEST_DIR/$seq-ovl-mid
>  lowerdir=$OVL_BASE_TEST_DIR/$seq-ovl-lower
> @@ -206,6 +211,9 @@ 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"
>
> +# check overlayfs
> +_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 699c4e1..46df686 100755
> --- a/tests/overlay/043
> +++ b/tests/overlay/043
> @@ -56,7 +56,9 @@ _cleanup()
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +# Use non-default scratch underlying overlay dirs, we need to check
> +# them explicity after test.
> +_require_scratch_nocheck
>  _require_test
>  _require_test_program "af_unix"
>  _require_test_program "t_dir_type"
> @@ -153,6 +155,9 @@ _overlay_scratch_mount_dirs $lowerdir $upperdir $workdir
>  # Compare inode numbers before/after mount cycle
>  check_inode_numbers $testdir $tmp.after_move $tmp.after_cycle
>
> +# check overlayfs
> +_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 e57f6f7..2ab3035 100755
> --- a/tests/overlay/044
> +++ b/tests/overlay/044
> @@ -49,7 +49,9 @@ _cleanup()
>  # real QA test starts here
>  _supported_fs overlay
>  _supported_os Linux
> -_require_scratch
> +# Use non-default scratch underlying overlay dirs, we need to check
> +# them explicity after test.
> +_require_scratch_nocheck
>  _require_test
>  _require_scratch_feature index
>  _require_test_program "t_dir_type"
> @@ -122,6 +124,7 @@ check_ino_nlink $SCRATCH_MNT $tmp.before $tmp.after_one
>
>  # Verify that the hardlinks survive a mount cycle
>  $UMOUNT_PROG $SCRATCH_MNT
> +_overlay_check_scratch_dirs $lowerdir $upperdir $workdir -o index=on
>  _overlay_scratch_mount_dirs $lowerdir $upperdir $workdir -o index=on
>
>  echo "== After mount cycle =="
> @@ -138,5 +141,8 @@ echo "== After write two =="
>  cat $FILES
>  check_ino_nlink $SCRATCH_MNT $tmp.after_one $tmp.after_two
>
> +# check overlayfs
> +_overlay_check_scratch_dirs $lowerdir $upperdir $workdir -o index=on
> +
>  status=0
>  exit
> diff --git a/tests/overlay/051 b/tests/overlay/051
> index ae7844d..04bd133 100755
> --- a/tests/overlay/051
> +++ b/tests/overlay/051
> @@ -61,8 +61,10 @@ _cleanup()
>  _supported_fs overlay
>  _supported_os Linux
>  _require_test
> -_require_scratch
>  _require_test_program "open_by_handle"
> +# Use non-default scratch underlying overlay dirs, we need to check
> +# them explicity after test.
> +_require_scratch_nocheck
>  # We need to require both features together, because nfs_export cannot
>  # be enabled when index is disabled
>  _require_scratch_overlay_features index nfs_export
> @@ -122,10 +124,14 @@ mount_dirs()
>                                 -o "index=on,nfs_export=on"
>  }
>
> -# Unmount the overlay without unmounting base fs
> +# Unmount the overlay without unmounting base fs and check the
> +# underlying dirs
>  unmount_dirs()
>  {
>         $UMOUNT_PROG $SCRATCH_MNT
> +
> +       _overlay_check_scratch_dirs $middle:$lower $upper $work \
> +                               -o "index=on,nfs_export=on"
>  }
>
>  # Check non-stale file handles of lower/upper files and verify
> diff --git a/tests/overlay/053 b/tests/overlay/053
> index d4fdcde..d766cde 100755
> --- a/tests/overlay/053
> +++ b/tests/overlay/053
> @@ -63,8 +63,10 @@ _cleanup()
>  _supported_fs overlay
>  _supported_os Linux
>  _require_test
> -_require_scratch
>  _require_test_program "open_by_handle"
> +# Use non-default scratch underlying overlay dirs, we need to check
> +# them explicity after test.
> +_require_scratch_nocheck
>  # We need to require all features together, because nfs_export cannot
>  # be enabled when index is disabled
>  _require_scratch_overlay_features index nfs_export redirect_dir
> @@ -118,10 +120,14 @@ mount_dirs()
>                                 -o "index=on,nfs_export=on,redirect_dir=on"
>  }
>
> -# Unmount the overlay without unmounting base fs
> +# Unmount the overlay without unmounting base fs and check the
> +# underlying dirs
>  unmount_dirs()
>  {
>         $UMOUNT_PROG $SCRATCH_MNT
> +
> +       _overlay_check_scratch_dirs $middle:$lower $upper $work \
> +                               -o "index=on,nfs_export=on,redirect_dir=on"
>  }
>
>  # Check non-stale file handles of lower/upper moved files
> diff --git a/tests/overlay/055 b/tests/overlay/055
> index 70fe6ac..02da6ef 100755
> --- a/tests/overlay/055
> +++ b/tests/overlay/055
> @@ -67,8 +67,10 @@ _cleanup()
>  _supported_fs overlay
>  _supported_os Linux
>  _require_test
> -_require_scratch
>  _require_test_program "open_by_handle"
> +# Use non-default scratch underlying overlay dirs, we need to check
> +# them explicity after test.
> +_require_scratch_nocheck
>  # We need to require all features together, because nfs_export cannot
>  # be enabled when index is disabled
>  _require_scratch_overlay_features index nfs_export redirect_dir
> @@ -122,10 +124,14 @@ mount_dirs()
>                                 -o "index=on,nfs_export=on,redirect_dir=on"
>  }
>
> -# Unmount the overlay without unmounting base fs
> +# Unmount the overlay without unmounting base fs and check the
> +# underlying dirs
>  unmount_dirs()
>  {
>         $UMOUNT_PROG $SCRATCH_MNT
> +
> +       _overlay_check_scratch_dirs $middle:$lower $upper $work \
> +                               -o "index=on,nfs_export=on,redirect_dir=on"
>  }
>
>  # Check file handles of dir with ancestor under lower redirect
> --
> 2.5.0
>