On Mon, 2022-10-31 at 21:00 +0000, Chuck Lever III wrote:
>
> > On Oct 31, 2022, at 7:37 AM, 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 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?
>
In the interest of getting the fixes in this series merged, let's just
drop this one for now. We can debate how to best handle this in a
follow-on series.
>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > 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
>
>
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
