| Message ID | 20191228221317.16654-1-deepa.kernel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] generic/402: Make timestamp range check conditional | expand |
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_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 > +} > + 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: > 1. 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. > 2. 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. > 3. 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: > > 1. 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. > > > 2. 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 ;-) > > > 3. 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_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 > > +} > > + > > 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? 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_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 > > > +} > > > + > > > > 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? > > 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_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 > > > > +} > > > > + > > > > > > 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? > > > > 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 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: > > 1. moving _scratch_mount before _require_timestamp_range > > 2. 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: > > > 1. moving _scratch_mount before _require_timestamp_range > > > 2. 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: > > > > 1. moving _scratch_mount before _require_timestamp_range > > > > 2. 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
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
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(+)