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:     trondmy@kernel.org
Cc:     linux-nfs@vger.kernel.org
Subject: Re: [PATCH v9 00/27] Readdir improvements
Date:   Wed, 09 Mar 2022 16:32:13 -0500
Message-ID: <23255C00-57BD-49D5-B228-F4D2C0AFDB32@redhat.com>
In-Reply-To: <20220227231227.9038-1-trondmy@kernel.org>
References: <20220227231227.9038-1-trondmy@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On 27 Feb 2022, at 18:12, trondmy@kernel.org wrote:

> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> The current NFS readdir code will always try to maximise the amount of
> readahead it performs on the assumption that we can cache anything that
> isn't immediately read by the process.
> There are several cases where this assumption breaks down, including
> when the 'ls -l' heuristic kicks in to try to force use of readdirplus
> as a batch replacement for lookup/getattr.
>
> This series also implement Ben's page cache filter to ensure that we can
> improve the ability to share cached data between processes that are
> reading the same directory at the same time, and to avoid live-locks
> when the directory is simultaneously changing.
>
> --
> v2: Remove reset of dtsize when NFS_INO_FORCE_READDIR is set
> v3: Avoid excessive window shrinking in uncached_readdir case
> v4: Track 'ls -l' cache hit/miss statistics
>     Improved algorithm for falling back to uncached readdir
>     Skip readdirplus when files are being written to
> v5: bugfixes
>     Skip readdirplus when the acdirmax/acregmax values are low
>     Request a full XDR buffer when doing READDIRPLUS
> v6: Add tracing
>     Don't have lookup request readdirplus when it won't help
> v7: Implement Ben's page cache filter
>     Reduce the use of uncached readdir
>     Change indexing of the page cache to improve seekdir() performance.
> v8: Reduce the page cache overhead by shrinking the cookie hashvalue size
>     Incorporate other feedback from Anna, Ben and Neil
>     Fix nfs2/3_decode_dirent return values
>     Fix the change attribute value set in nfs_readdir_page_get_next()
> v9: Address bugs that were hit when testing with large directories
>     Misc cleanups

Hi Trond, thanks for all this work!  I went through these from your testing
branch (612896ec5a4e) rather than the posting.

If it pleases, these can all get marked with my

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>
and/or
Tested-by: Benjamin Coddington <bcodding@redhat.com>

.. except for 25/27, which is missing from the testing branch.

As I replied to 23/27, I don't understand how the page index hashing is
going to help out the re-export seekdir case, I think it might make it
worse, and I think its unnecessary extra complication.

I did test extensively total directory listing correctness, and it appears
to me that you are correct, we are not regressing.  We're in a similar place
as before.  I think we can be even more correct with directory verifiers or
post-op updates with GETATTR in the READDIR compound for very little cost,
but I've already made those arguments a few weeks ago.

Thanks again,
Ben