Re: [PATCH v5 00/22] Readdir enhancements

["Mkrtchyan, Tigran" <tigran.mkrtchyan@desy.de>]

Hi Trond,

I can confirm that with latest patchset we see 3x improvement
on directories listings with 100K and 500K files.

The downside is that our users will start to create directories
with even more files :). This weeks record 2.5M files at
200 files-per-second rate....

Thanks again,
   Tigran.

----- Original Message -----
> From: "Benjamin Coddington" <bcodding@redhat.com>
> To: "trondmy" <trondmy@hammerspace.com>
> Cc: "linux-nfs" <linux-nfs@vger.kernel.org>, "David Wysochanski" <dwysocha@redhat.com>
> Sent: Thursday, 12 November, 2020 21:23:32
> Subject: Re: [PATCH v5 00/22] Readdir enhancements

> On 12 Nov 2020, at 14:09, Benjamin Coddington wrote:
> 
>> On 12 Nov 2020, at 14:04, Trond Myklebust wrote:
>>
>>> On Thu, 2020-11-12 at 13:39 -0500, Benjamin Coddington wrote:
>>>>
>>>>
>>>> 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.
>>>
>>> That's why I moved the check in readdir_search_pagecache. Unless that
>>> process has set desc->dir_cookie == 0, then that should prevent the
>>> refilling.
>>
>> My pathological benchmarking does send another process in with
>> desc->dir_cookie == 0.
>>
>> I'm doing fork(), while(getdents) every second with acdirmin=1 and a
>> listing
>> that takes longer than 1 second to complete uncached, while touching
>> the
>> directory every second.
> 
> 
> As long as I use a buffer > 16k for getdents(), this hack can keep up
> with the testcase:
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index d7a9efd31ecd..ae687662112a 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -78,6 +78,7 @@ static struct nfs_open_dir_context
> *alloc_nfs_open_dir_context(struct inode *dir
>                 ctx->attr_gencount = nfsi->attr_gencount;
>                 ctx->dir_cookie = 0;
>                 ctx->dup_cookie = 0;
> +               ctx->page_index = 0;
>                 spin_lock(&dir->i_lock);
>                 if (list_empty(&nfsi->open_files) &&
>                     (nfsi->cache_validity & NFS_INO_DATA_INVAL_DEFER))
> @@ -150,6 +151,7 @@ struct nfs_readdir_descriptor {
>         struct page     *page;
>         struct dir_context *ctx;
>         pgoff_t         page_index;
> +       pgoff_t         last_page_index;
>         u64             dir_cookie;
>         u64             last_cookie;
>         u64             dup_cookie;
> @@ -928,7 +930,12 @@ static bool nfs_readdir_dont_search_cache(struct
> nfs_readdir_descriptor *desc)
>          * Default to uncached readdir if the page cache is empty, and
>          * we're looking for a non-zero cookie in a large directory.
>          */
> -       return desc->dir_cookie != 0 && mapping->nrpages == 0 && size >
> dtsize;
> +       if (desc->dir_cookie == 0)
> +               return false;
> +       else if (mapping->nrpages < desc->last_page_index && size >
> dtsize)
> +               return true;
> +
> +       return false;
>  }
> 
>  /* Search for desc->dir_cookie from the beginning of the page cache */
> @@ -936,10 +943,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;
> @@ -1062,11 +1069,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);
> @@ -1082,6 +1087,7 @@ static int nfs_readdir(struct file *file, struct
> dir_context *ctx)
>         desc->duped = dir_ctx->duped;
>         desc->attr_gencount = dir_ctx->attr_gencount;
>         memcpy(desc->verf, dir_ctx->verf, sizeof(desc->verf));
> +       desc->last_page_index = dir_ctx->page_index;
>         spin_unlock(&file->f_lock);
> 
>         do {
> @@ -1121,6 +1127,8 @@ static int nfs_readdir(struct file *file, struct
> dir_context *ctx)
>         dir_ctx->duped = desc->duped;
>         dir_ctx->attr_gencount = desc->attr_gencount;
>         memcpy(dir_ctx->verf, desc->verf, sizeof(dir_ctx->verf));
> +       if (desc->page_index > dir_ctx->page_index)
> +               dir_ctx->page_index = desc->page_index;
>         spin_unlock(&file->f_lock);
> 
>         kfree(desc);
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index 681ed98e4ba8..ad1f8e9a22e6 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -98,6 +98,7 @@ struct nfs_open_dir_context {
>         __u64 dir_cookie;
>         __u64 dup_cookie;
>         signed char duped;
> +       pgoff_t page_index;
>  };
> 
>  /*
> 
> 
> 
> Here's the testcase I'm using:
> #include <stdio.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <sys/syscall.h>
> #include <sys/time.h>
> 
> /*
>  * mkdir /exports/10k_dentries
>  * for i in {1..10000}; do printf "/exports/10k_dentries/file%.12d\n"
> $i; done | xargs touch
>  */
> #define DIR "/mnt/fedora/10k_dentries"
> #define BUF_SIZE 1024 * 16
> 
> void evict_pagecache() {
>	int dir_fd = open(DIR, O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC);
>	posix_fadvise(dir_fd, 0, 0, POSIX_FADV_DONTNEED);
>	close(dir_fd);
> }
> 
> /* call getdents64() until no more, return time taken */
> int listdir() {
>	struct timeval tvs, tve;
>	char buf[BUF_SIZE];
>	int dir_fd = open(DIR, O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC);
> 
>	gettimeofday(&tvs, NULL);
>	while (syscall(SYS_getdents, dir_fd, buf, BUF_SIZE)) { }
>	gettimeofday(&tve, NULL);
>	return tve.tv_sec - tvs.tv_sec;
> }
> 
> int main(int argc, char **argv)
> {
>	evict_pagecache();
> 
>	while (1) {
>		if (!fork()) {
>			int secs;
>			printf("pid %d starts..\n", getpid());
>			fflush(stdout);
> 
>			secs = listdir();
> 
>			printf("pid %d done        %d seconds\n", getpid(), secs);
>			fflush(stdout);
>			return 0;
>		}
>		sleep (1);
>	}
> }
> 
> Ben