[PATCH v2 0/2] Test for overlayfs fix in v6.6-rc2

[PATCH v2 0/2] Test for overlayfs fix in v6.6-rc2

[Amir Goldstein]

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
on symlink namely ext4, btrfs, ... but not xfs.

The test does _notrun on overlayfs over xfs for that reason.

Thanks,
Amir.

Changes since v1:
- Create helper _require_chattr_inherit
- Improve documnetation of the regression

Amir Goldstein (2):
  common: add helper _require_chattr_inherit
  overlay: add test for rename of lower symlink with NOATIME attr

 common/rc             | 52 ++++++++++++++++++++++++++++------
 tests/overlay/082     | 66 +++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/082.out |  2 ++
 3 files changed, 111 insertions(+), 9 deletions(-)
 create mode 100755 tests/overlay/082
 create mode 100644 tests/overlay/082.out

-- 
2.34.1

[PATCH v2 1/2] common: add helper _require_chattr_inherit

[Amir Goldstein]

Similar to _require_chattr, but also checks if an attribute is
inheritted from parent dir to children.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 common/rc | 52 +++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 9 deletions(-)

diff --git a/common/rc b/common/rc
index 1618ded5..00cfd434 100644
--- a/common/rc
+++ b/common/rc
@@ -4235,23 +4235,57 @@ _require_test_lsattr()
 		_notrun "lsattr not supported by test filesystem type: $FSTYP"
 }
 
+_check_chattr_inherit()
+{
+	local attribute=$1
+	local path=$2
+	local inherit=$3
+
+	touch $path
+	$CHATTR_PROG "+$attribute" $path > $tmp.chattr 2>&1
+	local ret=$?
+	if [ -n "$inherit" ]; then
+		touch "$path/$inherit"
+	fi
+	$CHATTR_PROG "-$attribute" $path > $tmp.chattr 2>&1
+	if [ "$ret" -ne 0 ]; then
+		_notrun "file system doesn't support chattr +$attribute"
+	fi
+	cat $tmp.chattr >> $seqres.full
+	rm -f $tmp.chattr
+	return $ret
+}
+
 _require_chattr()
 {
 	if [ -z "$1" ]; then
 		echo "Usage: _require_chattr <attr>"
 		exit 1
 	fi
-	local attribute=$1
+	_check_chattr_inherit $1 $TEST_DIR/syscalltest
+}
 
-	touch $TEST_DIR/syscalltest
-	chattr "+$attribute" $TEST_DIR/syscalltest > $TEST_DIR/syscalltest.out 2>&1
-	local ret=$?
-	chattr "-$attribute" $TEST_DIR/syscalltest > $TEST_DIR/syscalltest.out 2>&1
-	if [ "$ret" -ne 0 ]; then
-		_notrun "file system doesn't support chattr +$attribute"
+_require_chattr_inherit()
+{
+	if [ -z "$1" ]; then
+		echo "Usage: _require_chattr_inherit <attr>"
+		exit 1
 	fi
-	cat $TEST_DIR/syscalltest.out >> $seqres.full
-	rm -f $TEST_DIR/syscalltest.out
+	local attribute=$1
+	local testdir="$TEST_DIR/chattrtest"
+	mkdir -p $testdir
+	_check_chattr_inherit $attribute $testdir testfile || \
+		return
+
+	local testfile="$TEST_DIR/chattrtest/testfile"
+	local lsattrout=($($LSATTR_PROG $testfile 2>> $seqres.full))
+	echo ${lsattrout[*]} >> $seqres.full
+	echo ${lsattrout[0]} | grep -q $attribute || \
+		_notrun "file system doesn't inherit chattr +$attribute"
+
+	$CHATTR_PROG "-$attribute" $testfile >> $seqres.full 2>&1
+	rm -f $testfile
+	rmdir $testdir
 }
 
 _get_total_inode()
-- 
2.34.1

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

[Amir Goldstein]

Test for a regression in copy up of symlink that has the S_NOATIME
inode flag.

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

In the bug report, the symlink has the S_NOATIME inode flag because it is
on an NFS/FUSE filesystem that sets S_NOATIME for all inodes.

The reproducer uses another technique to create a symlink with
S_NOATIME inode flag by using chattr +A inheritance on filesystems
that inherit chattr flags to symlinks.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/082     | 66 +++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/082.out |  2 ++
 2 files changed, 68 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..97ef445e
--- /dev/null
+++ b/tests/overlay/082
@@ -0,0 +1,66 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2023 CTERA Networks. All Rights Reserved.
+#
+# FS QA Test 082
+#
+# Test for a regression in copy up of symlink that has the noatime inode
+# attribute.
+#
+# kernel commit 72db82115d2b ("ovl: copy up sync/noatime fileattr flags")
+# from v5.15 introduced the regression.
+#
+. ./common/preamble
+_begin_fstest auto quick copyup symlink atime
+
+# 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_inherit 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 ||
+	echo "failed to set No_Atime flag on $lowerdir/testdir"
+
+# The NOATIME is inherited 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 inherited
+# 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
+
+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
+# and fails with error ENXIO, if the bug is reproduced
+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 v2 1/2] common: add helper _require_chattr_inherit

[Zorro Lang]

On Thu, Sep 21, 2023 at 05:31:01PM +0300, Amir Goldstein wrote:
> Similar to _require_chattr, but also checks if an attribute is
> inheritted from parent dir to children.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  common/rc | 52 +++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 43 insertions(+), 9 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 1618ded5..00cfd434 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4235,23 +4235,57 @@ _require_test_lsattr()
>  		_notrun "lsattr not supported by test filesystem type: $FSTYP"
>  }
>  
> +_check_chattr_inherit()
> +{
> +	local attribute=$1
> +	local path=$2
> +	local inherit=$3

As I understand, this function calls _check_chattr_inherit, so it will
return zero or non-zero to clarify if $path support $attribute inheritance.
...

> +
> +	touch $path
> +	$CHATTR_PROG "+$attribute" $path > $tmp.chattr 2>&1
> +	local ret=$?
> +	if [ -n "$inherit" ]; then
> +		touch "$path/$inherit"
> +	fi

... but looks like it doesn't, it only create a $inherit file, then let the
caller check if the $attribute is inherited.

I think that's a little confused. I think we can name the function as
_check_chattr() and the 3rd argument $inherit as a bool variable, to
decide if we check inheritance or not.

Or you'd like to have two functions _check_chattr and _check_chattr_inherit,
_check_chattr_inherit calls _check_chattr then keep checking inheritance.

What do you think?

Thanks,
Zorro

> +	$CHATTR_PROG "-$attribute" $path > $tmp.chattr 2>&1
> +	if [ "$ret" -ne 0 ]; then
> +		_notrun "file system doesn't support chattr +$attribute"
> +	fi
> +	cat $tmp.chattr >> $seqres.full
> +	rm -f $tmp.chattr
> +	return $ret
> +}
> +
>  _require_chattr()
>  {
>  	if [ -z "$1" ]; then
>  		echo "Usage: _require_chattr <attr>"
>  		exit 1
>  	fi
> -	local attribute=$1
> +	_check_chattr_inherit $1 $TEST_DIR/syscalltest
> +}
>  
> -	touch $TEST_DIR/syscalltest
> -	chattr "+$attribute" $TEST_DIR/syscalltest > $TEST_DIR/syscalltest.out 2>&1
> -	local ret=$?
> -	chattr "-$attribute" $TEST_DIR/syscalltest > $TEST_DIR/syscalltest.out 2>&1
> -	if [ "$ret" -ne 0 ]; then
> -		_notrun "file system doesn't support chattr +$attribute"
> +_require_chattr_inherit()
> +{
> +	if [ -z "$1" ]; then
> +		echo "Usage: _require_chattr_inherit <attr>"
> +		exit 1
>  	fi
> -	cat $TEST_DIR/syscalltest.out >> $seqres.full
> -	rm -f $TEST_DIR/syscalltest.out
> +	local attribute=$1
> +	local testdir="$TEST_DIR/chattrtest"
> +	mkdir -p $testdir
> +	_check_chattr_inherit $attribute $testdir testfile || \
> +		return
> +
> +	local testfile="$TEST_DIR/chattrtest/testfile"
> +	local lsattrout=($($LSATTR_PROG $testfile 2>> $seqres.full))
> +	echo ${lsattrout[*]} >> $seqres.full
> +	echo ${lsattrout[0]} | grep -q $attribute || \
> +		_notrun "file system doesn't inherit chattr +$attribute"
> +
> +	$CHATTR_PROG "-$attribute" $testfile >> $seqres.full 2>&1
> +	rm -f $testfile
> +	rmdir $testdir
>  }
>  
>  _get_total_inode()
> -- 
> 2.34.1
> 

Re: [PATCH v2 1/2] common: add helper _require_chattr_inherit

[Amir Goldstein]

On Thu, Sep 21, 2023 at 6:26 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Thu, Sep 21, 2023 at 05:31:01PM +0300, Amir Goldstein wrote:
> > Similar to _require_chattr, but also checks if an attribute is
> > inheritted from parent dir to children.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  common/rc | 52 +++++++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 43 insertions(+), 9 deletions(-)
> >
> > diff --git a/common/rc b/common/rc
> > index 1618ded5..00cfd434 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -4235,23 +4235,57 @@ _require_test_lsattr()
> >               _notrun "lsattr not supported by test filesystem type: $FSTYP"
> >  }
> >
> > +_check_chattr_inherit()
> > +{
> > +     local attribute=$1
> > +     local path=$2
> > +     local inherit=$3
>
> As I understand, this function calls _check_chattr_inherit, so it will
> return zero or non-zero to clarify if $path support $attribute inheritance.
> ...
>
> > +
> > +     touch $path
> > +     $CHATTR_PROG "+$attribute" $path > $tmp.chattr 2>&1
> > +     local ret=$?
> > +     if [ -n "$inherit" ]; then
> > +             touch "$path/$inherit"
> > +     fi
>
> ... but looks like it doesn't, it only create a $inherit file, then let the
> caller check if the $attribute is inherited.
>
> I think that's a little confused.

I agree.

> I think we can name the function as _check_chattr()

I agree.

> and the 3rd argument $inherit as a bool variable, to
> decide if we check inheritance or not.
>

Not my prefered choice.

> Or you'd like to have two functions _check_chattr and _check_chattr_inherit,
> _check_chattr_inherit calls _check_chattr then keep checking inheritance.
>
> What do you think?

I think this is over engineering for a helper that may not
be ever used by any other test.

Suggest to just change the name to _check_chattr()
to match the meaning to the return value.

The 3rd inherit argument just means that we request
to create a file after chattr + and before chattr -,
so that the caller could check it itself later.

If you accept this minor change is enough
can you apply it yourself on commit?

Thanks,
Amir.

Re: [PATCH v2 1/2] common: add helper _require_chattr_inherit

[Zorro Lang]

On Thu, Sep 21, 2023 at 06:40:49PM +0300, Amir Goldstein wrote:
> On Thu, Sep 21, 2023 at 6:26 PM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Thu, Sep 21, 2023 at 05:31:01PM +0300, Amir Goldstein wrote:
> > > Similar to _require_chattr, but also checks if an attribute is
> > > inheritted from parent dir to children.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  common/rc | 52 +++++++++++++++++++++++++++++++++++++++++++---------
> > >  1 file changed, 43 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/common/rc b/common/rc
> > > index 1618ded5..00cfd434 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -4235,23 +4235,57 @@ _require_test_lsattr()
> > >               _notrun "lsattr not supported by test filesystem type: $FSTYP"
> > >  }
> > >
> > > +_check_chattr_inherit()
> > > +{
> > > +     local attribute=$1
> > > +     local path=$2
> > > +     local inherit=$3
> >
> > As I understand, this function calls _check_chattr_inherit, so it will
> > return zero or non-zero to clarify if $path support $attribute inheritance.
> > ...
> >
> > > +
> > > +     touch $path
> > > +     $CHATTR_PROG "+$attribute" $path > $tmp.chattr 2>&1
> > > +     local ret=$?
> > > +     if [ -n "$inherit" ]; then
> > > +             touch "$path/$inherit"
> > > +     fi
> >
> > ... but looks like it doesn't, it only create a $inherit file, then let the
> > caller check if the $attribute is inherited.
> >
> > I think that's a little confused.
> 
> I agree.
> 
> > I think we can name the function as _check_chattr()
> 
> I agree.
> 
> > and the 3rd argument $inherit as a bool variable, to
> > decide if we check inheritance or not.
> >
> 
> Not my prefered choice.
> 
> > Or you'd like to have two functions _check_chattr and _check_chattr_inherit,
> > _check_chattr_inherit calls _check_chattr then keep checking inheritance.
> >
> > What do you think?
> 
> I think this is over engineering for a helper that may not
> be ever used by any other test.
> 
> Suggest to just change the name to _check_chattr()
> to match the meaning to the return value.
> 
> The 3rd inherit argument just means that we request
> to create a file after chattr + and before chattr -,
> so that the caller could check it itself later.

I still think it doesn't make sense ... but I don't want to give you
that pressure, so ...

> 
> If you accept this minor change is enough
> can you apply it yourself on commit?

... If you think it's too complicated, we can drop the inheritance checking
common helper. Just change the _require_chattr(), make it to accept one more
*directory* argument (use $TEST_DIR by default). Then we can do:

_require_chattr A $BASE_SCRATCH_MNT
_require_chattr A $SCRATCH_MNT

And then do all inheritance checking in the overlay case itself (likes you did in
V1), don't make them to be a common helper. Due to only this case need the
_require_chattr_inheritance, so we can think about that common helper when more
cases need that :)

I think this change is simple enough (base on your V1 patch). Is that good to
you :)

Thanks,
Zorro

> 
> Thanks,
> Amir.
> 

Re: [PATCH v2 1/2] common: add helper _require_chattr_inherit

[Amir Goldstein]

On Thu, Sep 21, 2023 at 7:23 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Thu, Sep 21, 2023 at 06:40:49PM +0300, Amir Goldstein wrote:
> > On Thu, Sep 21, 2023 at 6:26 PM Zorro Lang <zlang@redhat.com> wrote:
> > >
> > > On Thu, Sep 21, 2023 at 05:31:01PM +0300, Amir Goldstein wrote:
> > > > Similar to _require_chattr, but also checks if an attribute is
> > > > inheritted from parent dir to children.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > ---
> > > >  common/rc | 52 +++++++++++++++++++++++++++++++++++++++++++---------
> > > >  1 file changed, 43 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/common/rc b/common/rc
> > > > index 1618ded5..00cfd434 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -4235,23 +4235,57 @@ _require_test_lsattr()
> > > >               _notrun "lsattr not supported by test filesystem type: $FSTYP"
> > > >  }
> > > >
> > > > +_check_chattr_inherit()
> > > > +{
> > > > +     local attribute=$1
> > > > +     local path=$2
> > > > +     local inherit=$3
> > >
> > > As I understand, this function calls _check_chattr_inherit, so it will
> > > return zero or non-zero to clarify if $path support $attribute inheritance.
> > > ...
> > >
> > > > +
> > > > +     touch $path
> > > > +     $CHATTR_PROG "+$attribute" $path > $tmp.chattr 2>&1
> > > > +     local ret=$?
> > > > +     if [ -n "$inherit" ]; then
> > > > +             touch "$path/$inherit"
> > > > +     fi
> > >
> > > ... but looks like it doesn't, it only create a $inherit file, then let the
> > > caller check if the $attribute is inherited.
> > >
> > > I think that's a little confused.
> >
> > I agree.
> >
> > > I think we can name the function as _check_chattr()
> >
> > I agree.
> >
> > > and the 3rd argument $inherit as a bool variable, to
> > > decide if we check inheritance or not.
> > >
> >
> > Not my prefered choice.
> >
> > > Or you'd like to have two functions _check_chattr and _check_chattr_inherit,
> > > _check_chattr_inherit calls _check_chattr then keep checking inheritance.
> > >
> > > What do you think?
> >
> > I think this is over engineering for a helper that may not
> > be ever used by any other test.
> >
> > Suggest to just change the name to _check_chattr()
> > to match the meaning to the return value.
> >
> > The 3rd inherit argument just means that we request
> > to create a file after chattr + and before chattr -,
> > so that the caller could check it itself later.
>
> I still think it doesn't make sense ... but I don't want to give you
> that pressure, so ...
>
> >
> > If you accept this minor change is enough
> > can you apply it yourself on commit?
>
> ... If you think it's too complicated, we can drop the inheritance checking
> common helper. Just change the _require_chattr(), make it to accept one more
> *directory* argument (use $TEST_DIR by default). Then we can do:
>
> _require_chattr A $BASE_SCRATCH_MNT

This is not needed.
Overlayfs (on $SCRATCH_MNT) will not pass the require_chattr
check if the base fs does not support chattr.

> _require_chattr A $SCRATCH_MNT

This is practically equivalent to _require_chattr A
on the test partition, there is no reason to test the
scratch partition specifically.

So there is no need for the proposed change in _require_chattr.

>
> And then do all inheritance checking in the overlay case itself (likes you did in
> V1), don't make them to be a common helper. Due to only this case need the
> _require_chattr_inheritance, so we can think about that common helper when more
> cases need that :)
>
> I think this change is simple enough (base on your V1 patch). Is that good to
> you :)

V1 is good enough for me as is. :)
You can take the commit message and test
header comment fixes from V2.

Note that the common _require_chattr_inheritance in V2
almost did not remove any lines from the test at all -
it moved one _notrun line into _require_chattr_inheritance
and turned another _notrun into echo "fail".

So I agree that if no other test is going to use the new helpers
their value is limited.

Thanks,
Amir.

Re: [PATCH v2 1/2] common: add helper _require_chattr_inherit

[Zorro Lang]

On Thu, Sep 21, 2023 at 07:46:12PM +0300, Amir Goldstein wrote:
> On Thu, Sep 21, 2023 at 7:23 PM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Thu, Sep 21, 2023 at 06:40:49PM +0300, Amir Goldstein wrote:
> > > On Thu, Sep 21, 2023 at 6:26 PM Zorro Lang <zlang@redhat.com> wrote:
> > > >
> > > > On Thu, Sep 21, 2023 at 05:31:01PM +0300, Amir Goldstein wrote:
> > > > > Similar to _require_chattr, but also checks if an attribute is
> > > > > inheritted from parent dir to children.
> > > > >
> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > ---
> > > > >  common/rc | 52 +++++++++++++++++++++++++++++++++++++++++++---------
> > > > >  1 file changed, 43 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/common/rc b/common/rc
> > > > > index 1618ded5..00cfd434 100644
> > > > > --- a/common/rc
> > > > > +++ b/common/rc
> > > > > @@ -4235,23 +4235,57 @@ _require_test_lsattr()
> > > > >               _notrun "lsattr not supported by test filesystem type: $FSTYP"
> > > > >  }
> > > > >
> > > > > +_check_chattr_inherit()
> > > > > +{
> > > > > +     local attribute=$1
> > > > > +     local path=$2
> > > > > +     local inherit=$3
> > > >
> > > > As I understand, this function calls _check_chattr_inherit, so it will
> > > > return zero or non-zero to clarify if $path support $attribute inheritance.
> > > > ...
> > > >
> > > > > +
> > > > > +     touch $path
> > > > > +     $CHATTR_PROG "+$attribute" $path > $tmp.chattr 2>&1
> > > > > +     local ret=$?
> > > > > +     if [ -n "$inherit" ]; then
> > > > > +             touch "$path/$inherit"
> > > > > +     fi
> > > >
> > > > ... but looks like it doesn't, it only create a $inherit file, then let the
> > > > caller check if the $attribute is inherited.
> > > >
> > > > I think that's a little confused.
> > >
> > > I agree.
> > >
> > > > I think we can name the function as _check_chattr()
> > >
> > > I agree.
> > >
> > > > and the 3rd argument $inherit as a bool variable, to
> > > > decide if we check inheritance or not.
> > > >
> > >
> > > Not my prefered choice.
> > >
> > > > Or you'd like to have two functions _check_chattr and _check_chattr_inherit,
> > > > _check_chattr_inherit calls _check_chattr then keep checking inheritance.
> > > >
> > > > What do you think?
> > >
> > > I think this is over engineering for a helper that may not
> > > be ever used by any other test.
> > >
> > > Suggest to just change the name to _check_chattr()
> > > to match the meaning to the return value.
> > >
> > > The 3rd inherit argument just means that we request
> > > to create a file after chattr + and before chattr -,
> > > so that the caller could check it itself later.
> >
> > I still think it doesn't make sense ... but I don't want to give you
> > that pressure, so ...
> >
> > >
> > > If you accept this minor change is enough
> > > can you apply it yourself on commit?
> >
> > ... If you think it's too complicated, we can drop the inheritance checking
> > common helper. Just change the _require_chattr(), make it to accept one more
> > *directory* argument (use $TEST_DIR by default). Then we can do:
> >
> > _require_chattr A $BASE_SCRATCH_MNT
> 
> This is not needed.
> Overlayfs (on $SCRATCH_MNT) will not pass the require_chattr
> check if the base fs does not support chattr.

Oh, I saw you wrote as this:

  +# 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"

so I thought you might want a `_require_chattr A $BASE_SCRATCH_MNT`.

> 
> > _require_chattr A $SCRATCH_MNT
> 
> This is practically equivalent to _require_chattr A
> on the test partition, there is no reason to test the
> scratch partition specifically.
> 
> So there is no need for the proposed change in _require_chattr.
> 
> >
> > And then do all inheritance checking in the overlay case itself (likes you did in
> > V1), don't make them to be a common helper. Due to only this case need the
> > _require_chattr_inheritance, so we can think about that common helper when more
> > cases need that :)
> >
> > I think this change is simple enough (base on your V1 patch). Is that good to
> > you :)
> 
> V1 is good enough for me as is. :)
> You can take the commit message and test
> header comment fixes from V2.

I'll merge v2 patch 2/2, with ...
1) Change _require_chattr_inherit to _require_chattr.
2) Add `sleep 2s` under the "before=$(stat -c %x $lowerdir/testdir/lnk)" line.

Thanks,
Zorro

> 
> Note that the common _require_chattr_inheritance in V2
> almost did not remove any lines from the test at all -
> it moved one _notrun line into _require_chattr_inheritance
> and turned another _notrun into echo "fail".
> 
> So I agree that if no other test is going to use the new helpers
> their value is limited.
> 
> Thanks,
> Amir.
> 

Re: [PATCH v2 1/2] common: add helper _require_chattr_inherit

[Amir Goldstein]

On Thu, Sep 21, 2023 at 8:06 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Thu, Sep 21, 2023 at 07:46:12PM +0300, Amir Goldstein wrote:
> > On Thu, Sep 21, 2023 at 7:23 PM Zorro Lang <zlang@redhat.com> wrote:
> > >
> > > On Thu, Sep 21, 2023 at 06:40:49PM +0300, Amir Goldstein wrote:
> > > > On Thu, Sep 21, 2023 at 6:26 PM Zorro Lang <zlang@redhat.com> wrote:
> > > > >
> > > > > On Thu, Sep 21, 2023 at 05:31:01PM +0300, Amir Goldstein wrote:
> > > > > > Similar to _require_chattr, but also checks if an attribute is
> > > > > > inheritted from parent dir to children.
> > > > > >
> > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > > ---
> > > > > >  common/rc | 52 +++++++++++++++++++++++++++++++++++++++++++---------
> > > > > >  1 file changed, 43 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > diff --git a/common/rc b/common/rc
> > > > > > index 1618ded5..00cfd434 100644
> > > > > > --- a/common/rc
> > > > > > +++ b/common/rc
> > > > > > @@ -4235,23 +4235,57 @@ _require_test_lsattr()
> > > > > >               _notrun "lsattr not supported by test filesystem type: $FSTYP"
> > > > > >  }
> > > > > >
> > > > > > +_check_chattr_inherit()
> > > > > > +{
> > > > > > +     local attribute=$1
> > > > > > +     local path=$2
> > > > > > +     local inherit=$3
> > > > >
> > > > > As I understand, this function calls _check_chattr_inherit, so it will
> > > > > return zero or non-zero to clarify if $path support $attribute inheritance.
> > > > > ...
> > > > >
> > > > > > +
> > > > > > +     touch $path
> > > > > > +     $CHATTR_PROG "+$attribute" $path > $tmp.chattr 2>&1
> > > > > > +     local ret=$?
> > > > > > +     if [ -n "$inherit" ]; then
> > > > > > +             touch "$path/$inherit"
> > > > > > +     fi
> > > > >
> > > > > ... but looks like it doesn't, it only create a $inherit file, then let the
> > > > > caller check if the $attribute is inherited.
> > > > >
> > > > > I think that's a little confused.
> > > >
> > > > I agree.
> > > >
> > > > > I think we can name the function as _check_chattr()
> > > >
> > > > I agree.
> > > >
> > > > > and the 3rd argument $inherit as a bool variable, to
> > > > > decide if we check inheritance or not.
> > > > >
> > > >
> > > > Not my prefered choice.
> > > >
> > > > > Or you'd like to have two functions _check_chattr and _check_chattr_inherit,
> > > > > _check_chattr_inherit calls _check_chattr then keep checking inheritance.
> > > > >
> > > > > What do you think?
> > > >
> > > > I think this is over engineering for a helper that may not
> > > > be ever used by any other test.
> > > >
> > > > Suggest to just change the name to _check_chattr()
> > > > to match the meaning to the return value.
> > > >
> > > > The 3rd inherit argument just means that we request
> > > > to create a file after chattr + and before chattr -,
> > > > so that the caller could check it itself later.
> > >
> > > I still think it doesn't make sense ... but I don't want to give you
> > > that pressure, so ...
> > >
> > > >
> > > > If you accept this minor change is enough
> > > > can you apply it yourself on commit?
> > >
> > > ... If you think it's too complicated, we can drop the inheritance checking
> > > common helper. Just change the _require_chattr(), make it to accept one more
> > > *directory* argument (use $TEST_DIR by default). Then we can do:
> > >
> > > _require_chattr A $BASE_SCRATCH_MNT
> >
> > This is not needed.
> > Overlayfs (on $SCRATCH_MNT) will not pass the require_chattr
> > check if the base fs does not support chattr.
>
> Oh, I saw you wrote as this:
>
>   +# 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"
>
> so I thought you might want a `_require_chattr A $BASE_SCRATCH_MNT`.
>

You are right. it should have been an error, not _notrun
as it is in V2.

> >
> > > _require_chattr A $SCRATCH_MNT
> >
> > This is practically equivalent to _require_chattr A
> > on the test partition, there is no reason to test the
> > scratch partition specifically.
> >
> > So there is no need for the proposed change in _require_chattr.
> >
> > >
> > > And then do all inheritance checking in the overlay case itself (likes you did in
> > > V1), don't make them to be a common helper. Due to only this case need the
> > > _require_chattr_inheritance, so we can think about that common helper when more
> > > cases need that :)
> > >
> > > I think this change is simple enough (base on your V1 patch). Is that good to
> > > you :)
> >
> > V1 is good enough for me as is. :)
> > You can take the commit message and test
> > header comment fixes from V2.
>
> I'll merge v2 patch 2/2, with ...
> 1) Change _require_chattr_inherit to _require_chattr.
> 2) Add `sleep 2s` under the "before=$(stat -c %x $lowerdir/testdir/lnk)" line.
>

OK.

Thanks!
Amir.