Page MenuHomeFreeBSD

Do not drop NFS vnode lock when performing consistency checks.
ClosedPublic

Authored by kib on Aug 19 2017, 2:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jan 31, 8:08 PM
Unknown Object (File)
Nov 17 2024, 2:57 AM
Unknown Object (File)
Nov 14 2024, 8:09 PM
Unknown Object (File)
Nov 14 2024, 7:52 PM
Unknown Object (File)
Oct 18 2024, 11:19 AM
Unknown Object (File)
Oct 3 2024, 4:26 PM
Unknown Object (File)
Sep 27 2024, 11:11 AM
Unknown Object (File)
Sep 23 2024, 7:45 AM
Subscribers

Details

Summary

Currently several paths in the NFS client upgrade the shared vnode lock to exclusive, which might cause temporal dropping of the lock. This action appears to be fatal for nullfs mounts over NFS. If the operation is performed over nullfs vnode, then bypassed down to NFS VOP, and the lock is dropped, other thread might reclaim the upper nullfs vnode. Since on reclaim the nullfs vnode lock and NFS vnode lock are split, the original lock state of the nullfs vnode is not restored. As result, VFS operations receive not locked vnode after a VOP call. See https://people.freebsd.org/~pho/stress/log/kostik1019.txt as an example.

The solution I propose is to stop upgrading the vnode lock when we check the consistency or flush buffers as result of detected inconsistency. Instead, each NFS vnode gets a new lockmgr lock which is locked exclusive instead of the vnode lock upgrade. In other words, the other parallel modification of the vnode are excluded by either vnode lock conflict or exclusivity of the new lock when the vnode lock is shared.

Also revert r316529 because the vnode cannot be reclaimed during ncl_vinvalbuf().

This mostly worked fine, the only VFS change I have to add is to allow vinvalbuf() to operate with the shared vnode lock. This mode allows other clean buffers to arrive while we flush the buf lists for the vnode, which is fine. In fact, only the assert have to be relaxed.

I considered that adding a new lockmgr lock to each NFS vnode is fine even with the associated node structure grow. If considered to be excessive, a per-mount lock can be used instead, but the upgrade lock is used often and contended highly enough.

Test Plan

The patch was developed in collaboration with and tested by Peter Holm.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I've accepted this revision, but I do have one concern.
If a future code change resulted in the vnode lock changing from shared->exclusive
before the ncl_downgrade_vnlock(), it would erroneously do an unlock of this
new lock.

  • If you left ncl_upgrade_vnlock() returning oldlock and passed that into ncl_downgrade_lock() to use to decide whether or not to unlock the new lock, this would be avoided.

However, I do not see how the vnode lock would go from shared->exclusive
with this change, so it is just a suggestion.

Oh, and I might argue that the names for ncl_upgrade_vnlock()/nfs_downgrade_vnlock()
might want to change, since they no longer change the vnode lock?

Thanks for doing this, rick

This revision is now accepted and ready to land.Aug 19 2017, 8:15 PM
kib edited edge metadata.

Rename the functions to ncl_excl_start() and ncl_excl_finish().
Restore the old_lock argument for ncl_excl_finish().

Since the patch is considered as correct, I also reverted the r316529, because the vnode lock is not longer dropped and vnode cannot be reclaimed during ncl_vinvalbuf().

This revision now requires review to proceed.Aug 19 2017, 8:51 PM

This version looks fine to me.
Thanks for doing this, rick.

This revision was automatically updated to reflect the committed changes.