Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/ad-namecache]: src/sys/kern Push the vnode locking in namei() about as f...



details:   https://anonhg.NetBSD.org/src/rev/50ffcf3cc48e
branches:  ad-namecache
changeset: 982983:50ffcf3cc48e
user:      ad <ad%NetBSD.org@localhost>
date:      Thu Jan 16 16:45:30 2020 +0000

description:
Push the vnode locking in namei() about as far back as it will go.

diffstat:

 sys/kern/vfs_lookup.c |  306 ++++++++++++++++++++++++++++---------------------
 1 files changed, 174 insertions(+), 132 deletions(-)

diffs (truncated from 601 to 300 lines):

diff -r d04df17660be -r 50ffcf3cc48e sys/kern/vfs_lookup.c
--- a/sys/kern/vfs_lookup.c     Tue Jan 14 11:08:01 2020 +0000
+++ b/sys/kern/vfs_lookup.c     Thu Jan 16 16:45:30 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: vfs_lookup.c,v 1.212 2019/07/18 09:39:40 hannken Exp $ */
+/*     $NetBSD: vfs_lookup.c,v 1.212.4.1 2020/01/16 16:45:30 ad Exp $  */
 
 /*
  * Copyright (c) 1982, 1986, 1989, 1993
@@ -37,7 +37,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: vfs_lookup.c,v 1.212 2019/07/18 09:39:40 hannken Exp $");
+__KERNEL_RCSID(0, "$NetBSD: vfs_lookup.c,v 1.212.4.1 2020/01/16 16:45:30 ad Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_magiclinks.h"
@@ -710,8 +710,6 @@
                return ENOTDIR;
        }
 
-       vn_lock(startdir, LK_EXCLUSIVE | LK_RETRY);
-
        *startdir_ret = startdir;
        return 0;
 }
@@ -749,15 +747,17 @@
        size_t linklen;
        int error;
 
-       KASSERT(VOP_ISLOCKED(searchdir) == LK_EXCLUSIVE);
-       KASSERT(VOP_ISLOCKED(foundobj) == LK_EXCLUSIVE);
        if (ndp->ni_loopcnt++ >= MAXSYMLINKS) {
                return ELOOP;
        }
+
+       vn_lock(foundobj, LK_EXCLUSIVE | LK_RETRY);
        if (foundobj->v_mount->mnt_flag & MNT_SYMPERM) {
                error = VOP_ACCESS(foundobj, VEXEC, cnp->cn_cred);
-               if (error != 0)
+               if (error != 0) {
+                       VOP_UNLOCK(foundobj);
                        return error;
+               }
        }
 
        /* FUTURE: fix this to not use a second buffer */
@@ -771,6 +771,7 @@
        auio.uio_resid = MAXPATHLEN;
        UIO_SETUP_SYSSPACE(&auio);
        error = VOP_READLINK(foundobj, &auio, cnp->cn_cred);
+       VOP_UNLOCK(foundobj);
        if (error) {
                PNBUF_PUT(cp);
                return error;
@@ -807,14 +808,11 @@
        /* we're now starting from the beginning of the buffer again */
        cnp->cn_nameptr = ndp->ni_pnbuf;
 
-       /* must unlock this before relocking searchdir */
-       VOP_UNLOCK(foundobj);
-
        /*
         * Check if root directory should replace current directory.
         */
        if (ndp->ni_pnbuf[0] == '/') {
-               vput(searchdir);
+               vrele(searchdir);
                /* Keep absolute symbolic links inside emulation root */
                searchdir = ndp->ni_erootdir;
                if (searchdir == NULL ||
@@ -825,7 +823,6 @@
                        searchdir = ndp->ni_rootdir;
                }
                vref(searchdir);
-               vn_lock(searchdir, LK_EXCLUSIVE | LK_RETRY);
                while (cnp->cn_nameptr[0] == '/') {
                        cnp->cn_nameptr++;
                        ndp->ni_pathlen--;
@@ -833,7 +830,6 @@
        }
 
        *newsearchdir_ret = searchdir;
-       KASSERT(VOP_ISLOCKED(searchdir) == LK_EXCLUSIVE);
        return 0;
 }
 
@@ -933,19 +929,20 @@
 lookup_once(struct namei_state *state,
            struct vnode *searchdir,
            struct vnode **newsearchdir_ret,
-           struct vnode **foundobj_ret)
+           struct vnode **foundobj_ret,
+           bool *newsearchdir_locked_ret)
 {
        struct vnode *tmpvn;            /* scratch vnode */
        struct vnode *foundobj;         /* result */
        struct mount *mp;               /* mount table entry */
        struct lwp *l = curlwp;
+       bool searchdir_locked = false;
        int error;
 
        struct componentname *cnp = state->cnp;
        struct nameidata *ndp = state->ndp;
 
        KASSERT(cnp == &ndp->ni_cnd);
-       KASSERT(VOP_ISLOCKED(searchdir) == LK_EXCLUSIVE);
        *newsearchdir_ret = searchdir;
 
        /*
@@ -977,9 +974,7 @@
                        if (ndp->ni_rootdir != rootvnode) {
                                int retval;
 
-                               VOP_UNLOCK(searchdir);
                                retval = vn_isunder(searchdir, ndp->ni_rootdir, l);
-                               vn_lock(searchdir, LK_EXCLUSIVE | LK_RETRY);
                                if (!retval) {
                                    /* Oops! We got out of jail! */
                                    log(LOG_WARNING,
@@ -988,12 +983,11 @@
                                        p->p_pid, kauth_cred_geteuid(l->l_cred),
                                        p->p_comm);
                                    /* Put us at the jail root. */
-                                   vput(searchdir);
+                                   vrele(searchdir);
                                    searchdir = NULL;
                                    foundobj = ndp->ni_rootdir;
                                    vref(foundobj);
                                    vref(foundobj);
-                                   vn_lock(foundobj, LK_EXCLUSIVE | LK_RETRY);
                                    *newsearchdir_ret = foundobj;
                                    *foundobj_ret = foundobj;
                                    error = 0;
@@ -1006,18 +1000,19 @@
                        tmpvn = searchdir;
                        searchdir = searchdir->v_mount->mnt_vnodecovered;
                        vref(searchdir);
-                       vput(tmpvn);
-                       vn_lock(searchdir, LK_EXCLUSIVE | LK_RETRY);
+                       vrele(tmpvn);
                        *newsearchdir_ret = searchdir;
                }
        }
 
        /*
         * We now have a segment name to search for, and a directory to search.
-        * Our vnode state here is that "searchdir" is held and locked.
+        * Our vnode state here is that "searchdir" is held.
         */
 unionlookup:
        foundobj = NULL;
+       vn_lock(searchdir, LK_EXCLUSIVE | LK_RETRY);
+       searchdir_locked = true;
        error = VOP_LOOKUP(searchdir, &foundobj, cnp);
 
        if (error != 0) {
@@ -1034,7 +1029,7 @@
                        searchdir = searchdir->v_mount->mnt_vnodecovered;
                        vref(searchdir);
                        vput(tmpvn);
-                       vn_lock(searchdir, LK_EXCLUSIVE | LK_RETRY);
+                       searchdir_locked = false;
                        *newsearchdir_ret = searchdir;
                        goto unionlookup;
                }
@@ -1089,84 +1084,99 @@
        }
 
        /*
-        * "searchdir" is locked and held, "foundobj" is held,
-        * they may be the same vnode.
-        */
-       if (searchdir != foundobj) {
-               if (cnp->cn_flags & ISDOTDOT)
-                       VOP_UNLOCK(searchdir);
-               error = vn_lock(foundobj, LK_EXCLUSIVE);
-               if (cnp->cn_flags & ISDOTDOT)
-                       vn_lock(searchdir, LK_EXCLUSIVE | LK_RETRY);
-               if (error != 0) {
-                       vrele(foundobj);
-                       goto done;
-               }
-       }
-
-       /*
-        * Check to see if the vnode has been mounted on;
-        * if so find the root of the mounted file system.
+        * Do an unlocked check to see if the vnode has been mounted on; if
+        * so find the root of the mounted file system.
         */
        KASSERT(searchdir != NULL);
-       while (foundobj->v_type == VDIR &&
-              (mp = foundobj->v_mountedhere) != NULL &&
-              (cnp->cn_flags & NOCROSSMOUNT) == 0) {
-
-               KASSERT(searchdir != foundobj);
-
-               error = vfs_busy(mp);
-               if (error != 0) {
-                       vput(foundobj);
-                       goto done;
-               }
-               if (searchdir != NULL) {
-                       VOP_UNLOCK(searchdir);
-               }
-               vput(foundobj);
-               error = VFS_ROOT(mp, &foundobj);
-               vfs_unbusy(mp);
-               if (error) {
-                       if (searchdir != NULL) {
-                               vn_lock(searchdir, LK_EXCLUSIVE | LK_RETRY);
+       if (foundobj->v_type == VDIR && foundobj->v_mountedhere != NULL &&
+           (cnp->cn_flags & NOCROSSMOUNT) == 0) {
+               /*
+                * "searchdir" is locked and held, "foundobj" is held,
+                * they may be the same vnode.
+                */
+               if (searchdir != foundobj) {
+                       if (vn_lock(foundobj, LK_EXCLUSIVE | LK_NOWAIT) != 0) {
+                               if (cnp->cn_flags & ISDOTDOT)
+                                       VOP_UNLOCK(searchdir);
+                               error = vn_lock(foundobj, LK_EXCLUSIVE);
+                               if (cnp->cn_flags & ISDOTDOT)
+                                       vn_lock(searchdir, LK_EXCLUSIVE | LK_RETRY);
+                               if (error != 0) {
+                                       vrele(foundobj);
+                                       goto done;
+                               }
                        }
-                       goto done;
                }
-               /*
-                * Avoid locking vnodes from two filesystems because
-                * it's prone to deadlock, e.g. when using puffs.
-                * Also, it isn't a good idea to propagate slowness of
-                * a filesystem up to the root directory. For now,
-                * only handle the common case, where foundobj is
-                * VDIR.
-                *
-                * In this case set searchdir to null to avoid using
-                * it again. It is not correct to set searchdir ==
-                * foundobj here as that will confuse the caller.
-                * (See PR 40740.)
-                */
-               if (searchdir == NULL) {
-                       /* already been here once; do nothing further */
-               } else if (foundobj->v_type == VDIR) {
-                       vrele(searchdir);
-                       *newsearchdir_ret = searchdir = NULL;
-               } else {
-                       VOP_UNLOCK(foundobj);
-                       vn_lock(searchdir, LK_EXCLUSIVE | LK_RETRY);
-                       vn_lock(foundobj, LK_EXCLUSIVE | LK_RETRY);
+               while (foundobj->v_type == VDIR &&
+                      (mp = foundobj->v_mountedhere) != NULL &&
+                      (cnp->cn_flags & NOCROSSMOUNT) == 0) {
+
+                       KASSERT(searchdir != foundobj);
+
+                       error = vfs_busy(mp);
+                       if (error != 0) {
+                               vput(foundobj);
+                               goto done;
+                       }
+                       if (searchdir != NULL) {
+                               VOP_UNLOCK(searchdir);
+                               searchdir_locked = false;
+                       }
+                       vput(foundobj);
+                       error = VFS_ROOT(mp, &foundobj);
+                       vfs_unbusy(mp);
+                       if (error) {
+                               if (searchdir != NULL) {
+                                       vn_lock(searchdir, LK_EXCLUSIVE | LK_RETRY);
+                                       searchdir_locked = true;
+                               }
+                               goto done;
+                       }
+                       /*
+                        * Avoid locking vnodes from two filesystems because
+                        * it's prone to deadlock, e.g. when using puffs.
+                        * Also, it isn't a good idea to propagate slowness of
+                        * a filesystem up to the root directory. For now,
+                        * only handle the common case, where foundobj is
+                        * VDIR.
+                        *
+                        * In this case set searchdir to null to avoid using
+                        * it again. It is not correct to set searchdir ==
+                        * foundobj here as that will confuse the caller.
+                        * (See PR 47040.)
+                        */
+                       if (searchdir == NULL) {
+                               /* already been here once; do nothing further */
+                       } else if (foundobj->v_type == VDIR) {
+                               vrele(searchdir);
+                               *newsearchdir_ret = searchdir = NULL;
+                       } else {
+                               VOP_UNLOCK(foundobj);



Home | Main Index | Thread Index | Old Index