On Wed, 19 Oct 2022, Chuck Lever wrote: > NFSv4 operations manage the lifetime of nfsd_file items they use by > means of NFSv4 OPEN and CLOSE. Hence there's no need for them to be > garbage collected. > > Introduce a mechanism to enable garbage collection for nfsd_file > items used only by NFSv2/3 callers. > > Note that the change in nfsd_file_put() ensures that both CLOSE and > DELEGRETURN will actually close out and free an nfsd_file on last > reference of a non-garbage-collected file. > > Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=394 > Suggested-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> > Tested-by: Jeff Layton <jlayton@xxxxxxxxxx> > Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx> > --- > fs/nfsd/filecache.c | 61 +++++++++++++++++++++++++++++++++++++++++++++------ > fs/nfsd/filecache.h | 3 +++ > fs/nfsd/nfs3proc.c | 4 ++- > fs/nfsd/trace.h | 3 ++- > fs/nfsd/vfs.c | 4 ++- > 5 files changed, 63 insertions(+), 12 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index b7aa523c2010..87fce5c95726 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -63,6 +63,7 @@ struct nfsd_file_lookup_key { > struct net *net; > const struct cred *cred; > unsigned char need; > + unsigned char gc:1; > enum nfsd_file_lookup_type type; > }; > > @@ -162,6 +163,8 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg, > return 1; > if (!nfsd_match_cred(nf->nf_cred, key->cred)) > return 1; > + if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc) > + return 1; > if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) == 0) > return 1; > break; > @@ -297,6 +300,8 @@ nfsd_file_alloc(struct nfsd_file_lookup_key *key, unsigned int may) > nf->nf_flags = 0; > __set_bit(NFSD_FILE_HASHED, &nf->nf_flags); > __set_bit(NFSD_FILE_PENDING, &nf->nf_flags); > + if (key->gc) > + __set_bit(NFSD_FILE_GC, &nf->nf_flags); > nf->nf_inode = key->inode; > /* nf_ref is pre-incremented for hash table */ > refcount_set(&nf->nf_ref, 2); > @@ -428,16 +433,27 @@ nfsd_file_put_noref(struct nfsd_file *nf) > } > } > > +static void > +nfsd_file_unhash_and_put(struct nfsd_file *nf) > +{ > + if (nfsd_file_unhash(nf)) > + nfsd_file_put_noref(nf); > +} > + > void > nfsd_file_put(struct nfsd_file *nf) > { > might_sleep(); > > - nfsd_file_lru_add(nf); > + if (test_bit(NFSD_FILE_GC, &nf->nf_flags) == 1) Clearly this is a style choice on which sensible people might disagree, but I much prefer to leave out the "== 1" That is what most callers in fs/nfsd/ do - only exceptions are here in filecache.c. Even "!= 0" would be better than "== 1". I think test_bit() is declared as a bool, but it is hard to be certain. > + nfsd_file_lru_add(nf); > + else if (refcount_read(&nf->nf_ref) == 2) > + nfsd_file_unhash_and_put(nf); Tests on the value of a refcount are almost always racy. I suspect there is an implication that as NFSD_FILE_GC is not set, this *must* be hashed which implies there is guaranteed to be a refcount from the hashtable. So this is really a test to see if the pre-biased refcount is one. The safe way to test if a refcount is 1 is dec_and_test. i.e. linkage from the hash table should not count as a reference (in the not-GC case). Lookup in the hash table should fail if the found entry cannot achieve an inc_not_zero. When dec_and_test says the refcount is zero, we remove from the hash table (certain that no new references will be taken). > + > if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) == 0) { > nfsd_file_flush(nf); > nfsd_file_put_noref(nf); This seems weird. If the file was unhashed above (because nf_ref was 2), it would now not be flushed. Why don't we want it to be flushed in that case? > - } else if (nf->nf_file) { > + } else if (nf->nf_file && test_bit(NFSD_FILE_GC, &nf->nf_flags) == 1) { > nfsd_file_put_noref(nf); > nfsd_file_schedule_laundrette(); > } else > @@ -1017,12 +1033,14 @@ nfsd_file_is_cached(struct inode *inode) > > static __be32 > nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > - unsigned int may_flags, struct nfsd_file **pnf, bool open) > + unsigned int may_flags, struct nfsd_file **pnf, > + bool open, int want_gc) I too would prefer "bool" for all intstance of gc and want_gc. NeilBrown
