From owner-svn-src-head@FreeBSD.ORG Mon Dec 3 22:15:16 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id B099D4FB; Mon, 3 Dec 2012 22:15:16 +0000 (UTC) (envelope-from kib@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:1900:2254:2068::e6a:0]) by mx1.freebsd.org (Postfix) with ESMTP id 9847C8FC15; Mon, 3 Dec 2012 22:15:16 +0000 (UTC) Received: from svn.freebsd.org (localhost [127.0.0.1]) by svn.freebsd.org (8.14.5/8.14.5) with ESMTP id qB3MFGDA062682; Mon, 3 Dec 2012 22:15:16 GMT (envelope-from kib@svn.freebsd.org) Received: (from kib@localhost) by svn.freebsd.org (8.14.5/8.14.5/Submit) id qB3MFGST062681; Mon, 3 Dec 2012 22:15:16 GMT (envelope-from kib@svn.freebsd.org) Message-Id: <201212032215.qB3MFGST062681@svn.freebsd.org> From: Konstantin Belousov Date: Mon, 3 Dec 2012 22:15:16 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r243835 - head/sys/kern X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Dec 2012 22:15:16 -0000 Author: kib Date: Mon Dec 3 22:15:16 2012 New Revision: 243835 URL: http://svnweb.freebsd.org/changeset/base/243835 Log: The vnode_free_list_mtx is required unconditionally when iterating over the active list. The mount interlock is not enough to guarantee the validity of the tailq link pointers. The __mnt_vnode_next_active() and __mnt_vnode_first_active() active lists iterators helper functions did not provided the neccessary stability for the list, allowing the iterators to pick garbage. This was uncovered after the r243599 made the active list iterators non-nop. Since a vnode interlock is before the vnode_free_list_mtx, obtain the vnode ilock in the non-blocking manner when under vnode_free_list_mtx, and restart iteration after the yield if the lock attempt failed. Assert that a vnode found on the list is active, and assert that the helpers return the vnode with interlock owned. Reported and tested by: pho MFC after: 1 week Modified: head/sys/kern/vfs_subr.c Modified: head/sys/kern/vfs_subr.c ============================================================================== --- head/sys/kern/vfs_subr.c Mon Dec 3 21:49:37 2012 (r243834) +++ head/sys/kern/vfs_subr.c Mon Dec 3 22:15:16 2012 (r243835) @@ -4718,10 +4718,20 @@ __mnt_vnode_next_active(struct vnode **m if (should_yield()) kern_yield(PRI_UNCHANGED); MNT_ILOCK(mp); +restart: + mtx_lock(&vnode_free_list_mtx); KASSERT((*mvp)->v_mount == mp, ("marker vnode mount list mismatch")); vp = TAILQ_NEXT(*mvp, v_actfreelist); while (vp != NULL) { - VI_LOCK(vp); + if (vp->v_type == VMARKER) { + vp = TAILQ_NEXT(vp, v_actfreelist); + continue; + } + if (!VI_TRYLOCK(vp)) { + mtx_unlock(&vnode_free_list_mtx); + kern_yield(PRI_UNCHANGED); + goto restart; + } if (vp->v_mount == mp && vp->v_type != VMARKER && (vp->v_iflag & VI_DOOMED) == 0) break; @@ -4732,16 +4742,18 @@ __mnt_vnode_next_active(struct vnode **m /* Check if we are done */ if (vp == NULL) { + mtx_unlock(&vnode_free_list_mtx); __mnt_vnode_markerfree_active(mvp, mp); /* MNT_IUNLOCK(mp); -- done in above function */ mtx_assert(MNT_MTX(mp), MA_NOTOWNED); return (NULL); } - mtx_lock(&vnode_free_list_mtx); TAILQ_REMOVE(&mp->mnt_activevnodelist, *mvp, v_actfreelist); TAILQ_INSERT_AFTER(&mp->mnt_activevnodelist, vp, *mvp, v_actfreelist); mtx_unlock(&vnode_free_list_mtx); MNT_IUNLOCK(mp); + ASSERT_VI_LOCKED(vp, "active iter"); + KASSERT((vp->v_iflag & VI_ACTIVE) != 0, ("Non-active vp %p", vp)); return (vp); } @@ -4755,9 +4767,19 @@ __mnt_vnode_first_active(struct vnode ** MNT_REF(mp); (*mvp)->v_type = VMARKER; +restart: + mtx_lock(&vnode_free_list_mtx); vp = TAILQ_FIRST(&mp->mnt_activevnodelist); while (vp != NULL) { - VI_LOCK(vp); + if (vp->v_type == VMARKER) { + vp = TAILQ_NEXT(vp, v_actfreelist); + continue; + } + if (!VI_TRYLOCK(vp)) { + mtx_unlock(&vnode_free_list_mtx); + kern_yield(PRI_UNCHANGED); + goto restart; + } if (vp->v_mount == mp && vp->v_type != VMARKER && (vp->v_iflag & VI_DOOMED) == 0) break; @@ -4768,6 +4790,7 @@ __mnt_vnode_first_active(struct vnode ** /* Check if we are done */ if (vp == NULL) { + mtx_unlock(&vnode_free_list_mtx); MNT_REL(mp); MNT_IUNLOCK(mp); free(*mvp, M_VNODE_MARKER); @@ -4775,10 +4798,11 @@ __mnt_vnode_first_active(struct vnode ** return (NULL); } (*mvp)->v_mount = mp; - mtx_lock(&vnode_free_list_mtx); TAILQ_INSERT_AFTER(&mp->mnt_activevnodelist, vp, *mvp, v_actfreelist); mtx_unlock(&vnode_free_list_mtx); MNT_IUNLOCK(mp); + ASSERT_VI_LOCKED(vp, "active iter first"); + KASSERT((vp->v_iflag & VI_ACTIVE) != 0, ("Non-active vp %p", vp)); return (vp); }