[PATCH 0/4] fstests: fixes for config option OVERLAY_MOUNT_OPTIONS

[PATCH 0/4] fstests: fixes for config option OVERLAY_MOUNT_OPTIONS

[Amir Goldstein]

Eryu,

Since v4.13, overlayfs supports mount option index=on/off per mount
and kernel config option CONFIG_OVERLAY_FS_INDEX that sets the default
system wide value when mount option is not specified.

The behavior of overlayfs may differ for index=on/off, specifically
for hardlinks. It is therefore important to be able to test overlayfs
with both index=on/off.

To that end exists the OVERLAY_MOUNT_OPTIONS config option, only
when I tried to use it I found it was broken in more than one way.
This series fixed the use of OVERLAY_MOUNT_OPTIONS and refactors
some customized overlay mount helpers along the way.

NOTE for users of kvm-xfstests, this test-appliance change is also
required for invoking kvm-xfstests -m index=on/off
https://github.com/tytso/xfstests-bld/pull/4

Amir Goldstein (4):
  overlay: remove stale implementation of _scratch_mount_options
  overlay: use default overlay mount options _overlay_mount_dirs()
  overlay: create helper _overlay_scratch_mount_dirs()
  overlay: fix _overlay_config_override of MOUNT_OPTIONS

 common/config     |  4 ++++
 common/rc         | 36 ++++++++++++++++--------------------
 tests/overlay/005 |  2 +-
 tests/overlay/010 |  3 +--
 tests/overlay/014 |  8 +++-----
 tests/overlay/031 | 11 +++++------
 tests/overlay/035 |  3 +--
 tests/overlay/037 | 14 ++++++--------
 tests/overlay/038 |  3 +--
 9 files changed, 38 insertions(+), 46 deletions(-)

-- 
2.7.4

[PATCH 1/4] overlay: remove stale implementation of _scratch_mount_options

[Amir Goldstein]

_scratch_mount_options() was not implemented correctly for
overlayfs and wasn't used by any overlay tests.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 common/rc | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/common/rc b/common/rc
index 53bbb11..1a61549 100644
--- a/common/rc
+++ b/common/rc
@@ -300,26 +300,10 @@ _common_dev_mount_options()
 	echo $MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS $*
 }
 
-_overlay_basic_mount_options()
-{
-	echo "-o lowerdir=$1/$OVL_LOWER,upperdir=$1/$OVL_UPPER,workdir=$1/$OVL_WORK"
-}
-
-_overlay_mount_options()
-{
-	echo `_common_dev_mount_options` \
-	     `_overlay_basic_mount_options $1` \
-	     $OVERLAY_MOUNT_OPTIONS
-}
-
 _scratch_mount_options()
 {
 	_scratch_options mount
 
-	if [ "$FSTYP" == "overlay" ]; then
-		echo `_overlay_mount_options $OVL_BASE_SCRATCH_MNT`
-		return 0
-	fi
 	echo `_common_dev_mount_options $*` $SCRATCH_OPTIONS \
 					$SCRATCH_DEV $SCRATCH_MNT
 }
-- 
2.7.4

[PATCH 2/4] overlay: use default overlay mount options _overlay_mount_dirs()

[Amir Goldstein]

Tests that use _overlay_mount_dirs() should also use the
default overlay mount options.
Move mount options from overlay_mount() into _overlay_mount_dirs()
and use helper common_dev_mount_opts() to get options.

OVERLAY_MOUNT_OPTIONS is assigned to MOUNT_OPTIONS, so
there is no need to use OVERLAY_MOUNT_OPTIONS directly.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Use common_dev_mount_opts in _overlay_mount_dirs()
---
 common/rc | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/common/rc b/common/rc
index 1a61549..b30c4b9 100644
--- a/common/rc
+++ b/common/rc
@@ -342,7 +342,7 @@ _overlay_mount_dirs()
 	shift 3
 
 	$MOUNT_PROG -t overlay -o lowerdir=$lowerdir -o upperdir=$upperdir \
-		    -o workdir=$workdir $*
+		    -o workdir=$workdir `_common_dev_mount_options $*`
 }
 
 _overlay_mkdirs()
@@ -367,9 +367,8 @@ _overlay_mount()
 
 	_overlay_mkdirs $dir
 
-	_overlay_mount_dirs $dir/$OVL_LOWER $dir/$OVL_UPPER \
-			    $dir/$OVL_WORK $OVERLAY_MOUNT_OPTIONS \
-			    $SELINUX_MOUNT_OPTIONS $* $dir $mnt
+	_overlay_mount_dirs $dir/$OVL_LOWER $dir/$OVL_UPPER $dir/$OVL_WORK \
+				$* $dir $mnt
 }
 
 _overlay_base_test_mount()
-- 
2.7.4

[PATCH 3/4] overlay: create helper _overlay_scratch_mount_dirs()

[Amir Goldstein]

A helper to mount with same options/mnt/dev of scratch mount, but
optionally with different lower/upper/work dirs.
use instead of _overlay_mount_dirs() in all tests where applicable.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 common/rc         | 13 +++++++++++++
 tests/overlay/005 |  2 +-
 tests/overlay/010 |  3 +--
 tests/overlay/014 |  8 +++-----
 tests/overlay/031 | 11 +++++------
 tests/overlay/035 |  3 +--
 tests/overlay/037 | 14 ++++++--------
 tests/overlay/038 |  3 +--
 8 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/common/rc b/common/rc
index b30c4b9..54f81d3 100644
--- a/common/rc
+++ b/common/rc
@@ -345,6 +345,19 @@ _overlay_mount_dirs()
 		    -o workdir=$workdir `_common_dev_mount_options $*`
 }
 
+# Mount with same options/mnt/dev of scratch mount, but optionally
+# with different lower/upper/work dirs
+_overlay_scratch_mount_dirs()
+{
+	local lowerdir=$1
+	local upperdir=$2
+	local workdir=$3
+	shift 3
+
+	_overlay_mount_dirs $lowerdir $upperdir $workdir \
+				$* $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT
+}
+
 _overlay_mkdirs()
 {
 	local dir=$1
diff --git a/tests/overlay/005 b/tests/overlay/005
index f885fdc..17992a3 100755
--- a/tests/overlay/005
+++ b/tests/overlay/005
@@ -93,7 +93,7 @@ $XFS_IO_PROG -f -c "truncate 48m" ${lowerd}/test_file \
 	>>$seqres.full 2>&1
 
 # mount new overlayfs
-_overlay_mount_dirs $lowerd $upperd $workd $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT
+_overlay_scratch_mount_dirs $lowerd $upperd $workd
 
 # the open call triggers copy-up and it will fail ENOSPC
 $XFS_IO_PROG -f -c "o" ${SCRATCH_MNT}/test_file \
diff --git a/tests/overlay/010 b/tests/overlay/010
index dee99cf..f55ebec 100755
--- a/tests/overlay/010
+++ b/tests/overlay/010
@@ -67,8 +67,7 @@ touch $lowerdir1/testdir/a $lowerdir1/testdir/b
 mknod $lowerdir2/testdir/a c 0 0
 
 # Mount overlayfs and remove testdir, which led to kernel crash
-_overlay_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir \
-		    $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT
+_overlay_scratch_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
 rm -rf $SCRATCH_MNT/testdir
 
 # success, all done
diff --git a/tests/overlay/014 b/tests/overlay/014
index cd76835..9f308d3 100755
--- a/tests/overlay/014
+++ b/tests/overlay/014
@@ -72,7 +72,7 @@ mkdir -p $lowerdir1/testdir/d
 
 # mount overlay with $lowerdir2 as upperdir, and remove & recreate testdir,
 # make testdir on $lowerdir2 opaque
-_overlay_mount_dirs $lowerdir1 $lowerdir2 $workdir2 $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT
+_overlay_scratch_mount_dirs $lowerdir1 $lowerdir2 $workdir2
 rm -rf $SCRATCH_MNT/testdir
 mkdir -p $SCRATCH_MNT/testdir/visibledir
 # unmount overlayfs but not base fs
@@ -81,15 +81,13 @@ $UMOUNT_PROG $SCRATCH_MNT
 # 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
-_overlay_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir \
-		    $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT
+_overlay_scratch_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
 touch $SCRATCH_MNT/testdir/visiblefile
 
 # umount and mount overlay again, buggy kernel treats the copied-up dir as
 # opaque, visibledir is not seen in merged dir.
 $UMOUNT_PROG $SCRATCH_MNT
-_overlay_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir \
-		    $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT
+_overlay_scratch_mount_dirs "$lowerdir2:$lowerdir1" $upperdir $workdir
 ls $SCRATCH_MNT/testdir
 
 # success, all done
diff --git a/tests/overlay/031 b/tests/overlay/031
index 70ee299..186b409 100755
--- a/tests/overlay/031
+++ b/tests/overlay/031
@@ -51,7 +51,7 @@ create_whiteout()
 	mkdir -p $lower/testdir
 	touch $lower/testdir/$file
 
-	_overlay_mount_dirs $lower $upper $work $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT
+	_overlay_scratch_mount_dirs $lower $upper $work
 
 	rm -f $SCRATCH_MNT/testdir/$file
 
@@ -91,7 +91,7 @@ create_whiteout $lowerdir1 $upperdir $workdir $testfile1
 # whiteout will expose.
 rm -rf $lowerdir1/testdir
 
-_overlay_mount_dirs $lowerdir1 $upperdir $workdir $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT
+_overlay_scratch_mount_dirs $lowerdir1 $upperdir $workdir
 
 ls $SCRATCH_MNT/testdir
 
@@ -104,7 +104,7 @@ rm -rf $SCRATCH_MNT/testdir 2>&1 | _filter_scratch
 $UMOUNT_PROG $SCRATCH_MNT
 touch $lowerdir1/testdir
 
-_overlay_mount_dirs $lowerdir1 $upperdir $workdir $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT
+_overlay_scratch_mount_dirs $lowerdir1 $upperdir $workdir
 
 # try to remove test dir from overlay dir, trigger ovl_remove_and_whiteout,
 # it will not clean up the dir and lead to residue.
@@ -121,8 +121,7 @@ create_whiteout $lowerdir2 $lowerdir1 $workdir1 $testfile1
 
 rm -rf $lowerdir2/testdir
 
-_overlay_mount_dirs "$lowerdir1:$lowerdir2" $upperdir $workdir \
-		     $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT
+_overlay_scratch_mount_dirs "$lowerdir1:$lowerdir2" $upperdir $workdir
 
 ls $SCRATCH_MNT/testdir
 rm -rf $SCRATCH_MNT/testdir 2>&1 | _filter_scratch
@@ -139,7 +138,7 @@ rm -rf $lowerdir1/testdir/$testfile1
 
 create_whiteout $lowerdir2 $lowerdir1 $workdir1 $testfile2
 
-_overlay_mount_dirs $lowerdir1 $upperdir $workdir $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT
+_overlay_scratch_mount_dirs $lowerdir1 $upperdir $workdir
 
 ls $SCRATCH_MNT/testdir
 rm -rf $SCRATCH_MNT/testdir 2>&1 | _filter_scratch
diff --git a/tests/overlay/035 b/tests/overlay/035
index 0ef91c6..64fcd70 100755
--- a/tests/overlay/035
+++ b/tests/overlay/035
@@ -77,8 +77,7 @@ $CHATTR_PROG +i $workdir
 
 # Mount overlay with upper and workdir and expect failure to re-create workdir.
 # Verify that overlay is mounted read-only and that it cannot be remounted rw.
-_overlay_mount_dirs $lowerdir2 $upperdir $workdir \
-		    $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT
+_overlay_scratch_mount_dirs $lowerdir2 $upperdir $workdir
 touch $SCRATCH_MNT/bar 2>&1 | _filter_scratch
 _scratch_remount rw 2>&1 | _filter_scratch
 
diff --git a/tests/overlay/037 b/tests/overlay/037
index 6be9a26..7287329 100755
--- a/tests/overlay/037
+++ b/tests/overlay/037
@@ -71,23 +71,21 @@ mkdir -p $lowerdir $lowerdir2 $upperdir $upperdir2 $workdir
 
 # Mount overlay with lowerdir, upperdir, workdir and index=on
 # to store the file handles of lowerdir and upperdir in overlay.origin xattr
-_overlay_mount_dirs $lowerdir $upperdir $workdir \
-		    $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT -oindex=on
+_overlay_scratch_mount_dirs $lowerdir $upperdir $workdir -oindex=on
 $UMOUNT_PROG $SCRATCH_MNT
 
 # Try to mount an overlay with the same upperdir and different lowerdir - expect ESTALE
-_overlay_mount_dirs $lowerdir2 $upperdir $workdir \
-		    $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT -oindex=on 2>&1 | _filter_scratch
+_overlay_scratch_mount_dirs $lowerdir2 $upperdir $workdir -oindex=on \
+	2>&1 | _filter_scratch
 $UMOUNT_PROG $SCRATCH_MNT 2>/dev/null
 
 # Try to mount an overlay with the same workdir and different upperdir - expect ESTALE
-_overlay_mount_dirs $lowerdir $upperdir2 $workdir \
-		    $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT -oindex=on 2>&1 | _filter_scratch
+_overlay_scratch_mount_dirs $lowerdir $upperdir2 $workdir -oindex=on \
+	2>&1 | _filter_scratch
 $UMOUNT_PROG $SCRATCH_MNT 2>/dev/null
 
 # Mount overlay with original lowerdir, upperdir, workdir and index=on - expect success
-_overlay_mount_dirs $lowerdir $upperdir $workdir \
-		    $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT -oindex=on
+_overlay_scratch_mount_dirs $lowerdir $upperdir $workdir -oindex=on
 
 # success, all done
 status=0
diff --git a/tests/overlay/038 b/tests/overlay/038
index 69fd76a..a281691 100755
--- a/tests/overlay/038
+++ b/tests/overlay/038
@@ -177,8 +177,7 @@ mkdir -p $workdir
 mkdir -p $middir/test_dir
 mkdir -p $lowerdir/test_dir/pure_lower_dir
 
-$MOUNT_PROG -t overlay overlay -o lowerdir=$middir:$lowerdir \
-	    -o upperdir=$upperdir -o workdir=$workdir $SCRATCH_MNT
+_overlay_scratch_mount_dirs "$middir:$lowerdir" $upperdir $workdir
 
 # Copy up test_dir
 touch $SCRATCH_MNT/test_dir/test_file
-- 
2.7.4

[PATCH 4/4] overlay: fix _overlay_config_override of MOUNT_OPTIONS

[Amir Goldstein]

The config variable OVERLAY_MOUNT_OPTIONS is used to configure
the overlay mount options when running ./check -overlay.
The config variable MOUNT_OPTIONS is used to configure the
mount options for base fs.

If config sets value of OVERLAY_MOUNT_OPTIONS and
does not set MOUNT_OPTIONS, the value of MOUNT_OPTIONS
may be leftover from previous _overlay_config_override, so
don't use that value for base fs mount.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 common/config | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/common/config b/common/config
index 71798f0..8844173 100644
--- a/common/config
+++ b/common/config
@@ -532,6 +532,10 @@ _overlay_config_override()
 	# Store original base fs vars
 	export OVL_BASE_TEST_DEV="$TEST_DEV"
 	export OVL_BASE_TEST_DIR="$TEST_DIR"
+	# If config does not set MOUNT_OPTIONS, its value may be
+	# leftover from previous _overlay_config_override, so
+	# don't use that value for base fs mount
+	[ "$MOUNT_OPTIONS" != "$OVERLAY_MOUNT_OPTIONS" ] || unset MOUNT_OPTIONS
 	export OVL_BASE_MOUNT_OPTIONS="$MOUNT_OPTIONS"
 
 	# Set TEST vars to overlay base and mount dirs inside base fs
-- 
2.7.4

Re: [PATCH 2/4] overlay: use default overlay mount options _overlay_mount_dirs()

[Amir Goldstein]

On Wed, Sep 27, 2017 at 10:04 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> Tests that use _overlay_mount_dirs() should also use the
> default overlay mount options.

Vivek, Eryu,

I should make a disclaimer here: I did not test with SELINUX_MOUNT_OPTIONS
because I have no SELinux in my test setup.

Specifically, I am concerned that tests that compose "special" overlay mounts,
like tmpfs mounts and stacked overlay mounts (overlay/029) may not play well
with SELINUX_MOUNT_OPTIONS applied to the special mounts.
I am less concerned about the tests that were converted to use
_overlay_scratch_mount_dirs() helper.

Can either of you run a -g overlay/quick test with this series and valid
SELINUX_MOUNT_OPTIONS?

Thanks,
Amir.

Re: [PATCH 2/4] overlay: use default overlay mount options _overlay_mount_dirs()

[Eryu Guan]

On Wed, Sep 27, 2017 at 10:50:46AM +0300, Amir Goldstein wrote:
> On Wed, Sep 27, 2017 at 10:04 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> > Tests that use _overlay_mount_dirs() should also use the
> > default overlay mount options.
> 
> Vivek, Eryu,
> 
> I should make a disclaimer here: I did not test with SELINUX_MOUNT_OPTIONS
> because I have no SELinux in my test setup.
> 
> Specifically, I am concerned that tests that compose "special" overlay mounts,
> like tmpfs mounts and stacked overlay mounts (overlay/029) may not play well
> with SELINUX_MOUNT_OPTIONS applied to the special mounts.
> I am less concerned about the tests that were converted to use
> _overlay_scratch_mount_dirs() helper.
> 
> Can either of you run a -g overlay/quick test with this series and valid
> SELINUX_MOUNT_OPTIONS?

Sure, I'll do the testings anyway, and I have selinux enabled on my test
vms.

I noticed the overlay mount option mess too when I was reviewing
MOUNT_OPTIONS fixes for btrfs from Gu Jinxiang last week. I was hoping
to refactor the whole mount option handling through fstests. Perhaps
this patchset is good start toward that direction. I'll test and review
them. Thanks!

Eryu

[PATCH 5/5] overlay: move _overlay helpers to common/overlay

[Amir Goldstein]

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Eryu,

If you don't mind, please apply this cleanup patch on
top of the OVERLAY_MOUNT_OPTIONS series.

This is actaully a prep patch for implementing the first
_check_overlay_filesystem, but might as well take it now.

Amir.

 common/overlay | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 common/rc      | 152 +-----------------------------------------------------
 2 files changed, 162 insertions(+), 150 deletions(-)
 create mode 100644 common/overlay

diff --git a/common/overlay b/common/overlay
new file mode 100644
index 0000000..c3bb9ee
--- /dev/null
+++ b/common/overlay
@@ -0,0 +1,160 @@
+#
+# overlayfs specific common functions.
+#
+
+# helper function to do the actual overlayfs mount operation
+_overlay_mount_dirs()
+{
+	local lowerdir=$1
+	local upperdir=$2
+	local workdir=$3
+	shift 3
+
+	$MOUNT_PROG -t overlay -o lowerdir=$lowerdir -o upperdir=$upperdir \
+		    -o workdir=$workdir `_common_dev_mount_options $*`
+}
+
+# Mount with same options/mnt/dev of scratch mount, but optionally
+# with different lower/upper/work dirs
+_overlay_scratch_mount_dirs()
+{
+	local lowerdir=$1
+	local upperdir=$2
+	local workdir=$3
+	shift 3
+
+	_overlay_mount_dirs $lowerdir $upperdir $workdir \
+				$* $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT
+}
+
+_overlay_mkdirs()
+{
+	local dir=$1
+
+	mkdir -p $dir/$OVL_UPPER
+	mkdir -p $dir/$OVL_LOWER
+	mkdir -p $dir/$OVL_WORK
+	mkdir -p $dir/$OVL_MNT
+}
+
+# Given a base fs dir, set up overlay directories and mount on the given mnt.
+# The dir is used as the mount device so it can be seen from df or mount
+_overlay_mount()
+{
+	local dir=$1
+	local mnt=$2
+	shift 2
+
+	_supports_filetype $dir || _notrun "upper fs needs to support d_type"
+
+	_overlay_mkdirs $dir
+
+	_overlay_mount_dirs $dir/$OVL_LOWER $dir/$OVL_UPPER $dir/$OVL_WORK \
+				$* $dir $mnt
+}
+
+_overlay_base_test_mount()
+{
+	if [ -z "$OVL_BASE_TEST_DEV" -o -z "$OVL_BASE_TEST_DIR" ] || \
+		_check_mounted_on OVL_BASE_TEST_DEV $OVL_BASE_TEST_DEV \
+				OVL_BASE_TEST_DIR $OVL_BASE_TEST_DIR
+	then
+		# no base fs or already mounted
+		return 0
+	elif [ $? -ne 1 ]
+	then
+		# base fs mounted but not on mount point
+		return 1
+	fi
+
+	_mount $TEST_FS_MOUNT_OPTS \
+		$SELINUX_MOUNT_OPTIONS \
+		$OVL_BASE_TEST_DEV $OVL_BASE_TEST_DIR
+}
+
+_overlay_test_mount()
+{
+	_overlay_base_test_mount && \
+		_overlay_mount $OVL_BASE_TEST_DIR $TEST_DIR $*
+}
+
+_overlay_base_scratch_mount()
+{
+	if [ -z "$OVL_BASE_SCRATCH_DEV" -o -z "$OVL_BASE_SCRATCH_MNT" ] || \
+		_check_mounted_on OVL_BASE_SCRATCH_DEV $OVL_BASE_SCRATCH_DEV \
+				OVL_BASE_SCRATCH_MNT $OVL_BASE_SCRATCH_MNT
+	then
+		# no base fs or already mounted
+		return 0
+	elif [ $? -ne 1 ]
+	then
+		# base fs mounted but not on mount point
+		return 1
+	fi
+
+	_mount $OVL_BASE_MOUNT_OPTIONS \
+		$SELINUX_MOUNT_OPTIONS \
+		$OVL_BASE_SCRATCH_DEV $OVL_BASE_SCRATCH_MNT
+}
+
+_overlay_base_scratch_unmount()
+{
+	[ -n "$OVL_BASE_SCRATCH_DEV" -a -n "$OVL_BASE_SCRATCH_MNT" ] || return 0
+
+	$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT
+}
+
+_overlay_scratch_mount()
+{
+	_overlay_base_scratch_mount && \
+		_overlay_mount $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT $*
+}
+
+_overlay_base_test_unmount()
+{
+	[ -n "$OVL_BASE_TEST_DEV" -a -n "$OVL_BASE_TEST_DIR" ] || return 0
+
+	$UMOUNT_PROG $OVL_BASE_TEST_DIR
+}
+
+_overlay_test_unmount()
+{
+	$UMOUNT_PROG $TEST_DIR
+	_overlay_base_test_unmount
+}
+
+_overlay_scratch_unmount()
+{
+	$UMOUNT_PROG $SCRATCH_MNT
+	_overlay_base_scratch_unmount
+}
+
+# Require a specific overlayfs feature
+_require_scratch_overlay_feature()
+{
+	local feature=$1
+
+	# overalyfs features (e.g. redirect_dir, index) are
+	# configurable from Kconfig (the build default), by module
+	# parameter (the system default) and per mount by mount
+	# option ${feature}=[on|off].
+	#
+	# If the module parameter does not exist then there is no
+	# point in checking the mount option.
+	local default=`_get_fs_module_param ${feature}`
+	[ "$default" = Y ] || [ "$default" = N ] || \
+		_notrun "feature '${feature}' not supported by ${FSTYP}"
+
+	_scratch_mkfs > /dev/null 2>&1
+	_scratch_mount -o ${feature}=on || \
+		_notrun "${FSTYP} feature '${feature}' cannot be enabled on ${SCRATCH_DEV}"
+	# Check options to be sure. For example, Overlayfs will fallback to
+	# index=off if underlying fs does not support file handles.
+	# Overlayfs only displays mount option if it differs from the default.
+	# Overlayfs may enable the feature, but fallback to read-only mount.
+	((( [ "$default" = N ] && _fs_options $SCRATCH_DEV | grep -q "${feature}=on" ) || \
+	  ( [ "$default" = Y ] && ! _fs_options $SCRATCH_DEV | grep -q "${feature}=off" )) && \
+	    touch $SCRATCH_MNT/foo 2>/dev/null ) || \
+	        _notrun "${FSTYP} feature '${feature}' cannot be enabled on ${SCRATCH_DEV}"
+	_scratch_unmount
+}
diff --git a/common/rc b/common/rc
index 54f81d3..201159b 100644
--- a/common/rc
+++ b/common/rc
@@ -171,6 +171,7 @@ case "$FSTYP" in
     glusterfs)
 	 ;;
     overlay)
+	 . ./common/overlay
 	 ;;
     reiser4)
 	 [ "$MKFS_REISER4_PROG" = "" ] && _fatal "mkfs.reiser4 not found"
@@ -333,133 +334,6 @@ _supports_filetype()
 	esac
 }
 
-# helper function to do the actual overlayfs mount operation
-_overlay_mount_dirs()
-{
-	local lowerdir=$1
-	local upperdir=$2
-	local workdir=$3
-	shift 3
-
-	$MOUNT_PROG -t overlay -o lowerdir=$lowerdir -o upperdir=$upperdir \
-		    -o workdir=$workdir `_common_dev_mount_options $*`
-}
-
-# Mount with same options/mnt/dev of scratch mount, but optionally
-# with different lower/upper/work dirs
-_overlay_scratch_mount_dirs()
-{
-	local lowerdir=$1
-	local upperdir=$2
-	local workdir=$3
-	shift 3
-
-	_overlay_mount_dirs $lowerdir $upperdir $workdir \
-				$* $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT
-}
-
-_overlay_mkdirs()
-{
-	local dir=$1
-
-	mkdir -p $dir/$OVL_UPPER
-	mkdir -p $dir/$OVL_LOWER
-	mkdir -p $dir/$OVL_WORK
-	mkdir -p $dir/$OVL_MNT
-}
-
-# Given a base fs dir, set up overlay directories and mount on the given mnt.
-# The dir is used as the mount device so it can be seen from df or mount
-_overlay_mount()
-{
-	local dir=$1
-	local mnt=$2
-	shift 2
-
-	_supports_filetype $dir || _notrun "upper fs needs to support d_type"
-
-	_overlay_mkdirs $dir
-
-	_overlay_mount_dirs $dir/$OVL_LOWER $dir/$OVL_UPPER $dir/$OVL_WORK \
-				$* $dir $mnt
-}
-
-_overlay_base_test_mount()
-{
-	if [ -z "$OVL_BASE_TEST_DEV" -o -z "$OVL_BASE_TEST_DIR" ] || \
-		_check_mounted_on OVL_BASE_TEST_DEV $OVL_BASE_TEST_DEV \
-				OVL_BASE_TEST_DIR $OVL_BASE_TEST_DIR
-	then
-		# no base fs or already mounted
-		return 0
-	elif [ $? -ne 1 ]
-	then
-		# base fs mounted but not on mount point
-		return 1
-	fi
-
-	_mount $TEST_FS_MOUNT_OPTS \
-		$SELINUX_MOUNT_OPTIONS \
-		$OVL_BASE_TEST_DEV $OVL_BASE_TEST_DIR
-}
-
-_overlay_test_mount()
-{
-	_overlay_base_test_mount && \
-		_overlay_mount $OVL_BASE_TEST_DIR $TEST_DIR $*
-}
-
-_overlay_base_scratch_mount()
-{
-	if [ -z "$OVL_BASE_SCRATCH_DEV" -o -z "$OVL_BASE_SCRATCH_MNT" ] || \
-		_check_mounted_on OVL_BASE_SCRATCH_DEV $OVL_BASE_SCRATCH_DEV \
-				OVL_BASE_SCRATCH_MNT $OVL_BASE_SCRATCH_MNT
-	then
-		# no base fs or already mounted
-		return 0
-	elif [ $? -ne 1 ]
-	then
-		# base fs mounted but not on mount point
-		return 1
-	fi
-
-	_mount $OVL_BASE_MOUNT_OPTIONS \
-		$SELINUX_MOUNT_OPTIONS \
-		$OVL_BASE_SCRATCH_DEV $OVL_BASE_SCRATCH_MNT
-}
-
-_overlay_base_scratch_unmount()
-{
-	[ -n "$OVL_BASE_SCRATCH_DEV" -a -n "$OVL_BASE_SCRATCH_MNT" ] || return 0
-
-	$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT
-}
-
-_overlay_scratch_mount()
-{
-	_overlay_base_scratch_mount && \
-		_overlay_mount $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT $*
-}
-
-_overlay_base_test_unmount()
-{
-	[ -n "$OVL_BASE_TEST_DEV" -a -n "$OVL_BASE_TEST_DIR" ] || return 0
-
-	$UMOUNT_PROG $OVL_BASE_TEST_DIR
-}
-
-_overlay_test_unmount()
-{
-	$UMOUNT_PROG $TEST_DIR
-	_overlay_base_test_unmount
-}
-
-_overlay_scratch_unmount()
-{
-	$UMOUNT_PROG $SCRATCH_MNT
-	_overlay_base_scratch_unmount
-}
-
 _scratch_mount()
 {
     if [ "$FSTYP" == "overlay" ]; then
@@ -3672,29 +3546,7 @@ _require_scratch_feature()
 
 	case "$FSTYP" in
 	overlay)
-		# overalyfs features (e.g. redirect_dir, index) are
-		# configurable from Kconfig (the build default), by module
-		# parameter (the system default) and per mount by mount
-		# option ${feature}=[on|off].
-		#
-		# If the module parameter does not exist then there is no
-		# point in checking the mount option.
-		local default=`_get_fs_module_param ${feature}`
-		[ "$default" = Y ] || [ "$default" = N ] || \
-			_notrun "feature '${feature}' not supported by ${FSTYP}"
-
-		_scratch_mkfs > /dev/null 2>&1
-		_scratch_mount -o ${feature}=on || \
-			_notrun "${FSTYP} feature '${feature}' cannot be enabled on ${SCRATCH_DEV}"
-		# Check options to be sure. For example, Overlayfs will fallback to
-		# index=off if underlying fs does not support file handles.
-		# Overlayfs only displays mount option if it differs from the default.
-		# Overlayfs may enable the feature, but fallback to read-only mount.
-		((( [ "$default" = N ] && _fs_options $SCRATCH_DEV | grep -q "${feature}=on" ) || \
-		  ( [ "$default" = Y ] && ! _fs_options $SCRATCH_DEV | grep -q "${feature}=off" )) && \
-		    touch $SCRATCH_MNT/foo 2>/dev/null ) || \
-		        _notrun "${FSTYP} feature '${feature}' cannot be enabled on ${SCRATCH_DEV}"
-		_scratch_unmount
+		_require_scratch_overlay_feature ${feature}
 		;;
 	*)
 		_fail "Test for feature '${feature}' of ${FSTYP} is not implemented"
-- 
2.7.4

[PATCH 6/6] overlay: deduplicate code in overlay mount helpers

[Amir Goldstein]

factor out helpers _overlay_base_mount() and _overlay_base_umount()
to reduce code duplication.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Eryu,

Appologies for the rolling series, but since you said you are looking
into re-factoring mount option handling, I figured you may want to take
this additional cleanup as well.

Beyond reducing code duplication, this change puts the difference in
mount options for base test fs and base scratch fs in wrapper functions
and leaves the common code free of those differences.

Amir.

 common/overlay | 66 ++++++++++++++++++++++++++--------------------------------
 1 file changed, 29 insertions(+), 37 deletions(-)

diff --git a/common/overlay b/common/overlay
index c3bb9ee..79097ba 100644
--- a/common/overlay
+++ b/common/overlay
@@ -53,23 +53,31 @@ _overlay_mount()
 				$* $dir $mnt
 }
 
-_overlay_base_test_mount()
+_overlay_base_mount()
 {
-	if [ -z "$OVL_BASE_TEST_DEV" -o -z "$OVL_BASE_TEST_DIR" ] || \
-		_check_mounted_on OVL_BASE_TEST_DEV $OVL_BASE_TEST_DEV \
-				OVL_BASE_TEST_DIR $OVL_BASE_TEST_DIR
-	then
+	local devname=$1
+	local mntname=$2
+	local dev=$3
+	local mnt=$4
+	shift 4
+
+	if [ -z "$dev" -o -z "$mnt" ] || \
+		_check_mounted_on $devname $dev $mntname $mnt; then
 		# no base fs or already mounted
 		return 0
-	elif [ $? -ne 1 ]
-	then
+	elif [ $? -ne 1 ]; then
 		# base fs mounted but not on mount point
 		return 1
 	fi
 
-	_mount $TEST_FS_MOUNT_OPTS \
-		$SELINUX_MOUNT_OPTIONS \
-		$OVL_BASE_TEST_DEV $OVL_BASE_TEST_DIR
+	_mount $* $dev $mnt
+}
+
+_overlay_base_test_mount()
+{
+	_overlay_base_mount OVL_BASE_TEST_DEV OVL_BASE_TEST_DIR \
+			"$OVL_BASE_TEST_DEV" "$OVL_BASE_TEST_DIR" \
+			$TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS
 }
 
 _overlay_test_mount()
@@ -80,28 +88,9 @@ _overlay_test_mount()
 
 _overlay_base_scratch_mount()
 {
-	if [ -z "$OVL_BASE_SCRATCH_DEV" -o -z "$OVL_BASE_SCRATCH_MNT" ] || \
-		_check_mounted_on OVL_BASE_SCRATCH_DEV $OVL_BASE_SCRATCH_DEV \
-				OVL_BASE_SCRATCH_MNT $OVL_BASE_SCRATCH_MNT
-	then
-		# no base fs or already mounted
-		return 0
-	elif [ $? -ne 1 ]
-	then
-		# base fs mounted but not on mount point
-		return 1
-	fi
-
-	_mount $OVL_BASE_MOUNT_OPTIONS \
-		$SELINUX_MOUNT_OPTIONS \
-		$OVL_BASE_SCRATCH_DEV $OVL_BASE_SCRATCH_MNT
-}
-
-_overlay_base_scratch_unmount()
-{
-	[ -n "$OVL_BASE_SCRATCH_DEV" -a -n "$OVL_BASE_SCRATCH_MNT" ] || return 0
-
-	$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT
+	_overlay_base_mount OVL_BASE_SCRATCH_DEV OVL_BASE_SCRATCH_MNT \
+			"$OVL_BASE_SCRATCH_DEV" "$OVL_BASE_SCRATCH_MNT" \
+			$OVL_BASE_MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS
 }
 
 _overlay_scratch_mount()
@@ -110,23 +99,26 @@ _overlay_scratch_mount()
 		_overlay_mount $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT $*
 }
 
-_overlay_base_test_unmount()
+_overlay_base_unmount()
 {
-	[ -n "$OVL_BASE_TEST_DEV" -a -n "$OVL_BASE_TEST_DIR" ] || return 0
+	local dev=$1
+	local mnt=$2
+
+	[ -n "$dev" -a -n "$mnt" ] || return 0
 
-	$UMOUNT_PROG $OVL_BASE_TEST_DIR
+	$UMOUNT_PROG $mnt
 }
 
 _overlay_test_unmount()
 {
 	$UMOUNT_PROG $TEST_DIR
-	_overlay_base_test_unmount
+	_overlay_base_unmount "$OVL_BASE_TEST_DEV" "$OVL_BASE_TEST_DIR"
 }
 
 _overlay_scratch_unmount()
 {
 	$UMOUNT_PROG $SCRATCH_MNT
-	_overlay_base_scratch_unmount
+	_overlay_base_unmount "$OVL_BASE_SCRATCH_DEV" "$OVL_BASE_SCRATCH_MNT"
 }
 
 # Require a specific overlayfs feature
-- 
2.7.4

Re: [PATCH 6/6] overlay: deduplicate code in overlay mount helpers

[Eryu Guan]

On Thu, Sep 28, 2017 at 02:55:10PM +0300, Amir Goldstein wrote:
> factor out helpers _overlay_base_mount() and _overlay_base_umount()
> to reduce code duplication.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Eryu,
> 
> Appologies for the rolling series, but since you said you are looking
> into re-factoring mount option handling, I figured you may want to take
> this additional cleanup as well.

No problem, thanks for all the work! However, I haven't got some time to
test this patchset yet (but will start testing today), and the mount
option handling seems quite complicated from my quick look last week, I
may need some time to think more about it and play with this patchset.
But it's public holiday next week here in China, I may take more time
than expected :)

Thanks,
Eryu

> 
> Beyond reducing code duplication, this change puts the difference in
> mount options for base test fs and base scratch fs in wrapper functions
> and leaves the common code free of those differences.
> 
> Amir.
> 
>  common/overlay | 66 ++++++++++++++++++++++++++--------------------------------
>  1 file changed, 29 insertions(+), 37 deletions(-)
> 
> diff --git a/common/overlay b/common/overlay
> index c3bb9ee..79097ba 100644
> --- a/common/overlay
> +++ b/common/overlay
> @@ -53,23 +53,31 @@ _overlay_mount()
>  				$* $dir $mnt
>  }
>  
> -_overlay_base_test_mount()
> +_overlay_base_mount()
>  {
> -	if [ -z "$OVL_BASE_TEST_DEV" -o -z "$OVL_BASE_TEST_DIR" ] || \
> -		_check_mounted_on OVL_BASE_TEST_DEV $OVL_BASE_TEST_DEV \
> -				OVL_BASE_TEST_DIR $OVL_BASE_TEST_DIR
> -	then
> +	local devname=$1
> +	local mntname=$2
> +	local dev=$3
> +	local mnt=$4
> +	shift 4
> +
> +	if [ -z "$dev" -o -z "$mnt" ] || \
> +		_check_mounted_on $devname $dev $mntname $mnt; then
>  		# no base fs or already mounted
>  		return 0
> -	elif [ $? -ne 1 ]
> -	then
> +	elif [ $? -ne 1 ]; then
>  		# base fs mounted but not on mount point
>  		return 1
>  	fi
>  
> -	_mount $TEST_FS_MOUNT_OPTS \
> -		$SELINUX_MOUNT_OPTIONS \
> -		$OVL_BASE_TEST_DEV $OVL_BASE_TEST_DIR
> +	_mount $* $dev $mnt
> +}
> +
> +_overlay_base_test_mount()
> +{
> +	_overlay_base_mount OVL_BASE_TEST_DEV OVL_BASE_TEST_DIR \
> +			"$OVL_BASE_TEST_DEV" "$OVL_BASE_TEST_DIR" \
> +			$TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS
>  }
>  
>  _overlay_test_mount()
> @@ -80,28 +88,9 @@ _overlay_test_mount()
>  
>  _overlay_base_scratch_mount()
>  {
> -	if [ -z "$OVL_BASE_SCRATCH_DEV" -o -z "$OVL_BASE_SCRATCH_MNT" ] || \
> -		_check_mounted_on OVL_BASE_SCRATCH_DEV $OVL_BASE_SCRATCH_DEV \
> -				OVL_BASE_SCRATCH_MNT $OVL_BASE_SCRATCH_MNT
> -	then
> -		# no base fs or already mounted
> -		return 0
> -	elif [ $? -ne 1 ]
> -	then
> -		# base fs mounted but not on mount point
> -		return 1
> -	fi
> -
> -	_mount $OVL_BASE_MOUNT_OPTIONS \
> -		$SELINUX_MOUNT_OPTIONS \
> -		$OVL_BASE_SCRATCH_DEV $OVL_BASE_SCRATCH_MNT
> -}
> -
> -_overlay_base_scratch_unmount()
> -{
> -	[ -n "$OVL_BASE_SCRATCH_DEV" -a -n "$OVL_BASE_SCRATCH_MNT" ] || return 0
> -
> -	$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT
> +	_overlay_base_mount OVL_BASE_SCRATCH_DEV OVL_BASE_SCRATCH_MNT \
> +			"$OVL_BASE_SCRATCH_DEV" "$OVL_BASE_SCRATCH_MNT" \
> +			$OVL_BASE_MOUNT_OPTIONS $SELINUX_MOUNT_OPTIONS
>  }
>  
>  _overlay_scratch_mount()
> @@ -110,23 +99,26 @@ _overlay_scratch_mount()
>  		_overlay_mount $OVL_BASE_SCRATCH_MNT $SCRATCH_MNT $*
>  }
>  
> -_overlay_base_test_unmount()
> +_overlay_base_unmount()
>  {
> -	[ -n "$OVL_BASE_TEST_DEV" -a -n "$OVL_BASE_TEST_DIR" ] || return 0
> +	local dev=$1
> +	local mnt=$2
> +
> +	[ -n "$dev" -a -n "$mnt" ] || return 0
>  
> -	$UMOUNT_PROG $OVL_BASE_TEST_DIR
> +	$UMOUNT_PROG $mnt
>  }
>  
>  _overlay_test_unmount()
>  {
>  	$UMOUNT_PROG $TEST_DIR
> -	_overlay_base_test_unmount
> +	_overlay_base_unmount "$OVL_BASE_TEST_DEV" "$OVL_BASE_TEST_DIR"
>  }
>  
>  _overlay_scratch_unmount()
>  {
>  	$UMOUNT_PROG $SCRATCH_MNT
> -	_overlay_base_scratch_unmount
> +	_overlay_base_unmount "$OVL_BASE_SCRATCH_DEV" "$OVL_BASE_SCRATCH_MNT"
>  }
>  
>  # Require a specific overlayfs feature
> -- 
> 2.7.4
> 

Re: [PATCH 2/4] overlay: use default overlay mount options _overlay_mount_dirs()

[Eryu Guan]

On Wed, Sep 27, 2017 at 10:04:10AM +0300, Amir Goldstein wrote:
> Tests that use _overlay_mount_dirs() should also use the
> default overlay mount options.
> Move mount options from overlay_mount() into _overlay_mount_dirs()
> and use helper common_dev_mount_opts() to get options.
> 
> OVERLAY_MOUNT_OPTIONS is assigned to MOUNT_OPTIONS, so
> there is no need to use OVERLAY_MOUNT_OPTIONS directly.

Agreed, all the fs-specific mount options should be assigned to
MOUNT_OPTIONS in common/config at test init time, if MOUNT_OPTIONS is
not specified. We should always use MOUNT_OPTIONS or TEST_FS_MOUNT_OPTS
in other places.

Thanks,
Eryu

> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> 
> Use common_dev_mount_opts in _overlay_mount_dirs()
> ---
>  common/rc | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 1a61549..b30c4b9 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -342,7 +342,7 @@ _overlay_mount_dirs()
>  	shift 3
>  
>  	$MOUNT_PROG -t overlay -o lowerdir=$lowerdir -o upperdir=$upperdir \
> -		    -o workdir=$workdir $*
> +		    -o workdir=$workdir `_common_dev_mount_options $*`
>  }
>  
>  _overlay_mkdirs()
> @@ -367,9 +367,8 @@ _overlay_mount()
>  
>  	_overlay_mkdirs $dir
>  
> -	_overlay_mount_dirs $dir/$OVL_LOWER $dir/$OVL_UPPER \
> -			    $dir/$OVL_WORK $OVERLAY_MOUNT_OPTIONS \
> -			    $SELINUX_MOUNT_OPTIONS $* $dir $mnt
> +	_overlay_mount_dirs $dir/$OVL_LOWER $dir/$OVL_UPPER $dir/$OVL_WORK \
> +				$* $dir $mnt
>  }
>  
>  _overlay_base_test_mount()
> -- 
> 2.7.4
> 

Re: [PATCH 4/4] overlay: fix _overlay_config_override of MOUNT_OPTIONS

[Eryu Guan]

On Wed, Sep 27, 2017 at 10:04:12AM +0300, Amir Goldstein wrote:
> The config variable OVERLAY_MOUNT_OPTIONS is used to configure
> the overlay mount options when running ./check -overlay.
> The config variable MOUNT_OPTIONS is used to configure the
> mount options for base fs.
> 
> If config sets value of OVERLAY_MOUNT_OPTIONS and
> does not set MOUNT_OPTIONS, the value of MOUNT_OPTIONS
> may be leftover from previous _overlay_config_override, so
> don't use that value for base fs mount.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  common/config | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/common/config b/common/config
> index 71798f0..8844173 100644
> --- a/common/config
> +++ b/common/config
> @@ -532,6 +532,10 @@ _overlay_config_override()
>  	# Store original base fs vars
>  	export OVL_BASE_TEST_DEV="$TEST_DEV"
>  	export OVL_BASE_TEST_DIR="$TEST_DIR"
> +	# If config does not set MOUNT_OPTIONS, its value may be
> +	# leftover from previous _overlay_config_override, so
> +	# don't use that value for base fs mount
> +	[ "$MOUNT_OPTIONS" != "$OVERLAY_MOUNT_OPTIONS" ] || unset MOUNT_OPTIONS
>  	export OVL_BASE_MOUNT_OPTIONS="$MOUNT_OPTIONS"

Hmm, I don't like the idea that specify mount options for base fs
through MOUNT_OPTIONS. With this, we can only specify overlay mount
options via OVERLAY_MOUNT_OPTIONS but not MOUNT_OPTIONS. Ideally, I'd
like both of them work, and OVERLAY_MOUNT_OPTIONS is assigned to
MOUNT_OPTIONS when the latter is empty.

I think MOUNT_OPTIONS and TEST_FS_MOUNT_OPTS should always be the mount
options for filesystem we're currently testing (overlay in this case)
not something else.

OTOH, We can always specify OVL_BASE_MOUNT_OPTIONS directly for base fs
mount options if we want.

But this problem has nothing to do with this patchset, it goes back to
the time when overlay supports base test/scratch dev, so I'm fine with
this patch going in right now.

Thanks,
Eryu

>  
>  	# Set TEST vars to overlay base and mount dirs inside base fs
> -- 
> 2.7.4
> 

Re: [PATCH 4/4] overlay: fix _overlay_config_override of MOUNT_OPTIONS

[Amir Goldstein]

On Wed, Oct 11, 2017 at 2:50 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Wed, Sep 27, 2017 at 10:04:12AM +0300, Amir Goldstein wrote:
>> The config variable OVERLAY_MOUNT_OPTIONS is used to configure
>> the overlay mount options when running ./check -overlay.
>> The config variable MOUNT_OPTIONS is used to configure the
>> mount options for base fs.
>>
>> If config sets value of OVERLAY_MOUNT_OPTIONS and
>> does not set MOUNT_OPTIONS, the value of MOUNT_OPTIONS
>> may be leftover from previous _overlay_config_override, so
>> don't use that value for base fs mount.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  common/config | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/common/config b/common/config
>> index 71798f0..8844173 100644
>> --- a/common/config
>> +++ b/common/config
>> @@ -532,6 +532,10 @@ _overlay_config_override()
>>       # Store original base fs vars
>>       export OVL_BASE_TEST_DEV="$TEST_DEV"
>>       export OVL_BASE_TEST_DIR="$TEST_DIR"
>> +     # If config does not set MOUNT_OPTIONS, its value may be
>> +     # leftover from previous _overlay_config_override, so
>> +     # don't use that value for base fs mount
>> +     [ "$MOUNT_OPTIONS" != "$OVERLAY_MOUNT_OPTIONS" ] || unset MOUNT_OPTIONS
>>       export OVL_BASE_MOUNT_OPTIONS="$MOUNT_OPTIONS"
>
> Hmm, I don't like the idea that specify mount options for base fs
> through MOUNT_OPTIONS. With this, we can only specify overlay mount
> options via OVERLAY_MOUNT_OPTIONS but not MOUNT_OPTIONS. Ideally, I'd
> like both of them work, and OVERLAY_MOUNT_OPTIONS is assigned to
> MOUNT_OPTIONS when the latter is empty.
>
> I think MOUNT_OPTIONS and TEST_FS_MOUNT_OPTS should always be the mount
> options for filesystem we're currently testing (overlay in this case)
> not something else.
>
> OTOH, We can always specify OVL_BASE_MOUNT_OPTIONS directly for base fs
> mount options if we want.
>

It's not exactly how the base fs options work.
Right now user is never expected to configure OVL_BASE options directly
They are always inherited from a config file of another fs, so you can
run check and check -overlay with the same fs config file.

You are right that OVERLAY MOUNT OPTIONS was meant to be used for
default mount options for overlay mount options and not specifically
for check -overlay, so maybe need a new name for config options of
overlay test and scratch mounts, but I am out of ideas for good names.
It does make sense to use it in the context of check -overlay run,
because there is no other value for mount options for the overlay
mount.

> But this problem has nothing to do with this patchset, it goes back to
> the time when overlay supports base test/scratch dev, so I'm fine with
> this patch going in right now.
>
> Thanks,
> Eryu
>
>>
>>       # Set TEST vars to overlay base and mount dirs inside base fs
>> --
>> 2.7.4
>>

Re: [PATCH 2/4] overlay: use default overlay mount options _overlay_mount_dirs()

[Eryu Guan]

On Wed, Sep 27, 2017 at 04:05:49PM +0800, Eryu Guan wrote:
> On Wed, Sep 27, 2017 at 10:50:46AM +0300, Amir Goldstein wrote:
> > On Wed, Sep 27, 2017 at 10:04 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> > > Tests that use _overlay_mount_dirs() should also use the
> > > default overlay mount options.
> > 
> > Vivek, Eryu,
> > 
> > I should make a disclaimer here: I did not test with SELINUX_MOUNT_OPTIONS
> > because I have no SELinux in my test setup.
> > 
> > Specifically, I am concerned that tests that compose "special" overlay mounts,
> > like tmpfs mounts and stacked overlay mounts (overlay/029) may not play well
> > with SELINUX_MOUNT_OPTIONS applied to the special mounts.
> > I am less concerned about the tests that were converted to use
> > _overlay_scratch_mount_dirs() helper.
> > 
> > Can either of you run a -g overlay/quick test with this series and valid
> > SELINUX_MOUNT_OPTIONS?
> 
> Sure, I'll do the testings anyway, and I have selinux enabled on my test
> vms.

I just finished a '-g auto' run with OVERLAY_MOUNT_OPTIONS="-o index=on"
and selinux enabled, and the results look fine, all failures are known
issues. Thanks for the work!

Eryu