tech-kern archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: Vnode API change: mnt_vnodelist traversal
Date: Mon, 3 Mar 2014 11:11:04 +0100
From: "J. Hannken-Illjes" <hannken%eis.cs.tu-bs.de@localhost>
Add an interface to iterate over a vnode list:
void vfs_vnode_iterator_init(struct mount *mp, void **marker)
void vfs_vnode_iterator_destroy(void *marker)
bool vfs_vnode_iterator_next(void *marker, struct vnode **vpp)
Looks pretty good to me, but let's make the API a little type-safer:
/* mount.h */
struct vnode_iterator; /* opaque */
void vfs_vnode_iterator_init(struct mount *mp, struct vnode_iterator **vip);
void vfs_vnode_iterator_destroy(struct vnode_iterator *vi);
bool vfs_vnode_iterator_next(struct vnode_iterator *vi, struct vnode **vpp);
/* vfs_mount.c */
struct vnode_iterator {
struct vnode vi_vnode;
};
void
vfs_vnode_iterator_init(struct mount *mp, struct vnode_iterator **vip)
{
struct vnode *vp;
vp = vnalloc(mp);
mutex_enter(&mntvnode_lock);
TAILQ_INSERT_HEAD(&mp->mnt_vnodelist, vp, v_mntvnodes);
mutex_exit(&mntvnode_lock);
*vip = (struct vnode_iterator *)vp;
}
void
vfs_vnode_iterator_destroy(struct vnode_iterator *vi)
{
struct vnode *vp = &vi->vi_vnode;
...
}
Why not have vfs_vnode_iterator_next return the vnode pointer or NULL?
struct vnode *vfs_vnode_iterator_next(struct vnode_iterator *vi);
while ((vp = vfs_vnode_iterator_next(vi)) != NULL) {
...
}
A couple little comments on the patch:
--- sys/kern/vfs_mount.c 27 Feb 2014 13:00:06 -0000 1.26
+++ sys/kern/vfs_mount.c 3 Mar 2014 10:02:29 -0000
@@ -374,8 +374,68 @@ vunmark(vnode_t *mvp)
...
+void
+vfs_vnode_iterator_destroy(void *marker)
+{
+ struct vnode *mvp = marker;
+
+ KASSERT((mvp->v_iflag & VI_MARKER) != 0);
I'd prefer to say vismarker(mvp) here, or at least ISSET(mvp->v_iflag,
VI_MARKER) if you want to kill vismarker -- my eyes cross when I see
the mess of (vp->v_iflag & VI_XLXKMRK) {!,}= 0 everywhere.
/*
* If WRITECLOSE is set, only flush out regular file
* vnodes open for writing.
*/
- if ((flags & WRITECLOSE) &&
- (vp->v_writecount == 0 || vp->v_type != VREG)) {
+ if ((flags & WRITECLOSE) && vp->v_type == VREG) {
+ mutex_enter(vp->v_interlock);
+ if (vp->v_writecount == 0) {
+ mutex_exit(vp->v_interlock);
+ vrele(vp);
+ continue;
+ }
mutex_exit(vp->v_interlock);
- continue;
This looks suspect: we make a decision based on vp->v_writecount under
the lock, and then drop the lock. Perhaps it doesn't matter if this
is an opportunistic thing (and I suspect since we don't hold the vnode
lock the original code had the same issue), but a comment to that
effect would be nice.
Home |
Main Index |
Thread Index |
Old Index