Re: [PATCH v5 00/22] Readdir enhancements

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

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