Re: [PATCH] nfsd: allow reaping files that are still under writeback

[Trond Myklebust <trondmy@kernel.org>]

On Tue, 2023-02-14 at 10:16 -0500, Jeff Layton wrote:
> On Tue, 2023-02-14 at 10:00 -0500, Trond Myklebust wrote:
> > On Tue, 2023-02-14 at 14:48 +0000, Chuck Lever III wrote:
> > > 
> > > 
> > > > On Feb 13, 2023, at 3:23 PM, Jeff Layton <jlayton@kernel.org>
> > > > wrote:
> > > > 
> > > > There's no reason to delay reaping an nfsd_file just because
> > > > its
> > > > underlying inode is still under writeback. nfsd just relies on
> > > > client
> > > > activity or the local flusher threads to do writeback.
> > > > 
> > > > Holding the file open does nothing to facilitate that, nor does
> > > > it
> > > > help
> > > > with tracking errors. Just allow it to close and let the kernel
> > > > do
> > > > writeback as it normally would.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > 
> > > Thanks! Applied to topic-filecache-cleanups.
> > > 
> > > 
> > > > ---
> > > > fs/nfsd/filecache.c | 22 ----------------------
> > > > 1 file changed, 22 deletions(-)
> > > > 
> > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > > > index e6617431df7c..3b9a10378c83 100644
> > > > --- a/fs/nfsd/filecache.c
> > > > +++ b/fs/nfsd/filecache.c
> > > > @@ -296,19 +296,6 @@ nfsd_file_free(struct nfsd_file *nf)
> > > >         call_rcu(&nf->nf_rcu, nfsd_file_slab_free);
> > > > }
> > > > 
> > > > -static bool
> > > > -nfsd_file_check_writeback(struct nfsd_file *nf)
> > > > -{
> > > > -       struct file *file = nf->nf_file;
> > > > -       struct address_space *mapping;
> > > > -
> > > > -       if (!file || !(file->f_mode & FMODE_WRITE))
> > > > -               return false;
> > > > -       mapping = file->f_mapping;
> > > > -       return mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) ||
> > > > -               mapping_tagged(mapping,
> > > > PAGECACHE_TAG_WRITEBACK);
> > > > -}
> > > > -
> > > > static bool nfsd_file_lru_add(struct nfsd_file *nf)
> > > > {
> > > >         set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
> > > > @@ -438,15 +425,6 @@ nfsd_file_lru_cb(struct list_head *item,
> > > > struct list_lru_one *lru,
> > > >         /* We should only be dealing with GC entries here */
> > > >         WARN_ON_ONCE(!test_bit(NFSD_FILE_GC, &nf->nf_flags));
> > > > 
> > > > -       /*
> > > > -        * Don't throw out files that are still undergoing I/O
> > > > or
> > > > -        * that have uncleared errors pending.
> > > > -        */
> > > > -       if (nfsd_file_check_writeback(nf)) {
> > > > -               trace_nfsd_file_gc_writeback(nf);
> > > > -               return LRU_SKIP;
> > > > -       }
> > > > -
> > > >         /* If it was recently added to the list, skip it */
> > > >         if (test_and_clear_bit(NFSD_FILE_REFERENCED, &nf-
> > > > > nf_flags)) {
> > > >                 trace_nfsd_file_gc_referenced(nf);
> > > > -- 
> > > > 2.39.1
> > > > 
> > > 
> > > --
> > > Chuck Lever
> > > 
> > > 
> > > 
> > 
> > Wait... There is a good reason for wanting to do this in the case
> > of
> > NFS re-exports, since close() is a very expensive operation if the
> > file
> > has dirty data.
> > 
> 
> Fair enough. What if we added a new EXPORT_OP_FLUSH_ON_CLOSE flag
> that
> filesystems could set in the export_operations? Then we could make
> this
> behavior conditional on that being set?

Sure.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com