generic/402: fix for updated behavior of timestamp limits
diff mbox series

Message ID 20190719041231.26500-1-deepa.kernel@gmail.com
State New
Headers show
Series
  • generic/402: fix for updated behavior of timestamp limits
Related show

Commit Message

Deepa Dinamani July 19, 2019, 4:12 a.m. UTC
The mount behavior will not be altered because of the unsupported
timestamps on the filesystems.

Adjust the test accordingly.

An updated series to be posted after the merge window is hosted at
<https://github.com/deepa-hub/vfs/tree/limits>

Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
---
 common/rc             | 36 +++++++++---------
 tests/generic/402     | 87 ++++++++++++++++---------------------------
 tests/generic/402.out |  2 +-
 3 files changed, 53 insertions(+), 72 deletions(-)

Comments

Eryu Guan July 21, 2019, 4:47 p.m. UTC | #1
On Thu, Jul 18, 2019 at 09:12:31PM -0700, Deepa Dinamani wrote:
> The mount behavior will not be altered because of the unsupported
> timestamps on the filesystems.

It'd be better to provide more details in the commit log, e.g. what
kind of mount behavior and what's the changes being made to the tests.

> 
> Adjust the test accordingly.
> 
> An updated series to be posted after the merge window is hosted at
> <https://github.com/deepa-hub/vfs/tree/limits>
> 
> Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>

Thanks for the update! But I'd like to wait for the kernel patches
merged into mainline first. Would you please send out a notification by
replying this thread then? Thanks a lot!

Eryu

> ---
>  common/rc             | 36 +++++++++---------
>  tests/generic/402     | 87 ++++++++++++++++---------------------------
>  tests/generic/402.out |  2 +-
>  3 files changed, 53 insertions(+), 72 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 25203bb4..39a2deb0 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1959,16 +1959,9 @@ _run_aiodio()
>      return $status
>  }
>  
> -# this test requires y2038 sysfs switch and filesystem
> -# timestamp ranges support.
> -_require_y2038()
> +_require_timestamp_range()
>  {
>  	local device=${1:-$TEST_DEV}
> -	local sysfsdir=/proc/sys/fs/fs-timestamp-check-on
> -
> -	if [ ! -e $sysfsdir ]; then
> -		_notrun "no kernel support for y2038 sysfs switch"
> -	fi
>  
>  	local tsmin tsmax
>  	read tsmin tsmax <<<$(_filesystem_timestamp_range $device)
> @@ -1980,23 +1973,32 @@ _require_y2038()
>  _filesystem_timestamp_range()
>  {
>  	local device=${1:-$TEST_DEV}
> +	u32max=$(((1<<32)-1))
> +	s32min=-$((1<<31))
> +	s32max=$(((1<<31)-1))
> +	s64max=$(((1<<63)-1))
> +	s64min=$((1<<63))
> +
>  	case $FSTYP in
> -	ext4)
> +	ext2)
> +		echo "$s32min $s32max"
> +		;;
> +	ext3|ext4)
>  		if [ $(dumpe2fs -h $device 2>/dev/null | grep "Inode size:" | cut -d: -f2) -gt 128 ]; then
> -			echo "-2147483648 15032385535"
> +			printf "%d %d\n" $s32min 0x37fffffff
>  		else
> -			echo "-2147483648 2147483647"
> +			echo "$s32min $s32max"
>  		fi
>  		;;
>  
> -	xfs)
> -		echo "-2147483648 2147483647"
> -		;;
>  	jfs)
> -		echo "0 4294967295"
> +		echo "0 $u32max"
>  		;;
> -	f2fs)
> -		echo "-2147483648 2147483647"
> +	xfs)
> +		echo "$s32min $s32max"
> +		;;
> +	btrfs)
> +		echo "$s64min $s64max"
>  		;;
>  	*)
>  		echo "-1 -1"
> diff --git a/tests/generic/402 b/tests/generic/402
> index f742fedd..dd136ec2 100755
> --- a/tests/generic/402
> +++ b/tests/generic/402
> @@ -4,15 +4,10 @@
>  #
>  # FS QA Test 402
>  #
> -# Tests to verify policy for filesystem timestamps for
> -# supported ranges:
> -# 1. Verify filesystem rw mount according to sysctl
> -# timestamp_supported.
> -# 2. Verify timestamp clamping for timestamps beyond max
> -# timestamp supported.
> +# Test to verify filesystem timestamps for supported ranges.
>  #
> -# Exit status 1: either or both tests above fail.
> -# Exit status 0: both the above tests pass.
> +# Exit status 1: test failed.
> +# Exit status 0: test passed.
>  #
>  seq=`basename $0`
>  seqres=$RESULT_DIR/$seq
> @@ -49,47 +44,59 @@ check_stat()
>  	prev_timestamp="$timestamp;$timestamp"
>  	if [ $prev_timestamp != $stat_timestamp ]; then
>  		echo "$prev_timestamp != $stat_timestamp" | tee -a $seqres.full
> +		return 1
>  	fi
> +	return 0
>  }
>  
>  run_test_individual()
>  {
> +	fail=0
>  	file=$1
>  	timestamp=$2
>  	update_time=$3
>  
>  	# check if the time needs update
>  	if [ $update_time -eq 1 ]; then
> -		echo "Updating file: $file to timestamp `date -d @$timestamp`"  >> $seqres.full
> +		echo "Updating file: $file to timestamp $timestamp"  >> $seqres.full
>  		$XFS_IO_PROG -f -c "utimes $timestamp 0 $timestamp 0" $file
>  		if [ $? -ne 0 ]; then
>  			echo "Failed to update times on $file" | tee -a $seqres.full
> +			fail=1
>  		fi
>  	fi
>  
> -	tsclamp=$(($timestamp>$tsmax?$tsmax:$timestamp))
> -	echo "Checking file: $file Updated timestamp is `date -d @$tsclamp`"  >> $seqres.full
> -	check_stat $file $tsclamp
> +	tsclamp=$((timestamp<tsmin?tsmin:timestamp>tsmax?tsmax:timestamp))
> +	echo "Checking file: $file Updated timestamp is $tsclamp"  >> $seqres.full
> +	if ! check_stat $file $tsclamp; then
> +		fail=1
> +	fi
> +	return $fail
>  }
>  
>  run_test()
>  {
> +	fail=0
>  	update_time=$1
>  
>  	n=1
>  
>  	for TIME in "${TIMESTAMPS[@]}"; do
> -		run_test_individual ${SCRATCH_MNT}/test_$n $TIME $update_time
> +		if ! run_test_individual ${SCRATCH_MNT}/test_$n $TIME $update_time; then
> +			fail=1
> +		fi
>  		((n++))
>  	done
> +
> +	return $fail
>  }
>  
>  _scratch_mkfs &>> $seqres.full 2>&1 || _fail "mkfs failed"
> -_require_y2038 $SCRATCH_DEV
> +_require_timestamp_range $SCRATCH_DEV
>  
>  read tsmin tsmax <<<$(_filesystem_timestamp_range $SCRATCH_DEV)
> -echo min supported timestamp $tsmin $(date --date=@$tsmin) >> $seqres.full
> -echo max supported timestamp $tsmax $(date --date=@$tsmax) >> $seqres.full
> +echo min supported timestamp $tsmin >> $seqres.full
> +echo max supported timestamp $tsmax >> $seqres.full
>  
>  # Test timestamps array
>  
> @@ -97,45 +104,14 @@ declare -a TIMESTAMPS=(
>  	$tsmin
>  	0
>  	$tsmax
> +	$((tsmax/2))
>  	$((tsmax+1))
> -	4294967295
> -	8589934591
> -	34359738367
>  )
>  
> -# Max timestamp is hardcoded to Mon Jan 18 19:14:07 PST 2038
> -sys_tsmax=2147483647
> -echo "max timestamp that needs to be supported by fs for rw mount is" \
> -	"$((sys_tsmax+1)) $(date --date=@$((sys_tsmax+1)))" >> $seqres.full
> -
> -read ts_check <<<$(cat /proc/sys/fs/fs-timestamp-check-on)
> -
>  _scratch_mount
>  result=$?
>  
> -if [ $ts_check -ne 0 ]; then
> -	echo "sysctl filesystem timestamp check is on" >> $seqres.full
> -	# check for mount failure if the minimum requirement for max timestamp
> -	# supported is not met.
> -	if [ $sys_tsmax -ge $tsmax ]; then
> -		if [ $result -eq 0 ]; then
> -			echo "mount test failed"  | tee -a $seqres.full
> -			exit
> -		fi
> -	else
> -		if [ $result -ne 0 ]; then
> -			echo "failed to mount $SCRATCH_DEV"  | tee -a $seqres.full
> -			exit
> -		fi
> -	fi
> -else
> -	# if sysctl switch is off then mount should succeed always.
> -	echo "sysctl filesystem timestamp check is off" >> $seqres.full
> -	if [ $result -ne 0 ]; then
> -		echo "failed to mount $SCRATCH_DEV and timestamp check is off"  >> $seqres.full
> -		exit
> -	fi
> -fi
> +status=0
>  
>  # Begin test case 1
>  echo "In memory timestamps update test start" >> $seqres.full
> @@ -143,7 +119,9 @@ echo "In memory timestamps update test start" >> $seqres.full
>  # update time on the file
>  update_time=1
>  
> -run_test $update_time
> +if ! run_test $update_time; then
> +	status=1
> +fi
>  
>  echo "In memory timestamps update complete" >> $seqres.full
>  
> @@ -162,12 +140,13 @@ update_time=0
>  echo "On disk timestamps update test start" >> $seqres.full
>  
>  # Re-run test
> -run_test $update_time
> +if ! run_test $update_time; then
> +	status=1
> +fi
>  
>  echo "On disk timestamps update test complete" >> $seqres.full
>  
> -echo "y2038 inode timestamp tests completed successfully"
> +echo "inode timestamp tests completed status $status"
>  
>  # success, all done
> -status=0
> -exit
> +exit $status
> diff --git a/tests/generic/402.out b/tests/generic/402.out
> index 6c5b9308..4500e6c7 100644
> --- a/tests/generic/402.out
> +++ b/tests/generic/402.out
> @@ -1,2 +1,2 @@
>  QA output created by 402
> -y2038 inode timestamp tests completed successfully
> +inode timestamp tests completed status 0
> -- 
> 2.17.1
>
Deepa Dinamani Oct. 2, 2019, 10:06 p.m. UTC | #2
On Sun, Jul 21, 2019 at 9:47 AM Eryu Guan <guaneryu@gmail.com> wrote:
>
> On Thu, Jul 18, 2019 at 09:12:31PM -0700, Deepa Dinamani wrote:
> > The mount behavior will not be altered because of the unsupported
> > timestamps on the filesystems.
>
> It'd be better to provide more details in the commit log, e.g. what
> kind of mount behavior and what's the changes being made to the tests.
>
> >
> > Adjust the test accordingly.
> >
> > An updated series to be posted after the merge window is hosted at
> > <https://github.com/deepa-hub/vfs/tree/limits>
> >
> > Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
>
> Thanks for the update! But I'd like to wait for the kernel patches
> merged into mainline first. Would you please send out a notification by
> replying this thread then? Thanks a lot!

This series has been merged now:
https://git.kernel.org/torvalds/c/cfb82e1df8b7c76991ea12958855897c2fb4debc

-Deepa
Eryu Guan Oct. 5, 2019, 6:35 p.m. UTC | #3
On Thu, Jul 18, 2019 at 09:12:31PM -0700, Deepa Dinamani wrote:
> The mount behavior will not be altered because of the unsupported
> timestamps on the filesystems.
> 
> Adjust the test accordingly.

Thanks for the heads-up about the merge of the fixes

https://git.kernel.org/torvalds/c/cfb82e1df8b7c76991ea12958855897c2fb4debc

> 
> An updated series to be posted after the merge window is hosted at
> <https://github.com/deepa-hub/vfs/tree/limits>
> 
> Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
>
> ---
>  common/rc             | 36 +++++++++---------
>  tests/generic/402     | 87 ++++++++++++++++---------------------------
>  tests/generic/402.out |  2 +-
>  3 files changed, 53 insertions(+), 72 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 25203bb4..39a2deb0 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1959,16 +1959,9 @@ _run_aiodio()
>      return $status
>  }
>  
> -# this test requires y2038 sysfs switch and filesystem
> -# timestamp ranges support.
> -_require_y2038()
> +_require_timestamp_range()
>  {
>  	local device=${1:-$TEST_DEV}
> -	local sysfsdir=/proc/sys/fs/fs-timestamp-check-on
> -
> -	if [ ! -e $sysfsdir ]; then
> -		_notrun "no kernel support for y2038 sysfs switch"
> -	fi
>  
>  	local tsmin tsmax
>  	read tsmin tsmax <<<$(_filesystem_timestamp_range $device)
> @@ -1980,23 +1973,32 @@ _require_y2038()
>  _filesystem_timestamp_range()
>  {
>  	local device=${1:-$TEST_DEV}
> +	u32max=$(((1<<32)-1))
> +	s32min=-$((1<<31))
> +	s32max=$(((1<<31)-1))
> +	s64max=$(((1<<63)-1))
> +	s64min=$((1<<63))
> +
>  	case $FSTYP in
> -	ext4)
> +	ext2)
> +		echo "$s32min $s32max"
> +		;;
> +	ext3|ext4)
>  		if [ $(dumpe2fs -h $device 2>/dev/null | grep "Inode size:" | cut -d: -f2) -gt 128 ]; then
> -			echo "-2147483648 15032385535"
> +			printf "%d %d\n" $s32min 0x37fffffff
>  		else
> -			echo "-2147483648 2147483647"
> +			echo "$s32min $s32max"
>  		fi
>  		;;
>  
> -	xfs)
> -		echo "-2147483648 2147483647"
> -		;;
>  	jfs)
> -		echo "0 4294967295"
> +		echo "0 $u32max"
>  		;;
> -	f2fs)
> -		echo "-2147483648 2147483647"
> +	xfs)
> +		echo "$s32min $s32max"
> +		;;
> +	btrfs)
> +		echo "$s64min $s64max"
>  		;;
>  	*)
>  		echo "-1 -1"
> diff --git a/tests/generic/402 b/tests/generic/402
> index f742fedd..dd136ec2 100755
> --- a/tests/generic/402
> +++ b/tests/generic/402
> @@ -4,15 +4,10 @@
>  #
>  # FS QA Test 402
>  #
> -# Tests to verify policy for filesystem timestamps for
> -# supported ranges:
> -# 1. Verify filesystem rw mount according to sysctl
> -# timestamp_supported.
> -# 2. Verify timestamp clamping for timestamps beyond max
> -# timestamp supported.
> +# Test to verify filesystem timestamps for supported ranges.
>  #
> -# Exit status 1: either or both tests above fail.
> -# Exit status 0: both the above tests pass.
> +# Exit status 1: test failed.
> +# Exit status 0: test passed.

These exit status checks are not needed, please see below.

>  #
>  seq=`basename $0`
>  seqres=$RESULT_DIR/$seq
> @@ -49,47 +44,59 @@ check_stat()
>  	prev_timestamp="$timestamp;$timestamp"
>  	if [ $prev_timestamp != $stat_timestamp ]; then
>  		echo "$prev_timestamp != $stat_timestamp" | tee -a $seqres.full
> +		return 1

We already print error message on test failure, which will break the
golden image, so there's no need to return 0 or 1. All similar checks
and returns are not needed in other functions.

Thanks,
Eryu

>  	fi
> +	return 0
>  }
>  
>  run_test_individual()
>  {
> +	fail=0
>  	file=$1
>  	timestamp=$2
>  	update_time=$3
>  
>  	# check if the time needs update
>  	if [ $update_time -eq 1 ]; then
> -		echo "Updating file: $file to timestamp `date -d @$timestamp`"  >> $seqres.full
> +		echo "Updating file: $file to timestamp $timestamp"  >> $seqres.full
>  		$XFS_IO_PROG -f -c "utimes $timestamp 0 $timestamp 0" $file
>  		if [ $? -ne 0 ]; then
>  			echo "Failed to update times on $file" | tee -a $seqres.full
> +			fail=1
>  		fi
>  	fi
>  
> -	tsclamp=$(($timestamp>$tsmax?$tsmax:$timestamp))
> -	echo "Checking file: $file Updated timestamp is `date -d @$tsclamp`"  >> $seqres.full
> -	check_stat $file $tsclamp
> +	tsclamp=$((timestamp<tsmin?tsmin:timestamp>tsmax?tsmax:timestamp))
> +	echo "Checking file: $file Updated timestamp is $tsclamp"  >> $seqres.full
> +	if ! check_stat $file $tsclamp; then
> +		fail=1
> +	fi
> +	return $fail
>  }
>  
>  run_test()
>  {
> +	fail=0
>  	update_time=$1
>  
>  	n=1
>  
>  	for TIME in "${TIMESTAMPS[@]}"; do
> -		run_test_individual ${SCRATCH_MNT}/test_$n $TIME $update_time
> +		if ! run_test_individual ${SCRATCH_MNT}/test_$n $TIME $update_time; then
> +			fail=1
> +		fi
>  		((n++))
>  	done
> +
> +	return $fail
>  }
>  
>  _scratch_mkfs &>> $seqres.full 2>&1 || _fail "mkfs failed"
> -_require_y2038 $SCRATCH_DEV
> +_require_timestamp_range $SCRATCH_DEV
>  
>  read tsmin tsmax <<<$(_filesystem_timestamp_range $SCRATCH_DEV)
> -echo min supported timestamp $tsmin $(date --date=@$tsmin) >> $seqres.full
> -echo max supported timestamp $tsmax $(date --date=@$tsmax) >> $seqres.full
> +echo min supported timestamp $tsmin >> $seqres.full
> +echo max supported timestamp $tsmax >> $seqres.full
>  
>  # Test timestamps array
>  
> @@ -97,45 +104,14 @@ declare -a TIMESTAMPS=(
>  	$tsmin
>  	0
>  	$tsmax
> +	$((tsmax/2))
>  	$((tsmax+1))
> -	4294967295
> -	8589934591
> -	34359738367
>  )
>  
> -# Max timestamp is hardcoded to Mon Jan 18 19:14:07 PST 2038
> -sys_tsmax=2147483647
> -echo "max timestamp that needs to be supported by fs for rw mount is" \
> -	"$((sys_tsmax+1)) $(date --date=@$((sys_tsmax+1)))" >> $seqres.full
> -
> -read ts_check <<<$(cat /proc/sys/fs/fs-timestamp-check-on)
> -
>  _scratch_mount
>  result=$?
>  
> -if [ $ts_check -ne 0 ]; then
> -	echo "sysctl filesystem timestamp check is on" >> $seqres.full
> -	# check for mount failure if the minimum requirement for max timestamp
> -	# supported is not met.
> -	if [ $sys_tsmax -ge $tsmax ]; then
> -		if [ $result -eq 0 ]; then
> -			echo "mount test failed"  | tee -a $seqres.full
> -			exit
> -		fi
> -	else
> -		if [ $result -ne 0 ]; then
> -			echo "failed to mount $SCRATCH_DEV"  | tee -a $seqres.full
> -			exit
> -		fi
> -	fi
> -else
> -	# if sysctl switch is off then mount should succeed always.
> -	echo "sysctl filesystem timestamp check is off" >> $seqres.full
> -	if [ $result -ne 0 ]; then
> -		echo "failed to mount $SCRATCH_DEV and timestamp check is off"  >> $seqres.full
> -		exit
> -	fi
> -fi
> +status=0
>  
>  # Begin test case 1
>  echo "In memory timestamps update test start" >> $seqres.full
> @@ -143,7 +119,9 @@ echo "In memory timestamps update test start" >> $seqres.full
>  # update time on the file
>  update_time=1
>  
> -run_test $update_time
> +if ! run_test $update_time; then
> +	status=1
> +fi
>  
>  echo "In memory timestamps update complete" >> $seqres.full
>  
> @@ -162,12 +140,13 @@ update_time=0
>  echo "On disk timestamps update test start" >> $seqres.full
>  
>  # Re-run test
> -run_test $update_time
> +if ! run_test $update_time; then
> +	status=1
> +fi
>  
>  echo "On disk timestamps update test complete" >> $seqres.full
>  
> -echo "y2038 inode timestamp tests completed successfully"
> +echo "inode timestamp tests completed status $status"
>  
>  # success, all done
> -status=0
> -exit
> +exit $status
> diff --git a/tests/generic/402.out b/tests/generic/402.out
> index 6c5b9308..4500e6c7 100644
> --- a/tests/generic/402.out
> +++ b/tests/generic/402.out
> @@ -1,2 +1,2 @@
>  QA output created by 402
> -y2038 inode timestamp tests completed successfully
> +inode timestamp tests completed status 0
> -- 
> 2.17.1
>
Deepa Dinamani Oct. 23, 2019, 10:17 p.m. UTC | #4
On Sat, Oct 5, 2019 at 11:35 AM Eryu Guan <guaneryu@gmail.com> wrote:
> We already print error message on test failure, which will break the
> golden image, so there's no need to return 0 or 1. All similar checks
> and returns are not needed in other functions.

I removed the status checks and associated returns.

The patch is posted at
https://lore.kernel.org/fstests/20191023220401.12335-1-deepa.kernel@gmail.com/

-Deepa
Amir Goldstein Dec. 12, 2019, 1:11 p.m. UTC | #5
On Fri, Jul 19, 2019 at 7:21 AM Deepa Dinamani <deepa.kernel@gmail.com> wrote:
>
> The mount behavior will not be altered because of the unsupported
> timestamps on the filesystems.
>
> Adjust the test accordingly.
>
> An updated series to be posted after the merge window is hosted at
> <https://github.com/deepa-hub/vfs/tree/limits>
>
> Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
> ---
>  common/rc             | 36 +++++++++---------
>  tests/generic/402     | 87 ++++++++++++++++---------------------------
>  tests/generic/402.out |  2 +-
>  3 files changed, 53 insertions(+), 72 deletions(-)
>
> diff --git a/common/rc b/common/rc
> index 25203bb4..39a2deb0 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1959,16 +1959,9 @@ _run_aiodio()
>      return $status
>  }
>
> -# this test requires y2038 sysfs switch and filesystem
> -# timestamp ranges support.
> -_require_y2038()
> +_require_timestamp_range()
>  {
>         local device=${1:-$TEST_DEV}
> -       local sysfsdir=/proc/sys/fs/fs-timestamp-check-on
> -
> -       if [ ! -e $sysfsdir ]; then
> -               _notrun "no kernel support for y2038 sysfs switch"
> -       fi
>

Deepa,

This change, which is already merged removed the test for kernel support
and replaced it with test only for filesystem support.

This impacts people validating stable kernel releases with xfstest, because
this test now always fails on stable kernels and I don't think that timestamp
clamping behavior is going to stable kernels.

Of course stable kernel testers can exclude this test, but this will remove test
coverage and may result in silent breakage of > y2038 timetamps in stable
kernels.

I think test should identify if kernel had clamping behavior and change the
expected timestamp values accordingly, similar to how the test was before
adjusting it to new behavior.

Do you agree? Can you make these changes?

Thanks,
Amir.
Deepa Dinamani Dec. 12, 2019, 9:55 p.m. UTC | #6
> >  {
> >         local device=${1:-$TEST_DEV}
> > -       local sysfsdir=/proc/sys/fs/fs-timestamp-check-on
> > -
> > -       if [ ! -e $sysfsdir ]; then
> > -               _notrun "no kernel support for y2038 sysfs switch"
> > -       fi
> >
>
> Deepa,
>
> This change, which is already merged removed the test for kernel support
> and replaced it with test only for filesystem support.
>
> This impacts people validating stable kernel releases with xfstest, because
> this test now always fails on stable kernels and I don't think that timestamp
> clamping behavior is going to stable kernels.
>
> Of course stable kernel testers can exclude this test, but this will remove test
> coverage and may result in silent breakage of > y2038 timetamps in stable
> kernels.
>
> I think test should identify if kernel had clamping behavior and change the
> expected timestamp values accordingly, similar to how the test was before
> adjusting it to new behavior.
>
> Do you agree? Can you make these changes?

Initially, we were going to allow rw mounts for filesystems which
could not update timestamps. And, this was a big change in behavior.
That is why we had the behavior exposed from the kernel.
I wasn't aware that the failing fstests would cause such a nuisence.
Since everybody seems to just rely on all the fstests passing, yes I
agree that bringing this back would be the easiest to not fail on
stable kernels.
I will post the kernel and the xfstest patch.

Thanks for helping me catch it.

-Deepa
Deepa Dinamani Dec. 18, 2019, 8:21 p.m. UTC | #7
I looked at this more closely. Here is the patch that added the sysctl
to the kernel previously: https://lkml.org/lkml/2016/11/2/300.

This was meant to be configurable earlier. That is why this made
sense. But, now it is not. We unconditionally clamp to the fs limits.
I looked around to see if we ever expose information about internal
kernel changes to userspace. This is almost never done. And, this is
always in the form of maybe a syscall failing. Given that we don't see
any modified behaviour that the user can point out, I don't think we
can expose the presence of clamping in the kernel.

fsinfo though exposes a fs max and min that could be useful if we fill
in an unknown pattern as default:
https://lwn.net/ml/linux-api/153314004147.18964.11925284995448007945.stgit@warthog.procyon.org.uk/.

I also spoke about this to Arnd, and he also suggested the fsinfo as
an alternative.

Is it easy to not run the test on older kernels? Otherwise, we just
have to rely on fsinfo being merged?

-Deepa
Amir Goldstein Dec. 18, 2019, 8:46 p.m. UTC | #8
On Wed, Dec 18, 2019 at 10:21 PM Deepa Dinamani <deepa.kernel@gmail.com> wrote:
>
> I looked at this more closely. Here is the patch that added the sysctl
> to the kernel previously: https://lkml.org/lkml/2016/11/2/300.
>
> This was meant to be configurable earlier. That is why this made
> sense. But, now it is not. We unconditionally clamp to the fs limits.
> I looked around to see if we ever expose information about internal
> kernel changes to userspace. This is almost never done. And, this is
> always in the form of maybe a syscall failing. Given that we don't see
> any modified behaviour that the user can point out, I don't think we
> can expose the presence of clamping in the kernel.
>
> fsinfo though exposes a fs max and min that could be useful if we fill
> in an unknown pattern as default:
> https://lwn.net/ml/linux-api/153314004147.18964.11925284995448007945.stgit@warthog.procyon.org.uk/.
>
> I also spoke about this to Arnd, and he also suggested the fsinfo as
> an alternative.
>
> Is it easy to not run the test on older kernels? Otherwise, we just
> have to rely on fsinfo being merged?
>

Is it easy to blacklist the test if that is what you are asking.
How people run their stable kernel tests I don't know.
I believe Sasha is running xfstests to validate stable release
candidate patches.

I don't think there is a clear policy about being friendly to testing
less that master kernels in xfstest (Eryu?), but IMO we should try to
accommodate
this use case, because it is in the best interest of everyone that stable kernel
will be regularly tested with xfstests with as little noisy failures
as possible.

Thanks,
Amir.
Arnd Bergmann Dec. 19, 2019, 8:28 a.m. UTC | #9
On Wed, Dec 18, 2019 at 9:46 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> I don't think there is a clear policy about being friendly to testing
> less that master kernels in xfstest (Eryu?), but IMO we should try to
> accommodate
> this use case, because it is in the best interest of everyone that stable kernel
> will be regularly tested with xfstests with as little noisy failures
> as possible.

I think what makes this one particularly hard is that there are most likely
people that do care about the failure on older kernels being reported and
would rather backport the kernel changes into their product kernels
to have them behave sanely.

I'm also not sure if we could just backport the changes to stable
kernels after all.

Greg, do you have an opinion on whether the 19 patches from
v5.3-rc6 to cba465b4f982 can be considered stable material?

The best argument that I have seen in favor of treating it as a bugfix
is that the posx man pages require that "The file's relevant timestamp shall
be set to the greatest value supported by the file system that is not greater
than the specified time"[1], and this is something that Linux has always
done wrong before the series (we overflow and underflow out-of-range
arguments to a value that is both file system and CPU architecture
specific).

The main argument against backporting would be that 19 patches
is too much, I think each patch in the series would qualify on its own.
Changing the layout of 'struct super_block' also breaks the module
binary interface, which will annoy some distros that care about this,
but I don't think it's stopping us from adding the patch to a stable
kernel.

       Arnd

[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/futimens.html
Greg Kroah-Hartman Dec. 19, 2019, 8:40 a.m. UTC | #10
On Thu, Dec 19, 2019 at 09:28:23AM +0100, Arnd Bergmann wrote:
> On Wed, Dec 18, 2019 at 9:46 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > I don't think there is a clear policy about being friendly to testing
> > less that master kernels in xfstest (Eryu?), but IMO we should try to
> > accommodate
> > this use case, because it is in the best interest of everyone that stable kernel
> > will be regularly tested with xfstests with as little noisy failures
> > as possible.
> 
> I think what makes this one particularly hard is that there are most likely
> people that do care about the failure on older kernels being reported and
> would rather backport the kernel changes into their product kernels
> to have them behave sanely.
> 
> I'm also not sure if we could just backport the changes to stable
> kernels after all.
> 
> Greg, do you have an opinion on whether the 19 patches from
> v5.3-rc6 to cba465b4f982 can be considered stable material?
> 
> The best argument that I have seen in favor of treating it as a bugfix
> is that the posx man pages require that "The file's relevant timestamp shall
> be set to the greatest value supported by the file system that is not greater
> than the specified time"[1], and this is something that Linux has always
> done wrong before the series (we overflow and underflow out-of-range
> arguments to a value that is both file system and CPU architecture
> specific).
> 
> The main argument against backporting would be that 19 patches
> is too much, I think each patch in the series would qualify on its own.
> Changing the layout of 'struct super_block' also breaks the module
> binary interface, which will annoy some distros that care about this,
> but I don't think it's stopping us from adding the patch to a stable
> kernel.
> 
>        Arnd
> 
> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/futimens.html

Ugh, that's a mess.  Why not just use 5.4 if people really care about
this type of thing?

But yes, on their own, each individual patch would be fine for stable,
it's just that I would want someone to "own" the backport and testing of
such a thing.  If for no other reason than to have someone to "blame"
for when things go wrong and get them to fix up the fallout :)

Who really really wants this in their older kernels?  And are those same
people already taking all of the stable updates for those kernels as
well?

thanks,

greg k-h
Arnd Bergmann Dec. 19, 2019, 11:29 a.m. UTC | #11
On Thu, Dec 19, 2019 at 9:40 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Dec 19, 2019 at 09:28:23AM +0100, Arnd Bergmann wrote:
> > On Wed, Dec 18, 2019 at 9:46 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/futimens.html
>
> Ugh, that's a mess.  Why not just use 5.4 if people really care about
> this type of thing?
>
> But yes, on their own, each individual patch would be fine for stable,
> it's just that I would want someone to "own" the backport and testing of
> such a thing.  If for no other reason than to have someone to "blame"
> for when things go wrong and get them to fix up the fallout :)

I was going to volunteer Deepa and me, but I just tried out what a backport
would look like, and backporting to v4.14 or earlier would involve a
major rewrite unless we also backport Deepa's earlier y2038 patches that
are much more invasive. Backporting to v4.19 (across the mount API
change) would be possible, but this doesn't really help the cause of
getting xfstests to report correct behavior on all stable kernels.

> Who really really wants this in their older kernels?  And are those same
> people already taking all of the stable updates for those kernels as
> well?

I see two potential groups of people:

- the one that started this thread: xfstests correctly reports a failure on
  stable kernels that have a known problem with compliance. If you are
  aiming for 100% pass rate on a test suite, you can either mark a correct
  test case as "skip", or backport the fix. Neither one is super attractive
  here, but it seemed worth considering which one is more harmful. (I
  guess I answered that now -- backporting to v4.14 would be more
  harmful)

- Users of CIP SLTS kernels with extreme service life that may involve
  not upgrading until after y2038 (this is obviously not recommended if
  you connect to a public network, but I'm sure some people do this anyway).
  For running user space, this requires either a 32-bit kernel with the
  linux-5.1 syscall changes or a 64-bit kernel. If you run a 64-bit linux-4.9
  kernel in a deeply embedded non-networked machine, it still makes
  sense to have working inode timestamps and be able to test that.

It may still make sense to backport this to linux-4.19.y-cip or another
downstream version of 4.19, but let's not do it for the normal LTS
kernels then.

       Arnd
Greg Kroah-Hartman Dec. 19, 2019, 11:35 a.m. UTC | #12
On Thu, Dec 19, 2019 at 12:29:39PM +0100, Arnd Bergmann wrote:
> On Thu, Dec 19, 2019 at 9:40 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Thu, Dec 19, 2019 at 09:28:23AM +0100, Arnd Bergmann wrote:
> > > On Wed, Dec 18, 2019 at 9:46 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/futimens.html
> >
> > Ugh, that's a mess.  Why not just use 5.4 if people really care about
> > this type of thing?
> >
> > But yes, on their own, each individual patch would be fine for stable,
> > it's just that I would want someone to "own" the backport and testing of
> > such a thing.  If for no other reason than to have someone to "blame"
> > for when things go wrong and get them to fix up the fallout :)
> 
> I was going to volunteer Deepa and me, but I just tried out what a backport
> would look like, and backporting to v4.14 or earlier would involve a
> major rewrite unless we also backport Deepa's earlier y2038 patches that
> are much more invasive. Backporting to v4.19 (across the mount API
> change) would be possible, but this doesn't really help the cause of
> getting xfstests to report correct behavior on all stable kernels.
> 
> > Who really really wants this in their older kernels?  And are those same
> > people already taking all of the stable updates for those kernels as
> > well?
> 
> I see two potential groups of people:
> 
> - the one that started this thread: xfstests correctly reports a failure on
>   stable kernels that have a known problem with compliance. If you are
>   aiming for 100% pass rate on a test suite, you can either mark a correct
>   test case as "skip", or backport the fix. Neither one is super attractive
>   here, but it seemed worth considering which one is more harmful. (I
>   guess I answered that now -- backporting to v4.14 would be more
>   harmful)

Marking the test as "skip" for older kernels should be just fine for
this.

> - Users of CIP SLTS kernels with extreme service life that may involve
>   not upgrading until after y2038 (this is obviously not recommended if
>   you connect to a public network, but I'm sure some people do this anyway).
>   For running user space, this requires either a 32-bit kernel with the
>   linux-5.1 syscall changes or a 64-bit kernel. If you run a 64-bit linux-4.9
>   kernel in a deeply embedded non-networked machine, it still makes
>   sense to have working inode timestamps and be able to test that.
> 
> It may still make sense to backport this to linux-4.19.y-cip or another
> downstream version of 4.19, but let's not do it for the normal LTS
> kernels then.

CIP is in for a world of hurt for a lot of other reasons, all
self-inflicted :)

I'll let those people deal with the fallout of their odd decisions, but
I will quote a company that is supporting Linux devices in the wild for
20+ years:
	We update the kernel and all software on them for at least 18
	years as these devices are "alive", why would we declare them
	"dead" right away and only provide triage?  That's an insane way
	to support a product.

So let's just leave this as-is, 5.4 should be fine for people worrying
about y2038.

thanks,

greg k-h
Amir Goldstein Dec. 19, 2019, 12:09 p.m. UTC | #13
On Thu, Dec 19, 2019 at 10:28 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Dec 18, 2019 at 9:46 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > I don't think there is a clear policy about being friendly to testing
> > less that master kernels in xfstest (Eryu?), but IMO we should try to
> > accommodate
> > this use case, because it is in the best interest of everyone that stable kernel
> > will be regularly tested with xfstests with as little noisy failures
> > as possible.
>
> I think what makes this one particularly hard is that there are most likely
> people that do care about the failure on older kernels being reported and
> would rather backport the kernel changes into their product kernels
> to have them behave sanely.

Getting back to the thread before it diverged into the backport option.

The test used to detect kernel support and be skipped automatically on
old kernel
and now the test fails because the kernel knob and test for it were removed.

I perceive that as a regression to the test.
I don't mind waiting for fsinfo() to fix this regression, as long as
fsinfo() is going to
be backported to stable kernel 5.4???

Deepa,

You added this warning:
pr_warn("Mounted %s file system at %s supports timestamps until ...
along with timestamp clamping

I suggest that you implement kernel support check based on grepping
for this warning after loop mounting an ext2 test image.
A bit over the top and ugly, but the test should be reliable and
mkfs.ext2 is probably available in all xfstest deployments.

xfs/049 makes use of an ext2 loop mount, so your test won't be the
first one to use ext2 for testing other fs.

When kernel gets fsinfo() the test for kernel support can be improved
to using fsinfo() before doing the ext2 loop mount hack.

Thanks,
Amir.
Ben Hutchings Dec. 19, 2019, 3:48 p.m. UTC | #14
[+cc cip-dev]

On Thu, 2019-12-19 at 12:29 +0100, Arnd Bergmann wrote:
[...]
> - Users of CIP SLTS kernels with extreme service life that may involve
>   not upgrading until after y2038 (this is obviously not recommended if
>   you connect to a public network, but I'm sure some people do this anyway).
>   For running user space, this requires either a 32-bit kernel with the
>   linux-5.1 syscall changes or a 64-bit kernel. If you run a 64-bit linux-4.9
>   kernel in a deeply embedded non-networked machine, it still makes
>   sense to have working inode timestamps and be able to test that.
[...]

CIP is currently aiming for a 10 year support lifetime, so both of its
currently existing branches (4.4, 4.19) should be long out of support
in 2038.  Still, it's possible that some people hope to extend that
later.

Ben.
Arnd Bergmann Dec. 19, 2019, 8:35 p.m. UTC | #15
On Thu, Dec 19, 2019 at 4:48 PM Ben Hutchings
<ben.hutchings@codethink.co.uk> wrote:
> On Thu, 2019-12-19 at 12:29 +0100, Arnd Bergmann wrote:
> [...]
> > - Users of CIP SLTS kernels with extreme service life that may involve
> >   not upgrading until after y2038 (this is obviously not recommended if
> >   you connect to a public network, but I'm sure some people do this anyway).
> >   For running user space, this requires either a 32-bit kernel with the
> >   linux-5.1 syscall changes or a 64-bit kernel. If you run a 64-bit linux-4.9
> >   kernel in a deeply embedded non-networked machine, it still makes
> >   sense to have working inode timestamps and be able to test that.
> [...]
>
> CIP is currently aiming for a 10 year support lifetime, so both of its
> currently existing branches (4.4, 4.19) should be long out of support
> in 2038.  Still, it's possible that some people hope to extend that
> later.

It is also common that end-users keep relying on machines that they
bought for many years after any support contracts end. This may be
fine for a lot of use cases where there is no risk in running an old
operating system, and replacing it is prohibitively expensive, as
software generally does not suddenly stop working after support ends.

With the y2038-safe interfaces and with file system limits, the
difference is that there is a specific point in time when things will
break.

        Arnd
Deepa Dinamani Dec. 20, 2019, 10:45 p.m. UTC | #16
> Deepa,
>
> You added this warning:
> pr_warn("Mounted %s file system at %s supports timestamps until ...
> along with timestamp clamping
>
> I suggest that you implement kernel support check based on grepping
> for this warning after loop mounting an ext2 test image.
> A bit over the top and ugly, but the test should be reliable and
> mkfs.ext2 is probably available in all xfstest deployments.
>
> xfs/049 makes use of an ext2 loop mount, so your test won't be the
> first one to use ext2 for testing other fs.

Ok, this can work. Let me give this a try.

Thanks,
Deepa

Patch
diff mbox series

diff --git a/common/rc b/common/rc
index 25203bb4..39a2deb0 100644
--- a/common/rc
+++ b/common/rc
@@ -1959,16 +1959,9 @@  _run_aiodio()
     return $status
 }
 
-# this test requires y2038 sysfs switch and filesystem
-# timestamp ranges support.
-_require_y2038()
+_require_timestamp_range()
 {
 	local device=${1:-$TEST_DEV}
-	local sysfsdir=/proc/sys/fs/fs-timestamp-check-on
-
-	if [ ! -e $sysfsdir ]; then
-		_notrun "no kernel support for y2038 sysfs switch"
-	fi
 
 	local tsmin tsmax
 	read tsmin tsmax <<<$(_filesystem_timestamp_range $device)
@@ -1980,23 +1973,32 @@  _require_y2038()
 _filesystem_timestamp_range()
 {
 	local device=${1:-$TEST_DEV}
+	u32max=$(((1<<32)-1))
+	s32min=-$((1<<31))
+	s32max=$(((1<<31)-1))
+	s64max=$(((1<<63)-1))
+	s64min=$((1<<63))
+
 	case $FSTYP in
-	ext4)
+	ext2)
+		echo "$s32min $s32max"
+		;;
+	ext3|ext4)
 		if [ $(dumpe2fs -h $device 2>/dev/null | grep "Inode size:" | cut -d: -f2) -gt 128 ]; then
-			echo "-2147483648 15032385535"
+			printf "%d %d\n" $s32min 0x37fffffff
 		else
-			echo "-2147483648 2147483647"
+			echo "$s32min $s32max"
 		fi
 		;;
 
-	xfs)
-		echo "-2147483648 2147483647"
-		;;
 	jfs)
-		echo "0 4294967295"
+		echo "0 $u32max"
 		;;
-	f2fs)
-		echo "-2147483648 2147483647"
+	xfs)
+		echo "$s32min $s32max"
+		;;
+	btrfs)
+		echo "$s64min $s64max"
 		;;
 	*)
 		echo "-1 -1"
diff --git a/tests/generic/402 b/tests/generic/402
index f742fedd..dd136ec2 100755
--- a/tests/generic/402
+++ b/tests/generic/402
@@ -4,15 +4,10 @@ 
 #
 # FS QA Test 402
 #
-# Tests to verify policy for filesystem timestamps for
-# supported ranges:
-# 1. Verify filesystem rw mount according to sysctl
-# timestamp_supported.
-# 2. Verify timestamp clamping for timestamps beyond max
-# timestamp supported.
+# Test to verify filesystem timestamps for supported ranges.
 #
-# Exit status 1: either or both tests above fail.
-# Exit status 0: both the above tests pass.
+# Exit status 1: test failed.
+# Exit status 0: test passed.
 #
 seq=`basename $0`
 seqres=$RESULT_DIR/$seq
@@ -49,47 +44,59 @@  check_stat()
 	prev_timestamp="$timestamp;$timestamp"
 	if [ $prev_timestamp != $stat_timestamp ]; then
 		echo "$prev_timestamp != $stat_timestamp" | tee -a $seqres.full
+		return 1
 	fi
+	return 0
 }
 
 run_test_individual()
 {
+	fail=0
 	file=$1
 	timestamp=$2
 	update_time=$3
 
 	# check if the time needs update
 	if [ $update_time -eq 1 ]; then
-		echo "Updating file: $file to timestamp `date -d @$timestamp`"  >> $seqres.full
+		echo "Updating file: $file to timestamp $timestamp"  >> $seqres.full
 		$XFS_IO_PROG -f -c "utimes $timestamp 0 $timestamp 0" $file
 		if [ $? -ne 0 ]; then
 			echo "Failed to update times on $file" | tee -a $seqres.full
+			fail=1
 		fi
 	fi
 
-	tsclamp=$(($timestamp>$tsmax?$tsmax:$timestamp))
-	echo "Checking file: $file Updated timestamp is `date -d @$tsclamp`"  >> $seqres.full
-	check_stat $file $tsclamp
+	tsclamp=$((timestamp<tsmin?tsmin:timestamp>tsmax?tsmax:timestamp))
+	echo "Checking file: $file Updated timestamp is $tsclamp"  >> $seqres.full
+	if ! check_stat $file $tsclamp; then
+		fail=1
+	fi
+	return $fail
 }
 
 run_test()
 {
+	fail=0
 	update_time=$1
 
 	n=1
 
 	for TIME in "${TIMESTAMPS[@]}"; do
-		run_test_individual ${SCRATCH_MNT}/test_$n $TIME $update_time
+		if ! run_test_individual ${SCRATCH_MNT}/test_$n $TIME $update_time; then
+			fail=1
+		fi
 		((n++))
 	done
+
+	return $fail
 }
 
 _scratch_mkfs &>> $seqres.full 2>&1 || _fail "mkfs failed"
-_require_y2038 $SCRATCH_DEV
+_require_timestamp_range $SCRATCH_DEV
 
 read tsmin tsmax <<<$(_filesystem_timestamp_range $SCRATCH_DEV)
-echo min supported timestamp $tsmin $(date --date=@$tsmin) >> $seqres.full
-echo max supported timestamp $tsmax $(date --date=@$tsmax) >> $seqres.full
+echo min supported timestamp $tsmin >> $seqres.full
+echo max supported timestamp $tsmax >> $seqres.full
 
 # Test timestamps array
 
@@ -97,45 +104,14 @@  declare -a TIMESTAMPS=(
 	$tsmin
 	0
 	$tsmax
+	$((tsmax/2))
 	$((tsmax+1))
-	4294967295
-	8589934591
-	34359738367
 )
 
-# Max timestamp is hardcoded to Mon Jan 18 19:14:07 PST 2038
-sys_tsmax=2147483647
-echo "max timestamp that needs to be supported by fs for rw mount is" \
-	"$((sys_tsmax+1)) $(date --date=@$((sys_tsmax+1)))" >> $seqres.full
-
-read ts_check <<<$(cat /proc/sys/fs/fs-timestamp-check-on)
-
 _scratch_mount
 result=$?
 
-if [ $ts_check -ne 0 ]; then
-	echo "sysctl filesystem timestamp check is on" >> $seqres.full
-	# check for mount failure if the minimum requirement for max timestamp
-	# supported is not met.
-	if [ $sys_tsmax -ge $tsmax ]; then
-		if [ $result -eq 0 ]; then
-			echo "mount test failed"  | tee -a $seqres.full
-			exit
-		fi
-	else
-		if [ $result -ne 0 ]; then
-			echo "failed to mount $SCRATCH_DEV"  | tee -a $seqres.full
-			exit
-		fi
-	fi
-else
-	# if sysctl switch is off then mount should succeed always.
-	echo "sysctl filesystem timestamp check is off" >> $seqres.full
-	if [ $result -ne 0 ]; then
-		echo "failed to mount $SCRATCH_DEV and timestamp check is off"  >> $seqres.full
-		exit
-	fi
-fi
+status=0
 
 # Begin test case 1
 echo "In memory timestamps update test start" >> $seqres.full
@@ -143,7 +119,9 @@  echo "In memory timestamps update test start" >> $seqres.full
 # update time on the file
 update_time=1
 
-run_test $update_time
+if ! run_test $update_time; then
+	status=1
+fi
 
 echo "In memory timestamps update complete" >> $seqres.full
 
@@ -162,12 +140,13 @@  update_time=0
 echo "On disk timestamps update test start" >> $seqres.full
 
 # Re-run test
-run_test $update_time
+if ! run_test $update_time; then
+	status=1
+fi
 
 echo "On disk timestamps update test complete" >> $seqres.full
 
-echo "y2038 inode timestamp tests completed successfully"
+echo "inode timestamp tests completed status $status"
 
 # success, all done
-status=0
-exit
+exit $status
diff --git a/tests/generic/402.out b/tests/generic/402.out
index 6c5b9308..4500e6c7 100644
--- a/tests/generic/402.out
+++ b/tests/generic/402.out
@@ -1,2 +1,2 @@ 
 QA output created by 402
-y2038 inode timestamp tests completed successfully
+inode timestamp tests completed status 0