In message <[EMAIL PROTECTED]>, 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 [EMAIL PROTECTED]
with "unsubscribe freebsd-hackers" in the body of the message
