Re: [PATCH v4 3/7] NFSD: Add an NFSD_FILE_GC flag to enable nfsd_file garbage collection
From: NeilBrown <hidden>
Date: 2022-10-24 02:34:03
On Wed, 19 Oct 2022, Chuck Lever wrote:
quoted hunk ↗ jump to hunk
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 <redacted> Tested-by: Jeff Layton <jlayton@kernel.org> Signed-off-by: Chuck Lever <redacted> --- 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?
quoted hunk ↗ jump to hunk
- } 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