* [PATCH v2 1/5] src/t_dir_offset2: Add an option to limit of buffer size
2021-04-25 7:14 [PATCH v2 0/5] Test overlayfs readdir cache Amir Goldstein
@ 2021-04-25 7:14 ` Amir Goldstein
2021-04-25 7:14 ` [PATCH v2 2/5] src/t_dir_offset2: Add an option to find file by name Amir Goldstein
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Amir Goldstein @ 2021-04-25 7:14 UTC (permalink / raw)
To: Eryu Guan; +Cc: Miklos Szeredi, linux-unionfs, fstests
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 2/5] src/t_dir_offset2: Add an option to find file by name
2021-04-25 7:14 [PATCH v2 0/5] Test overlayfs readdir cache Amir Goldstein
2021-04-25 7:14 ` [PATCH v2 1/5] src/t_dir_offset2: Add an option to limit of buffer size Amir Goldstein
@ 2021-04-25 7:14 ` Amir Goldstein
2021-04-25 7:14 ` [PATCH v2 3/5] src/t_dir_offset2: Add option to create or unlink file Amir Goldstein
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Amir Goldstein @ 2021-04-25 7:14 UTC (permalink / raw)
To: Eryu Guan; +Cc: Miklos Szeredi, linux-unionfs, fstests
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 3/5] src/t_dir_offset2: Add option to create or unlink file
2021-04-25 7:14 [PATCH v2 0/5] Test overlayfs readdir cache Amir Goldstein
2021-04-25 7:14 ` [PATCH v2 1/5] src/t_dir_offset2: Add an option to limit of buffer size Amir Goldstein
2021-04-25 7:14 ` [PATCH v2 2/5] src/t_dir_offset2: Add an option to find file by name Amir Goldstein
@ 2021-04-25 7:14 ` Amir Goldstein
2021-04-25 7:14 ` [PATCH v2 4/5] generic: Test readdir of modified directrory Amir Goldstein
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Amir Goldstein @ 2021-04-25 7:14 UTC (permalink / raw)
To: Eryu Guan; +Cc: Miklos Szeredi, linux-unionfs, fstests
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 4/5] generic: Test readdir of modified directrory
2021-04-25 7:14 [PATCH v2 0/5] Test overlayfs readdir cache Amir Goldstein
` (2 preceding siblings ...)
2021-04-25 7:14 ` [PATCH v2 3/5] src/t_dir_offset2: Add option to create or unlink file Amir Goldstein
@ 2021-04-25 7:14 ` Amir Goldstein
2021-04-25 7:14 ` [PATCH v2 5/5] overlay: Test invalidate of readdir cache Amir Goldstein
2021-04-26 10:07 ` [PATCH v2 0/5] Test overlayfs " Miklos Szeredi
5 siblings, 0 replies; 10+ messages in thread
From: Amir Goldstein @ 2021-04-25 7:14 UTC (permalink / raw)
To: Eryu Guan; +Cc: Miklos Szeredi, linux-unionfs, fstests
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 5/5] overlay: Test invalidate of readdir cache
2021-04-25 7:14 [PATCH v2 0/5] Test overlayfs readdir cache Amir Goldstein
` (3 preceding siblings ...)
2021-04-25 7:14 ` [PATCH v2 4/5] generic: Test readdir of modified directrory Amir Goldstein
@ 2021-04-25 7:14 ` Amir Goldstein
2021-04-26 10:07 ` [PATCH v2 0/5] Test overlayfs " Miklos Szeredi
5 siblings, 0 replies; 10+ messages in thread
From: Amir Goldstein @ 2021-04-25 7:14 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 | 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 0/5] Test overlayfs readdir cache
2021-04-25 7:14 [PATCH v2 0/5] Test overlayfs readdir cache Amir Goldstein
` (4 preceding siblings ...)
2021-04-25 7:14 ` [PATCH v2 5/5] overlay: Test invalidate of readdir cache Amir Goldstein
@ 2021-04-26 10:07 ` Miklos Szeredi
2021-04-26 10:15 ` Amir Goldstein
5 siblings, 1 reply; 10+ messages in thread
From: Miklos Szeredi @ 2021-04-26 10:07 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Eryu Guan, overlayfs, fstests
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 0/5] Test overlayfs readdir cache
2021-04-26 10:07 ` [PATCH v2 0/5] Test overlayfs " Miklos Szeredi
@ 2021-04-26 10:15 ` Amir Goldstein
2021-04-26 13:12 ` Miklos Szeredi
0 siblings, 1 reply; 10+ messages in thread
From: Amir Goldstein @ 2021-04-26 10:15 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Eryu Guan, overlayfs, fstests
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/5] Test overlayfs readdir cache
2021-04-26 10:15 ` Amir Goldstein
@ 2021-04-26 13:12 ` Miklos Szeredi
2021-04-26 15:22 ` Amir Goldstein
0 siblings, 1 reply; 10+ messages in thread
From: Miklos Szeredi @ 2021-04-26 13:12 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Eryu Guan, overlayfs, fstests
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/5] Test overlayfs readdir cache
2021-04-26 13:12 ` Miklos Szeredi
@ 2021-04-26 15:22 ` Amir Goldstein
0 siblings, 0 replies; 10+ messages in thread
From: Amir Goldstein @ 2021-04-26 15:22 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Eryu Guan, overlayfs, fstests
[-- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread