On Mon, 2021-06-21 at 16:49 +0000, Chuck Lever III wrote:
> Hi-
>
> > On Jun 17, 2021, at 7:26 PM, trondmy@xxxxxxxxxx wrote:
> >
> > From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> >
> > When flushing out the unstable file writes as part of a COMMIT
> > call, try
> > to perform most of of the data writes and waits outside the
> > semaphore.
> >
> > This means that if the client is sending the COMMIT as part of a
> > memory
> > reclaim operation, then it can continue performing I/O, with
> > contention
> > for the lock occurring only once the data sync is finished.
> >
> > Fixes: 5011af4c698a ("nfsd: Fix stable writes")
> > Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>
> The good news is I've found no functional regressions. The bad
> news is I haven't seen any difference in performance. Is there
> a particular test that I can run to observe improvement?
I'd expect that re-exported NFS would be the best test since fsync() is
a high latency operation when the page cache is loaded. You also want
to use a client with relatively limited memory so that it will try to
reclaim memory by pushing out dirty pages and doing COMMIT.
>
> I wonder about adding a Fixes: tag for a change that the patch
> description describes as an optimization.
I've occasionally hit OOM situations in the re-export case when the r/w
lock contention causes softerr failure to be serialised.
i.e. if the server is down, and you're essentially hoping that the nfsd
threads will give up and return EJUKEBOX/NFS4ERR_DELAY to the client,
then that lock ensures that threads fail one-by-one (grab lock, write,
timeout, release lock) instead of being able to all fail at once
(write, timeout).
>
>
> > ---
> > fs/nfsd/vfs.c | 18 ++++++++++++++++--
> > 1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 15adf1f6ab21..46485c04740d 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -1123,6 +1123,19 @@ nfsd_write(struct svc_rqst *rqstp, struct
> > svc_fh *fhp, loff_t offset,
> > }
> >
> > #ifdef CONFIG_NFSD_V3
> > +static int
> > +nfsd_filemap_write_and_wait_range(struct nfsd_file *nf, loff_t
> > offset,
> > + loff_t end)
> > +{
> > + struct address_space *mapping = nf->nf_file->f_mapping;
> > + int ret = filemap_fdatawrite_range(mapping, offset, end);
> > +
> > + if (ret)
> > + return ret;
> > + filemap_fdatawait_range_keep_errors(mapping, offset, end);
> > + return 0;
> > +}
> > +
> > /*
> > * Commit all pending writes to stable storage.
> > *
> > @@ -1153,10 +1166,11 @@ nfsd_commit(struct svc_rqst *rqstp, struct
> > svc_fh *fhp,
> > if (err)
> > goto out;
> > if (EX_ISSYNC(fhp->fh_export)) {
> > - int err2;
> > + int err2 = nfsd_filemap_write_and_wait_range(nf,
> > offset, end);
> >
> > down_write(&nf->nf_rwsem);
> > - err2 = vfs_fsync_range(nf->nf_file, offset, end,
> > 0);
> > + if (!err2)
> > + err2 = vfs_fsync_range(nf->nf_file, offset,
> > end, 0);
> > switch (err2) {
> > case 0:
> > nfsd_copy_boot_verifier(verf,
> > net_generic(nf->nf_net,
> > --
> > 2.31.1
> >
>
> --
> Chuck Lever
>
>
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
