Added a comment w.r.t. why "localhost" is hardcoded. I will also note that the kernel will
use these hardcoped values (I don't believe the kernel can do a name lookup and I wouldn't
want it to in this case), so the code doesn't want to use any other value, if a sysadmin were
to change the values for "localhost".

The problem with doing a name lookup in this daemon is that it assumes/expects it
to work. Often (an NFS mounted root fs is one example and a system configured to
use DNS before the local /etc/hosts file) are not yet working and an NFS mount is
expected to work.
(ie. In general I'd agree with you, but for NFS not so much...)

Looks fine to me now. Thanks, rick

Thanks for adding the strlen() check. I am going to be nitpicky and suggest an
error message be printed (not sure if the nfsd should fail or just log an error?),
since silently ignoring the "-V" argument could cause confusion too, I think?

Looks ok to me. (I'll admit I don't understand when it is useful, but that's ok.;-)
The one change I might suggest is a sanity check for the strlen() of the "-V" optarg
being <= MAXHOSTNAMELEN.

This one looks fine to me, rick.

First off, I should note that I am not the author of this code. If dfr@ could review this, that could be better.

Looks fine to me, although I'll admit I've never noticed there was a CLIENT_MAX in this code.
(I wasn't the author.)

Added an inline suggestion related to the man page update.

Assuming you resolve the man page issue that kib@ has commented on, this looks ok to me.

I can't seem to remember how to do inline comments (if I ever knew), so I'll stick it here...

W.r.t. ZFS, I have no idea. It appears that it would need to be a temporary property, but I can't see how/where
those get added?

nfs_dolock() is the front end for the NLM stuff, which I know nothing about.
I'll try emailing dfr@ and see if he can tell me what locking is required.
(You might have noticed that I don't have much use for the NLM;-)

Added an inline comment.

Added two inline comments.

Hmm, not sure. If you look at the code near the end of nfs_advlock() { around line#3100-3120 in head's sys/fs/nfsclient/nfs_clvnops.c } that isn't really
shown in the patch, then where the vnode gets VOP_UNLOCK()'d gets more convoluted. Such as before the lf_advlock() call.
I could do an errout: case so that many of the VOP_UNLOCK(); return(X); cases could become error = X; goto errout; but I'm not sure that
makes the code more readable.
If you think it does make it more readable, let me know and I'll change it.
{ Some like single return points and others don't like goto's. I have no strong opinion w.r.t. this. }

I just ran a test without the timeout in the loop waiting for a connection
attempt and the time it took for a failover was only 1minute more than
with that patch.

The problem is that, without the "timeout" it takes too long for the msleep() to wake up.
The "timeout" or whatever you prefer to call it is meant to set the upper bound on the
time that this loop waits for a TCP connection to complete to approximately
tv.tv_sec (the same timeout as set for SO_SNDTIMEO).

Added comments as kib@ suggested and used tvtohz(), so that tv_usec component is included in timeout.
(I have emailed kib@ one that uses the 1sec timeout and I put that here if he prefers that one.)

I commented inline. I'll admit I don't see spurious wakeup()s will be a problem.
I can (actually already have for the patch I have) include the tv_usec field in
the timeout calculation.
(I'll admit I get nervous fooling with things like time_uptime, which can go
negative in 2038 and assorted things related to overflow.)