| Message ID | 20190719041231.26500-1-deepa.kernel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | generic/402: fix for updated behavior of timestamp limits | expand |
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 >
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 > 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 >
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
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.
> > { > > 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
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
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.
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
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.
[+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
> 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
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
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(-)