Subject: Re: mp->mnt_vnodelist change
To: Reinoud Zandijk <reinoud@netbsd.org>
From: Simon Burge <simonb@NetBSD.org>
List: tech-kern
Date: 10/20/2006 11:38:28
Reinoud Zandijk wrote:

> One userland program is affected, usr.sbin/pstat. AFAIK it should not break 
> any binary compatibilities but and old `pstat -v' will now give `vnode size 
> mismatch'.

Not much you can do about that - kmem grovellers will break each time
something like this changes (unless you want to sysctlise pstat, but
pstat will be "fun" to do this since there's lots of per-FS knowledge
in it).

In retrospect with the following suggestions, it would have been easier
to say "try to use TAILQ_FOREACH every time you see a for loop, rather
than list them all out...

> Index: sys/coda/coda_subr.c
> ===================================================================
> RCS file: /cvsroot/src/sys/coda/coda_subr.c,v
> retrieving revision 1.20
> diff -u -r1.20 coda_subr.c
> --- sys/coda/coda_subr.c	12 Oct 2006 01:30:47 -0000	1.20
> +++ sys/coda/coda_subr.c	19 Oct 2006 20:57:12 -0000
> @@ -312,10 +312,10 @@
>  	struct cnode *cp;
>  	int count = 0, bad = 0;
>  loop:
> -	for (vp = mp->mnt_vnodelist.lh_first; vp; vp = nvp) {
> +	for (vp = TAILQ_FIRST(&mp->mnt_vnodelist); vp; vp = nvp) {

Can you use TAILQ_FOREACH here

> Index: sys/fs/msdosfs/msdosfs_vfsops.c
> ===================================================================
> RCS file: /cvsroot/src/sys/fs/msdosfs/msdosfs_vfsops.c,v
> retrieving revision 1.36
> diff -u -r1.36 msdosfs_vfsops.c
> --- sys/fs/msdosfs/msdosfs_vfsops.c	12 Oct 2006 01:32:11 -0000	1.36
> +++ sys/fs/msdosfs/msdosfs_vfsops.c	19 Oct 2006 20:57:12 -0000
> @@ -904,7 +904,7 @@
>  	 */
>  	simple_lock(&mntvnode_slock);
>  loop:
> -	for (vp = mp->mnt_vnodelist.lh_first; vp != NULL; vp = nvp) {
> +	for (vp = TAILQ_FIRST(&mp->mnt_vnodelist); vp != NULL; vp = nvp) {

and here...

> Index: sys/fs/smbfs/smbfs_vfsops.c
> ===================================================================
> RCS file: /cvsroot/src/sys/fs/smbfs/smbfs_vfsops.c,v
> retrieving revision 1.55
> diff -u -r1.55 smbfs_vfsops.c
> --- sys/fs/smbfs/smbfs_vfsops.c	12 Oct 2006 01:32:14 -0000	1.55
> +++ sys/fs/smbfs/smbfs_vfsops.c	19 Oct 2006 20:57:12 -0000
> @@ -434,7 +434,7 @@
>  	 */
>  	simple_lock(&mntvnode_slock);
>  loop:
> -	for (vp = LIST_FIRST(&mp->mnt_vnodelist); vp != NULL; vp = nvp) {
> +	for (vp = TAILQ_FIRST(&mp->mnt_vnodelist); vp != NULL; vp = nvp) {

and here?  I looks like the "vp = nvp;" for each case is the same
as just saying "vp = vp->v_mntvnodes ....".

As they stand, wouldn't those changes above need to change the

	nvp = vp->v_mntvnodes.le_next;

line to

	nvp = TAILQ_NEXT(vp->v_mntvnodes)

?

> Index: sys/fs/union/union_vfsops.c
> ===================================================================
> RCS file: /cvsroot/src/sys/fs/union/union_vfsops.c,v
> retrieving revision 1.38
> diff -u -r1.38 union_vfsops.c
> --- sys/fs/union/union_vfsops.c	12 Oct 2006 01:32:14 -0000	1.38
> +++ sys/fs/union/union_vfsops.c	19 Oct 2006 20:57:12 -0000
> @@ -350,9 +350,9 @@
>  		int n;
>  
>  		/* count #vnodes held on mount list */
> -		for (n = 0, vp = mp->mnt_vnodelist.lh_first;
> +		for (n = 0, vp = TAILQ_FIRST(&mp->mnt_vnodelist);
>  				vp != NULLVP;
> -				vp = vp->v_mntvnodes.le_next)
> +				vp = TAILQ_NEXT(vp, v_mntvnodes))

I think this one can be a:

		n = 0;
		TAILQ_FOREACH( ... )

> Index: sys/kern/vfs_subr.c
> ===================================================================
> RCS file: /cvsroot/src/sys/kern/vfs_subr.c,v
> retrieving revision 1.271
> diff -u -r1.271 vfs_subr.c
> --- sys/kern/vfs_subr.c	12 Oct 2006 01:32:19 -0000	1.271
> +++ sys/kern/vfs_subr.c	19 Oct 2006 20:57:13 -0000
> @@ -1411,10 +1416,10 @@
>  
>  	simple_lock(&mntvnode_slock);
>  loop:
> -	for (vp = LIST_FIRST(&mp->mnt_vnodelist); vp; vp = nvp) {
> +	for (vp = TAILQ_FIRST(&mp->mnt_vnodelist); vp; vp = nvp) {
>  		if (vp->v_mount != mp)
>  			goto loop;
> -		nvp = LIST_NEXT(vp, v_mntvnodes);
> +		nvp = TAILQ_NEXT(vp, v_mntvnodes);

another TAILQ_FOREACH.

> @@ -2023,7 +2028,7 @@
>  		savebp = bp;
>  again:
>  		simple_lock(&mntvnode_slock);
> -		for (vp = LIST_FIRST(&mp->mnt_vnodelist);
> +		for (vp = TAILQ_FIRST(&mp->mnt_vnodelist);

and again...

> Index: sys/ufs/ext2fs/ext2fs_vfsops.c
> ===================================================================
> RCS file: /cvsroot/src/sys/ufs/ext2fs/ext2fs_vfsops.c,v
> retrieving revision 1.102
> diff -u -r1.102 ext2fs_vfsops.c
> --- sys/ufs/ext2fs/ext2fs_vfsops.c	12 Oct 2006 01:32:51 -0000	1.102
> +++ sys/ufs/ext2fs/ext2fs_vfsops.c	19 Oct 2006 20:57:14 -0000
> @@ -541,12 +541,12 @@
>  
>  loop:
>  	simple_lock(&mntvnode_slock);
> -	for (vp = mountp->mnt_vnodelist.lh_first; vp != NULL; vp = nvp) {
> +	for (vp = TAILQ_FIRST(&mountp->mnt_vnodelist); vp != NULL; vp = nvp) {

and again...

> @@ -846,7 +846,7 @@
>  	 */
>  	simple_lock(&mntvnode_slock);
>  loop:
> -	for (vp = LIST_FIRST(&mp->mnt_vnodelist); vp != NULL; vp = nvp) {
> +	for (vp = TAILQ_FIRST(&mp->mnt_vnodelist); vp != NULL; vp = nvp) {

and again...

> Index: sys/ufs/ffs/ffs_snapshot.c
> ===================================================================
> RCS file: /cvsroot/src/sys/ufs/ffs/ffs_snapshot.c,v
> retrieving revision 1.33
> diff -u -r1.33 ffs_snapshot.c
> --- sys/ufs/ffs/ffs_snapshot.c	12 Oct 2006 01:32:51 -0000	1.33
> +++ sys/ufs/ffs/ffs_snapshot.c	19 Oct 2006 20:57:14 -0000
> @@ -363,14 +363,14 @@
>  	    FSMAXSNAP + 1 /* superblock */ + 1 /* last block */ + 1 /* size */;
>  	MNT_ILOCK(mp);
>  loop:
> -	for (xvp = LIST_FIRST(&mp->mnt_vnodelist); xvp; xvp = nvp) {
> +	for (xvp = TAILQ_FIRST(&mp->mnt_vnodelist); xvp; xvp = nvp) {

and again...

> Index: sys/ufs/ffs/ffs_vfsops.c
> ===================================================================
> RCS file: /cvsroot/src/sys/ufs/ffs/ffs_vfsops.c,v
> retrieving revision 1.187
> diff -u -r1.187 ffs_vfsops.c
> --- sys/ufs/ffs/ffs_vfsops.c	12 Oct 2006 01:32:51 -0000	1.187
> +++ sys/ufs/ffs/ffs_vfsops.c	19 Oct 2006 20:57:15 -0000
> @@ -650,12 +650,12 @@
>  
>  loop:
>  	simple_lock(&mntvnode_slock);
> -	for (vp = mp->mnt_vnodelist.lh_first; vp != NULL; vp = nvp) {
> +	for (vp = TAILQ_FIRST(&mp->mnt_vnodelist); vp != NULL; vp = nvp) {

and again...

> @@ -1314,7 +1314,7 @@
>  	 */
>  	simple_lock(&mntvnode_slock);
>  loop:
> -	for (vp = LIST_FIRST(&mp->mnt_vnodelist); vp != NULL; vp = nvp) {
> +	for (vp = TAILQ_FIRST(&mp->mnt_vnodelist); vp != NULL; vp = nvp) {

and again (are we detecting a pattern yet? :-)...

> Index: sys/ufs/lfs/lfs_segment.c
> ===================================================================
> RCS file: /cvsroot/src/sys/ufs/lfs/lfs_segment.c,v
> retrieving revision 1.193
> diff -u -r1.193 lfs_segment.c
> --- sys/ufs/lfs/lfs_segment.c	12 Oct 2006 01:32:51 -0000	1.193
> +++ sys/ufs/lfs/lfs_segment.c	19 Oct 2006 20:57:15 -0000
> @@ -494,26 +494,16 @@
>  	int error = 0;
>  
>  	ASSERT_SEGLOCK(fs);
> -#ifndef LFS_NO_BACKVP_HACK
> -	/* BEGIN HACK */
> -#define	VN_OFFSET	\
> -	(((caddr_t)&LIST_NEXT(vp, v_mntvnodes)) - (caddr_t)vp)
> -#define	BACK_VP(VP)	\
> -	((struct vnode *)(((caddr_t)(VP)->v_mntvnodes.le_prev) - VN_OFFSET))
> -#define	BEG_OF_VLIST	\
> -	((struct vnode *)(((caddr_t)&LIST_FIRST(&mp->mnt_vnodelist)) \
> -	- VN_OFFSET))
> -
> -	/* Find last vnode. */
> - loop:	for (vp = LIST_FIRST(&mp->mnt_vnodelist);
> -	     vp && LIST_NEXT(vp, v_mntvnodes) != NULL;
> -	     vp = LIST_NEXT(vp, v_mntvnodes));
> -	for (; vp && vp != BEG_OF_VLIST; vp = nvp) {
> -		nvp = BACK_VP(vp);
> +#if 0
> +	/* start at last vnode. */
> +	loop:
> +	for (vp = TAILQ_LAST(&mp->mnt_vnodelist, vnodelst); vp; vp = nvp) {
> +		nvp = TAILQ_PREV(vp, vnodelst, v_mntvnodes);
and again...

>  #else
> +	/* start at oldest accessed vnode */
>  	loop:
> -	for (vp = LIST_FIRST(&mp->mnt_vnodelist); vp; vp = nvp) {
> -		nvp = LIST_NEXT(vp, v_mntvnodes);
> +	for (vp = TAILQ_FIRST(&mp->mnt_vnodelist); vp; vp = nvp) {
> +		nvp = TAILQ_NEXT(vp, v_mntvnodes);

and again...

> Index: sys/ufs/ufs/ufs_quota.c
> ===================================================================
> RCS file: /cvsroot/src/sys/ufs/ufs/ufs_quota.c,v
> retrieving revision 1.41
> diff -u -r1.41 ufs_quota.c
> --- sys/ufs/ufs/ufs_quota.c	23 Jul 2006 22:06:15 -0000	1.41
> +++ sys/ufs/ufs/ufs_quota.c	19 Oct 2006 20:57:15 -0000
> @@ -384,8 +384,8 @@
>  	 * NB: only need to add dquot's for inodes being modified.
>  	 */
>  again:
> -	for (vp = LIST_FIRST(&mp->mnt_vnodelist); vp != NULL; vp = nextvp) {
> -		nextvp = LIST_NEXT(vp, v_mntvnodes);
> +	for (vp = TAILQ_FIRST(&mp->mnt_vnodelist); vp != NULL; vp = nextvp) {
> +		nextvp = TAILQ_NEXT(vp, v_mntvnodes);

and again...

> @@ -425,8 +425,8 @@
>  	 * deleting any references to quota file being closed.
>  	 */
>  again:
> -	for (vp = LIST_FIRST(&mp->mnt_vnodelist); vp != NULL; vp = nextvp) {
> -		nextvp = LIST_NEXT(vp, v_mntvnodes);
> +	for (vp = TAILQ_FIRST(&mp->mnt_vnodelist); vp != NULL; vp = nextvp) {
> +		nextvp = TAILQ_NEXT(vp, v_mntvnodes);

and again...

> @@ -595,10 +595,10 @@
>  	 */
>  	simple_lock(&mntvnode_slock);
>  again:
> -	for (vp = LIST_FIRST(&mp->mnt_vnodelist); vp != NULL; vp = nextvp) {
> +	for (vp = TAILQ_FIRST(&mp->mnt_vnodelist); vp != NULL; vp = nextvp) {

and again...

> Index: usr.sbin/pstat/pstat.c
> ===================================================================
> RCS file: /cvsroot/src/usr.sbin/pstat/pstat.c,v
> retrieving revision 1.95
> diff -u -r1.95 pstat.c
> --- usr.sbin/pstat/pstat.c	25 May 2006 01:49:30 -0000	1.95
> +++ usr.sbin/pstat/pstat.c	19 Oct 2006 20:57:16 -0000
> @@ -740,8 +740,8 @@
>  	for (mp = mountlist.cqh_first;;
>  	    mp = mount.mnt_list.cqe_next) {
>  		KGET2(mp, &mount, sizeof(mount), "mount entry");
> -		for (vp = mount.mnt_vnodelist.lh_first;
> -		    vp != NULL; vp = vnode.v_mntvnodes.le_next) {
> +		for (vp = TAILQ_FIRST(&mount.mnt_vnodelist);
> +		    vp != NULL; vp = TAILQ_NEXT(&vnode, v_mntvnodes)) {

TAILQ_FOREACH should be OK here too I think.

Simon.