Page MenuHomeFreeBSD

zfs: fix spurious lock contention during path lookup
Needs ReviewPublic

Authored by mjg on Sun, Nov 3, 12:54 AM.

Details

Reviewers
mav
mmacy
sef
avg
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:

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 27319

Event Timeline

mjg created this revision.Sun, Nov 3, 12:54 AM
mjg edited the summary of this revision. (Show Details)Sun, Nov 3, 12:54 AM
mjg edited the summary of this revision. (Show Details)
mjg edited the summary of this revision. (Show Details)Sun, Nov 3, 12:57 AM
mjg edited the summary of this revision. (Show Details)
sef added a comment.Sun, Nov 3, 12:58 AM

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

mjg added a comment.Sun, Nov 3, 1:00 AM

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 added a comment.Sun, Nov 3, 1:01 AM

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

mav added a reviewer: avg.Mon, Nov 4, 12:38 AM
mav added a comment.Mon, Nov 4, 1:24 AM
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 added a comment.Mon, Nov 4, 1:41 AM

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 added a comment.Tue, Nov 5, 3:20 AM
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 added a comment.Tue, Nov 5, 4:44 AM

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

mav added a comment.Tue, Nov 5, 2:20 PM
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 added a comment.Sat, Nov 16, 12:06 AM

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.