On Fri, Jan 3, 2020 at 8:46 AM Deepa Dinamani <deepa.kernel@xxxxxxxxx> 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@xxxxxxxxx>
> 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@xxxxxxxxx>
> Signed-off-by: Deepa Dinamani <deepa.kernel@xxxxxxxxx>
> ---
> 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
