Re: [PATCH v4 5/5] nfsd: start non-blocking writeback after adding nfsd_file to the LRU
From: Chuck Lever III <hidden>
Date: 2022-10-31 21:00:51
On Oct 31, 2022, at 7:37 AM, Jeff Layton [off-list ref] wrote: When a GC entry gets added to the LRU, kick off SYNC_NONE writeback so that we can be ready to close it when the time comes. This should help minimize delays when freeing objects reaped from the LRU.
Tested against a btrfs export. So, the current code does indeed do a synchronous fsync when garbage collecting files (via nfsd_file_delayed_close()). That indicates that it's at least safe to do, and 3/5 isn't changing the safety of the filecache by moving the vfs_fsync() call into nfsd_file_free(). These calls take between 10 and 20 milliseconds each. But I see the filemap_flush() call added here taking dozens of milliseconds on my system for large files. This is done before the WRITE reply is sent to the client, so it adds significant latency to large UNSTABLE WRITEs. In the current code, the vfs_fsync() in nfsd_file_put() does not seem to fire except in very rare circumstances, so it doesn't seem to have much if any impact. My scalability concerns therefore are with code that pre-dates this patch series. I can deal with that later in a separate series. Do we need to keep this one?
quoted hunk ↗ jump to hunk
Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/filecache.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index 47cdc6129a7b..c43b6cff03e2 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c@@ -325,6 +325,20 @@ nfsd_file_fsync(struct nfsd_file *nf)nfsd_reset_write_verifier(net_generic(nf->nf_net, nfsd_net_id)); } +static void +nfsd_file_flush(struct nfsd_file *nf) +{ + struct file *file = nf->nf_file; + struct address_space *mapping; + + if (!file || !(file->f_mode & FMODE_WRITE)) + return; + + mapping = file->f_mapping; + if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) + filemap_flush(mapping); +} + static int nfsd_file_check_write_error(struct nfsd_file *nf) {@@ -484,9 +498,14 @@ nfsd_file_put(struct nfsd_file *nf)/* Try to add it to the LRU. If that fails, decrement. */ if (nfsd_file_lru_add(nf)) { - /* If it's still hashed, we're done */ - if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) + /* + * If it's still hashed, we can just return now, + * after kicking off SYNC_NONE writeback. + */ + if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) { + nfsd_file_flush(nf); return; + } /* * We're racing with unhashing, so try to remove it from -- 2.38.1
-- Chuck Lever