On Fri, Jan 3, 2020 at 8:46 AM Deepa Dinamani <deepa.ker...@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.ker...@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 <amir7...@gmail.com>
> Signed-off-by: Deepa Dinamani <deepa.ker...@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
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Reply via email to