tech-kern archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: Vnode API change: mnt_vnodelist traversal
On Mar 3, 2014, at 3:28 PM, Taylor R Campbell
<campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
> 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;
> };
No. I want to keep the state opaque to the caller. There is no need for
the calling party to know anything about state internals.
> 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?
I prefer bool/int result and returning data through argument list.
> 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.
Ok.
> /*
> * 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.
Flag WRITECLOSE is used for r/w -> ro mount update. This has much more
problems, if I remember right there is even a PR open. So not now ...
New diff at http://www.netbsd.org/~hannken/vnode-pass4-2.diff
--
J. Hannken-Illjes - hannken%eis.cs.tu-bs.de@localhost - TU Braunschweig
(Germany)
Home |
Main Index |
Thread Index |
Old Index