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? 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'. +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; +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. + if (node != NULL /* && node->vn_node == NULL */) { Turn the commented fragment into a kassert. + new_node = kmem_alloc(sizeof(*new_node), KM_SLEEP); Should use pool_cache(9) rather than kmem(9) for fixed allocation. + 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. + /* 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. /* + * 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.) 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);