[src/trunk]: src/sys specfs: Remove specnode from hash table in spec_node_rev...

[riastradh <riastradh%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/dc714d194877
branches:  trunk
changeset: 364520:dc714d194877
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Mon Mar 28 12:37:56 2022 +0000

description:
specfs: Remove specnode from hash table in spec_node_revoke.

Previously, it was possible for spec_node_lookup_by_dev to handle a
speconde that a concurrent spec_node_destroy is about to remove from
the hash table and then free, as soon as spec_node_lookup_by_dev
releases device_lock.

Now, the ordering is:

1. Remove specnode from hash table in spec_node_revoke.  At this
   point, no _new_ vnode references are possible (other than possibly
   one acquired by vcache_vget under v_interlock), but there may be
   existing ones.

2. Mark vnode reclaimed so vcache_vget will fail.

3. The last vrele (or equivalent logic in vcache_vget) will then free
   the specnode in spec_node_destroy.

This way, _if_ a thread in spec_node_lookup_by_dev finds a specnode
in the hash table under device_lock/v_interlock, _then_ it will not
be freed until the thread completes vcache_vget.

This change requires calling spec_node_revoke unconditionally for
device special nodes, not just for active ones.  Might introduce
slightly more contention on device_lock but not much because we
already have to take it in this path anyway a little later in
spec_node_destroy.

diffstat:

 sys/kern/vfs_vnode.c           |  11 ++++-------
 sys/miscfs/specfs/spec_vnops.c |  40 ++++++++++++++++++++--------------------
 2 files changed, 24 insertions(+), 27 deletions(-)

diffs (144 lines):

diff -r f1c45b242bc6 -r dc714d194877 sys/kern/vfs_vnode.c
--- a/sys/kern/vfs_vnode.c      Mon Mar 28 12:37:46 2022 +0000
+++ b/sys/kern/vfs_vnode.c      Mon Mar 28 12:37:56 2022 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: vfs_vnode.c,v 1.140 2022/03/28 12:37:46 riastradh Exp $        */
+/*     $NetBSD: vfs_vnode.c,v 1.141 2022/03/28 12:37:56 riastradh Exp $        */
 
 /*-
  * Copyright (c) 1997-2011, 2019, 2020 The NetBSD Foundation, Inc.
@@ -148,7 +148,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: vfs_vnode.c,v 1.140 2022/03/28 12:37:46 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: vfs_vnode.c,v 1.141 2022/03/28 12:37:56 riastradh Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_pax.h"
@@ -1811,14 +1811,13 @@
        uint32_t hash;
        uint8_t temp_buf[64], *temp_key;
        size_t temp_key_len;
-       bool recycle, active;
+       bool recycle;
        int error;
 
        KASSERT(VOP_ISLOCKED(vp) == LK_EXCLUSIVE);
        KASSERT(mutex_owned(vp->v_interlock));
        KASSERT(vrefcnt(vp) != 0);
 
-       active = (vrefcnt(vp) > 1);
        temp_key_len = vip->vi_key.vk_key_len;
        /*
         * Prevent the vnode from being recycled or brought into use
@@ -1861,8 +1860,6 @@
 
        /*
         * Clean out any cached data associated with the vnode.
-        * If purging an active vnode, it must be closed and
-        * deactivated before being reclaimed.
         */
        error = vinvalbuf(vp, V_SAVE, NOCRED, l, 0, 0);
        if (error != 0) {
@@ -1872,7 +1869,7 @@
        }
        KASSERTMSG((error == 0), "vinvalbuf failed: %d", error);
        KASSERT((vp->v_iflag & VI_ONWORKLST) == 0);
-       if (active && (vp->v_type == VBLK || vp->v_type == VCHR)) {
+       if (vp->v_type == VBLK || vp->v_type == VCHR) {
                 spec_node_revoke(vp);
        }
 
diff -r f1c45b242bc6 -r dc714d194877 sys/miscfs/specfs/spec_vnops.c
--- a/sys/miscfs/specfs/spec_vnops.c    Mon Mar 28 12:37:46 2022 +0000
+++ b/sys/miscfs/specfs/spec_vnops.c    Mon Mar 28 12:37:56 2022 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: spec_vnops.c,v 1.208 2022/03/28 12:37:46 riastradh Exp $       */
+/*     $NetBSD: spec_vnops.c,v 1.209 2022/03/28 12:37:56 riastradh Exp $       */
 
 /*-
  * Copyright (c) 2008 The NetBSD Foundation, Inc.
@@ -58,7 +58,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.208 2022/03/28 12:37:46 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: spec_vnops.c,v 1.209 2022/03/28 12:37:56 riastradh Exp $");
 
 #include <sys/param.h>
 #include <sys/proc.h>
@@ -591,6 +591,7 @@
 {
        specnode_t *sn;
        specdev_t *sd;
+       struct vnode **vpp;
 
        KASSERT(VOP_ISLOCKED(vp) == LK_EXCLUSIVE);
 
@@ -603,10 +604,10 @@
 
        mutex_enter(&device_lock);
        KASSERT(sn->sn_opencnt <= sd->sd_opencnt);
+       sn->sn_gone = true;
        if (sn->sn_opencnt != 0) {
                sd->sd_opencnt -= (sn->sn_opencnt - 1);
                sn->sn_opencnt = 1;
-               sn->sn_gone = true;
                mutex_exit(&device_lock);
 
                VOP_CLOSE(vp, FNONBLOCK, NOCRED);
@@ -624,6 +625,22 @@
         */
        while (sd->sd_closing)
                cv_wait(&specfs_iocv, &device_lock);
+
+       /*
+        * Remove from the hash so lookups stop returning this
+        * specnode.  We will dissociate it from the specdev -- and
+        * possibly free the specdev -- in spec_node_destroy.
+        */
+       KASSERT(sn->sn_gone);
+       KASSERT(sn->sn_opencnt == 0);
+       for (vpp = &specfs_hash[SPECHASH(vp->v_rdev)];;
+            vpp = &(*vpp)->v_specnext) {
+               if (*vpp == vp) {
+                       *vpp = vp->v_specnext;
+                       vp->v_specnext = NULL;
+                       break;
+               }
+       }
        mutex_exit(&device_lock);
 }
 
@@ -636,7 +653,6 @@
 {
        specnode_t *sn;
        specdev_t *sd;
-       vnode_t **vpp, *vp2;
        int refcnt;
 
        sn = vp->v_specnode;
@@ -647,22 +663,6 @@
        KASSERT(sn->sn_opencnt == 0);
 
        mutex_enter(&device_lock);
-       /* Remove from the hash and destroy the node. */
-       vpp = &specfs_hash[SPECHASH(vp->v_rdev)];
-       for (vp2 = *vpp;; vp2 = vp2->v_specnext) {
-               if (vp2 == NULL) {
-                       panic("spec_node_destroy: corrupt hash");
-               }
-               if (vp2 == vp) {
-                       KASSERT(vp == *vpp);
-                       *vpp = vp->v_specnext;
-                       break;
-               }
-               if (vp2->v_specnext == vp) {
-                       vp2->v_specnext = vp->v_specnext;
-                       break;
-               }
-       }
        sn = vp->v_specnode;
        vp->v_specnode = NULL;
        refcnt = sd->sd_refcnt--;