Re: Add a mountlist iterator

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Sun, 2 Apr 2017 11:09:49 +0200
> From: "J. Hannken-Illjes" <hannken%eis.cs.tu-bs.de@localhost>
> 
> > On 1. Apr 2017, at 23:03, Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
> > 
> > Any particular reason to use a pointer-to-opaque-pointer here instead
> > of a caller-allocated struct mountlist_iterator object?
> 
> Just to make it opaque to the caller.  I see no reason to make the
> mountlist internals part of the kernel API and expose them down to
> the file system implementations.

The caller-allocated struct mountlist_iterator could be made to
contain just a single pointer as the current API uses.

*shrug* Not a big deal either way.

> As _mountlist_next() exists for DDB only (no locks, no allocs) I see
> no problems here.

One might be concerned in ddb with having to chase many pointers in a
possibly corrupt kernel memory state, so there is some small advantage
to avoiding list traversal.

>                    Embedding the mountlist_entry in "struct mount"
> forces us to use a "struct mount" as a marker and this struct
> has a size of ~2.5 kBytes.

By `embed' I didn't mean that the marker has to be a whole struct
mount.  Rather, something more like:

	struct mountlist_entry {
		TAILQ_ENTRY(mountlist_entry)		mle_tqe;
		enum { MNTENT_MARKER, MNTENT_MOUNT }	mle_tag;
	};

	struct mount {
		...
		struct mountlist_entry mnt_entry;
		...
	};

	TAILQ_HEAD(, mountlist_entry) mount_list;

To recover the struct mount from a struct mountlist_entry that is
tagged as a mount, simply do

	struct mountlist_entry *mle = ...;
	struct mount *mp = container_of(mle, struct mount, mnt_entry);

> > Candidate for cacheline-aligned struct?  Why do we need a new lock
> > mount_list_lock for this instead of using the existing mountlist_lock?
> 
> For the transition it is better to use a separate lock.  After the
> transition "mountlist_lock" gets removed.

OK, fair enough.  Can you add a comment over the declaration stating
that plan?

> > Beware: TAILQ_PREV (and TAILQ_LAST) violates strict aliasing rules, so
> > we should probably try to phase it out rather than add new users.
> > Maybe we should add a new kind of queue(3) that doesn't violate strict
> > aliasing but supports *_PREV and *_LAST.
> > 
> > All that said, it is not clear to me why you need reverse iteration
> > here.  None of your patches seem to use it.  Are you planning to use
> > it for some future purpose?
> 
> Historical the mountlist gets traversed in mount order.  Some syscalls
> return in this order so it should be kept.
> 
> Unmounting all file system on shutdown has to be done in reverse mount
> order so here (vfs_unmount_forceone / vfs_unmountall1) we need it.

I see -- your patch just hasn't addressed those yet.  Maybe Someone^TM
should just write a replacement for TAILQ so we can use it here now.

> > Why do we need a new refcnt dance here?  Why isn't it enough to just
> > use vfs_busy like all the existing callers?
> 
> Because vfs_busy() expects the caller to already hold a reference and
> one of the goals here is to drop the mount_list_lock as early as possible.

I suppose you want to avoid setting down a lock order for
mount_list_lock and mp->mnt_unmounting.  I guess this doesn't matter
much since it's limited to vfs internals.

> > Can you make another separate commit to drop the keepref parameter to
> > vfs_unbusy, and change all the callers with keepref=true to just use
> > vfs_destroy themselves?
> 
> You mean the opposite (keepref=false -> caller does vfs_destroy), yes?
> 
> Of the 81 usages of vfs_unbusy() only 6 have keepref=true, all others
> would have to be changed to "vfs_unbusy(mp); vfs_destroy(mp);".

Heh.  This confusion on my part suggest two things:

1. Perhaps in this change, we ought to add a different function for
use with mount iteration, say mountlist_iterator_next_done, so that
any legacy confusion about the vfs_busy/unbusy API is hidden:

	mountlist_iterator_init(&it);
	while ((mp = mountlist_iterator_next(&it)) != NULL) {
		...
		mountlist_iterator_next_done(&it, mp);
	}
	mountlist_iterator_destroy(&it);

2. Perhaps in another change, we ought to replace vfs_unbusy(mp,
false) by vfs_unbusy(mp), and vfs_unbusy(mp, true) by

vfs_acquire(mp);
vfs_unbusy(mp);

so that there is less confusing boolean-flagged asymmetry between
vfs_busy/vfs_unbusy.

Thus, the busy count (determining whether a mount can be unmounted
from the file system namespace) and the reference count (determining
whether the struct mount kernel data structures can be freed) would be
maintained in parallel:

	/* file system mount point busy count (implies reference count too) */
	error = vfs_busy(mp);
	if (error)
		goto fail;
	...
	vfs_unbusy(mp);

	/* data structure reference count */
	vfs_acquire(mp);
	...
	vfs_destroy(mp);

(Meanwhile, vfs_release would be a better name than vfs_destroy, in a
subsequent change.)