All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] nfsd: Reduce contention for the nfsd_file nf_rwsem
@ 2021-06-17 23:26 trondmy
  2021-06-18 17:59 ` Chuck Lever III
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: trondmy @ 2021-06-17 23:26 UTC (permalink / raw)
  To: Chuck Lever, J. Bruce Fields; +Cc: linux-nfs

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 related	[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
                   ` (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

end of thread, other threads:[~2021-06-23 21:38 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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-21 18:27     ` Chuck Lever III
2021-06-21 19:06       ` Trond Myklebust
2021-06-21 19:35         ` Trond Myklebust
2021-06-22 14:51 ` Chuck Lever III
2021-06-23 21:38 ` J. Bruce Fields

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.