Overall, looks good! Just one question and a couple nits.
What's the performance of this operation like? Checking the fields is fast, obviously, but what about the freeing? Is that something we want to do while holding the global lock? If it's not, we could remove them from their lists and stick them on a to-free list, and then free them while the global lock isn't being held (though we'll have to re-hold the host locks to do that).
[nit]: for if statements that are this many lines, adding braces can improve readability. Just personal preference, though, feel free to disregard.
[nit]: see above
6124 NLM server holds vnodes too long
Review Request #84 — Created Aug. 14, 2015 and submitted
The NLM server creates a nlm_vhold structure for every vnode it touches to make sure the vnode is properly held when there are any active locks for the particular vnode. There one nlm_vhold structure allocated for every host/vnode pair, even in a case the number of locks on the particular vnode is higher than one. The nlm_vhold structure is not used to track all of the locks, it's purpose is just to mark a vnode as "touched" and held for the particular host.
Once a lock is released for the particular vnode, the NLM implementation does not free the nlm_vhold structure, since we are not sure whether this lock is the last one (and the vnode is no longer locked by the particular host), or not. To determine this, some expensive tests are needed, so we defer such tests to the nlm_gc() - the NLM garbage collector.
In the nlm_gc() nlm_vhold structures are scanned to see whether they are still needed or not (if not, they are freed). The problem is that the nlm_gc() checks nlm_vhold structs only for some idle hosts. The non-idle hosts are rarely checked for unused nlm_vhold structs and such vholds are never (or too late) released. This causes the particular vnodes are held for too long time, even the files are removed.
I ran the test from the bugreport to make sure the problem is no longer
reproducible. With the fix, the NLM garbage collector is properly releasing
the vhold structure, the vnode reference counter is decreased and the space is
For some background: the old (closed source) NLM apparently never dropped its vnode holds until the NFS export went away (i.e. unmount).
When designing the GC functionality Dan K. and I assumed it would be sufficient to GC those holds in "lazy" fashion, looking at the holds under a given NLM host only when it has been idle for a while.
The recent use case where someone would like deleted files to recover space faster rasises an interesting new requirement that the previous design does not satisfy.
The proposed fix makes this GC function much more "aggressive" (I wouldn't call it "lazy" anymore, as it now operates on every host).
I'm not sure that's the best approach. That could introduce quite a lot of activity in these data structures that wasn't there before.
(right? It's also just very different from the original "lazy" design)
What other approaches were considered for fixing this?
Another couple ideas (thinking as I write this):
Implement "last used" times for vnode holds like we do for hosts (update in vhold get), and only touch those vholds that have not been used recently.
Implement a notification method, i.e. some nlm_vnode_deleted() function to be called by file-system implementations (like ZFS) to tell NLM it might want to drop its vhold, if it has one.
(that might eliminate the need to make GC of vholds more aggressive)
Status: Closed (submitted)
commit b1087aec0c7377254423f3429c35960b116e8b34 Author: Marcel Telka <email@example.com> AuthorDate: Mon Aug 17 20:07:18 2015 +0200 Commit: Dan McDonald <firstname.lastname@example.org> CommitDate: Tue Aug 25 17:17:15 2015 -0400 6124 NLM server holds vnodes too long Reviewed by: Alek Pinchuk <email@example.com> Reviewed by: Dan Fields <firstname.lastname@example.org> Reviewed by: Paul Dagnelie <email@example.com> Reviewed by: Gordon Ross <Gordon.W.Ross@gmail.com> Approved by: Dan McDonald <firstname.lastname@example.org> :100644 100644 a10ba86... 0f57ec4... M usr/src/uts/common/klm/nlm_impl.c