Skip to contentSkip to conversation

fstests: fixes for config option OVERLAY_MOUNT_OPTIONS

PATCH In review Amir Goldstein 2 participants 14 messages Sep 27, 2017 – Oct 12, 2017

lore
Share & tools
lore
#0 lore raw

cover fstests: fixes for config option OVERLAY_MOUNT_OPTIONS

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(-)
Signature · 1 line
2.7.4
#1 lore raw

PATCH 1/4 overlay: remove stale implementation of _scratch_mount_options

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

---
 common/rc | 16 ----------------
 1 file changed, 16 deletions(-)
common/rc +0 -16
index 53bbb11..1a61549 100644
@@ -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
 }
Signature · 1 line
2.7.4
#2 lore raw

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

In reply to 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.

Use common_dev_mount_opts in _overlay_mount_dirs()

---
 common/rc | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
common/rc +3 -4
index 1a61549..b30c4b9 100644
@@ -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()
Signature · 1 line
2.7.4
#3 lore raw

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

Quoted 2 lines · Amir Goldstein

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.

#4 lore raw

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

Quoted 9 lines · Amir Goldstein
Quoted 2 lines · Amir Goldstein

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.

Quoted 8 lines

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

#5 lore raw

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

Quoted 16 lines · Eryu Guan
Quoted 15 lines · Amir Goldstein
Quoted 2 lines · Amir Goldstein

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.

Quoted 5 lines
Quoted 2 lines

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

#6 lore raw

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

In reply to Amir Goldstein
Quoted 7 lines · 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.

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

Quoted 36 lines

Use common_dev_mount_opts in _overlay_mount_dirs()

---
 common/rc | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
common/rc +3 -4
index 1a61549..b30c4b9 100644
@@ -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()
Signature · 1 line
2.7.4
#7 lore raw

PATCH 3/4 overlay: create helper _overlay_scratch_mount_dirs()

In reply to 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.

---
 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(-)
common/rc +13 -0
index b30c4b9..54f81d3 100644
@@ -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
tests/overlay/005 +1 -1
index f885fdc..17992a3 100755
@@ -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 \
tests/overlay/010 +1 -2
index dee99cf..f55ebec 100755
@@ -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
tests/overlay/014 +3 -5
index cd76835..9f308d3 100755
@@ -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
tests/overlay/031 +5 -6
index 70ee299..186b409 100755
@@ -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
tests/overlay/035 +1 -2
index 0ef91c6..64fcd70 100755
@@ -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
 
tests/overlay/037 +6 -8
index 6be9a26..7287329 100755
@@ -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
tests/overlay/038 +1 -2
index 69fd76a..a281691 100755
@@ -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
Signature · 1 line
2.7.4
#8 lore raw

PATCH 4/4 overlay: fix _overlay_config_override of MOUNT_OPTIONS

In reply to 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.

---
 common/config | 4 ++++
 1 file changed, 4 insertions(+)
common/config +4 -0
index 71798f0..8844173 100644
@@ -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
Signature · 1 line
2.7.4
#9 lore raw

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

Quoted 20 lines · 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.

---
 common/config | 4 ++++
 1 file changed, 4 insertions(+)
common/config +0 -0
index 71798f0..8844173 100644

common/config _overlay_config_override() · line 539

Quoted 8 lines
common/config +4 -0
@@ -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

Quoted 5 lines
common/config +0 -0
 	# Set TEST vars to overlay base and mount dirs inside base fs
Signature · 1 line
2.7.4
#10 lore raw

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

Quoted 36 lines · Eryu Guan
Quoted 20 lines · 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.

---
 common/config | 4 ++++
 1 file changed, 4 insertions(+)
common/config +0 -0
index 71798f0..8844173 100644

common/config _overlay_config_override() · line 539

Quoted 8 lines
common/config +4 -0
@@ -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.

Quoted 7 lines

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.

Quoted 12 lines

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

Quoted 5 lines
common/config +0 -1
      # Set TEST vars to overlay base and mount dirs inside base fs
--

2.7.4

#11 lore raw

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

In reply to Amir Goldstein

---

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
common/overlay new file +160 -0
new file mode 100644
index 0000000..c3bb9ee
@@ -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
+}
common/rc +2 -150

170 lines not on this page · raw message

Signature · 1 line
2.7.4
#12 lore raw

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

In reply to Amir Goldstein

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

---

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(-)
common/overlay +29 -37
index c3bb9ee..79097ba 100644
@@ -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
Signature · 1 line
2.7.4
#13 lore raw

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

Quoted 11 lines · Amir Goldstein

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

---

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

Quoted 123 lines

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(-)
common/overlay +29 -37
index c3bb9ee..79097ba 100644
@@ -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
Signature · 1 line
2.7.4