[PATCH] overlay: add test for copy up of lower file attributes

[PATCH] overlay: add test for copy up of lower file attributes

[Amir Goldstein]

Overlayfs copies up a subset of lower file attributes since kernel
commits:
173ff5c9ec37 ("ovl: consistent behavior for immutable/append-only inodes")
2e3f6e87c2b0 ("ovl: copy up sync/noatime fileattr flags")

This test verifies this functionality works correctly and that it
survives power failure and/or mount cycle.

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

Eryu,

This test is failing on master and passes on overlayfs-next.

Thanks,
Amir.

 tests/overlay/078     | 145 ++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/078.out |   2 +
 2 files changed, 147 insertions(+)
 create mode 100755 tests/overlay/078
 create mode 100644 tests/overlay/078.out

diff --git a/tests/overlay/078 b/tests/overlay/078
new file mode 100755
index 00000000..b43449d1
--- /dev/null
+++ b/tests/overlay/078
@@ -0,0 +1,145 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2018 Huawei.  All Rights Reserved.
+# Copyright (C) 2021 CTERA Networks. All Rights Reserved.
+#
+# FS QA Test 078
+#
+# Test copy up of lower file attributes.
+#
+# Overlayfs copies up a subset of lower file attributes since kernel commits:
+# 173ff5c9ec37 ("ovl: consistent behavior for immutable/append-only inodes")
+# 2e3f6e87c2b0 ("ovl: copy up sync/noatime fileattr flags")
+#
+# This test is similar and was derived from generic/507, but instead
+# of creating new files which are created in upper layer, prepare
+# the file with attributes in lower layer and verify that attributes
+# are not lost during copy up, (optional) shutdown and mount cycle.
+#
+. ./common/preamble
+_begin_fstest auto quick perms shutdown
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	$CHATTR_PROG -ai $lowertestfile &> /dev/null
+	rm -f $tmp.*
+}
+
+# Import common functions.
+. ./common/filter
+
+# real QA test starts here
+_supported_fs generic
+
+_require_command "$LSATTR_PROG" lasttr
+_require_command "$CHATTR_PROG" chattr
+_require_chattr ASai
+_require_xfs_io_command "syncfs"
+
+_require_scratch
+_require_scratch_shutdown
+
+lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
+upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
+workdir=$OVL_BASE_SCRATCH_MNT/$OVL_WORK
+lowertestfile=$lowerdir/testfile
+testfile=$SCRATCH_MNT/testfile
+
+_scratch_mkfs
+mkdir -p $lowerdir
+touch $lowertestfile
+_scratch_mount
+
+# Set another flag on lowertestfile and verify all flags
+# are kept though copy up (optional shutdown) and mount cycle
+do_check()
+{
+	attr=$1
+
+	echo "Test chattr +$1 $2" >> $seqres.full
+
+	$UMOUNT_PROG $SCRATCH_MNT
+
+	# Add attribute to lower file
+	$CHATTR_PROG +$attr $lowertestfile
+
+	# Re-create upperdir/workdir
+	rm -rf $upperdir $workdir
+	mkdir -p $upperdir $workdir
+
+	if [ "$2" == "shutdown" ]; then
+		$XFS_IO_PROG -r $lowertestfile -c "fsync" | _filter_xfs_io
+	fi
+
+	_scratch_mount
+
+	before=`$LSATTR_PROG $testfile`
+
+	# Write file in append mode to test copy up of append-only attribute
+	# Expect failure on write to immutable file
+	expect=0
+	if [ "$1" == "i" ]; then
+		expect=1
+	fi
+	$XFS_IO_PROG -a -c "pwrite -S 0x61 0 10" $testfile >> $seqres.full 2>&1
+	result=$?
+	if [ $result != $expect ]; then
+		echo "Write unexpectedly returned $result for file with attribute '$attr'"
+	fi
+
+	if [ "$2" == "shutdown" ]; then
+		$XFS_IO_PROG -r $testfile -c "fsync" | _filter_xfs_io
+		_scratch_shutdown | tee -a $seqres.full
+	fi
+
+	_scratch_cycle_mount
+
+	after=`$LSATTR_PROG $testfile`
+	echo "Before copy up: $before" >> $seqres.full
+	echo "After  copy up: $after" >> $seqres.full
+
+	# Verify attributes were not lost during copy up, shutdown and mount cycle
+	if [ "$before" != "$after" ]; then
+		echo "Before copy up: $before"
+		echo "After  copy up: $after"
+	fi
+
+	echo "Test chattr -$1 $2" >> $seqres.full
+
+	# Delete attribute from overlay file
+	$CHATTR_PROG -$attr $testfile
+
+	before=`$LSATTR_PROG $testfile`
+
+	if [ "$2" == "shutdown" ]; then
+		$XFS_IO_PROG -r $testfile -c "fsync" | _filter_xfs_io
+		_scratch_shutdown | tee -a $seqres.full
+	fi
+
+	_scratch_cycle_mount
+
+	after=`$LSATTR_PROG $testfile`
+	echo "Before mount cycle: $before" >> $seqres.full
+	echo "After  mount cycle: $after" >> $seqres.full
+
+	# Verify attribute deletion was not lost during shutdown or mount cycle
+	if [ "$before" != "$after" ]; then
+		echo "Before mount cycle: $before"
+		echo "After  mount cycle: $after"
+	fi
+}
+
+echo "Silence is golden"
+
+# This is the subset of attributes copied up by overlayfs since kernel
+# commit ...
+opts="A S a i"
+for i in $opts; do
+	do_check $i
+	do_check $i shutdown
+done
+
+status=0
+exit
diff --git a/tests/overlay/078.out b/tests/overlay/078.out
new file mode 100644
index 00000000..b8acea8c
--- /dev/null
+++ b/tests/overlay/078.out
@@ -0,0 +1,2 @@
+QA output created by 078
+Silence is golden
-- 
2.32.0

Re: [PATCH] overlay: add test for copy up of lower file attributes

[Eryu Guan]

On Thu, Jul 22, 2021 at 07:46:34PM +0300, Amir Goldstein wrote:
> Overlayfs copies up a subset of lower file attributes since kernel
> commits:
> 173ff5c9ec37 ("ovl: consistent behavior for immutable/append-only inodes")
> 2e3f6e87c2b0 ("ovl: copy up sync/noatime fileattr flags")
> 
> This test verifies this functionality works correctly and that it
> survives power failure and/or mount cycle.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good to me overall, just one minor question below.

> ---
> 
> Eryu,
> 
> This test is failing on master and passes on overlayfs-next.
> 
> Thanks,
> Amir.
> 
>  tests/overlay/078     | 145 ++++++++++++++++++++++++++++++++++++++++++
>  tests/overlay/078.out |   2 +
>  2 files changed, 147 insertions(+)
>  create mode 100755 tests/overlay/078
>  create mode 100644 tests/overlay/078.out
> 
> diff --git a/tests/overlay/078 b/tests/overlay/078
> new file mode 100755
> index 00000000..b43449d1
> --- /dev/null
> +++ b/tests/overlay/078
> @@ -0,0 +1,145 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2018 Huawei.  All Rights Reserved.
> +# Copyright (C) 2021 CTERA Networks. All Rights Reserved.
> +#
> +# FS QA Test 078
> +#
> +# Test copy up of lower file attributes.
> +#
> +# Overlayfs copies up a subset of lower file attributes since kernel commits:
> +# 173ff5c9ec37 ("ovl: consistent behavior for immutable/append-only inodes")
> +# 2e3f6e87c2b0 ("ovl: copy up sync/noatime fileattr flags")
> +#
> +# This test is similar and was derived from generic/507, but instead
> +# of creating new files which are created in upper layer, prepare
> +# the file with attributes in lower layer and verify that attributes
> +# are not lost during copy up, (optional) shutdown and mount cycle.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick perms shutdown

I noticed that generic/507 has the same groups defined, but I'm
wondering if 'perms' is right group, 'attr' seems a better fit to me.
And we could add 'copyup' group as well.

> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	cd /
> +	$CHATTR_PROG -ai $lowertestfile &> /dev/null
> +	rm -f $tmp.*
> +}
> +
> +# Import common functions.
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_fs generic

s/generic/overlay/

Thanks,
Eryu

> +
> +_require_command "$LSATTR_PROG" lasttr
> +_require_command "$CHATTR_PROG" chattr
> +_require_chattr ASai
> +_require_xfs_io_command "syncfs"
> +
> +_require_scratch
> +_require_scratch_shutdown
> +
> +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
> +upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
> +workdir=$OVL_BASE_SCRATCH_MNT/$OVL_WORK
> +lowertestfile=$lowerdir/testfile
> +testfile=$SCRATCH_MNT/testfile
> +
> +_scratch_mkfs
> +mkdir -p $lowerdir
> +touch $lowertestfile
> +_scratch_mount
> +
> +# Set another flag on lowertestfile and verify all flags
> +# are kept though copy up (optional shutdown) and mount cycle
> +do_check()
> +{
> +	attr=$1
> +
> +	echo "Test chattr +$1 $2" >> $seqres.full
> +
> +	$UMOUNT_PROG $SCRATCH_MNT
> +
> +	# Add attribute to lower file
> +	$CHATTR_PROG +$attr $lowertestfile
> +
> +	# Re-create upperdir/workdir
> +	rm -rf $upperdir $workdir
> +	mkdir -p $upperdir $workdir
> +
> +	if [ "$2" == "shutdown" ]; then
> +		$XFS_IO_PROG -r $lowertestfile -c "fsync" | _filter_xfs_io
> +	fi
> +
> +	_scratch_mount
> +
> +	before=`$LSATTR_PROG $testfile`
> +
> +	# Write file in append mode to test copy up of append-only attribute
> +	# Expect failure on write to immutable file
> +	expect=0
> +	if [ "$1" == "i" ]; then
> +		expect=1
> +	fi
> +	$XFS_IO_PROG -a -c "pwrite -S 0x61 0 10" $testfile >> $seqres.full 2>&1
> +	result=$?
> +	if [ $result != $expect ]; then
> +		echo "Write unexpectedly returned $result for file with attribute '$attr'"
> +	fi
> +
> +	if [ "$2" == "shutdown" ]; then
> +		$XFS_IO_PROG -r $testfile -c "fsync" | _filter_xfs_io
> +		_scratch_shutdown | tee -a $seqres.full
> +	fi
> +
> +	_scratch_cycle_mount
> +
> +	after=`$LSATTR_PROG $testfile`
> +	echo "Before copy up: $before" >> $seqres.full
> +	echo "After  copy up: $after" >> $seqres.full
> +
> +	# Verify attributes were not lost during copy up, shutdown and mount cycle
> +	if [ "$before" != "$after" ]; then
> +		echo "Before copy up: $before"
> +		echo "After  copy up: $after"
> +	fi
> +
> +	echo "Test chattr -$1 $2" >> $seqres.full
> +
> +	# Delete attribute from overlay file
> +	$CHATTR_PROG -$attr $testfile
> +
> +	before=`$LSATTR_PROG $testfile`
> +
> +	if [ "$2" == "shutdown" ]; then
> +		$XFS_IO_PROG -r $testfile -c "fsync" | _filter_xfs_io
> +		_scratch_shutdown | tee -a $seqres.full
> +	fi
> +
> +	_scratch_cycle_mount
> +
> +	after=`$LSATTR_PROG $testfile`
> +	echo "Before mount cycle: $before" >> $seqres.full
> +	echo "After  mount cycle: $after" >> $seqres.full
> +
> +	# Verify attribute deletion was not lost during shutdown or mount cycle
> +	if [ "$before" != "$after" ]; then
> +		echo "Before mount cycle: $before"
> +		echo "After  mount cycle: $after"
> +	fi
> +}
> +
> +echo "Silence is golden"
> +
> +# This is the subset of attributes copied up by overlayfs since kernel
> +# commit ...
> +opts="A S a i"
> +for i in $opts; do
> +	do_check $i
> +	do_check $i shutdown
> +done
> +
> +status=0
> +exit
> diff --git a/tests/overlay/078.out b/tests/overlay/078.out
> new file mode 100644
> index 00000000..b8acea8c
> --- /dev/null
> +++ b/tests/overlay/078.out
> @@ -0,0 +1,2 @@
> +QA output created by 078
> +Silence is golden
> -- 
> 2.32.0
> 

Re: [PATCH] overlay: add test for copy up of lower file attributes

[Amir Goldstein]

On Sun, Jul 25, 2021 at 7:27 PM Eryu Guan <guan@eryu.me> wrote:
>
> On Thu, Jul 22, 2021 at 07:46:34PM +0300, Amir Goldstein wrote:
> > Overlayfs copies up a subset of lower file attributes since kernel
> > commits:
> > 173ff5c9ec37 ("ovl: consistent behavior for immutable/append-only inodes")
> > 2e3f6e87c2b0 ("ovl: copy up sync/noatime fileattr flags")
> >
> > This test verifies this functionality works correctly and that it
> > survives power failure and/or mount cycle.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Looks good to me overall, just one minor question below.
>
> > ---
> >
> > Eryu,
> >
> > This test is failing on master and passes on overlayfs-next.
> >
> > Thanks,
> > Amir.
> >
> >  tests/overlay/078     | 145 ++++++++++++++++++++++++++++++++++++++++++
> >  tests/overlay/078.out |   2 +
> >  2 files changed, 147 insertions(+)
> >  create mode 100755 tests/overlay/078
> >  create mode 100644 tests/overlay/078.out
> >
> > diff --git a/tests/overlay/078 b/tests/overlay/078
> > new file mode 100755
> > index 00000000..b43449d1
> > --- /dev/null
> > +++ b/tests/overlay/078
> > @@ -0,0 +1,145 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2018 Huawei.  All Rights Reserved.
> > +# Copyright (C) 2021 CTERA Networks. All Rights Reserved.
> > +#
> > +# FS QA Test 078
> > +#
> > +# Test copy up of lower file attributes.
> > +#
> > +# Overlayfs copies up a subset of lower file attributes since kernel commits:
> > +# 173ff5c9ec37 ("ovl: consistent behavior for immutable/append-only inodes")
> > +# 2e3f6e87c2b0 ("ovl: copy up sync/noatime fileattr flags")
> > +#
> > +# This test is similar and was derived from generic/507, but instead
> > +# of creating new files which are created in upper layer, prepare
> > +# the file with attributes in lower layer and verify that attributes
> > +# are not lost during copy up, (optional) shutdown and mount cycle.
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick perms shutdown
>
> I noticed that generic/507 has the same groups defined, but I'm
> wondering if 'perms' is right group, 'attr' seems a better fit to me.

The term "attr" is now very much overloaded in filesystems.
It may refer to info of stat() (i.e. getattr())
it my refer to xattr and it may refer to lsattr/chattr,
which is referred to as fileattr in latest vfs API.

In fstests, most of the tests in the 'attr' group include
./common/attr which refers to ACL xattrs in particular...
so it is not a good fit IMO.

The group 'perm' is already used for overlayfs tests that deal with
immutable files and generic tests that deal with immutable files
don't really have a common group AFAICT.

I have no objection for creating a new group for this
purpose but that would involve marking all related tests and worse...
...finding a name for that group ;-)

> And we could add 'copyup' group as well.
>

Sure.

> > +
> > +# Override the default cleanup function.
> > +_cleanup()
> > +{
> > +     cd /
> > +     $CHATTR_PROG -ai $lowertestfile &> /dev/null
> > +     rm -f $tmp.*
> > +}
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +_supported_fs generic
>
> s/generic/overlay/

Oops. I assume you would be fix this typo on commit.

Thanks,
Amir.

Re: [PATCH] overlay: add test for copy up of lower file attributes

[Eryu Guan]

On Sun, Jul 25, 2021 at 09:21:03PM +0300, Amir Goldstein wrote:
> On Sun, Jul 25, 2021 at 7:27 PM Eryu Guan <guan@eryu.me> wrote:
> >
> > On Thu, Jul 22, 2021 at 07:46:34PM +0300, Amir Goldstein wrote:
> > > Overlayfs copies up a subset of lower file attributes since kernel
> > > commits:
> > > 173ff5c9ec37 ("ovl: consistent behavior for immutable/append-only inodes")
> > > 2e3f6e87c2b0 ("ovl: copy up sync/noatime fileattr flags")
> > >
> > > This test verifies this functionality works correctly and that it
> > > survives power failure and/or mount cycle.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Looks good to me overall, just one minor question below.
> >
> > > ---
> > >
> > > Eryu,
> > >
> > > This test is failing on master and passes on overlayfs-next.
> > >
> > > Thanks,
> > > Amir.
> > >
> > >  tests/overlay/078     | 145 ++++++++++++++++++++++++++++++++++++++++++
> > >  tests/overlay/078.out |   2 +
> > >  2 files changed, 147 insertions(+)
> > >  create mode 100755 tests/overlay/078
> > >  create mode 100644 tests/overlay/078.out
> > >
> > > diff --git a/tests/overlay/078 b/tests/overlay/078
> > > new file mode 100755
> > > index 00000000..b43449d1
> > > --- /dev/null
> > > +++ b/tests/overlay/078
> > > @@ -0,0 +1,145 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2018 Huawei.  All Rights Reserved.
> > > +# Copyright (C) 2021 CTERA Networks. All Rights Reserved.
> > > +#
> > > +# FS QA Test 078
> > > +#
> > > +# Test copy up of lower file attributes.
> > > +#
> > > +# Overlayfs copies up a subset of lower file attributes since kernel commits:
> > > +# 173ff5c9ec37 ("ovl: consistent behavior for immutable/append-only inodes")
> > > +# 2e3f6e87c2b0 ("ovl: copy up sync/noatime fileattr flags")
> > > +#
> > > +# This test is similar and was derived from generic/507, but instead
> > > +# of creating new files which are created in upper layer, prepare
> > > +# the file with attributes in lower layer and verify that attributes
> > > +# are not lost during copy up, (optional) shutdown and mount cycle.
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick perms shutdown
> >
> > I noticed that generic/507 has the same groups defined, but I'm
> > wondering if 'perms' is right group, 'attr' seems a better fit to me.
> 
> The term "attr" is now very much overloaded in filesystems.
> It may refer to info of stat() (i.e. getattr())
> it my refer to xattr and it may refer to lsattr/chattr,
> which is referred to as fileattr in latest vfs API.
> 
> In fstests, most of the tests in the 'attr' group include
> ./common/attr which refers to ACL xattrs in particular...
> so it is not a good fit IMO.
> 
> The group 'perm' is already used for overlayfs tests that deal with
> immutable files and generic tests that deal with immutable files
> don't really have a common group AFAICT.

Makes sense to me. Then I'll take 'perm' for now.

> 
> I have no objection for creating a new group for this
> purpose but that would involve marking all related tests and worse...
> ...finding a name for that group ;-)

Yeah, naming is always a problem :)

> 
> > And we could add 'copyup' group as well.
> >
> 
> Sure.
> 
> > > +
> > > +# Override the default cleanup function.
> > > +_cleanup()
> > > +{
> > > +     cd /
> > > +     $CHATTR_PROG -ai $lowertestfile &> /dev/null
> > > +     rm -f $tmp.*
> > > +}
> > > +
> > > +# Import common functions.
> > > +. ./common/filter
> > > +
> > > +# real QA test starts here
> > > +_supported_fs generic
> >
> > s/generic/overlay/
> 
> Oops. I assume you would be fix this typo on commit.

Sure, I'll pick it up in next week's update and fix supported fs and add
copyup group.

Thanks,
Eryu

Re: [PATCH] overlay: add test for copy up of lower file attributes

[Darrick J. Wong]

On Thu, Jul 22, 2021 at 07:46:34PM +0300, Amir Goldstein wrote:
> Overlayfs copies up a subset of lower file attributes since kernel
> commits:
> 173ff5c9ec37 ("ovl: consistent behavior for immutable/append-only inodes")
> 2e3f6e87c2b0 ("ovl: copy up sync/noatime fileattr flags")
> 
> This test verifies this functionality works correctly and that it
> survives power failure and/or mount cycle.

Just out of curiosity -- is this supposed to succeed with a 5.14-rc4
kernel?  I noticed a massive regression with this week's fstests,
probably because something didn't get cleaned up properly:

--- overlay/078.out
+++ overlay/078.out.bad
@@ -1,2 +1,17 @@
 QA output created by 078
 Silence is golden
+Before copy up: -------A-------------- /opt/ovl-mnt/testfile
+After  copy up: ---------------------- /opt/ovl-mnt/testfile
+Before copy up: -------A-------------- /opt/ovl-mnt/testfile
+After  copy up: ---------------------- /opt/ovl-mnt/testfile
+Before copy up: --S----A-------------- /opt/ovl-mnt/testfile
+After  copy up: ---------------------- /opt/ovl-mnt/testfile
+Before copy up: --S----A-------------- /opt/ovl-mnt/testfile
+After  copy up: ---------------------- /opt/ovl-mnt/testfile
+Before copy up: --S--a-A-------------- /opt/ovl-mnt/testfile
+After  copy up: ---------------------- /opt/ovl-mnt/testfile
+Before copy up: --S--a-A-------------- /opt/ovl-mnt/testfile
+After  copy up: ---------------------- /opt/ovl-mnt/testfile
+rm: cannot remove '/opt/ovl-upper/testfile': Operation not permitted
+rm: cannot remove
'/opt/ovl-work/index/00fb2100812f1a30dc474847dbad5281308293ece9030e00020000000054816fd1':
Operation not permitted
+Write unexpectedly returned 0 for file with attribute 'i'

and then the tests after it (e.g. generic/030) fail with:

+mount: /opt/ovl-mnt: mount(2) system call failed: Stale file handle.
+mount failed
+(see /var/tmp/fstests/generic/030.full for details)


--D

> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Eryu,
> 
> This test is failing on master and passes on overlayfs-next.
> 
> Thanks,
> Amir.
> 
>  tests/overlay/078     | 145 ++++++++++++++++++++++++++++++++++++++++++
>  tests/overlay/078.out |   2 +
>  2 files changed, 147 insertions(+)
>  create mode 100755 tests/overlay/078
>  create mode 100644 tests/overlay/078.out
> 
> diff --git a/tests/overlay/078 b/tests/overlay/078
> new file mode 100755
> index 00000000..b43449d1
> --- /dev/null
> +++ b/tests/overlay/078
> @@ -0,0 +1,145 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2018 Huawei.  All Rights Reserved.
> +# Copyright (C) 2021 CTERA Networks. All Rights Reserved.
> +#
> +# FS QA Test 078
> +#
> +# Test copy up of lower file attributes.
> +#
> +# Overlayfs copies up a subset of lower file attributes since kernel commits:
> +# 173ff5c9ec37 ("ovl: consistent behavior for immutable/append-only inodes")
> +# 2e3f6e87c2b0 ("ovl: copy up sync/noatime fileattr flags")
> +#
> +# This test is similar and was derived from generic/507, but instead
> +# of creating new files which are created in upper layer, prepare
> +# the file with attributes in lower layer and verify that attributes
> +# are not lost during copy up, (optional) shutdown and mount cycle.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick perms shutdown
> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	cd /
> +	$CHATTR_PROG -ai $lowertestfile &> /dev/null
> +	rm -f $tmp.*
> +}
> +
> +# Import common functions.
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_fs generic
> +
> +_require_command "$LSATTR_PROG" lasttr
> +_require_command "$CHATTR_PROG" chattr
> +_require_chattr ASai
> +_require_xfs_io_command "syncfs"
> +
> +_require_scratch
> +_require_scratch_shutdown
> +
> +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
> +upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
> +workdir=$OVL_BASE_SCRATCH_MNT/$OVL_WORK
> +lowertestfile=$lowerdir/testfile
> +testfile=$SCRATCH_MNT/testfile
> +
> +_scratch_mkfs
> +mkdir -p $lowerdir
> +touch $lowertestfile
> +_scratch_mount
> +
> +# Set another flag on lowertestfile and verify all flags
> +# are kept though copy up (optional shutdown) and mount cycle
> +do_check()
> +{
> +	attr=$1
> +
> +	echo "Test chattr +$1 $2" >> $seqres.full
> +
> +	$UMOUNT_PROG $SCRATCH_MNT
> +
> +	# Add attribute to lower file
> +	$CHATTR_PROG +$attr $lowertestfile
> +
> +	# Re-create upperdir/workdir
> +	rm -rf $upperdir $workdir
> +	mkdir -p $upperdir $workdir
> +
> +	if [ "$2" == "shutdown" ]; then
> +		$XFS_IO_PROG -r $lowertestfile -c "fsync" | _filter_xfs_io
> +	fi
> +
> +	_scratch_mount
> +
> +	before=`$LSATTR_PROG $testfile`
> +
> +	# Write file in append mode to test copy up of append-only attribute
> +	# Expect failure on write to immutable file
> +	expect=0
> +	if [ "$1" == "i" ]; then
> +		expect=1
> +	fi
> +	$XFS_IO_PROG -a -c "pwrite -S 0x61 0 10" $testfile >> $seqres.full 2>&1
> +	result=$?
> +	if [ $result != $expect ]; then
> +		echo "Write unexpectedly returned $result for file with attribute '$attr'"
> +	fi
> +
> +	if [ "$2" == "shutdown" ]; then
> +		$XFS_IO_PROG -r $testfile -c "fsync" | _filter_xfs_io
> +		_scratch_shutdown | tee -a $seqres.full
> +	fi
> +
> +	_scratch_cycle_mount
> +
> +	after=`$LSATTR_PROG $testfile`
> +	echo "Before copy up: $before" >> $seqres.full
> +	echo "After  copy up: $after" >> $seqres.full
> +
> +	# Verify attributes were not lost during copy up, shutdown and mount cycle
> +	if [ "$before" != "$after" ]; then
> +		echo "Before copy up: $before"
> +		echo "After  copy up: $after"
> +	fi
> +
> +	echo "Test chattr -$1 $2" >> $seqres.full
> +
> +	# Delete attribute from overlay file
> +	$CHATTR_PROG -$attr $testfile
> +
> +	before=`$LSATTR_PROG $testfile`
> +
> +	if [ "$2" == "shutdown" ]; then
> +		$XFS_IO_PROG -r $testfile -c "fsync" | _filter_xfs_io
> +		_scratch_shutdown | tee -a $seqres.full
> +	fi
> +
> +	_scratch_cycle_mount
> +
> +	after=`$LSATTR_PROG $testfile`
> +	echo "Before mount cycle: $before" >> $seqres.full
> +	echo "After  mount cycle: $after" >> $seqres.full
> +
> +	# Verify attribute deletion was not lost during shutdown or mount cycle
> +	if [ "$before" != "$after" ]; then
> +		echo "Before mount cycle: $before"
> +		echo "After  mount cycle: $after"
> +	fi
> +}
> +
> +echo "Silence is golden"
> +
> +# This is the subset of attributes copied up by overlayfs since kernel
> +# commit ...
> +opts="A S a i"
> +for i in $opts; do
> +	do_check $i
> +	do_check $i shutdown
> +done
> +
> +status=0
> +exit
> diff --git a/tests/overlay/078.out b/tests/overlay/078.out
> new file mode 100644
> index 00000000..b8acea8c
> --- /dev/null
> +++ b/tests/overlay/078.out
> @@ -0,0 +1,2 @@
> +QA output created by 078
> +Silence is golden
> -- 
> 2.32.0
> 

Re: [PATCH] overlay: add test for copy up of lower file attributes

[Amir Goldstein]

On Tue, Aug 3, 2021 at 2:07 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Thu, Jul 22, 2021 at 07:46:34PM +0300, Amir Goldstein wrote:
> > Overlayfs copies up a subset of lower file attributes since kernel
> > commits:
> > 173ff5c9ec37 ("ovl: consistent behavior for immutable/append-only inodes")
> > 2e3f6e87c2b0 ("ovl: copy up sync/noatime fileattr flags")
> >
> > This test verifies this functionality works correctly and that it
> > survives power failure and/or mount cycle.
>
> Just out of curiosity -- is this supposed to succeed with a 5.14-rc4
> kernel?

No. The documented fix commits are in linux-next.
Looks like they are heading for 5.15-rc1.

> I noticed a massive regression with this week's fstests,
> probably because something didn't get cleaned up properly:
>
> --- overlay/078.out
> +++ overlay/078.out.bad
> @@ -1,2 +1,17 @@
>  QA output created by 078
>  Silence is golden
> +Before copy up: -------A-------------- /opt/ovl-mnt/testfile
> +After  copy up: ---------------------- /opt/ovl-mnt/testfile
> +Before copy up: -------A-------------- /opt/ovl-mnt/testfile
> +After  copy up: ---------------------- /opt/ovl-mnt/testfile
> +Before copy up: --S----A-------------- /opt/ovl-mnt/testfile
> +After  copy up: ---------------------- /opt/ovl-mnt/testfile
> +Before copy up: --S----A-------------- /opt/ovl-mnt/testfile
> +After  copy up: ---------------------- /opt/ovl-mnt/testfile
> +Before copy up: --S--a-A-------------- /opt/ovl-mnt/testfile
> +After  copy up: ---------------------- /opt/ovl-mnt/testfile
> +Before copy up: --S--a-A-------------- /opt/ovl-mnt/testfile
> +After  copy up: ---------------------- /opt/ovl-mnt/testfile
> +rm: cannot remove '/opt/ovl-upper/testfile': Operation not permitted
> +rm: cannot remove
> '/opt/ovl-work/index/00fb2100812f1a30dc474847dbad5281308293ece9030e00020000000054816fd1':

I'm curious, are you running with non-default mount/config options
on purpose? i.e. index=on or nfs_export=on?

> Operation not permitted
> +Write unexpectedly returned 0 for file with attribute 'i'

Oops, sorry. The problem is even sooner than _cleanup().
I hadn't noticed it because the head snippet of 078.out.bad was expected
and I did not look past it.

>
> and then the tests after it (e.g. generic/030) fail with:
>
> +mount: /opt/ovl-mnt: mount(2) system call failed: Stale file handle.
> +mount failed
> +(see /var/tmp/fstests/generic/030.full for details)
>

And I hadn't noticed that because overlay/078 is the last test to reuse
/opt/ovl-upper/ and kvm-xfstests zaps the base fs before every run.

Posted a fix.
Sorry for the trouble.

Thanks,
Amir.

Re: [PATCH] overlay: add test for copy up of lower file attributes

[Darrick J. Wong]

On Tue, Aug 03, 2021 at 10:21:02AM +0300, Amir Goldstein wrote:
> On Tue, Aug 3, 2021 at 2:07 AM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Thu, Jul 22, 2021 at 07:46:34PM +0300, Amir Goldstein wrote:
> > > Overlayfs copies up a subset of lower file attributes since kernel
> > > commits:
> > > 173ff5c9ec37 ("ovl: consistent behavior for immutable/append-only inodes")
> > > 2e3f6e87c2b0 ("ovl: copy up sync/noatime fileattr flags")
> > >
> > > This test verifies this functionality works correctly and that it
> > > survives power failure and/or mount cycle.
> >
> > Just out of curiosity -- is this supposed to succeed with a 5.14-rc4
> > kernel?
> 
> No. The documented fix commits are in linux-next.
> Looks like they are heading for 5.15-rc1.
> 
> > I noticed a massive regression with this week's fstests,
> > probably because something didn't get cleaned up properly:
> >
> > --- overlay/078.out
> > +++ overlay/078.out.bad
> > @@ -1,2 +1,17 @@
> >  QA output created by 078
> >  Silence is golden
> > +Before copy up: -------A-------------- /opt/ovl-mnt/testfile
> > +After  copy up: ---------------------- /opt/ovl-mnt/testfile
> > +Before copy up: -------A-------------- /opt/ovl-mnt/testfile
> > +After  copy up: ---------------------- /opt/ovl-mnt/testfile
> > +Before copy up: --S----A-------------- /opt/ovl-mnt/testfile
> > +After  copy up: ---------------------- /opt/ovl-mnt/testfile
> > +Before copy up: --S----A-------------- /opt/ovl-mnt/testfile
> > +After  copy up: ---------------------- /opt/ovl-mnt/testfile
> > +Before copy up: --S--a-A-------------- /opt/ovl-mnt/testfile
> > +After  copy up: ---------------------- /opt/ovl-mnt/testfile
> > +Before copy up: --S--a-A-------------- /opt/ovl-mnt/testfile
> > +After  copy up: ---------------------- /opt/ovl-mnt/testfile
> > +rm: cannot remove '/opt/ovl-upper/testfile': Operation not permitted
> > +rm: cannot remove
> > '/opt/ovl-work/index/00fb2100812f1a30dc474847dbad5281308293ece9030e00020000000054816fd1':
> 
> I'm curious, are you running with non-default mount/config options
> on purpose? i.e. index=on or nfs_export=on?
> 
> > Operation not permitted
> > +Write unexpectedly returned 0 for file with attribute 'i'
> 
> Oops, sorry. The problem is even sooner than _cleanup().
> I hadn't noticed it because the head snippet of 078.out.bad was expected
> and I did not look past it.

./check -r ;)

> 
> >
> > and then the tests after it (e.g. generic/030) fail with:
> >
> > +mount: /opt/ovl-mnt: mount(2) system call failed: Stale file handle.
> > +mount failed
> > +(see /var/tmp/fstests/generic/030.full for details)
> >
> 
> And I hadn't noticed that because overlay/078 is the last test to reuse
> /opt/ovl-upper/ and kvm-xfstests zaps the base fs before every run.
> 
> Posted a fix.
> Sorry for the trouble.

I'll give it a spin overnight.

--D

> Thanks,
> Amir.