From owner-svn-src-all@freebsd.org Fri Jan 24 07:47:45 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 7116D230FBA; Fri, 24 Jan 2020 07:47:45 +0000 (UTC) (envelope-from mjg@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 483rpT1lgYz4dfH; Fri, 24 Jan 2020 07:47:45 +0000 (UTC) (envelope-from mjg@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 3732124104; Fri, 24 Jan 2020 07:47:45 +0000 (UTC) (envelope-from mjg@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 00O7liFV021324; Fri, 24 Jan 2020 07:47:44 GMT (envelope-from mjg@FreeBSD.org) Received: (from mjg@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 00O7li83021323; Fri, 24 Jan 2020 07:47:44 GMT (envelope-from mjg@FreeBSD.org) Message-Id: <202001240747.00O7li83021323@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: mjg set sender to mjg@FreeBSD.org using -f From: Mateusz Guzik Date: Fri, 24 Jan 2020 07:47:44 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r357072 - head/sys/kern X-SVN-Group: head X-SVN-Commit-Author: mjg X-SVN-Commit-Paths: head/sys/kern X-SVN-Commit-Revision: 357072 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Jan 2020 07:47:45 -0000 Author: mjg Date: Fri Jan 24 07:47:44 2020 New Revision: 357072 URL: https://svnweb.freebsd.org/changeset/base/357072 Log: vfs: allow v_usecount to transition 0->1 without the interlock There is nothing to do but to bump the count even during said transition. There are 2 places which can do it: - vget only does this after locking the vnode, meaning there is no change in contract versus inactive or reclamantion - vref only ever did it with the interlock held which did not protect against either (that is, it would always succeed) VCHR vnodes retain special casing due to the need to maintain dev use count. Reviewed by: jeff, kib Tested by: pho (previous version) Differential Revision: https://reviews.freebsd.org/D23185 Modified: head/sys/kern/vfs_subr.c Modified: head/sys/kern/vfs_subr.c ============================================================================== --- head/sys/kern/vfs_subr.c Fri Jan 24 07:45:59 2020 (r357071) +++ head/sys/kern/vfs_subr.c Fri Jan 24 07:47:44 2020 (r357072) @@ -2826,11 +2826,10 @@ v_decr_devcount(struct vnode *vp) * see doomed vnodes. If inactive processing was delayed in * vput try to do it here. * - * usecount is manipulated using atomics without holding any locks, - * except when transitioning 0->1 in which case the interlock is held. - - * holdcnt is manipulated using atomics without holding any locks, - * except when transitioning 1->0 in which case the interlock is held. + * usecount is manipulated using atomics without holding any locks. + * + * holdcnt can be manipulated using atomics without holding any locks, + * except when transitioning 1<->0, in which case the interlock is held. */ enum vgetstate vget_prep(struct vnode *vp) @@ -2857,10 +2856,46 @@ vget(struct vnode *vp, int flags, struct thread *td) return (vget_finish(vp, flags, vs)); } +static int __noinline +vget_finish_vchr(struct vnode *vp) +{ + + VNASSERT(vp->v_type == VCHR, vp, ("type != VCHR)")); + + /* + * See the comment in vget_finish before usecount bump. + */ + if (refcount_acquire_if_not_zero(&vp->v_usecount)) { +#ifdef INVARIANTS + int old = atomic_fetchadd_int(&vp->v_holdcnt, -1); + VNASSERT(old > 0, vp, ("%s: wrong hold count %d", __func__, old)); +#else + refcount_release(&vp->v_holdcnt); +#endif + return (0); + } + + VI_LOCK(vp); + if (refcount_acquire_if_not_zero(&vp->v_usecount)) { +#ifdef INVARIANTS + int old = atomic_fetchadd_int(&vp->v_holdcnt, -1); + VNASSERT(old > 1, vp, ("%s: wrong hold count %d", __func__, old)); +#else + refcount_release(&vp->v_holdcnt); +#endif + VI_UNLOCK(vp); + return (0); + } + v_incr_devcount(vp); + refcount_acquire(&vp->v_usecount); + VI_UNLOCK(vp); + return (0); +} + int vget_finish(struct vnode *vp, int flags, enum vgetstate vs) { - int error; + int error, old; VNASSERT((flags & LK_TYPE_MASK) != 0, vp, ("%s: invalid lock operation", __func__)); @@ -2890,69 +2925,106 @@ vget_finish(struct vnode *vp, int flags, enum vgetstat return (0); } + if (__predict_false(vp->v_type == VCHR)) + return (vget_finish_vchr(vp)); + /* * We hold the vnode. If the usecount is 0 it will be utilized to keep * the vnode around. Otherwise someone else lended their hold count and * we have to drop ours. */ - if (refcount_acquire_if_not_zero(&vp->v_usecount)) { + old = atomic_fetchadd_int(&vp->v_usecount, 1); + VNASSERT(old >= 0, vp, ("%s: wrong use count %d", __func__, old)); + if (old != 0) { #ifdef INVARIANTS - int old = atomic_fetchadd_int(&vp->v_holdcnt, -1); + old = atomic_fetchadd_int(&vp->v_holdcnt, -1); VNASSERT(old > 1, vp, ("%s: wrong hold count %d", __func__, old)); #else refcount_release(&vp->v_holdcnt); #endif - return (0); } + return (0); +} +/* + * Increase the reference (use) and hold count of a vnode. + * This will also remove the vnode from the free list if it is presently free. + */ +static void __noinline +vref_vchr(struct vnode *vp, bool interlock) +{ + /* - * We don't guarantee that any particular close will - * trigger inactive processing so just make a best effort - * here at preventing a reference to a removed file. If - * we don't succeed no harm is done. - * - * Upgrade our holdcnt to a usecount. + * See the comment in vget_finish before usecount bump. */ - VI_LOCK(vp); - /* - * See the previous section. By the time we get here we may find - * ourselves in the same spot. - */ + if (!interlock) { + if (refcount_acquire_if_not_zero(&vp->v_usecount)) { + VNODE_REFCOUNT_FENCE_ACQ(); + VNASSERT(vp->v_holdcnt > 0, vp, + ("%s: active vnode not held", __func__)); + return; + } + VI_LOCK(vp); + /* + * By the time we get here the vnode might have been doomed, at + * which point the 0->1 use count transition is no longer + * protected by the interlock. Since it can't bounce back to + * VCHR and requires vref semantics, punt it back + */ + if (__predict_false(vp->v_type == VBAD)) { + VI_UNLOCK(vp); + vref(vp); + return; + } + } + VNASSERT(vp->v_type == VCHR, vp, ("type != VCHR)")); if (refcount_acquire_if_not_zero(&vp->v_usecount)) { -#ifdef INVARIANTS - int old = atomic_fetchadd_int(&vp->v_holdcnt, -1); - VNASSERT(old > 1, vp, ("%s: wrong hold count %d", __func__, old)); -#else - refcount_release(&vp->v_holdcnt); -#endif - VI_UNLOCK(vp); - return (0); + VNODE_REFCOUNT_FENCE_ACQ(); + VNASSERT(vp->v_holdcnt > 0, vp, + ("%s: active vnode not held", __func__)); + if (!interlock) + VI_UNLOCK(vp); + return; } + vhold(vp); v_incr_devcount(vp); refcount_acquire(&vp->v_usecount); - VI_UNLOCK(vp); - return (0); + if (!interlock) + VI_UNLOCK(vp); + return; } -/* - * Increase the reference (use) and hold count of a vnode. - * This will also remove the vnode from the free list if it is presently free. - */ void vref(struct vnode *vp) { + int old; - ASSERT_VI_UNLOCKED(vp, __func__); CTR2(KTR_VFS, "%s: vp %p", __func__, vp); + if (__predict_false(vp->v_type == VCHR)) { + vref_vchr(vp, false); + return; + } + if (refcount_acquire_if_not_zero(&vp->v_usecount)) { VNODE_REFCOUNT_FENCE_ACQ(); VNASSERT(vp->v_holdcnt > 0, vp, ("%s: active vnode not held", __func__)); return; } - VI_LOCK(vp); - vrefl(vp); - VI_UNLOCK(vp); + vhold(vp); + /* + * See the comment in vget_finish. + */ + old = atomic_fetchadd_int(&vp->v_usecount, 1); + VNASSERT(old >= 0, vp, ("%s: wrong use count %d", __func__, old)); + if (old != 0) { +#ifdef INVARIANTS + old = atomic_fetchadd_int(&vp->v_holdcnt, -1); + VNASSERT(old > 1, vp, ("%s: wrong hold count %d", __func__, old)); +#else + refcount_release(&vp->v_holdcnt); +#endif + } } void @@ -2961,15 +3033,11 @@ vrefl(struct vnode *vp) ASSERT_VI_LOCKED(vp, __func__); CTR2(KTR_VFS, "%s: vp %p", __func__, vp); - if (refcount_acquire_if_not_zero(&vp->v_usecount)) { - VNODE_REFCOUNT_FENCE_ACQ(); - VNASSERT(vp->v_holdcnt > 0, vp, - ("%s: active vnode not held", __func__)); + if (__predict_false(vp->v_type == VCHR)) { + vref_vchr(vp, true); return; } - vholdl(vp); - v_incr_devcount(vp); - refcount_acquire(&vp->v_usecount); + vref(vp); } void @@ -3147,8 +3215,6 @@ vputx(struct vnode *vp, enum vputx_op func) } break; } - VNASSERT(vp->v_usecount == 0 || (vp->v_iflag & VI_OWEINACT) == 0, vp, - ("vnode with usecount and VI_OWEINACT set")); if (error == 0) { vinactive(vp); if (func != VPUTX_VUNREF)