| Message ID | 20221031113742.26480-6-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: clean up refcounting in the filecache | expand |
> On Oct 31, 2022, at 7:37 AM, Jeff Layton <jlayton@kernel.org> 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? > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > 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
On Mon, 2022-10-31 at 21:00 +0000, Chuck Lever III wrote: > > > On Oct 31, 2022, at 7:37 AM, Jeff Layton <jlayton@kernel.org> 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@kernel.org> > > --- > > 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 > > >
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
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. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/filecache.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)