[PATCH 0/4] Assorted overlay test fixes

[PATCH 0/4] Assorted overlay test fixes

[Amir Goldstein]

Eryu,

These assorted fixes improve test-ability with different overlay
mount option configurtions.

The first 2 patches don't fix any known issue, but they fix correctness
of the tests.

Patch 3 prepares the tests to work correctly with nfs_export=on, which
is now on Miklos' overlayfs-next branch. It shouldn't change behavior for
tests running on kernel without nfs_export.

First 3 patches may have trivial conflicts with Zhangyi's patch ("[5/5]
overlay: correct scratch dirs check").

Patch 4 fixes overlay/017 for the default redirect_dir=off configuration.

I believe there has been a miscommunication between us w.r.t overlay/017.
I guess it always fails in your tests and you thought that was expected.
Actually, since commit 0d84683 ("overlay/017: Remove constant
st_ino/d_ino test for hardlinks") the test was expected to pass with
default upstream configuration, but I missed the redirect_dir requirement
and probably also missed communicating this expectation to you.

When I tested overlay/017 with different configurations I ran into a
strange random failure that only happens with base fs xfs and overlay
mount options index=on,nfs_export=off. I finally tracked it down to an
xfs v4.15 regression and figured out why index=on improves the chances
to hit the bug (because overlay inodes are hashed).

So after patch 4 is applied, overlay/017 is expected to pass on upstream
kernel with default configuration and may may with inodex=on.
xfs bug fix was sent to Darrick.

Thanks,
Amir.

Amir Goldstein (4):
  ovelray: drop explicit use of OVERLAY_MOUNT_OPTIONS
  overlay/036: fix upper/lower dir mismatch
  overlay: consider index dir with whiteouts clean
  overlay/017: require and enable redirect_dir

 tests/overlay/017   | 6 ++++--
 tests/overlay/033   | 8 ++++++--
 tests/overlay/034   | 8 ++++++--
 tests/overlay/036   | 4 ++--
 tests/overlay/041   | 6 ++----
 tests/overlay/043   | 6 ++----
 tests/overlay/044   | 7 ++-----
 tests/overlay/048   | 4 +++-
 tests/overlay/group | 2 +-
 9 files changed, 28 insertions(+), 23 deletions(-)

-- 
2.7.4

[PATCH 1/4] ovelray: drop explicit use of OVERLAY_MOUNT_OPTIONS

[Amir Goldstein]

Tests don't need to specify $OVERLAY_MOUNT_OPTIONS for overlay mount
helpers. These options have already been assigned to MOUNT_OPTIONS by
_overlay_config_override or by _mount_opts and will be added to mount
command by _common_dev_mount_options in _overlay_mount_dirs.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/041 | 6 ++----
 tests/overlay/043 | 6 ++----
 tests/overlay/044 | 7 ++-----
 3 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/tests/overlay/041 b/tests/overlay/041
index 4152107..11efacb 100755
--- a/tests/overlay/041
+++ b/tests/overlay/041
@@ -68,8 +68,7 @@ _scratch_mkfs
 upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
 workdir=$OVL_BASE_SCRATCH_MNT/$OVL_WORK
 
-_overlay_scratch_mount_dirs $lowerdir $upperdir $workdir \
-			$OVERLAY_MOUNT_OPTIONS
+_overlay_scratch_mount_dirs $lowerdir $upperdir $workdir
 
 test_dir=$SCRATCH_MNT/test_dir/
 
@@ -185,8 +184,7 @@ _scratch_mkfs
 upperdir=$OVL_BASE_SCRATCH_MNT/ovl-upper
 workdir=$OVL_BASE_SCRATCH_MNT/ovl-work
 
-_overlay_scratch_mount_dirs $middir:$lowerdir $upperdir $workdir \
-			$OVERLAY_MOUNT_OPTIONS
+_overlay_scratch_mount_dirs $middir:$lowerdir $upperdir $workdir
 
 # Copy up test_dir
 touch $test_dir/test_file
diff --git a/tests/overlay/043 b/tests/overlay/043
index 858b6a9..699c4e1 100755
--- a/tests/overlay/043
+++ b/tests/overlay/043
@@ -81,8 +81,7 @@ _scratch_mkfs >>$seqres.full 2>&1
 upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
 workdir=$OVL_BASE_SCRATCH_MNT/$OVL_WORK
 
-_overlay_scratch_mount_dirs $lowerdir $upperdir $workdir \
-			    $OVERLAY_MOUNT_OPTIONS
+_overlay_scratch_mount_dirs $lowerdir $upperdir $workdir
 
 FILES="dir file symlink chrdev blkdev fifo socket"
 
@@ -149,8 +148,7 @@ check_inode_numbers $testdir $tmp.after_copyup $tmp.after_move
 
 # Verify that the inode numbers survive a mount cycle
 $UMOUNT_PROG $SCRATCH_MNT
-_overlay_scratch_mount_dirs $lowerdir $upperdir $workdir \
-			    $OVERLAY_MOUNT_OPTIONS
+_overlay_scratch_mount_dirs $lowerdir $upperdir $workdir
 
 # Compare inode numbers before/after mount cycle
 check_inode_numbers $testdir $tmp.after_move $tmp.after_cycle
diff --git a/tests/overlay/044 b/tests/overlay/044
index 9c0ff04..e57f6f7 100755
--- a/tests/overlay/044
+++ b/tests/overlay/044
@@ -98,9 +98,7 @@ upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
 workdir=$OVL_BASE_SCRATCH_MNT/$OVL_WORK
 
 # Enable overlay index feature to prevent breaking hardlinks on copy up
-OVERLAY_MOUNT_OPTIONS="${OVERLAY_MOUNT_OPTIONS} -o index=on"
-_overlay_scratch_mount_dirs $lowerdir $upperdir $workdir \
-			    $OVERLAY_MOUNT_OPTIONS
+_overlay_scratch_mount_dirs $lowerdir $upperdir $workdir -o index=on
 
 rm -f $tmp.*
 
@@ -124,8 +122,7 @@ check_ino_nlink $SCRATCH_MNT $tmp.before $tmp.after_one
 
 # Verify that the hardlinks survive a mount cycle
 $UMOUNT_PROG $SCRATCH_MNT
-_overlay_scratch_mount_dirs $lowerdir $upperdir $workdir \
-			    $OVERLAY_MOUNT_OPTIONS
+_overlay_scratch_mount_dirs $lowerdir $upperdir $workdir -o index=on
 
 echo "== After mount cycle =="
 cat $FILES
-- 
2.7.4

[PATCH 2/4] overlay/036: fix upper/lower dir mismatch

[Amir Goldstein]

Mount option index=on checks on mount that upper dir is not being
reused with a different lower dir than the first lower dir it was
mounted with. This behavior is verified by test overlay/037.

In this test however, it is not desired to fail mount on mismatch of
upper/lower, so use the matching upper/lower dirs in this test.

The mismatch went unnoticed because the index=off mounts do not verify
lower dir and the index=on mounts fails on EBUSY (dir in use by another
live mount) before failing on ESTALE (upper/lower dir mismatch).

Never the less, fix the mismatch, so a change in the kernel between
the two sanity checks (EBUSY vs. ESTALE) won't break the test.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/036 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/overlay/036 b/tests/overlay/036
index e0c13ae..e04aaee 100755
--- a/tests/overlay/036
+++ b/tests/overlay/036
@@ -90,7 +90,7 @@ _overlay_mount_dirs $lowerdir $upperdir $workdir \
 
 # Try to mount another overlay with the same upperdir
 # with index=off - expect success
-_overlay_mount_dirs $lowerdir2 $upperdir $workdir2 \
+_overlay_mount_dirs $lowerdir $upperdir $workdir2 \
 		    overlay0 $SCRATCH_MNT -oindex=off && \
 		    $UMOUNT_PROG $SCRATCH_MNT
 
@@ -102,7 +102,7 @@ _overlay_mount_dirs $lowerdir2 $upperdir2 $workdir \
 
 # Try to mount another overlay with the same upperdir
 # with index=on - expect EBUSY
-_overlay_mount_dirs $lowerdir2 $upperdir $workdir2 \
+_overlay_mount_dirs $lowerdir $upperdir $workdir2 \
 	    overlay2 $SCRATCH_MNT -oindex=on 2>&1 | _filter_busy_mount
 
 # Try to mount another overlay with the same workdir
-- 
2.7.4

[PATCH 3/4] overlay: consider index dir with whiteouts clean

[Amir Goldstein]

Several tests check that index dir is empty after upper entries have
been unlinked. With nfs_export=on, index will contain a whiteout index
entry in that case so, allow chardevs when checking for clean index dir.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/033 | 8 ++++++--
 tests/overlay/034 | 8 ++++++--
 tests/overlay/048 | 4 +++-
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/tests/overlay/033 b/tests/overlay/033
index 30780c6..3e67500 100755
--- a/tests/overlay/033
+++ b/tests/overlay/033
@@ -69,7 +69,6 @@ report_nlink()
 # Create lower hardlinks
 create_hardlinks()
 {
-	lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
 	mkdir -p $lowerdir
 	touch $lowerdir/0
 	ln $lowerdir/0 $lowerdir/1
@@ -123,9 +122,14 @@ test_hardlinks()
 	rm $SCRATCH_MNT/2
 
 	# Verify that orphan index is cleaned when dropping nlink to zero
-	ls $OVL_BASE_SCRATCH_MNT/$OVL_WORK/index
+	# With nfs_export=on index will contain a whiteout index entry, so allow
+	# chardev entries in index dir.
+	find $workdir/index -mindepth 1 -type c -o -print
 }
 
+lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
+workdir=$OVL_BASE_SCRATCH_MNT/$OVL_WORK
+
 # Remove all files from previous tests
 _scratch_mkfs
 
diff --git a/tests/overlay/034 b/tests/overlay/034
index dc354c6..d9f9798 100755
--- a/tests/overlay/034
+++ b/tests/overlay/034
@@ -67,11 +67,13 @@ _require_scratch
 # Without overlay index feature hardlinks are broken on copy up
 _require_scratch_feature index
 
+lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
+workdir=$OVL_BASE_SCRATCH_MNT/$OVL_WORK
+
 # 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
@@ -103,7 +105,9 @@ rm $SCRATCH_MNT/4
 
 # Verify that orphan index is cleaned on mount
 _scratch_cycle_mount index=on
-ls $OVL_BASE_SCRATCH_MNT/$OVL_WORK/index
+# With nfs_export=on index will contain a whiteout index entry, so allow
+# chardev entries in index dir.
+find $workdir/index -mindepth 1 -type c -o -print
 
 echo "Silence is golden"
 status=0
diff --git a/tests/overlay/048 b/tests/overlay/048
index 4b2c58f..3ce6270 100755
--- a/tests/overlay/048
+++ b/tests/overlay/048
@@ -111,7 +111,9 @@ test_hardlinks_offline()
 	report_nlink "unlink last lower"
 
 	# Verify that orphan index is cleaned when dropping nlink to zero
-	ls $workdir/index
+	# With nfs_export=on index will contain a whiteout index entry, so allow
+	# chardev entries in index dir.
+	find $workdir/index -mindepth 1 -type c -o -print
 }
 
 lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
-- 
2.7.4

[PATCH 4/4] overlay/017: require and enable redirect_dir

[Amir Goldstein]

This test renames a merge directory so it needs to enable redirect_dir
feature, which is not enabled by default.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/017   | 6 ++++--
 tests/overlay/group | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/overlay/017 b/tests/overlay/017
index 03955d0..e7e8925 100755
--- a/tests/overlay/017
+++ b/tests/overlay/017
@@ -57,6 +57,7 @@ _supported_os Linux
 _require_scratch
 _require_test_program "af_unix"
 _require_test_program "t_dir_type"
+_require_scratch_feature redirect_dir
 
 rm -f $seqres.full
 
@@ -111,7 +112,8 @@ function check_inode_numbers()
 	done
 }
 
-_scratch_mount
+# Enable redirect_dir for renaming a merge directory
+_scratch_mount -o "redirect_dir=on"
 
 
 rm -f $tmp.*
@@ -140,7 +142,7 @@ echo 3 > /proc/sys/vm/drop_caches
 check_inode_numbers $testdir $tmp.after_copyup $tmp.after_move
 
 # Verify that the inode numbers survive a mount cycle
-_scratch_cycle_mount
+_scratch_cycle_mount "redirect_dir=on"
 
 # Compare inode numbers before/after mount cycle
 check_inode_numbers $testdir $tmp.after_move $tmp.after_cycle
diff --git a/tests/overlay/group b/tests/overlay/group
index 7e541e4..edea64a 100644
--- a/tests/overlay/group
+++ b/tests/overlay/group
@@ -19,7 +19,7 @@
 014 auto quick copyup
 015 auto quick whiteout
 016 auto quick copyup
-017 auto quick copyup
+017 auto quick copyup redirect
 018 auto quick copyup hardlink
 019 auto stress
 020 auto quick copyup perms
-- 
2.7.4

Re: [PATCH 4/4] overlay/017: require and enable redirect_dir

[Eryu Guan]

On Fri, Jan 26, 2018 at 09:59:08AM +0200, Amir Goldstein wrote:
> This test renames a merge directory so it needs to enable redirect_dir
> feature, which is not enabled by default.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

As you mentioned, I thought the failure was expected.. Thanks for the
updates! I just made some cosmetic changes on commit.

> ---
>  tests/overlay/017   | 6 ++++--
>  tests/overlay/group | 2 +-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/overlay/017 b/tests/overlay/017
> index 03955d0..e7e8925 100755
> --- a/tests/overlay/017
> +++ b/tests/overlay/017
> @@ -57,6 +57,7 @@ _supported_os Linux
>  _require_scratch
>  _require_test_program "af_unix"
>  _require_test_program "t_dir_type"
> +_require_scratch_feature redirect_dir

Added comments on why we require redirect_dir feature here.

>  
>  rm -f $seqres.full
>  
> @@ -111,7 +112,8 @@ function check_inode_numbers()
>  	done
>  }
>  
> -_scratch_mount
> +# Enable redirect_dir for renaming a merge directory
> +_scratch_mount -o "redirect_dir=on"

Quoted the whole extra mount options here, "-o redirect_dir=on"

>  

and removed an extra empty line.

Thanks,
Eryu

>  
>  rm -f $tmp.*
> @@ -140,7 +142,7 @@ echo 3 > /proc/sys/vm/drop_caches
>  check_inode_numbers $testdir $tmp.after_copyup $tmp.after_move
>  
>  # Verify that the inode numbers survive a mount cycle
> -_scratch_cycle_mount
> +_scratch_cycle_mount "redirect_dir=on"
>  
>  # Compare inode numbers before/after mount cycle
>  check_inode_numbers $testdir $tmp.after_move $tmp.after_cycle
> diff --git a/tests/overlay/group b/tests/overlay/group
> index 7e541e4..edea64a 100644
> --- a/tests/overlay/group
> +++ b/tests/overlay/group
> @@ -19,7 +19,7 @@
>  014 auto quick copyup
>  015 auto quick whiteout
>  016 auto quick copyup
> -017 auto quick copyup
> +017 auto quick copyup redirect
>  018 auto quick copyup hardlink
>  019 auto stress
>  020 auto quick copyup perms
> -- 
> 2.7.4
> 
> --
> 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 4/4] overlay/017: require and enable redirect_dir

[Amir Goldstein]

On Mon, Jan 29, 2018 at 10:10 AM, Eryu Guan <eguan@redhat.com> wrote:
> On Fri, Jan 26, 2018 at 09:59:08AM +0200, Amir Goldstein wrote:
>> This test renames a merge directory so it needs to enable redirect_dir
>> feature, which is not enabled by default.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> As you mentioned, I thought the failure was expected.. Thanks for the
> updates! I just made some cosmetic changes on commit.
>
>> ---
>>  tests/overlay/017   | 6 ++++--
>>  tests/overlay/group | 2 +-
>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/overlay/017 b/tests/overlay/017
>> index 03955d0..e7e8925 100755
>> --- a/tests/overlay/017
>> +++ b/tests/overlay/017
>> @@ -57,6 +57,7 @@ _supported_os Linux
>>  _require_scratch
>>  _require_test_program "af_unix"
>>  _require_test_program "t_dir_type"
>> +_require_scratch_feature redirect_dir
>
> Added comments on why we require redirect_dir feature here.
>
>>
>>  rm -f $seqres.full
>>
>> @@ -111,7 +112,8 @@ function check_inode_numbers()
>>       done
>>  }
>>
>> -_scratch_mount
>> +# Enable redirect_dir for renaming a merge directory
>> +_scratch_mount -o "redirect_dir=on"
>
> Quoted the whole extra mount options here, "-o redirect_dir=on"
>

Sure. I see there are plenty cases of quoted as well as unquoted
options passed to _scratch_mount.
I must say I lean towards the unquoted flavor, but doesn't really matter.

The worst yet is that _scratch_cycle_mount are passed without -o.
Another time..

Thanks,
Amir.