svn commit: r346120 - head/sys/kern

[Konstantin Belousov]

On Thu, Apr 11, 2019 at 08:59:14AM -0700, Conrad Meyer wrote:
> 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.
LK_UPGRADE drops the vnode lock, this was discussed in the phab review.
Perhaps you can do LK_TRYUPGRADE first, by the cost of even more
complicated code then was in the initial review, if fixed.

> 
> 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.
Statically linked binaries do not need interpreter.

> 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?
I do not understand the question.

> 
> 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.
VOP is bypassed, unlike direct vnode flags access.  As result, when you
execute file on nullfs, VV_TEXT is set on the lower vnode, instead of
the upper nullfs vnode.  If lower vnode is not marked by VV_TEXT, EBUSY
does not work.  This was described in the commit log where VOPs were
introduced.