zfs: fix spurious lock contention during path lookup

[mjg]

Summary

Path lookup on illumos uses zfs_fastaccesschk_execute, which in particular tries to elide all the hard work in the common case of checking for VEXEC while it is definitely allowed. It still takes the acl lock, unnecessarily.

The FreeBSD variant keeps calling what's effectively a slowpath lookup -- performing ZFS_ENTER, taking the acl lock and iterating acl list (if any). This can be trivially patched to also elide all the work + the lock.

Apart from being slower single-threaded, this induces significant lock contention. Sample wait times from doing an incremental make -j 40 bzImage on a kernel running with other patches on top:

before	after
223760259 (sx:zp->z_acl_lock)	2472269 (rw:vm object)
129273053 (sx:rrl->rr_lock)	2103785 (sleep mutex:vm page)
2471834 (lockmgr:zfs)	1980574 (lockmgr:zfs)
2108764 (rw:vm object)	1675503 (sx:rrl->rr_lock)
1283443 (spin mutex:sleepq chain)	999978 (rw:pmap pv list)
350059 (rw:pmap pv list)	616252 (sleep mutex:vnode interlock)

That is, acl lock contention is eliminated and rrl lock (ZFS_ENTER) goes significantly down.

Test Plan

I'm going to need someone to run a correctness test suite. I only stress tested the patch.

[sef]

A quick skim and the main question I had is: no acl check?

[mjg]

You can see the fast path is gated by ZFS_NO_EXECS_DENIED. Should anything be there to deny an exec to /someone/, fast path fails.

[sef]

Great, thanks! :) I'll go over it in more detail over the next few days.

[mav]

In D22224#485688, @mjg wrote:

You can see the fast path is gated by ZFS_NO_EXECS_DENIED. Should anything be there to deny an exec to /someone/, fast path fails.

I'm sorry, but I read that part different. As I see it, ZFS_NO_EXECS_DENIED makes access to be given fast when both mode and ACLs allow execution, that is good. But if any of them denies execution, then code execution goes further and starts checking user/group/other execution mode bits. But as Sean worried, I can't see how would this code (same as Illumos) handle the case when there is ACL for different specific user or group denies exec access. As I see, this code will just fall to "other" in case if neither main user or group match. Please correct me if I am wrong, I am not very familiar with this code.

[mjg]

Setting ACLs performs the following:

/*
 * Failure to allow is effectively a deny, so execute permission
 * is denied if it was never mentioned or if we explicitly
 * weren't allowed it.
 */
if (!an_exec_denied &&
    ((seen & ALL_MODE_EXECS) != ALL_MODE_EXECS ||
    (mode & ALL_MODE_EXECS) != ALL_MODE_EXECS))
        an_exec_denied = B_TRUE;

if (an_exec_denied)
        *pflags &= ~ZFS_NO_EXECS_DENIED;
else
        *pflags |= ZFS_NO_EXECS_DENIED;

Therefore, if *any* acl denies an exec to *someone* the flag is unset. If the flag is unset, fast path bails. If no acls deny exec, the flag is set and fastpath continues.

[mav]

In D22224#485861, @mjg wrote:

If the flag is unset, fast path bails. If no acls deny exec, the flag is set and fastpath continues.

According to this:

if (zdp->z_pflags & ZFS_NO_EXECS_DENIED)
        return (0);

, if the flag is set, function returns 0, which means fast path succeeds. If the flag is not set, then code execution continue fast path processing, just slightly slower.

[mjg]

The point was ACLs are always walked if needed which is not hard to verify and it answers the question.

[mav]

In D22224#486144, @mjg wrote:

The point was ACLs are always walked if needed which is not hard to verify and it answers the question.

I'm sorry, I still don't get it. Yes, ACLs were walked. If all of them allow execution, then everything is perfect. But what if some ACL for specific user/group denies execution? The code will not return success immediately, falling back to fast check of mode bits. And if mode bits allow execution, then function will return 0 (success). But what about that denying ACL?

[mjg]

I noted if /any/ acl denies execution, the newly added function fails. Then the caller is expected to fall back to the current code which will also perform an acl walk.