[PATCH v2 0/7] overlay hardlink tests

[PATCH v2 0/7] overlay hardlink tests

[Amir Goldstein]

Eryu,

Sorry, v1 had an unintentional squashed patch, so re-posting v2.

overlay/018 was written to track the non-standard behavior of
overlayfs, where lower hardlinks are broken on copy up.

While working on fixing the hardlinks breakage issue, I found
several false positives and uncovered cases in the test.

This series addresses the shortcomings of the original overlay/018
test and introduces three more tests to overlay/hardlink group.

Thanks,
Amir.

Amir Goldstein (7):
  overlay/018: re-factor and add to hardlink group
  overlay/018: print hardlink content to golden output
  overlay/018: test broken hardlinks after mount cycle
  overlay/018: test lower hardlinks re-unite on copy up
  overlay: test concurrent copy up of lower hardlinks
  overlay: test nlink accounting of overlay hardlinks
  overlay: test dropping nlink below zero

 tests/overlay/018     |  60 ++++++++++++++++----
 tests/overlay/018.out |  21 ++++++-
 tests/overlay/032     | 102 ++++++++++++++++++++++++++++++++++
 tests/overlay/032.out |   4 ++
 tests/overlay/033     | 149 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/033.out |  69 +++++++++++++++++++++++
 tests/overlay/034     |  91 ++++++++++++++++++++++++++++++
 tests/overlay/034.out |   2 +
 tests/overlay/group   |   5 +-
 9 files changed, 489 insertions(+), 14 deletions(-)
 create mode 100755 tests/overlay/032
 create mode 100644 tests/overlay/032.out
 create mode 100755 tests/overlay/033
 create mode 100644 tests/overlay/033.out
 create mode 100755 tests/overlay/034
 create mode 100644 tests/overlay/034.out

-- 
2.7.4

[PATCH v2 1/7] overlay/018: re-factor and add to hardlink group

[Amir Goldstein]

Use helpers to records and check inode numbers so we can repeat
the same test after each hardlink copy up and mount cycle.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/018   | 33 +++++++++++++++++++++++++--------
 tests/overlay/group |  2 +-
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/tests/overlay/018 b/tests/overlay/018
index 7e47732..527b9d1 100755
--- a/tests/overlay/018
+++ b/tests/overlay/018
@@ -61,25 +61,42 @@ echo "patient zero" >> $lowerdir/foo
 ln $lowerdir/foo $lowerdir/bar
 
 
+# Record inode numbers in format <ino> <nlink>
+function record_ino_nlink()
+{
+	ls -li $FILES | awk '{ print $1, $3 }' > $1
+}
+
+# Check inode numbers match recorded inode numbers
+function check_ino_nlink()
+{
+	before=$1
+	after=$2
+
+	record_ino_nlink $after
+
+	# Test constant stat(2) st_ino/st_nlink -
+	#   Compare before..after - expect silence
+	# We use diff -u so out.bad will tell us which stage failed
+	diff -u $before $after
+}
+
 _scratch_mount
 
 
-rm -f $tmp.before $tmp.after
+rm -f $tmp.*
 
 foo=$SCRATCH_MNT/foo
 bar=$SCRATCH_MNT/bar
 
-# Record inode number and nlink before copy up
-ls -li $foo $bar | awk '{ print $1, $3 }' > $tmp.before
+FILES="$foo $bar"
+
+record_ino_nlink $tmp.before
 
 # Modify content of one of the hardlinks
 echo "mutated" >> $foo
 
-# Record inode number and nlink after copy up
-ls -li $foo $bar | awk '{ print $1, $3 }' > $tmp.after
-
-# Compare ino/nlink before..after - expect silence
-diff $tmp.before $tmp.after
+check_ino_nlink $tmp.before $tmp.after
 
 # Compare content of files - expect silence
 diff $foo $bar
diff --git a/tests/overlay/group b/tests/overlay/group
index 64d200c..28df5b6 100644
--- a/tests/overlay/group
+++ b/tests/overlay/group
@@ -20,7 +20,7 @@
 015 auto quick whiteout
 016 auto quick copyup
 017 auto quick copyup
-018 auto quick copyup
+018 auto quick copyup hardlink
 019 auto stress
 020 auto quick copyup perms
 021 auto quick copyup
-- 
2.7.4

[PATCH v2 2/7] overlay/018: print hardlink content to golden output

[Amir Goldstein]

diff may skips comparing content of files with identical st_ino/st_dev.
Overlayfs stat(2) may return same st_dev/st_ino for hardlink copy ups,
but it does not mean that read(2) will return the same content.

Convert the test to output hardlink files content to golden output
instead of using diff.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/018     | 12 ++++++------
 tests/overlay/018.out |  9 ++++++++-
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/tests/overlay/018 b/tests/overlay/018
index 527b9d1..7570a16 100755
--- a/tests/overlay/018
+++ b/tests/overlay/018
@@ -57,7 +57,7 @@ _scratch_mkfs >>$seqres.full 2>&1
 # Create 2 hardlinked files in lower
 lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
 mkdir -p $lowerdir
-echo "patient zero" >> $lowerdir/foo
+echo "zero" >> $lowerdir/foo
 ln $lowerdir/foo $lowerdir/bar
 
 
@@ -91,16 +91,16 @@ bar=$SCRATCH_MNT/bar
 
 FILES="$foo $bar"
 
+echo "== Before copy up =="
+cat $FILES
 record_ino_nlink $tmp.before
 
 # Modify content of one of the hardlinks
-echo "mutated" >> $foo
+echo "one" >> $foo
 
+echo "== After write one =="
+cat $FILES
 check_ino_nlink $tmp.before $tmp.after
 
-# Compare content of files - expect silence
-diff $foo $bar
-
-echo "Silence is golden"
 status=0
 exit
diff --git a/tests/overlay/018.out b/tests/overlay/018.out
index 8849e30..784e8bc 100644
--- a/tests/overlay/018.out
+++ b/tests/overlay/018.out
@@ -1,2 +1,9 @@
 QA output created by 018
-Silence is golden
+== Before copy up ==
+zero
+zero
+== After write one ==
+zero
+one
+zero
+one
-- 
2.7.4

[PATCH v2 3/7] overlay/018: test broken hardlinks after mount cycle

[Amir Goldstein]

This test checks if overlayfs hardlinks are preserved across
copy up.  Check if they are preserved also after copy up and
mount cycle.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/018     | 13 +++++++++++--
 tests/overlay/018.out |  5 +++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/tests/overlay/018 b/tests/overlay/018
index 7570a16..46097a9 100755
--- a/tests/overlay/018
+++ b/tests/overlay/018
@@ -96,11 +96,20 @@ cat $FILES
 record_ino_nlink $tmp.before
 
 # Modify content of one of the hardlinks
-echo "one" >> $foo
+# Intentionally modify the last hardlink in $FILES, so after mount cycle
+# when reading the first file in $FILES, last file won't be in inode/dcache
+echo "one" >> $bar
 
 echo "== After write one =="
 cat $FILES
-check_ino_nlink $tmp.before $tmp.after
+check_ino_nlink $tmp.before $tmp.after_one
+
+# Verify that the hardlinks survive a mount cycle
+_scratch_cycle_mount
+
+echo "== After mount cycle =="
+cat $FILES
+check_ino_nlink $tmp.after_one $tmp.after_cycle
 
 status=0
 exit
diff --git a/tests/overlay/018.out b/tests/overlay/018.out
index 784e8bc..5b74ee1 100644
--- a/tests/overlay/018.out
+++ b/tests/overlay/018.out
@@ -7,3 +7,8 @@ zero
 one
 zero
 one
+== After mount cycle ==
+zero
+one
+zero
+one
-- 
2.7.4

[PATCH v2 4/7] overlay/018: test lower hardlinks re-unite on copy up

[Amir Goldstein]

Test that when two lower hardlinks are copied up, they end up
as two upper hardlinks of the same upper inode.

Drop caches before copy up so there is no knowledge of the
copied up hardlink in inode/dcache.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/018     | 10 ++++++++++
 tests/overlay/018.out |  7 +++++++
 2 files changed, 17 insertions(+)

diff --git a/tests/overlay/018 b/tests/overlay/018
index 46097a9..41855dc 100755
--- a/tests/overlay/018
+++ b/tests/overlay/018
@@ -111,5 +111,15 @@ echo "== After mount cycle =="
 cat $FILES
 check_ino_nlink $tmp.after_one $tmp.after_cycle
 
+# Drop caches to get the copied up hardlink out of cache
+echo 3 > /proc/sys/vm/drop_caches
+
+# Modify content of the other hardlink
+echo "two" >> $foo
+
+echo "== After write two =="
+cat $FILES
+check_ino_nlink $tmp.after_one $tmp.after_two
+
 status=0
 exit
diff --git a/tests/overlay/018.out b/tests/overlay/018.out
index 5b74ee1..adc7f72 100644
--- a/tests/overlay/018.out
+++ b/tests/overlay/018.out
@@ -12,3 +12,10 @@ zero
 one
 zero
 one
+== After write two ==
+zero
+one
+two
+zero
+one
+two
-- 
2.7.4

[PATCH v2 5/7] overlay: test concurrent copy up of lower hardlinks

[Amir Goldstein]

Two tasks make a modification concurrently on two hardlinks of a large
lower inode.  The copy up should be triggered by one of the tasks and the
other should be waiting for copy up to complete.  Both copy up targets
should end up being upper hardlinks and both metadata changes should be
visible in both hardlinks.

With kernel <= v4.12, hardlinks are broken on copy up, meaning that copy up
is performed independetly and the resulting upper copy up targets each have
only one of the the metadata changes visible.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/032     | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/032.out |   4 ++
 tests/overlay/group   |   1 +
 3 files changed, 107 insertions(+)
 create mode 100755 tests/overlay/032
 create mode 100644 tests/overlay/032.out

diff --git a/tests/overlay/032 b/tests/overlay/032
new file mode 100755
index 0000000..9a7995e
--- /dev/null
+++ b/tests/overlay/032
@@ -0,0 +1,102 @@
+#! /bin/bash
+# FS QA Test 032
+#
+# Test concurrent copy up of lower hardlinks.
+#
+# Two tasks make a metadata change concurrently on two hardlinks of a large
+# lower inode.  The copy up should be triggers by one of the tasks and the
+# other should be waiting for copy up to complete.  Both copy up targets
+# should end up being upper hardlinks and both metadata changes should be
+# applied.
+#
+#-----------------------------------------------------------------------
+# Copyright (C) 2017 CTERA Networks. All Rights Reserved.
+# Author: Amir Goldstein <amir73il@gmail.com>
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+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
+
+# Remove all files from previous tests
+_scratch_mkfs
+
+# overlay copy_up doesn't deal with sparse file well, holes will be filled by
+# zeros, so if both hardlinks are broken on copy up, we need (2*500M) free space
+# on $OVL_BASE_SCRATCH_MNT.
+_require_fs_space $OVL_BASE_SCRATCH_MNT $((500*1024*2))
+
+# Create a large file in lower with 2 hardlinks
+lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
+mkdir -p $lowerdir
+touch $lowerdir/zero
+$XFS_IO_PROG -c "truncate 500m" $lowerdir/zero
+ln $lowerdir/zero $lowerdir/one
+ln $lowerdir/zero $lowerdir/two
+
+_scratch_mount
+
+_do_cmd()
+{
+	echo "`date +%T` $1..." >> $seqres.full
+	eval "$1"
+	echo "`date +%T` ...$1" >> $seqres.full
+}
+
+# Perform one modification on each hardlink (size and owner)
+_do_cmd "echo >> $SCRATCH_MNT/one" &
+#
+# When hardlinks are broken and overlayfs supports concurrent copy up,
+# $seqres.full will show that file two copy up started ~2s after file one
+# copy up started and ended ~2s after file one copy up ended.
+# With synchronized copy up of lower inodes, $seqres.full will show that
+# file two copy up ended at the same time as file one copy up.
+#
+sleep 2
+_do_cmd "chown 100 $SCRATCH_MNT/two" &
+
+wait
+
+# Expect all hardlinks to show both metadata modifications
+for f in zero one two; do
+	_ls_l -n $SCRATCH_MNT/$f | awk '{ print $2, $3, $5, $9 }' | _filter_scratch
+done
+
+status=0
+exit
diff --git a/tests/overlay/032.out b/tests/overlay/032.out
new file mode 100644
index 0000000..0069740
--- /dev/null
+++ b/tests/overlay/032.out
@@ -0,0 +1,4 @@
+QA output created by 032
+3 100 524288001 SCRATCH_MNT/zero
+3 100 524288001 SCRATCH_MNT/one
+3 100 524288001 SCRATCH_MNT/two
diff --git a/tests/overlay/group b/tests/overlay/group
index 28df5b6..2baba3a 100644
--- a/tests/overlay/group
+++ b/tests/overlay/group
@@ -34,3 +34,4 @@
 029 auto quick
 030 auto quick perms
 031 auto quick whiteout
+032 auto quick copyup hardlink
-- 
2.7.4

[PATCH v2 6/7] overlay: test nlink accounting of overlay hardlinks

[Amir Goldstein]

nlink of overlay inode should account for the union of lower
and upper hardlinks.

persistent overlay union nlink is stored in an extended attribute
on the upper inode.

In order to test persistent overlay nlink accounting, the test is
repeated with both warm and cold dentry/inode cache.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/033     | 149 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/033.out |  69 +++++++++++++++++++++++
 tests/overlay/group   |   1 +
 3 files changed, 219 insertions(+)
 create mode 100755 tests/overlay/033
 create mode 100644 tests/overlay/033.out

diff --git a/tests/overlay/033 b/tests/overlay/033
new file mode 100755
index 0000000..743edc8
--- /dev/null
+++ b/tests/overlay/033
@@ -0,0 +1,149 @@
+#! /bin/bash
+# FS QA Test 033
+#
+# Test nlink accounting of overlay hardlinks.
+#
+# nlink of overlay inode should account for the union of lower and upper
+# hardlinks.
+#
+#-----------------------------------------------------------------------
+# Copyright (C) 2017 CTERA Networks. All Rights Reserved.
+# Author: Amir Goldstein <amir73il@gmail.com>
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+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
+
+report_nlink()
+{
+	when=$1
+
+	[ $DCACHETEMP != cold ] || echo 2 > /proc/sys/vm/drop_caches
+
+	# check nlink with warm dcache after overlay modification
+	echo "== $when - $DCACHETEMP dcache =="
+	for f in $HARDLINKS; do
+		_ls_l $SCRATCH_MNT/$f | awk '{ print $2, $9 }' | _filter_scratch
+	done
+}
+
+# Create lower hardlinks
+create_hardlinks()
+{
+	lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
+	mkdir -p $lowerdir
+	touch $lowerdir/0
+	ln $lowerdir/0 $lowerdir/1
+	ln $lowerdir/0 $lowerdir/2
+	ln $lowerdir/0 $lowerdir/3
+}
+
+test_hardlinks()
+{
+	HARDLINKS=`seq 0 3`
+	report_nlink "all lower"
+
+	# Unlink lower hardlink
+	rm $SCRATCH_MNT/0
+	HARDLINKS=`seq 1 3`
+	report_nlink "unlink lower"
+
+	# Link to lower hardlink
+	ln $SCRATCH_MNT/3 $SCRATCH_MNT/4
+	HARDLINKS=`seq 1 4`
+	report_nlink "link lower"
+
+	# Link to upper hardlink
+	ln $SCRATCH_MNT/4 $SCRATCH_MNT/5
+	HARDLINKS=`seq 1 5`
+	report_nlink "link upper"
+
+	# Rename over lower hardlink
+	touch $SCRATCH_MNT/new
+	mv $SCRATCH_MNT/new $SCRATCH_MNT/1
+	HARDLINKS=`seq 2 5`
+	report_nlink "cover lower"
+
+	# Unlink upper hardlink
+	rm $SCRATCH_MNT/5
+	HARDLINKS=`seq 2 4`
+	report_nlink "unlink upper"
+
+	# Rename over upper hardlink
+	touch $SCRATCH_MNT/new
+	mv $SCRATCH_MNT/new $SCRATCH_MNT/4
+	HARDLINKS=`seq 2 3`
+	report_nlink "cover upper"
+
+	# Unlink last upper (union still has one lower)
+	rm $SCRATCH_MNT/3
+	HARDLINKS=2
+	report_nlink "unlink last upper"
+
+	# Unlink last lower and drop union nlink to zero (and hopefully not below)
+	rm $SCRATCH_MNT/2
+
+	# Verify that orphan index is cleaned when dropping nlink to zero
+	ls $OVL_BASE_SCRATCH_MNT/$OVL_WORK/index 2>/dev/null
+}
+
+# Remove all files from previous tests
+_scratch_mkfs
+
+# Create lower hardlinks
+create_hardlinks
+
+_scratch_mount
+# Test hardlinks with warm dcache
+DCACHETEMP=warm
+test_hardlinks
+
+# Reset to lower hardlinks
+_scratch_unmount
+_scratch_mkfs
+create_hardlinks
+_scratch_mount
+
+# Test hardlinks with cold dcache
+DCACHETEMP=cold
+test_hardlinks
+
+status=0
+exit
diff --git a/tests/overlay/033.out b/tests/overlay/033.out
new file mode 100644
index 0000000..506ccc1
--- /dev/null
+++ b/tests/overlay/033.out
@@ -0,0 +1,69 @@
+QA output created by 033
+== all lower - warm dcache ==
+4 SCRATCH_MNT/0
+4 SCRATCH_MNT/1
+4 SCRATCH_MNT/2
+4 SCRATCH_MNT/3
+== unlink lower - warm dcache ==
+3 SCRATCH_MNT/1
+3 SCRATCH_MNT/2
+3 SCRATCH_MNT/3
+== link lower - warm dcache ==
+4 SCRATCH_MNT/1
+4 SCRATCH_MNT/2
+4 SCRATCH_MNT/3
+4 SCRATCH_MNT/4
+== link upper - warm dcache ==
+5 SCRATCH_MNT/1
+5 SCRATCH_MNT/2
+5 SCRATCH_MNT/3
+5 SCRATCH_MNT/4
+5 SCRATCH_MNT/5
+== cover lower - warm dcache ==
+4 SCRATCH_MNT/2
+4 SCRATCH_MNT/3
+4 SCRATCH_MNT/4
+4 SCRATCH_MNT/5
+== unlink upper - warm dcache ==
+3 SCRATCH_MNT/2
+3 SCRATCH_MNT/3
+3 SCRATCH_MNT/4
+== cover upper - warm dcache ==
+2 SCRATCH_MNT/2
+2 SCRATCH_MNT/3
+== unlink last upper - warm dcache ==
+1 SCRATCH_MNT/2
+== all lower - cold dcache ==
+4 SCRATCH_MNT/0
+4 SCRATCH_MNT/1
+4 SCRATCH_MNT/2
+4 SCRATCH_MNT/3
+== unlink lower - cold dcache ==
+3 SCRATCH_MNT/1
+3 SCRATCH_MNT/2
+3 SCRATCH_MNT/3
+== link lower - cold dcache ==
+4 SCRATCH_MNT/1
+4 SCRATCH_MNT/2
+4 SCRATCH_MNT/3
+4 SCRATCH_MNT/4
+== link upper - cold dcache ==
+5 SCRATCH_MNT/1
+5 SCRATCH_MNT/2
+5 SCRATCH_MNT/3
+5 SCRATCH_MNT/4
+5 SCRATCH_MNT/5
+== cover lower - cold dcache ==
+4 SCRATCH_MNT/2
+4 SCRATCH_MNT/3
+4 SCRATCH_MNT/4
+4 SCRATCH_MNT/5
+== unlink upper - cold dcache ==
+3 SCRATCH_MNT/2
+3 SCRATCH_MNT/3
+3 SCRATCH_MNT/4
+== cover upper - cold dcache ==
+2 SCRATCH_MNT/2
+2 SCRATCH_MNT/3
+== unlink last upper - cold dcache ==
+1 SCRATCH_MNT/2
diff --git a/tests/overlay/group b/tests/overlay/group
index 2baba3a..35cd5a5 100644
--- a/tests/overlay/group
+++ b/tests/overlay/group
@@ -35,3 +35,4 @@
 030 auto quick perms
 031 auto quick whiteout
 032 auto quick copyup hardlink
+033 auto quick copyup hardlink
-- 
2.7.4

[PATCH v2 7/7] overlay: test dropping nlink below zero

[Amir Goldstein]

nlink of overlay inode could be dropped indefinety by adding
un-accounted lower hardlinks underneath a mounted overlay and
trying to remove them.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/034     | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/034.out |  2 ++
 tests/overlay/group   |  1 +
 3 files changed, 94 insertions(+)
 create mode 100755 tests/overlay/034
 create mode 100644 tests/overlay/034.out

diff --git a/tests/overlay/034 b/tests/overlay/034
new file mode 100755
index 0000000..cb023bf
--- /dev/null
+++ b/tests/overlay/034
@@ -0,0 +1,91 @@
+#! /bin/bash
+# FS QA Test 034
+#
+# Test overlay nlink when adding lower hardlinks.
+#
+# nlink of overlay inode could be dropped indefinety by adding
+# unaccounted lower hardlinks underneath a mounted overlay and
+# trying to remove them.
+#
+#-----------------------------------------------------------------------
+# Copyright (C) 2017 CTERA Networks. All Rights Reserved.
+# Author: Amir Goldstein <amir73il@gmail.com>
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+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
+
+# Remove all files from previous tests
+_scratch_mkfs
+
+# Create lower hardlink
+lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
+mkdir -p $lowerdir
+touch $lowerdir/0
+ln $lowerdir/0 $lowerdir/1
+
+_scratch_mount
+
+# Copy up lower hardlink - nlink should be 2
+touch $SCRATCH_MNT/0
+
+# Add lower hardlinks while overlay is mounted
+ln $lowerdir/0 $lowerdir/2
+ln $lowerdir/0 $lowerdir/3
+
+# Unlink un-accounted lowers to drive nlink to 0
+rm $SCRATCH_MNT/2
+rm $SCRATCH_MNT/3
+
+# Check for getting ENOENT for trying to link !I_LINKABLE with nlink 0
+ln $SCRATCH_MNT/0 $SCRATCH_MNT/4
+
+# Unlink all hardlink to drive nlink below 0
+rm $SCRATCH_MNT/0
+rm $SCRATCH_MNT/1
+rm $SCRATCH_MNT/4
+
+# Verify that orphan index is cleaned on mount
+_scratch_cycle_mount
+ls $OVL_BASE_SCRATCH_MNT/$OVL_WORK/index 2>/dev/null
+
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/overlay/034.out b/tests/overlay/034.out
new file mode 100644
index 0000000..4c8873c
--- /dev/null
+++ b/tests/overlay/034.out
@@ -0,0 +1,2 @@
+QA output created by 034
+Silence is golden
diff --git a/tests/overlay/group b/tests/overlay/group
index 35cd5a5..b55ed0c 100644
--- a/tests/overlay/group
+++ b/tests/overlay/group
@@ -36,3 +36,4 @@
 031 auto quick whiteout
 032 auto quick copyup hardlink
 033 auto quick copyup hardlink
+034 auto quick copyup hardlink
-- 
2.7.4

Re: [PATCH v2 5/7] overlay: test concurrent copy up of lower hardlinks

[Eryu Guan]

On Tue, Jul 04, 2017 at 02:40:32PM +0300, Amir Goldstein wrote:
> Two tasks make a modification concurrently on two hardlinks of a large
> lower inode.  The copy up should be triggered by one of the tasks and the
> other should be waiting for copy up to complete.  Both copy up targets
> should end up being upper hardlinks and both metadata changes should be
> visible in both hardlinks.
> 
> With kernel <= v4.12, hardlinks are broken on copy up, meaning that copy up

So this will be fixed in 4.13-rc1 kernel?

> is performed independetly and the resulting upper copy up targets each have
> only one of the the metadata changes visible.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  tests/overlay/032     | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/overlay/032.out |   4 ++
>  tests/overlay/group   |   1 +
>  3 files changed, 107 insertions(+)
>  create mode 100755 tests/overlay/032
>  create mode 100644 tests/overlay/032.out
> 
> diff --git a/tests/overlay/032 b/tests/overlay/032
> new file mode 100755
> index 0000000..9a7995e
> --- /dev/null
> +++ b/tests/overlay/032
> @@ -0,0 +1,102 @@
> +#! /bin/bash
> +# FS QA Test 032
> +#
> +# Test concurrent copy up of lower hardlinks.
> +#
> +# Two tasks make a metadata change concurrently on two hardlinks of a large
> +# lower inode.  The copy up should be triggers by one of the tasks and the
> +# other should be waiting for copy up to complete.  Both copy up targets
> +# should end up being upper hardlinks and both metadata changes should be
> +# applied.
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (C) 2017 CTERA Networks. All Rights Reserved.
> +# Author: Amir Goldstein <amir73il@gmail.com>
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +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
> +
> +# Remove all files from previous tests
> +_scratch_mkfs
> +
> +# overlay copy_up doesn't deal with sparse file well, holes will be filled by
> +# zeros, so if both hardlinks are broken on copy up, we need (2*500M) free space
> +# on $OVL_BASE_SCRATCH_MNT.
> +_require_fs_space $OVL_BASE_SCRATCH_MNT $((500*1024*2))
> +
> +# Create a large file in lower with 2 hardlinks
> +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
> +mkdir -p $lowerdir
> +touch $lowerdir/zero
> +$XFS_IO_PROG -c "truncate 500m" $lowerdir/zero
> +ln $lowerdir/zero $lowerdir/one
> +ln $lowerdir/zero $lowerdir/two
> +
> +_scratch_mount
> +
> +_do_cmd()

I'd rename it to do_cmd for a local helper function.

> +{
> +	echo "`date +%T` $1..." >> $seqres.full
> +	eval "$1"
> +	echo "`date +%T` ...$1" >> $seqres.full
> +}
> +
> +# Perform one modification on each hardlink (size and owner)
> +_do_cmd "echo >> $SCRATCH_MNT/one" &
> +#
> +# When hardlinks are broken and overlayfs supports concurrent copy up,
> +# $seqres.full will show that file two copy up started ~2s after file one
> +# copy up started and ended ~2s after file one copy up ended.
> +# With synchronized copy up of lower inodes, $seqres.full will show that
> +# file two copy up ended at the same time as file one copy up.
> +#
> +sleep 2

If the first copyup finished within 2 seconds, it's not a concurrent
modification on the two hardlinks. Does that break the test assumption?

> +_do_cmd "chown 100 $SCRATCH_MNT/two" &
> +
> +wait
> +
> +# Expect all hardlinks to show both metadata modifications
> +for f in zero one two; do
> +	_ls_l -n $SCRATCH_MNT/$f | awk '{ print $2, $3, $5, $9 }' | _filter_scratch

Better to comment on what are the 4 fields.

Thanks,
Eryu

> +done
> +
> +status=0
> +exit
> diff --git a/tests/overlay/032.out b/tests/overlay/032.out
> new file mode 100644
> index 0000000..0069740
> --- /dev/null
> +++ b/tests/overlay/032.out
> @@ -0,0 +1,4 @@
> +QA output created by 032
> +3 100 524288001 SCRATCH_MNT/zero
> +3 100 524288001 SCRATCH_MNT/one
> +3 100 524288001 SCRATCH_MNT/two
> diff --git a/tests/overlay/group b/tests/overlay/group
> index 28df5b6..2baba3a 100644
> --- a/tests/overlay/group
> +++ b/tests/overlay/group
> @@ -34,3 +34,4 @@
>  029 auto quick
>  030 auto quick perms
>  031 auto quick whiteout
> +032 auto quick copyup hardlink
> -- 
> 2.7.4
> 

Re: [PATCH v2 7/7] overlay: test dropping nlink below zero

[Eryu Guan]

On Tue, Jul 04, 2017 at 02:40:34PM +0300, Amir Goldstein wrote:
> nlink of overlay inode could be dropped indefinety by adding
> un-accounted lower hardlinks underneath a mounted overlay and
> trying to remove them.

Sorry, I didn't quite follow the hardlink patches, could you please
describe what is "accounted/un-accounted" hardlinks and the expected
behavior in commit log? And what does "indefinety" mean? I couldn't find
it in dictionary.

> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  tests/overlay/034     | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/overlay/034.out |  2 ++
>  tests/overlay/group   |  1 +
>  3 files changed, 94 insertions(+)
>  create mode 100755 tests/overlay/034
>  create mode 100644 tests/overlay/034.out
> 
> diff --git a/tests/overlay/034 b/tests/overlay/034
> new file mode 100755
> index 0000000..cb023bf
> --- /dev/null
> +++ b/tests/overlay/034
> @@ -0,0 +1,91 @@
> +#! /bin/bash
> +# FS QA Test 034
> +#
> +# Test overlay nlink when adding lower hardlinks.
> +#
> +# nlink of overlay inode could be dropped indefinety by adding
> +# unaccounted lower hardlinks underneath a mounted overlay and
> +# trying to remove them.
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (C) 2017 CTERA Networks. All Rights Reserved.
> +# Author: Amir Goldstein <amir73il@gmail.com>
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +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
> +
> +# Remove all files from previous tests
> +_scratch_mkfs
> +
> +# Create lower hardlink
> +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
> +mkdir -p $lowerdir
> +touch $lowerdir/0
> +ln $lowerdir/0 $lowerdir/1
> +
> +_scratch_mount
> +
> +# Copy up lower hardlink - nlink should be 2
> +touch $SCRATCH_MNT/0
> +
> +# Add lower hardlinks while overlay is mounted

Why "while overlay is mounted" is needed?

> +ln $lowerdir/0 $lowerdir/2
> +ln $lowerdir/0 $lowerdir/3
> +
> +# Unlink un-accounted lowers to drive nlink to 0
> +rm $SCRATCH_MNT/2
> +rm $SCRATCH_MNT/3

drive nlink of which file to 0? Sorry, I'm a bit lost on overlay
hardlink behaviors..

> +
> +# Check for getting ENOENT for trying to link !I_LINKABLE with nlink 0
> +ln $SCRATCH_MNT/0 $SCRATCH_MNT/4

Shouldn't we expect an error message from this ln, if "check for getting
ENOEND"? BTW, this test passed on 4.12-rc7 kernel, I'm not sure if
that's expected result.

Thanks,
Eryu

> +
> +# Unlink all hardlink to drive nlink below 0
> +rm $SCRATCH_MNT/0
> +rm $SCRATCH_MNT/1
> +rm $SCRATCH_MNT/4
> +
> +# Verify that orphan index is cleaned on mount
> +_scratch_cycle_mount
> +ls $OVL_BASE_SCRATCH_MNT/$OVL_WORK/index 2>/dev/null
> +
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/overlay/034.out b/tests/overlay/034.out
> new file mode 100644
> index 0000000..4c8873c
> --- /dev/null
> +++ b/tests/overlay/034.out
> @@ -0,0 +1,2 @@
> +QA output created by 034
> +Silence is golden
> diff --git a/tests/overlay/group b/tests/overlay/group
> index 35cd5a5..b55ed0c 100644
> --- a/tests/overlay/group
> +++ b/tests/overlay/group
> @@ -36,3 +36,4 @@
>  031 auto quick whiteout
>  032 auto quick copyup hardlink
>  033 auto quick copyup hardlink
> +034 auto quick copyup hardlink
> -- 
> 2.7.4
> 

Re: [PATCH v2 5/7] overlay: test concurrent copy up of lower hardlinks

[Amir Goldstein]

On Wed, Jul 5, 2017 at 12:59 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Tue, Jul 04, 2017 at 02:40:32PM +0300, Amir Goldstein wrote:
>> Two tasks make a modification concurrently on two hardlinks of a large
>> lower inode.  The copy up should be triggered by one of the tasks and the
>> other should be waiting for copy up to complete.  Both copy up targets
>> should end up being upper hardlinks and both metadata changes should be
>> visible in both hardlinks.
>>
>> With kernel <= v4.12, hardlinks are broken on copy up, meaning that copy up
>
> So this will be fixed in 4.13-rc1 kernel?

It is fixed on current overlayfs-next branch. Yes.


>> +
>> +_do_cmd()
>
> I'd rename it to do_cmd for a local helper function.
>

sure

>> +{
>> +     echo "`date +%T` $1..." >> $seqres.full
>> +     eval "$1"
>> +     echo "`date +%T` ...$1" >> $seqres.full
>> +}
>> +
>> +# Perform one modification on each hardlink (size and owner)
>> +_do_cmd "echo >> $SCRATCH_MNT/one" &
>> +#
>> +# When hardlinks are broken and overlayfs supports concurrent copy up,
>> +# $seqres.full will show that file two copy up started ~2s after file one
>> +# copy up started and ended ~2s after file one copy up ended.
>> +# With synchronized copy up of lower inodes, $seqres.full will show that
>> +# file two copy up ended at the same time as file one copy up.
>> +#
>> +sleep 2
>
> If the first copyup finished within 2 seconds, it's not a concurrent
> modification on the two hardlinks. Does that break the test assumption?

It will not be a race, plain 2 consequent copy ups, so not testing anything
special, but won't cause test to fail.

I am testing overlayfs with 2 underlying fs configurations:
1. xfs with clone support
2. ext4

With xfs clone up takes fraction of a second, the test is not really concurrent.
With ext4, on my laptop with SSD, the 500M copy up takes ~3 seconds,
which is quite close to the 2 seconds delay.
I increased the copy up file size to 1G and now first copy up takes ~6 seconds,
so there is more slack for the 2 sec delay, but still keeps the test 'quick'.

>
>> +_do_cmd "chown 100 $SCRATCH_MNT/two" &
>> +
>> +wait
>> +
>> +# Expect all hardlinks to show both metadata modifications
>> +for f in zero one two; do
>> +     _ls_l -n $SCRATCH_MNT/$f | awk '{ print $2, $3, $5, $9 }' | _filter_scratch
>
> Better to comment on what are the 4 fields.
>

They show the metadata that was changes by chown and write.
Will add a comment.

Thanks,
Amir.

Re: [PATCH v2 7/7] overlay: test dropping nlink below zero

[Amir Goldstein]

On Wed, Jul 5, 2017 at 1:09 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Tue, Jul 04, 2017 at 02:40:34PM +0300, Amir Goldstein wrote:
>> nlink of overlay inode could be dropped indefinety by adding
>> un-accounted lower hardlinks underneath a mounted overlay and
>> trying to remove them.
>
> Sorry, I didn't quite follow the hardlink patches, could you please
> describe what is "accounted/un-accounted" hardlinks and the expected
> behavior in commit log? And what does "indefinety" mean? I couldn't find
> it in dictionary.
>

Try the typos dictionary ;) I meant indefinitely

I will try to explain better in change log, but here is the full story:

The simplest way to understand this test is this:
Imagine that you have a tool (e.g. xfs_db) with which
you can add hardlinks, without changing the value of nlink
stored on-disk for the inode. This is exactly what this test does when
it adds lower hardlinks underneath a mounted overlay.

Overlayfs assumes that the lower layer files are not modified
underneath it and if they do, the documentation says:
"Changes to the underlying filesystems while part of a mounted overlay
filesystem are not allowed.  If the underlying filesystem is changed,
the behavior of the overlay is undefined, though it will not result in
a crash or deadlock."

As far as I know, this test cannot crash the kernel, but it does trigger
the WARN_ON in drop_link() when nlink drops below zero, so it's not
nice behavior and could possibly results in worse outcomes.



>> +
>> +# Create lower hardlink
>> +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
>> +mkdir -p $lowerdir
>> +touch $lowerdir/0
>> +ln $lowerdir/0 $lowerdir/1
>> +
>> +_scratch_mount
>> +
>> +# Copy up lower hardlink - nlink should be 2

At this time overlay records the initial lower nlink
and assumes it is not going to change.

>> +touch $SCRATCH_MNT/0
>> +
>> +# Add lower hardlinks while overlay is mounted
>
> Why "while overlay is mounted" is needed?

Now overlay *knows* about 2 hardlinks, but we
actually have 4 hardlinks.

>
>> +ln $lowerdir/0 $lowerdir/2
>> +ln $lowerdir/0 $lowerdir/3
>> +
>> +# Unlink un-accounted lowers to drive nlink to 0
>> +rm $SCRATCH_MNT/2
>> +rm $SCRATCH_MNT/3
>
> drive nlink of which file to 0? Sorry, I'm a bit lost on overlay
> hardlink behaviors..

This is new and confusing.
The nlink of the union overlay file is driven to zero, because
it started with nlink 2 (which was copied up from lower) and
then 2 (unaccounted) hardlinks where unlinked dropping nlink
by 2 without ever adding 2.

>
>> +
>> +# Check for getting ENOENT for trying to link !I_LINKABLE with nlink 0
>> +ln $SCRATCH_MNT/0 $SCRATCH_MNT/4
>
> Shouldn't we expect an error message from this ln, if "check for getting
> ENOEND"?

The error is expected if overlay is fooled by this maneuver, but we
detect this use case and fix nlink to > 0 when it is about to drop to zero
because of wrong accounting.

> BTW, this test passed on 4.12-rc7 kernel, I'm not sure if
> that's expected result.
>

Yes, it is expected because in 4.12 hardilnks are always broken on copy up
so they are each of the new lower hardlinks is copied up to a new file
with nlink 1,
so there is no "union nlink accounting" in v4.12 at all and there is
nothing to fool.

Re: [PATCH v2 7/7] overlay: test dropping nlink below zero

[Eryu Guan]

On Wed, Jul 05, 2017 at 02:17:41PM +0300, Amir Goldstein wrote:
> On Wed, Jul 5, 2017 at 1:09 PM, Eryu Guan <eguan@redhat.com> wrote:
> > On Tue, Jul 04, 2017 at 02:40:34PM +0300, Amir Goldstein wrote:
> >> nlink of overlay inode could be dropped indefinety by adding
> >> un-accounted lower hardlinks underneath a mounted overlay and
> >> trying to remove them.
> >
> > Sorry, I didn't quite follow the hardlink patches, could you please
> > describe what is "accounted/un-accounted" hardlinks and the expected
> > behavior in commit log? And what does "indefinety" mean? I couldn't find
> > it in dictionary.
> >
> 
> Try the typos dictionary ;) I meant indefinitely
> 
> I will try to explain better in change log, but here is the full story:
> 
> The simplest way to understand this test is this:
> Imagine that you have a tool (e.g. xfs_db) with which
> you can add hardlinks, without changing the value of nlink
> stored on-disk for the inode. This is exactly what this test does when
> it adds lower hardlinks underneath a mounted overlay.
> 
> Overlayfs assumes that the lower layer files are not modified
> underneath it and if they do, the documentation says:
> "Changes to the underlying filesystems while part of a mounted overlay
> filesystem are not allowed.  If the underlying filesystem is changed,
> the behavior of the overlay is undefined, though it will not result in
> a crash or deadlock."
> 
> As far as I know, this test cannot crash the kernel, but it does trigger
> the WARN_ON in drop_link() when nlink drops below zero, so it's not
> nice behavior and could possibly results in worse outcomes.

I understand now, thanks! Yeah, it's good to have these in commit log or
comments :)

Thanks,
Eryu

Re: [PATCH v2 5/7] overlay: test concurrent copy up of lower hardlinks

[Eryu Guan]

On Wed, Jul 05, 2017 at 01:46:51PM +0300, Amir Goldstein wrote:
> On Wed, Jul 5, 2017 at 12:59 PM, Eryu Guan <eguan@redhat.com> wrote:
> > On Tue, Jul 04, 2017 at 02:40:32PM +0300, Amir Goldstein wrote:
> >> Two tasks make a modification concurrently on two hardlinks of a large
> >> lower inode.  The copy up should be triggered by one of the tasks and the
> >> other should be waiting for copy up to complete.  Both copy up targets
> >> should end up being upper hardlinks and both metadata changes should be
> >> visible in both hardlinks.
> >>
> >> With kernel <= v4.12, hardlinks are broken on copy up, meaning that copy up
> >
> > So this will be fixed in 4.13-rc1 kernel?
> 
> It is fixed on current overlayfs-next branch. Yes.
> 
> 
> >> +
> >> +_do_cmd()
> >
> > I'd rename it to do_cmd for a local helper function.
> >
> 
> sure
> 
> >> +{
> >> +     echo "`date +%T` $1..." >> $seqres.full
> >> +     eval "$1"
> >> +     echo "`date +%T` ...$1" >> $seqres.full
> >> +}
> >> +
> >> +# Perform one modification on each hardlink (size and owner)
> >> +_do_cmd "echo >> $SCRATCH_MNT/one" &
> >> +#
> >> +# When hardlinks are broken and overlayfs supports concurrent copy up,
> >> +# $seqres.full will show that file two copy up started ~2s after file one
> >> +# copy up started and ended ~2s after file one copy up ended.
> >> +# With synchronized copy up of lower inodes, $seqres.full will show that
> >> +# file two copy up ended at the same time as file one copy up.
> >> +#
> >> +sleep 2
> >
> > If the first copyup finished within 2 seconds, it's not a concurrent
> > modification on the two hardlinks. Does that break the test assumption?
> 
> It will not be a race, plain 2 consequent copy ups, so not testing anything
> special, but won't cause test to fail.
> 
> I am testing overlayfs with 2 underlying fs configurations:
> 1. xfs with clone support
> 2. ext4
> 
> With xfs clone up takes fraction of a second, the test is not really concurrent.
> With ext4, on my laptop with SSD, the 500M copy up takes ~3 seconds,
> which is quite close to the 2 seconds delay.
> I increased the copy up file size to 1G and now first copy up takes ~6 seconds,
> so there is more slack for the 2 sec delay, but still keeps the test 'quick'.

Then what about don't sleep at all? So it doesn't rely on the copyup
speed, the only drawback is the timestamps won't tell us more about the
copyup behavior, but that doesn't seem critical to this test.

But if sleep is still needed, some comments about it would be good.

Thanks,
Eryu

> 
> >
> >> +_do_cmd "chown 100 $SCRATCH_MNT/two" &
> >> +
> >> +wait
> >> +
> >> +# Expect all hardlinks to show both metadata modifications
> >> +for f in zero one two; do
> >> +     _ls_l -n $SCRATCH_MNT/$f | awk '{ print $2, $3, $5, $9 }' | _filter_scratch
> >
> > Better to comment on what are the 4 fields.
> >
> 
> They show the metadata that was changes by chown and write.
> Will add a comment.
> 
> Thanks,
> Amir.
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/7] overlay: test concurrent copy up of lower hardlinks

[Amir Goldstein]

On Wed, Jul 5, 2017 at 2:32 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Wed, Jul 05, 2017 at 01:46:51PM +0300, Amir Goldstein wrote:
>> On Wed, Jul 5, 2017 at 12:59 PM, Eryu Guan <eguan@redhat.com> wrote:
>> > On Tue, Jul 04, 2017 at 02:40:32PM +0300, Amir Goldstein wrote:
>> >> Two tasks make a modification concurrently on two hardlinks of a large
>> >> lower inode.  The copy up should be triggered by one of the tasks and the
>> >> other should be waiting for copy up to complete.  Both copy up targets
>> >> should end up being upper hardlinks and both metadata changes should be
>> >> visible in both hardlinks.
>> >>
>> >> With kernel <= v4.12, hardlinks are broken on copy up, meaning that copy up
>> >
>> > So this will be fixed in 4.13-rc1 kernel?
>>
>> It is fixed on current overlayfs-next branch. Yes.
>>
>>
>> >> +
>> >> +_do_cmd()
>> >
>> > I'd rename it to do_cmd for a local helper function.
>> >
>>
>> sure
>>
>> >> +{
>> >> +     echo "`date +%T` $1..." >> $seqres.full
>> >> +     eval "$1"
>> >> +     echo "`date +%T` ...$1" >> $seqres.full
>> >> +}
>> >> +
>> >> +# Perform one modification on each hardlink (size and owner)
>> >> +_do_cmd "echo >> $SCRATCH_MNT/one" &
>> >> +#
>> >> +# When hardlinks are broken and overlayfs supports concurrent copy up,
>> >> +# $seqres.full will show that file two copy up started ~2s after file one
>> >> +# copy up started and ended ~2s after file one copy up ended.
>> >> +# With synchronized copy up of lower inodes, $seqres.full will show that
>> >> +# file two copy up ended at the same time as file one copy up.
>> >> +#
>> >> +sleep 2
>> >
>> > If the first copyup finished within 2 seconds, it's not a concurrent
>> > modification on the two hardlinks. Does that break the test assumption?
>>
>> It will not be a race, plain 2 consequent copy ups, so not testing anything
>> special, but won't cause test to fail.
>>
>> I am testing overlayfs with 2 underlying fs configurations:
>> 1. xfs with clone support
>> 2. ext4
>>
>> With xfs clone up takes fraction of a second, the test is not really concurrent.
>> With ext4, on my laptop with SSD, the 500M copy up takes ~3 seconds,
>> which is quite close to the 2 seconds delay.
>> I increased the copy up file size to 1G and now first copy up takes ~6 seconds,
>> so there is more slack for the 2 sec delay, but still keeps the test 'quick'.
>
> Then what about don't sleep at all? So it doesn't rely on the copyup
> speed, the only drawback is the timestamps won't tell us more about the
> copyup behavior, but that doesn't seem critical to this test.
>
> But if sleep is still needed, some comments about it would be good.

I'll go with comments.
Thanks.

Re: [PATCH v2 5/7] overlay: test concurrent copy up of lower hardlinks

[Amir Goldstein]

On Wed, Jul 5, 2017 at 1:46 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Jul 5, 2017 at 12:59 PM, Eryu Guan <eguan@redhat.com> wrote:
>> On Tue, Jul 04, 2017 at 02:40:32PM +0300, Amir Goldstein wrote:
>>> Two tasks make a modification concurrently on two hardlinks of a large
>>> lower inode.  The copy up should be triggered by one of the tasks and the
>>> other should be waiting for copy up to complete.  Both copy up targets
>>> should end up being upper hardlinks and both metadata changes should be
>>> visible in both hardlinks.
>>>
>>> With kernel <= v4.12, hardlinks are broken on copy up, meaning that copy up
>>
>> So this will be fixed in 4.13-rc1 kernel?
>
> It is fixed on current overlayfs-next branch. Yes.
>

Eryu,

I realize that my answer was not accurate.
These tests do pass with current overlayfs-next branch, but
only with non default kernel config CONFIG_OVERLAY_FS_INDEX=y.

This may be the right way to test, meaning that default kernel
config reports failed tests related to hardlinks which are really broken
on copy up with default kernel config.

Another way to go at it is having these tests
"_require_fs_module_param index" (CONFIG_OVERLAY_FS_INDEX
sets the default of this parameter) and then opt-in to indexing in the test
using the index=on mount option.

This way, those test will notrun on old kernels and pass
on new kernels regardless of the kernel config option.
Then they will be testing that "hardlinks are not broken IF
admin opts-in for indexing".

I wonder which of the test methodologies you prefer.
If you like the second one better I can send a patch to make those
tests depend on and opt-in for indexing.

Thanks,
Amir.

Re: [PATCH v2 5/7] overlay: test concurrent copy up of lower hardlinks

[Eryu Guan]

On Tue, Jul 11, 2017 at 11:23:50PM +0300, Amir Goldstein wrote:
> On Wed, Jul 5, 2017 at 1:46 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Wed, Jul 5, 2017 at 12:59 PM, Eryu Guan <eguan@redhat.com> wrote:
> >> On Tue, Jul 04, 2017 at 02:40:32PM +0300, Amir Goldstein wrote:
> >>> Two tasks make a modification concurrently on two hardlinks of a large
> >>> lower inode.  The copy up should be triggered by one of the tasks and the
> >>> other should be waiting for copy up to complete.  Both copy up targets
> >>> should end up being upper hardlinks and both metadata changes should be
> >>> visible in both hardlinks.
> >>>
> >>> With kernel <= v4.12, hardlinks are broken on copy up, meaning that copy up
> >>
> >> So this will be fixed in 4.13-rc1 kernel?
> >
> > It is fixed on current overlayfs-next branch. Yes.
> >
> 
> Eryu,
> 
> I realize that my answer was not accurate.
> These tests do pass with current overlayfs-next branch, but
> only with non default kernel config CONFIG_OVERLAY_FS_INDEX=y.

Thanks for the heads-up!

> 
> This may be the right way to test, meaning that default kernel
> config reports failed tests related to hardlinks which are really broken
> on copy up with default kernel config.

So these hardlink tests are still valid tests and the failures should be
fixed eventually, even for OVERLAY_FS_INDEX=n kernels? If so, I think we
can just keep the tests unchanged, just like all other tests that are
targeting un-fixed bugs. Then the only issue is the commit log is not so
accurate.

Otherwise, I prefer your opt-in way, making these tests _notrun
(assmuing they're not valid tests for this kernel config).

Thanks,
Eryu

> 
> Another way to go at it is having these tests
> "_require_fs_module_param index" (CONFIG_OVERLAY_FS_INDEX
> sets the default of this parameter) and then opt-in to indexing in the test
> using the index=on mount option.
> 
> This way, those test will notrun on old kernels and pass
> on new kernels regardless of the kernel config option.
> Then they will be testing that "hardlinks are not broken IF
> admin opts-in for indexing".
> 
> I wonder which of the test methodologies you prefer.
> If you like the second one better I can send a patch to make those
> tests depend on and opt-in for indexing.
> 
> Thanks,
> Amir.

Re: [PATCH v2 5/7] overlay: test concurrent copy up of lower hardlinks

[Amir Goldstein]

On Wed, Jul 12, 2017 at 6:17 AM, Eryu Guan <eguan@redhat.com> wrote:
> On Tue, Jul 11, 2017 at 11:23:50PM +0300, Amir Goldstein wrote:
>> On Wed, Jul 5, 2017 at 1:46 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> > On Wed, Jul 5, 2017 at 12:59 PM, Eryu Guan <eguan@redhat.com> wrote:
>> >> On Tue, Jul 04, 2017 at 02:40:32PM +0300, Amir Goldstein wrote:
>> >>> Two tasks make a modification concurrently on two hardlinks of a large
>> >>> lower inode.  The copy up should be triggered by one of the tasks and the
>> >>> other should be waiting for copy up to complete.  Both copy up targets
>> >>> should end up being upper hardlinks and both metadata changes should be
>> >>> visible in both hardlinks.
>> >>>
>> >>> With kernel <= v4.12, hardlinks are broken on copy up, meaning that copy up
>> >>
>> >> So this will be fixed in 4.13-rc1 kernel?
>> >
>> > It is fixed on current overlayfs-next branch. Yes.
>> >
>>
>> Eryu,
>>
>> I realize that my answer was not accurate.
>> These tests do pass with current overlayfs-next branch, but
>> only with non default kernel config CONFIG_OVERLAY_FS_INDEX=y.
>
> Thanks for the heads-up!
>
>>
>> This may be the right way to test, meaning that default kernel
>> config reports failed tests related to hardlinks which are really broken
>> on copy up with default kernel config.
>
> So these hardlink tests are still valid tests and the failures should be
> fixed eventually, even for OVERLAY_FS_INDEX=n kernels? If so, I think we

The tests are valid anyway. hardlinks should not be broken when opened
for write.
This is why it might be best to leave the tests as is.
But no, it will not be fixed for OVERLAY_FS_INDEX=n kernels and
unlikely for kernel
default to become OVERLAY_FS_INDEX=y.

The way forward is for distros to change the their config to OVERLAY_FS_INDEX=y,
but there is a downside for OVERLAY_FS_INDEX=y related to copying upper layers
between file systems as described in [1].
So before distros can change the config to OVERLAY_FS_INDEX=y they need to make
sure that no tool is migrating upper layers between file systems or
copying within
a file system. If such tools exist, then they should "export/import"
the index while
migrating layers (to fix the broken file handles) and said
export/import tool does not exist yet.

[1] https://github.com/amir73il/linux/commit/9412812ef54861081904f24ddaf176b957b98d40

> can just keep the tests unchanged, just like all other tests that are
> targeting un-fixed bugs. Then the only issue is the commit log is not so
> accurate.
>
> Otherwise, I prefer your opt-in way, making these tests _notrun
> (assmuing they're not valid tests for this kernel config).

So as I explained, they are valid, but not expected to be fixed by any
change in upstream
kernel, only a change in distro kernel config.

I will post a patch to make them opt-in, because it will provide for
much better test coverage
of the new feature as long as the feature exists in tested kernel.

Thanks.