mbox series

[v2,00/16] Readdir enhancements

Message ID 20201103153329.531942-1-trondmy@kernel.org (mailing list archive)
Headers show
Series Readdir enhancements | expand

Message

trondmy@kernel.org Nov. 3, 2020, 3:33 p.m. UTC
From: Trond Myklebust <trond.myklebust@hammerspace.com>

The following patch series performs a number of cleanups on the readdir
code.
It also adds support for 1MB readdir RPC calls on-the-wire, and modifies
the caching code to ensure that we cache the entire contents of that
1MB call (instead of discarding the data that doesn't fit into a single
page).

v2: Fix the handling of the NFSv3/v4 directory verifier

Trond Myklebust (16):
  NFS: Ensure contents of struct nfs_open_dir_context are consistent
  NFS: Clean up readdir struct nfs_cache_array
  NFS: Clean up nfs_readdir_page_filler()
  NFS: Clean up directory array handling
  NFS: Don't discard readdir results
  NFS: Remove unnecessary kmap in nfs_readdir_xdr_to_array()
  NFS: Replace kmap() with kmap_atomic() in nfs_readdir_search_array()
  NFS: Simplify struct nfs_cache_array_entry
  NFS: Support larger readdir buffers
  NFS: More readdir cleanups
  NFS: nfs_do_filldir() does not return a value
  NFS: Reduce readdir stack usage
  NFS: Cleanup to remove nfs_readdir_descriptor_t typedef
  NFS: Allow the NFS generic code to pass in a verifier to readdir
  NFS: Handle NFS4ERR_NOT_SAME and NFSERR_BADCOOKIE from readdir calls
  NFS: Improve handling of directory verifiers

 fs/nfs/client.c         |   4 +-
 fs/nfs/dir.c            | 609 ++++++++++++++++++++++++----------------
 fs/nfs/inode.c          |   7 -
 fs/nfs/internal.h       |   6 -
 fs/nfs/nfs3proc.c       |  35 ++-
 fs/nfs/nfs4proc.c       |  40 +--
 fs/nfs/proc.c           |  18 +-
 include/linux/nfs_fs.h  |   9 +-
 include/linux/nfs_xdr.h |  17 +-
 9 files changed, 439 insertions(+), 306 deletions(-)

Comments

Benjamin Coddington Nov. 4, 2020, 4:14 p.m. UTC | #1
Hi Trond, these look great!

I'm doing some comparison testing before/after this set, and I'm getting
into some memory pressure on a client with 4G ram listing 1.5M dentries 
with
12 char filenames.

It looks like before this set, the readdir code was a bit more resilient 
in
the face of memory pressure, and I'm wondering if we've dropped a call 
to
mark_page_accessed().

* Ben adds:

@@ -460,7 +461,8 @@ static int nfs_readdir_search_array(struct 
nfs_readdir_descriptor *desc)
                 desc->last_cookie = array->last_cookie;
                 desc->current_index += array->size;
                 desc->page_index++;
-       }
+       } else
+               mark_page_accessed(desc->page);
         kunmap_atomic(array);
         return status;
  }

.. no, that's not any better.  I'm still getting evicted pages (or, at
least, low-indexed pages that don't have PageUptodate() set), which 
makes
it nearly impossible to finish listing this directory because we just 
keep
invalidating the mapping.

Any ideas?  I'll keep looking.

Ben
Trond Myklebust Nov. 4, 2020, 5:04 p.m. UTC | #2
Hi Ben

Thanks for the review and the testing!

On Wed, 2020-11-04 at 11:14 -0500, Benjamin Coddington wrote:
> Hi Trond, these look great!
> 
> I'm doing some comparison testing before/after this set, and I'm
> getting
> into some memory pressure on a client with 4G ram listing 1.5M
> dentries 
> with
> 12 char filenames.
> 
> It looks like before this set, the readdir code was a bit more
> resilient 
> in
> the face of memory pressure, and I'm wondering if we've dropped a
> call 
> to
> mark_page_accessed().
> 
> * Ben adds:
> 
> @@ -460,7 +461,8 @@ static int nfs_readdir_search_array(struct 
> nfs_readdir_descriptor *desc)
>                  desc->last_cookie = array->last_cookie;
>                  desc->current_index += array->size;
>                  desc->page_index++;
> -       }
> +       } else
> +               mark_page_accessed(desc->page);
>          kunmap_atomic(array);
>          return status;
>   }
> 
> .. no, that's not any better.  I'm still getting evicted pages (or,
> at
> least, low-indexed pages that don't have PageUptodate() set), which 
> makes
> it nearly impossible to finish listing this directory because we just
> keep
> invalidating the mapping.
> 

You're right that I had screwed up the page access marking in the
previous patchsets. I believe this should be fixed in v3 by the
conversion to use grab_cache_page(), which calls find_or_create_page()
and should therefore do the right thing with the FGP_ACCESSED flag.

I believe the reason why your patch above fails to fully correct the
issue is because we always want to mark the page as accessed if we've
scanned it.
Benjamin Coddington Nov. 4, 2020, 5:19 p.m. UTC | #3
On 4 Nov 2020, at 12:04, Trond Myklebust wrote:

> Hi Ben
>
> Thanks for the review and the testing!

Thank /you/ for the work!

> You're right that I had screwed up the page access marking in the
> previous patchsets. I believe this should be fixed in v3 by the
> conversion to use grab_cache_page(), which calls find_or_create_page()
> and should therefore do the right thing with the FGP_ACCESSED flag.
>
> I believe the reason why your patch above fails to fully correct the
> issue is because we always want to mark the page as accessed if we've
> scanned it.

Ah, that make sense.  I'll take the v3 for a ride tomorrow morning.

Ben