On 07 Apr 2014, at 18:02, Taylor R Campbell <campbell+netbsd-tech-k...@mumble.net> wrote:
> Date: Mon, 7 Apr 2014 15:51:00 +0200 > From: "J. Hannken-Illjes" <hann...@eis.cs.tu-bs.de> > > On 07 Apr 2014, at 03:22, Taylor R Campbell > <campbell+netbsd-tech-k...@mumble.net> wrote: > >> Is the plan to nix getnewvnode and ungetnewvnode? It would be good to >> avoid long-term duplication of its body. But I suspect we'll want to >> keep those for, e.g., tmpfs, in which case this should use getnewvnode >> and ungetnewvnode instead of vnalloc/setup/teardown/vnfree. > > Not sure if we have to keep getnewvnode() in the long term. > > It cannot be used here as we don't want the partially initialised > vnode (before VFS_LOAD_NODE) on the mnt_vnodelist. > > If we end up replacing getnewvnode() with a two-stage allocator that > initialises in the first stage and finalises in the second stage we > may use it here. > > In that case, could you set the VI_CHANGING bit in vp, rather than > node->vn_vnode = NULL in the vcache node, to indicate that the vnode > is being initialized in vcache_intern? That way, > > (a) it is safe to put the vnode on the mount list -- anyone trying to > use it from there must use vget first --; > > (b) there is only one place waits happen, namely vget (vwait), so no > need for vcache.cv or some of the complex loop logic in vcache_intern; > > (c) tmpfs can mirror the vcache_intern logic without more duplication > of code from getnewvnode; and > > (d) as a tiny added bonus, omitting vcache.cv will make the vcache > struct fit in a single cache line on, e.g., amd64. > > Generally, we should have only one mechanism for weak references to > vnodes, and currently that's vget. If we do want to take dholland's > idea of a separate structure for weak references to vnodes, then the > vnode mount list and anything else should use that too. But I think > vget is good enough for now. > > Also, it occurs to me that we're going to have not two but multiple > copies of initialization code that is currently in getnewvnode, > because in your API, some is copied in vcache_intern and some must be > copied in VFS_LOAD_NODE. > > So I'm inclined to say that vcache_intern should itself look a little > more like getnewvnode, and perhaps VFS_LOAD_NODE should be VOP_LOAD > instead. See attached file for a sketch this, and a tmpfs version. > > Except for the VI_CHANGING part, the vcache_intern I attached uses > nothing internal to the vnode abstraction. To address VI_CHANGING, we > could make getnewvnode set it by default and require callers to use > some new routine, say vready, to clear it. Then vcache_intern, or > clones of it like tmpfs will want, can live entirely outside the vnode > API. Sorry for being late -- back from the openssl disaster now ... There is no need to do this VI_CHANGING etc. here. Primary goal of the vnode cache is to always initialise vnode / fs node pairs before they become visible or appear on any lists. There is no need to make the vcache API look like getnewvnode(). But getnewvnode() should change to a two-pass allocator that initialises in the first pass and finalises, adds to mount list in the second pass. -- J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)