FBSD: Give 0 'rootrefs' to vflush on unmount 09/13709/4
authorAndrew Deason <adeason@dson.org>
Sun, 21 Jul 2019 04:09:27 +0000 (23:09 -0500)
committerBenjamin Kaduk <kaduk@mit.edu>
Fri, 23 Aug 2019 03:12:12 +0000 (23:12 -0400)
Currently, in afs_unmount, we give vflush a 'rootrefs' arg of 1,
indicating that we hold 1 reference on the root vnode. But ever since
commit 6eb1088a (freebsd: properly track vcache references), we drop
the ref for the root vnode at the beginning of this function.

What happens currently in afs_unmount for a normal successful umount
is something like this (at least, on FreeBSD 11.2-RELEASE):

- We afs_PutVCache the afs_globalVp vcache, reducing its v_usecount
  and v_holdcnt to 0, and afs_globalVp is set to NULL.

- vflush calls afs_root() to get the root vnode, which sees that
  afs_globalVp is NULL, and so calls afs_GetVCache for the root fid
  and returns it (and sets afs_globalVp to that vcache), with a
  v_usecount of 1.

- vflush tries to vgonel() all of our vnodes, which calls our
  afs_vop_reclaim, which calls afs_FlushVCache(). For the root vnode
  specifically, vflush() sees that v_usecount is nonzero, and so skips
  calling vgonel() at first, but later calls vgone() on it
  specifically because we gave a nonzero 'rootrefs'. The resulting
  afs_FlushVCache() for the root vnode fails, because the root vnode's
  v_usecount is still 1. Since a failure from afs_vop_reclaim would
  cause a panic, we just log a warning and try to continue on anyway.

- vflush() calls vrele() on the root vnode, right before returning.

All of this allows the unmount to proceed, but this means that most of
afs_FlushVCache() doesn't actually run for the root vcache, and it
means we always log a warning like this on unmount:

    afs_vop_reclaim: afs_FlushVCache failed code 16 [...]

In addition, this means that setting afs_globalVp at the beginning of
afs_unmount() is largely pointless, since it gets set to a vcache
again near the beginning of vflush().

To avoid all of this, stop lying to vflush about how many references
to the root vnode we hold, and just say that we hold 0 references.

Change-Id: Ib434c5fc48e67c3863fcad41279c3d9e0e0b8c2b
Reviewed-on: https://gerrit.openafs.org/13709
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>

src/afs/FBSD/osi_vfsops.c

index c9361fe..d8dabd1 100644 (file)
@@ -222,19 +222,18 @@ afs_unmount(struct mount *mp, int flags, struct thread *p)
        error = EBUSY;
     AFS_GUNLOCK();
 
-    /*
-     * Release any remaining vnodes on this mount point.
-     * The `1' means that we hold one extra reference on
-     * the root vnode (this is just a guess right now).
-     * This has to be done outside the global lock.
-     */
     if (!error) {
+       /*
+        * Release any remaining vnodes on this mount point. The second
+        * argument is how many refs we hold on the root vnode. Since we
+        * released our reference to the root vnode up above, give 0.
+        */
 #if defined(AFS_FBSD80_ENV)
-       error = vflush(mp, 1, (flags & MNT_FORCE) ? FORCECLOSE : 0, curthread);
+       error = vflush(mp, 0, (flags & MNT_FORCE) ? FORCECLOSE : 0, curthread);
 #elif defined(AFS_FBSD53_ENV)
-       error = vflush(mp, 1, (flags & MNT_FORCE) ? FORCECLOSE : 0, p);
+       error = vflush(mp, 0, (flags & MNT_FORCE) ? FORCECLOSE : 0, p);
 #else
-       error = vflush(mp, 1, (flags & MNT_FORCE) ? FORCECLOSE : 0);
+       error = vflush(mp, 0, (flags & MNT_FORCE) ? FORCECLOSE : 0);
 #endif
     }
     if (error)