On Feb 25, 2014, at 4:44 AM, Taylor R Campbell <[email protected]> wrote:
> Date: Fri, 21 Feb 2014 12:35:40 +0100 > From: "J. Hannken-Illjes" <[email protected]> > > Diff available at http://www.netbsd.org/~hannken/vnode-pass3-1.diff > > Comments or objections anyone? > > This seems needlessly complicated to me. Outside of the vfs lifecycle > code, why would you ever want to lock a dead vnode? It seems to me it > would be better to just bite the bullet, kill LK_RETRY, and make > everyone handle possible vn_lock failure (or VOP_LOCK, since vn_lock > would cease to be necessary). I'm quite sure we cannot kill ALL instances of LK_RETRY as - at least union file system needs the unlock / relock when looking up DOTDOT and the relock has to succeed as the protocol wants the node locked on return. I suppose there are more such cases inside the file systems. - this would break our revoke(2) semantics. It must be possible to read and close a revoked character device. - vrelel() and vclean() need it to work on layered vnodes. When the lower node of a layer node gets revoked it must still be possible to lock the layer node to inactivate and reclaim it. The change of VOP_LOCK() to vn_lock(..., LK_RETRY) for vrelel() and vclean() was missing from the diff. > The LK_INTERLOCK flag doesn't seem right. It looks like the only > place you use it is a place where you recently removed some code to > drop the interlock before taking the vnode lock. We ought to be > removing superfluous extra states, not adding more lock order > workarounds, especially ones that involve asking more of the > particular file systems by passing them new flags. OK. New diff at http://www.netbsd.org/~hannken/vnode-pass3-2.diff > A little more broadly, do you have a plan for what the vnode life > cycle should look like? We ought to have a well-defined set of states > that a vnode can be in (something like embryonic, active, cached, > dead) and a clear set of transitions with invariants and lock order > and a clear protocol for file systems to implement. Without that, I'm > sceptical of churn in vrelel, vclean, &c. - outside of vfs a vnode is one of non-existant, inactive, active or dead. - vget() changes from inactive to active. - vrele() changes from active to inactive. - vrecycle() changes from inactive to dead. - vgone() and vrevoke() changes from either active or inactive to dead. These transitions are mutual exclusive, protected by VI_CHANGING. Still missing: - a protected transition from non-existant to active. - cleanup of mnt_vnodelist traversal. - make VI_CLEAN and VI_XLOCK (renamed to VI_CLEANING) private. -- J. Hannken-Illjes - [email protected] - TU Braunschweig (Germany)
