Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Benjamin Coddington <bcodding@redhat.com>
To:     Kinglong Mee <kinglongmee@gmail.com>
Cc:     Anna Schumaker <anna@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Trond Myklebust <trondmy@hammerspace.com>
Subject: Re: [PATCH] nfs: fix redundant readdir request after get eof
Date:   Thu, 13 Jul 2023 15:17:07 -0400
Message-ID: <5E28252D-6EA1-4DD9-A5B3-957E13589982@redhat.com>
In-Reply-To: <8d6d9329-f5f1-2f15-f578-e4f8010b9b02@gmail.com>
References: <8d6d9329-f5f1-2f15-f578-e4f8010b9b02@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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