[PATCH 0/5] fstests: more tests for overlay constant inode numbers

[PATCH 0/5] fstests: more tests for overlay constant inode numbers

[Amir Goldstein]

Hi Eryu,

This series contains enhancements to overlay/017, which I used to test
my work on overlayfs constant inode numbers. [1]

The original test was a bit naiive, not taking into account renames,
drop caches and mount cycle. All those are added by this series.

My work covers only the inode numbers returned from stat(2) and not
the inode numbers returned in d_ino from readdir(3), so the 'find -inum'
part of this test could still fail with my overlayfs patches.

However, I ran my tests in kvm-xfstests VM, where 'find -inum' called
stat(2) for each entry, so the test did pass.

I will dig deeper into this behavior when I work on fixing d_ino
values in the next part of my work.

Amir.

[1] https://marc.info/?l=linux-unionfs&m=149324252301397&w=2

Amir Goldstein (5):
  overlay/017: silence test output
  overlay/017: use af_unix to create socket test file
  overlay/017: create a helper to record inode number
  overlay/017: verify constant inode number after rename
  overlay/017: test persistent inode numbers after mount cycle

 tests/overlay/017     | 69 +++++++++++++++++++++++++++++++++++++++++++++------
 tests/overlay/017.out |  7 +-----
 2 files changed, 62 insertions(+), 14 deletions(-)

-- 
2.7.4

[PATCH 1/5] overlay/017: silence test output

[Amir Goldstein]

Change test to output golden silence on success.

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

diff --git a/tests/overlay/017 b/tests/overlay/017
index 5ca3040..d6ba2ae 100755
--- a/tests/overlay/017
+++ b/tests/overlay/017
@@ -89,11 +89,13 @@ done
 
 # find by inode number - expect to find file by inode number
 cat $tmp.before | while read ino f; do
-	find $f -inum $ino -maxdepth 0 | _filter_scratch
+	find $SCRATCH_MNT/ -inum $ino -maxdepth 1 | grep -q $f || \
+		echo "$f not found by ino $ino"
 done
 
 # Compare before..after - expect silence
 diff $tmp.before $tmp.after
 
+echo "Silence is golden"
 status=0
 exit
diff --git a/tests/overlay/017.out b/tests/overlay/017.out
index 1f277c5..8222844 100644
--- a/tests/overlay/017.out
+++ b/tests/overlay/017.out
@@ -1,7 +1,2 @@
 QA output created by 017
-SCRATCH_MNT/dir
-SCRATCH_MNT/file
-SCRATCH_MNT/symlink
-SCRATCH_MNT/chrdev
-SCRATCH_MNT/blkdev
-SCRATCH_MNT/fifo
+Silence is golden
-- 
2.7.4

Re: [PATCH 1/5] overlay/017: silence test output

[Eryu Guan]

On Thu, Apr 27, 2017 at 06:09:31PM +0300, Amir Goldstein wrote:
> Change test to output golden silence on success.

Some words on why this change is needed would be good.

> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  tests/overlay/017     | 4 +++-
>  tests/overlay/017.out | 7 +------
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/overlay/017 b/tests/overlay/017
> index 5ca3040..d6ba2ae 100755
> --- a/tests/overlay/017
> +++ b/tests/overlay/017
> @@ -89,11 +89,13 @@ done
>  
>  # find by inode number - expect to find file by inode number
>  cat $tmp.before | while read ino f; do
> -	find $f -inum $ino -maxdepth 0 | _filter_scratch
> +	find $SCRATCH_MNT/ -inum $ino -maxdepth 1 | grep -q $f || \
> +		echo "$f not found by ino $ino"

Hmm, if I run this find command manually, I got warning about -maxdepth
position.

# find `pwd` -inum 3932421 -maxdepth 1
find: warning: you have specified the -maxdepth option after a non-option argument -inum, but options are not positional (-maxdepth affects tests specified before it as well as those specified after it).  Please specify options before other arguments.

/root/xfstests/check

Move -maxdepth option just after find path works fine

# find `pwd` -maxdepth 1 -inum 3932421 
/root/xfstests/check

But I'm not sure why I didn't see this warning by running overlay/017..
Anyway, moving -maxdepth around seems better to me.

Thanks,
Eryu

>  done
>  
>  # Compare before..after - expect silence
>  diff $tmp.before $tmp.after
>  
> +echo "Silence is golden"
>  status=0
>  exit
> diff --git a/tests/overlay/017.out b/tests/overlay/017.out
> index 1f277c5..8222844 100644
> --- a/tests/overlay/017.out
> +++ b/tests/overlay/017.out
> @@ -1,7 +1,2 @@
>  QA output created by 017
> -SCRATCH_MNT/dir
> -SCRATCH_MNT/file
> -SCRATCH_MNT/symlink
> -SCRATCH_MNT/chrdev
> -SCRATCH_MNT/blkdev
> -SCRATCH_MNT/fifo
> +Silence is golden
> -- 
> 2.7.4
> 

Re: [PATCH 1/5] overlay/017: silence test output

[Amir Goldstein]

On Fri, Apr 28, 2017 at 8:36 AM, Eryu Guan <eguan@redhat.com> wrote:
> On Thu, Apr 27, 2017 at 06:09:31PM +0300, Amir Goldstein wrote:
>> Change test to output golden silence on success.
>
> Some words on why this change is needed would be good.
>
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  tests/overlay/017     | 4 +++-
>>  tests/overlay/017.out | 7 +------
>>  2 files changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/tests/overlay/017 b/tests/overlay/017
>> index 5ca3040..d6ba2ae 100755
>> --- a/tests/overlay/017
>> +++ b/tests/overlay/017
>> @@ -89,11 +89,13 @@ done
>>
>>  # find by inode number - expect to find file by inode number
>>  cat $tmp.before | while read ino f; do
>> -     find $f -inum $ino -maxdepth 0 | _filter_scratch
>> +     find $SCRATCH_MNT/ -inum $ino -maxdepth 1 | grep -q $f || \
>> +             echo "$f not found by ino $ino"
>
> Hmm, if I run this find command manually, I got warning about -maxdepth
> position.
>
> # find `pwd` -inum 3932421 -maxdepth 1
> find: warning: you have specified the -maxdepth option after a non-option argument -inum, but options are not positional (-maxdepth affects tests specified before it as well as those specified after it).  Please specify options before other arguments.
>
> /root/xfstests/check
>
> Move -maxdepth option just after find path works fine
>
> # find `pwd` -maxdepth 1 -inum 3932421
> /root/xfstests/check
>
> But I'm not sure why I didn't see this warning by running overlay/017..

man find...

"The default behavior corresponds to -warn if standard input is a tty,
and to -nowarn otherwise."

> Anyway, moving -maxdepth around seems better to me.
>

Sure.

Re: [PATCH 1/5] overlay/017: silence test output

[Eryu Guan]

On Fri, Apr 28, 2017 at 08:45:22AM +0300, Amir Goldstein wrote:
> On Fri, Apr 28, 2017 at 8:36 AM, Eryu Guan <eguan@redhat.com> wrote:
> > On Thu, Apr 27, 2017 at 06:09:31PM +0300, Amir Goldstein wrote:
> >> Change test to output golden silence on success.
> >
> > Some words on why this change is needed would be good.
> >
> >>
> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >> ---
> >>  tests/overlay/017     | 4 +++-
> >>  tests/overlay/017.out | 7 +------
> >>  2 files changed, 4 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/tests/overlay/017 b/tests/overlay/017
> >> index 5ca3040..d6ba2ae 100755
> >> --- a/tests/overlay/017
> >> +++ b/tests/overlay/017
> >> @@ -89,11 +89,13 @@ done
> >>
> >>  # find by inode number - expect to find file by inode number
> >>  cat $tmp.before | while read ino f; do
> >> -     find $f -inum $ino -maxdepth 0 | _filter_scratch
> >> +     find $SCRATCH_MNT/ -inum $ino -maxdepth 1 | grep -q $f || \
> >> +             echo "$f not found by ino $ino"
> >
> > Hmm, if I run this find command manually, I got warning about -maxdepth
> > position.
> >
> > # find `pwd` -inum 3932421 -maxdepth 1
> > find: warning: you have specified the -maxdepth option after a non-option argument -inum, but options are not positional (-maxdepth affects tests specified before it as well as those specified after it).  Please specify options before other arguments.
> >
> > /root/xfstests/check
> >
> > Move -maxdepth option just after find path works fine
> >
> > # find `pwd` -maxdepth 1 -inum 3932421
> > /root/xfstests/check
> >
> > But I'm not sure why I didn't see this warning by running overlay/017..
> 
> man find...
> 
> "The default behavior corresponds to -warn if standard input is a tty,
> and to -nowarn otherwise."

That explains, thanks!

Eryu

> 
> > Anyway, moving -maxdepth around seems better to me.
> >
> 
> Sure.

Re: [PATCH 1/5] overlay/017: silence test output

[Amir Goldstein]

On Fri, Apr 28, 2017 at 8:50 AM, Eryu Guan <eguan@redhat.com> wrote:
> On Fri, Apr 28, 2017 at 08:45:22AM +0300, Amir Goldstein wrote:
>> On Fri, Apr 28, 2017 at 8:36 AM, Eryu Guan <eguan@redhat.com> wrote:
>> > On Thu, Apr 27, 2017 at 06:09:31PM +0300, Amir Goldstein wrote:
>> >> Change test to output golden silence on success.
>> >
>> > Some words on why this change is needed would be good.
>> >
>> >>
>> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> >> ---
>> >>  tests/overlay/017     | 4 +++-
>> >>  tests/overlay/017.out | 7 +------
>> >>  2 files changed, 4 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/tests/overlay/017 b/tests/overlay/017
>> >> index 5ca3040..d6ba2ae 100755
>> >> --- a/tests/overlay/017
>> >> +++ b/tests/overlay/017
>> >> @@ -89,11 +89,13 @@ done
>> >>
>> >>  # find by inode number - expect to find file by inode number
>> >>  cat $tmp.before | while read ino f; do
>> >> -     find $f -inum $ino -maxdepth 0 | _filter_scratch
>> >> +     find $SCRATCH_MNT/ -inum $ino -maxdepth 1 | grep -q $f || \
>> >> +             echo "$f not found by ino $ino"
>> >
>> > Hmm, if I run this find command manually, I got warning about -maxdepth
>> > position.
>> >
>> > # find `pwd` -inum 3932421 -maxdepth 1
>> > find: warning: you have specified the -maxdepth option after a non-option argument -inum, but options are not positional (-maxdepth affects tests specified before it as well as those specified after it).  Please specify options before other arguments.
>> >
>> > /root/xfstests/check
>> >
>> > Move -maxdepth option just after find path works fine
>> >
>> > # find `pwd` -maxdepth 1 -inum 3932421
>> > /root/xfstests/check
>> >
>> > But I'm not sure why I didn't see this warning by running overlay/017..
>>
>> man find...
>>
>> "The default behavior corresponds to -warn if standard input is a tty,
>> and to -nowarn otherwise."
>
> That explains, thanks!
>

In your setup does strace find `pwd` -maxdepth 1 -inum 3932421
show newfstatat  for each inode?
It should be doing stat only for dirs according to the code and
Enabled features of find --version.
Not sure why it does newfstatat for every inode on my kvm machine...

Amir.

Re: [PATCH 1/5] overlay/017: silence test output

[Eryu Guan]

On Fri, Apr 28, 2017 at 09:34:18AM +0300, Amir Goldstein wrote:
> In your setup does strace find `pwd` -maxdepth 1 -inum 3932421
> show newfstatat  for each inode?
> It should be doing stat only for dirs according to the code and
> Enabled features of find --version.
> Not sure why it does newfstatat for every inode on my kvm machine...
> 
> Amir.

My find only calls newfstatat on dirs, find version
findutils-4.5.11-5.el7.x86_64

# strace find `pwd` -maxdepth 1 -inum 3932421 2>&1 | grep newfstatat
newfstatat(AT_FDCWD, "/root/xfstests", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(AT_FDCWD, "/root/xfstests", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "testlogs", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "tests", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "btrfsstress-v5", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "testpatch", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "btrfsstress-v4", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "include", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "ltp", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "autom4te.cache", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "btrfsstress", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "crash", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "build", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "tools", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "lib", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "btrfsstress-v2", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, ".git", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "configs", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "dmapi", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "patches", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "btrfsstress-v3", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "src", {st_mode=S_IFDIR|0775, st_size=12288, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "results", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "m4", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "doc", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(5, "common", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0

Eryu

[PATCH 2/5] overlay/017: use af_unix to create socket test file

[Amir Goldstein]

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

diff --git a/tests/overlay/017 b/tests/overlay/017
index d6ba2ae..66a028f 100755
--- a/tests/overlay/017
+++ b/tests/overlay/017
@@ -34,6 +34,7 @@ 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
@@ -51,6 +52,7 @@ _cleanup()
 _supported_fs overlay
 _supported_os Linux
 _require_scratch
+_require_test_program "af_unix"
 
 rm -f $seqres.full
 
@@ -68,6 +70,7 @@ ln -s $lowerdir/file $lowerdir/symlink
 mknod $lowerdir/chrdev c 1 1
 mknod $lowerdir/blkdev b 1 1
 mknod $lowerdir/fifo p
+$here/src/af_unix $lowerdir/socket
 
 
 _scratch_mount
@@ -78,7 +81,7 @@ rm -f $tmp.before $tmp.after
 # Test stable stat(2) st_ino
 
 # Record inode numbers before and after copy up
-for f in dir file symlink chrdev blkdev fifo; do
+for f in dir file symlink chrdev blkdev fifo socket; do
 	ls -id $SCRATCH_MNT/$f >> $tmp.before
 	# chown -h modifies all those file types
 	chown -h 100 $SCRATCH_MNT/$f
-- 
2.7.4

[PATCH 3/5] overlay/017: create a helper to record inode number

[Amir Goldstein]

helper records inode number and file basename, so the output
is independant of the files full path. This is needed for adding
rename test.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/017 | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/tests/overlay/017 b/tests/overlay/017
index 66a028f..f3bf454 100755
--- a/tests/overlay/017
+++ b/tests/overlay/017
@@ -41,6 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _cleanup()
 {
+	cd /
 	rm -f $tmp.*
 }
 
@@ -72,22 +73,41 @@ mknod $lowerdir/blkdev b 1 1
 mknod $lowerdir/fifo p
 $here/src/af_unix $lowerdir/socket
 
+FILES="dir file symlink chrdev blkdev fifo socket"
+
+# record inode numbers in format <ino> <basename>
+function record_inode_numbers()
+{
+	dir=$1
+	outfile=$2
+
+	for f in $FILES; do
+		ls -id $dir/$f
+	done | \
+	while read ino file; do
+		echo $ino `basename $file` >> $outfile
+	done
+}
+
 
 _scratch_mount
 
 
-rm -f $tmp.before $tmp.after
+rm -f $tmp.*
 
 # Test stable stat(2) st_ino
 
-# Record inode numbers before and after copy up
-for f in dir file symlink chrdev blkdev fifo socket; do
-	ls -id $SCRATCH_MNT/$f >> $tmp.before
+# Record inode numbers before copy up
+record_inode_numbers $SCRATCH_MNT $tmp.before
+
+for f in $FILES; do
 	# chown -h modifies all those file types
 	chown -h 100 $SCRATCH_MNT/$f
-	ls -id $SCRATCH_MNT/$f >> $tmp.after
 done
 
+# Record inode numbers after copy up
+record_inode_numbers $SCRATCH_MNT $tmp.after
+
 # Test stable readdir(3)/getdents(2) d_ino
 
 # find by inode number - expect to find file by inode number
-- 
2.7.4

[PATCH 4/5] overlay/017: verify constant inode number after rename

[Amir Goldstein]

The test verifies constant inode number after copy up.
Verify that inode number remains constant also after rename
and drop caches (when overlayfs needs to find the lower
inodes in another location).

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

diff --git a/tests/overlay/017 b/tests/overlay/017
index f3bf454..1cf684d 100755
--- a/tests/overlay/017
+++ b/tests/overlay/017
@@ -8,7 +8,8 @@
 # - modify A to trigger copy up
 # - stat file A shows inode number Y != X
 #
-# Also test if d_ino of readdir entries changes after copy up.
+# Also test if d_ino of readdir entries changes after copy up
+# and if inode numbers persist after rename and drop caches.
 #
 #-----------------------------------------------------------------------
 #
@@ -94,6 +95,8 @@ _scratch_mount
 
 
 rm -f $tmp.*
+testdir=$SCRATCH_MNT/test
+mkdir -p $testdir
 
 # Test stable stat(2) st_ino
 
@@ -106,18 +109,29 @@ for f in $FILES; do
 done
 
 # Record inode numbers after copy up
-record_inode_numbers $SCRATCH_MNT $tmp.after
+record_inode_numbers $SCRATCH_MNT $tmp.after_copyup
+
+for f in $FILES; do
+	# move to another dir
+	mv $SCRATCH_MNT/$f $testdir/
+done
+
+echo 3 > /proc/sys/vm/drop_caches
+
+# Record inode numbers after rename and drop caches
+record_inode_numbers $testdir $tmp.after_move
 
 # Test stable readdir(3)/getdents(2) d_ino
 
 # find by inode number - expect to find file by inode number
 cat $tmp.before | while read ino f; do
-	find $SCRATCH_MNT/ -inum $ino -maxdepth 1 | grep -q $f || \
+	find $testdir/ -inum $ino -maxdepth 1 | grep -q $f || \
 		echo "$f not found by ino $ino"
 done
 
 # Compare before..after - expect silence
-diff $tmp.before $tmp.after
+diff -u $tmp.before $tmp.after_copyup
+diff -u $tmp.after_copyup $tmp.after_move
 
 echo "Silence is golden"
 status=0
-- 
2.7.4

Re: [PATCH 4/5] overlay/017: verify constant inode number after rename

[Eryu Guan]

On Thu, Apr 27, 2017 at 06:09:34PM +0300, Amir Goldstein wrote:
> The test verifies constant inode number after copy up.
> Verify that inode number remains constant also after rename
> and drop caches (when overlayfs needs to find the lower
> inodes in another location).
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  tests/overlay/017 | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/overlay/017 b/tests/overlay/017
> index f3bf454..1cf684d 100755
> --- a/tests/overlay/017
> +++ b/tests/overlay/017
> @@ -8,7 +8,8 @@
>  # - modify A to trigger copy up
>  # - stat file A shows inode number Y != X
>  #
> -# Also test if d_ino of readdir entries changes after copy up.
> +# Also test if d_ino of readdir entries changes after copy up
> +# and if inode numbers persist after rename and drop caches.
>  #
>  #-----------------------------------------------------------------------
>  #
> @@ -94,6 +95,8 @@ _scratch_mount
>  
>  
>  rm -f $tmp.*
> +testdir=$SCRATCH_MNT/test
> +mkdir -p $testdir
>  
>  # Test stable stat(2) st_ino
>  
> @@ -106,18 +109,29 @@ for f in $FILES; do
>  done
>  
>  # Record inode numbers after copy up
> -record_inode_numbers $SCRATCH_MNT $tmp.after
> +record_inode_numbers $SCRATCH_MNT $tmp.after_copyup

inode numbers are saved after copyup and will be tested by diff with
$tmp.before, but find (d_ino) is not tested after a pure copyup...

> +
> +for f in $FILES; do
> +	# move to another dir
> +	mv $SCRATCH_MNT/$f $testdir/
> +done
> +
> +echo 3 > /proc/sys/vm/drop_caches
> +
> +# Record inode numbers after rename and drop caches
> +record_inode_numbers $testdir $tmp.after_move
>  
>  # Test stable readdir(3)/getdents(2) d_ino
>  
>  # find by inode number - expect to find file by inode number
>  cat $tmp.before | while read ino f; do
> -	find $SCRATCH_MNT/ -inum $ino -maxdepth 1 | grep -q $f || \
> +	find $testdir/ -inum $ino -maxdepth 1 | grep -q $f || \
>  		echo "$f not found by ino $ino"
>  done

it's only tested here, after a rename and a drop_caches.

IMO, it's better to test both find and stat after each operation that
would cause inode number change. So how about factoring out a helper
that does this inode number check (find and diff), and calling it after
each operation?

e.g. a new helper called check_inode_number(), then

<create files>
<copyup>
check_inode_number [with necessary args]
<rename>
check_inode_number
<drop_caches>
check_inode_number
<cycle mount>
check_inode_number

Thanks,
Eryu

>  
>  # Compare before..after - expect silence
> -diff $tmp.before $tmp.after
> +diff -u $tmp.before $tmp.after_copyup
> +diff -u $tmp.after_copyup $tmp.after_move
>  
>  echo "Silence is golden"
>  status=0
> -- 
> 2.7.4
> 

Re: [PATCH 4/5] overlay/017: verify constant inode number after rename

[Amir Goldstein]

On Fri, Apr 28, 2017 at 8:47 AM, Eryu Guan <eguan@redhat.com> wrote:
> On Thu, Apr 27, 2017 at 06:09:34PM +0300, Amir Goldstein wrote:
>> The test verifies constant inode number after copy up.
>> Verify that inode number remains constant also after rename
>> and drop caches (when overlayfs needs to find the lower
>> inodes in another location).
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  tests/overlay/017 | 22 ++++++++++++++++++----
>>  1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/overlay/017 b/tests/overlay/017
>> index f3bf454..1cf684d 100755
>> --- a/tests/overlay/017
>> +++ b/tests/overlay/017
>> @@ -8,7 +8,8 @@
>>  # - modify A to trigger copy up
>>  # - stat file A shows inode number Y != X
>>  #
>> -# Also test if d_ino of readdir entries changes after copy up.
>> +# Also test if d_ino of readdir entries changes after copy up
>> +# and if inode numbers persist after rename and drop caches.
>>  #
>>  #-----------------------------------------------------------------------
>>  #
>> @@ -94,6 +95,8 @@ _scratch_mount
>>
>>
>>  rm -f $tmp.*
>> +testdir=$SCRATCH_MNT/test
>> +mkdir -p $testdir
>>
>>  # Test stable stat(2) st_ino
>>
>> @@ -106,18 +109,29 @@ for f in $FILES; do
>>  done
>>
>>  # Record inode numbers after copy up
>> -record_inode_numbers $SCRATCH_MNT $tmp.after
>> +record_inode_numbers $SCRATCH_MNT $tmp.after_copyup
>
> inode numbers are saved after copyup and will be tested by diff with
> $tmp.before, but find (d_ino) is not tested after a pure copyup...
>
>> +
>> +for f in $FILES; do
>> +     # move to another dir
>> +     mv $SCRATCH_MNT/$f $testdir/
>> +done
>> +
>> +echo 3 > /proc/sys/vm/drop_caches
>> +
>> +# Record inode numbers after rename and drop caches
>> +record_inode_numbers $testdir $tmp.after_move
>>
>>  # Test stable readdir(3)/getdents(2) d_ino
>>
>>  # find by inode number - expect to find file by inode number
>>  cat $tmp.before | while read ino f; do
>> -     find $SCRATCH_MNT/ -inum $ino -maxdepth 1 | grep -q $f || \
>> +     find $testdir/ -inum $ino -maxdepth 1 | grep -q $f || \
>>               echo "$f not found by ino $ino"
>>  done
>
> it's only tested here, after a rename and a drop_caches.
>
> IMO, it's better to test both find and stat after each operation that
> would cause inode number change. So how about factoring out a helper
> that does this inode number check (find and diff), and calling it after
> each operation?
>
> e.g. a new helper called check_inode_number(), then
>
> <create files>
> <copyup>
> check_inode_number [with necessary args]
> <rename>
> check_inode_number
> <drop_caches>
> check_inode_number
> <cycle mount>
> check_inode_number
>


Yes, that would be much better.
Thanks!

[PATCH 5/5] overlay/017: test persistent inode numbers after mount cycle

[Amir Goldstein]

Overlayfs directory inodes are constant across copy up,
but not persistent on mount cycle.
Compare the inode numbers before and after mount cycle.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/017 | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tests/overlay/017 b/tests/overlay/017
index 1cf684d..fe66f4c 100755
--- a/tests/overlay/017
+++ b/tests/overlay/017
@@ -9,7 +9,8 @@
 # - stat file A shows inode number Y != X
 #
 # Also test if d_ino of readdir entries changes after copy up
-# and if inode numbers persist after rename and drop caches.
+# and if inode numbers persist after rename, drop caches and
+# mount cycle.
 #
 #-----------------------------------------------------------------------
 #
@@ -133,6 +134,19 @@ done
 diff -u $tmp.before $tmp.after_copyup
 diff -u $tmp.after_copyup $tmp.after_move
 
+# Verify that the inode numbers survive a mount cycle
+_scratch_cycle_mount
+
+record_inode_numbers $testdir $tmp.after_cycle
+
+cat $tmp.after_move | while read ino f; do
+	find $testdir/ -inum $ino -maxdepth 1 | grep -q $f || \
+		echo "$f not found by ino $ino"
+done
+
+# Compare before..after - expect silence
+diff -u $tmp.after_move $tmp.after_cycle
+
 echo "Silence is golden"
 status=0
 exit
-- 
2.7.4

Re: [PATCH 0/5] fstests: more tests for overlay constant inode numbers

[Eryu Guan]

On Thu, Apr 27, 2017 at 06:09:30PM +0300, Amir Goldstein wrote:
> Hi Eryu,
> 
> This series contains enhancements to overlay/017, which I used to test
> my work on overlayfs constant inode numbers. [1]
> 
> The original test was a bit naiive, not taking into account renames,
> drop caches and mount cycle. All those are added by this series.

Thanks for the tests! Though usually we don't add new tests to existing
test cases, given that overlay/017 never passed before and won't pass
after adding these tests too, I think it's fine to merge this patchset,
as overlay/017 won't cause any false regression in tests.

Some minor comments go to individual patches.

Thanks,
Eryu

> 
> My work covers only the inode numbers returned from stat(2) and not
> the inode numbers returned in d_ino from readdir(3), so the 'find -inum'
> part of this test could still fail with my overlayfs patches.
> 
> However, I ran my tests in kvm-xfstests VM, where 'find -inum' called
> stat(2) for each entry, so the test did pass.
> 
> I will dig deeper into this behavior when I work on fixing d_ino
> values in the next part of my work.
> 
> Amir.
> 
> [1] https://marc.info/?l=linux-unionfs&m=149324252301397&w=2
> 
> Amir Goldstein (5):
>   overlay/017: silence test output
>   overlay/017: use af_unix to create socket test file
>   overlay/017: create a helper to record inode number
>   overlay/017: verify constant inode number after rename
>   overlay/017: test persistent inode numbers after mount cycle
> 
>  tests/overlay/017     | 69 +++++++++++++++++++++++++++++++++++++++++++++------
>  tests/overlay/017.out |  7 +-----
>  2 files changed, 62 insertions(+), 14 deletions(-)
> 
> -- 
> 2.7.4
> 

Re: [PATCH 0/5] fstests: more tests for overlay constant inode numbers

[Amir Goldstein]

On Fri, Apr 28, 2017 at 8:30 AM, Eryu Guan <eguan@redhat.com> wrote:
> On Thu, Apr 27, 2017 at 06:09:30PM +0300, Amir Goldstein wrote:
>> Hi Eryu,
>>
>> This series contains enhancements to overlay/017, which I used to test
>> my work on overlayfs constant inode numbers. [1]
>>
>> The original test was a bit naiive, not taking into account renames,
>> drop caches and mount cycle. All those are added by this series.
>
> Thanks for the tests! Though usually we don't add new tests to existing
> test cases, given that overlay/017 never passed before and won't pass
> after adding these tests too, I think it's fine to merge this patchset,
> as overlay/017 won't cause any false regression in tests.
>

That was my thinking as well...
016-018 were born to track the 'Non-standard behavior' cases from
Documentation/filesystems/overlayfs.txt in the expectation that those
will be fixed at some point.
017 was just not doing a good enough job to cover the 'non constant ino'
behavior.

I have seen some people already wanted to test my patch set, so better
have 017 prepared for them instead of having to pull it from my branch.

Heads up - I also have some patches to enhance overlay/018, but
I intend to add some more before I send out a batch.

I may also add more beef to 017 related to constant st_dev/st_ino
and not just st_ino. Since 017 is still failing, I figure that would be ok.

Amir.