[PATCH 1/2] common: add a helper for setting module param

[PATCH 1/2] common: add a helper for setting module param

[Chengguang Xu]

Add a new helper _set_fs_module_param for setting
module param.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 common/module | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/common/module b/common/module
index 39e4e793..148e8c8f 100644
--- a/common/module
+++ b/common/module
@@ -81,3 +81,12 @@ _get_fs_module_param()
 {
 	cat /sys/module/${FSTYP}/parameters/${1} 2>/dev/null
 }
+ # Set the value of a filesystem module parameter
+ # at /sys/module/$FSTYP/parameters/$PARAM
+ #
+ # Usage example:
+ #   _set_fs_module_param param value
+ _set_fs_module_param()
+{
+	echo ${2} > /sys/module/${FSTYP}/parameters/${1} 2>/dev/null
+}
-- 
2.20.1

[PATCH 2/2] overlay/072: test for sharing inode with whiteout files

[Chengguang Xu]

This is a test for whiteout inode sharing feature.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
Hi Eryu,

Kernel patch of this feature is still in review but I hope to merge
test case first, so that we can check the correctness in a convenient
way. The test case will carefully check new module param and skip the
test if the param does not exist.


 tests/overlay/072     | 148 ++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/072.out |   2 +
 tests/overlay/group   |   1 +
 3 files changed, 151 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..1cff386d
--- /dev/null
+++ b/tests/overlay/072
@@ -0,0 +1,148 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2020 Chengguang Xu <cgxu519@mykernel.net>.
+# All Rights Reserved.
+#
+# FS QA Test 072
+#
+# This is a test for inode sharing with whiteout files.
+#
+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_test
+_require_scratch
+
+param_name="whiteout_link_max"
+check_whiteout_link_max()
+{
+	local param_value=`_get_fs_module_param ${param_name}`
+	if [ -z ${param_value} ]; then
+		_notrun "${FSTYP} module param ${param_name} does not exist"
+	fi
+}
+
+lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
+upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
+merged=$OVL_BASE_SCRATCH_MNT/$OVL_MNT
+
+#Make some files in lowerdir.
+make_lower_files()
+{
+	seq 1 $file_count | while read line; do
+		`touch $lowerdir/test${line} 1>&2 2>/dev/null`
+	done
+}
+
+#Delete all copy-uped files in upperdir.
+make_whiteout_files()
+{
+	rm -f $merged/* 1>&2 2>/dev/null
+}
+
+#Check link count of whiteout files.
+check_whiteout_files()
+{
+	seq 1 $file_count | while read line; do
+		local real_count=`stat -c %h $upperdir/test${line} 2>/dev/null`
+		if [[ $link_count != $real_count ]]; then
+			echo "Expected whiteout link count is $link_count but real count is $real_count"
+		fi
+	done
+}
+
+check_whiteout_link_max
+
+# Case1:
+# Setting whiteout_link_max=0 will not share inode
+# with whiteout files, it means each whiteout file
+# will has it's own inode.
+
+file_count=10
+link_max=0
+link_count=1
+_scratch_mkfs
+_set_fs_module_param $param_name $link_max
+make_lower_files
+_scratch_mount
+make_whiteout_files
+check_whiteout_files
+$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT/$OVL_MNT
+
+# Case2:
+# Setting whiteout_link_max=1 will not share inode
+# with whiteout files, it means each whiteout file
+# will has it's own inode.
+
+file_count=10
+link_max=1
+link_count=1
+_scratch_mkfs
+_set_fs_module_param $param_name $link_max
+make_lower_files
+_scratch_mount
+make_whiteout_files
+check_whiteout_files $link_count
+$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT/$OVL_MNT
+
+# Case3:
+# Setting whiteout_link_max=2 will not share inode
+# with whiteout files, it means each whiteout file
+# will has it's own inode. However, the inode will
+# be shared with tmpfile(in workdir) which is used
+# for creating whiteout file.
+
+file_count=10
+link_max=2
+link_count=2
+_scratch_mkfs
+_set_fs_module_param $param_name $link_max
+make_lower_files
+_scratch_mount
+make_whiteout_files
+check_whiteout_files
+$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT/$OVL_MNT
+
+# Case4:
+# Setting whiteout_link_max=10 will share inode
+# with 9 whiteout files and meanwhile the inode
+# will also share with tmpfile(in workdir) which
+# is used for creating whiteout file.
+
+file_count=18
+link_max=10
+link_count=10
+_scratch_mkfs
+_set_fs_module_param $param_name $link_max
+make_lower_files
+_scratch_mount
+make_whiteout_files
+check_whiteout_files
+$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT/$OVL_MNT
+
+# 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 2/2] overlay/072: test for sharing inode with whiteout files

[Amir Goldstein]

On Fri, Apr 10, 2020 at 4:21 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> This is a test for whiteout inode sharing feature.
>
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> ---
> Hi Eryu,
>
> Kernel patch of this feature is still in review but I hope to merge
> test case first, so that we can check the correctness in a convenient
> way. The test case will carefully check new module param and skip the
> test if the param does not exist.
>
>
>  tests/overlay/072     | 148 ++++++++++++++++++++++++++++++++++++++++++
>  tests/overlay/072.out |   2 +
>  tests/overlay/group   |   1 +
>  3 files changed, 151 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..1cff386d
> --- /dev/null
> +++ b/tests/overlay/072
> @@ -0,0 +1,148 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2020 Chengguang Xu <cgxu519@mykernel.net>.
> +# All Rights Reserved.
> +#
> +# FS QA Test 072
> +#
> +# This is a test for inode sharing with whiteout files.
> +#
> +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_test
> +_require_scratch
> +
> +param_name="whiteout_link_max"
> +check_whiteout_link_max()
> +{
> +       local param_value=`_get_fs_module_param ${param_name}`

Keep this value and set it back on _cleanup()

> +       if [ -z ${param_value} ]; then
> +               _notrun "${FSTYP} module param ${param_name} does not exist"

This message will be more helpful:
"overlayfs does not support whiteout inode sharing"

> +       fi
> +}
> +
> +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
> +upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
> +merged=$OVL_BASE_SCRATCH_MNT/$OVL_MNT

Best if you use $SCRATCH_MNT instead of $merged

> +
> +#Make some files in lowerdir.
> +make_lower_files()
> +{
> +       seq 1 $file_count | while read line; do

I think that a for statement would be more readable here.

> +               `touch $lowerdir/test${line} 1>&2 2>/dev/null`
> +       done
> +}
> +
> +#Delete all copy-uped files in upperdir.
> +make_whiteout_files()
> +{
> +       rm -f $merged/* 1>&2 2>/dev/null
> +}
> +
> +#Check link count of whiteout files.
> +check_whiteout_files()
> +{
> +       seq 1 $file_count | while read line; do
> +               local real_count=`stat -c %h $upperdir/test${line} 2>/dev/null`
> +               if [[ $link_count != $real_count ]]; then
> +                       echo "Expected whiteout link count is $link_count but real count is $real_count"
> +               fi
> +       done
> +}
> +
> +check_whiteout_link_max
> +
> +# Case1:
> +# Setting whiteout_link_max=0 will not share inode
> +# with whiteout files, it means each whiteout file
> +# will has it's own inode.
> +
> +file_count=10
> +link_max=0
> +link_count=1

Would be nicer to put all the below in a run_test_case() function
with above as arguments.

> +_scratch_mkfs
> +_set_fs_module_param $param_name $link_max
> +make_lower_files
> +_scratch_mount
> +make_whiteout_files
> +check_whiteout_files
> +$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT/$OVL_MNT

Better:
$UMOUNT_PROG $SCRATCH_MNT

Even better:
_scratch_umount

The difference is that if the test case has reference leaks on inode or
dentry, _overlay_base_unmount() will detect them.

> +
> +# Case2:
> +# Setting whiteout_link_max=1 will not share inode
> +# with whiteout files, it means each whiteout file
> +# will has it's own inode.
> +
> +file_count=10
> +link_max=1
> +link_count=1
> +_scratch_mkfs
> +_set_fs_module_param $param_name $link_max
> +make_lower_files
> +_scratch_mount
> +make_whiteout_files
> +check_whiteout_files $link_count
> +$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT/$OVL_MNT
> +
> +# Case3:
> +# Setting whiteout_link_max=2 will not share inode
> +# with whiteout files, it means each whiteout file
> +# will has it's own inode. However, the inode will
> +# be shared with tmpfile(in workdir) which is used
> +# for creating whiteout file.

First, that is  strange outcome of whiteout_link_max=2
I would not expect it.
Second, how can every whiteout be shared with tmpfile?
There should be at most one tmpfile at all times, so the
whiteouts that already reached whiteout_link_max should
not be linked to any tmpfile.

Please add to test_case() verification that work dir contains
at most one tmpfile.
But please make sure that the test is clever enough to check
both work and index dirs for tmpfiles (names beginning with #).

> +
> +file_count=10
> +link_max=2
> +link_count=2
> +_scratch_mkfs
> +_set_fs_module_param $param_name $link_max
> +make_lower_files
> +_scratch_mount
> +make_whiteout_files
> +check_whiteout_files
> +$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT/$OVL_MNT
> +
> +# Case4:
> +# Setting whiteout_link_max=10 will share inode
> +# with 9 whiteout files and meanwhile the inode
> +# will also share with tmpfile(in workdir) which
> +# is used for creating whiteout file.
> +

Same here. nlink 10 is what I would expect, not 9.

Thanks,
Amir.

Re: [PATCH 1/2] common: add a helper for setting module param

[Eryu Guan]

On Fri, Apr 10, 2020 at 09:20:58AM +0800, Chengguang Xu wrote:
> Add a new helper _set_fs_module_param for setting
> module param.
> 
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>

I think this could go with the test, so a single patch introduces both
test case and the needed helper functions, and usually that's easier to
review, as we could know how the helper be used from the context.

Thanks,
Eryu

> ---
>  common/module | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/common/module b/common/module
> index 39e4e793..148e8c8f 100644
> --- a/common/module
> +++ b/common/module
> @@ -81,3 +81,12 @@ _get_fs_module_param()
>  {
>  	cat /sys/module/${FSTYP}/parameters/${1} 2>/dev/null
>  }
> + # Set the value of a filesystem module parameter
> + # at /sys/module/$FSTYP/parameters/$PARAM
> + #
> + # Usage example:
> + #   _set_fs_module_param param value
> + _set_fs_module_param()
> +{
> +	echo ${2} > /sys/module/${FSTYP}/parameters/${1} 2>/dev/null
> +}
> -- 
> 2.20.1
> 
> 

Re: [PATCH 2/2] overlay/072: test for sharing inode with whiteout files

[Eryu Guan]

On Fri, Apr 10, 2020 at 09:20:59AM +0800, Chengguang Xu wrote:
> This is a test for whiteout inode sharing feature.
> 
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> ---
> Hi Eryu,
> 
> Kernel patch of this feature is still in review but I hope to merge

If this case tests a new & unmerged feature, I'd wait for the kernel
patch land in first, or at least the maintainer of the subsystem of the
kernel acks that the feature will be in kernel, just that the patch
itself needs some improvements at the moment.

As there were cases that I merged a case that aimed to test a new
feature or a new behavior, but the kernel patch was dropped eventually,
and the case became broken.

> test case first, so that we can check the correctness in a convenient
> way. The test case will carefully check new module param and skip the
> test if the param does not exist.

Or you could provide a personal repo that contains the case, so kernel
maintainers & reviewers could verify the feature with that repo?

Thanks,
Eryu

> 
> 
>  tests/overlay/072     | 148 ++++++++++++++++++++++++++++++++++++++++++
>  tests/overlay/072.out |   2 +
>  tests/overlay/group   |   1 +
>  3 files changed, 151 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..1cff386d
> --- /dev/null
> +++ b/tests/overlay/072
> @@ -0,0 +1,148 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2020 Chengguang Xu <cgxu519@mykernel.net>.
> +# All Rights Reserved.
> +#
> +# FS QA Test 072
> +#
> +# This is a test for inode sharing with whiteout files.
> +#
> +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_test
> +_require_scratch
> +
> +param_name="whiteout_link_max"
> +check_whiteout_link_max()
> +{
> +	local param_value=`_get_fs_module_param ${param_name}`
> +	if [ -z ${param_value} ]; then
> +		_notrun "${FSTYP} module param ${param_name} does not exist"
> +	fi
> +}
> +
> +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
> +upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
> +merged=$OVL_BASE_SCRATCH_MNT/$OVL_MNT
> +
> +#Make some files in lowerdir.
> +make_lower_files()
> +{
> +	seq 1 $file_count | while read line; do
> +		`touch $lowerdir/test${line} 1>&2 2>/dev/null`
> +	done
> +}
> +
> +#Delete all copy-uped files in upperdir.
> +make_whiteout_files()
> +{
> +	rm -f $merged/* 1>&2 2>/dev/null
> +}
> +
> +#Check link count of whiteout files.
> +check_whiteout_files()
> +{
> +	seq 1 $file_count | while read line; do
> +		local real_count=`stat -c %h $upperdir/test${line} 2>/dev/null`
> +		if [[ $link_count != $real_count ]]; then
> +			echo "Expected whiteout link count is $link_count but real count is $real_count"
> +		fi
> +	done
> +}
> +
> +check_whiteout_link_max
> +
> +# Case1:
> +# Setting whiteout_link_max=0 will not share inode
> +# with whiteout files, it means each whiteout file
> +# will has it's own inode.
> +
> +file_count=10
> +link_max=0
> +link_count=1
> +_scratch_mkfs
> +_set_fs_module_param $param_name $link_max
> +make_lower_files
> +_scratch_mount
> +make_whiteout_files
> +check_whiteout_files
> +$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT/$OVL_MNT
> +
> +# Case2:
> +# Setting whiteout_link_max=1 will not share inode
> +# with whiteout files, it means each whiteout file
> +# will has it's own inode.
> +
> +file_count=10
> +link_max=1
> +link_count=1
> +_scratch_mkfs
> +_set_fs_module_param $param_name $link_max
> +make_lower_files
> +_scratch_mount
> +make_whiteout_files
> +check_whiteout_files $link_count
> +$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT/$OVL_MNT
> +
> +# Case3:
> +# Setting whiteout_link_max=2 will not share inode
> +# with whiteout files, it means each whiteout file
> +# will has it's own inode. However, the inode will
> +# be shared with tmpfile(in workdir) which is used
> +# for creating whiteout file.
> +
> +file_count=10
> +link_max=2
> +link_count=2
> +_scratch_mkfs
> +_set_fs_module_param $param_name $link_max
> +make_lower_files
> +_scratch_mount
> +make_whiteout_files
> +check_whiteout_files
> +$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT/$OVL_MNT
> +
> +# Case4:
> +# Setting whiteout_link_max=10 will share inode
> +# with 9 whiteout files and meanwhile the inode
> +# will also share with tmpfile(in workdir) which
> +# is used for creating whiteout file.
> +
> +file_count=18
> +link_max=10
> +link_count=10
> +_scratch_mkfs
> +_set_fs_module_param $param_name $link_max
> +make_lower_files
> +_scratch_mount
> +make_whiteout_files
> +check_whiteout_files
> +$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT/$OVL_MNT
> +
> +# 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 2/2] overlay/072: test for sharing inode with whiteout files

[Amir Goldstein]

On Sun, Apr 12, 2020 at 2:26 PM Eryu Guan <guan@eryu.me> wrote:
>
> On Fri, Apr 10, 2020 at 09:20:59AM +0800, Chengguang Xu wrote:
> > This is a test for whiteout inode sharing feature.
> >
> > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> > ---
> > Hi Eryu,
> >
> > Kernel patch of this feature is still in review but I hope to merge
>
> If this case tests a new & unmerged feature, I'd wait for the kernel
> patch land in first, or at least the maintainer of the subsystem of the
> kernel acks that the feature will be in kernel, just that the patch
> itself needs some improvements at the moment.
>
> As there were cases that I merged a case that aimed to test a new
> feature or a new behavior, but the kernel patch was dropped eventually,
> and the case became broken.
>
> > test case first, so that we can check the correctness in a convenient
> > way. The test case will carefully check new module param and skip the
> > test if the param does not exist.
>
> Or you could provide a personal repo that contains the case, so kernel
> maintainers & reviewers could verify the feature with that repo?
>

FWIW, I am glad the test was posted early for review as a proof of
what was tested, but no reason to merge it before the kernel patch.
A personal repo link and/or the test in a single patch with the helper
should be good enough for reviewers.

Thanks,
Amir.

Re: [PATCH 1/2] common: add a helper for setting module param

[Chengguang Xu]

 ---- 在 星期日, 2020-04-12 19:20:34 Eryu Guan <guan@eryu.me> 撰写 ----
 > On Fri, Apr 10, 2020 at 09:20:58AM +0800, Chengguang Xu wrote:
 > > Add a new helper _set_fs_module_param for setting
 > > module param.
 > > 
 > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > 
 > I think this could go with the test, so a single patch introduces both
 > test case and the needed helper functions, and usually that's easier to
 > review, as we could know how the helper be used from the context.
 > 

Hi Eryu,

OK, I'll put it into test patch in V2.

Thanks,
cgxu

Re: [PATCH 2/2] overlay/072: test for sharing inode with whiteout files

[Chengguang Xu]

 ---- 在 星期日, 2020-04-12 19:34:41 Amir Goldstein <amir73il@gmail.com> 撰写 ----
 > On Sun, Apr 12, 2020 at 2:26 PM Eryu Guan <guan@eryu.me> wrote:
 > >
 > > On Fri, Apr 10, 2020 at 09:20:59AM +0800, Chengguang Xu wrote:
 > > > This is a test for whiteout inode sharing feature.
 > > >
 > > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > > > ---
 > > > Hi Eryu,
 > > >
 > > > Kernel patch of this feature is still in review but I hope to merge
 > >
 > > If this case tests a new & unmerged feature, I'd wait for the kernel
 > > patch land in first, or at least the maintainer of the subsystem of the
 > > kernel acks that the feature will be in kernel, just that the patch
 > > itself needs some improvements at the moment.
 > >
 > > As there were cases that I merged a case that aimed to test a new
 > > feature or a new behavior, but the kernel patch was dropped eventually,
 > > and the case became broken.
 > >
 > > > test case first, so that we can check the correctness in a convenient
 > > > way. The test case will carefully check new module param and skip the
 > > > test if the param does not exist.
 > >
 > > Or you could provide a personal repo that contains the case, so kernel
 > > maintainers & reviewers could verify the feature with that repo?
 > >
 > 
 > FWIW, I am glad the test was posted early for review as a proof of
 > what was tested, but no reason to merge it before the kernel patch.
 > A personal repo link and/or the test in a single patch with the helper
 > should be good enough for reviewers.
 > 

Hi Eryu, Amir

I got it, I will post V2 with all fixes in a single patch.


Thanks,
cgxu

Re: [PATCH 2/2] overlay/072: test for sharing inode with whiteout files

[Chengguang Xu]

 ---- 在 星期五, 2020-04-10 15:21:51 Amir Goldstein <amir73il@gmail.com> 撰写 ----
 > On Fri, Apr 10, 2020 at 4:21 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
 > >
 > > This is a test for whiteout inode sharing feature.
 > >
 > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > > ---
 > > Hi Eryu,
 > >
 > > Kernel patch of this feature is still in review but I hope to merge
 > > test case first, so that we can check the correctness in a convenient
 > > way. The test case will carefully check new module param and skip the
 > > test if the param does not exist.
 > >
 > >
 > >  tests/overlay/072     | 148 ++++++++++++++++++++++++++++++++++++++++++
 > >  tests/overlay/072.out |   2 +
 > >  tests/overlay/group   |   1 +
 > >  3 files changed, 151 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..1cff386d
 > > --- /dev/null
 > > +++ b/tests/overlay/072
 > > @@ -0,0 +1,148 @@
 > > +#! /bin/bash
 > > +# SPDX-License-Identifier: GPL-2.0
 > > +# Copyright (c) 2020 Chengguang Xu <cgxu519@mykernel.net>.
 > > +# All Rights Reserved.
 > > +#
 > > +# FS QA Test 072
 > > +#
 > > +# This is a test for inode sharing with whiteout files.
 > > +#
 > > +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_test
 > > +_require_scratch
 > > +
 > > +param_name="whiteout_link_max"
 > > +check_whiteout_link_max()
 > > +{
 > > +       local param_value=`_get_fs_module_param ${param_name}`
 > 
 > Keep this value and set it back on _cleanup()
 > 
 > > +       if [ -z ${param_value} ]; then
 > > +               _notrun "${FSTYP} module param ${param_name} does not exist"
 > 
 > This message will be more helpful:
 > "overlayfs does not support whiteout inode sharing"
 > 
 > > +       fi
 > > +}
 > > +
 > > +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
 > > +upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
 > > +merged=$OVL_BASE_SCRATCH_MNT/$OVL_MNT
 > 
 > Best if you use $SCRATCH_MNT instead of $merged
 > 
 > > +
 > > +#Make some files in lowerdir.
 > > +make_lower_files()
 > > +{
 > > +       seq 1 $file_count | while read line; do
 > 
 > I think that a for statement would be more readable here.
 > 
 > > +               `touch $lowerdir/test${line} 1>&2 2>/dev/null`
 > > +       done
 > > +}
 > > +
 > > +#Delete all copy-uped files in upperdir.
 > > +make_whiteout_files()
 > > +{
 > > +       rm -f $merged/* 1>&2 2>/dev/null
 > > +}
 > > +
 > > +#Check link count of whiteout files.
 > > +check_whiteout_files()
 > > +{
 > > +       seq 1 $file_count | while read line; do
 > > +               local real_count=`stat -c %h $upperdir/test${line} 2>/dev/null`
 > > +               if [[ $link_count != $real_count ]]; then
 > > +                       echo "Expected whiteout link count is $link_count but real count is $real_count"
 > > +               fi
 > > +       done
 > > +}
 > > +
 > > +check_whiteout_link_max
 > > +
 > > +# Case1:
 > > +# Setting whiteout_link_max=0 will not share inode
 > > +# with whiteout files, it means each whiteout file
 > > +# will has it's own inode.
 > > +
 > > +file_count=10
 > > +link_max=0
 > > +link_count=1
 > 
 > Would be nicer to put all the below in a run_test_case() function
 > with above as arguments.
 > 

Something like below?
---------
# Arguments:
# $1: Maximum link count
# $2: Testing file number
# $3: Expected link count
run_test_case()
{
        _scratch_mkfs
        _set_fs_module_param $param_name ${1}
        make_lower_files ${2}
        _scratch_mount
        make_whiteout_files
        check_whiteout_files ${2} ${3}
        $UMOUNT_PROG $SCRATCH_MNT
}

link_max=1
file_count=10
link_count=1
run_test_case $link_max $file_count $link_count
---------

 > > +_scratch_mkfs
 > > +_set_fs_module_param $param_name $link_max
 > > +make_lower_files
 > > +_scratch_mount
 > > +make_whiteout_files
 > > +check_whiteout_files
 > > +$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT/$OVL_MNT
 > 
 > Better:
 > $UMOUNT_PROG $SCRATCH_MNT
 > 
 > Even better:
 > _scratch_umount

I haven't found the definition of _scratch_umount,
have we implemented it?


 > 
 > The difference is that if the test case has reference leaks on inode or
 > dentry, _overlay_base_unmount() will detect them.
 > 
 > > +
 > > +# Case2:
 > > +# Setting whiteout_link_max=1 will not share inode
 > > +# with whiteout files, it means each whiteout file
 > > +# will has it's own inode.
 > > +
 > > +file_count=10
 > > +link_max=1
 > > +link_count=1
 > > +_scratch_mkfs
 > > +_set_fs_module_param $param_name $link_max
 > > +make_lower_files
 > > +_scratch_mount
 > > +make_whiteout_files
 > > +check_whiteout_files $link_count
 > > +$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT/$OVL_MNT
 > > +
 > > +# Case3:
 > > +# Setting whiteout_link_max=2 will not share inode
 > > +# with whiteout files, it means each whiteout file
 > > +# will has it's own inode. However, the inode will
 > > +# be shared with tmpfile(in workdir) which is used
 > > +# for creating whiteout file.
 > 
 > First, that is  strange outcome of whiteout_link_max=2
 > I would not expect it.
 > Second, how can every whiteout be shared with tmpfile?
 > There should be at most one tmpfile at all times, so the
 > whiteouts that already reached whiteout_link_max should
 > not be linked to any tmpfile.

I think I misunderstood your comment in my kernel patch, so I changed
the logic to keep all tmpfiles in workdir and cleanup them during next mount.
I'll fix it in V3 kernel patch.

 > 
 > Please add to test_case() verification that work dir contains
 > at most one tmpfile.
 > But please make sure that the test is clever enough to check
 > both work and index dirs for tmpfiles (names beginning with #).
 > 

So should I check module params of index and nfs_export for checking
tmpfile in index dir?

Thanks,
cgxu

Re: [PATCH 2/2] overlay/072: test for sharing inode with whiteout files

[Amir Goldstein]

>  > > +
>  > > +# Case1:
>  > > +# Setting whiteout_link_max=0 will not share inode
>  > > +# with whiteout files, it means each whiteout file
>  > > +# will has it's own inode.
>  > > +
>  > > +file_count=10
>  > > +link_max=0
>  > > +link_count=1
>  >
>  > Would be nicer to put all the below in a run_test_case() function
>  > with above as arguments.
>  >
>
> Something like below?
> ---------
> # Arguments:
> # $1: Maximum link count
> # $2: Testing file number
> # $3: Expected link count
> run_test_case()
> {
>         _scratch_mkfs
>         _set_fs_module_param $param_name ${1}
>         make_lower_files ${2}
>         _scratch_mount
>         make_whiteout_files
>         check_whiteout_files ${2} ${3}
>         $UMOUNT_PROG $SCRATCH_MNT
> }
>
> link_max=1
> file_count=10
> link_count=1
> run_test_case $link_max $file_count $link_count
> ---------
>

Yes. That's better.

>  > > +_scratch_mkfs
>  > > +_set_fs_module_param $param_name $link_max
>  > > +make_lower_files
>  > > +_scratch_mount
>  > > +make_whiteout_files
>  > > +check_whiteout_files
>  > > +$UMOUNT_PROG $OVL_BASE_SCRATCH_MNT/$OVL_MNT
>  >
>  > Better:
>  > $UMOUNT_PROG $SCRATCH_MNT
>  >
>  > Even better:
>  > _scratch_umount
>
> I haven't found the definition of _scratch_umount,
> have we implemented it?

typo. I meant _scratch_unmount

On top of $UMOUNT_PROG $SCRATCH_MNT
_scratch_unmount also unmount the base fs.

...

>  > First, that is  strange outcome of whiteout_link_max=2
>  > I would not expect it.
>  > Second, how can every whiteout be shared with tmpfile?
>  > There should be at most one tmpfile at all times, so the
>  > whiteouts that already reached whiteout_link_max should
>  > not be linked to any tmpfile.
>
> I think I misunderstood your comment in my kernel patch, so I changed
> the logic to keep all tmpfiles in workdir and cleanup them during next mount.
> I'll fix it in V3 kernel patch.
>

Ah, so its good you posted the test ;-)

>  >
>  > Please add to test_case() verification that work dir contains
>  > at most one tmpfile.
>  > But please make sure that the test is clever enough to check
>  > both work and index dirs for tmpfiles (names beginning with #).
>  >
>
> So should I check module params of index and nfs_export for checking
> tmpfile in index dir?
>

No need for that.

Verifying that both index and work dir contain no more than a single tmpfile
together is good enough and should be pretty simple too:

ls $workdir/work/#* $workdir/index/#*|wc -l

Thanks,
Amir.