Re: nfs mp vnode list remove safe

[Alexander Bluhm]

On Tue, Jan 14, 2020 at 01:32:50PM -0500, Ted Unangst wrote:
> Alexander Bluhm wrote:
> >  loop:
> > -   TAILQ_FOREACH_SAFE(vp, >mnt_vnodelist, v_mntvnodes, nvp) {
> > +   TAILQ_FOREACH(vp, >mnt_vnodelist, v_mntvnodes) {
> > if (vp->v_mount != mp)  /* Paranoia */
> > goto loop;
>
> that looks like an infinite loop? if there's a bad vnode on the list, it'll
> still be there next time around.

Yes, that is stupid.  We have it in multiple loops.  Should be a
kassert.  Here it was added at Sat May 5 17:07:46 1990 UTC:

https://svnweb.freebsd.org/csrg/sys/kern/vfs_subr.c?r1=41420=41421;

That was the oldest commit I have found.  From there it spreaded
over the tree.  Have to check whether a kassert fits everywhere.

bluhm

Re: nfs mp vnode list remove safe

[Ted Unangst]

Alexander Bluhm wrote:
> Hi,
> 
> There is no remove and no sleep in this loop.  So _SAFE is unnecessary.
> 
> ok?

sure, with one bonus note.

> 
> bluhm
> 
> Index: nfs/nfs_subs.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/nfs/nfs_subs.c,v
> retrieving revision 1.141
> diff -u -p -r1.141 nfs_subs.c
> --- nfs/nfs_subs.c10 Jan 2020 10:33:35 -  1.141
> +++ nfs/nfs_subs.c13 Jan 2020 18:02:54 -
> @@ -1509,16 +1509,16 @@ netaddr_match(int family, union nethosta
>  void
>  nfs_clearcommit(struct mount *mp)
>  {
> - struct vnode *vp, *nvp;
> - struct buf *bp, *nbp;
> + struct vnode *vp;
> + struct buf *bp;
>   int s;
> 
>   s = splbio();
>  loop:
> - TAILQ_FOREACH_SAFE(vp, >mnt_vnodelist, v_mntvnodes, nvp) {
> + TAILQ_FOREACH(vp, >mnt_vnodelist, v_mntvnodes) {
>   if (vp->v_mount != mp)  /* Paranoia */
>   goto loop;

that looks like an infinite loop? if there's a bad vnode on the list, it'll
still be there next time around.