svn commit: r346120 - head/sys/kern

[Conrad Meyer]

Hi Edward,

I have a question about this change below.

On Thu, Apr 11, 2019 at 4:22 AM Edward Tomasz Napierala
<trasz at freebsd.org> wrote:
>
> Author: trasz
> Date: Thu Apr 11 11:21:45 2019
> New Revision: 346120
> URL: https://svnweb.freebsd.org/changeset/base/346120
>
> Log:
>   Use shared vnode locks for the ELF interpreter.
>
> ...
>   Differential Revision:        https://reviews.freebsd.org/D19874
> ...
> Modified: head/sys/kern/imgact_elf.c
> ==============================================================================
> --- head/sys/kern/imgact_elf.c  Thu Apr 11 08:06:45 2019        (r346119)
> +++ head/sys/kern/imgact_elf.c  Thu Apr 11 11:21:45 2019        (r346120)
> ...
> -       NDINIT(nd, LOOKUP, LOCKLEAF | FOLLOW, UIO_SYSSPACE, file, curthread);
> +       flags = FOLLOW | LOCKSHARED | LOCKLEAF;
> +
> +again:
> +       NDINIT(nd, LOOKUP, flags, UIO_SYSSPACE, file, curthread);
>         if ((error = namei(nd)) != 0) {
> ...
> @@ -759,15 +762,30 @@ __elfN(load_file)(struct proc *p, const char *file, u_
> ...
> +       if (VOP_IS_TEXT(nd->ni_vp) == 0) {
> +               if (VOP_ISLOCKED(nd->ni_vp) != LK_EXCLUSIVE) {
> +                       /*
> +                        * LK_UPGRADE could have resulted in dropping
> +                        * the lock.  Just try again from the start,
> +                        * this time with exclusive vnode lock.
> +                        */
> +                       vput(nd->ni_vp);
> +                       flags &= ~LOCKSHARED;
> +                       goto again;

It's unclear to me why we don't attempt LK_UPGRADE first.  If upgrade
succeeds, we avoid an extra filesystem traversal (namei/lookup).  If
it fails, of course we can 'goto again' the same as we do
unconditionally here.

There was some discussion about the topic in the linked phabricator PR
with Konstantin, but I did not follow it fully.

On the one hand, perhaps VOP_IS_TEXT() is rarely false for common
interpreters anyway.  On the other hand, there is sort of a
renaissance of static linking happening.  So maybe the thought is,
!VOP_IS_TEXT is likely to be rare, and LK_UPGRADE success even more
rare, so why bother writing additional code for it?

Thanks,
Conrad

P.S., It is orthogonal to this discussion, but I don't see any reason
for VOP_IS_TEXT to be a vnode_if operation.  Neither it, nor
VOP_UNSET_TEXT, is ever specialized.  They simply check or clear the
VV_TEXT flag on the vnode's vflags, respectively.  It is common for
the kernel to reach out and interact with other vnode vflags directly;
e.g., pretty much all other VV_flags, like VV_ROOT.  The only
specialization of VOP_SET_TEXT is NFSclient, and it is unclear to me
why the same requirements NFS client has for setting VV_TEXT do not
apply universally.