I wonder why would you need a lock there at all ? The only thing that could go wrong, from my reading, is the handling of p->p_nlmino. But you do not take the nfsinfolock_mtx in nlminfo_release. Also, even if locked, what if nlminfo_release is executed right after we reassigned the p_nlminfo value ? Wouldn't it destroy our memory ?

That said, if keeping the current approach, I suggest to restructure the loop in nfs_dolock(). Note that you in fact need to create a new __lock_msg each time you start the loop iteration. If you drop the lock at the loop entry, malloc, re-acquire the lock, I believe the logic would become clearer.

I think your understanding is correct, in that the locking is to protect
use of p_nlminfo.
If I read it correctly, nlminfo_release() is only called from code in exit1()
when the process is terminated, so it would no longer be doing any locking.
(It is kind of a weird hack, where nlminfo_release_p is set to nlminfo_release()
and then nlminfo_release_p is checked for non-NULL and called during process exit.)

As for restructuring the loop, I can do that. I agree that the current loop is
kinda silly, since it only interates for the one case where the "continue;" is.
I went with the "minimal change" patch, so that the changes would be clearer,
but I'll come up with one later to-day that cleans up the loop.
(Just fyi, I wasn't the author of this code.)

Thanks for the comments, rick

Further to kib@'s comment, here is how I understand the algorithm.
nfs_dolock() is called when a process does a file lock request on an NFSv3
mount that doesn't have the "nolockd" mount option.
It locked Giant to protect the code in lines 283-294 (before patch), but
except for the rare case where the malloc(M_WAITOK) in nfslock_send()
slept, held it until the tsleep() call at line#325.

Then, normally nfslockdans() is called by the rpc.lockd process/thread
during the 20sec tsleep(). It checked fields in p_nlminfo for a match, updated
some fields if it was a match and then did a wakeup(). { Was there something
in the old code that would have acquired Giant as a part of the call to nfslockdans()
through the lock device d_write() function? }
If not, there did not seem to be any guarantee that the code in nfslockdans() wouldn't
be executed between lines 302-325 (before the tsleep() call).

As such, I added locking of the nfsinfolock_mtx in nfslockdans() to ensure it was
serialized with the code between line 283-325 in nfs_dolock(). This might be overkill,
but seems safe (and necessary if the old code held Giant when the d_write() device
function was called by rpc.lockd, that then calls nfslockdans()).

As noted before, nlminfo_release() does not get called until the process is in exit1().

Well, you might get a chuckle about this...
Turns out this code that I've been hacking to get rid of Giant
never gets executed.

Back around FreeBSD7, dfr@ committed a kernel based nlm.
At first, it was only enabled via a "-k" option, but it now is enabled by default.
As such, the only time this code gets executed is when both

• options NFSLOCKD is not specified in the kernel config

and

• nfslockd.ko can't be loaded

Which is basically never.

Looks like the correct patch is to just rip it out.
(It defines a couple of global variables that need to be defined
elsewhere and, although nlminfo_release() does still get called,
it doesn't do anything, since p_nlminfo is always NULL.)

In D22933#502585, @rmacklem wrote:

Well, you might get a chuckle about this...
Turns out this code that I've been hacking to get rid of Giant
never gets executed.

Back around FreeBSD7, dfr@ committed a kernel based nlm.
At first, it was only enabled via a "-k" option, but it now is enabled by default.
As such, the only time this code gets executed is when both

• options NFSLOCKD is not specified in the kernel config

and

• nfslockd.ko can't be loaded

Which is basically never.

Looks like the correct patch is to just rip it out.
(It defines a couple of global variables that need to be defined
elsewhere and, although nlminfo_release() does still get called,
it doesn't do anything, since p_nlminfo is always NULL.)

So will you post the patch that removes the dead code ? I think it is quite positive to remove p_nlminfo and three lines from kern_exit.c.

In D22933#502845, @rmacklem wrote:

I replaced p_nlminfo with p_share.
If the comment in sys/kern/kern_thread.c is correct, p_nlminfo can be
deleted in head so long as the static offsets are adjusted accordingly.
Is deleting p_nlminfo preferred over replacing it with p_share?

There is no point in keeping p_share around.

I'll update the patch with a version that doesn't have p_spare in a day or two.