Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Benjamin Coddington" <bcodding@redhat.com>
To:     "Anna Schumaker" <schumaker.anna@gmail.com>
Cc:     "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v1 07/10] NFS: Support headless readdir pagecache pages
Date:   Thu, 21 Jan 2021 12:57:12 -0500
Message-ID: <4DE5379B-E52D-4C94-8701-29FEFCE8C438@redhat.com>
In-Reply-To: <CAFX2Jf=Kg+fbxSfgJ_Kzxe6LerQ8RcZu_8AYp2JFF4THDfy8fQ@mail.gmail.com>
References: <cover.1611160120.git.bcodding@redhat.com>
 <7394b4d348d0c92b64cd0fb4fbf74bfa6e676d24.1611160121.git.bcodding@redhat.com>
 <CAFX2Jf=Kg+fbxSfgJ_Kzxe6LerQ8RcZu_8AYp2JFF4THDfy8fQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; format=flowed
Precedence: bulk

On 21 Jan 2021, at 12:24, Anna Schumaker wrote:

> Hi Ben,
>
> On Wed, Jan 20, 2021 at 12:04 PM Benjamin Coddington
> <bcodding@redhat.com> wrote:
>>
>> It is now possible that a reader will resume a directory listing 
>> after an
>> invalidation and fill the rest of the pages with the offset left over 
>> from
>> the last partially-filled page.  These pages will then be recycled 
>> and
>> refilled by the next reader since their alignment is incorrect.
>>
>> Add an index to the nfs_cache_array that will indicate where the next 
>> entry
>> should be filled.  This allows partially-filled pages to have the 
>> best
>> alignment possible.  They are more likely to be useful to readers 
>> that
>> follow.
>>
>> This optimization targets the case when there are multiple processes
>> listing the directory simultaneously.  Often the processes will 
>> collect and
>> block on the same page waiting for a READDIR call to fill the 
>> pagecache.
>> If the pagecache is invalidated, a partially-filled page will usually
>> result.  This partially-filled page can immediately be used by all
>> processes to emit entries rather than having to discard and refill it 
>> for
>> every process.
>>
>> The addition of another integer to struct nfs_cache_array increases 
>> its
>> size to 24 bytes. We do not lose the original capacity of 127 entries 
>> per
>> page.
>
> This patch causes cthon basic test #6 to start failing with unexpected
> dir entries across all NFS versions:
>
>   ./test6: readdir
>   basic tests failed
>       ./test6: (/mnt/test/anna/Connectathon/cthon04) unexpected dir 
> entry 'h'

Ah, yes -- because we /must/ revalidate before using any cached pages.  
I
think this test unlinks files and that would be a breakage.

> Luckily, the next patch seems to resolve this issue. Could they maybe
> be squashed together?

Yes, they should be squashed - I'll do that in another pass.  I'd like 
to
get rid of uncached_readdir() altogether and then send another version.
Thanks for the test!

Ben