* Re: [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem
2021-06-17 23:26 [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem trondmy
@ 2021-06-18 17:59 ` Chuck Lever III
2021-06-21 16:49 ` Chuck Lever III
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Chuck Lever III @ 2021-06-18 17:59 UTC (permalink / raw)
To: trondmy; +Cc: Bruce Fields, Linux NFS Mailing List
> On Jun 17, 2021, at 7:26 PM, trondmy@kernel.org wrote:
>
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> 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@hammerspace.com>
I'll toss this onto my server over the weekend for some testing.
> ---
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem
2021-06-17 23:26 [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem trondmy
2021-06-18 17:59 ` Chuck Lever III
@ 2021-06-21 16:49 ` Chuck Lever III
2021-06-21 18:00 ` Trond Myklebust
2021-06-22 14:51 ` Chuck Lever III
2021-06-23 21:38 ` J. Bruce Fields
3 siblings, 1 reply; 9+ messages in thread
From: Chuck Lever III @ 2021-06-21 16:49 UTC (permalink / raw)
To: trondmy; +Cc: Bruce Fields, Linux NFS Mailing List
Hi-
> On Jun 17, 2021, at 7:26 PM, trondmy@kernel.org wrote:
>
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> 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@hammerspace.com>
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 wonder about adding a Fixes: tag for a change that the patch
description describes as an optimization.
> ---
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem
2021-06-21 16:49 ` Chuck Lever III
@ 2021-06-21 18:00 ` Trond Myklebust
2021-06-21 18:27 ` Chuck Lever III
0 siblings, 1 reply; 9+ messages in thread
From: Trond Myklebust @ 2021-06-21 18:00 UTC (permalink / raw)
To: chuck.lever, trondmy; +Cc: linux-nfs, bfields
On Mon, 2021-06-21 at 16:49 +0000, Chuck Lever III wrote:
> Hi-
>
> > On Jun 17, 2021, at 7:26 PM, trondmy@kernel.org wrote:
> >
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> >
> > 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@hammerspace.com>
>
> 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@hammerspace.com
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem
2021-06-21 18:00 ` Trond Myklebust
@ 2021-06-21 18:27 ` Chuck Lever III
2021-06-21 19:06 ` Trond Myklebust
0 siblings, 1 reply; 9+ messages in thread
From: Chuck Lever III @ 2021-06-21 18:27 UTC (permalink / raw)
To: Trond Myklebust; +Cc: trondmy, Linux NFS Mailing List, Bruce Fields
> On Jun 21, 2021, at 2:00 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Mon, 2021-06-21 at 16:49 +0000, Chuck Lever III wrote:
>> Hi-
>>
>>> On Jun 17, 2021, at 7:26 PM, trondmy@kernel.org wrote:
>>>
>>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>>
>>> 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@hammerspace.com>
>>
>> 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 thought I was hitting those low-memory cases with direct I/O testing. <shrug>
>> 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@hammerspace.com
--
Chuck Lever
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem
2021-06-21 18:27 ` Chuck Lever III
@ 2021-06-21 19:06 ` Trond Myklebust
2021-06-21 19:35 ` Trond Myklebust
0 siblings, 1 reply; 9+ messages in thread
From: Trond Myklebust @ 2021-06-21 19:06 UTC (permalink / raw)
To: chuck.lever; +Cc: linux-nfs, bfields, trondmy
On Mon, 2021-06-21 at 18:27 +0000, Chuck Lever III wrote:
>
>
> > On Jun 21, 2021, at 2:00 PM, Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> >
> > On Mon, 2021-06-21 at 16:49 +0000, Chuck Lever III wrote:
> > > Hi-
> > >
> > > > On Jun 17, 2021, at 7:26 PM, trondmy@kernel.org wrote:
> > > >
> > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > >
> > > > 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@hammerspace.com>
> > >
> > > 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 thought I was hitting those low-memory cases with direct I/O
> testing. <shrug>
If you do O_DIRECT with 100MB write() calls, then maybe. If you use <
1MB write()s then you'll rarely if ever hit any COMMIT calls.
>
>
> > > 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@hammerspace.com
>
> --
> Chuck Lever
>
>
>
--
Trond Myklebust
CTO, Hammerspace Inc
4984 El Camino Real, Suite 208
Los Altos, CA 94022
www.hammer.space
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem
2021-06-21 19:06 ` Trond Myklebust
@ 2021-06-21 19:35 ` Trond Myklebust
0 siblings, 0 replies; 9+ messages in thread
From: Trond Myklebust @ 2021-06-21 19:35 UTC (permalink / raw)
To: chuck.lever; +Cc: linux-nfs, bfields
On Mon, 2021-06-21 at 15:06 -0400, Trond Myklebust wrote:
> On Mon, 2021-06-21 at 18:27 +0000, Chuck Lever III wrote:
> >
> >
> > > On Jun 21, 2021, at 2:00 PM, Trond Myklebust
> > > <trondmy@hammerspace.com> wrote:
> > >
> > > On Mon, 2021-06-21 at 16:49 +0000, Chuck Lever III wrote:
> > > > Hi-
> > > >
> > > > > On Jun 17, 2021, at 7:26 PM, trondmy@kernel.org wrote:
> > > > >
> > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > >
> > > > > 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@hammerspace.com>
> > > >
> > > > 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 thought I was hitting those low-memory cases with direct I/O
> > testing. <shrug>
>
> If you do O_DIRECT with 100MB write() calls, then maybe. If you use <
> 1MB write()s then you'll rarely if ever hit any COMMIT calls.
Let me correct myself: You'd need to do aio/dio with 100MB write system
calls so that the flush part of the COMMIT operation takes significant
time on the server, and can interfere with other COMMIT operations.
>
> >
> >
> > > > 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@hammerspace.com
> >
> > --
> > Chuck Lever
> >
> >
> >
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem
2021-06-17 23:26 [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem trondmy
2021-06-18 17:59 ` Chuck Lever III
2021-06-21 16:49 ` Chuck Lever III
@ 2021-06-22 14:51 ` Chuck Lever III
2021-06-23 21:38 ` J. Bruce Fields
3 siblings, 0 replies; 9+ messages in thread
From: Chuck Lever III @ 2021-06-22 14:51 UTC (permalink / raw)
To: trondmy, Bruce Fields; +Cc: Linux NFS Mailing List
> On Jun 17, 2021, at 7:26 PM, trondmy@kernel.org wrote:
>
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> 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@hammerspace.com>
I can see write throughput improvement now.
Tested-by: Chuck Lever <chuck.lever@oracle.com>
This is NFSv3 against an NVMe-backed XFS export, wsize=8192:
v5.13-rc6:
Command line used: /home/cel/bin/iozone -M -+u -i0 -i1 -s1g -r256k -t12 -I
Output is in kBytes/sec
Time Resolution = 0.000001 seconds.
Processor cache size set to 1024 kBytes.
Processor cache line size set to 32 bytes.
File stride size set to 17 * record size.
Throughput test with 12 processes
Each process writes a 1048576 kByte file in 256 kByte records
Children see throughput for 12 initial writers = 416004.59 kB/sec
Parent sees throughput for 12 initial writers = 415691.65 kB/sec
Min throughput per process = 34630.36 kB/sec
Max throughput per process = 34703.62 kB/sec
Avg throughput per process = 34667.05 kB/sec
Min xfer = 1046528.00 kB
CPU Utilization: Wall time 30.239 CPU time 5.854 CPU utilization 19.36 %
Children see throughput for 12 rewriters = 516605.59 kB/sec
Parent sees throughput for 12 rewriters = 516530.05 kB/sec
Min throughput per process = 43007.56 kB/sec
Max throughput per process = 43074.50 kB/sec
Avg throughput per process = 43050.47 kB/sec
Min xfer = 1047040.00 kB
CPU utilization: Wall time 24.347 CPU time 5.882 CPU utilization 24.16 %
v5.13-rc6 + Trond's patch:
Command line used: /home/cel/bin/iozone -M -+u -i0 -i1 -s1g -r256k -t12 -I
Output is in kBytes/sec
Time Resolution = 0.000001 seconds.
Processor cache size set to 1024 kBytes.
Processor cache line size set to 32 bytes.
File stride size set to 17 * record size.
Throughput test with 12 processes
Each process writes a 1048576 kByte file in 256 kByte records
Children see throughput for 12 initial writers = 434971.09 kB/sec
Parent sees throughput for 12 initial writers = 434649.13 kB/sec
Min throughput per process = 36209.41 kB/sec
Max throughput per process = 36287.55 kB/sec
Avg throughput per process = 36247.59 kB/sec
Min xfer = 1046528.00 kB
CPU Utilization: Wall time 28.920 CPU time 5.705 CPU utilization 19.73 %
Children see throughput for 12 rewriters = 544700.37 kB/sec
Parent sees throughput for 12 rewriters = 544623.91 kB/sec
Min throughput per process = 45320.82 kB/sec
Max throughput per process = 45456.07 kB/sec
Avg throughput per process = 45391.70 kB/sec
Min xfer = 1045504.00 kB
CPU utilization: Wall time 23.071 CPU time 5.708 CPU utilization 24.74 %
> ---
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem
2021-06-17 23:26 [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem trondmy
` (2 preceding siblings ...)
2021-06-22 14:51 ` Chuck Lever III
@ 2021-06-23 21:38 ` J. Bruce Fields
3 siblings, 0 replies; 9+ messages in thread
From: J. Bruce Fields @ 2021-06-23 21:38 UTC (permalink / raw)
To: trondmy; +Cc: Chuck Lever, J. Bruce Fields, linux-nfs
Applying, with Chuck's Tested-by, thanks.--b.
On Thu, Jun 17, 2021 at 07:26:52PM -0400, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> 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@hammerspace.com>
> ---
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread