Date: Sun, 13 Jan 2002 00:43:40 +0000 From: Ian Dowse <iedowse@maths.tcd.ie> To: Matthew Dillon <dillon@apollo.backplane.com> Cc: Alfred Perlstein <bright@mu.org>, "Alan L. Cox" <alc@imimic.com>, FreeBSD-hackers@FreeBSD.ORG Subject: Re: Need review of NFS patch set for server .. missing/wrong vput() issues Message-ID: <200201130043.aa90380@salmon.maths.tcd.ie> In-Reply-To: Your message of "Thu, 10 Jan 2002 19:23:13 PST." <200201110323.g0B3NDt38028@apollo.backplane.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200201130043.aa90380>