(Illustration by Gaich Muramatsu)
This is hard to follow if you don't grok vfs locking rules, but FYI for those that do. (I'm only newly almost in that camp...) I hope that a fix will be committed soon - if others concur that I am right in my analysis below. ------- Forwarded Message Return-Path: gdt_at_ir.bbn.com Delivery-Date: Fri Mar 28 14:48:45 2003 Return-Path: <gdt_at_ir.bbn.com> Delivered-To: gdt_at_ir.bbn.com Received: from fnord.ir.bbn.com (localhost [127.0.0.1]) by fnord.ir.bbn.com (Postfix) with ESMTP id 8620E78D; Fri, 28 Mar 2003 14:48:45 -0500 (EST) From: Greg Troxel <gdt_at_ir.bbn.com> To: Jaromir Dolecek <jdolecek_at_netbsd.org> Cc: Greg Troxel <gdt_at_ir.bbn.com>, tech-kern_at_netbsd.org Subject: Re: [solution] vnode refcount panic, perhaps due to kern/vfs_lookup.c:lookup() In-Reply-To: Message from Jaromir Dolecek <jdolecek_at_netbsd.org> of "Wed, 19 Mar 2003 10:30:33 +0100." <200303190930.h2J9UXj05731_at_s102-n054.tele2.cz> Date: Fri, 28 Mar 2003 14:48:45 -0500 Sender: gdt_at_ir.bbn.com Message-Id: <20030328194845.8620E78D_at_fnord.ir.bbn.com> X-Spam-Status: No, hits=-1.4 required=5.0 tests=AWL,IN_REP_TO,PATCH_UNIFIED_DIFF,SPAM_PHRASE_00_01 version=2.43 X-Spam-Level: Sorry for the delay in checking your proposed alternative change (I had to loan out my crash/kgdb box to someone else at work). I looked at your patch, and concluded that it was incorrect to unlock tdvp, since VOP_LOOKUP requires that dvp is the locked vnode of the directory to search. So, I started with your patch minus the VOP_UNLOCK. It is a good thing that UNLOCK is inappropriate, because now we hold a lock to the parent dir the whole time the symlink creation operation is in progress. sys_symlink promptly discards the returned value, but the system call would return an error if e.g. someone snuck an unlink in between the venus symlink creation and looking up the symlink. On running with just VOP_LOOKUP, I got a locking against myself panic in namei from a stat operation following the symlink. After running gdb, I noticed LOCKPARENT set in the componentname structure passed to VOP_LOOKUP. After reading a lot of code/man pages, I found that sys_symlink sets LOCKPARENT in nameidata to get the locked directory where the symlink is to be placed. This makes sense, because VOP_SYMLINK requires a locked directory vnode. But: The componentname struct from nd, still containing the LOCKPARENT flag, is passed to VOP_SYMLINK. VOP_SYMLINK is not defined to pay attention to this. But when calling VOP_LOOKUP with the same cnp, the parent remains locked, and we lose on the next access with locking against myself. Also, it looks like if venus_symlink fails that tdvp will still be locked. vnode_if.src indicates that dvp should be unlocked on error. So I added a vput in that case. I think most coda problems are found in the namei, and once we get that far venus is likely to perform the symlink. So, I think sys_symlink is buggy, and should clean up the LOCKPARENT flag in the struct componentname before reusing it. At a minimum, having extra flags that don't have the obvious semantics on some calls seems unclean and confusing. (All diffs are against very recent netbsd-1-6.) - --- vfs_syscalls.c.~1.1.1.3.~ Fri May 10 20:45:06 2002 +++ vfs_syscalls.c Fri Mar 28 14:35:16 2003 @@ -1518,6 +1518,8 @@ VATTR_NULL(&vattr); vattr.va_mode = ACCESSPERMS &~ p->p_cwdi->cwdi_cmask; VOP_LEASE(nd.ni_dvp, p, p->p_ucred, LEASE_WRITE); + /* VOP_SYMLINK might call LOOKUP (e.g. coda), so LOCKPARENT isn't safe */ + nd.ni_cnd.cn_flags &= ~LOCKPARENT; error = VOP_SYMLINK(nd.ni_dvp, &nd.ni_vp, &nd.ni_cnd, &vattr, path); if (error == 0) vput(nd.ni_vp); Here is the patch to coda_vnops that works for me (thousands of create/delete symlink operations with emacs/cfs/coda were ok), followed by the snippet of my current code. This patch works even without the sys_symlink patch above. Index: coda_vnops.c =================================================================== RCS file: /FOO-CVS/netbsd/src/sys/coda/coda_vnops.c,v retrieving revision 1.1.1.2 diff -u -u -r1.1.1.2 coda_vnops.c - --- coda_vnops.c 2001/12/06 04:27:40 1.1.1.2 +++ coda_vnops.c 2003/03/28 19:28:46 @@ -1642,26 +1642,17 @@ /* Invalidate the parent's attr cache, the modification time has changed */ tdcp->c_flags &= ~C_VATTR; - - if (!error) - - { - - struct nameidata nd; - - NDINIT(&nd, LOOKUP, FOLLOW|LOCKLEAF, UIO_SYSSPACE, nm, p); - - nd.ni_cnd.cn_cred = cred; - - nd.ni_loopcnt = 0; - - nd.ni_startdir = tdvp; - - nd.ni_cnd.cn_pnbuf = (char *)nm; - - nd.ni_cnd.cn_nameptr = nd.ni_cnd.cn_pnbuf; - - nd.ni_pathlen = len; - - vput(tdvp); - - error = lookup(&nd); - - *ap->a_vpp = nd.ni_vp; - - } - - - - /* - - * Free the name buffer - - */ - - if ((cnp->cn_flags & SAVESTART) == 0) { - - PNBUF_PUT(cnp->cn_pnbuf); + if (!error) { + /* + * We don't want to keep the parent locked, but sys_symlink left + * flag turds from an earlier namei call. + */ + cnp->cn_flags &= ~LOCKPARENT; + error = VOP_LOOKUP(tdvp, ap->a_vpp, cnp); + /* Either an error occurs, or ap->a_vpp is locked. */ + } else { + /* error, so unlock and dereference parent */ + vput(tdvp); } exit: The way it looks after munging: error = venus_symlink(vtomi(tdvp), &tdcp->c_fid, path, plen, nm, len, tva, cred, p); /* Invalidate the parent's attr cache, the modification time has changed */ tdcp->c_flags &= ~C_VATTR; if (!error) { /* * We don't want to keep the parent locked, but sys_symlink left * flag turds from an earlier namei call. */ cnp->cn_flags &= ~LOCKPARENT; error = VOP_LOOKUP(tdvp, ap->a_vpp, cnp); /* Either an error occurs, or ap->a_vpp is locked. */ } else { /* error, so unlock and deference parent */ vput(tdvp); } ------- End of Forwarded MessageReceived on 2003-03-28 14:59:32