Re: [PATCH 4/5] NFS: Further optimisations for 'ls -l'

["Benjamin Coddington" <bcodding@redhat.com>]

On 29 Sep 2021, at 12:23, Trond Myklebust wrote:

> On Wed, 2021-09-29 at 10:56 -0400, Benjamin Coddington wrote:
>> On 29 Sep 2021, at 9:49, trondmy@kernel.org wrote:
>>
>>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>>
>>> If a user is doing 'ls -l', we have a heuristic in GETATTR that
>>> tells
>>> the readdir code to try to use READDIRPLUS in order to refresh the
>>> inode
>>> attributes. In certain cirumstances, we also try to invalidate the
>>> remaining directory entries in order to ensure this refresh.
>>>
>>> If there are multiple readers of the directory, we probably should
>>> avoid
>>> invalidating the page cache, since the heuristic breaks down in
>>> that
>>> situation anyway.
>>
>> Hi Trond, I'm curious about the motivation for this work because
>> we're often
>> managing expectations about performance between various workloads. 
>> What
>> does the workload look like that prompted you to make this
>> optimization?
>>
>> I'm also interested because I have some work that improves readdir
>> performance for multiple readers on large directories, but is rotting
>> because things have been "good enough" for folks over here.
>>
>
> Are you talking about this patch specifically, or the series in
> general?

A bit of both - the first two patches didn't really make sense to me, but I
get it now.  Thanks.

> The general motivation for the series is that in certain circumstances
> involving a read-only scenario (no changes to the directory) and
> multiple processes we're seeing a regression w.r.t. older kernels.
>
> The motivation for this particular patch is twofold:
>    1. I want to get rid of the cached page_index in the inode and
>       replace it with something less persistent (inodes are forever,
>       unlike file descriptors).
>    2. The heuristic in question is designed to only work in the single
>       process/single threaded case.

Make sense to me now.

I'm wondering if this might be creating a READDIR op amplification for N
readers sharing the directory's fd because one process can be dropping the
cache, which artificially deflates desc->page_index for a given dir_cookie.
That lower page_index gets reused on the next pass dropping the cache, and
it'll keep using the bottom pages and doing READDIRs.  That wasn't possible
before because we only invalidated past nfsi->page_index.

Maybe an unlikely case (but I think some http servers could behave this
way), and I'd have to test it to be sure.  I can try to do that unless you
think its not possible or concerning.

Ben