Re: Vnode API change: add global vnode cache

["J. Hannken-Illjes" <hannken%eis.cs.tu-bs.de@localhost>]

On 07 Apr 2014, at 03:22, Taylor R Campbell 
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:

>   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?

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 - hannken%eis.cs.tu-bs.de@localhost - TU Braunschweig 
(Germany)