Message-ID: <e8f5b07200f197a2972beb0628df55ec92b627b5.camel@kernel.org>
Subject: Re: [PATCH] nfsd: allow reaping files that are still under writeback
From:   Jeff Layton <jlayton@kernel.org>
To:     Trond Myklebust <trondmy@kernel.org>,
        Chuck Lever III <chuck.lever@oracle.com>
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Date:   Tue, 14 Feb 2023 10:16:27 -0500
In-Reply-To: <d7eea987852cffb5fc719310ed01f771390d60d2.camel@kernel.org>
References: <20230213202346.291008-1-jlayton@kernel.org>
         <E1A055FD-45C3-47A0-A6CB-296C84985D43@oracle.com>
         <d7eea987852cffb5fc719310ed01f771390d60d2.camel@kernel.org>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

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:
> >=20
> >=20
> > > On Feb 13, 2023, at 3:23 PM, Jeff Layton <jlayton@kernel.org>
> > > wrote:
> > >=20
> > > 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.
> > >=20
> > > 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.
> > >=20
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> >=20
> > Thanks! Applied to topic-filecache-cleanups.
> >=20
> >=20
> > > ---
> > > fs/nfsd/filecache.c | 22 ----------------------
> > > 1 file changed, 22 deletions(-)
> > >=20
> > > 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)
> > > =A0=A0=A0=A0=A0=A0=A0=A0call_rcu(&nf->nf_rcu, nfsd_file_slab_free);
> > > }
> > >=20
> > > -static bool
> > > -nfsd_file_check_writeback(struct nfsd_file *nf)
> > > -{
> > > -=A0=A0=A0=A0=A0=A0=A0struct file *file =3D nf->nf_file;
> > > -=A0=A0=A0=A0=A0=A0=A0struct address_space *mapping;
> > > -
> > > -=A0=A0=A0=A0=A0=A0=A0if (!file || !(file->f_mode & FMODE_WRITE))
> > > -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return false;
> > > -=A0=A0=A0=A0=A0=A0=A0mapping =3D file->f_mapping;
> > > -=A0=A0=A0=A0=A0=A0=A0return mapping_tagged(mapping, PAGECACHE_TAG_DI=
RTY) ||
> > > -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0mapping_tagged(mapping,=
 PAGECACHE_TAG_WRITEBACK);
> > > -}
> > > -
> > > static bool nfsd_file_lru_add(struct nfsd_file *nf)
> > > {
> > > =A0=A0=A0=A0=A0=A0=A0=A0set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
> > > @@ -438,15 +425,6 @@ nfsd_file_lru_cb(struct list_head *item,
> > > struct list_lru_one *lru,
> > > =A0=A0=A0=A0=A0=A0=A0=A0/* We should only be dealing with GC entries =
here */
> > > =A0=A0=A0=A0=A0=A0=A0=A0WARN_ON_ONCE(!test_bit(NFSD_FILE_GC, &nf->nf_=
flags));
> > >=20
> > > -=A0=A0=A0=A0=A0=A0=A0/*
> > > -=A0=A0=A0=A0=A0=A0=A0 * Don't throw out files that are still undergo=
ing I/O or
> > > -=A0=A0=A0=A0=A0=A0=A0 * that have uncleared errors pending.
> > > -=A0=A0=A0=A0=A0=A0=A0 */
> > > -=A0=A0=A0=A0=A0=A0=A0if (nfsd_file_check_writeback(nf)) {
> > > -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0trace_nfsd_file_gc_writ=
eback(nf);
> > > -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return LRU_SKIP;
> > > -=A0=A0=A0=A0=A0=A0=A0}
> > > -
> > > =A0=A0=A0=A0=A0=A0=A0=A0/* If it was recently added to the list, skip=
 it */
> > > =A0=A0=A0=A0=A0=A0=A0=A0if (test_and_clear_bit(NFSD_FILE_REFERENCED, =
&nf-
> > > > nf_flags)) {
> > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0trace_nfsd_file_gc_re=
ferenced(nf);
> > > --=20
> > > 2.39.1
> > >=20
> >=20
> > --
> > Chuck Lever
> >=20
> >=20
> >=20
>=20
> 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.
>=20

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?
--=20
Jeff Layton <jlayton@kernel.org>