Re: Vnode API change: add global vnode cache

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

   Date: Sun, 6 Apr 2014 12:14:24 +0200
   From: "J. Hannken-Illjes" <hannken%eis.cs.tu-bs.de@localhost>

   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);