Re: Vnode API change: VI_CLEAN and VI_XLOCK

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

On Mar 21, 2014, at 3:07 PM, Taylor R Campbell 
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:

>   Date: Fri, 21 Mar 2014 11:10:54 +0100
>   From: "J. Hannken-Illjes" <hannken%eis.cs.tu-bs.de@localhost>
> 
>   The vnode flags VI_CLEAN and VI_XLOCK are used in file systems to
>   check for reclaimed or reclaiming but still active vnodes, so
>   make the vnode flags VI_XLOCK, VI_CLEAN (and VI_LOCKSHARE) private to
>   the files kern/vfs_* and add a function to check the vnode state:
> 
>     int vdead_check(struct vnode *vp, bool may_sleep)
> 
>   will return one of:
> 
>     EBUSY:  vnode is becoming dead, with "may_sleep == false" only.
>     ENOENT: vnode is dead.
>     0:      otherwise.
> 
> Is this about vnodes that have been *revoked* or vnodes that the
> system has chosen to *destroy*?  (`Dead' currently means both, but I
> believe that joint meaning is a mistake and I'd like to either kill
> that term or assign it one meaning or the other.)
> 
> If it's vnodes that have been revoked, OK -- I'm not sure there's much
> real use for this outside the lock routines (lfs looks sketchy, as
> always) and I think it would be better to make that distinction in
> vn_lock rather than in all the VOP_LOCKs, but OK.
> 
> But if it's vnodes that the system has chosen to destroy, then it
> seems wrong -- it shouldn't be possible to use this routine without
> first having called vget.

It is vnodes that either are revoking (at a point of no return) or
that have been revoked.

And yes -- in the future (after lfs and specfs got fixed) it should
assert "v_usecount > 0".

> For example, the logic in spec_node_lookup_by_dev looks wrong to me,
> before and after your patch -- it's not clear to me why it doesn't
> just first call vget (and then perhaps call vget again for
> vp->v_specnode->sn_dev->sd_bdevvp).

Yes -- this was wrong and still is.  But not with this commit.

> Other minor notes:
> 
> Does this also make vwait private

Yes -- taken.

> I'd rather use a named constant flag than a boolean here.  I can never
> keep straight what true or false means and whether true means it or
> false means it, but vdead_check(vp, VDEAD_NOWAIT) or vdead_check(vp,
> VDEAD_WAITOK) is clear enough.

Ok -- will use VDEAD_NOWAIT.

>  Also, what about EINTR/ERESTART?

It is (still) not possible to interrupt vwait() ...

New diff at http://www.netbsd.org/~hannken/vnode-pass5-2.diff

--
J. Hannken-Illjes - hannken%eis.cs.tu-bs.de@localhost - TU Braunschweig 
(Germany)