[PATCH v4] overlay/072: test for whiteout inode sharing

[PATCH v4] overlay/072: test for whiteout inode sharing

[Chengguang Xu]

This is a test for whiteout inode sharing feature.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
v1->v2:
- Address Amir's comments in v1

v2->v3:
- Address Amir's comments in v2 

v3->v4:
- Fix test case based on latest kernel patch(removed module param)
https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/commit/?h=overlayfs-next&id=4e49695244661568130bfefcb6143dd1eaa3d8e7

 tests/overlay/072     | 106 ++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/072.out |   2 +
 tests/overlay/group   |   1 +
 3 files changed, 109 insertions(+)
 create mode 100755 tests/overlay/072
 create mode 100644 tests/overlay/072.out

diff --git a/tests/overlay/072 b/tests/overlay/072
new file mode 100755
index 00000000..fc847092
--- /dev/null
+++ b/tests/overlay/072
@@ -0,0 +1,106 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2020 Chengguang Xu <cgxu519@mykernel.net>.
+# All Rights Reserved.
+#
+# FS QA Test 072
+#
+# Test whiteout inode sharing functionality.
+#
+# A "whiteout" is an object that has special meaning in overlayfs.
+# A whiteout on an upper layer will effectively hide a matching file
+# in the lower layer, making it appear as if the file didn't exist.
+#
+# Whiteout inode sharing means multiple whiteout objects will share
+# one inode in upper layer, without this feature every whiteout object
+# will consume one inode in upper layer.
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+_supported_fs overlay
+_supported_os Linux
+_require_scratch
+
+lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
+upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
+workdir=$OVL_BASE_SCRATCH_MNT/$OVL_WORK
+
+# Make some testing files in lowerdir.
+# Argument:
+# $1: Testing file number
+make_lower_files()
+{
+	for name in `seq ${1}`; do
+		touch $lowerdir/file${name} &>/dev/null
+	done
+}
+
+# Delete all copy-uped files in upperdir.
+make_whiteout_files()
+{
+	rm -f $SCRATCH_MNT/* &>/dev/null
+}
+
+# Check link count of whiteout files.
+# Arguments:
+# $1: Testing file number
+# $2: Expected link count
+check_whiteout_files()
+{
+	for name in `seq ${1}`; do
+		local real_count=`stat -c %h $upperdir/file${name} 2>/dev/null`
+		if [[ ${2} != $real_count ]]; then
+			echo "Expected link count is ${2} but real count is $real_count, file name is file${name}"
+		fi
+	done
+	local tmpfile_count=`ls $workdir/work/\#* $workdir/index/\#* 2>/dev/null |wc -l 2>/dev/null`
+	if [[ -n "$tmpfile_count" && $tmpfile_count > 1 ]]; then
+		echo "There are more than one whiteout tmpfile in work/index dir!"
+		ls -l $workdir/work/\#* $workdir/index/\#* 2>/dev/null
+	fi
+}
+
+# Run test case with specific arguments.
+# Arguments:
+# $1: Testing file number
+# $2: Expected link count
+run_test_case()
+{
+	_scratch_mkfs
+	make_lower_files ${1}
+	_scratch_mount
+	make_whiteout_files
+	check_whiteout_files ${1} ${2}
+	_scratch_unmount
+}
+
+#Test case
+file_count=10
+link_count=11
+run_test_case $file_count $link_count
+
+# success, all done
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/overlay/072.out b/tests/overlay/072.out
new file mode 100644
index 00000000..590bbc6c
--- /dev/null
+++ b/tests/overlay/072.out
@@ -0,0 +1,2 @@
+QA output created by 072
+Silence is golden
diff --git a/tests/overlay/group b/tests/overlay/group
index 43ad8a52..8b2276f1 100644
--- a/tests/overlay/group
+++ b/tests/overlay/group
@@ -74,3 +74,4 @@
 069 auto quick copyup hardlink exportfs nested nonsamefs
 070 auto quick copyup redirect nested
 071 auto quick copyup redirect nested nonsamefs
+072 auto quick whiteout
-- 
2.20.1

Re: [PATCH v4] overlay/072: test for whiteout inode sharing

[Amir Goldstein]

On Wed, May 6, 2020 at 1:15 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> This is a test for whiteout inode sharing feature.
>
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
> v1->v2:
> - Address Amir's comments in v1
>
> v2->v3:
> - Address Amir's comments in v2
>
> v3->v4:
> - Fix test case based on latest kernel patch(removed module param)
> https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/commit/?h=overlayfs-next&id=4e49695244661568130bfefcb6143dd1eaa3d8e7
>
>  tests/overlay/072     | 106 ++++++++++++++++++++++++++++++++++++++++++
>  tests/overlay/072.out |   2 +
>  tests/overlay/group   |   1 +
>  3 files changed, 109 insertions(+)
>  create mode 100755 tests/overlay/072
>  create mode 100644 tests/overlay/072.out
>
> diff --git a/tests/overlay/072 b/tests/overlay/072
> new file mode 100755
> index 00000000..fc847092
> --- /dev/null
> +++ b/tests/overlay/072
> @@ -0,0 +1,106 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2020 Chengguang Xu <cgxu519@mykernel.net>.
> +# All Rights Reserved.
> +#
> +# FS QA Test 072
> +#
> +# Test whiteout inode sharing functionality.
> +#
> +# A "whiteout" is an object that has special meaning in overlayfs.
> +# A whiteout on an upper layer will effectively hide a matching file
> +# in the lower layer, making it appear as if the file didn't exist.
> +#
> +# Whiteout inode sharing means multiple whiteout objects will share
> +# one inode in upper layer, without this feature every whiteout object
> +# will consume one inode in upper layer.
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1       # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +       cd /
> +       rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +_supported_fs overlay
> +_supported_os Linux
> +_require_scratch
> +
> +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
> +upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
> +workdir=$OVL_BASE_SCRATCH_MNT/$OVL_WORK
> +
> +# Make some testing files in lowerdir.
> +# Argument:
> +# $1: Testing file number
> +make_lower_files()
> +{
> +       for name in `seq ${1}`; do
> +               touch $lowerdir/file${name} &>/dev/null
> +       done
> +}
> +
> +# Delete all copy-uped files in upperdir.
> +make_whiteout_files()
> +{
> +       rm -f $SCRATCH_MNT/* &>/dev/null
> +}
> +
> +# Check link count of whiteout files.
> +# Arguments:
> +# $1: Testing file number
> +# $2: Expected link count
> +check_whiteout_files()
> +{
> +       for name in `seq ${1}`; do
> +               local real_count=`stat -c %h $upperdir/file${name} 2>/dev/null`
> +               if [[ ${2} != $real_count ]]; then
> +                       echo "Expected link count is ${2} but real count is $real_count, file name is file${name}"
> +               fi
> +       done
> +       local tmpfile_count=`ls $workdir/work/\#* $workdir/index/\#* 2>/dev/null |wc -l 2>/dev/null`
> +       if [[ -n "$tmpfile_count" && $tmpfile_count > 1 ]]; then
> +               echo "There are more than one whiteout tmpfile in work/index dir!"
> +               ls -l $workdir/work/\#* $workdir/index/\#* 2>/dev/null
> +       fi
> +}
> +
> +# Run test case with specific arguments.
> +# Arguments:
> +# $1: Testing file number
> +# $2: Expected link count
> +run_test_case()
> +{
> +       _scratch_mkfs
> +       make_lower_files ${1}
> +       _scratch_mount
> +       make_whiteout_files
> +       check_whiteout_files ${1} ${2}
> +       _scratch_unmount
> +}
> +
> +#Test case
> +file_count=10
> +link_count=11
> +run_test_case $file_count $link_count
> +
> +# success, all done
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/overlay/072.out b/tests/overlay/072.out
> new file mode 100644
> index 00000000..590bbc6c
> --- /dev/null
> +++ b/tests/overlay/072.out
> @@ -0,0 +1,2 @@
> +QA output created by 072
> +Silence is golden
> diff --git a/tests/overlay/group b/tests/overlay/group
> index 43ad8a52..8b2276f1 100644
> --- a/tests/overlay/group
> +++ b/tests/overlay/group
> @@ -74,3 +74,4 @@
>  069 auto quick copyup hardlink exportfs nested nonsamefs
>  070 auto quick copyup redirect nested
>  071 auto quick copyup redirect nested nonsamefs
> +072 auto quick whiteout
> --
> 2.20.1
>
>

Re: [PATCH v4] overlay: test for whiteout inode sharing

[Eryu Guan]

On Wed, May 06, 2020 at 06:15:28PM +0800, Chengguang Xu wrote:
> This is a test for whiteout inode sharing feature.
> 
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> ---
> v1->v2:
> - Address Amir's comments in v1
> 
> v2->v3:
> - Address Amir's comments in v2 
> 
> v3->v4:
> - Fix test case based on latest kernel patch(removed module param)
> https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/commit/?h=overlayfs-next&id=4e49695244661568130bfefcb6143dd1eaa3d8e7
> 
>  tests/overlay/073     | 106 ++++++++++++++++++++++++++++++++++++++++++
>  tests/overlay/073.out |   2 +
>  tests/overlay/group   |   1 +
>  3 files changed, 109 insertions(+)
>  create mode 100755 tests/overlay/073
>  create mode 100644 tests/overlay/073.out
> 
> diff --git a/tests/overlay/073 b/tests/overlay/073
> new file mode 100755
> index 00000000..fc847092
> --- /dev/null
> +++ b/tests/overlay/073
> @@ -0,0 +1,106 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2020 Chengguang Xu <cgxu519@mykernel.net>.
> +# All Rights Reserved.
> +#
> +# FS QA Test 073
> +#
> +# Test whiteout inode sharing functionality.
> +#
> +# A "whiteout" is an object that has special meaning in overlayfs.
> +# A whiteout on an upper layer will effectively hide a matching file
> +# in the lower layer, making it appear as if the file didn't exist.
> +#
> +# Whiteout inode sharing means multiple whiteout objects will share
> +# one inode in upper layer, without this feature every whiteout object
> +# will consume one inode in upper layer.
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +_supported_fs overlay
> +_supported_os Linux
> +_require_scratch

I see no feature detection logic, so test just fails on old kernels
without this feature? I tried with v5.7-r4 kernel, test fails because
each whiteout file has only one hardlink.

Thanks,
Eryu

> +
> +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
> +upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
> +workdir=$OVL_BASE_SCRATCH_MNT/$OVL_WORK
> +
> +# Make some testing files in lowerdir.
> +# Argument:
> +# $1: Testing file number
> +make_lower_files()
> +{
> +	for name in `seq ${1}`; do
> +		touch $lowerdir/file${name} &>/dev/null
> +	done
> +}
> +
> +# Delete all copy-uped files in upperdir.
> +make_whiteout_files()
> +{
> +	rm -f $SCRATCH_MNT/* &>/dev/null
> +}
> +
> +# Check link count of whiteout files.
> +# Arguments:
> +# $1: Testing file number
> +# $2: Expected link count
> +check_whiteout_files()
> +{
> +	for name in `seq ${1}`; do
> +		local real_count=`stat -c %h $upperdir/file${name} 2>/dev/null`
> +		if [[ ${2} != $real_count ]]; then
> +			echo "Expected link count is ${2} but real count is $real_count, file name is file${name}"
> +		fi
> +	done
> +	local tmpfile_count=`ls $workdir/work/\#* $workdir/index/\#* 2>/dev/null |wc -l 2>/dev/null`
> +	if [[ -n "$tmpfile_count" && $tmpfile_count > 1 ]]; then
> +		echo "There are more than one whiteout tmpfile in work/index dir!"
> +		ls -l $workdir/work/\#* $workdir/index/\#* 2>/dev/null
> +	fi
> +}
> +
> +# Run test case with specific arguments.
> +# Arguments:
> +# $1: Testing file number
> +# $2: Expected link count
> +run_test_case()
> +{
> +	_scratch_mkfs
> +	make_lower_files ${1}
> +	_scratch_mount
> +	make_whiteout_files
> +	check_whiteout_files ${1} ${2}
> +	_scratch_unmount
> +}
> +
> +#Test case
> +file_count=10
> +link_count=11
> +run_test_case $file_count $link_count
> +
> +# success, all done
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/overlay/073.out b/tests/overlay/073.out
> new file mode 100644
> index 00000000..590bbc6c
> --- /dev/null
> +++ b/tests/overlay/073.out
> @@ -0,0 +1,2 @@
> +QA output created by 073
> +Silence is golden
> diff --git a/tests/overlay/group b/tests/overlay/group
> index 43ad8a52..8b2276f1 100644
> --- a/tests/overlay/group
> +++ b/tests/overlay/group
> @@ -74,3 +74,4 @@
>  070 auto quick copyup redirect nested
>  071 auto quick copyup redirect nested nonsamefs
>  072 auto quick copyup hardlink
> +073 auto quick whiteout
> -- 
> 2.20.1
> 

Re: [PATCH v4] overlay: test for whiteout inode sharing

[Chengguang Xu]

 ---- 在 星期日, 2020-05-10 23:50:37 Eryu Guan <guan@eryu.me> 撰写 ----
 > On Wed, May 06, 2020 at 06:15:28PM +0800, Chengguang Xu wrote:
 > > This is a test for whiteout inode sharing feature.
 > > 
 > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > > ---
 > > v1->v2:
 > > - Address Amir's comments in v1
 > > 
 > > v2->v3:
 > > - Address Amir's comments in v2 
 > > 
 > > v3->v4:
 > > - Fix test case based on latest kernel patch(removed module param)
 > > https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/commit/?h=overlayfs-next&id=4e49695244661568130bfefcb6143dd1eaa3d8e7
 > > 
 > >  tests/overlay/073     | 106 ++++++++++++++++++++++++++++++++++++++++++
 > >  tests/overlay/073.out |   2 +
 > >  tests/overlay/group   |   1 +
 > >  3 files changed, 109 insertions(+)
 > >  create mode 100755 tests/overlay/073
 > >  create mode 100644 tests/overlay/073.out
 > > 
 > > diff --git a/tests/overlay/073 b/tests/overlay/073
 > > new file mode 100755
 > > index 00000000..fc847092
 > > --- /dev/null
 > > +++ b/tests/overlay/073
 > > @@ -0,0 +1,106 @@
 > > +#! /bin/bash
 > > +# SPDX-License-Identifier: GPL-2.0
 > > +# Copyright (c) 2020 Chengguang Xu <cgxu519@mykernel.net>.
 > > +# All Rights Reserved.
 > > +#
 > > +# FS QA Test 073
 > > +#
 > > +# Test whiteout inode sharing functionality.
 > > +#
 > > +# A "whiteout" is an object that has special meaning in overlayfs.
 > > +# A whiteout on an upper layer will effectively hide a matching file
 > > +# in the lower layer, making it appear as if the file didn't exist.
 > > +#
 > > +# Whiteout inode sharing means multiple whiteout objects will share
 > > +# one inode in upper layer, without this feature every whiteout object
 > > +# will consume one inode in upper layer.
 > > +
 > > +seq=`basename $0`
 > > +seqres=$RESULT_DIR/$seq
 > > +echo "QA output created by $seq"
 > > +
 > > +here=`pwd`
 > > +tmp=/tmp/$
 > > +status=1    # failure is the default!
 > > +trap "_cleanup; exit \$status" 0 1 2 3 15
 > > +
 > > +_cleanup()
 > > +{
 > > +    cd /
 > > +    rm -f $tmp.*
 > > +}
 > > +
 > > +# get standard environment, filters and checks
 > > +. ./common/rc
 > > +. ./common/filter
 > > +
 > > +# remove previous $seqres.full before test
 > > +rm -f $seqres.full
 > > +
 > > +# real QA test starts here
 > > +_supported_fs overlay
 > > +_supported_os Linux
 > > +_require_scratch
 > 
 > I see no feature detection logic, so test just fails on old kernels
 > without this feature? I tried with v5.7-r4 kernel, test fails because
 > each whiteout file has only one hardlink.
 
That's true.

Thanks,
cgxu

Re: [PATCH v4] overlay: test for whiteout inode sharing

[Eryu Guan]

On Mon, May 11, 2020 at 09:32:20AM +0800, Chengguang Xu wrote:
>  ---- 在 星期日, 2020-05-10 23:50:37 Eryu Guan <guan@eryu.me> 撰写 ----
>  > On Wed, May 06, 2020 at 06:15:28PM +0800, Chengguang Xu wrote:
>  > > This is a test for whiteout inode sharing feature.
>  > > 
>  > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
>  > > ---
>  > > v1->v2:
>  > > - Address Amir's comments in v1
>  > > 
>  > > v2->v3:
>  > > - Address Amir's comments in v2 
>  > > 
>  > > v3->v4:
>  > > - Fix test case based on latest kernel patch(removed module param)
>  > > https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/commit/?h=overlayfs-next&id=4e49695244661568130bfefcb6143dd1eaa3d8e7
>  > > 
>  > >  tests/overlay/073     | 106 ++++++++++++++++++++++++++++++++++++++++++
>  > >  tests/overlay/073.out |   2 +
>  > >  tests/overlay/group   |   1 +
>  > >  3 files changed, 109 insertions(+)
>  > >  create mode 100755 tests/overlay/073
>  > >  create mode 100644 tests/overlay/073.out
>  > > 
>  > > diff --git a/tests/overlay/073 b/tests/overlay/073
>  > > new file mode 100755
>  > > index 00000000..fc847092
>  > > --- /dev/null
>  > > +++ b/tests/overlay/073
>  > > @@ -0,0 +1,106 @@
>  > > +#! /bin/bash
>  > > +# SPDX-License-Identifier: GPL-2.0
>  > > +# Copyright (c) 2020 Chengguang Xu <cgxu519@mykernel.net>.
>  > > +# All Rights Reserved.
>  > > +#
>  > > +# FS QA Test 073
>  > > +#
>  > > +# Test whiteout inode sharing functionality.
>  > > +#
>  > > +# A "whiteout" is an object that has special meaning in overlayfs.
>  > > +# A whiteout on an upper layer will effectively hide a matching file
>  > > +# in the lower layer, making it appear as if the file didn't exist.
>  > > +#
>  > > +# Whiteout inode sharing means multiple whiteout objects will share
>  > > +# one inode in upper layer, without this feature every whiteout object
>  > > +# will consume one inode in upper layer.
>  > > +
>  > > +seq=`basename $0`
>  > > +seqres=$RESULT_DIR/$seq
>  > > +echo "QA output created by $seq"
>  > > +
>  > > +here=`pwd`
>  > > +tmp=/tmp/$
>  > > +status=1    # failure is the default!
>  > > +trap "_cleanup; exit \$status" 0 1 2 3 15
>  > > +
>  > > +_cleanup()
>  > > +{
>  > > +    cd /
>  > > +    rm -f $tmp.*
>  > > +}
>  > > +
>  > > +# get standard environment, filters and checks
>  > > +. ./common/rc
>  > > +. ./common/filter
>  > > +
>  > > +# remove previous $seqres.full before test
>  > > +rm -f $seqres.full
>  > > +
>  > > +# real QA test starts here
>  > > +_supported_fs overlay
>  > > +_supported_os Linux
>  > > +_require_scratch
>  > 
>  > I see no feature detection logic, so test just fails on old kernels
>  > without this feature? I tried with v5.7-r4 kernel, test fails because
>  > each whiteout file has only one hardlink.
>  
> That's true.

I'd like to see it _notrun on old kernels where the feature is not
available. But that seems hard to do.. Do you have any better ideas?

Thanks,
Eryu

Re: [PATCH v4] overlay: test for whiteout inode sharing

[Amir Goldstein]

> >  > I see no feature detection logic, so test just fails on old kernels
> >  > without this feature? I tried with v5.7-r4 kernel, test fails because
> >  > each whiteout file has only one hardlink.
> >
> > That's true.
>
> I'd like to see it _notrun on old kernels where the feature is not
> available. But that seems hard to do.. Do you have any better ideas?
>

I've got a few.
1. LTP has the concept of require minimum kernel version.
    This would mean that functionality will be not be tested if feature
    is backported to old kernels.
2. We could add to overlayfs advertising of supported features, like
     /sys/fs/ext4/features/, but it already does "advertise" the configurable
     features at  /sys/module/overlay/parameters/, and we were already
     asking the question during patch review:
        /* Is there a reason anyone would want not to share whiteouts? */
        ofs->share_whiteout = true;
     and we left the answer to "later" time.

So a simple solution would be to add the module parameter (without adding
a mount option), because:
- It doesn't hurt (?)
- Somebody may end up using it, for some reason we did not think of
- We can use it in test to require the feature

The one non-trivial thing that this will require is to add Documentation
of the module parameter in the section about Whiteouts and opaque directories.

Thanks,
Amir.

Re: [PATCH v4] overlay: test for whiteout inode sharing

[Eryu Guan]

On Tue, May 12, 2020 at 07:56:35PM +0300, Amir Goldstein wrote:
> > >  > I see no feature detection logic, so test just fails on old kernels
> > >  > without this feature? I tried with v5.7-r4 kernel, test fails because
> > >  > each whiteout file has only one hardlink.
> > >
> > > That's true.
> >
> > I'd like to see it _notrun on old kernels where the feature is not
> > available. But that seems hard to do.. Do you have any better ideas?
> >
> 
> I've got a few.
> 1. LTP has the concept of require minimum kernel version.
>     This would mean that functionality will be not be tested if feature
>     is backported to old kernels.
> 2. We could add to overlayfs advertising of supported features, like
>      /sys/fs/ext4/features/, but it already does "advertise" the configurable
>      features at  /sys/module/overlay/parameters/, and we were already
>      asking the question during patch review:
>         /* Is there a reason anyone would want not to share whiteouts? */
>         ofs->share_whiteout = true;
>      and we left the answer to "later" time.
> 
> So a simple solution would be to add the module parameter (without adding
> a mount option), because:
> - It doesn't hurt (?)
> - Somebody may end up using it, for some reason we did not think of
> - We can use it in test to require the feature

Yeah, I think that works. And I see that ext4 and btrfs both have a
/sys/fs/<fs>/features directory and list supported features there, is
this something overlay could do? Or is this basically the same thing as
what you proposed?

Thanks,
Eryu

> 
> The one non-trivial thing that this will require is to add Documentation
> of the module parameter in the section about Whiteouts and opaque directories.
> 
> Thanks,
> Amir.

Re: [PATCH v4] overlay: test for whiteout inode sharing

[Chengguang Xu]

 ---- 在 星期三, 2020-05-13 09:10:19 Eryu Guan <eguan@linux.alibaba.com> 撰写 ----
 > On Tue, May 12, 2020 at 07:56:35PM +0300, Amir Goldstein wrote:
 > > > >  > I see no feature detection logic, so test just fails on old kernels
 > > > >  > without this feature? I tried with v5.7-r4 kernel, test fails because
 > > > >  > each whiteout file has only one hardlink.
 > > > >
 > > > > That's true.
 > > >
 > > > I'd like to see it _notrun on old kernels where the feature is not
 > > > available. But that seems hard to do.. Do you have any better ideas?
 > > >
 > > 
 > > I've got a few.
 > > 1. LTP has the concept of require minimum kernel version.
 > >     This would mean that functionality will be not be tested if feature
 > >     is backported to old kernels.
 > > 2. We could add to overlayfs advertising of supported features, like
 > >      /sys/fs/ext4/features/, but it already does "advertise" the configurable
 > >      features at  /sys/module/overlay/parameters/, and we were already
 > >      asking the question during patch review:
 > >         /* Is there a reason anyone would want not to share whiteouts? */
 > >         ofs->share_whiteout = true;
 > >      and we left the answer to "later" time.
 > > 
 > > So a simple solution would be to add the module parameter (without adding
 > > a mount option), because:
 > > - It doesn't hurt (?)
 > > - Somebody may end up using it, for some reason we did not think of
 > > - We can use it in test to require the feature
 > 
 > Yeah, I think that works. And I see that ext4 and btrfs both have a
 > /sys/fs/<fs>/features directory and list supported features there, is
 > this something overlay could do? Or is this basically the same thing as
 > what you proposed?
 > 

IMO, for those features which don't need to change module param, maybe feature list 
is more suitable.


Thanks,
cgxu
 

Re: [PATCH v4] overlay: test for whiteout inode sharing

[Amir Goldstein]

On Wed, May 13, 2020 at 6:17 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
>  ---- 在 星期三, 2020-05-13 09:10:19 Eryu Guan <eguan@linux.alibaba.com> 撰写 ----
>  > On Tue, May 12, 2020 at 07:56:35PM +0300, Amir Goldstein wrote:
>  > > > >  > I see no feature detection logic, so test just fails on old kernels
>  > > > >  > without this feature? I tried with v5.7-r4 kernel, test fails because
>  > > > >  > each whiteout file has only one hardlink.
>  > > > >
>  > > > > That's true.
>  > > >
>  > > > I'd like to see it _notrun on old kernels where the feature is not
>  > > > available. But that seems hard to do.. Do you have any better ideas?
>  > > >
>  > >
>  > > I've got a few.
>  > > 1. LTP has the concept of require minimum kernel version.
>  > >     This would mean that functionality will be not be tested if feature
>  > >     is backported to old kernels.
>  > > 2. We could add to overlayfs advertising of supported features, like
>  > >      /sys/fs/ext4/features/, but it already does "advertise" the configurable
>  > >      features at  /sys/module/overlay/parameters/, and we were already
>  > >      asking the question during patch review:
>  > >         /* Is there a reason anyone would want not to share whiteouts? */
>  > >         ofs->share_whiteout = true;
>  > >      and we left the answer to "later" time.
>  > >
>  > > So a simple solution would be to add the module parameter (without adding
>  > > a mount option), because:
>  > > - It doesn't hurt (?)
>  > > - Somebody may end up using it, for some reason we did not think of
>  > > - We can use it in test to require the feature
>  >
>  > Yeah, I think that works. And I see that ext4 and btrfs both have a
>  > /sys/fs/<fs>/features directory and list supported features there, is
>  > this something overlay could do? Or is this basically the same thing as
>  > what you proposed?
>  >
>
> IMO, for those features which don't need to change module param, maybe feature list
> is more suitable.
>

I suppose it is more suitable, but since at the moment there is only one(?)
such feature and there is an open question whether it should or should not
be configurable, I myself would have taken the easy path, but Miklos
often has a different perspective on these sort of things...

Thanks,
Amir.

Re: [PATCH v4] overlay: test for whiteout inode sharing

[Miklos Szeredi]

On Wed, May 13, 2020 at 5:38 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, May 13, 2020 at 6:17 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
> >
> >  ---- 在 星期三, 2020-05-13 09:10:19 Eryu Guan <eguan@linux.alibaba.com> 撰写 ----
> >  > On Tue, May 12, 2020 at 07:56:35PM +0300, Amir Goldstein wrote:
> >  > > > >  > I see no feature detection logic, so test just fails on old kernels
> >  > > > >  > without this feature? I tried with v5.7-r4 kernel, test fails because
> >  > > > >  > each whiteout file has only one hardlink.
> >  > > > >
> >  > > > > That's true.
> >  > > >
> >  > > > I'd like to see it _notrun on old kernels where the feature is not
> >  > > > available. But that seems hard to do.. Do you have any better ideas?
> >  > > >
> >  > >
> >  > > I've got a few.
> >  > > 1. LTP has the concept of require minimum kernel version.
> >  > >     This would mean that functionality will be not be tested if feature
> >  > >     is backported to old kernels.
> >  > > 2. We could add to overlayfs advertising of supported features, like
> >  > >      /sys/fs/ext4/features/, but it already does "advertise" the configurable
> >  > >      features at  /sys/module/overlay/parameters/, and we were already
> >  > >      asking the question during patch review:
> >  > >         /* Is there a reason anyone would want not to share whiteouts? */
> >  > >         ofs->share_whiteout = true;
> >  > >      and we left the answer to "later" time.
> >  > >
> >  > > So a simple solution would be to add the module parameter (without adding
> >  > > a mount option), because:
> >  > > - It doesn't hurt (?)
> >  > > - Somebody may end up using it, for some reason we did not think of
> >  > > - We can use it in test to require the feature
> >  >
> >  > Yeah, I think that works. And I see that ext4 and btrfs both have a
> >  > /sys/fs/<fs>/features directory and list supported features there, is
> >  > this something overlay could do? Or is this basically the same thing as
> >  > what you proposed?
> >  >
> >
> > IMO, for those features which don't need to change module param, maybe feature list
> > is more suitable.
> >
>
> I suppose it is more suitable, but since at the moment there is only one(?)
> such feature and there is an open question whether it should or should not
> be configurable, I myself would have taken the easy path, but Miklos
> often has a different perspective on these sort of things...

What exactly are we testing?

Hard linked whiteouts are an optimization, not something to be relied
on in any case.  The test should succeed even if overlayfs decides for
some reason not to share the inode.

Thanks,
Miklos

Re: [PATCH v4] overlay: test for whiteout inode sharing

[Amir Goldstein]

> > I suppose it is more suitable, but since at the moment there is only one(?)
> > such feature and there is an open question whether it should or should not
> > be configurable, I myself would have taken the easy path, but Miklos
> > often has a different perspective on these sort of things...
>
> What exactly are we testing?
>

What are testing against silent regressions.
If some change breaks whiteout share we won't know about it
without a test.

> Hard linked whiteouts are an optimization, not something to be relied
> on in any case.  The test should succeed even if overlayfs decides for
> some reason not to share the inode.
>

The best practice would be to ask overlay if feature is supported
in the kernel AND make sure that overlay mount did not disable it
on a specific instance (because of upper fs capabilities).
That is what  _check_overlay_feature does for "features" that have
both module param AND a mount option.

If overlay says that the feature is expected to be enabled on the
instance, we check correctness of the feature. Otherwise, we
report that "feature" is not supported on this instance.

Thanks,
Amir.