Re: [PATCH v3 4/4] nfsd: start non-blocking writeback after adding nfsd_file to the LRU

[Chuck Lever III <chuck.lever@oracle.com>]

> On Oct 28, 2022, at 4:30 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Fri, 2022-10-28 at 19:50 +0000, Chuck Lever III wrote:
>> 
>>> On Oct 28, 2022, at 2:57 PM, Jeff Layton <jlayton@kernel.org> 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.
>>> 
>>> 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;
>>> +			}
>> 
>> nfsd_write() calls nfsd_file_put() after every nfsd_vfs_write(). In some
>> cases, this new logic adds an async flush after every UNSTABLE NFSv3 WRITE.
>> 
>> I'll need to see performance measurements demonstrating no negative
>> impact on throughput or latency of NFSv3 WRITEs with large payloads.
>> 
>> 
> 
> In your earlier mail, you mentioned that you wanted the writeback work
> to be done in the context of nfsd threads. nfsd_file_put is how nfsd
> threads put their references so this seems like the right place to do
> it.
> 
> If you're concerned about calling filemap_flush too often because we
> have an entry that's cycling onto and off of the LRU, then another
> (maybe better) idea might be to kick off writeback when we clear the
> REFERENCED flag.

I think we are doing just about that today by flushing in nfsd_file_put
right when the REFERENCED bit is set. :-)

But yes: that is essentially it. nfsd is a good place to do the flush,
but we don't want to flush too often, because that will be noticeable.


> That would need to be done in the gc thread context, however.

Apparently it is already doing this via filp_close(), though it's
not clear how often that call needs to wait for I/O. You could
schedule a worker to complete the tear down if the open file has
dirty pages.

To catch errors that might occur when the client is delaying its
COMMITs for a long while, maybe don't evict nfsd_files that have
dirty pages...?


>>> 			/*
>>> 			 * We're racing with unhashing, so try to remove it from
>>> -- 
>>> 2.37.3
>>> 
>> 
>> --
>> Chuck Lever
>> 
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever