> On Jun 23, 2022, at 6:33 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote: > > On Thu, Jun 23, 2022 at 05:27:20PM +0000, Chuck Lever III wrote: >> Also I just found Neil's nice rhashtable explainer: >> >> https://lwn.net/Articles/751374/ >> >> Where he writes that: >> >>> Sometimes you might want a hash table to potentially contain >>> multiple objects for any given key. In that case you can use >>> "rhltables" — rhashtables with lists of objects. >> >> I believe that is the case for the filecache. The hash value is >> computed based on the inode pointer, and therefore there can be more >> than one nfsd_file object for a particular inode (depending on who >> is opening and for what access). So I think filecache needs to use >> rhltable, not rhashtable. Any thoughts from rhashtable experts? > > Huh, I assumed the file cache was just hashing the whole key so that > every object in the rht has it's own unique key and hash and there's > no need to handle multiple objects per key... > > What are you trying to optimise by hashing only the inode *pointer* > in the nfsd_file object keyspace? Well, this design is inherited from the current filecache implementation. It assumes that all nfsd_file objects that refer to the same inode will always get chained into the same bucket. That way: 506 static void 507 __nfsd_file_close_inode(struct inode *inode, unsigned int hashval, 508 struct list_head *dispose) 509 { 510 struct nfsd_file *nf; 511 struct hlist_node *tmp; 512 513 spin_lock(&nfsd_file_hashtbl[hashval].nfb_lock); 514 hlist_for_each_entry_safe(nf, tmp, &nfsd_file_hashtbl[hashval].nfb_head, nf_node) { 515 if (inode == nf->nf_inode) 516 nfsd_file_unhash_and_release_locked(nf, dispose); 517 } 518 spin_unlock(&nfsd_file_hashtbl[hashval].nfb_lock); 519 } nfsd_file_close_inode() can lock one hash bucket and just walk that hash chain to find all the nfsd_file's associated with a particular in-core inode. Actually I don't think there's any other reason to keep that hashing design, but Jeff can confirm that. So I guess we could use rhltable and keep the nfsd_file items for the same inode on the same hash list? I'm not sure it's worth the trouble: this part of filecache isn't really on the hot path. -- Chuck Lever