nfs: fix redundant readdir request after get eof

Commit Message

Kinglong Mee July 13, 2023, 2:52 p.m. UTC

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 <[e-mail]>
---
 fs/nfs/dir.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Benjamin Coddington July 13, 2023, 7:17 p.m. UTC | #1

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

Benjamin Coddington July 14, 2023, 3:19 p.m. UTC | #2

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

Kinglong Mee July 15, 2023, 12:06 p.m. UTC | #3

On Fri, Jul 14, 2023 at 11:19 PM Benjamin Coddington
<[e-mail]> 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

Patch

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;