On 07 Apr 2014, at 03:22, Taylor R Campbell <campbell+netbsd-tech-k...@mumble.net> wrote:
> Date: Sun, 6 Apr 2014 12:14:24 +0200 > From: "J. Hannken-Illjes" <hann...@eis.cs.tu-bs.de> > > Currently all file systems have to implement their own cache of > vnode / fs node pairs. Most file systems use a copy and pasted > version of ufs_ihash. > > So add a global vnode cache with lookup and remove: > [...] > http://www.netbsd.org/~hannken/vnode-pass6-1.diff > > Comments or objections anyone? > > Generally looks pretty good, and largely better than the draft I threw > together last month to do more or less the same thing. > > There are a number of file systems that have copypasta of ufs_ihash; > adapting these too to vcache should be a piece of cake. Have you > surveyed the more different file systems -- e.g., nfs, coda, tmpfs, > puffs -- to see whether this abstraction will make sense there too? > > Some specific comments: > > int > vcache_lookup(struct mount *mp, void *key, size_t key_len, struct vnode > **vpp) > > Call it vcache_intern rather than vcache_lookup, since it inserts if > not already there? Ok. > Why `void *key' and not `const void *key'? > > void > vcache_remove(struct mount *mp, void *key, size_t key_len) > > Same about `void *key'. > > int > vfs_load_node(struct mount *mp, struct vnode *vp, > const void *key, size_t key_len, void **new_key) > > Same about `void **new_key'. Ok - all const now. > +static kmutex_t vcache_lock __cacheline_aligned; > +static kcondvar_t vcache_cv __cacheline_aligned; > +static rb_tree_t vcache_rb_tree __cacheline_aligned; > > Do these all need to be cacheline-aligned separately? Is vcache_lock > not enough, since the others will be used only while vcache_lock is > held? Or, how about this? > > struct { > kmutex_t lock; > kcondvar_t cv; > rb_tree_t tree; > } vcache __cacheline_aligned; Ok - using a struct looks good. > +int > +vcache_lookup(struct mount *mp, void *key, size_t key_len, struct vnode > **vpp) > +{ > ... > +#ifdef DIAGNOSTICS > > DIAGNOSTIC, not DIAGNOSTICS. > > + new_key = NULL; > + *vpp = NULL; > +#endif > > Why not just make these unconditional, or remove the unconditional > assertions that rely on them? We generally ought to avoid assertions > that are true only if DIAGNOSTIC -- it makes code harder to reason > about. If I see KASSERT(*vpp != NULL), I am going to assume that *vpp > is nonnull, whether DIAGNOSTIC is set or not. Removed the #ifdef and always clear new_key and *vpp on top. > + if (node != NULL /* && node->vn_node == NULL */) { > > Turn the commented fragment into a kassert. Ok. > + new_node = kmem_alloc(sizeof(*new_node), KM_SLEEP); > > Should use pool_cache(9) rather than kmem(9) for fixed allocation. Ok. > + vp = vnalloc(NULL); > + vp->v_usecount = 1; > + vp->v_type = VNON; > + vp->v_size = vp->v_writesize = VSIZENOTSET; > > 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. > + /* Load the fs node. Exclusive as new_node->vn_vnode is NULL. */ > + error = VFS_LOAD_NODE(mp, vp, key, key_len, &new_key); > + if (error == 0) { > > Switch the sense of this branch so the main-line code is success and > the indented (and shorter) alternative is failure. Ok. > /* > + * Look up and return a vnode/inode pair by inode number. > + */ > +int > +ufs_vget(struct mount *mp, ino_t ino, struct vnode **vpp) > +{ > > Does anything use VFS_VGET in ufs now? If so, why? If not, this > should just fail. (Eventually we ought to nix VFS_VGET, I think.) VFS_VGET() is used by NFS V3 readdirplus so we have to keep it. > The one call in ufs_rename.c should be easy to fix -- just replace > > VOP_UNLOCK(vp); > error = VFS_VGET(mp, dotdot_ino, &dvp); > vrele(vp); > > by > > error = vcache_intern(mp, &dotdot_ino, sizeof(dotdot_ino), &dvp); > vput(vp); This is wrong, did you mean error = vcache_intern(mp, &dotdot_ino, sizeof(dotdot_ino), &dvp); vput(vp); if (error) return error; error = vn_lock(dvp, LK_EXCLUSIVE); if (error) return error; Anyway, not now. New diff at http://www.netbsd.org/~hannken/vnode-pass6-2.diff -- J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)