Re: Do the pidhashtbl_locks added by r340742 need to be sx locks?

On 4/11/19, Rick Macklem <rmacklem_at_uoguelph.ca> wrote:
> Hi,
>
> I finally got around to looking at what effect replacing pfind_locked()
> with
> pfind() has for the NFSv4 client and it is broken.
>
> The problem is that the NFS code needs to call some variant of "pfind()"
> while
> holding a mutex lock. The current _pfind() code uses the pidhashtbl_locks,
> which are "sx" locks.
>
> There are a few ways to fix this:
> 1 - Create a custom version of _pfind() for the NFS client with the sx_X()
> calls
>       removed, plus replace the locking of allproc_lock with locking of all
> the
>       pidhashtbl_locks, so that the "sx" locks are acquired before the
> mutex.
>       --> Not very efficient, but since it is only done once/sec, I can live
> with it.
> 2 - Similar to the above, but still lock the allproc_lock and use a loop of
>      FOREACH_PROC_IN_SYSTEM(p) instead of a hash list for the pid in the
>      custom pfind(). (I don't know if this would be preferable to locking
> all
>      the pidhashtbl_locks for other users of pfind()?)
> 3 - Convert the pidhashtbl_locks to mutexes. Then the NFS client doesn't
> need
>      to acquire any proc related locks and it just works.
>      I can't see anywhere that "sleeps" while holding the pidhashtbl_locks,
> so I
>      think they can be converted, although I haven't tried it yet?
>
> From my perspective, #3 seems the better solution.
> What do others think?
>

Changing the lock type to rwlock may be doable and worthwhile on its own,
but I don't think it would constitute the right fix.

Preferably there would be an easy to use mechanism which allows
registering per-process callbacks. Currently it can be somewhat emulated
with EVENTHANDLERs, but it would give calls for all exiting processes, not
only the ones of interest. Then there would be no need to periodically
scan as you would always get notified on process exit.

Note the current code does not ref processes it is interested in any
manner and just performs a timestamp check to see if it got the one it
expected (with pid reuse in mind).

So I think a temporary hack which will do the trick will take the current
approach further: rely on struct proc being type-stable (i.e. never being
freed) and also store the pointer. You can always safely PROC_LOCK it, do
checks to see the proc is alive and has the right timestamp, all without
needing to pfind or similar.

-- 
Mateusz Guzik <mjguzik gmail.com>