> On Oct 23, 2022, at 10:33 PM, NeilBrown <neilb@xxxxxxx> 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. The definitions of test_bit() I've seen return "int" which is why I wrote it this way. >> @@ -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. OK. I think I've figured out a way to do that and still deal safely with the test_bit() return type silliness. v5 coming soon. -- Chuck Lever
