This is very good question.  I do not know.  I just know that we have similarly complex functions run with the global lock held, for example nlm_unexport().  If we will found in the real life that this piece of the code generates some lock contention (or some other performance issue) we could improve it later.
I just tried to measure the nlm_vhold_destroy() and I found that it is about 25-35 times slower than AVL_NEXT() on this particular platform (average values from 10k+ nlm_vhold_destroy() runs and 400k+ AVL_NEXT() runs).  I'm not sure it is worth making the freeing of vholds more complex (using the to-free list, for example) to save this time.  As I said: once it is proven we need to improve it, we could do it later as a separate project.
Thanks.

That's a fair point.  I agree we probably don't need to worry about it right now.

My personal preference for this particular condition is without the braces, because the condition is already formatted in such a way that the readability should be good enough.  (And for me: less tokens, less confusion, usually :-)
Consider the difference:
  395 +                                if (nvp->nv_refcnt == 0 &&
  396 +                                    nvp->nv_vp->v_filocks == NULL &&
  397 +                                    nvp->nv_vp->v_shrlocks == NULL)

  395 +                                if ((nvp->nv_refcnt == 0) &&
  396 +                                    (nvp->nv_vp->v_filocks == NULL) &&
  397 +                                    (nvp->nv_vp->v_shrlocks == NULL))

I think it is almost the same.
But, OTOH, I have no strong preference for going without the braces, so in a case you (or somebody else) really wants to see the braces there, I'll add them.  Just let me know.

Sorry, I meant adding curly braces around the body of the if statement:
395 +                                if (nvp->nv_refcnt == 0 &&
396 +                                    nvp->nv_vp->v_filocks == NULL &&
397 +                                    nvp->nv_vp->v_shrlocks == NULL) {
398 +                                        nlm_vhold_destroy(hostp, nvp);
399 +                                }


But, again, not a big deal.

Ah, that one.  Yes, this definitely makes sense.  Braces added.

See above :-).

I just tested OpenIndiana 151a8 with the closed source NLM implementation and bug 6124 is not reproducible there.  It looks like the old closed source NLM dropped the vnode hold immediately once the file was deleted, because ZFS reclaimed the space occupied by the deleted file in less than 10 seconds after the file was deleted (the space reclaim is even faster than with the new open source NLM with my fix).
Yes, the proposed fix makes the vhold release more aggressive than before, but it still could be considered lazy.  The previous "lazy" implementation was too lazy, so lazy that some vholds were never released.  My new implementation is a bit less lazy, but it still releases vholds usually after many minutes of the waiting - the nlm_gc() idle period is 5 minutes by default, IIRC.
The "last used" idea:  The vhold/vnode release is cheap, so I do not see any gain with such proposal/implementation.
The "notification" idea:  This would need a modification in all other filesystems (or in the generic vnode manipulation/VOP routines) for unknown gain.  That's hardly justifiable.  But maybe I do not see something obvious...

I don't like that the GC thread now visits every vhold in the collection on each pass (which it didn't do before).

However, given that it's not doing anything expensive with each vhold, I'll "hold my nose".