linux-unionfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Test overlayfs readdir cache
@ 2021-04-21  9:23 Amir Goldstein
  2021-04-21  9:23 ` [PATCH 1/2] generic: Test readdir of modified directrory Amir Goldstein
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Amir Goldstein @ 2021-04-21  9:23 UTC (permalink / raw)
  To: Eryu Guan; +Cc: Miklos Szeredi, linux-unionfs, fstests

Eryu,

This extends the generic t_dir_offset2 test to verify
some more test cases 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.

Miklos,

I had noticed in the test full logs that readdir of
a merged dir behaves strangely - when seeking backwards
to offsets > 0, readdir returns unlinked entries in results.
The test does not fail on that behavior because the test
only asserts that this is not allowed after seek to offset 0.

Knowing the implementation of overlayfs readdir cache this is
not surprising to me, but I wonder if this behavior is POSIX
compliant, and if not, whether we should document it and/or
add a failing test for it.

Thanks,
Amir.

Amir Goldstein (2):
  generic: Test readdir of modified directrory
  overlay: Test invalidate of readdir cache

 src/t_dir_offset2.c   |  63 +++++++++++++++++++++++--
 tests/generic/700     |  60 ++++++++++++++++++++++++
 tests/generic/700.out |   2 +
 tests/generic/group   |   1 +
 tests/overlay/077     | 105 ++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/077.out |   2 +
 tests/overlay/group   |   1 +
 7 files changed, 231 insertions(+), 3 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

-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] generic: Test readdir of modified directrory
  2021-04-21  9:23 [PATCH 0/2] Test overlayfs readdir cache Amir Goldstein
@ 2021-04-21  9:23 ` Amir Goldstein
  2021-04-21  9:23 ` [PATCH 2/2] overlay: Test invalidate of readdir cache Amir Goldstein
  2021-04-22  6:18 ` [PATCH 0/2] Test overlayfs " Amir Goldstein
  2 siblings, 0 replies; 11+ messages in thread
From: Amir Goldstein @ 2021-04-21  9:23 UTC (permalink / raw)
  To: Eryu Guan; +Cc: Miklos Szeredi, linux-unionfs, fstests

Check that directory content read from an open dir fd reflects
changes to the directory after open.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 src/t_dir_offset2.c   | 63 ++++++++++++++++++++++++++++++++++++++++---
 tests/generic/700     | 60 +++++++++++++++++++++++++++++++++++++++++
 tests/generic/700.out |  2 ++
 tests/generic/group   |  1 +
 4 files changed, 123 insertions(+), 3 deletions(-)
 create mode 100755 tests/generic/700
 create mode 100644 tests/generic/700.out

diff --git a/src/t_dir_offset2.c b/src/t_dir_offset2.c
index 5a6d7193..84dab9cf 100644
--- a/src/t_dir_offset2.c
+++ b/src/t_dir_offset2.c
@@ -13,6 +13,7 @@
 #include <unistd.h>
 #include <stdint.h>
 #include <stdlib.h>
+#include <string.h>
 #include <sys/stat.h>
 #include <sys/syscall.h>
 
@@ -26,19 +27,28 @@ struct linux_dirent64 {
 
 #define BUF_SIZE 4096
 #define HISTORY_LEN 1024
+#define NAME_SIZE 10
 
 static uint64_t d_off_history[HISTORY_LEN];
 static uint64_t d_ino_history[HISTORY_LEN];
+static char d_name_history[HISTORY_LEN][NAME_SIZE+1];
+
+static int is_dot_or_dot_dot(const char *name)
+{
+	return !strcmp(name, ".") || !strcmp(name, "..");
+}
 
 int
 main(int argc, char *argv[])
 {
-	int fd, nread;
+	int fd, nread, nread_0 = 0;
 	char buf[BUF_SIZE];
 	struct linux_dirent64 *d;
 	int bpos, total, i;
 	off_t lret;
 	int retval = EXIT_SUCCESS;
+	int create = 0, unlink = 0;
+	FILE *warn = stderr;
 
 	fd = open(argv[1], O_RDONLY | O_DIRECTORY);
 	if (fd < 0) {
@@ -46,6 +56,15 @@ main(int argc, char *argv[])
 		exit(EXIT_FAILURE);
 	}
 
+	if (argc > 2) {
+		if (!strcmp(argv[2], "-u"))
+			unlink = 1;
+		else
+			create = 1;
+		/* Some mismatch warnings are expected */
+		warn = stdout;
+	}
+
 	total = 0;
 	for ( ; ; ) {
 		nread = syscall(SYS_getdents64, fd, buf, BUF_SIZE);
@@ -54,6 +73,10 @@ main(int argc, char *argv[])
 			exit(EXIT_FAILURE);
 		}
 
+		if (total == 0 && (create || unlink)) {
+			nread_0 = nread;
+			printf("getdents at offset 0 returned %d bytes\n", nread);
+		}
 		if (nread == 0)
 			break;
 
@@ -75,20 +98,40 @@ main(int argc, char *argv[])
 			}
 			d_off_history[total] = d->d_off;
 			d_ino_history[total] = d->d_ino;
+			strncpy(d_name_history[total], d->d_name, NAME_SIZE);
 			bpos += d->d_reclen;
 		}
 	}
 
+	if (create) {
+		if (openat(fd, argv[2], O_CREAT, 0600) < 0) {
+			perror("openat");
+			exit(EXIT_FAILURE);
+		}
+		printf("created entry %d:%s\n", total, argv[2]);
+	}
+
 	/* check if seek works correctly */
 	d = (struct linux_dirent64 *)buf;
 	for (i = total - 1; i >= 0; i--)
 	{
+		const char *name = i > 0 ? d_name_history[i - 1] : NULL;
+
 		lret = lseek(fd, i > 0 ? d_off_history[i - 1] : 0, SEEK_SET);
 		if (lret == -1) {
 			perror("lseek");
 			exit(EXIT_FAILURE);
 		}
 
+		/* Unlink prev entry, but not "." nor ".." */
+		if (unlink && name && !is_dot_or_dot_dot(name)) {
+			if (unlinkat(fd, name, 0) < 0) {
+				perror("unlinkat");
+				exit(EXIT_FAILURE);
+			}
+			printf("unlinked entry %d:%s\n", i - 1, name);
+		}
+
 		nread = syscall(SYS_getdents64, fd, buf, BUF_SIZE);
 		if (nread == -1) {
 			perror("getdents");
@@ -96,17 +139,31 @@ main(int argc, char *argv[])
 		}
 
 		if (nread == 0) {
-			fprintf(stderr, "getdents returned 0 on entry %d\n", i);
+			fprintf(warn, "getdents returned 0 on entry %d\n", i);
+			retval = EXIT_FAILURE;
+		}
+
+		if (i == 0 && nread_0 && nread != nread_0) {
+			fprintf(warn, "getdents at offset 0 returned %d bytes, expected %d\n", nread, nread_0);
 			retval = EXIT_FAILURE;
 		}
 
 		if (d->d_ino != d_ino_history[i]) {
-			fprintf(stderr, "entry %d has inode %lld, expected %lld\n",
+			fprintf(warn, "entry %d has inode %lld, expected %lld\n",
 				i, (long long int)d->d_ino, (long long int)d_ino_history[i]);
 			retval = EXIT_FAILURE;
 		}
+		if (create || unlink)
+			printf("found entry %d:%s (nread=%d)\n", i, d->d_name, nread);
 	}
 
+	/*
+	 * With create/unlink option we expect to find some mismatch with getdents
+	 * before create/unlink, so if everything matches there is something wrong.
+	 */
+	if (create || unlink)
+		retval = retval ? EXIT_SUCCESS : EXIT_FAILURE;
+
 	close(fd);
 	exit(retval);
 }
diff --git a/tests/generic/700 b/tests/generic/700
new file mode 100755
index 00000000..eb08958d
--- /dev/null
+++ b/tests/generic/700
@@ -0,0 +1,60 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2021 CTERA Networks. All Rights Reserved.
+#
+# Check that directory content read from an open dir fd reflects
+# changes to the directory after open.
+#
+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 number of files that we can assume to be returned in a single
+# gendents() call, so t_dir_offset2 can compare the entries count before
+# and after seek to start
+for n in {1..10}; do
+    touch $testdir/$n
+done
+
+# Does opendir, readdir, seek to offsets, readdir and
+# compares d_ino of entries on second readdir
+$here/src/t_dir_offset2 $testdir
+
+# Check readdir content changes after adding a new file before seek to start
+echo "Create file in an open dir:" >> $seqres.full
+$here/src/t_dir_offset2 $testdir 0 2>&1 >> $seqres.full || \
+	echo "Missing created file in open dir (see $seqres.full for details)"
+
+# Check readdir content changes after removing files before seek to start
+echo "Remove files in an open dir:" >> $seqres.full
+$here/src/t_dir_offset2 $testdir -u 2>&1 >> $seqres.full || \
+	echo "Found unlinked files in open dir (see $seqres.full for details)"
+
+# 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.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] overlay: Test invalidate of readdir cache
  2021-04-21  9:23 [PATCH 0/2] Test overlayfs readdir cache Amir Goldstein
  2021-04-21  9:23 ` [PATCH 1/2] generic: Test readdir of modified directrory Amir Goldstein
@ 2021-04-21  9:23 ` Amir Goldstein
  2021-04-21  9:33   ` Amir Goldstein
  2021-04-22  6:18 ` [PATCH 0/2] Test overlayfs " Amir Goldstein
  2 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2021-04-21  9:23 UTC (permalink / raw)
  To: Eryu Guan; +Cc: Miklos Szeredi, linux-unionfs, fstests

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     | 105 ++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/077.out |   2 +
 tests/overlay/group   |   1 +
 3 files changed, 108 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..e254aec1
--- /dev/null
+++ b/tests/overlay/077
@@ -0,0 +1,105 @@
+#! /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.*
+}
+
+# create test directory and test file, mount overlayfs and remove
+# testfile to create a whiteout in upper dir.
+create_whiteout()
+{
+	local lower=$1
+	local upper=$2
+	local work=$3
+	local file=$4
+
+	mkdir -p $lower/testdir
+	touch $lower/testdir/$file
+
+	_overlay_scratch_mount_dirs $lower $upper $work
+
+	rm -f $SCRATCH_MNT/testdir/$file
+
+	$UMOUNT_PROG $SCRATCH_MNT
+}
+
+# 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
+
+# remove all files from previous runs
+_scratch_mkfs
+
+# Create test area with a merge dir, a "former" merge dir and
+# a pure upper dir
+lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
+upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
+
+mkdir -p $lowerdir/merge $lowerdir/former $upperdir/pure
+touch $lowerdir/merge/{a,b,c}
+
+_scratch_mount
+
+touch $SCRATCH_MNT/pure/{a,b,c}
+touch $SCRATCH_MNT/former/{a,b,c}
+
+# 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 "Create file in pure upper dir:" >> $seqres.full
+$here/src/t_dir_offset2 $SCRATCH_MNT/pure A 2>&1 >> $seqres.full || \
+	echo "Missing created file in pure upper dir (see $seqres.full for details)"
+echo "Remove files in pure upper dir:" >> $seqres.full
+$here/src/t_dir_offset2 $SCRATCH_MNT/pure -u 2>&1 >> $seqres.full || \
+	echo "Found unlinked files in pure upper dir (see $seqres.full for details)"
+
+# Check readdir cache invalidate on merge dir
+echo "Create file in merge dir:" >> $seqres.full
+$here/src/t_dir_offset2 $SCRATCH_MNT/merge A 2>&1 >> $seqres.full || \
+	echo "Missing created file in merge dir (see $seqres.full for details)"
+echo "Remove files in merge dir:" >> $seqres.full
+$here/src/t_dir_offset2 $SCRATCH_MNT/merge -u 2>&1 >> $seqres.full || \
+	echo "Found unlinked files in merge dir (see $seqres.full for details)"
+
+# Check readdir cache invalidate on former merge dir
+echo "Create file in former merge dir:" >> $seqres.full
+$here/src/t_dir_offset2 $SCRATCH_MNT/former A 2>&1 >> $seqres.full || \
+	echo "Missing created file in former merge dir (see $seqres.full for details)"
+echo "Remove files in former merge dir:" >> $seqres.full
+$here/src/t_dir_offset2 $SCRATCH_MNT/former -u 2>&1 >> $seqres.full || \
+	echo "Found unlinked files 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..a8f98789 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 whiteout dir
 100 auto quick union samefs
 101 auto quick union nonsamefs
 102 auto quick union nonsamefs xino
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] overlay: Test invalidate of readdir cache
  2021-04-21  9:23 ` [PATCH 2/2] overlay: Test invalidate of readdir cache Amir Goldstein
@ 2021-04-21  9:33   ` Amir Goldstein
  2021-04-22  6:23     ` Amir Goldstein
  0 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2021-04-21  9:33 UTC (permalink / raw)
  To: Eryu Guan; +Cc: Miklos Szeredi, overlayfs, fstests

On Wed, Apr 21, 2021 at 12:23 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> 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     | 105 ++++++++++++++++++++++++++++++++++++++++++
>  tests/overlay/077.out |   2 +
>  tests/overlay/group   |   1 +
>  3 files changed, 108 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..e254aec1
> --- /dev/null
> +++ b/tests/overlay/077
> @@ -0,0 +1,105 @@
> +#! /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.*
> +}
> +
> +# create test directory and test file, mount overlayfs and remove
> +# testfile to create a whiteout in upper dir.
> +create_whiteout()
> +{
> +       local lower=$1
> +       local upper=$2
> +       local work=$3
> +       local file=$4
> +
> +       mkdir -p $lower/testdir
> +       touch $lower/testdir/$file
> +
> +       _overlay_scratch_mount_dirs $lower $upper $work
> +
> +       rm -f $SCRATCH_MNT/testdir/$file
> +
> +       $UMOUNT_PROG $SCRATCH_MNT
> +}
> +

Oops. Unused leftover.
A former merge dir does not need to actually have whiteouts for this test...

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] Test overlayfs readdir cache
  2021-04-21  9:23 [PATCH 0/2] Test overlayfs readdir cache Amir Goldstein
  2021-04-21  9:23 ` [PATCH 1/2] generic: Test readdir of modified directrory Amir Goldstein
  2021-04-21  9:23 ` [PATCH 2/2] overlay: Test invalidate of readdir cache Amir Goldstein
@ 2021-04-22  6:18 ` Amir Goldstein
  2021-04-22  7:53   ` Miklos Szeredi
  2 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2021-04-22  6:18 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: overlayfs, fstests, Eryu Guan

On Wed, Apr 21, 2021 at 12:23 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Eryu,
>
> This extends the generic t_dir_offset2 test to verify
> some more test cases 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.
>
> Miklos,
>
> I had noticed in the test full logs that readdir of
> a merged dir behaves strangely - when seeking backwards
> to offsets > 0, readdir returns unlinked entries in results.
> The test does not fail on that behavior because the test
> only asserts that this is not allowed after seek to offset 0.
>
> Knowing the implementation of overlayfs readdir cache this is
> not surprising to me, but I wonder if this behavior is POSIX
> compliant, and if not, whether we should document it and/or
> add a failing test for it.
>

I think the outcome could be worse.
If a copied up entry is unlinked after populating the dir cache
but before ovl_cache_update_ino() then ovl_cache_update_ino()
and subsequently the getdents call will fail with ENOENT.

This test is not smart enough to cover this case (if it really exists).
I think we need to relax the case of negative lookup result in
ovl_cache_update_ino() and just set p->whiteout without and
continue with no error.

This can solve several test cases.
In principle, we can "semi-reset" the merge dir cache if the dir was
modified after every seek, not just seek to 0.
By "semi-reset" I mean use the list, but force ovl_cache_update_ino()
to all upper entries, similar to ovl_dir_read_impure().

OR.. we can just do that unconditionally in ovl_iterate().
The ovl dentry cache for the children will be populated after the first
ovl_iterate() anyway, so maybe the penalty is not so bad?

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] overlay: Test invalidate of readdir cache
  2021-04-21  9:33   ` Amir Goldstein
@ 2021-04-22  6:23     ` Amir Goldstein
  0 siblings, 0 replies; 11+ messages in thread
From: Amir Goldstein @ 2021-04-22  6:23 UTC (permalink / raw)
  To: Eryu Guan; +Cc: Miklos Szeredi, overlayfs, fstests

On Wed, Apr 21, 2021 at 12:33 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Apr 21, 2021 at 12:23 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > 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     | 105 ++++++++++++++++++++++++++++++++++++++++++
> >  tests/overlay/077.out |   2 +
> >  tests/overlay/group   |   1 +
> >  3 files changed, 108 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..e254aec1
> > --- /dev/null
> > +++ b/tests/overlay/077
> > @@ -0,0 +1,105 @@
> > +#! /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.*
> > +}
> > +
> > +# create test directory and test file, mount overlayfs and remove
> > +# testfile to create a whiteout in upper dir.
> > +create_whiteout()
> > +{
> > +       local lower=$1
> > +       local upper=$2
> > +       local work=$3
> > +       local file=$4
> > +
> > +       mkdir -p $lower/testdir
> > +       touch $lower/testdir/$file
> > +
> > +       _overlay_scratch_mount_dirs $lower $upper $work
> > +
> > +       rm -f $SCRATCH_MNT/testdir/$file
> > +
> > +       $UMOUNT_PROG $SCRATCH_MNT
> > +}
> > +
>
> Oops. Unused leftover.
> A former merge dir does not need to actually have whiteouts for this test...
>

But it would be better for test coverage if the merge dir had copied up
files on first readdir (making it "impure").

And I will also add the "upper impure" test case in V2 posting.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] Test overlayfs readdir cache
  2021-04-22  6:18 ` [PATCH 0/2] Test overlayfs " Amir Goldstein
@ 2021-04-22  7:53   ` Miklos Szeredi
  2021-04-22  8:47     ` Amir Goldstein
                       ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Miklos Szeredi @ 2021-04-22  7:53 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: overlayfs, fstests, Eryu Guan

On Thu, Apr 22, 2021 at 8:18 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Apr 21, 2021 at 12:23 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Eryu,
> >
> > This extends the generic t_dir_offset2 test to verify
> > some more test cases 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.
> >
> > Miklos,
> >
> > I had noticed in the test full logs that readdir of
> > a merged dir behaves strangely - when seeking backwards
> > to offsets > 0, readdir returns unlinked entries in results.
> > The test does not fail on that behavior because the test
> > only asserts that this is not allowed after seek to offset 0.
> >
> > Knowing the implementation of overlayfs readdir cache this is
> > not surprising to me, but I wonder if this behavior is POSIX
> > compliant, and if not, whether we should document it and/or
> > add a failing test for it.
> >
>
> I think the outcome could be worse.
> If a copied up entry is unlinked after populating the dir cache
> but before ovl_cache_update_ino() then ovl_cache_update_ino()
> and subsequently the getdents call will fail with ENOENT.
>
> This test is not smart enough to cover this case (if it really exists).
> I think we need to relax the case of negative lookup result in
> ovl_cache_update_ino() and just set p->whiteout without and
> continue with no error.
>
> This can solve several test cases.
> In principle, we can "semi-reset" the merge dir cache if the dir was
> modified after every seek, not just seek to 0.
> By "semi-reset" I mean use the list, but force ovl_cache_update_ino()
> to all upper entries, similar to ovl_dir_read_impure().
>
> OR.. we can just do that unconditionally in ovl_iterate().
> The ovl dentry cache for the children will be populated after the first
> ovl_iterate() anyway, so maybe the penalty is not so bad?

POSIX does allow stale readdir data (not just in case of non-zero seek):

"If a file is removed from or added to the directory after the most
recent call to opendir() or rewinddir(), whether a subsequent call to
readdir() returns an entry for that file is unspecified."

If you think about the way readdir(3) is implemented by the libc, this
is inevitable.

Returning ENOENT from readdir(3) is obviously a bug.

The merge case being not super high performance is perfectly okay.
The only thing I've worried about in that case is unbound memory use,
but apparently that hasn't been an issue in practice.

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] Test overlayfs readdir cache
  2021-04-22  7:53   ` Miklos Szeredi
@ 2021-04-22  8:47     ` Amir Goldstein
  2021-04-22  9:03       ` Miklos Szeredi
  2021-04-23 10:20     ` Amir Goldstein
  2021-04-23 19:03     ` Amir Goldstein
  2 siblings, 1 reply; 11+ messages in thread
From: Amir Goldstein @ 2021-04-22  8:47 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: overlayfs, fstests, Eryu Guan

On Thu, Apr 22, 2021 at 10:53 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, Apr 22, 2021 at 8:18 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Wed, Apr 21, 2021 at 12:23 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > Eryu,
> > >
> > > This extends the generic t_dir_offset2 test to verify
> > > some more test cases 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.
> > >
> > > Miklos,
> > >
> > > I had noticed in the test full logs that readdir of
> > > a merged dir behaves strangely - when seeking backwards
> > > to offsets > 0, readdir returns unlinked entries in results.
> > > The test does not fail on that behavior because the test
> > > only asserts that this is not allowed after seek to offset 0.
> > >
> > > Knowing the implementation of overlayfs readdir cache this is
> > > not surprising to me, but I wonder if this behavior is POSIX
> > > compliant, and if not, whether we should document it and/or
> > > add a failing test for it.
> > >
> >
> > I think the outcome could be worse.
> > If a copied up entry is unlinked after populating the dir cache
> > but before ovl_cache_update_ino() then ovl_cache_update_ino()
> > and subsequently the getdents call will fail with ENOENT.
> >
> > This test is not smart enough to cover this case (if it really exists).
> > I think we need to relax the case of negative lookup result in
> > ovl_cache_update_ino() and just set p->whiteout without and
> > continue with no error.
> >
> > This can solve several test cases.
> > In principle, we can "semi-reset" the merge dir cache if the dir was
> > modified after every seek, not just seek to 0.
> > By "semi-reset" I mean use the list, but force ovl_cache_update_ino()
> > to all upper entries, similar to ovl_dir_read_impure().
> >
> > OR.. we can just do that unconditionally in ovl_iterate().
> > The ovl dentry cache for the children will be populated after the first
> > ovl_iterate() anyway, so maybe the penalty is not so bad?
>
> POSIX does allow stale readdir data (not just in case of non-zero seek):
>
> "If a file is removed from or added to the directory after the most
> recent call to opendir() or rewinddir(), whether a subsequent call to
> readdir() returns an entry for that file is unspecified."
>
> If you think about the way readdir(3) is implemented by the libc, this
> is inevitable.

I see. In that case, I would defer merging this test as it assumes too much
about readdir behavior (even though applications may expect this behavior).

>
> Returning ENOENT from readdir(3) is obviously a bug.
>
> The merge case being not super high performance is perfectly okay.
> The only thing I've worried about in that case is unbound memory use,
> but apparently that hasn't been an issue in practice.
>

Okay, so I will try to reproduce the ENOENT and fix it.
In any case, even if the bug exists it's not urgent.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] Test overlayfs readdir cache
  2021-04-22  8:47     ` Amir Goldstein
@ 2021-04-22  9:03       ` Miklos Szeredi
  0 siblings, 0 replies; 11+ messages in thread
From: Miklos Szeredi @ 2021-04-22  9:03 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: overlayfs, fstests, Eryu Guan

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

On Thu, Apr 22, 2021 at 10:47 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Apr 22, 2021 at 10:53 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Thu, Apr 22, 2021 at 8:18 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Wed, Apr 21, 2021 at 12:23 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > Eryu,
> > > >
> > > > This extends the generic t_dir_offset2 test to verify
> > > > some more test cases 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.
> > > >
> > > > Miklos,
> > > >
> > > > I had noticed in the test full logs that readdir of
> > > > a merged dir behaves strangely - when seeking backwards
> > > > to offsets > 0, readdir returns unlinked entries in results.
> > > > The test does not fail on that behavior because the test
> > > > only asserts that this is not allowed after seek to offset 0.
> > > >
> > > > Knowing the implementation of overlayfs readdir cache this is
> > > > not surprising to me, but I wonder if this behavior is POSIX
> > > > compliant, and if not, whether we should document it and/or
> > > > add a failing test for it.
> > > >
> > >
> > > I think the outcome could be worse.
> > > If a copied up entry is unlinked after populating the dir cache
> > > but before ovl_cache_update_ino() then ovl_cache_update_ino()
> > > and subsequently the getdents call will fail with ENOENT.
> > >
> > > This test is not smart enough to cover this case (if it really exists).
> > > I think we need to relax the case of negative lookup result in
> > > ovl_cache_update_ino() and just set p->whiteout without and
> > > continue with no error.
> > >
> > > This can solve several test cases.
> > > In principle, we can "semi-reset" the merge dir cache if the dir was
> > > modified after every seek, not just seek to 0.
> > > By "semi-reset" I mean use the list, but force ovl_cache_update_ino()
> > > to all upper entries, similar to ovl_dir_read_impure().
> > >
> > > OR.. we can just do that unconditionally in ovl_iterate().
> > > The ovl dentry cache for the children will be populated after the first
> > > ovl_iterate() anyway, so maybe the penalty is not so bad?
> >
> > POSIX does allow stale readdir data (not just in case of non-zero seek):
> >
> > "If a file is removed from or added to the directory after the most
> > recent call to opendir() or rewinddir(), whether a subsequent call to
> > readdir() returns an entry for that file is unspecified."
> >
> > If you think about the way readdir(3) is implemented by the libc, this
> > is inevitable.
>
> I see. In that case, I would defer merging this test as it assumes too much
> about readdir behavior (even though applications may expect this behavior).

FWIW, I started writing a readdir stress/validator similar to
fsx-linux but for directories.

It's unfinished and  has performance problem as the directory grows
due to linear searches.

Putting it out in case someone wants to continue working on it, or
just take some ideas.

Thanks,
Miklos

[-- Attachment #2: readdir-stress.c --]
[-- Type: text/x-csrc, Size: 6084 bytes --]

/*
 * Copyright (C) 2021 Miklos Szeredi <miklos@szeredi.hu>
 *
 * This program can be distributed under the terms of the GNU GPLv2.
 */

#include <stdio.h>
#include <unistd.h>
#include <stdint.h>
#include <stdlib.h>
#include <fcntl.h>
#include <dirent.h>
#include <string.h>
#include <err.h>
#include <sys/stat.h>
#include <sys/syscall.h>

struct linux_dirent64 {
	uint64_t	d_ino;
	int64_t		d_off;
	unsigned short	d_reclen;
	unsigned char	d_type;
	char		d_name[0];
};

struct entry {
	char *name;
	unsigned char type;
	ino_t ino;
	int add_ver;
	int del_ver;
};

struct list_entry {
	char *name;
	uint64_t ino;
	int64_t off;
	unsigned char type;
	int version;

	struct list_entry *next;
};

struct dir {
	int fd;
	off_t pos;
	int version;
	struct list_entry *list;
};

static struct dir global_dir;

static int version = 1;
static unsigned int num = 0;
static unsigned int num_active = 0;
static struct entry *array = NULL;

static void *oom(void *ptr)
{
	if (!ptr)
		errx(1, "out of memory");
	return ptr;
}

static void add_entry(char *name, unsigned char type)
{
	struct stat st;
	int res;

	res = lstat(name, &st);
	if (res == -1)
		err(1, "failed to stat <%s>", name);

	array = oom(realloc(array, (num + 1) * sizeof(*array)));

	array[num].name = oom(strdup(name));

	array[num].type = type;
	array[num].ino = st.st_ino;
	version++;
	array[num].add_ver = version;
	array[num].del_ver = 0;
	num++;
	num_active++;
}

static int create_entry(void)
{
	unsigned char type;
	unsigned int namelen;
	unsigned int i;
	char name[256];
	mode_t mode;
	int res;

	type = (random() % 7) * 2;
	if (!type)
		type = 1;

	if (type == DT_CHR || type == DT_BLK)
		type = DT_REG;

	mode = type << 12 | 0666;

retry:
	namelen = (random() % 10) ? (random() % 32 + 1) : (random() % 222 + 33);
//	namelen = random() % 30 + 1;

	for (i = 0; i < namelen; i++) {
		unsigned char c;

		c = random() % 94 + 32;
		if (c >= '/')
			c++;
		name[i] = c;
	}
	name[namelen] = '\0';

	if (strcmp(name, ".") == 0 || strcmp(name, "..") == 0)
		goto retry;

	for (i = 0; i < num; i++) {
		if (strcmp(name, array[i].name) == 0)
			goto retry;
	}

	if (S_ISDIR(mode))
		res = mkdir(name, 0777);
	else if (S_ISLNK(mode))
		res = symlink("target", name);
	else
		res = mknod(name, mode, 1);

	if (res == -1)
		err(1, "failed to create \"%s\", mode 0%o", name, mode);

	add_entry(name, type);

	return 0;
}

static int remove_entry(void)
{
	unsigned int i;
	struct entry *x;
	int res;

	if (!num_active)
		return 0;
retry:
	i = random() % num;
	x = &array[i];
	if (x->del_ver)
		goto retry;

	if (x->type == DT_DIR)
		res = rmdir(x->name);
	else
		res = unlink(x->name);
	if (res == -1)
		err(1, "failed to remove \"%s\"", x->name);

	version++;
	x->del_ver = version;
	num_active--;

	return 0;
}

static void add_list_entry(struct dir *dir, struct list_entry *ent)
{
	struct list_entry **pp;

	for (pp = &dir->list; *pp != NULL; pp = &(*pp)->next) {
		struct list_entry *p = *pp;

		if (ent->off == p->off)
			errx(1, "two entries have the same offset: %li",
				ent->off);
		if (strcmp(ent->name, p->name) == 0)
			errx(1, "two entries have the same name");
	}
	*pp = ent;
}

static void check_entry(struct dir *dir, struct list_entry *ent)
{
	unsigned int i;

	if (strcmp(ent->name, ".") == 0 || strcmp(ent->name, "..") == 0)
		return;

	for (i = 0; i < num; i++) {
		struct entry *x = &array[i];

		if (strcmp(ent->name, x->name) == 0 &&
		    (!x->del_ver || x->del_ver > dir->version)) {
			return;
		}
	}
	errx(1, "unknown entry returned by getdents64() \"%s\"", ent->name);
}

static void check_complete(struct dir *dir)
{
	unsigned int i;

	for (i = 0; i < num; i++) {
		struct entry *x = &array[i];

		if (x->add_ver <= dir->version && !x->del_ver) {
			struct list_entry *p;

			for (p = dir->list; p; p = p->next) {
				if (strcmp(x->name, p->name) == 0)
					break;
			}
			if (!p) {
				errx(1, "\"%s\" not found in complete listing",
				     x->name);
			}
		}
	}
}

static int read_dir(struct dir *dir)
{
	char buf[65536];
	unsigned int size = random() % (sizeof(buf) - 512) + 512;
	int res;
	int nread;
	struct linux_dirent64 *d;
	struct list_entry *ent;
	int bpos;
	int version = version;

	res = syscall(SYS_getdents64, dir->fd, buf, size);
	if (res == -1)
		err(1, "getdents64(%i, %p, %u)", dir->fd, buf, size);
	nread = res;

	if (nread == 0) {
		check_complete(dir);
		return 0;
	}

	for (bpos = 0; bpos < nread; ) {
		d = (struct linux_dirent64 *) (buf + bpos);

		ent = oom(calloc(1, sizeof(*ent)));
		ent->name = oom(strdup(d->d_name));
		ent->ino = d->d_ino;
		ent->type = d->d_type;
		ent->off = dir->pos;
		ent->version = version;
		dir->pos = d->d_off;

		check_entry(dir, ent);

		add_list_entry(dir, ent);

		bpos += d->d_reclen;
	}
	if (bpos != nread)
		errx(1, "invalid readdir length");

	return 0;
}

static int seek_dir(struct dir *dir)
{
	off_t res;
	struct list_entry *p, *next;

	dir->version = version;

	res = lseek(dir->fd, 0, SEEK_SET);
	if (res == (off_t) -1)
		err(1, "rewind");

	for (p = dir->list; p; p = next) {
		next = p->next;
		free(p->name);
		free(p);
	}
	dir->list = NULL;
	dir->pos = 0;

	return 0;
}

static int one(void)
{
	switch (random() % 10) {
	case 0:
	case 1:
		return create_entry();
	case 2:
		return remove_entry();
	case 3:
	case 4:
	case 5:
	case 6:
	case 7:
	case 8:
		return read_dir(&global_dir);
	case 9:
		return seek_dir(&global_dir);
	}
	errx(1, "unreachable");
}

static int run(unsigned int iters)
{
	unsigned int i;
	int res = 0;

	srandom(1);

	global_dir.version = version;
	global_dir.fd = open(".", O_RDONLY | O_DIRECTORY);
	if (global_dir.fd == -1)
		err(1, "failed to open \".\"");

	for (i = 0; i < iters; i++) {
		if (i % 100 == 1)
			printf("[%i] total entries: %u active entries: %u\n",
			       i, num, num_active);
		res = one();
		if (res)
			break;
	}

	close(global_dir.fd);

	return res;
}

int main(int argc, char *argv[])
{
	char *testdir = argv[1];
	int res;

	if (argc < 2)
		errx(1, "usage: %s test_directory", argv[0]);

	res = chdir(testdir);
	if (res == -1)
		err(1, "chdir(%s)", testdir);


	return run(1000000);
}

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] Test overlayfs readdir cache
  2021-04-22  7:53   ` Miklos Szeredi
  2021-04-22  8:47     ` Amir Goldstein
@ 2021-04-23 10:20     ` Amir Goldstein
  2021-04-23 19:03     ` Amir Goldstein
  2 siblings, 0 replies; 11+ messages in thread
From: Amir Goldstein @ 2021-04-23 10:20 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: overlayfs, fstests, Eryu Guan

On Thu, Apr 22, 2021 at 10:53 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, Apr 22, 2021 at 8:18 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Wed, Apr 21, 2021 at 12:23 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > Eryu,
> > >
> > > This extends the generic t_dir_offset2 test to verify
> > > some more test cases 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.
> > >
> > > Miklos,
> > >
> > > I had noticed in the test full logs that readdir of
> > > a merged dir behaves strangely - when seeking backwards
> > > to offsets > 0, readdir returns unlinked entries in results.
> > > The test does not fail on that behavior because the test
> > > only asserts that this is not allowed after seek to offset 0.
> > >
> > > Knowing the implementation of overlayfs readdir cache this is
> > > not surprising to me, but I wonder if this behavior is POSIX
> > > compliant, and if not, whether we should document it and/or
> > > add a failing test for it.
> > >
> >
> > I think the outcome could be worse.
> > If a copied up entry is unlinked after populating the dir cache
> > but before ovl_cache_update_ino() then ovl_cache_update_ino()
> > and subsequently the getdents call will fail with ENOENT.
> >
> > This test is not smart enough to cover this case (if it really exists).
> > I think we need to relax the case of negative lookup result in
> > ovl_cache_update_ino() and just set p->whiteout without and
> > continue with no error.
> >
> > This can solve several test cases.
> > In principle, we can "semi-reset" the merge dir cache if the dir was
> > modified after every seek, not just seek to 0.
> > By "semi-reset" I mean use the list, but force ovl_cache_update_ino()
> > to all upper entries, similar to ovl_dir_read_impure().
> >
> > OR.. we can just do that unconditionally in ovl_iterate().
> > The ovl dentry cache for the children will be populated after the first
> > ovl_iterate() anyway, so maybe the penalty is not so bad?
>
> POSIX does allow stale readdir data (not just in case of non-zero seek):
>
> "If a file is removed from or added to the directory after the most
> recent call to opendir() or rewinddir(), whether a subsequent call to
> readdir() returns an entry for that file is unspecified."
>
> If you think about the way readdir(3) is implemented by the libc, this
> is inevitable.
>

That makes the test I posted wrong, because it expects the
dir modifications to be visible after seek to 0.

The thing is, unlike readdir(3) implementation, overlayfs keeps the
readdir cache on the inode, so by keeping the original fd open
and opening a new fd, I could reproduce stale and missing entries
for a new opendir, which is definitely a bug.

Will post the updated test.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] Test overlayfs readdir cache
  2021-04-22  7:53   ` Miklos Szeredi
  2021-04-22  8:47     ` Amir Goldstein
  2021-04-23 10:20     ` Amir Goldstein
@ 2021-04-23 19:03     ` Amir Goldstein
  2 siblings, 0 replies; 11+ messages in thread
From: Amir Goldstein @ 2021-04-23 19:03 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: overlayfs, fstests, Eryu Guan

On Thu, Apr 22, 2021 at 10:53 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, Apr 22, 2021 at 8:18 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Wed, Apr 21, 2021 at 12:23 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > Eryu,
> > >
> > > This extends the generic t_dir_offset2 test to verify
> > > some more test cases 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.
> > >
> > > Miklos,
> > >
> > > I had noticed in the test full logs that readdir of
> > > a merged dir behaves strangely - when seeking backwards
> > > to offsets > 0, readdir returns unlinked entries in results.
> > > The test does not fail on that behavior because the test
> > > only asserts that this is not allowed after seek to offset 0.
> > >
> > > Knowing the implementation of overlayfs readdir cache this is
> > > not surprising to me, but I wonder if this behavior is POSIX
> > > compliant, and if not, whether we should document it and/or
> > > add a failing test for it.
> > >
> >
> > I think the outcome could be worse.
> > If a copied up entry is unlinked after populating the dir cache
> > but before ovl_cache_update_ino() then ovl_cache_update_ino()
> > and subsequently the getdents call will fail with ENOENT.
> >
> > This test is not smart enough to cover this case (if it really exists).
> > I think we need to relax the case of negative lookup result in
> > ovl_cache_update_ino() and just set p->whiteout without and
> > continue with no error.
> >
> > This can solve several test cases.
> > In principle, we can "semi-reset" the merge dir cache if the dir was
> > modified after every seek, not just seek to 0.
> > By "semi-reset" I mean use the list, but force ovl_cache_update_ino()
> > to all upper entries, similar to ovl_dir_read_impure().
> >
> > OR.. we can just do that unconditionally in ovl_iterate().
> > The ovl dentry cache for the children will be populated after the first
> > ovl_iterate() anyway, so maybe the penalty is not so bad?
>
> POSIX does allow stale readdir data (not just in case of non-zero seek):
>
> "If a file is removed from or added to the directory after the most
> recent call to opendir() or rewinddir(), whether a subsequent call to
> readdir() returns an entry for that file is unspecified."
>
> If you think about the way readdir(3) is implemented by the libc, this
> is inevitable.
>
> Returning ENOENT from readdir(3) is obviously a bug.

There is no ENOENT bug. I read the code in ovl_cache_update_ino()
wrong, unless lookup_one_len() returns PTR_ERR(-ENOENT) instead
of a negative dentry and I never understood if this ever happens, so
the most we need to do beyond the fix already in overlayfs-next in
to check that -ENOENT case.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-04-23 19:04 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-21  9:23 [PATCH 0/2] Test overlayfs readdir cache Amir Goldstein
2021-04-21  9:23 ` [PATCH 1/2] generic: Test readdir of modified directrory Amir Goldstein
2021-04-21  9:23 ` [PATCH 2/2] overlay: Test invalidate of readdir cache Amir Goldstein
2021-04-21  9:33   ` Amir Goldstein
2021-04-22  6:23     ` Amir Goldstein
2021-04-22  6:18 ` [PATCH 0/2] Test overlayfs " Amir Goldstein
2021-04-22  7:53   ` Miklos Szeredi
2021-04-22  8:47     ` Amir Goldstein
2021-04-22  9:03       ` Miklos Szeredi
2021-04-23 10:20     ` Amir Goldstein
2021-04-23 19:03     ` Amir Goldstein

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).