Re: Vnode API change: VOP_LOCK

[J. Hannken-Illjes <hannken%eis.cs.tu-bs.de@localhost>]

On Feb 25, 2014, at 4:44 AM, Taylor R Campbell 
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:

>   Date: Fri, 21 Feb 2014 12:35:40 +0100
>   From: "J. Hannken-Illjes" <hannken%eis.cs.tu-bs.de@localhost>
> 
>   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 - hannken%eis.cs.tu-bs.de@localhost - TU Braunschweig 
(Germany)