On Wed, Jan 8, 2020 at 10:09 AM Eryu Guan <guaneryu@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > > Signed-off-by: Deepa Dinamani <deepa.kernel@xxxxxxxxx>
>
> 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.
