get rid of Giant in nfs_lock.c

[rmacklem]

Summary

Giant is being deprecated, so this patch replaces it with a mutex called nfsinfolock_mtx.
A couple of malloc(..M_WAITOK)s are moved to places where the lock isn't held.

I also noticed that the code that does the wakeup() on p_nlminfo only held the PROC_LOCK(),
which wan not held for the code that used Giant.
As such, it seemed somewhat racey (although I doubt the NLM daemon would ever respond
quickly enough to beat the call to tsleep()).
I used a msleep(..PDROP) instead of the tsleep() and wrapped the code that does the wakeup()
with the nfsinfolock_mtx.
I cannot see anywhere else that the code that held Giant sleeps, so I think this is ok.

Test Plan

I ran the connectathon lock tests on an NFSv3 mount using rpc.lockd, rpc.statd and it seems ok.

[kib]

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.

[rmacklem]

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

[rmacklem]

This version of the patch recodes the loop so that I think it is more readable.
It does not really change the semantics in a significant way.
I chose not to unlock/relock the mutex around nfslock_send(), since the
unpatched version would normally only unlock/relock Giant at the tsleep() call.
(Yes, it is possible that there would have been a sleep and associated unlock/relock
of Giant in the unpatch nfslock_send() via the malloc(..M_WAITOK), but it wouldn't
normally have occurred.)

As such, I chose to only unlock/relock the mutex at the sleep call(), to retain the
locking behaviour that the unpatched code would normally have when using Giant.
(I am not that conversant with this code, but I thought that an unlock/relock around
nfslock_send() done every call might introduce a race w.r.t. handling of fields in p_nlminfo.)

[rmacklem]

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().

[rmacklem]

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.)

[kib]

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.

[rmacklem]

This patch is completely different than the previous one.
It prepares the source tree for removal of sys/nfs/nfs_lock.c,
since this code uses Giant and has not been running by default
since May 2008.

Post-May 2008, the only way to run sys/nfs/nfs_lock.c was to build
a kernel without "options NFSLOCKD" and also delete nfslockd.ko
from the kernel boot directory.
--> I suspect no one has been doing this, but I will ask on FreeBSD-current@.

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?

[kib]

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.

[rmacklem]

One file got missed for the previous version of the deletion patch.

[rmacklem]

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

[rmacklem]

This version of the patch just deleted p_nlminfo instead of
replacing it with p_spare.
The offsets in kern_thread.c have been adjusted, which the
comment indicates is sufficient for a change to "struct proc" in head.