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
next prev parent 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).