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
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
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
echo "-2147483648 2147483647"
echo "$s32min $s32max"
- xfs)
echo "-2147483648 2147483647";;echo "0 4294967295"
echo "0 $u32max"
- f2fs)
echo "-2147483648 2147483647"
- xfs)
echo "$s32min $s32max";;- btrfs)
echo "$s64min $s64max"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- 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.fullfail=1
- 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; thenfail=1fi- 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 ]; thenecho "mount test failed" | tee -a $seqres.fullexitfi- else
if [ $result -ne 0 ]; thenecho "failed to mount $SCRATCH_DEV" | tee -a $seqres.fullexitfi- 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.fullexit- 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
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
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
echo "-2147483648 2147483647"
echo "$s32min $s32max"
- xfs)
echo "-2147483648 2147483647";;echo "0 4294967295"
echo "0 $u32max"
- f2fs)
echo "-2147483648 2147483647"
- xfs)
echo "$s32min $s32max";;- btrfs)
echo "$s64min $s64max"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.fullfail=1
- 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; thenfail=1fi- 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 ]; thenecho "mount test failed" | tee -a $seqres.fullexitfi- else
if [ $result -ne 0 ]; thenecho "failed to mount $SCRATCH_DEV" | tee -a $seqres.fullexitfi- 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.fullexit- 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
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.co...
- Deepa
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.co...
-Deepa
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-onif [ ! -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.
{ local device=${1:-$TEST_DEV}
local sysfsdir=/proc/sys/fs/fs-timestamp-check-onif [ ! -e $sysfsdir ]; then_notrun "no kernel support for y2038 sysfs switch"fiDeepa,
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
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@w....
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
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@w....
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.
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
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
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
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
[+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.
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
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.
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
Addition of fs-specific timestamp range checking was added in 188d20bcd1eb ("vfs: Add file timestamp range support").
Add a check for whether the kernel supports the limits check before running the associated test.
Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com --- common/rc | 11 +++++++++++ tests/generic/402 | 3 +++ 2 files changed, 14 insertions(+)
diff --git a/common/rc b/common/rc index 816588d6..472db995 100644 --- a/common/rc +++ b/common/rc @@ -1981,6 +1981,17 @@ _run_aiodio() return $status }
+_require_kernel_timestamp_range() +{ + # 128-byte inodes do not have room for extended timestamp + MKFS_OPTIONS=-I128 _scratch_mkfs_ext4 &>> $seqres.full 2>&1 || _fail "ext4 mkfs failed" + + mount -t ext4 ${SCRATCH_DEV} ${SCRATCH_MNT} + _check_dmesg_for "ext4 filesystem being mounted at ${SCRATCH_MNT} supports timestamps until 2038" || \ + _notrun "Kernel does not support timestamp limits" + umount ${SCRATCH_MNT} +} + _require_timestamp_range() { local device=${1:-$TEST_DEV} diff --git a/tests/generic/402 b/tests/generic/402 index 0392c258..2be94c54 100755 --- a/tests/generic/402 +++ b/tests/generic/402 @@ -30,6 +30,7 @@ rm -f $seqres.full _supported_fs generic _supported_os Linux _require_scratch +_require_check_dmesg _require_xfs_io_command utimes
# Compare file timestamps obtained from stat @@ -79,6 +80,8 @@ run_test() done }
+_require_kernel_timestamp_range + _scratch_mkfs &>> $seqres.full 2>&1 || _fail "mkfs failed" _require_timestamp_range $SCRATCH_DEV
On Mon, Dec 23, 2019 at 7:16 AM Deepa Dinamani deepa.kernel@gmail.com wrote:
Addition of fs-specific timestamp range checking was added in 188d20bcd1eb ("vfs: Add file timestamp range support").
Add a check for whether the kernel supports the limits check before running the associated test.
Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com
common/rc | 11 +++++++++++ tests/generic/402 | 3 +++ 2 files changed, 14 insertions(+)
diff --git a/common/rc b/common/rc index 816588d6..472db995 100644 --- a/common/rc +++ b/common/rc @@ -1981,6 +1981,17 @@ _run_aiodio() return $status }
+_require_kernel_timestamp_range() +{
# 128-byte inodes do not have room for extended timestampMKFS_OPTIONS=-I128 _scratch_mkfs_ext4 &>> $seqres.full 2>&1 || _fail "ext4 mkfs failed"mount -t ext4 ${SCRATCH_DEV} ${SCRATCH_MNT}_check_dmesg_for "ext4 filesystem being mounted at ${SCRATCH_MNT} supports timestamps until 2038" || \_notrun "Kernel does not support timestamp limits"umount ${SCRATCH_MNT}+}
Deepa,
Thank you for following up. I am not sure if mkfs.ext4 of scratch partition in a generic test is going to be very popular - let's see what others have to say. You can certainly now do that without checking that ${SCRATCH_DEV} is a blockdev which is not the case for overlay and networking filesystems.
Why did you choose not to use a loop mounted ext2 for the check as I suggested? You can use _require_loop() and _require_ext2() inside the check. In any case, please also check for failure to mount.
Thanks, Amir.
On Sun, Dec 22, 2019 at 10:36 PM Amir Goldstein amir73il@gmail.com wrote:
On Mon, Dec 23, 2019 at 7:16 AM Deepa Dinamani deepa.kernel@gmail.com wrote:
Addition of fs-specific timestamp range checking was added in 188d20bcd1eb ("vfs: Add file timestamp range support").
Add a check for whether the kernel supports the limits check before running the associated test.
Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com
common/rc | 11 +++++++++++ tests/generic/402 | 3 +++ 2 files changed, 14 insertions(+)
diff --git a/common/rc b/common/rc index 816588d6..472db995 100644 --- a/common/rc +++ b/common/rc @@ -1981,6 +1981,17 @@ _run_aiodio() return $status }
+_require_kernel_timestamp_range() +{
# 128-byte inodes do not have room for extended timestampMKFS_OPTIONS=-I128 _scratch_mkfs_ext4 &>> $seqres.full 2>&1 || _fail "ext4 mkfs failed"mount -t ext4 ${SCRATCH_DEV} ${SCRATCH_MNT}_check_dmesg_for "ext4 filesystem being mounted at ${SCRATCH_MNT} supports timestamps until 2038" || \_notrun "Kernel does not support timestamp limits"umount ${SCRATCH_MNT}+}
Deepa,
Thank you for following up. I am not sure if mkfs.ext4 of scratch partition in a generic test is going to be very popular - let's see what others have to say. You can certainly now do that without checking that ${SCRATCH_DEV} is a blockdev which is not the case for overlay and networking filesystems.
_require_block_device() should alleviate this concern. But, I think making it a loop back device is a good idea.
I meant to check when mkfs.ext4 would fail, but I forgot. I will change this in v2.
Why did you choose not to use a loop mounted ext2 for the check as I suggested? You can use _require_loop() and _require_ext2() inside the check. In any case, please also check for failure to mount.
I did not not see anybody creating ext2 filesystem, and I thought that finding a kernel supporting ext4 was a lot more likely. We can add a _require_ext4() along the lines of _ext2_reqire() if really necessary. We should also add "_require_command "$MKFS_EXT4_PROG" mkfs.ext4".
I think it does not matter which filesystem we use unless we get the warning. Let me know if I missed something.
I will pick one of the two: ext4 (with small inode) or ext2 and do a loop back device instead for v2. I will also add the suggested-by that I forgot on this patch.
-Deepa
Addition of fs-specific timestamp range checking was added in 188d20bcd1eb ("vfs: Add file timestamp range support").
Add a check for whether the kernel supports the limits check before running the associated test.
ext4 has been chosen to test for the presence of kernel support for this feature. If there is a concern that ext4 could be built out of the kernel, I can include a _require_ext4() along the lines of _require_ext2().
Suggested-by: Amir Goldstein amir73il@gmail.com Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com --- * Changes since v1: used loopback device instead of mkfs scratch dev
common/rc | 26 ++++++++++++++++++++++++++ tests/generic/402 | 3 +++ 2 files changed, 29 insertions(+)
diff --git a/common/rc b/common/rc index 816588d6..6248adf7 100644 --- a/common/rc +++ b/common/rc @@ -1981,6 +1981,32 @@ _run_aiodio() return $status }
+_require_kernel_timestamp_range() +{ + LOOP_FILE=$SCRATCH_MNT/loop_file + LOOP_MNT=$SCRATCH_MNT/loop_mnt + + dd if=/dev/zero of=$LOOP_FILE bs=1M count=2 2>&1 | _filter_dd || _fail "loopback prep failed" + + # Use ext4 with 128-byte inodes, which do not have room for extended timestamp + FSTYP=ext4 MKFS_OPTIONS=-I128 \ + _mkfs_dev $LOOP_FILE >> $seqres.full 2>&1 || _fail "ext4 mkfs failed" + + LOOP_DEV=$(_create_loop_device $LOOP_FILE) + mkdir -p $LOOP_MNT >> $seqres.full 2>&1 || _fail "failed to create $LOOP_MNT" + mount -t ext4 ${LOOP_DEV} ${LOOP_MNT} >> $seqres.full 2>&1 || _fail "ext4 mount failed" + notrun=false + _check_dmesg_for "ext4 filesystem being mounted at ${LOOP_MNT} supports timestamps until 2038" || \ + notrun=true + + umount ${LOOP_MNT} >> $seqres.full 2>&1 ||_fail "failed to umount $LOOP_MNT" + _destroy_loop_device ${LOOP_DEV} >> $seqres.full 2>&1 + + if $notrun; then + _notrun "Kernel does not support timestamp limits" + fi +} + _require_timestamp_range() { local device=${1:-$TEST_DEV} diff --git a/tests/generic/402 b/tests/generic/402 index 0392c258..2be94c54 100755 --- a/tests/generic/402 +++ b/tests/generic/402 @@ -30,6 +30,7 @@ rm -f $seqres.full _supported_fs generic _supported_os Linux _require_scratch +_require_check_dmesg _require_xfs_io_command utimes
# Compare file timestamps obtained from stat @@ -79,6 +80,8 @@ run_test() done }
+_require_kernel_timestamp_range + _scratch_mkfs &>> $seqres.full 2>&1 || _fail "mkfs failed" _require_timestamp_range $SCRATCH_DEV
On Sun, Dec 29, 2019 at 12:13 AM Deepa Dinamani deepa.kernel@gmail.com wrote:
Addition of fs-specific timestamp range checking was added in 188d20bcd1eb ("vfs: Add file timestamp range support").
Add a check for whether the kernel supports the limits check before running the associated test.
ext4 has been chosen to test for the presence of kernel support for this feature. If there is a concern that ext4 could be built out of the kernel, I can include a _require_ext4() along the lines of _require_ext2().
Suggested-by: Amir Goldstein amir73il@gmail.com Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com
- Changes since v1: used loopback device instead of mkfs scratch dev
common/rc | 26 ++++++++++++++++++++++++++ tests/generic/402 | 3 +++ 2 files changed, 29 insertions(+)
diff --git a/common/rc b/common/rc index 816588d6..6248adf7 100644 --- a/common/rc +++ b/common/rc @@ -1981,6 +1981,32 @@ _run_aiodio() return $status }
+_require_kernel_timestamp_range() +{
LOOP_FILE=$SCRATCH_MNT/loop_fileLOOP_MNT=$SCRATCH_MNT/loop_mntdd if=/dev/zero of=$LOOP_FILE bs=1M count=2 2>&1 | _filter_dd || _fail "loopback prep failed"# Use ext4 with 128-byte inodes, which do not have room for extended timestampFSTYP=ext4 MKFS_OPTIONS=-I128 \_mkfs_dev $LOOP_FILE >> $seqres.full 2>&1 || _fail "ext4 mkfs failed"LOOP_DEV=$(_create_loop_device $LOOP_FILE)mkdir -p $LOOP_MNT >> $seqres.full 2>&1 || _fail "failed to create $LOOP_MNT"mount -t ext4 ${LOOP_DEV} ${LOOP_MNT} >> $seqres.full 2>&1 || _fail "ext4 mount failed"notrun=false_check_dmesg_for "ext4 filesystem being mounted at ${LOOP_MNT} supports timestamps until 2038" || \notrun=trueumount ${LOOP_MNT} >> $seqres.full 2>&1 ||_fail "failed to umount $LOOP_MNT"_destroy_loop_device ${LOOP_DEV} >> $seqres.full 2>&1if $notrun; then_notrun "Kernel does not support timestamp limits"fi+}
As a generic helper, this function has a few problems: 1. It assumes scratch dev is mounted (and you're not even calling it after _scratch_mount) 2. The cleanup() hook won't clean loop mnt/dev if interrupted 3. test doesn't have _require_loop (nor require ext4 as you mentioned)
All this leads me to think that perhaps it would be better off, at least until kernel has fsinfo, to keep this entire helper inside generic/402, while addressing the issues above in the test itself.
A more generic solution would be harder and IMO and overkill at this point.
What do you think?
Thanks, Amir.
As a generic helper, this function has a few problems:
- It assumes scratch dev is mounted (and you're not even calling it
after _scratch_mount)
Ok, this was an oversight. I was using rootfs as scratch and didn't catch this in my tests.
- The cleanup() hook won't clean loop mnt/dev if interrupted
Agreed. I can add these in.
There are some existing functions in common/rc that do not clean up. For example _require_scratch_swapfile might leave partial state if interrupted.
- test doesn't have _require_loop (nor require ext4 as you mentioned)
Some generic functions assume many preconditions. But, if it is preferred to be more self contained, I can model this after something like _require_scratch_swapfile()
All this leads me to think that perhaps it would be better off, at least until kernel has fsinfo, to keep this entire helper inside generic/402, while addressing the issues above in the test itself.
A more generic solution would be harder and IMO and overkill at this point.
With fsinfo as proposed, it would not be possible to tell if fs ranges are supported without doing the same kind of workaround. A capability bit could be added to advertise that feature of VFS, or it might be reasonable to assume it from the mere existence of fsinfo syscall.
What do you think?
The following proposed patch uses a local _cleanup handler that can handle this. I am okay with either approach. I'm not sure which one is preferable to xfstests maintainers. Let me know and I can post it as a V3.
-Deepa
From f539005511f3ad83563cabc6d185b2c76ae37dea Mon Sep 17 00:00:00 2001
From: Deepa Dinamani deepa.kernel@gmail.com Date: Sun, 22 Dec 2019 19:18:14 -0800 Subject: [PATCH v3 1/1] generic/402: Make timestamp range check conditional
Addition of fs-specific timestamp range checking was added in 188d20bcd1eb ("vfs: Add file timestamp range support").
Add a check for whether the kernel supports the limits check before running the associated test.
ext4 has been chosen to test for the presence of kernel support for this feature.
Suggested-by: Amir Goldstein amir73il@gmail.com Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com --- common/rc | 50 +++++++++++++++++++++++++++++++++++++++++++---- tests/generic/402 | 12 +++++++++++- 2 files changed, 57 insertions(+), 5 deletions(-)
diff --git a/common/rc b/common/rc index eeac1355..796052e6 100644 --- a/common/rc +++ b/common/rc @@ -1751,17 +1751,18 @@ _require_loop() # _require_ext2() { + fs=${1:-ext2} if [ "$HOSTOS" != "Linux" ] then - _notrun "This test requires linux for ext2 filesystem support" + _notrun "This test requires linux for $fs filesystem support" fi
- modprobe ext2 >/dev/null 2>&1 - if grep ext2 /proc/filesystems >/dev/null 2>&1 + modprobe $fs >/dev/null 2>&1 + if grep $fs /proc/filesystems >/dev/null 2>&1 then : else - _notrun "This test requires ext2 filesystem support" + _notrun "This test requires $fs filesystem support" fi }
@@ -1981,6 +1982,47 @@ _run_aiodio() return $status }
+_require_kernel_timestamp_range() +{ + + _require_scratch + _require_loop + _require_ext2 ext4 + + # Use a subshell to clean up the nested loop mount + _cleanup='( umount $LOOP_MNT ; _destroy_loop_device $LOOP_DEV ; rm -f $LOOP_FILE ; _scratch_unmount )' + + _scratch_mkfs >/dev/null + _scratch_mount + + LOOP_FILE=$SCRATCH_MNT/loop_file + LOOP_MNT=$SCRATCH_MNT/loop_mnt + + dd if=/dev/zero of=$LOOP_FILE bs=1M count=2 2>&1 | _filter_dd || _fail "loopback prep failed" + + # Use ext4 with 128-byte inodes, which do not have room for extended timestamp + FSTYP=ext4 MKFS_OPTIONS=-I128 \ + _mkfs_dev $LOOP_FILE >> $seqres.full 2>&1 || _fail "ext4 mkfs failed" + + LOOP_DEV=$(_create_loop_device $LOOP_FILE) + mkdir -p $LOOP_MNT >> $seqres.full 2>&1 || _fail "failed to create $LOOP_MNT" + mount -t ext4 ${LOOP_DEV} ${LOOP_MNT} >> $seqres.full 2>&1 || _fail "ext4 mount failed" + notrun=false + _check_dmesg_for "ext4 filesystem being mounted at ${LOOP_MNT} supports timestamps until 2038" || \ + notrun=true + + umount ${LOOP_MNT} >> $seqres.full 2>&1 || _fail "failed to umount $LOOP_MNT" + _destroy_loop_device ${LOOP_DEV} >> $seqres.full 2>&1 + + _scratch_unmount + + _cleanup=: + + if $notrun; then + _notrun "Kernel does not support timestamp limits" + fi +} + _require_timestamp_range() { local device=${1:-$TEST_DEV} diff --git a/tests/generic/402 b/tests/generic/402 index 0392c258..4288168a 100755 --- a/tests/generic/402 +++ b/tests/generic/402 @@ -16,7 +16,14 @@ echo "QA output created by $seq" here=`pwd` tmp=/tmp/$$ status=1 # failure is the default! -trap "exit $status" 0 1 2 3 15 +_cleanup=: +trap "eval $_cleanup; _cleanup; exit $status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +}
# Get standard environment, filters and checks. . ./common/rc @@ -30,6 +37,7 @@ rm -f $seqres.full _supported_fs generic _supported_os Linux _require_scratch +_require_check_dmesg _require_xfs_io_command utimes
# Compare file timestamps obtained from stat @@ -79,6 +87,8 @@ run_test() done }
+_require_kernel_timestamp_range + _scratch_mkfs &>> $seqres.full 2>&1 || _fail "mkfs failed" _require_timestamp_range $SCRATCH_DEV
On Fri, Jan 3, 2020 at 8:46 AM Deepa Dinamani deepa.kernel@gmail.com wrote:
As a generic helper, this function has a few problems:
- It assumes scratch dev is mounted (and you're not even calling it
after _scratch_mount)
Ok, this was an oversight. I was using rootfs as scratch and didn't catch this in my tests.
- The cleanup() hook won't clean loop mnt/dev if interrupted
Agreed. I can add these in.
There are some existing functions in common/rc that do not clean up. For example _require_scratch_swapfile might leave partial state if interrupted.
Right. Things are not perfect. We should try to not make them worse ;-)
- test doesn't have _require_loop (nor require ext4 as you mentioned)
Some generic functions assume many preconditions. But, if it is preferred to be more self contained, I can model this after something like _require_scratch_swapfile()
OK.
All this leads me to think that perhaps it would be better off, at least until kernel has fsinfo, to keep this entire helper inside generic/402, while addressing the issues above in the test itself.
A more generic solution would be harder and IMO and overkill at this point.
With fsinfo as proposed, it would not be possible to tell if fs ranges are supported without doing the same kind of workaround. A capability bit could be added to advertise that feature of VFS, or it might be reasonable to assume it from the mere existence of fsinfo syscall.
That is what I had in mind.
What do you think?
The following proposed patch uses a local _cleanup handler that can handle this. I am okay with either approach. I'm not sure which one is preferable to xfstests maintainers. Let me know and I can post it as a V3.
IMO the "nested" _cleanup handler is not a "clean" solution, but let's let Eryu decide how to tackle this.
Thanks, Amir.
From f539005511f3ad83563cabc6d185b2c76ae37dea Mon Sep 17 00:00:00 2001 From: Deepa Dinamani deepa.kernel@gmail.com Date: Sun, 22 Dec 2019 19:18:14 -0800 Subject: [PATCH v3 1/1] generic/402: Make timestamp range check conditional
Addition of fs-specific timestamp range checking was added in 188d20bcd1eb ("vfs: Add file timestamp range support").
Add a check for whether the kernel supports the limits check before running the associated test.
ext4 has been chosen to test for the presence of kernel support for this feature.
Suggested-by: Amir Goldstein amir73il@gmail.com Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com
common/rc | 50 +++++++++++++++++++++++++++++++++++++++++++---- tests/generic/402 | 12 +++++++++++- 2 files changed, 57 insertions(+), 5 deletions(-)
diff --git a/common/rc b/common/rc index eeac1355..796052e6 100644 --- a/common/rc +++ b/common/rc @@ -1751,17 +1751,18 @@ _require_loop() # _require_ext2() {
- fs=${1:-ext2} if [ "$HOSTOS" != "Linux" ] then
- _notrun "This test requires linux for ext2 filesystem support"
- _notrun "This test requires linux for $fs filesystem support" fi
- modprobe ext2 >/dev/null 2>&1
- if grep ext2 /proc/filesystems >/dev/null 2>&1
- modprobe $fs >/dev/null 2>&1
- if grep $fs /proc/filesystems >/dev/null 2>&1 then : else
- _notrun "This test requires ext2 filesystem support"
- _notrun "This test requires $fs filesystem support" fi
}
@@ -1981,6 +1982,47 @@ _run_aiodio() return $status }
+_require_kernel_timestamp_range() +{
- _require_scratch
- _require_loop
- _require_ext2 ext4
- # Use a subshell to clean up the nested loop mount
- _cleanup='( umount $LOOP_MNT ; _destroy_loop_device $LOOP_DEV ;
rm -f $LOOP_FILE ; _scratch_unmount )'
- _scratch_mkfs >/dev/null
- _scratch_mount
- LOOP_FILE=$SCRATCH_MNT/loop_file
- LOOP_MNT=$SCRATCH_MNT/loop_mnt
- dd if=/dev/zero of=$LOOP_FILE bs=1M count=2 2>&1 | _filter_dd ||
_fail "loopback prep failed"
- # Use ext4 with 128-byte inodes, which do not have room for
extended timestamp
- FSTYP=ext4 MKFS_OPTIONS=-I128 \
- _mkfs_dev $LOOP_FILE >> $seqres.full 2>&1 || _fail "ext4 mkfs failed"
- LOOP_DEV=$(_create_loop_device $LOOP_FILE)
- mkdir -p $LOOP_MNT >> $seqres.full 2>&1 || _fail "failed to
create $LOOP_MNT"
- mount -t ext4 ${LOOP_DEV} ${LOOP_MNT} >> $seqres.full 2>&1 ||
_fail "ext4 mount failed"
- notrun=false
- _check_dmesg_for "ext4 filesystem being mounted at ${LOOP_MNT}
supports timestamps until 2038" || \
notrun=true- umount ${LOOP_MNT} >> $seqres.full 2>&1 || _fail "failed to
umount $LOOP_MNT"
- _destroy_loop_device ${LOOP_DEV} >> $seqres.full 2>&1
- _scratch_unmount
- _cleanup=:
- if $notrun; then
_notrun "Kernel does not support timestamp limits"- fi
+}
_require_timestamp_range() { local device=${1:-$TEST_DEV} diff --git a/tests/generic/402 b/tests/generic/402 index 0392c258..4288168a 100755 --- a/tests/generic/402 +++ b/tests/generic/402 @@ -16,7 +16,14 @@ echo "QA output created by $seq" here=`pwd` tmp=/tmp/$$ status=1 # failure is the default! -trap "exit $status" 0 1 2 3 15 +_cleanup=: +trap "eval $_cleanup; _cleanup; exit $status" 0 1 2 3 15
+_cleanup() +{
- cd /
- rm -f $tmp.*
+}
# Get standard environment, filters and checks. . ./common/rc @@ -30,6 +37,7 @@ rm -f $seqres.full _supported_fs generic _supported_os Linux _require_scratch +_require_check_dmesg _require_xfs_io_command utimes
# Compare file timestamps obtained from stat @@ -79,6 +87,8 @@ run_test() done }
+_require_kernel_timestamp_range
_scratch_mkfs &>> $seqres.full 2>&1 || _fail "mkfs failed" _require_timestamp_range $SCRATCH_DEV
-- 2.17.1
On Mon, Dec 30, 2019 at 09:34:47AM +0200, Amir Goldstein wrote:
On Sun, Dec 29, 2019 at 12:13 AM Deepa Dinamani deepa.kernel@gmail.com wrote:
Addition of fs-specific timestamp range checking was added in 188d20bcd1eb ("vfs: Add file timestamp range support").
Add a check for whether the kernel supports the limits check before running the associated test.
ext4 has been chosen to test for the presence of kernel support for this feature. If there is a concern that ext4 could be built out of the kernel, I can include a _require_ext4() along the lines of _require_ext2().
Suggested-by: Amir Goldstein amir73il@gmail.com Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com
Sorry for chiming in so late..
- Changes since v1: used loopback device instead of mkfs scratch dev
common/rc | 26 ++++++++++++++++++++++++++ tests/generic/402 | 3 +++ 2 files changed, 29 insertions(+)
diff --git a/common/rc b/common/rc index 816588d6..6248adf7 100644 --- a/common/rc +++ b/common/rc @@ -1981,6 +1981,32 @@ _run_aiodio() return $status }
+_require_kernel_timestamp_range() +{
LOOP_FILE=$SCRATCH_MNT/loop_fileLOOP_MNT=$SCRATCH_MNT/loop_mntdd if=/dev/zero of=$LOOP_FILE bs=1M count=2 2>&1 | _filter_dd || _fail "loopback prep failed"# Use ext4 with 128-byte inodes, which do not have room for extended timestampFSTYP=ext4 MKFS_OPTIONS=-I128 \_mkfs_dev $LOOP_FILE >> $seqres.full 2>&1 || _fail "ext4 mkfs failed"LOOP_DEV=$(_create_loop_device $LOOP_FILE)mkdir -p $LOOP_MNT >> $seqres.full 2>&1 || _fail "failed to create $LOOP_MNT"mount -t ext4 ${LOOP_DEV} ${LOOP_MNT} >> $seqres.full 2>&1 || _fail "ext4 mount failed"notrun=false_check_dmesg_for "ext4 filesystem being mounted at ${LOOP_MNT} supports timestamps until 2038" || \notrun=trueumount ${LOOP_MNT} >> $seqres.full 2>&1 ||_fail "failed to umount $LOOP_MNT"_destroy_loop_device ${LOOP_DEV} >> $seqres.full 2>&1if $notrun; then_notrun "Kernel does not support timestamp limits"fi+}
As a generic helper, this function has a few problems:
- It assumes scratch dev is mounted (and you're not even calling it
after _scratch_mount) 2. The cleanup() hook won't clean loop mnt/dev if interrupted 3. test doesn't have _require_loop (nor require ext4 as you mentioned)
All this leads me to think that perhaps it would be better off, at least until kernel has fsinfo, to keep this entire helper inside generic/402, while addressing the issues above in the test itself.
A more generic solution would be harder and IMO and overkill at this point.
What do you think?
After reading through this thread, I prefer waiting for the comming fsinfo interface, detecting the timestamp limit support using ext2 & loop device doesn't look "pretty" and is just a temporary solution.
Thanks, Eryu
On Wed, Jan 8, 2020 at 10:09 AM Eryu Guan guaneryu@gmail.com wrote:
On Mon, Dec 30, 2019 at 09:34:47AM +0200, Amir Goldstein wrote:
On Sun, Dec 29, 2019 at 12:13 AM Deepa Dinamani deepa.kernel@gmail.com wrote:
Addition of fs-specific timestamp range checking was added in 188d20bcd1eb ("vfs: Add file timestamp range support").
Add a check for whether the kernel supports the limits check before running the associated test.
ext4 has been chosen to test for the presence of kernel support for this feature. If there is a concern that ext4 could be built out of the kernel, I can include a _require_ext4() along the lines of _require_ext2().
Suggested-by: Amir Goldstein amir73il@gmail.com Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com
Sorry for chiming in so late..
- Changes since v1: used loopback device instead of mkfs scratch dev
common/rc | 26 ++++++++++++++++++++++++++ tests/generic/402 | 3 +++ 2 files changed, 29 insertions(+)
diff --git a/common/rc b/common/rc index 816588d6..6248adf7 100644 --- a/common/rc +++ b/common/rc @@ -1981,6 +1981,32 @@ _run_aiodio() return $status }
+_require_kernel_timestamp_range() +{
LOOP_FILE=$SCRATCH_MNT/loop_fileLOOP_MNT=$SCRATCH_MNT/loop_mntdd if=/dev/zero of=$LOOP_FILE bs=1M count=2 2>&1 | _filter_dd || _fail "loopback prep failed"# Use ext4 with 128-byte inodes, which do not have room for extended timestampFSTYP=ext4 MKFS_OPTIONS=-I128 \_mkfs_dev $LOOP_FILE >> $seqres.full 2>&1 || _fail "ext4 mkfs failed"LOOP_DEV=$(_create_loop_device $LOOP_FILE)mkdir -p $LOOP_MNT >> $seqres.full 2>&1 || _fail "failed to create $LOOP_MNT"mount -t ext4 ${LOOP_DEV} ${LOOP_MNT} >> $seqres.full 2>&1 || _fail "ext4 mount failed"notrun=false_check_dmesg_for "ext4 filesystem being mounted at ${LOOP_MNT} supports timestamps until 2038" || \notrun=trueumount ${LOOP_MNT} >> $seqres.full 2>&1 ||_fail "failed to umount $LOOP_MNT"_destroy_loop_device ${LOOP_DEV} >> $seqres.full 2>&1if $notrun; then_notrun "Kernel does not support timestamp limits"fi+}
As a generic helper, this function has a few problems:
- It assumes scratch dev is mounted (and you're not even calling it
after _scratch_mount) 2. The cleanup() hook won't clean loop mnt/dev if interrupted 3. test doesn't have _require_loop (nor require ext4 as you mentioned)
All this leads me to think that perhaps it would be better off, at least until kernel has fsinfo, to keep this entire helper inside generic/402, while addressing the issues above in the test itself.
A more generic solution would be harder and IMO and overkill at this point.
What do you think?
After reading through this thread, I prefer waiting for the comming fsinfo interface, detecting the timestamp limit support using ext2 & loop device doesn't look "pretty" and is just a temporary solution.
I understand why you dislike the ext2+loop test, but please hear me out.
From all the fs types that are supported by the test, only btrfs and ext4 with
large inode size are y2038 ready. For the rest of the cases, we actually have a way to detect kernel support from the dmesg warning, without the need for hacky ext2 loop mount.
So how about just: 1. moving _scratch_mount before _require_timestamp_range 2. in _require_timestamp_range (untested): if [ $tsmax -lt $((1<<32)) ] && ! _check_dmesg_for "supports timestamps until 2038" ; then _notrun "Kernel does not support timestamp limits" fi
It's better than nothing and it does not add much complications, nor is this "hacky" IMO.
Thoughts?
Amir.
On Wed, Jan 08, 2020 at 10:45:29AM +0200, Amir Goldstein wrote:
On Wed, Jan 8, 2020 at 10:09 AM Eryu Guan guaneryu@gmail.com wrote:
On Mon, Dec 30, 2019 at 09:34:47AM +0200, Amir Goldstein wrote:
On Sun, Dec 29, 2019 at 12:13 AM Deepa Dinamani deepa.kernel@gmail.com wrote:
Addition of fs-specific timestamp range checking was added in 188d20bcd1eb ("vfs: Add file timestamp range support").
Add a check for whether the kernel supports the limits check before running the associated test.
ext4 has been chosen to test for the presence of kernel support for this feature. If there is a concern that ext4 could be built out of the kernel, I can include a _require_ext4() along the lines of _require_ext2().
Suggested-by: Amir Goldstein amir73il@gmail.com Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com
Sorry for chiming in so late..
- Changes since v1: used loopback device instead of mkfs scratch dev
common/rc | 26 ++++++++++++++++++++++++++ tests/generic/402 | 3 +++ 2 files changed, 29 insertions(+)
diff --git a/common/rc b/common/rc index 816588d6..6248adf7 100644 --- a/common/rc +++ b/common/rc @@ -1981,6 +1981,32 @@ _run_aiodio() return $status }
+_require_kernel_timestamp_range() +{
LOOP_FILE=$SCRATCH_MNT/loop_fileLOOP_MNT=$SCRATCH_MNT/loop_mntdd if=/dev/zero of=$LOOP_FILE bs=1M count=2 2>&1 | _filter_dd || _fail "loopback prep failed"# Use ext4 with 128-byte inodes, which do not have room for extended timestampFSTYP=ext4 MKFS_OPTIONS=-I128 \_mkfs_dev $LOOP_FILE >> $seqres.full 2>&1 || _fail "ext4 mkfs failed"LOOP_DEV=$(_create_loop_device $LOOP_FILE)mkdir -p $LOOP_MNT >> $seqres.full 2>&1 || _fail "failed to create $LOOP_MNT"mount -t ext4 ${LOOP_DEV} ${LOOP_MNT} >> $seqres.full 2>&1 || _fail "ext4 mount failed"notrun=false_check_dmesg_for "ext4 filesystem being mounted at ${LOOP_MNT} supports timestamps until 2038" || \notrun=trueumount ${LOOP_MNT} >> $seqres.full 2>&1 ||_fail "failed to umount $LOOP_MNT"_destroy_loop_device ${LOOP_DEV} >> $seqres.full 2>&1if $notrun; then_notrun "Kernel does not support timestamp limits"fi+}
As a generic helper, this function has a few problems:
- It assumes scratch dev is mounted (and you're not even calling it
after _scratch_mount) 2. The cleanup() hook won't clean loop mnt/dev if interrupted 3. test doesn't have _require_loop (nor require ext4 as you mentioned)
All this leads me to think that perhaps it would be better off, at least until kernel has fsinfo, to keep this entire helper inside generic/402, while addressing the issues above in the test itself.
A more generic solution would be harder and IMO and overkill at this point.
What do you think?
After reading through this thread, I prefer waiting for the comming fsinfo interface, detecting the timestamp limit support using ext2 & loop device doesn't look "pretty" and is just a temporary solution.
I understand why you dislike the ext2+loop test, but please hear me out.
From all the fs types that are supported by the test, only btrfs and ext4 with large inode size are y2038 ready. For the rest of the cases, we actually have a way to detect kernel support from the dmesg warning, without the need for hacky ext2 loop mount.
So how about just:
- moving _scratch_mount before _require_timestamp_range
- in _require_timestamp_range (untested): if [ $tsmax -lt $((1<<32)) ] && ! _check_dmesg_for "supports
Yeah, this looks fine. I thought about searching for dmesg warning after _scratch_mount as well, but that would _notrun if the fs is 2038-safe. This $tsmax check fixes my concern. Thanks!
Eryu
timestamps until 2038" ; then _notrun "Kernel does not support timestamp limits" fi
It's better than nothing and it does not add much complications, nor is this "hacky" IMO.
Thoughts?
Amir.
I understand why you dislike the ext2+loop test, but please hear me out.
From all the fs types that are supported by the test, only btrfs and ext4 with large inode size are y2038 ready. For the rest of the cases, we actually have a way to detect kernel support from the dmesg warning, without the need for hacky ext2 loop mount.
So how about just:
- moving _scratch_mount before _require_timestamp_range
- in _require_timestamp_range (untested): if [ $tsmax -lt $((1<<32)) ] && ! _check_dmesg_for "supports
Yeah, this looks fine. I thought about searching for dmesg warning after _scratch_mount as well, but that would _notrun if the fs is 2038-safe. This $tsmax check fixes my concern. Thanks!
Deepa,
Do you intend to post the simplified version or would you like me to re-spin your patch?
Thanks, Amir.
On Fri, Jan 17, 2020 at 1:09 AM Amir Goldstein amir73il@gmail.com wrote:
I understand why you dislike the ext2+loop test, but please hear me out.
From all the fs types that are supported by the test, only btrfs and ext4 with large inode size are y2038 ready. For the rest of the cases, we actually have a way to detect kernel support from the dmesg warning, without the need for hacky ext2 loop mount.
So how about just:
- moving _scratch_mount before _require_timestamp_range
- in _require_timestamp_range (untested): if [ $tsmax -lt $((1<<32)) ] && ! _check_dmesg_for "supports
Yeah, this looks fine. I thought about searching for dmesg warning after _scratch_mount as well, but that would _notrun if the fs is 2038-safe. This $tsmax check fixes my concern. Thanks!
Deepa,
Do you intend to post the simplified version or would you like me to re-spin your patch?
I intend to do this. Sorry, was distracted by other things. FWIW, just (1<<32) is not enough. The kernel prints this warning based on (current time + max uptime) <= tsmax. Will post this.
-Deepa
On Fri, Jan 17, 2020 at 10:23 AM Deepa Dinamani deepa.kernel@gmail.com wrote:
On Fri, Jan 17, 2020 at 1:09 AM Amir Goldstein amir73il@gmail.com wrote:
I understand why you dislike the ext2+loop test, but please hear me out.
From all the fs types that are supported by the test, only btrfs and ext4 with large inode size are y2038 ready. For the rest of the cases, we actually have a way to detect kernel support from the dmesg warning, without the need for hacky ext2 loop mount.
So how about just:
- moving _scratch_mount before _require_timestamp_range
- in _require_timestamp_range (untested): if [ $tsmax -lt $((1<<32)) ] && ! _check_dmesg_for "supports
Yeah, this looks fine. I thought about searching for dmesg warning after _scratch_mount as well, but that would _notrun if the fs is 2038-safe. This $tsmax check fixes my concern. Thanks!
Deepa,
Do you intend to post the simplified version or would you like me to re-spin your patch?
I intend to do this. Sorry, was distracted by other things. FWIW, just (1<<32) is not enough. The kernel prints this warning based on (current time + max uptime) <= tsmax. Will post this.
Also we are testing for the following timestamps in the test:
declare -a TIMESTAMPS=( $tsmin 0 $tsmax $((tsmax/2)) $((tsmax+1)) )
So the test can still fail if the above tsmax test passes, but the limit patches are not in the kernel. https://lkml.org/lkml/2019/7/29/1850 has the limits for all the filesystems I filled in. fat, cifs, hpfs etc. all fall in this category.
I do not see a good way of doing this unless we test for a fs that always fails in a predictable way.
Checking for fsinfo maybe is ok. But, at that point we could just rely on any syscall that got merged after the limits series isn't it?
-Deepa
Addition of fs-specific timestamp range checking was added in 188d20bcd1eb ("vfs: Add file timestamp range support").
Add a check for whether the kernel supports the limits check before running the associated test.
Based on an off-list discussion, we use a simpler interim approach until fsinfo syscall would provide fs timestamp limits info. This isn't perfect, but works for filesystems expiring in 2038.
Suggested-by: Amir Goldstein amir73il@gmail.com Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com --- common/rc | 7 +++++++ tests/generic/402 | 13 ++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/common/rc b/common/rc index eeac1355..fc82c17a 100644 --- a/common/rc +++ b/common/rc @@ -1990,6 +1990,13 @@ _require_timestamp_range() if [ $tsmin -eq -1 -a $tsmax -eq -1 ]; then _notrun "filesystem $FSTYP timestamp bounds are unknown" fi + + # expect console warning from rw scratch mount if fs limit is near + if [ $tsmax -le $((1<<31)) ] && \ + ! _check_dmesg_for "filesystem being mounted at .* supports timestamps until" + then + _notrun "Kernel does not support timestamp limits" + fi }
_filesystem_timestamp_range() diff --git a/tests/generic/402 b/tests/generic/402 index 0392c258..2a34d127 100755 --- a/tests/generic/402 +++ b/tests/generic/402 @@ -16,7 +16,13 @@ echo "QA output created by $seq" here=`pwd` tmp=/tmp/$$ status=1 # failure is the default! -trap "exit $status" 0 1 2 3 15 +trap "_cleanup; exit $status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +}
# Get standard environment, filters and checks. . ./common/rc @@ -30,6 +36,7 @@ rm -f $seqres.full _supported_fs generic _supported_os Linux _require_scratch +_require_check_dmesg _require_xfs_io_command utimes
# Compare file timestamps obtained from stat @@ -80,6 +87,8 @@ run_test() }
_scratch_mkfs &>> $seqres.full 2>&1 || _fail "mkfs failed" +_scratch_mount || _fail "scratch mount failed" + _require_timestamp_range $SCRATCH_DEV
read tsmin tsmax <<<$(_filesystem_timestamp_range $SCRATCH_DEV) @@ -96,8 +105,6 @@ declare -a TIMESTAMPS=( $((tsmax+1)) )
-_scratch_mount || _fail "scratch mount failed" - status=0
# Begin test case 1
On Sun, Jan 19, 2020 at 2:57 AM Deepa Dinamani deepa.kernel@gmail.com wrote:
Addition of fs-specific timestamp range checking was added in 188d20bcd1eb ("vfs: Add file timestamp range support").
Add a check for whether the kernel supports the limits check before running the associated test.
Based on an off-list discussion, we use a simpler interim approach until fsinfo syscall would provide fs timestamp limits info. This isn't perfect, but works for filesystems expiring in 2038.
Suggested-by: Amir Goldstein amir73il@gmail.com Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com
Excellent! Thank you.
Eryu, you may add any of: Reviewed-by: Amir Goldstein amir73il@gmail.com Tested-by: Amir Goldstein amir73il@gmail.com
On kernel v5.4, ext2,ext4,xfs,btrfs (default mkfs) still pass. On Kernel v5.3, ext2,xfs are notrun for lack of kernel support (instead of failing), ext4 (256 bytes inodes) still fails and btrfs still pass, because bash overflows $(($s64max + 1 )) just the same as the kernel...
Thanks, Amir.
On Sun, Jan 19, 2020 at 11:19:24AM +0200, Amir Goldstein wrote:
On Sun, Jan 19, 2020 at 2:57 AM Deepa Dinamani deepa.kernel@gmail.com wrote:
Addition of fs-specific timestamp range checking was added in 188d20bcd1eb ("vfs: Add file timestamp range support").
Add a check for whether the kernel supports the limits check before running the associated test.
Based on an off-list discussion, we use a simpler interim approach until fsinfo syscall would provide fs timestamp limits info. This isn't perfect, but works for filesystems expiring in 2038.
Suggested-by: Amir Goldstein amir73il@gmail.com Signed-off-by: Deepa Dinamani deepa.kernel@gmail.com
Excellent! Thank you.
Eryu, you may add any of: Reviewed-by: Amir Goldstein amir73il@gmail.com Tested-by: Amir Goldstein amir73il@gmail.com
On kernel v5.4, ext2,ext4,xfs,btrfs (default mkfs) still pass. On Kernel v5.3, ext2,xfs are notrun for lack of kernel support (instead of failing), ext4 (256 bytes inodes) still fails and btrfs still pass, because bash overflows $(($s64max + 1 )) just the same as the kernel...
Thanks a lot for the double checking! And many thanks to Deepa for the fix!
Thanks, Eryu
-
Amir Goldstein -
Arnd Bergmann -
Ben Hutchings -
Deepa Dinamani -
Eryu Guan -
Greg KH