From 0ae6383d39aa86d32ea3fe0706998b4ab6c45c28 Mon Sep 17 00:00:00 2001 From: Xin LI Date: Fri, 10 Aug 2007 05:24:49 +0000 Subject: [PATCH] MFp4: - Respect cnflag and don't lock vnode always as LK_EXCLUSIVE [1] - Properly lock around tn_vnode to avoid NULL deference - Be more careful handling vnodes (*) (*) This is a WIP [1] by pjd via howardsu Thanks kib@ for his valuable VFS related comments. Tested with: fsx, fstest, tmpfs regression test set Found by: pho's stress2 suite Approved by: re (tmpfs blanket) --- sys/fs/tmpfs/tmpfs.h | 4 ++-- sys/fs/tmpfs/tmpfs_subr.c | 47 ++++++++++++++++++++++--------------- sys/fs/tmpfs/tmpfs_vfsops.c | 7 ++++-- sys/fs/tmpfs/tmpfs_vnops.c | 45 +++++++++++++++++++++++++---------- 4 files changed, 68 insertions(+), 35 deletions(-) diff --git a/sys/fs/tmpfs/tmpfs.h b/sys/fs/tmpfs/tmpfs.h index f681e3247ddb..c57eb25f73d9 100644 --- a/sys/fs/tmpfs/tmpfs.h +++ b/sys/fs/tmpfs/tmpfs.h @@ -353,8 +353,8 @@ int tmpfs_alloc_dirent(struct tmpfs_mount *, struct tmpfs_node *, const char *, uint16_t, struct tmpfs_dirent **); void tmpfs_free_dirent(struct tmpfs_mount *, struct tmpfs_dirent *, boolean_t); -int tmpfs_alloc_vp(struct mount *, struct tmpfs_node *, struct vnode **, - struct thread *td); +int tmpfs_alloc_vp(struct mount *, struct tmpfs_node *, int, + struct vnode **, struct thread *); void tmpfs_free_vp(struct vnode *); int tmpfs_alloc_file(struct vnode *, struct vnode **, struct vattr *, struct componentname *, char *); diff --git a/sys/fs/tmpfs/tmpfs_subr.c b/sys/fs/tmpfs/tmpfs_subr.c index 4a2ae2da2e29..08127fd43d49 100644 --- a/sys/fs/tmpfs/tmpfs_subr.c +++ b/sys/fs/tmpfs/tmpfs_subr.c @@ -188,6 +188,12 @@ tmpfs_free_node(struct tmpfs_mount *tmp, struct tmpfs_node *node) { size_t pages = 0; +#ifdef INVARIANTS + TMPFS_NODE_LOCK(node); + MPASS(node->tn_vnode == NULL); + TMPFS_NODE_UNLOCK(node); +#endif + TMPFS_LOCK(tmp); LIST_REMOVE(node, tn_entries); tmp->tm_nodes_inuse--; @@ -302,15 +308,20 @@ tmpfs_free_dirent(struct tmpfs_mount *tmp, struct tmpfs_dirent *de, * Returns zero on success or an appropriate error code on failure. */ int -tmpfs_alloc_vp(struct mount *mp, struct tmpfs_node *node, struct vnode **vpp, - struct thread *td) +tmpfs_alloc_vp(struct mount *mp, struct tmpfs_node *node, int lkflag, + struct vnode **vpp, struct thread *td) { int error; struct vnode *vp; loop: + TMPFS_NODE_LOCK(node); if ((vp = node->tn_vnode) != NULL) { - error = vget(vp, LK_EXCLUSIVE | LK_RETRY, td); + VI_LOCK(vp); + TMPFS_NODE_UNLOCK(node); + vholdl(vp); + error = vget(vp, lkflag | LK_INTERLOCK | LK_RETRY, td); + vdrop(vp); if (error) return error; @@ -330,12 +341,11 @@ loop: * otherwise lock the vp list while we call getnewvnode * since that can block. */ - TMPFS_NODE_LOCK(node); if (node->tn_vpstate & TMPFS_VNODE_ALLOCATING) { node->tn_vpstate |= TMPFS_VNODE_WANT; error = msleep((caddr_t) &node->tn_vpstate, TMPFS_NODE_MTX(node), PDROP | PCATCH, - "tmpfs_vplock", 0); + "tmpfs_alloc_vp", 0); if (error) return error; @@ -351,7 +361,7 @@ loop: goto unlock; MPASS(vp != NULL); - error = vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, td); + error = vn_lock(vp, lkflag | LK_RETRY, td); if (error != 0) { vp->v_data = NULL; vput(vp); @@ -386,23 +396,15 @@ loop: vnode_pager_setsize(vp, node->tn_size); error = insmntque(vp, mp); - if (error) { - node->tn_vnode = NULL; - TMPFS_NODE_LOCK(node); - if (node->tn_vpstate & TMPFS_VNODE_WANT) { - node->tn_vpstate &= ~TMPFS_VNODE_WANT; - TMPFS_NODE_UNLOCK(node); - wakeup((caddr_t) &node->tn_vpstate); - } else - TMPFS_NODE_UNLOCK(node); - return error; - } - node->tn_vnode = vp; + if (error) + vp = NULL; unlock: TMPFS_NODE_LOCK(node); + MPASS(node->tn_vpstate & TMPFS_VNODE_ALLOCATING); node->tn_vpstate &= ~TMPFS_VNODE_ALLOCATING; + node->tn_vnode = vp; if (node->tn_vpstate & TMPFS_VNODE_WANT) { node->tn_vpstate &= ~TMPFS_VNODE_WANT; @@ -415,7 +417,11 @@ out: *vpp = vp; MPASS(IFF(error == 0, *vpp != NULL && VOP_ISLOCKED(*vpp, td))); +#ifdef INVARIANTS + TMPFS_NODE_LOCK(node); MPASS(*vpp == node->tn_vnode); + TMPFS_NODE_UNLOCK(node); +#endif return error; } @@ -433,8 +439,10 @@ tmpfs_free_vp(struct vnode *vp) node = VP_TO_TMPFS_NODE(vp); + TMPFS_NODE_LOCK(node); node->tn_vnode = NULL; vp->v_data = NULL; + TMPFS_NODE_UNLOCK(node); } /* --------------------------------------------------------------------- */ @@ -499,7 +507,8 @@ tmpfs_alloc_file(struct vnode *dvp, struct vnode **vpp, struct vattr *vap, } /* Allocate a vnode for the new file. */ - error = tmpfs_alloc_vp(dvp->v_mount, node, vpp, cnp->cn_thread); + error = tmpfs_alloc_vp(dvp->v_mount, node, LK_EXCLUSIVE, vpp, + cnp->cn_thread); if (error != 0) { tmpfs_free_dirent(tmp, de, TRUE); tmpfs_free_node(tmp, node); diff --git a/sys/fs/tmpfs/tmpfs_vfsops.c b/sys/fs/tmpfs/tmpfs_vfsops.c index e860e163d81c..a2ebf09a0c0f 100644 --- a/sys/fs/tmpfs/tmpfs_vfsops.c +++ b/sys/fs/tmpfs/tmpfs_vfsops.c @@ -390,7 +390,7 @@ static int tmpfs_root(struct mount *mp, int flags, struct vnode **vpp, struct thread *td) { int error; - error = tmpfs_alloc_vp(mp, VFS_TO_TMPFS(mp)->tm_root, vpp, td); + error = tmpfs_alloc_vp(mp, VFS_TO_TMPFS(mp)->tm_root, flags, vpp, td); if (!error) (*vpp)->v_vflag |= VV_ROOT; @@ -429,7 +429,10 @@ tmpfs_fhtovp(struct mount *mp, struct fid *fhp, struct vnode **vpp) } TMPFS_UNLOCK(tmp); - return found ? tmpfs_alloc_vp(mp, node, vpp, curthread) : EINVAL; + if (found) + return (tmpfs_alloc_vp(mp, node, LK_EXCLUSIVE, vpp, curthread)); + + return (EINVAL); } /* --------------------------------------------------------------------- */ diff --git a/sys/fs/tmpfs/tmpfs_vnops.c b/sys/fs/tmpfs/tmpfs_vnops.c index 514aeb75738e..d43e26741039 100644 --- a/sys/fs/tmpfs/tmpfs_vnops.c +++ b/sys/fs/tmpfs/tmpfs_vnops.c @@ -95,12 +95,20 @@ tmpfs_lookup(struct vop_cachedlookup_args *v) !(cnp->cn_flags & ISDOTDOT))); if (cnp->cn_flags & ISDOTDOT) { + int ltype = 0; + + ltype = VOP_ISLOCKED(dvp, td); + vholdl(dvp); VOP_UNLOCK(dvp, 0, td); - /* Allocate a new vnode on the matching entry. */ - error = tmpfs_alloc_vp(dvp->v_mount, dnode->tn_dir.tn_parent, vpp, td); + error = tmpfs_alloc_vp(dvp->v_mount, dnode->tn_dir.tn_parent, + cnp->cn_lkflags, vpp, td); - vn_lock(dvp, LK_EXCLUSIVE | LK_RETRY, td); + vn_lock(dvp, ltype | LK_RETRY, td); + if (ltype & LK_INTERLOCK) + vdropl(dvp); + else + vdrop(dvp); dnode->tn_dir.tn_parent->tn_lookup_dirent = NULL; } else if (cnp->cn_namelen == 1 && cnp->cn_nameptr[0] == '.') { @@ -160,7 +168,8 @@ tmpfs_lookup(struct vop_cachedlookup_args *v) goto out; /* Allocate a new vnode on the matching entry. */ - error = tmpfs_alloc_vp(dvp->v_mount, tnode, vpp, td); + error = tmpfs_alloc_vp(dvp->v_mount, tnode, + cnp->cn_lkflags, vpp, td); if (error != 0) goto out; @@ -174,10 +183,10 @@ tmpfs_lookup(struct vop_cachedlookup_args *v) } tnode->tn_lookup_dirent = de; cnp->cn_flags |= SAVENAME; + } else { + error = tmpfs_alloc_vp(dvp->v_mount, tnode, + cnp->cn_lkflags, vpp, td); } - else - error = tmpfs_alloc_vp(dvp->v_mount, tnode, vpp, td); - } } @@ -478,7 +487,7 @@ lookupvpg: vm_page_wakeup(m); VM_OBJECT_UNLOCK(vobj); return (error); - } + } VM_OBJECT_UNLOCK(vobj); nocache: VM_OBJECT_LOCK(tobj); @@ -886,13 +895,13 @@ tmpfs_rename(struct vop_rename_args *v) struct vnode *tdvp = v->a_tdvp; struct vnode *tvp = v->a_tvp; struct componentname *tcnp = v->a_tcnp; - struct tmpfs_node *tnode = 0; /* pacify gcc */ char *newname; int error; struct tmpfs_dirent *de; struct tmpfs_node *fdnode; struct tmpfs_node *fnode; + struct tmpfs_node *tnode; struct tmpfs_node *tdnode; MPASS(VOP_ISLOCKED(tdvp, tcnp->cn_thread)); @@ -902,6 +911,7 @@ tmpfs_rename(struct vop_rename_args *v) fdnode = VP_TO_TMPFS_DIR(fdvp); fnode = VP_TO_TMPFS_NODE(fvp); + tnode = (tvp == NULL) ? NULL : VP_TO_TMPFS_NODE(tvp); de = fnode->tn_lookup_dirent; /* Disallow cross-device renames. @@ -934,7 +944,7 @@ tmpfs_rename(struct vop_rename_args *v) * Kern_rename gurantees the destination to be a directory * if the source is one. */ if (tvp != NULL) { - tnode = VP_TO_TMPFS_NODE(tvp); + MPASS(tnode != NULL); if ((tnode->tn_flags & (NOUNLINK | IMMUTABLE | APPEND)) || (tdnode->tn_flags & (APPEND | IMMUTABLE))) { @@ -942,9 +952,20 @@ tmpfs_rename(struct vop_rename_args *v) goto out; } - if ((de->td_node->tn_type == VDIR) && (tnode->tn_size > 0)) { - error = ENOTEMPTY; + if (fnode->tn_type == VDIR && tnode->tn_type == VDIR) { + if (tnode->tn_size > 0) { + error = ENOTEMPTY; + goto out; + } + } else if (fnode->tn_type == VDIR && tnode->tn_type != VDIR) { + error = ENOTDIR; goto out; + } else if (fnode->tn_type != VDIR && tnode->tn_type == VDIR) { + error = EISDIR; + goto out; + } else { + MPASS(fnode->tn_type != VDIR && + tnode->tn_type != VDIR); } }