fstests.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Amir Goldstein <amir73il@gmail.com>
To: Deepa Dinamani <deepa.kernel@gmail.com>
Cc: fstests <fstests@vger.kernel.org>, Arnd Bergmann <arnd@arndb.de>,
	Greg KH <gregkh@linuxfoundation.org>,
	Eryu Guan <guaneryu@gmail.com>, Sasha Levin <sashal@kernel.org>,
	y2038 Mailman List <y2038@lists.linaro.org>
Subject: Re: [PATCH v2] generic/402: Make timestamp range check conditional
Date: Fri, 3 Jan 2020 11:58:04 +0200	[thread overview]
Message-ID: <CAOQ4uxgCrq1V-Hkzg9h+wW26zfd4WUHQx7n8rUsRTsX9YuP5sw@mail.gmail.com> (raw)
In-Reply-To: <CABeXuvqxLnfUMcq9+ip8vkMU43gAphr9MvveD1bw+5QeWo+3zA@mail.gmail.com>

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

  reply	other threads:[~2020-01-03  9:58 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-19  4:12 [PATCH] generic/402: fix for updated behavior of timestamp limits Deepa Dinamani
2019-07-21 16:47 ` Eryu Guan
2019-10-02 22:06   ` Deepa Dinamani
2019-10-05 18:35 ` Eryu Guan
2019-10-23 22:17   ` Deepa Dinamani
2019-12-12 13:11 ` Amir Goldstein
2019-12-12 21:55   ` Deepa Dinamani
2019-12-18 20:21     ` Deepa Dinamani
2019-12-18 20:46       ` Amir Goldstein
2019-12-19  8:28         ` [Y2038] " Arnd Bergmann
2019-12-19  8:40           ` Greg KH
2019-12-19 11:29             ` Arnd Bergmann
2019-12-19 11:35               ` Greg KH
2019-12-19 15:48               ` Ben Hutchings
2019-12-19 20:35                 ` Arnd Bergmann
2019-12-19 12:09           ` Amir Goldstein
2019-12-20 22:45             ` Deepa Dinamani
2019-12-23  5:16               ` [PATCH] generic/402: Make timestamp range check conditional Deepa Dinamani
2019-12-23  6:36                 ` Amir Goldstein
2019-12-24  1:15                   ` Deepa Dinamani
2019-12-28 22:13                     ` [PATCH v2] " Deepa Dinamani
2019-12-30  7:34                       ` Amir Goldstein
2020-01-03  6:46                         ` Deepa Dinamani
2020-01-03  9:58                           ` Amir Goldstein [this message]
2020-01-08  8:09                         ` Eryu Guan
2020-01-08  8:45                           ` Amir Goldstein
2020-01-08  9:50                             ` Eryu Guan
2020-01-17  9:09                               ` Amir Goldstein
2020-01-17 18:23                                 ` Deepa Dinamani
2020-01-17 19:01                                   ` Deepa Dinamani
2020-01-19  0:57                                     ` [PATCH v3 1/1] " Deepa Dinamani
2020-01-19  9:19                                       ` Amir Goldstein
2020-02-01  9:14                                         ` Eryu Guan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAOQ4uxgCrq1V-Hkzg9h+wW26zfd4WUHQx7n8rUsRTsX9YuP5sw@mail.gmail.com \
    --to=amir73il@gmail.com \
    --cc=arnd@arndb.de \
    --cc=deepa.kernel@gmail.com \
    --cc=fstests@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=guaneryu@gmail.com \
    --cc=sashal@kernel.org \
    --cc=y2038@lists.linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).