On Sat, Nov 21, 2020 at 12:16 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote: > > > > > On Nov 21, 2020, at 12:01 PM, David Wysochanski <dwysocha@xxxxxxxxxx> wrote: > > > > On Sat, Nov 21, 2020 at 11:14 AM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote: > >> > >> Hi Dave- > >> > >>> On Nov 21, 2020, at 8:29 AM, Dave Wysochanski <dwysocha@xxxxxxxxxx> wrote: > >>> > >>> These patches update the NFS client to use the new netfs and fscache > >>> APIs and are at: > >>> https://github.com/DaveWysochanskiRH/kernel.git > >>> https://github.com/DaveWysochanskiRH/kernel/commit/94e9633d98a5542ea384b1095290ac6f915fc917 > >>> https://github.com/DaveWysochanskiRH/kernel/releases/tag/fscache-iter-nfs-20201120 > >>> > >>> The patches are based on David Howells fscache-iter tree at > >>> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-iter > >>> > >>> The first 6 patches refactor some of the NFS read code to facilitate > >>> re-use, the next 6 patches do the conversion to the new API, and the > >>> last patch is a somewhat awkward fix for a problem seen in final > >>> testing. > >>> > >>> Per David Howell's recent post, note that the new fscache API is > >>> divided into two separate APIs, a 'netfs' API and an 'fscache' API. > >>> The netfs API was done to help simplify the IO paths of network > >>> filesystems, and can be called even when fscache is disabled, thus > >>> simplifing both readpage and readahead implementations. However, > >>> for now these NFS conversion patches only call the netfs API when > >>> fscache is enabled, similar to the existing NFS code. > >>> > >>> Trond and Anna, I would appreciate your guidance on this patchset. > >>> At least I would like your feedback regarding the direction > >>> you would like these patches to go, in particular, the following > >>> items: > >>> > >>> 1. Whether you are ok with using the netfs API unconditionally even > >>> when fscache is disabled, or prefer this "least invasive to NFS" > >>> approach. Note the unconditional use of the netfs API is the > >>> recommended approach per David's post and the AFS and CEPH > >>> implementations, but I was unsure if you would accept this > >>> approach or would prefer to minimize changes to NFS. Note if > >>> we keep the current approach to minimize NFS changes, we will > >>> have to address some problems with page unlocking such as with > >>> patch 13 in the series. > >>> > >>> 2. Whether to keep the NFS fscache implementation as "read only" > >>> or if we add write through support. Today we only enable fscache > >>> when a file is open read-only and disable / invalidate when a file > >>> is open for write. > >>> > >>> Still TODO > >>> 1. Address known issues (lockdep, page unlocking), depending on > >>> what is decided as far as implementation direction > >>> a) nfs_issue_op: takes rcu_read_lock but may calls nfs_page_alloc() > >>> with GFP_KERNEL which may sleep (dhowells noted this in a review) > >>> b) nfs_refresh_inode() takes inode->i_lock but may call > >>> __fscache_invalidate() which may sleep (found with lockdep) > >>> 2. Fixup NFS fscache stats (NFSIOS_FSCACHE_*) > >>> * Compare with netfs stats and determine if still needed > >>> 3. Cleanup dfprintks and/or convert to tracepoints > >>> 4. Further tests (see "Not tested yet") > >> > >> Can you say whether your approach has any performance impact? > > No I cannot. > > > >> In particular, what comparative benchmarks have been run? > >> > > No comparisons so far, but note the last bullet - "performance". > > > > Are you wondering about performance with/without fscache for this > > series, or the old vs new fscache, or something else? > > I'd like to have some explicit performance-related merge worthiness > criteria. For example: "No performance regressions, and here's how > we're going to determine that we're good: fio / iozone / yada with > NFS/TCP and NFS/RDMA on 100GbE; for very little additional CPU > cost measured via perf xyzzy. Also some benchmark that measures lock > contention." > > We haven't been especially careful about this in the past when > reworking the client's primary I/O paths. Nothing unreasonable, but > it should be stated up front where we want to end up. > Makes sense. > Another approach might be: we're going to start by making fscache > opt-in. As confidence increases over time and good performance is > demonstrated, then we'll unify the fscache and non-cached I/O paths. > It sounds like what you want is what I've done in this first implementation. This implementation takes a "least invasive to NFS" approach, staying with the old fscache on/off logic, even though this was not ideal or what was recommended as the end game for the netfs API. > > >>> Checks run > >>> 1. checkpatch: PASS* > >>> * a few warnings, mostly trivial fixups, some unrelated to this set > >>> 2. kernel builds with each patch: PASS > >>> * each patch in series built cleanly which ensure bisection > >>> > >>> Tests run > >>> 1. Custom NFS+fscache unit tests for basic operation: PASS* > >>> * no oops or data corruptions > >>> * Some op counts are a bit off but these are mostly due > >>> to statistics not implemented properly (NFSIOS_FSCACHE_*) > >>> 2. cthon04: PASS (test options "-b -g -s -l", fsc,vers=3,4.0,4.1,4.2,sec=sys) > >>> * No failures or oopses for any version or test options > >>> 3. iozone tests (fsc,vers=3,4.0,4.1,4.2,sec=sys): PASS > >>> * No failures or oopses > >>> 4. kernel build (fsc,vers=3,4.1,4.2): PASS* > >>> * all builds finish without errors or data corruption > >>> * one lockdep "scheduling while atomic" fired with NFS41 and > >>> was due to one an fscache invalidation code path (known issue 'b' above) > >>> 5. xfstests/generic (fsc,vers=4.2, nofsc,vers=4.2): PASS* > >>> * generic/013 (pass but triggers i_lock lockdep warning known issue 'a' above) > >>> * NOTE: The following tests failed with errors, but they > >>> also fail on vanilla 5.10-rc4 so are not related to this > >>> patchset > >>> * generic/074 (lockep invalid wait context - nfs_free_request()) > >>> * generic/538 (short read) > >>> * generic/551 (pread: Unknown error 524, Data verification fail) > >>> * generic/568 (ERROR: File grew from 4096 B to 8192 B when writing to the fallocated range) > >>> > >>> Not tested yet: > >>> * error injections (for example, connection disruptions, server errors during IO, etc) > >>> * pNFS > >>> * many process mixed read/write on same file > >>> * performance > >>> Dave Wysochanski (13): > >>> NFS: Clean up nfs_readpage() and nfs_readpages() > >>> NFS: In nfs_readpage() only increment NFSIOS_READPAGES when read > >>> succeeds > >>> NFS: Refactor nfs_readpage() and nfs_readpage_async() to use > >>> nfs_readdesc > >>> NFS: Call readpage_async_filler() from nfs_readpage_async() > >>> NFS: Add nfs_pageio_complete_read() and remove nfs_readpage_async() > >>> NFS: Allow internal use of read structs and functions > >>> NFS: Convert fscache_acquire_cookie and fscache_relinquish_cookie > >>> NFS: Convert fscache_enable_cookie and fscache_disable_cookie > >>> NFS: Convert fscache invalidation and update aux_data and i_size > >>> NFS: Convert to the netfs API and nfs_readpage to use netfs_readpage > >>> NFS: Convert readpage to readahead and use netfs_readahead for fscache > >>> NFS: Allow NFS use of new fscache API in build > >>> NFS: Ensure proper page unlocking when reads fail with retryable > >>> errors > >>> > >>> fs/nfs/Kconfig | 2 +- > >>> fs/nfs/direct.c | 2 + > >>> fs/nfs/file.c | 22 ++-- > >>> fs/nfs/fscache-index.c | 94 -------------- > >>> fs/nfs/fscache.c | 315 ++++++++++++++++++++------------------------- > >>> fs/nfs/fscache.h | 132 +++++++------------ > >>> fs/nfs/inode.c | 4 +- > >>> fs/nfs/internal.h | 8 ++ > >>> fs/nfs/nfs4proc.c | 2 +- > >>> fs/nfs/pagelist.c | 2 + > >>> fs/nfs/read.c | 248 ++++++++++++++++------------------- > >>> fs/nfs/write.c | 3 +- > >>> include/linux/nfs_fs.h | 5 +- > >>> include/linux/nfs_iostat.h | 2 +- > >>> include/linux/nfs_page.h | 1 + > >>> include/linux/nfs_xdr.h | 1 + > >>> 16 files changed, 339 insertions(+), 504 deletions(-) > >>> > >>> -- > >>> 1.8.3.1 > >>> > >> > >> -- > >> Chuck Lever > > -- > Chuck Lever > > >
