On Mon, 2022-10-24 at 13:33 +1100, NeilBrown wrote: > 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. Agreed, and there's a clear race above, now that I look more closely. If nf_ref is 3 and two puts are racing then neither of them will call nfsd_file_unhash_and_put. We really should be letting the outcome of the decrement drive things (like you say below). > 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). > This does seem a more sensible approach. That would go a long way toward simplifying nfsd_file_put. > > > + > > 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 -- Jeff Layton <jlayton@xxxxxxxxxx>
