netdev - Re: [PATCH v3 00/32] Overhaul NFSD filecache

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 8 Jul 2022 20:32:04 +0000
From:   Chuck Lever III <chuck.lever@...cle.com>
To:     Jeff Layton <jlayton@...nel.org>,
        Dave Chinner <david@...morbit.com>,
        Trond Myklebust <trondmy@...merspace.com>
CC:     Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>, "tgraf@...g.ch" <tgraf@...g.ch>
Subject: Re: [PATCH v3 00/32] Overhaul NFSD filecache



> On Jul 8, 2022, at 4:27 PM, Jeff Layton <jlayton@...nel.org> wrote:
> 
> On Fri, 2022-07-08 at 14:23 -0400, Chuck Lever wrote:
>> This series overhauls the NFSD filecache, a cache of server-side
>> "struct file" objects recently used by NFS clients. The purposes of
>> this overhaul are an immediate improvement in cache scalability in
>> the number of open files, and preparation for further improvements.
>> 
>> There are three categories of patches in this series:
>> 
>> 1. Add observability of cache operation so we can see what we're
>> doing as changes are made to the code.
>> 
>> 2. Improve the scalability of filecache garbage collection,
>> addressing several bugs along the way.
>> 
>> 3. Improve the scalability of the filecache hash table by converting
>> it to use rhashtable.
>> 
>> These patches are available in the for-next branch of:
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git 
>> 
>> Changes since v2:
>> - Fix a cred use-after-free crasher
>> - Fix a Smatch error reported by Dan Carpenter
>> - Replace dereferences of nfsd_file::nf_inode
>> - Further clean-ups and white-space adjustments
>> 
>> Changes since RFC:
>> - Fixed several crashers
>> - Adjusted some of the new observability
>> - Tests with generic/531 now pass
>> - Fixed bugzilla 387 too, maybe
>> - Plenty of clean-ups
>> 
>> ---
>> 
>> Chuck Lever (32):
>>      NFSD: Demote a WARN to a pr_warn()
>>      NFSD: Report filecache LRU size
>>      NFSD: Report count of calls to nfsd_file_acquire()
>>      NFSD: Report count of freed filecache items
>>      NFSD: Report average age of filecache items
>>      NFSD: Add nfsd_file_lru_dispose_list() helper
>>      NFSD: Refactor nfsd_file_gc()
>>      NFSD: Refactor nfsd_file_lru_scan()
>>      NFSD: Report the number of items evicted by the LRU walk
>>      NFSD: Record number of flush calls
>>      NFSD: Zero counters when the filecache is re-initialized
>>      NFSD: Hook up the filecache stat file
>>      NFSD: WARN when freeing an item still linked via nf_lru
>>      NFSD: Trace filecache LRU activity
>>      NFSD: Leave open files out of the filecache LRU
>>      NFSD: Fix the filecache LRU shrinker
>>      NFSD: Never call nfsd_file_gc() in foreground paths
>>      NFSD: No longer record nf_hashval in the trace log
>>      NFSD: Remove lockdep assertion from unhash_and_release_locked()
>>      NFSD: nfsd_file_unhash can compute hashval from nf->nf_inode
>>      NFSD: Refactor __nfsd_file_close_inode()
>>      NFSD: nfsd_file_hash_remove can compute hashval
>>      NFSD: Remove nfsd_file::nf_hashval
>>      NFSD: Replace the "init once" mechanism
>>      NFSD: Set up an rhashtable for the filecache
>>      NFSD: Convert the filecache to use rhashtable
>>      NFSD: Clean up unused code after rhashtable conversion
>>      NFSD: Separate tracepoints for acquire and create
>>      NFSD: Move nfsd_file_trace_alloc() tracepoint
>>      NFSD: Update the nfsd_file_fsnotify_handle_event() tracepoint
>>      NFSD: NFSv4 CLOSE should release an nfsd_file immediately
>>      NFSD: Ensure nf_inode is never dereferenced
>> 
>> 
>> fs/nfsd/filecache.c       | 727 ++++++++++++++++++++++++--------------
>> fs/nfsd/filecache.h       |   7 +-
>> fs/nfsd/nfs4proc.c        |   6 +-
>> fs/nfsd/nfs4state.c       |   7 +-
>> fs/nfsd/nfsctl.c          |  10 +
>> fs/nfsd/trace.h           | 300 +++++++++++++---
>> include/trace/events/fs.h |  37 ++
>> 7 files changed, 774 insertions(+), 320 deletions(-)
>> 
>> --
>> Chuck Lever
>> 
> 
> Nice work, Chuck!
> 
> You can add this to all but #15 (where I still have a question about
> whether adding it to the LRU on every put is the right thing to do).

Thanks for your review! #15 emulates what other list_lru consumers
appear to do, but I'd like to hear from Trond and/or Dave who
advocated for that approach.


> Reviewed-by: Jeff Layton <jlayton@...nel.org>

--
Chuck Lever