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

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

[Amir Goldstein]

A test for a regression from v5.15 reported by Ruiwen Zhao:
https://lore.kernel.org/linux-unionfs/CAKd=y5Hpg7J2gxrFT02F94o=FM9QvGp=kcH1Grctx8HzFYvpiA@mail.gmail.com/

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

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.

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.
+#
+. ./common/preamble
+_begin_fstest auto quick
+
+# Import common functions.
+. ./common/filter
+
+# 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
+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/
+
+# success, all done
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/overlay/082.out b/tests/overlay/082.out
new file mode 100644
index 00000000..2977f141
--- /dev/null
+++ b/tests/overlay/082.out
@@ -0,0 +1,2 @@
+QA output created by 082
+Silence is golden
-- 
2.34.1

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

[Zorro Lang]

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@mail.gmail.com/
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> 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.
> 
> 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.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick
> +
> +# Import common functions.
> +. ./common/filter
> +
> +# 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
> +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.

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?

Thanks,
Zorro

> +
> +# success, all done
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/overlay/082.out b/tests/overlay/082.out
> new file mode 100644
> index 00000000..2977f141
> --- /dev/null
> +++ b/tests/overlay/082.out
> @@ -0,0 +1,2 @@
> +QA output created by 082
> +Silence is golden
> -- 
> 2.34.1
> 

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

[Amir Goldstein]

On Wed, Sep 20, 2023 at 6:14 PM Zorro Lang <zlang@redhat.com> 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@mail.gmail.com/
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > 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.
> >
> > 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.
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +
> > +# 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
> > +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,
Amir.

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

[Zorro Lang]

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@redhat.com> 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@mail.gmail.com/

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@gmail.com>
> > > ---
> > >
> > > 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.
> > >
> > > 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 :)

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

Thanks,
Zorro

> 
> Thanks,
> Amir.
> 

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

[Amir Goldstein]

On Thu, Sep 21, 2023 at 9:26 AM Zorro Lang <zlang@redhat.com> 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@redhat.com> 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@mail.gmail.com/
>
> 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@gmail.com>
> > > > ---
> > > >
> > > > 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.

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

[Zorro Lang]

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@redhat.com> 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@redhat.com> 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@mail.gmail.com/
> >
> > 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@gmail.com>
> > > > > ---
> > > > >
> > > > > 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.
> 

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

[Amir Goldstein]

On Thu, Sep 21, 2023 at 5:24 PM Zorro Lang <zlang@redhat.com> wrote:
>
> 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@redhat.com> 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@redhat.com> 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@mail.gmail.com/
> > >
> > > 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@gmail.com>
> > > > > > ---
> > > > > >
> > > > > > 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 :)
>

I find it quite strange that symlink and special inodes inherit those
attributes, but then the attributes cannot be queried and not changed
on those inodes.
It seems to me like all the fs expect xfs have it wrong, but I don't think
that is likely to change.

Thanks,
Amir.