When a directory contains 18 files (includes . and ..), nfs client sends
a redundant readdir request after get eof.
A simple reproduce,
At NFS server, create a directory with 18 files under exported directory.
# mkdir test
# cd test
# for i in {0..16} ; do touch $i; done
At NFS client, no matter mounting through nfsv3 or nfsv4,
does ls (or ll) at the created test directory.
A tshark output likes following,
# tshark -i eth0 tcp port 2049 -Tfields -e ip.src -e ip.dst -e nfs -e nfs.cookie4
srcip dstip SEQUENCE, PUTFH, READDIR 0
dstip srcip SEQUENCE PUTFH READDIR 909539109313539306,2108391201987888856,2305312124304486544,2566335452463141496,2978225129081509984,4263037479923412583,4304697173036510679,4666703455469210097,4759208201298769007,4776701232145978803,5338408478512081262,5949498658935544804,5971526429894832903,6294060338267709855,6528840566229532529,8600463293536422524,9223372036854775807
srcip dstip
srcip dstip SEQUENCE, PUTFH, READDIR 9223372036854775807
dstip srcip SEQUENCE PUTFH READDIR
The READDIR with cookie 9223372036854775807(0x7FFFFFFFFFFFFFFF) is redundant.
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfs/dir.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 8f3112e71a6a..0f944b246278 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1089,6 +1089,11 @@ static void nfs_do_filldir(struct nfs_readdir_descriptor *desc,
for (i = desc->cache_entry_index; i < array->size; i++) {
struct nfs_cache_array_entry *ent;
+ if (first_emit && i > NFS_READDIR_CACHE_MISS_THRESHOLD + 1) {
+ desc->eob = true;
+ break;
+ }
+
ent = &array->array[i];
if (!dir_emit(desc->ctx, ent->name, ent->name_len,
nfs_compat_user_ino64(ent->ino), ent->d_type)) {
@@ -1107,10 +1112,6 @@ static void nfs_do_filldir(struct nfs_readdir_descriptor *desc,
desc->ctx->pos = desc->dir_cookie;
else
desc->ctx->pos++;
- if (first_emit && i > NFS_READDIR_CACHE_MISS_THRESHOLD + 1) {
- desc->eob = true;
- break;
- }
}
if (array->folio_is_eof)
desc->eof = !desc->eob;
--
2.41.0
On 13 Jul 2023, at 10:52, Kinglong Mee wrote:
> When a directory contains 18 files (includes . and ..), nfs client sends
> a redundant readdir request after get eof.
This breaks the optimization in
85aa8ddc3818 NFS: Trigger the "ls -l" readdir heuristic sooner
The way to see that breakage happing is to "ls -l" a directory with more
than 16 dentries, and then when you do a 2nd "ls -l" you'll see that the NFS
client does a GETATTR for every single dentry instead of just the first 16
and then user READDIRPLUS for the rest.
I think what's going wrong with Kinglong's case is that when
array->folio_is_eof, we set desc->eof to the negation of desc->eob. That
does the wrong thing for directories with 18 dentries.
Here's a way around it, but I hate how ugly it is just for this single case:
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 8f3112e71a6a..ace454da9d4d 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1107,14 +1107,20 @@ static void nfs_do_filldir(struct nfs_readdir_descriptor *desc,
desc->ctx->pos = desc->dir_cookie;
else
desc->ctx->pos++;
+
if (first_emit && i > NFS_READDIR_CACHE_MISS_THRESHOLD + 1) {
desc->eob = true;
- break;
+ /* handle the case where there are NFS_READDIR_CACHE_MISS_THRESHOLD + 2
+ * entries: we also need to set desc->eof */
+ if (array->folio_is_eof && i == array->size - 1)
+ desc->eof = true;
+ goto done;
}
}
if (array->folio_is_eof)
desc->eof = !desc->eob;
+done:
kunmap_local(array);
dfprintk(DIRCACHE, "NFS: nfs_do_filldir() filling ended @ cookie %llu\n",
(unsigned long long)desc->dir_cookie);
Ben
On 13 Jul 2023, at 23:07, Kinglong Mee wrote:
> Hi Ben,
>
...
> Comparing with the above one, this seems work.
This fixes it for me and keeps the optimization. Its quite a subtle bit of
logic - maybe a comment is appropriate? One non-intuitive thing here is
that array->size == 19 for a directory with 18 entries, since we count the
"eof" entry as a blank entry instead of the last real entry.
Ben
On Fri, Jul 14, 2023 at 11:19 PM Benjamin Coddington
<[email protected]> wrote:
>
> On 13 Jul 2023, at 23:07, Kinglong Mee wrote:
>
> > Hi Ben,
> >
> ...
> > Comparing with the above one, this seems work.
>
> This fixes it for me and keeps the optimization. Its quite a subtle bit of
> logic - maybe a comment is appropriate?
Thanks for your testing.
I will send a new patch with a comment.
>
> One non-intuitive thing here is
> that array->size == 19 for a directory with 18 entries, since we count the
> "eof" entry as a blank entry instead of the last real entry.
No.
This is not a blank entry, every entry is a real one.
For the first emit, only returns 18 entries to the caller,
the next emit will return the 19th one.
thanks,
Kinglong Mee