> On Oct 27, 2022, at 5:52 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> When a GC entry gets added to the LRU, kick off SYNC_NONE writeback
> so that we can be ready to close it out when the time comes.
For a large file, a background flush still has to walk the file's
pages to see if they are dirty, and that consumes time, CPU, and
memory bandwidth. We're talking hundreds of microseconds for a
large file.
Then the final flush does all that again.
Basically, two (or more!) passes through the file for exactly the
same amount of work. Is there any measured improvement in latency
or throughput?
And then... for a GC file, no-one is waiting on data persistence
during nfsd_file_put() so I'm not sure what is gained by taking
control of the flushing process away from the underlying filesystem.
Remind me why the filecache is flushing? Shouldn't NFSD rely on
COMMIT operations for that? (It's not obvious reading the code,
maybe there should be a documenting comment somewhere that
explains this arrangement).
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/nfsd/filecache.c | 37 +++++++++++++++++++++++++++++++------
> 1 file changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index d2bbded805d4..491d3d9a1870 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -316,7 +316,7 @@ nfsd_file_alloc(struct nfsd_file_lookup_key *key, unsigned int may)
> }
>
> static void
> -nfsd_file_flush(struct nfsd_file *nf)
> +nfsd_file_fsync(struct nfsd_file *nf)
> {
> struct file *file = nf->nf_file;
>
> @@ -327,6 +327,22 @@ nfsd_file_flush(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;
> + unsigned long nrpages;
> +
> + if (!file || !(file->f_mode & FMODE_WRITE))
> + return;
> +
> + nrpages = file->f_mapping->nrpages;
> + if (nrpages) {
> + this_cpu_add(nfsd_file_pages_flushed, nrpages);
> + filemap_flush(file->f_mapping);
> + }
> +}
> +
> static void
> nfsd_file_free(struct nfsd_file *nf)
> {
> @@ -337,7 +353,7 @@ nfsd_file_free(struct nfsd_file *nf)
> this_cpu_inc(nfsd_file_releases);
> this_cpu_add(nfsd_file_total_age, age);
>
> - nfsd_file_flush(nf);
> + nfsd_file_fsync(nf);
>
> if (nf->nf_mark)
> nfsd_file_mark_put(nf->nf_mark);
> @@ -500,12 +516,21 @@ nfsd_file_put(struct nfsd_file *nf)
>
> if (test_bit(NFSD_FILE_GC, &nf->nf_flags)) {
> /*
> - * If this is the last reference (nf_ref == 1), then transfer
> - * it to the LRU. If the add to the LRU fails, just put it as
> - * usual.
> + * If this is the last reference (nf_ref == 1), then try
> + * to transfer it to the LRU.
> + */
> + if (refcount_dec_not_one(&nf->nf_ref))
> + return;
> +
> + /*
> + * If the add to the list succeeds, try to kick off SYNC_NONE
> + * writeback. If the add fails, then just fall through to
> + * decrement as usual.
These comments simply repeat what the code does, so they seem
redundant to me. Could they instead explain why?
> */
> - if (refcount_dec_not_one(&nf->nf_ref) || nfsd_file_lru_add(nf))
> + if (nfsd_file_lru_add(nf)) {
> + nfsd_file_flush(nf);
> return;
> + }
> }
> __nfsd_file_put(nf);
> }
> --
> 2.37.3
>
--
Chuck Lever
