On Tue, 25 Oct 2022, Chuck Lever III wrote: > > > 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. Just for completeness, I found #define test_bit(nr, addr) bitop(_test_bit, nr, addr) in linux/bitops.h. bitop() is defined just above and (I think) in the case where nr is constant and addr is not NULL, this becomes a call to const_test_bit(). In include/asm-generic/bitops/generic-non-atomic.h I found static __always_inline bool const_test_bit(unsigned long nr, const volatile unsigned long *addr) In the non-constant case it calls _test_bit() which is static __always_inline bool _test_bit(unsigned long nr, const volatile unsigned long *addr) But as an int that is known to be 0 or 1 is type-compatible with a bool, ht shouldn't really matter from a type perspective, only for a human readability perspective. Thanks, NeilBrown
