From 51a3f758d33231ca997d4440025aa74fa3417a2c Mon Sep 17 00:00:00 2001 From: riastradh Date: Mon, 28 Mar 2022 12:37:56 +0000 Subject: [PATCH] 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. --- sys/kern/vfs_vnode.c | 11 ++++------ sys/miscfs/specfs/spec_vnops.c | 40 +++++++++++++++++----------------- 2 files changed, 24 insertions(+), 27 deletions(-) diff --git a/sys/kern/vfs_vnode.c b/sys/kern/vfs_vnode.c index 39a2216fc3e5..dc6c3336e2dc 100644 --- a/sys/kern/vfs_vnode.c +++ b/sys/kern/vfs_vnode.c @@ -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 -__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 @@ vcache_reclaim(vnode_t *vp) 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 @@ vcache_reclaim(vnode_t *vp) /* * 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 @@ vcache_reclaim(vnode_t *vp) } 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 --git a/sys/miscfs/specfs/spec_vnops.c b/sys/miscfs/specfs/spec_vnops.c index 633d54fe890f..94c21fb37ace 100644 --- a/sys/miscfs/specfs/spec_vnops.c +++ b/sys/miscfs/specfs/spec_vnops.c @@ -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 -__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 #include @@ -591,6 +591,7 @@ spec_node_revoke(vnode_t *vp) { specnode_t *sn; specdev_t *sd; + struct vnode **vpp; KASSERT(VOP_ISLOCKED(vp) == LK_EXCLUSIVE); @@ -603,10 +604,10 @@ spec_node_revoke(vnode_t *vp) 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 @@ spec_node_revoke(vnode_t *vp) */ 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 @@ spec_node_destroy(vnode_t *vp) { specnode_t *sn; specdev_t *sd; - vnode_t **vpp, *vp2; int refcnt; sn = vp->v_specnode; @@ -647,22 +663,6 @@ spec_node_destroy(vnode_t *vp) 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--;