From owner-freebsd-hackers Sat Jan 12 16:43:49 2002 Delivered-To: freebsd-hackers@freebsd.org Received: from salmon.maths.tcd.ie (salmon.maths.tcd.ie [134.226.81.11]) by hub.freebsd.org (Postfix) with SMTP id 456EA37B421 for ; Sat, 12 Jan 2002 16:43:45 -0800 (PST) Received: from walton.maths.tcd.ie by salmon.maths.tcd.ie with SMTP id ; 13 Jan 2002 00:43:44 +0000 (GMT) To: Matthew Dillon Cc: Alfred Perlstein , "Alan L. Cox" , FreeBSD-hackers@FreeBSD.ORG Subject: Re: Need review of NFS patch set for server .. missing/wrong vput() issues In-Reply-To: Your message of "Thu, 10 Jan 2002 19:23:13 PST." <200201110323.g0B3NDt38028@apollo.backplane.com> Date: Sun, 13 Jan 2002 00:43:40 +0000 From: Ian Dowse Message-ID: <200201130043.aa90380@salmon.maths.tcd.ie> Sender: owner-freebsd-hackers@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG In message <200201110323.g0B3NDt38028@apollo.backplane.com>, Matthew Dillon wri tes: > Patch section 1 > > Here we were previously vput()ing nd.ni_vp only if error == 0. > If error is returned non-zero from namei() this would normally be > correct. However, we force error on a number of occassions after > namei() succeeds, in which case nd.ni_vp may be non-NULL and we > must release it. This fixes it so nd.ni_vp is vput()'d if it is > non-NULL whether an error is specified at this point or not. I don't think this is necessary, because the cleanup code at the end of nfsrv_mknod() catches any cases where nd.ni_vp was not released earlier. It would be harmless to add it though. > (I believe this may have been Alexey's 'NFS hangs in inode state' > problem, which occurs if you are running innd over an NFS filesystem) Was that a client-side or server-side issue? > Patch section's 2 & 3 > > Here namei() is called only with LOCKPARENT, which means that the > leaf is not locked. So when releasing the vnodes we should not > have the if (vp == dvp) test, we should just vput() the dvp and > vrele the vp. Hmm, it seems that lookup() doesn't actually leave the parent locked in this case (it probably should), so I think the existing code is correct in that distorted sense of `correct'. The exit code in lookup() is: if ((cnp->cn_flags & LOCKLEAF) == 0) VOP_UNLOCK(dp, 0, td); return (0); I tried reproducing the vp == dvp case in nfsrv_link by attempting to create a link called `/somedir/.' to an existing regular file (I did this at the protocol level; I'm not sure if you can do this easily from a normal client). Instrumentation confirmed that the code in question does get executed with vp == dvp, but I saw no problems or panics either with or without your patch (!). It seems we don't have any VFS locking assertions compiled in even with INVARIANTS... When I added some assertions, your patch triggered my "vput: vnode not locked" error as soon as the weird link operation was repeated, but the existing code works fine. We really need some basic locking assertions such as checking that a vnode is locked when you vput it, and checking that it isn't locked when the last reference is vrele'd. This is complicated by the fact that we have at least 3 different types of vnode locking: vop_stdlock (ufs etc), vop_sharedlock (nfs), and vop_nolock (devfs, procfs etc). Maybe a VOP_LOCKASSERT would help, because VOP_ISLOCKED isn't useful for vop_nolock filesystems. Note that there are the `options DEBUG_VFS_LOCKS' assertions, but these are used in ways that can result in false positives. Ian To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-hackers" in the body of the message