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:     "Trond Myklebust" <trondmy@hammerspace.com>
Cc:     dwysocha@redhat.com, linux-nfs@vger.kernel.org
Subject: Re: [PATCH v5 00/22] Readdir enhancements
Date:   Thu, 12 Nov 2020 13:39:01 -0500
Message-ID: <DC49EB88-4B79-40FC-97C3-47D2A588F96C@redhat.com>
In-Reply-To: <6b07ff95824f5b46237fa07f5f72d8261d764007.camel@hammerspace.com>
References: <20201110213741.860745-1-trondmy@kernel.org>
 <CALF+zOkdXMDZ3TNGSNJQPtxy-ru_4iCYTz3U2uwkPAo3j55FZg@mail.gmail.com>
 <CALF+zO=-Si+CcEJvgzaYAjd2j8APV=4Xwm=FJibhuJRV+zWE5Q@mail.gmail.com>
 <723ef5d47994e34804f5514b06940e96620e2b70.camel@hammerspace.com>
 <6b07ff95824f5b46237fa07f5f72d8261d764007.camel@hammerspace.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


On 12 Nov 2020, at 13:26, Trond Myklebust wrote:

> On Thu, 2020-11-12 at 16:51 +0000, Trond Myklebust wrote:
>>
>> I was going to ask you if perhaps reverting Scott's commit
>> 07b5ce8ef2d8
>> ("NFS: Make nfs_readdir revalidate less often") might help here?
>> My thinking is that will trigger more cache invalidations when the
>> directory is changing underneath us, and will now trigger uncached
>> readdir in those situations.
>>
>>>
>
> IOW, the suggestion would be to apply something like the following on
> top of the existing readdir patchset:

I'm all for this approach, but - I'm rarely seeing the mapping->nrpages == 0
since the cache is dropped by a process in nfs_readdir() that immediately
starts filling the cache again.

It would make a lot more sense to me if we could do something like stash
desc->page_index << PAGE_SHIFT in f_pos after each nfs_readdir, then the
hueristic could check f_pos >> PAGE_SHIFT > nrpages.

Yes, f_pos is growing many different meanings in NFS directories, maybe we
can stash it on the directory context.

Ben

>
> ---
>  fs/nfs/dir.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 3f70697729d8..384a4663f742 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -956,10 +956,10 @@ static int readdir_search_pagecache(struct
> nfs_readdir_descriptor *desc)
>  {
>  	int res;
>
> -	if (nfs_readdir_dont_search_cache(desc))
> -		return -EBADCOOKIE;
> -
>  	do {
> +		if (nfs_readdir_dont_search_cache(desc))
> +			return -EBADCOOKIE;
> +
>  		if (desc->page_index == 0) {
>  			desc->current_index = 0;
>  			desc->prev_index = 0;
> @@ -1082,11 +1082,9 @@ static int nfs_readdir(struct file *file, struct
> dir_context *ctx)
>  	 * to either find the entry with the appropriate number or
>  	 * revalidate the cookie.
>  	 */
> -	if (ctx->pos == 0 || nfs_attribute_cache_expired(inode)) {
> -		res = nfs_revalidate_mapping(inode, file->f_mapping);
> -		if (res < 0)
> -			goto out;
> -	}
> +	res = nfs_revalidate_mapping(inode, file->f_mapping);
> +	if (res < 0)
> +		goto out;
>
>  	res = -ENOMEM;
>  	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
>
>
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com