* [PATCH 1/2] nfsd: Fix a write performance regression
@ 2022-03-31 13:54 trondmy
2022-03-31 13:54 ` [PATCH 2/2] nfsd: Clean up nfsd_file_put() trondmy
2022-03-31 14:36 ` [PATCH 1/2] nfsd: Fix a write performance regression Jan Kara
0 siblings, 2 replies; 4+ messages in thread
From: trondmy @ 2022-03-31 13:54 UTC (permalink / raw)
To: Chuck Lever, Jan Kara; +Cc: linux-nfs
From: Trond Myklebust <trond.myklebust@hammerspace.com>
The call to filemap_flush() in nfsd_file_put() is there to ensure that
we clear out any writes belonging to a NFSv3 client relatively quickly
and avoid situations where the file can't be evicted by the garbage
collector. It also ensures that we detect write errors quickly.
The problem is this causes a regression in performance for some
workloads.
So try to improve matters by deferring writeback until we're ready to
close the file, and need to detect errors so that we can force the
client to resend.
Tested-by: Jan Kara <jack@suse.cz>
Fixes: b6669305d35a ("nfsd: Reduce the number of calls to nfsd_file_gc()")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
fs/nfsd/filecache.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 8bc807c5fea4..9578a6317709 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -235,6 +235,13 @@ nfsd_file_check_write_error(struct nfsd_file *nf)
return filemap_check_wb_err(file->f_mapping, READ_ONCE(file->f_wb_err));
}
+static void
+nfsd_file_flush(struct nfsd_file *nf)
+{
+ if (nf->nf_file && vfs_fsync(nf->nf_file, 1) != 0)
+ nfsd_reset_write_verifier(net_generic(nf->nf_net, nfsd_net_id));
+}
+
static void
nfsd_file_do_unhash(struct nfsd_file *nf)
{
@@ -302,11 +309,14 @@ nfsd_file_put(struct nfsd_file *nf)
return;
}
- filemap_flush(nf->nf_file->f_mapping);
is_hashed = test_bit(NFSD_FILE_HASHED, &nf->nf_flags) != 0;
- nfsd_file_put_noref(nf);
- if (is_hashed)
+ if (!is_hashed) {
+ nfsd_file_flush(nf);
+ nfsd_file_put_noref(nf);
+ } else {
+ nfsd_file_put_noref(nf);
nfsd_file_schedule_laundrette();
+ }
if (atomic_long_read(&nfsd_filecache_count) >= NFSD_FILE_LRU_LIMIT)
nfsd_file_gc();
}
@@ -327,6 +337,7 @@ nfsd_file_dispose_list(struct list_head *dispose)
while(!list_empty(dispose)) {
nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
list_del(&nf->nf_lru);
+ nfsd_file_flush(nf);
nfsd_file_put_noref(nf);
}
}
@@ -340,6 +351,7 @@ nfsd_file_dispose_list_sync(struct list_head *dispose)
while(!list_empty(dispose)) {
nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
list_del(&nf->nf_lru);
+ nfsd_file_flush(nf);
if (!refcount_dec_and_test(&nf->nf_ref))
continue;
if (nfsd_file_free(nf))
--
2.35.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 2/2] nfsd: Clean up nfsd_file_put()
2022-03-31 13:54 [PATCH 1/2] nfsd: Fix a write performance regression trondmy
@ 2022-03-31 13:54 ` trondmy
2022-03-31 14:36 ` [PATCH 1/2] nfsd: Fix a write performance regression Jan Kara
1 sibling, 0 replies; 4+ messages in thread
From: trondmy @ 2022-03-31 13:54 UTC (permalink / raw)
To: Chuck Lever, Jan Kara; +Cc: linux-nfs
From: Trond Myklebust <trond.myklebust@hammerspace.com>
Make it a little less racy, by removing the refcount_read() test. Then
remove the redundant 'is_hashed' variable.
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
fs/nfsd/filecache.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 9578a6317709..46714a71ec87 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -301,21 +301,14 @@ nfsd_file_put_noref(struct nfsd_file *nf)
void
nfsd_file_put(struct nfsd_file *nf)
{
- bool is_hashed;
-
set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
- if (refcount_read(&nf->nf_ref) > 2 || !nf->nf_file) {
- nfsd_file_put_noref(nf);
- return;
- }
-
- is_hashed = test_bit(NFSD_FILE_HASHED, &nf->nf_flags) != 0;
- if (!is_hashed) {
+ if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) == 0) {
nfsd_file_flush(nf);
nfsd_file_put_noref(nf);
} else {
nfsd_file_put_noref(nf);
- nfsd_file_schedule_laundrette();
+ if (nf->nf_file)
+ nfsd_file_schedule_laundrette();
}
if (atomic_long_read(&nfsd_filecache_count) >= NFSD_FILE_LRU_LIMIT)
nfsd_file_gc();
--
2.35.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] nfsd: Fix a write performance regression
2022-03-31 13:54 [PATCH 1/2] nfsd: Fix a write performance regression trondmy
2022-03-31 13:54 ` [PATCH 2/2] nfsd: Clean up nfsd_file_put() trondmy
@ 2022-03-31 14:36 ` Jan Kara
2022-03-31 14:38 ` Chuck Lever III
1 sibling, 1 reply; 4+ messages in thread
From: Jan Kara @ 2022-03-31 14:36 UTC (permalink / raw)
To: trondmy; +Cc: Chuck Lever, Jan Kara, linux-nfs
On Thu 31-03-22 09:54:01, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> The call to filemap_flush() in nfsd_file_put() is there to ensure that
> we clear out any writes belonging to a NFSv3 client relatively quickly
> and avoid situations where the file can't be evicted by the garbage
> collector. It also ensures that we detect write errors quickly.
>
> The problem is this causes a regression in performance for some
> workloads.
>
> So try to improve matters by deferring writeback until we're ready to
> close the file, and need to detect errors so that we can force the
> client to resend.
>
> Tested-by: Jan Kara <jack@suse.cz>
> Fixes: b6669305d35a ("nfsd: Reduce the number of calls to nfsd_file_gc()")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
Perphaps you could add:
Link: https://lore.kernel.org/all/20220330103457.r4xrhy2d6nhtouzk@quack3.lan
To make life of Thorsten Leemhuis doing regression tracking simpler :) (I
think his automation will close the regression once it sees a patch like
that merged).
Honza
> ---
> fs/nfsd/filecache.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 8bc807c5fea4..9578a6317709 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -235,6 +235,13 @@ nfsd_file_check_write_error(struct nfsd_file *nf)
> return filemap_check_wb_err(file->f_mapping, READ_ONCE(file->f_wb_err));
> }
>
> +static void
> +nfsd_file_flush(struct nfsd_file *nf)
> +{
> + if (nf->nf_file && vfs_fsync(nf->nf_file, 1) != 0)
> + nfsd_reset_write_verifier(net_generic(nf->nf_net, nfsd_net_id));
> +}
> +
> static void
> nfsd_file_do_unhash(struct nfsd_file *nf)
> {
> @@ -302,11 +309,14 @@ nfsd_file_put(struct nfsd_file *nf)
> return;
> }
>
> - filemap_flush(nf->nf_file->f_mapping);
> is_hashed = test_bit(NFSD_FILE_HASHED, &nf->nf_flags) != 0;
> - nfsd_file_put_noref(nf);
> - if (is_hashed)
> + if (!is_hashed) {
> + nfsd_file_flush(nf);
> + nfsd_file_put_noref(nf);
> + } else {
> + nfsd_file_put_noref(nf);
> nfsd_file_schedule_laundrette();
> + }
> if (atomic_long_read(&nfsd_filecache_count) >= NFSD_FILE_LRU_LIMIT)
> nfsd_file_gc();
> }
> @@ -327,6 +337,7 @@ nfsd_file_dispose_list(struct list_head *dispose)
> while(!list_empty(dispose)) {
> nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
> list_del(&nf->nf_lru);
> + nfsd_file_flush(nf);
> nfsd_file_put_noref(nf);
> }
> }
> @@ -340,6 +351,7 @@ nfsd_file_dispose_list_sync(struct list_head *dispose)
> while(!list_empty(dispose)) {
> nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
> list_del(&nf->nf_lru);
> + nfsd_file_flush(nf);
> if (!refcount_dec_and_test(&nf->nf_ref))
> continue;
> if (nfsd_file_free(nf))
> --
> 2.35.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] nfsd: Fix a write performance regression
2022-03-31 14:36 ` [PATCH 1/2] nfsd: Fix a write performance regression Jan Kara
@ 2022-03-31 14:38 ` Chuck Lever III
0 siblings, 0 replies; 4+ messages in thread
From: Chuck Lever III @ 2022-03-31 14:38 UTC (permalink / raw)
To: Jan Kara; +Cc: trondmy, Linux NFS Mailing List
> On Mar 31, 2022, at 10:36 AM, Jan Kara <jack@suse.cz> wrote:
>
> On Thu 31-03-22 09:54:01, trondmy@kernel.org wrote:
>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>
>> The call to filemap_flush() in nfsd_file_put() is there to ensure that
>> we clear out any writes belonging to a NFSv3 client relatively quickly
>> and avoid situations where the file can't be evicted by the garbage
>> collector. It also ensures that we detect write errors quickly.
>>
>> The problem is this causes a regression in performance for some
>> workloads.
>>
>> So try to improve matters by deferring writeback until we're ready to
>> close the file, and need to detect errors so that we can force the
>> client to resend.
>>
>> Tested-by: Jan Kara <jack@suse.cz>
>> Fixes: b6669305d35a ("nfsd: Reduce the number of calls to nfsd_file_gc()")
>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> Perphaps you could add:
>
> Link: https://lore.kernel.org/all/20220330103457.r4xrhy2d6nhtouzk@quack3.lan
>
> To make life of Thorsten Leemhuis doing regression tracking simpler :) (I
> think his automation will close the regression once it sees a patch like
> that merged).
I'll add that (no need to resend).
>
> Honza
>
>> ---
>> fs/nfsd/filecache.c | 18 +++++++++++++++---
>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>> index 8bc807c5fea4..9578a6317709 100644
>> --- a/fs/nfsd/filecache.c
>> +++ b/fs/nfsd/filecache.c
>> @@ -235,6 +235,13 @@ nfsd_file_check_write_error(struct nfsd_file *nf)
>> return filemap_check_wb_err(file->f_mapping, READ_ONCE(file->f_wb_err));
>> }
>>
>> +static void
>> +nfsd_file_flush(struct nfsd_file *nf)
>> +{
>> + if (nf->nf_file && vfs_fsync(nf->nf_file, 1) != 0)
>> + nfsd_reset_write_verifier(net_generic(nf->nf_net, nfsd_net_id));
>> +}
>> +
>> static void
>> nfsd_file_do_unhash(struct nfsd_file *nf)
>> {
>> @@ -302,11 +309,14 @@ nfsd_file_put(struct nfsd_file *nf)
>> return;
>> }
>>
>> - filemap_flush(nf->nf_file->f_mapping);
>> is_hashed = test_bit(NFSD_FILE_HASHED, &nf->nf_flags) != 0;
>> - nfsd_file_put_noref(nf);
>> - if (is_hashed)
>> + if (!is_hashed) {
>> + nfsd_file_flush(nf);
>> + nfsd_file_put_noref(nf);
>> + } else {
>> + nfsd_file_put_noref(nf);
>> nfsd_file_schedule_laundrette();
>> + }
>> if (atomic_long_read(&nfsd_filecache_count) >= NFSD_FILE_LRU_LIMIT)
>> nfsd_file_gc();
>> }
>> @@ -327,6 +337,7 @@ nfsd_file_dispose_list(struct list_head *dispose)
>> while(!list_empty(dispose)) {
>> nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
>> list_del(&nf->nf_lru);
>> + nfsd_file_flush(nf);
>> nfsd_file_put_noref(nf);
>> }
>> }
>> @@ -340,6 +351,7 @@ nfsd_file_dispose_list_sync(struct list_head *dispose)
>> while(!list_empty(dispose)) {
>> nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
>> list_del(&nf->nf_lru);
>> + nfsd_file_flush(nf);
>> if (!refcount_dec_and_test(&nf->nf_ref))
>> continue;
>> if (nfsd_file_free(nf))
>> --
>> 2.35.1
>>
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
--
Chuck Lever
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-31 14:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-31 13:54 [PATCH 1/2] nfsd: Fix a write performance regression trondmy
2022-03-31 13:54 ` [PATCH 2/2] nfsd: Clean up nfsd_file_put() trondmy
2022-03-31 14:36 ` [PATCH 1/2] nfsd: Fix a write performance regression Jan Kara
2022-03-31 14:38 ` Chuck Lever III
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).