Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: Re: [PATCH 00/16] Cache open file descriptors in knfsd
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20190630135240.7490-1-trond.myklebust@hammerspace.com>
Date:   Mon, 1 Jul 2019 11:02:13 -0400
Cc:     Bruce Fields <bfields@redhat.com>,
        Jeff Layton <jlayton@redhat.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        linux-fsdevel@vger.kernel.org
Content-Transfer-Encoding: 7bit
Message-Id: <F1C28446-51F0-4999-AAA6-4FEA9371E36A@oracle.com>
References: <20190630135240.7490-1-trond.myklebust@hammerspace.com>
To:     Trond Myklebust <trondmy@gmail.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

Interesting work! Kudos to you and Jeff.


> On Jun 30, 2019, at 9:52 AM, Trond Myklebust <trondmy@gmail.com> wrote:
> 
> When a NFSv3 READ or WRITE request comes in, the first thing knfsd has
> to do is open a new file descriptor. While this is often a relatively
> inexpensive thing to do for most local filesystems, it is usually less
> so for FUSE, clustered or networked filesystems that are being exported
> by knfsd.

True, I haven't measured much effect if any of open and close on local
file systems. It would be valuable if the cover letter provided a more
quantified assessment of the cost for these other use cases. It sounds
plausible to me that they would be more expensive, but I'm wondering if
the additional complexity of an open file cache is warranted and
effective. Do you have any benchmark results to share?

Are there particular workloads where you believe open caching will be
especially beneficial?


> This set of patches attempts to reduce some of that cost by caching
> open file descriptors so that they may be reused by other incoming
> READ/WRITE requests for the same file.

Is the open file cache a single cache per server? Wondering if there
can be significant interference (eg lock contention or cache sloshing)
between separate workloads on different exports, for example.

Do you have any benchmark results that show that removing the raparms
cache is harmless?


> One danger when doing this, is that knfsd may end up caching file
> descriptors for files that have been unlinked. In order to deal with
> this issue, we use fsnotify to monitor the files, and have hooks to
> evict those descriptors from the file cache if the i_nlink value
> goes to 0.
> 
> Jeff Layton (12):
>  sunrpc: add a new cache_detail operation for when a cache is flushed
>  locks: create a new notifier chain for lease attempts
>  nfsd: add a new struct file caching facility to nfsd
>  nfsd: hook up nfsd_write to the new nfsd_file cache
>  nfsd: hook up nfsd_read to the nfsd_file cache
>  nfsd: hook nfsd_commit up to the nfsd_file cache
>  nfsd: convert nfs4_file->fi_fds array to use nfsd_files
>  nfsd: convert fi_deleg_file and ls_file fields to nfsd_file
>  nfsd: hook up nfs4_preprocess_stateid_op to the nfsd_file cache
>  nfsd: have nfsd_test_lock use the nfsd_file cache
>  nfsd: rip out the raparms cache
>  nfsd: close cached files prior to a REMOVE or RENAME that would
>    replace target
> 
> Trond Myklebust (4):
>  notify: export symbols for use by the knfsd file cache
>  vfs: Export flush_delayed_fput for use by knfsd.
>  nfsd: Fix up some unused variable warnings
>  nfsd: Fix the documentation for svcxdr_tmpalloc()
> 
> fs/file_table.c                  |   1 +
> fs/locks.c                       |  62 +++
> fs/nfsd/Kconfig                  |   1 +
> fs/nfsd/Makefile                 |   3 +-
> fs/nfsd/blocklayout.c            |   3 +-
> fs/nfsd/export.c                 |  13 +
> fs/nfsd/filecache.c              | 885 +++++++++++++++++++++++++++++++
> fs/nfsd/filecache.h              |  60 +++
> fs/nfsd/nfs4layouts.c            |  12 +-
> fs/nfsd/nfs4proc.c               |  83 +--
> fs/nfsd/nfs4state.c              | 183 ++++---
> fs/nfsd/nfs4xdr.c                |  31 +-
> fs/nfsd/nfssvc.c                 |  16 +-
> fs/nfsd/state.h                  |  10 +-
> fs/nfsd/trace.h                  | 140 +++++
> fs/nfsd/vfs.c                    | 295 ++++-------
> fs/nfsd/vfs.h                    |   9 +-
> fs/nfsd/xdr4.h                   |  19 +-
> fs/notify/fsnotify.h             |   2 -
> fs/notify/group.c                |   2 +
> fs/notify/mark.c                 |   6 +
> include/linux/fs.h               |   5 +
> include/linux/fsnotify_backend.h |   2 +
> include/linux/sunrpc/cache.h     |   1 +
> net/sunrpc/cache.c               |   3 +
> 25 files changed, 1465 insertions(+), 382 deletions(-)
> create mode 100644 fs/nfsd/filecache.c
> create mode 100644 fs/nfsd/filecache.h
> 
> -- 
> 2.21.0
> 

--
Chuck Lever