[PATCH v2 0/5] Test overlayfs readdir cache

[PATCH v2 0/5] Test overlayfs readdir cache

[Amir Goldstein]

Eryu,

This extends the generic t_dir_offset2 helper program to verify
some cases of missing/stale entries and adds a new generic test which
passes on overlayfs (and other fs) on upstream kernel.

The overlayfs specific test fails on upstream kernel and the fix commit
is currently in linux-next.  As usual, you may want to wait with merging
until the fix commit hits upstream.

Based on feedback from Miklos, I changed the test to check for the
missing/stale entries on a new fd, while old fd is kept open, because
POSIX allows for stale/missing entries in the old fd.

I was looking into another speculated bug in overlayfs which involves
multiple calls to getdents.  Although it turned out that overlayfs does
not have the speculated bug, I left both generic and overlay test with
multiple calls to getdents in order to excersize the relevant code.

The attached patch was used to verify that the overlayfs test excercises
the call to ovl_cache_update_ino() with stale entries.
Overlayfs populates the merge dir readdir cache with a list of files in
the first getdents call, but updates d_ino of files on the list in
subsequent getdents calls.  By that time, the last entry is stale and the
following warning is printed (on linux-next with patch below applied):
[   ] overlayfs: failed to look up (m100) for ino (0)
[   ] overlayfs: failed to look up (f100) for ino (0)

Miklos,

Do you think it is worth the trouble to set p->is_whiteout and skip
dir_emit() in this case? and do we need to worry about lookup_one_len()
returning -ENOENT in this case?

Thanks,
Amir.

Changes since v1:
- Use small getdents buffer to force multiple calls
- Tidy up new command line options for t_dir_offset2
- Check missing/stale entries on new fd
- Add impure dir use case to overlay test

Amir Goldstein (5):
  src/t_dir_offset2: Add an option to limit of buffer size
  src/t_dir_offset2: Add an option to find file by name
  src/t_dir_offset2: Add option to create or unlink file
  generic: Test readdir of modified directrory
  overlay: Test invalidate of readdir cache

 src/t_dir_offset2.c   | 113 ++++++++++++++++++++++++++++++++++++++--
 tests/generic/700     |  62 ++++++++++++++++++++++
 tests/generic/700.out |   2 +
 tests/generic/group   |   1 +
 tests/overlay/077     | 117 ++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/077.out |   2 +
 tests/overlay/group   |   1 +
 7 files changed, 293 insertions(+), 5 deletions(-)
 create mode 100755 tests/generic/700
 create mode 100644 tests/generic/700.out
 create mode 100755 tests/overlay/077
 create mode 100644 tests/overlay/077.out

--
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index cc1e80257064..cadcbfafa179 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -486,7 +486,7 @@ static int ovl_cache_update_ino(struct path *path, struct ovl_cache_entry *p)
                        this = NULL;
                        goto fail;
                }
-               goto out;
+               goto fail;
        }
 
 get:
-- 
2.31.1

[PATCH v2 1/5] src/t_dir_offset2: Add an option to limit of buffer size

[Amir Goldstein]

Will be used to force readdir in several getdents calls.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 src/t_dir_offset2.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/src/t_dir_offset2.c b/src/t_dir_offset2.c
index 5a6d7193..7aeb990e 100644
--- a/src/t_dir_offset2.c
+++ b/src/t_dir_offset2.c
@@ -30,16 +30,32 @@ struct linux_dirent64 {
 static uint64_t d_off_history[HISTORY_LEN];
 static uint64_t d_ino_history[HISTORY_LEN];
 
-int
-main(int argc, char *argv[])
+void usage()
 {
-	int fd, nread;
+	fprintf(stderr, "usage: t_dir_offset2: <dir> [bufsize]\n");
+	exit(EXIT_FAILURE);
+}
+
+int main(int argc, char *argv[])
+{
+	int fd;
 	char buf[BUF_SIZE];
+	int nread, bufsize = BUF_SIZE;
 	struct linux_dirent64 *d;
 	int bpos, total, i;
 	off_t lret;
 	int retval = EXIT_SUCCESS;
 
+	if (argc > 2) {
+		bufsize = atoi(argv[2]);
+		if (!bufsize)
+			usage();
+		if (bufsize > BUF_SIZE)
+			bufsize = BUF_SIZE;
+	} else if (argc < 2) {
+		usage();
+	}
+
 	fd = open(argv[1], O_RDONLY | O_DIRECTORY);
 	if (fd < 0) {
 		perror("open");
@@ -48,7 +64,7 @@ main(int argc, char *argv[])
 
 	total = 0;
 	for ( ; ; ) {
-		nread = syscall(SYS_getdents64, fd, buf, BUF_SIZE);
+		nread = syscall(SYS_getdents64, fd, buf, bufsize);
 		if (nread == -1) {
 			perror("getdents");
 			exit(EXIT_FAILURE);
@@ -89,7 +105,7 @@ main(int argc, char *argv[])
 			exit(EXIT_FAILURE);
 		}
 
-		nread = syscall(SYS_getdents64, fd, buf, BUF_SIZE);
+		nread = syscall(SYS_getdents64, fd, buf, bufsize);
 		if (nread == -1) {
 			perror("getdents");
 			exit(EXIT_FAILURE);
-- 
2.31.1

[PATCH v2 2/5] src/t_dir_offset2: Add an option to find file by name

[Amir Goldstein]

Will be used to check for missing/stale entries.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 src/t_dir_offset2.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/src/t_dir_offset2.c b/src/t_dir_offset2.c
index 7aeb990e..7af24519 100644
--- a/src/t_dir_offset2.c
+++ b/src/t_dir_offset2.c
@@ -8,11 +8,13 @@
  * that these offsets are seekable to entry with the right inode.
  */
 
+#include <errno.h>
 #include <fcntl.h>
 #include <stdio.h>
 #include <unistd.h>
 #include <stdint.h>
 #include <stdlib.h>
+#include <string.h>
 #include <sys/stat.h>
 #include <sys/syscall.h>
 
@@ -32,7 +34,7 @@ static uint64_t d_ino_history[HISTORY_LEN];
 
 void usage()
 {
-	fprintf(stderr, "usage: t_dir_offset2: <dir> [bufsize]\n");
+	fprintf(stderr, "usage: t_dir_offset2: <dir> [[bufsize] <filename> [-v]]\n");
 	exit(EXIT_FAILURE);
 }
 
@@ -45,6 +47,9 @@ int main(int argc, char *argv[])
 	int bpos, total, i;
 	off_t lret;
 	int retval = EXIT_SUCCESS;
+	const char *filename = NULL;
+	int exists = 0, found = 0;
+	int verbose = 0;
 
 	if (argc > 2) {
 		bufsize = atoi(argv[2]);
@@ -52,6 +57,12 @@ int main(int argc, char *argv[])
 			usage();
 		if (bufsize > BUF_SIZE)
 			bufsize = BUF_SIZE;
+
+		if (argc > 3) {
+			filename = argv[3];
+			if (argc > 4 && !strcmp(argv[4], "-v"))
+				verbose = 1;
+		}
 	} else if (argc < 2) {
 		usage();
 	}
@@ -62,6 +73,14 @@ int main(int argc, char *argv[])
 		exit(EXIT_FAILURE);
 	}
 
+	if (filename) {
+		exists = !faccessat(fd, filename, F_OK, AT_SYMLINK_NOFOLLOW);
+		if (!exists && errno != ENOENT) {
+			perror("faccessat");
+			exit(EXIT_FAILURE);
+		}
+	}
+
 	total = 0;
 	for ( ; ; ) {
 		nread = syscall(SYS_getdents64, fd, buf, bufsize);
@@ -91,10 +110,28 @@ int main(int argc, char *argv[])
 			}
 			d_off_history[total] = d->d_off;
 			d_ino_history[total] = d->d_ino;
+			if (filename) {
+				if (verbose)
+					printf("entry #%d: %s (d_ino=%lld, d_off=%lld)\n",
+					       i, d->d_name, (long long int)d->d_ino,
+					       (long long int)d->d_off);
+				if (!strcmp(filename, d->d_name))
+					found = 1;
+			}
 			bpos += d->d_reclen;
 		}
 	}
 
+	if (filename) {
+		if (exists == found) {
+			printf("entry %s %sfound as expected\n", filename, found ? "" : "not ");
+		} else {
+			fprintf(stderr, "%s entry %s\n",
+				exists ? "missing" : "stale", filename);
+			exit(EXIT_FAILURE);
+		}
+	}
+
 	/* check if seek works correctly */
 	d = (struct linux_dirent64 *)buf;
 	for (i = total - 1; i >= 0; i--)
-- 
2.31.1

[PATCH v2 3/5] src/t_dir_offset2: Add option to create or unlink file

[Amir Goldstein]

Will be used to test missing/stale entries after modifications to
an open dirfd.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 src/t_dir_offset2.c | 56 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 53 insertions(+), 3 deletions(-)

diff --git a/src/t_dir_offset2.c b/src/t_dir_offset2.c
index 7af24519..75b41c1a 100644
--- a/src/t_dir_offset2.c
+++ b/src/t_dir_offset2.c
@@ -34,13 +34,13 @@ static uint64_t d_ino_history[HISTORY_LEN];
 
 void usage()
 {
-	fprintf(stderr, "usage: t_dir_offset2: <dir> [[bufsize] <filename> [-v]]\n");
+	fprintf(stderr, "usage: t_dir_offset2: <dir> [[bufsize] [-|+]<filename> [-v]]\n");
 	exit(EXIT_FAILURE);
 }
 
 int main(int argc, char *argv[])
 {
-	int fd;
+	int fd, fd2 = -1;
 	char buf[BUF_SIZE];
 	int nread, bufsize = BUF_SIZE;
 	struct linux_dirent64 *d;
@@ -49,7 +49,7 @@ int main(int argc, char *argv[])
 	int retval = EXIT_SUCCESS;
 	const char *filename = NULL;
 	int exists = 0, found = 0;
-	int verbose = 0;
+	int modify = 0, verbose = 0;
 
 	if (argc > 2) {
 		bufsize = atoi(argv[2]);
@@ -60,6 +60,13 @@ int main(int argc, char *argv[])
 
 		if (argc > 3) {
 			filename = argv[3];
+			/* +<filename> creates, -<filename> removes */
+			if (filename[0] == '+')
+				modify = 1;
+			else if (filename[0] == '-')
+				modify = -1;
+			if (modify)
+				filename++;
 			if (argc > 4 && !strcmp(argv[4], "-v"))
 				verbose = 1;
 		}
@@ -89,6 +96,49 @@ int main(int argc, char *argv[])
 			exit(EXIT_FAILURE);
 		}
 
+		if (modify && fd2 < 0 && total == 0) {
+			printf("getdents at offset 0 returned %d bytes\n", nread);
+
+			/* create/unlink entry after first getdents */
+			if (modify > 0) {
+				if (openat(fd, filename, O_CREAT, 0600) < 0) {
+					perror("openat");
+					exit(EXIT_FAILURE);
+				}
+				exists = 1;
+				printf("created entry %s\n", filename);
+			} else if (modify < 0) {
+				if (unlinkat(fd, filename, 0) < 0) {
+					perror("unlinkat");
+					exit(EXIT_FAILURE);
+				}
+				exists = 0;
+				printf("unlinked entry %s\n", filename);
+			}
+
+			/*
+			 * Old fd may not return new entry and may return stale
+			 * entries which is allowed.  Keep old fd open and open
+			 * a new fd to check for stale or missing entries later.
+			 */
+			fd2 = open(argv[1], O_RDONLY | O_DIRECTORY);
+			if (fd2 < 0) {
+				perror("open fd2");
+				exit(EXIT_FAILURE);
+			}
+		}
+
+		if (nread == 0) {
+			if (fd2 < 0 || fd == fd2)
+				break;
+
+			/* Re-iterate with new fd leaving old fd open */
+			fd = fd2;
+			total = 0;
+			found = 0;
+			continue;
+		}
+
 		if (nread == 0)
 			break;
 
-- 
2.31.1

[PATCH v2 4/5] generic: Test readdir of modified directrory

[Amir Goldstein]

Check that directory modifications to an open dir fd are observed
by a new open fd.

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

diff --git a/tests/generic/700 b/tests/generic/700
new file mode 100755
index 00000000..3ece88d4
--- /dev/null
+++ b/tests/generic/700
@@ -0,0 +1,62 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2021 CTERA Networks. All Rights Reserved.
+#
+# Check that directory modifications to an open dir are observed
+# by a new open fd
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# real QA test starts here
+_supported_fs generic
+_require_test
+
+rm -f $seqres.full
+
+testdir=$TEST_DIR/test-$seq
+rm -rf $testdir
+mkdir $testdir
+
+# Use small getdents bufsize to fit less than 10 entries
+# stuct linux_dirent64 is 20 bytes not including d_name
+bufsize=200
+
+# Check readdir content of an empty dir changes when adding a new file
+echo -e "\nCreate file 0 in an open dir:" >> $seqres.full
+$here/src/t_dir_offset2 $testdir $bufsize "+0" 2>&1 >> $seqres.full || \
+	echo "Missing created file in open dir (see $seqres.full for details)"
+
+# Create enough files to be returned in multiple gendents() calls.
+# At least one of the files that we delete will not be listed in the
+# first call, so we may encounter stale entries in following calls.
+for n in {1..100}; do
+    touch $testdir/$n
+done
+
+# Check readdir content changes after removing files
+for n in {1..10}; do
+	echo -e "\nRemove file ${n}0 in an open dir:" >> $seqres.full
+	$here/src/t_dir_offset2 $testdir $bufsize "-${n}0" 2>&1 >> $seqres.full || \
+		echo "Found unlinked files in open dir (see $seqres.full for details)"
+done
+
+# success, all done
+echo "Silence is golden"
+status=0
diff --git a/tests/generic/700.out b/tests/generic/700.out
new file mode 100644
index 00000000..f9acaa71
--- /dev/null
+++ b/tests/generic/700.out
@@ -0,0 +1,2 @@
+QA output created by 700
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index ab00cc04..bac5aa48 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -634,3 +634,4 @@
 629 auto quick rw copy_range
 630 auto quick rw dedupe clone
 631 auto rw overlay rename
+700 auto quick dir
-- 
2.31.1

[PATCH v2 5/5] overlay: Test invalidate of readdir cache

[Amir Goldstein]

This is a regression test for kernel commit 65cd913ec9d9
("ovl: invalidate readdir cache on changes to dir with origin")

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

diff --git a/tests/overlay/077 b/tests/overlay/077
new file mode 100755
index 00000000..58c0f3b5
--- /dev/null
+++ b/tests/overlay/077
@@ -0,0 +1,117 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2021 CTERA Networks. All Rights Reserved.
+#
+# FS QA Test 077
+#
+# Test invalidate of readdir cache
+#
+# This is a regression test for kernel commit 65cd913ec9d9
+# ("ovl: invalidate readdir cache on changes to dir with origin")
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+rm -f $seqres.full
+
+# real QA test starts here
+_supported_fs overlay
+_require_scratch_nocheck
+
+# Use small getdents bufsize to fit less than 10 entries
+# stuct linux_dirent64 is 20 bytes not including d_name
+bufsize=200
+
+# Create enough files to be returned in multiple gendents() calls.
+# At least one of the files that we delete will not be listed in the
+# first call, so we may encounter stale entries in following calls.
+create_files() {
+	for n in {1..100}; do
+		touch ${1}/${2}${n}
+	done
+}
+
+# remove all files from previous runs
+_scratch_mkfs
+
+# Create test area with a merge dir, a "former" merge dir,
+# a pure upper dir and impure upper dir. For each case, overlayfs
+# readdir cache is used a bit differently.
+lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
+upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
+
+mkdir -p $lowerdir/merge $lowerdir/former $upperdir/pure $upperdir/impure
+# Lower files in merge dir are listed first
+create_files $lowerdir/merge m
+# Files to be moved into impure upper dir
+create_files $lowerdir o
+# File to be copied up to make former merge dir impure
+touch $lowerdir/former/f100
+
+_scratch_mount
+
+create_files $SCRATCH_MNT/pure p
+create_files $SCRATCH_MNT/former f
+# Copy up file so readdir will need to lookup its origin d_ino
+touch $SCRATCH_MNT/merge/m100
+# Move copied up files so readdir will need to lookup origin d_ino
+mv $SCRATCH_MNT/o* $SCRATCH_MNT/impure/
+
+# Remove the lower directory and mount overlay again to create
+# a "former merge dir"
+$UMOUNT_PROG $SCRATCH_MNT
+rm -rf $lowerdir/former
+_scratch_mount
+
+# Check readdir cache invalidate on pure upper dir
+echo -e "\nCreate file in pure upper dir:" >> $seqres.full
+$here/src/t_dir_offset2 $SCRATCH_MNT/pure $bufsize "+p0" 2>&1 >> $seqres.full || \
+	echo "Missing created file in pure upper dir (see $seqres.full for details)"
+echo -e "\nRemove file in pure upper dir:" >> $seqres.full
+$here/src/t_dir_offset2 $SCRATCH_MNT/pure $bufsize "-p100" 2>&1 >> $seqres.full || \
+	echo "Found unlinked file in pure upper dir (see $seqres.full for details)"
+
+# Check readdir cache invalidate on impure upper dir
+echo -e "\nCreate file in impure upper dir:" >> $seqres.full
+$here/src/t_dir_offset2 $SCRATCH_MNT/impure $bufsize "+o0" 2>&1 >> $seqres.full || \
+	echo "Missing created file in impure upper dir (see $seqres.full for details)"
+echo -e "\nRemove file in impure upper dir:" >> $seqres.full
+$here/src/t_dir_offset2 $SCRATCH_MNT/impure $bufsize  "-o100" 2>&1 >> $seqres.full || \
+	echo "Found unlinked file in impure upper dir (see $seqres.full for details)"
+
+# Check readdir cache invalidate on merge dir
+echo -e "\nCreate file in merge dir:" >> $seqres.full
+$here/src/t_dir_offset2 $SCRATCH_MNT/merge $bufsize "+m0" 2>&1 >> $seqres.full || \
+	echo "Missing created file in merge dir (see $seqres.full for details)"
+echo -e "\nRemove file in merge dir:" >> $seqres.full
+$here/src/t_dir_offset2 $SCRATCH_MNT/merge $bufsize "-m100" 2>&1 >> $seqres.full || \
+	echo "Found unlinked file in merge dir (see $seqres.full for details)"
+
+# Check readdir cache invalidate on former merge dir
+echo -e "\nCreate file in former merge dir:" >> $seqres.full
+$here/src/t_dir_offset2 $SCRATCH_MNT/former $bufsize "+f0" 2>&1 >> $seqres.full || \
+	echo "Missing created file in former merge dir (see $seqres.full for details)"
+echo -e "\nRemove file in former merge dir:" >> $seqres.full
+$here/src/t_dir_offset2 $SCRATCH_MNT/former $bufsize "-f100" 2>&1 >> $seqres.full || \
+	echo "Found unlinked file in former merge dir (see $seqres.full for details)"
+
+# success, all done
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/overlay/077.out b/tests/overlay/077.out
new file mode 100644
index 00000000..11538276
--- /dev/null
+++ b/tests/overlay/077.out
@@ -0,0 +1,2 @@
+QA output created by 077
+Silence is golden
diff --git a/tests/overlay/group b/tests/overlay/group
index ddc355e5..bd014f20 100644
--- a/tests/overlay/group
+++ b/tests/overlay/group
@@ -79,6 +79,7 @@
 074 auto quick exportfs dangerous
 075 auto quick perms
 076 auto quick perms dangerous
+077 auto quick dir
 100 auto quick union samefs
 101 auto quick union nonsamefs
 102 auto quick union nonsamefs xino
-- 
2.31.1

Re: [PATCH v2 0/5] Test overlayfs readdir cache

[Miklos Szeredi]

On Sun, Apr 25, 2021 at 9:14 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Eryu,
>
> This extends the generic t_dir_offset2 helper program to verify
> some cases of missing/stale entries and adds a new generic test which
> passes on overlayfs (and other fs) on upstream kernel.
>
> The overlayfs specific test fails on upstream kernel and the fix commit
> is currently in linux-next.  As usual, you may want to wait with merging
> until the fix commit hits upstream.
>
> Based on feedback from Miklos, I changed the test to check for the
> missing/stale entries on a new fd, while old fd is kept open, because
> POSIX allows for stale/missing entries in the old fd.
>
> I was looking into another speculated bug in overlayfs which involves
> multiple calls to getdents.  Although it turned out that overlayfs does
> not have the speculated bug, I left both generic and overlay test with
> multiple calls to getdents in order to excersize the relevant code.
>
> The attached patch was used to verify that the overlayfs test excercises
> the call to ovl_cache_update_ino() with stale entries.
> Overlayfs populates the merge dir readdir cache with a list of files in
> the first getdents call, but updates d_ino of files on the list in
> subsequent getdents calls.  By that time, the last entry is stale and the
> following warning is printed (on linux-next with patch below applied):
> [   ] overlayfs: failed to look up (m100) for ino (0)
> [   ] overlayfs: failed to look up (f100) for ino (0)
>
> Miklos,
>
> Do you think it is worth the trouble to set p->is_whiteout and skip
> dir_emit() in this case? and do we need to worry about lookup_one_len()
> returning -ENOENT in this case?

So lookup_one_len() first does a cached lookup, and if found returns
that.  If not then it calls the filesystem's ->lookup() callback,
which in this case is ovl_lookup().  AFAICS ovl_lookup() will never
return ENOENT, even if the underlying filesystem does.

Which means it's not necessary to worry about that case.

The other case you found it that in case of a stale direntry the i_ino
update will be skipped and so it will return an inconsistent result,
right?   Fixing that looks worthwhile, yes.

Thanks,
Miklos

Re: [PATCH v2 0/5] Test overlayfs readdir cache

[Amir Goldstein]

On Mon, Apr 26, 2021 at 1:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sun, Apr 25, 2021 at 9:14 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Eryu,
> >
> > This extends the generic t_dir_offset2 helper program to verify
> > some cases of missing/stale entries and adds a new generic test which
> > passes on overlayfs (and other fs) on upstream kernel.
> >
> > The overlayfs specific test fails on upstream kernel and the fix commit
> > is currently in linux-next.  As usual, you may want to wait with merging
> > until the fix commit hits upstream.
> >
> > Based on feedback from Miklos, I changed the test to check for the
> > missing/stale entries on a new fd, while old fd is kept open, because
> > POSIX allows for stale/missing entries in the old fd.
> >
> > I was looking into another speculated bug in overlayfs which involves
> > multiple calls to getdents.  Although it turned out that overlayfs does
> > not have the speculated bug, I left both generic and overlay test with
> > multiple calls to getdents in order to excersize the relevant code.
> >
> > The attached patch was used to verify that the overlayfs test excercises
> > the call to ovl_cache_update_ino() with stale entries.
> > Overlayfs populates the merge dir readdir cache with a list of files in
> > the first getdents call, but updates d_ino of files on the list in
> > subsequent getdents calls.  By that time, the last entry is stale and the
> > following warning is printed (on linux-next with patch below applied):
> > [   ] overlayfs: failed to look up (m100) for ino (0)
> > [   ] overlayfs: failed to look up (f100) for ino (0)
> >
> > Miklos,
> >
> > Do you think it is worth the trouble to set p->is_whiteout and skip
> > dir_emit() in this case? and do we need to worry about lookup_one_len()
> > returning -ENOENT in this case?
>
> So lookup_one_len() first does a cached lookup, and if found returns
> that.  If not then it calls the filesystem's ->lookup() callback,
> which in this case is ovl_lookup().  AFAICS ovl_lookup() will never
> return ENOENT, even if the underlying filesystem does.
>
> Which means it's not necessary to worry about that case.
>
> The other case you found it that in case of a stale direntry the i_ino
> update will be skipped and so it will return an inconsistent result,
> right?

Right. It returns a stale entry with the old real ino.
Not sure if that is an "inconsistent" result.
inconsistent w.r.t what?

> Fixing that looks worthwhile, yes.
>

Will look into it.

Thanks,
Amir.

Re: [PATCH v2 0/5] Test overlayfs readdir cache

[Miklos Szeredi]

On Mon, Apr 26, 2021 at 12:15 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, Apr 26, 2021 at 1:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:

> > The other case you found it that in case of a stale direntry the i_ino
> > update will be skipped and so it will return an inconsistent result,
> > right?
>
> Right. It returns a stale entry with the old real ino.
> Not sure if that is an "inconsistent" result.
> inconsistent w.r.t what?

It's inconsistent with previous (before the entry got deleted)
st_ino/i_ino.  This should actually be testable.

But it was a long time ago that I fully understood the readdir code,
so I might be missing something.

Thanks,
Miklos

Re: [PATCH v2 0/5] Test overlayfs readdir cache

[Amir Goldstein]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

On Mon, Apr 26, 2021 at 4:13 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, Apr 26, 2021 at 12:15 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Mon, Apr 26, 2021 at 1:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> > > The other case you found it that in case of a stale direntry the i_ino
> > > update will be skipped and so it will return an inconsistent result,
> > > right?
> >
> > Right. It returns a stale entry with the old real ino.
> > Not sure if that is an "inconsistent" result.
> > inconsistent w.r.t what?
>
> It's inconsistent with previous (before the entry got deleted)
> st_ino/i_ino.  This should actually be testable.

Right. it is testable:

     QA output created by 077
    +entry m100 has inconsistent d_ino (266 != 264)
    +entry f100 has inconsistent d_ino (367 != 16777542)
     Silence is golden

These prints are from the iteration on the first fd.
The first fd lists the stale entry with inconsistent d_ino.
The second fd does not list the stale entry (with bugfix in linux-next).
Will add it to the test in V3.

Attached patch fixes this problem.

Thanks,
Amir.

[-- Attachment #2: 0001-ovl-skip-stale-entries-in-merge-dir-cache-iteration.patch.txt --]
[-- Type: text/plain, Size: 1890 bytes --]

From 304e1599cc112fae388d0c0b2aaabdf385032226 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Mon, 26 Apr 2021 18:07:09 +0300
Subject: [PATCH] ovl: skip stale entries in merge dir cache iteration

On the first getdents call, ovl_iterate() populates the readdir cache
with a list of entries, but for upper entries with origin lower inode,
p->ino remains zero.

Following getdents calls traverse the readdir cache list and call
ovl_cache_update_ino() for entries with zero p->ino to lookup the entry
in the overlay and return d_ino that is consistent with st_ino.

If the upper file was unlinked between the first getdents call and the
getdents call that lists the file entry, ovl_cache_update_ino() will not
find the entry and fall back to setting d_ino to the upper real st_ino,
which is inconsistent with how this object was presented to users.

Instead of listing a stale entry with inconsistent d_ino, simply skip
the stale entry, which is better for users.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/readdir.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index cc1e80257064..10b7780e4bdc 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -481,6 +481,8 @@ static int ovl_cache_update_ino(struct path *path, struct ovl_cache_entry *p)
 	}
 	this = lookup_one_len(p->name, dir, p->len);
 	if (IS_ERR_OR_NULL(this) || !this->d_inode) {
+		/* Mark a stale entry */
+		p->is_whiteout = true;
 		if (IS_ERR(this)) {
 			err = PTR_ERR(this);
 			this = NULL;
@@ -776,6 +778,9 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 				if (err)
 					goto out;
 			}
+		}
+		/* ovl_cache_update_ino() sets is_whiteout on stale entry */
+		if (!p->is_whiteout) {
 			if (!dir_emit(ctx, p->name, p->len, p->ino, p->type))
 				break;
 		}
-- 
2.25.1