[PATCH 0/2] Require overlay index feature for hardlink tests

[PATCH 0/2] Require overlay index feature for hardlink tests

[Amir Goldstein]

Eryu,

Per your request, with these changes, the overlay/hardlink tests will
"notrun" on kernels <= v4.12, where they are bound to fail.
The test are expected to run and pass on -next kernel regardless of
the value of kernel config CONFIG_OVERLAY_FS_INDEX.

Amir Goldstein (2):
  fstest: add helper _require_fs_feature
  overlay: enable the index feature for overlay/hardlink tests

 common/rc         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/018 |  8 +++++---
 tests/overlay/032 |  4 +++-
 tests/overlay/033 |  8 +++++---
 tests/overlay/034 |  9 ++++++---
 5 files changed, 67 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH 1/2] fstest: add helper _require_fs_feature

[Amir Goldstein]

The helper is used to test if a specific filesystem feature can
be enabled. Currently only implemented testing overlayfs features.

Overalyfs features (e.g. redirect_dir, index) are configurable from
Kconfig (the build default), by module parameter (the system default)
and per mount using the mount option ${feature}=[on|off].

The helper is going to be used by overlay tests, which depend
on the inodes index feature.

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

diff --git a/common/rc b/common/rc
index b505365..260b48d 100644
--- a/common/rc
+++ b/common/rc
@@ -3626,6 +3626,54 @@ _get_fs_sysfs_attr()
 	cat /sys/fs/${FSTYP}/${dname}/${attr}
 }
 
+# Print the value of a filesystem module parameter
+# at /sys/module/$FSTYP/parameters/$PARAM
+#
+# Usage example (FSTYP=overlay):
+#   _get_fs_module_param index
+_get_fs_module_param()
+{
+	cat /sys/module/${FSTYP}/parameters/${1} 2>/dev/null
+}
+
+# Generic test for specific filesystem feature.
+# Currently only implemented to test overlayfs features.
+_require_fs_feature()
+{
+	local feature=$1
+
+	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
+		;;
+	    *)
+		_fail "Test for feature '${feature}' of ${FSTYP} is not implemented"
+		;;
+	esac
+}
+
 
 init_rc
 
-- 
2.7.4

[PATCH 2/2] overlay: enable the index feature for overlay/hardlink tests

[Amir Goldstein]

Overlayfs hardlink test are expected to fail if overlayfs does not
support the inodes index feature, so don't un them if kernel does
not support the feature.

If the feature is supported, enable it with the index=on mount option
for the hardlink tests, regardless of the build time default determined
by kernel config option CONFIG_OVERLAY_FS_INDEX.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/018 | 8 +++++---
 tests/overlay/032 | 4 +++-
 tests/overlay/033 | 8 +++++---
 tests/overlay/034 | 9 ++++++---
 4 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/tests/overlay/018 b/tests/overlay/018
index 41855dc..456f907 100755
--- a/tests/overlay/018
+++ b/tests/overlay/018
@@ -10,7 +10,7 @@
 #
 #-----------------------------------------------------------------------
 #
-# Copyright (C) 2016 CTERA Networks. All Rights Reserved.
+# Copyright (C) 2016-2017 CTERA Networks. All Rights Reserved.
 # Author: Amir Goldstein <amir73il@gmail.com>
 #
 # This program is free software; you can redistribute it and/or
@@ -49,6 +49,7 @@ _cleanup()
 _supported_fs overlay
 _supported_os Linux
 _require_scratch
+_require_fs_feature index
 
 rm -f $seqres.full
 
@@ -81,7 +82,8 @@ function check_ino_nlink()
 	diff -u $before $after
 }
 
-_scratch_mount
+# Enable overlay index feature to prevent breaking hardlinks on copy up
+_scratch_mount -o index=on
 
 
 rm -f $tmp.*
@@ -105,7 +107,7 @@ cat $FILES
 check_ino_nlink $tmp.before $tmp.after_one
 
 # Verify that the hardlinks survive a mount cycle
-_scratch_cycle_mount
+_scratch_cycle_mount index=on
 
 echo "== After mount cycle =="
 cat $FILES
diff --git a/tests/overlay/032 b/tests/overlay/032
index d986ef2..b610bc2 100755
--- a/tests/overlay/032
+++ b/tests/overlay/032
@@ -53,6 +53,7 @@ rm -f $seqres.full
 _supported_fs overlay
 _supported_os Linux
 _require_scratch
+_require_fs_feature index
 
 # Remove all files from previous tests
 _scratch_mkfs
@@ -71,7 +72,8 @@ $XFS_IO_PROG -fc "pwrite 1g 4k" $lowerdir/zero >> $seqres.full
 ln $lowerdir/zero $lowerdir/one
 ln $lowerdir/zero $lowerdir/two
 
-_scratch_mount
+# Enable overlay index feature to prevent breaking hardlinks on copy up
+_scratch_mount -o index=on
 
 do_cmd()
 {
diff --git a/tests/overlay/033 b/tests/overlay/033
index 3c21624..2213a10 100755
--- a/tests/overlay/033
+++ b/tests/overlay/033
@@ -50,6 +50,7 @@ rm -f $seqres.full
 _supported_fs overlay
 _supported_os Linux
 _require_scratch
+_require_fs_feature index
 
 report_nlink()
 {
@@ -122,7 +123,7 @@ test_hardlinks()
 	rm $SCRATCH_MNT/2
 
 	# Verify that orphan index is cleaned when dropping nlink to zero
-	ls $OVL_BASE_SCRATCH_MNT/$OVL_WORK/index 2>/dev/null
+	ls $OVL_BASE_SCRATCH_MNT/$OVL_WORK/index
 }
 
 # Remove all files from previous tests
@@ -131,7 +132,8 @@ _scratch_mkfs
 # Create lower hardlinks
 create_hardlinks
 
-_scratch_mount
+# Enable overlay index feature to prevent breaking hardlinks on copy up
+_scratch_mount -o index=on
 # Test hardlinks with warm dcache
 DCACHETEMP=warm
 test_hardlinks
@@ -140,7 +142,7 @@ test_hardlinks
 _scratch_unmount
 _scratch_mkfs
 create_hardlinks
-_scratch_mount
+_scratch_mount -o index=on
 
 # Test hardlinks with cold dcache
 DCACHETEMP=cold
diff --git a/tests/overlay/034 b/tests/overlay/034
index 2b4ca73..dba984b 100755
--- a/tests/overlay/034
+++ b/tests/overlay/034
@@ -64,6 +64,8 @@ rm -f $seqres.full
 _supported_fs overlay
 _supported_os Linux
 _require_scratch
+# Without overlay index feature hardlinks are broken on copy up
+_require_fs_feature index
 
 # Remove all files from previous tests
 _scratch_mkfs
@@ -74,7 +76,8 @@ mkdir -p $lowerdir
 touch $lowerdir/0
 ln $lowerdir/0 $lowerdir/1
 
-_scratch_mount
+# Enable overlay index feature to prevent breaking hardlinks on copy up
+_scratch_mount -o index=on
 
 # Copy up lower hardlink - overlay inode nlink 2 is copied from lower
 touch $SCRATCH_MNT/0
@@ -99,8 +102,8 @@ rm $SCRATCH_MNT/1
 rm $SCRATCH_MNT/4
 
 # Verify that orphan index is cleaned on mount
-_scratch_cycle_mount
-ls $OVL_BASE_SCRATCH_MNT/$OVL_WORK/index 2>/dev/null
+_scratch_cycle_mount index=on
+ls $OVL_BASE_SCRATCH_MNT/$OVL_WORK/index
 
 echo "Silence is golden"
 status=0
-- 
2.7.4

Re: [PATCH 1/2] fstest: add helper _require_fs_feature

[Eryu Guan]

On Wed, Jul 12, 2017 at 12:43:24PM +0300, Amir Goldstein wrote:
> The helper is used to test if a specific filesystem feature can
> be enabled. Currently only implemented testing overlayfs features.
> 
> Overalyfs features (e.g. redirect_dir, index) are configurable from
> Kconfig (the build default), by module parameter (the system default)
> and per mount using the mount option ${feature}=[on|off].
> 
> The helper is going to be used by overlay tests, which depend
> on the inodes index feature.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  common/rc | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index b505365..260b48d 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3626,6 +3626,54 @@ _get_fs_sysfs_attr()
>  	cat /sys/fs/${FSTYP}/${dname}/${attr}
>  }
>  
> +# Print the value of a filesystem module parameter
> +# at /sys/module/$FSTYP/parameters/$PARAM
> +#
> +# Usage example (FSTYP=overlay):
> +#   _get_fs_module_param index
> +_get_fs_module_param()
> +{
> +	cat /sys/module/${FSTYP}/parameters/${1} 2>/dev/null
> +}
> +
> +# Generic test for specific filesystem feature.
> +# Currently only implemented to test overlayfs features.
> +_require_fs_feature()

I'd think if it should be renamed to _require_scratch_feature, because
it assumes $SCRATCH_DEV is present, and it reminds people it's using
SCRATCH_DEV with "scratch" in the name.

Otherwise looks good to me, it has detailed explanations, thanks a lot!

> +{
> +	local feature=$1
> +
> +	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].

I'll fix the indention here, like

	case "$FSTYP" in
	overlay)
		...
		;;
	*)
		;;
	esac

Thanks,
Eryu

> +		#
> +		# 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
> +		;;
> +	    *)
> +		_fail "Test for feature '${feature}' of ${FSTYP} is not implemented"
> +		;;
> +	esac
> +}
> +
>  
>  init_rc
>  
> -- 
> 2.7.4
> 

Re: [PATCH 1/2] fstest: add helper _require_fs_feature

[Amir Goldstein]

On Thu, Jul 13, 2017 at 3:41 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Wed, Jul 12, 2017 at 12:43:24PM +0300, Amir Goldstein wrote:
>> The helper is used to test if a specific filesystem feature can
>> be enabled. Currently only implemented testing overlayfs features.
>>
>> Overalyfs features (e.g. redirect_dir, index) are configurable from
>> Kconfig (the build default), by module parameter (the system default)
>> and per mount using the mount option ${feature}=[on|off].
>>
>> The helper is going to be used by overlay tests, which depend
>> on the inodes index feature.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  common/rc | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>
>> diff --git a/common/rc b/common/rc
>> index b505365..260b48d 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -3626,6 +3626,54 @@ _get_fs_sysfs_attr()
>>       cat /sys/fs/${FSTYP}/${dname}/${attr}
>>  }
>>
>> +# Print the value of a filesystem module parameter
>> +# at /sys/module/$FSTYP/parameters/$PARAM
>> +#
>> +# Usage example (FSTYP=overlay):
>> +#   _get_fs_module_param index
>> +_get_fs_module_param()
>> +{
>> +     cat /sys/module/${FSTYP}/parameters/${1} 2>/dev/null
>> +}
>> +
>> +# Generic test for specific filesystem feature.
>> +# Currently only implemented to test overlayfs features.
>> +_require_fs_feature()
>
> I'd think if it should be renamed to _require_scratch_feature, because
> it assumes $SCRATCH_DEV is present, and it reminds people it's using
> SCRATCH_DEV with "scratch" in the name.

Right. makes sense. Do you need me to re-post or will you fix on commit?

>
> Otherwise looks good to me, it has detailed explanations, thanks a lot!
>
>> +{
>> +     local feature=$1
>> +
>> +     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].
>
> I'll fix the indention here, like
>
>         case "$FSTYP" in
>         overlay)
>                 ...
>                 ;;
>         *)
>                 ;;
>         esac
>
> Thanks,
> Eryu
>
>> +             #
>> +             # 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
>> +             ;;
>> +         *)
>> +             _fail "Test for feature '${feature}' of ${FSTYP} is not implemented"
>> +             ;;
>> +     esac
>> +}
>> +
>>
>>  init_rc
>>
>> --
>> 2.7.4
>>

Re: [PATCH 1/2] fstest: add helper _require_fs_feature

[Eryu Guan]

On Thu, Jul 13, 2017 at 04:10:44PM +0300, Amir Goldstein wrote:
> On Thu, Jul 13, 2017 at 3:41 PM, Eryu Guan <eguan@redhat.com> wrote:
> > On Wed, Jul 12, 2017 at 12:43:24PM +0300, Amir Goldstein wrote:
> >> The helper is used to test if a specific filesystem feature can
> >> be enabled. Currently only implemented testing overlayfs features.
> >>
> >> Overalyfs features (e.g. redirect_dir, index) are configurable from
> >> Kconfig (the build default), by module parameter (the system default)
> >> and per mount using the mount option ${feature}=[on|off].
> >>
> >> The helper is going to be used by overlay tests, which depend
> >> on the inodes index feature.
> >>
> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >> ---
> >>  common/rc | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 48 insertions(+)
> >>
> >> diff --git a/common/rc b/common/rc
> >> index b505365..260b48d 100644
> >> --- a/common/rc
> >> +++ b/common/rc
> >> @@ -3626,6 +3626,54 @@ _get_fs_sysfs_attr()
> >>       cat /sys/fs/${FSTYP}/${dname}/${attr}
> >>  }
> >>
> >> +# Print the value of a filesystem module parameter
> >> +# at /sys/module/$FSTYP/parameters/$PARAM
> >> +#
> >> +# Usage example (FSTYP=overlay):
> >> +#   _get_fs_module_param index
> >> +_get_fs_module_param()
> >> +{
> >> +     cat /sys/module/${FSTYP}/parameters/${1} 2>/dev/null
> >> +}
> >> +
> >> +# Generic test for specific filesystem feature.
> >> +# Currently only implemented to test overlayfs features.
> >> +_require_fs_feature()
> >
> > I'd think if it should be renamed to _require_scratch_feature, because
> > it assumes $SCRATCH_DEV is present, and it reminds people it's using
> > SCRATCH_DEV with "scratch" in the name.
> 
> Right. makes sense. Do you need me to re-post or will you fix on commit?

You've acked it, I can fix it myself :)

Thanks,
Eryu