> On Jun 12, 2022, at 3:22 AM, Wang Yugui <wangyugui@xxxxxxxxxxxx> wrote: > > NFSv4 need to close a file completely (no lingering open) when it does > a CLOSE or DELEGRETURN. > > When multiple NFSv4/OPEN from different clients, we need to check the > reference count. The flowing reference-count-check change the behavior > of NFSv3 nfsd_rename()/nfsd_unlink() too. > > Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=387 > Signed-off-by: Wang Yugui <wangyugui@xxxxxxxxxxxx> > --- > TO-CHECK: > 1) NFSv3 nfsd_rename()/nfsd_unlink() feature change is OK? > 2) Can we do better performance than nfsd_file_close_inode_sync()? > 3) nfsd_file_close_inode_sync()->nfsd_file_close_inode() in nfsd4_delegreturn() > => 'Text file busy' about 4s > 4) reference-count-check : refcount_read(&nf->nf_ref) <= 1 or ==0? > nfsd_file_alloc() refcount_set(&nf->nf_ref, 1); > > fs/nfsd/filecache.c | 2 +- > fs/nfsd/nfs4state.c | 4 ++++ > 2 files changed, 5 insertions(+), 1 deletion(-) I suppose I owe you (and Frank) a progress report on #386. I've fixed the LRU algorithm and added some observability features to measure how the fix impacts the cache's efficiency for NFSv3 workloads. These new features show that the hit rate and average age of cache items goes down after the fix is applied. I'm trying to understand if I've done something wrong or if the fix is supposed to do that. To handle the case of hundreds of thousands of open files more efficiently, I'd like to convert the filecache to use rhashtable. > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index 9cb2d590c036..8890a8fa7402 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -512,7 +512,7 @@ __nfsd_file_close_inode(struct inode *inode, unsigned int hashval, > > spin_lock(&nfsd_file_hashtbl[hashval].nfb_lock); > hlist_for_each_entry_safe(nf, tmp, &nfsd_file_hashtbl[hashval].nfb_head, nf_node) { > - if (inode == nf->nf_inode) > + if (inode == nf->nf_inode && refcount_read(&nf->nf_ref) <= 1) > nfsd_file_unhash_and_release_locked(nf, dispose); > } > spin_unlock(&nfsd_file_hashtbl[hashval].nfb_lock); > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 9409a0dc1b76..be4b480f5914 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -6673,6 +6673,8 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > > /* put reference from nfs4_preprocess_seqid_op */ > nfs4_put_stid(&stp->st_stid); > + > + nfsd_file_close_inode_sync(d_inode(cstate->current_fh.fh_dentry)); IIUC this closes /all/ nfsd_file objects that refer to the same inode. CLOSE is supposed to release only the state held by a particular user on a particular client. This is like closing a file descriptor; you release the resources associated with that file descriptor, and other users of the inode are not supposed to be affected. Thus using an inode-based API in nfsd4_close/delegreturn seems like the wrong approach. > out: > return status; > } > @@ -6702,6 +6704,8 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > destroy_delegation(dp); > put_stateid: > nfs4_put_stid(&dp->dl_stid); > + > + nfsd_file_close_inode_sync(d_inode(cstate->current_fh.fh_dentry)); > out: > return status; > } > -- > 2.36.1 > -- Chuck Lever
