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. > > > > > > > > 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.
