> On Oct 24, 2022, at 12:57 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > 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. So I cut-and-pasted the approach you used in the patch you sent a few weeks ago. I don't object to replacing that... but I don't see exactly where you guys are going with this. >>> + >>> 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> -- Chuck Lever
