Re: [PATCH] overlay: add test for rename of lower symlink with NOATIME attr

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Sep 21, 2023 at 11:00:37AM +0300, Amir Goldstein wrote:
> On Thu, Sep 21, 2023 at 9:26 AM Zorro Lang <zlang@xxxxxxxxxx> wrote:
> >
> > On Wed, Sep 20, 2023 at 06:34:21PM +0300, Amir Goldstein wrote:
> > > On Wed, Sep 20, 2023 at 6:14 PM Zorro Lang <zlang@xxxxxxxxxx> wrote:
> > > >
> > > > On Wed, Sep 20, 2023 at 04:03:55PM +0300, Amir Goldstein wrote:
> > > > > A test for a regression from v5.15 reported by Ruiwen Zhao:
> > > > > https://lore.kernel.org/linux-unionfs/CAKd=y5Hpg7J2gxrFT02F94o=FM9QvGp=kcH1Grctx8HzFYvpiA@xxxxxxxxxxxxxx/
> >
> > Could you give one more sentence to tell what kind of regression
> > does this case test for? Not only a link address.
> >
> > > > >
> > > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > > > ---
> > > > >
> > > > > Zorro,
> > > > >
> > > > > This is a test for a regression in kernel v5.15.
> > > > > The fix was merged for 6.6-rc2 and has been picked for
> > > > > the upcoming LTS releases 5.15, 6.1, 6.5.
> > > > >
> > > > > The reproducer only manifests the bug in fs that inherit noatime flag,
> > > > > namely ext4, btrfs, ... but not xfs.
> 
> FYI, I made a mistake in the statement above.
> xfs does support inherit of noatime flag, but
> it does not inherit noatime for *symlinks*.
> 
> I added the _require_chattr_inherit helper that you suggested
> in v2, but it only checks for inherit of noatime flag (the 2nd _notrun).
> I did not add a helper for _require_chattr_inherit_symlink
> because it was too specific and so I left the 3rd _notrun
> open coded in the test in v2.

OK, if xfs thinks it's an expected behavior which won't be changed :)

Thanks,
Zorro

> 
> > > > >
> > > > > The test does _notrun on xfs for that reason.
> > > > >
> > > > > Thanks,
> > > > > Amir.
> > > > >
> > > > >  tests/overlay/082     | 68 +++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/overlay/082.out |  2 ++
> > > > >  2 files changed, 70 insertions(+)
> > > > >  create mode 100755 tests/overlay/082
> > > > >  create mode 100644 tests/overlay/082.out
> > > > >
> > > > > diff --git a/tests/overlay/082 b/tests/overlay/082
> > > > > new file mode 100755
> > > > > index 00000000..abea3c2b
> > > > > --- /dev/null
> > > > > +++ b/tests/overlay/082
> > > > > @@ -0,0 +1,68 @@
> > > > > +#! /bin/bash
> > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > +# Copyright (C) 2023 CTERA Networks. All Rights Reserved.
> > > > > +#
> > > > > +# FS QA Test 082
> > > > > +#
> > > > > +# kernel commit 72db82115d2b ("ovl: copy up sync/noatime fileattr flags")
> > > > > +# from v5.15 introduced a regression.
> >
> > Hi Amir,
> >
> > Thanks for this new regression test. More detailed (picky:) review points
> > as below ...
> >
> > So this commit is the one which introduced a regression. But we generally
> > say what kind of regression does this case test for, in this comment.
> >
> > > > > +#
> > > > > +. ./common/preamble
> > > > > +_begin_fstest auto quick
> >
> > According the source code of this case, please think about more detailed group
> > names, likes: "symlink", "copyup" and "atime".
> >
> > > > > +
> > > > > +# Import common functions.
> > > > > +. ./common/filter
> >
> > I think this case doesn't use any filter helpers, right?
> >
> > > > > +
> > > > > +# real QA test starts here
> > > > > +_supported_fs overlay
> > > > > +_fixed_by_kernel_commit ab048302026d \
> > > > > +     "ovl: fix failed copyup of fileattr on a symlink"
> > > > > +
> > > > > +_require_scratch
> > > > > +_require_chattr A
> > > > > +
> > > > > +# remove all files from previous runs
> > > > > +_scratch_mkfs
> > > > > +
> > > > > +# prepare lower test dir with NOATIME flag
> > > > > +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
> > > > > +mkdir -p $lowerdir/testdir
> > > > > +$CHATTR_PROG +A $lowerdir/testdir >> $seqres.full 2>&1 ||
> > > > > +     _notrun "base fs $OVL_BASE_FSTYP does not support No_Atime flag"
> > > > > +
> > > > > +# The NOATIME is inheritted to children symlink in ext4/fs2fs
> > > > > +# (and on tmpfs on recent kernels).
> > > > > +# The overlayfs test will not fail unless base fs is
> > > > > +# one of those filesystems.
> > > > > +#
> > > > > +# The problem with this inheritence is that the NOATIME flag is inheritted
> > > > > +# to a symlink and the flag does take effect, but there is no way to query
> > > > > +# the flag (lsattr) or change it (chattr) on a symlink, so overlayfs will
> > > > > +# fail when trying to copy up NOATIME flag from lower to upper symlink.
> > > > > +#
> > > > > +touch $lowerdir/testdir/foo
> > > > > +ln -sf foo $lowerdir/testdir/lnk
> > > > > +
> > > > > +$LSATTR_PROG -l $lowerdir/testdir/foo >> $seqres.full 2>&1
> > > > > +$LSATTR_PROG -l $lowerdir/testdir/foo | grep -q No_Atime || \
> > > > > +     _notrun "base fs $OVL_BASE_FSTYP does not inherit No_Atime flag"
> > > > > +
> > > > > +before=$(stat -c %x $lowerdir/testdir/lnk)
> > > > > +echo "symlink atime before readlink: $before" >> $seqres.full 2>&1
> >
> > I remember some filesystems' timestamp for atime (e.g. exfat) might have more
> > seconds granularity. So it would be better to `sleep 2s` at here.
> >
> > Correct me if someone fs need more or less :)
> >
> 
> That would be a futile waste of 2 seconds IMO, because
> Those niche fs probably do not support chattr and because
> This is an overlayfs regression test and overlayfs is highly
> unlikely to be running in production on those niche fs and it
> probably does not support many of them.
> 
> 
> > > > > +cat $lowerdir/testdir/lnk
> > > > > +after=$(stat -c %x $lowerdir/testdir/lnk)
> > > > > +echo "symlink atime after readlink: $after" >> $seqres.full 2>&1
> > > > > +
> > > > > +[ "$before" == "$after" ] || \
> > > > > +     _notrun "base fs $OVL_BASE_FSTYP does not inherit No_Atime flag on symlink"
> > > > > +
> > > > > +# mounting overlay
> > > > > +_scratch_mount
> > > > > +
> > > > > +# moving symlink will try to copy up lower symlink flags
> > > > > +mv $SCRATCH_MNT/testdir/lnk $SCRATCH_MNT/
> > > >
> > > > Lots of above codes are checking if the underlying fs supports No_Atime (and inherit),
> > > > and _notrun if not support. How about do these checking steps in a require_*
> > > > function locally or in common/, likes _require_noatime_inheritance(). And we also
> > > > can let _require_chattr accept one more argument to specify a test directory.
> > > >
> > >
> > > ok.
> > >
> > > > The "mv ..." command looks like the final testing step. If there's not that bug,
> > > > nothing happen, but I'm wondering what should happen if there's a bug?
> > >
> > > mv fails with error ENXIO, see linked bug report in commit message.
> >
> > Thanks, I think we can add "fails with error ENXIO at here, if the bug is reproduced" in
> > the comment of that "mv ..." command.
> >
> 
> Sure. no problem.
> I will post v2 soon with _require_chattr_inherit and
> above comments fixed.
> 
> Thanks for the review!
> Amir.
> 




[Index of Archives]     [Linux Filesystems Devel]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux