tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Vnode API change: add global vnode cache

   Date: Sun, 6 Apr 2014 12:14:24 +0200
   From: "J. Hannken-Illjes" <>

   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:

   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:

   vcache_lookup(struct mount *mp, void *key, size_t key_len, struct vnode 

Call it vcache_intern rather than vcache_lookup, since it inserts if
not already there?

Why `void *key' and not `const void *key'?

   vcache_remove(struct mount *mp, void *key, size_t key_len)

Same about `void *key'.

   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;

   +vcache_lookup(struct mount *mp, void *key, size_t key_len, struct vnode 
   +#ifdef DIAGNOSTICS


   +       new_key = NULL;
   +       *vpp = NULL;

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.
   + */
   +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

        error = VFS_VGET(mp, dotdot_ino, &dvp);


        error = vcache_intern(mp, &dotdot_ino, sizeof(dotdot_ino), &dvp);

Home | Main Index | Thread Index | Old Index