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" <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);
Home |
Main Index |
Thread Index |
Old Index