Re: Vnode API change: mnt_vnodelist traversal

[David Laight <david%l8s.co.uk@localhost>]

On Mon, Mar 03, 2014 at 03:55:12PM +0100, J. Hannken-Illjes wrote:
> On Mar 3, 2014, at 11:32 AM, Thomas Klausner <wiz%netbsd.org@localhost> wrote:
> 
> > On Mon, Mar 03, 2014 at 11:11:04AM +0100, J. Hannken-Illjes wrote:
> >> A diff implementing this and using it for those operations running
> >> vrecycle() is at http://www.netbsd.org/~hannken/vnode-pass4-1.diff
> >> 
> >> Once all operations are converted, vmark() / vunmark() will go and
> >> man pages will be updated.
> >> 
> >> Comments or objections anyone?
> > 
> > I have no background clue, so please excuse my questions if they are
> > stupid :)
> > 
> > +void
> > +vfs_vnode_iterator_init(struct mount *mp, void **marker)
> > +{
> > +   struct vnode **mvpp = (struct vnode **)marker;
> > +
> > +   *mvpp = vnalloc(mp);
> > +
> > +   mutex_enter(&mntvnode_lock);
> > +   TAILQ_INSERT_HEAD(&mp->mnt_vnodelist, *mvpp, v_mntvnodes);
> > +   mutex_exit(&mntvnode_lock);
> > +}
> > +
> > +void
> > +vfs_vnode_iterator_destroy(void *marker)
> > +{
> > +   struct vnode *mvp = marker;
> > +
> > +   KASSERT((mvp->v_iflag & VI_MARKER) != 0);
> > +   vnfree(mvp);
> > +}
> > 
> > Why do you cast marker in init, but not in destroy or next?
> 
> Because (void **) to (othertype **) needs a cast.  Added casts to
> destroy and next anyway.
> 
> > I assume that the marker is not struct vnode * so that you can change
> > the type later if you want.
> 
> It is struct vnode * for now, to the caller it is simply opaque as the
> caller doesn't need to know the internals.

Use the correct type - if the caller doesn't need to know the internals
add a 'struct vnode' before the function definition.
(Or even 'struct foo' - which might currently be a vnode.)
If you use 'void *' it becomes unclear where the pointers are valid.

In this case I'm not sure that adding a marker vnode into the list
of vnodes is a good idea at all.

What you might want is a list of active iterators and their current
position so that the 'right' things can happen when a vnode is deleted
(especially if they need to save the 'next' vnode to allow the function
itself delete the current one).

In that case the approriate structure can be allocated in stack as
part of the iterator data.

For instance, you might decide to scan the vnodes from the hash lists.
And for SMP locking you might want to arrange the hash so that any
'next' pointers are within the hash structure - completely removing
any linked list between the vnodes themselves.


        David

-- 
David Laight: david%l8s.co.uk@localhost