Re: svn commit: r318285 - head/sys/kern

[Konstantin Belousov]

On Mon, May 15, 2017 at 10:02:45AM +, Konstantin Belousov wrote:
> Author: kib
> Date: Mon May 15 10:02:45 2017
> New Revision: 318285
> URL: https://svnweb.freebsd.org/changeset/base/318285
> 
> Log:
>   mnt_vnode_next_active: use conventional lock order when trylock fails.
>   
>   Previously, when the VI_TRYLOCK failed, we would spin under the mutex
>   that protects the vnode active list until we either succeeded or
>   noticed that we had hogged the CPU. Since we were violating the lock
>   order, this would guarantee that we would become a hog under any
>   deadlock condition (e.g. a race with vdrop(9) on the same vnode). In
>   the presence of many concurrent threads in sync(2) or vdrop etc, the
>   victim could hang for a long time.
>   
>   Now, avoid spinning by dropping and reacquiring the locks in the
>   conventional lock order when the trylock fails. This requires a dance
>   with the vnode hold count.
>   
>   Submitted by:   Tom Rix 
This must be Ryan Libby , I am sorry.

>   Tested by:  pho
>   Differential revision:  https://reviews.freebsd.org/D10692
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

svn commit: r318285 - head/sys/kern

[Konstantin Belousov]

Author: kib
Date: Mon May 15 10:02:45 2017
New Revision: 318285
URL: https://svnweb.freebsd.org/changeset/base/318285

Log:
  mnt_vnode_next_active: use conventional lock order when trylock fails.
  
  Previously, when the VI_TRYLOCK failed, we would spin under the mutex
  that protects the vnode active list until we either succeeded or
  noticed that we had hogged the CPU. Since we were violating the lock
  order, this would guarantee that we would become a hog under any
  deadlock condition (e.g. a race with vdrop(9) on the same vnode). In
  the presence of many concurrent threads in sync(2) or vdrop etc, the
  victim could hang for a long time.
  
  Now, avoid spinning by dropping and reacquiring the locks in the
  conventional lock order when the trylock fails. This requires a dance
  with the vnode hold count.
  
  Submitted by: Tom Rix 
  Tested by:pho
  Differential revision:https://reviews.freebsd.org/D10692

Modified:
  head/sys/kern/vfs_subr.c

Modified: head/sys/kern/vfs_subr.c
==
--- head/sys/kern/vfs_subr.cMon May 15 03:39:35 2017(r318284)
+++ head/sys/kern/vfs_subr.cMon May 15 10:02:45 2017(r318285)
@@ -5362,6 +5362,84 @@ mnt_vnode_markerfree_active(struct vnode
*mvp = NULL;
 }
 
+/*
+ * Relock the mp mount vnode list lock with the vp vnode interlock in the
+ * conventional lock order during mnt_vnode_next_active iteration.
+ *
+ * On entry, the mount vnode list lock is held and the vnode interlock is not.
+ * The list lock is dropped and reacquired.  On success, both locks are held.
+ * On failure, the mount vnode list lock is held but the vnode interlock is
+ * not, and the procedure may have yielded.
+ */
+static bool
+mnt_vnode_next_active_relock(struct vnode *mvp, struct mount *mp,
+struct vnode *vp)
+{
+   const struct vnode *tmp;
+   bool held, ret;
+
+   VNASSERT(mvp->v_mount == mp && mvp->v_type == VMARKER &&
+   TAILQ_NEXT(mvp, v_actfreelist) != NULL, mvp,
+   ("%s: bad marker", __func__));
+   VNASSERT(vp->v_mount == mp && vp->v_type != VMARKER, vp,
+   ("%s: inappropriate vnode", __func__));
+   ASSERT_VI_UNLOCKED(vp, __func__);
+   mtx_assert(>mnt_listmtx, MA_OWNED);
+
+   ret = false;
+
+   TAILQ_REMOVE(>mnt_activevnodelist, mvp, v_actfreelist);
+   TAILQ_INSERT_BEFORE(vp, mvp, v_actfreelist);
+
+   /*
+* Use a hold to prevent vp from disappearing while the mount vnode
+* list lock is dropped and reacquired.  Normally a hold would be
+* acquired with vhold(), but that might try to acquire the vnode
+* interlock, which would be a LOR with the mount vnode list lock.
+*/
+   held = vfs_refcount_acquire_if_not_zero(>v_holdcnt);
+   mtx_unlock(>mnt_listmtx);
+   if (!held)
+   goto abort;
+   VI_LOCK(vp);
+   if (!vfs_refcount_release_if_not_last(>v_holdcnt)) {
+   vdropl(vp);
+   goto abort;
+   }
+   mtx_lock(>mnt_listmtx);
+
+   /*
+* Determine whether the vnode is still the next one after the marker,
+* excepting any other markers.  If the vnode has not been doomed by
+* vgone() then the hold should have ensured that it remained on the
+* active list.  If it has been doomed but is still on the active list,
+* don't abort, but rather skip over it (avoid spinning on doomed
+* vnodes).
+*/
+   tmp = mvp;
+   do {
+   tmp = TAILQ_NEXT(tmp, v_actfreelist);
+   } while (tmp != NULL && tmp->v_type == VMARKER);
+   if (tmp != vp) {
+   mtx_unlock(>mnt_listmtx);
+   VI_UNLOCK(vp);
+   goto abort;
+   }
+
+   ret = true;
+   goto out;
+abort:
+   maybe_yield();
+   mtx_lock(>mnt_listmtx);
+out:
+   if (ret)
+   ASSERT_VI_LOCKED(vp, __func__);
+   else
+   ASSERT_VI_UNLOCKED(vp, __func__);
+   mtx_assert(>mnt_listmtx, MA_OWNED);
+   return (ret);
+}
+
 static struct vnode *
 mnt_vnode_next_active(struct vnode **mvp, struct mount *mp)
 {
@@ -5371,22 +5449,19 @@ mnt_vnode_next_active(struct vnode **mvp
KASSERT((*mvp)->v_mount == mp, ("marker vnode mount list mismatch"));
 restart:
vp = TAILQ_NEXT(*mvp, v_actfreelist);
-   TAILQ_REMOVE(>mnt_activevnodelist, *mvp, v_actfreelist);
while (vp != NULL) {
if (vp->v_type == VMARKER) {
vp = TAILQ_NEXT(vp, v_actfreelist);
continue;
}
-   if (!VI_TRYLOCK(vp)) {
-   if (mp_ncpus == 1 || should_yield()) {
-   TAILQ_INSERT_BEFORE(vp, *mvp, v_actfreelist);
-   mtx_unlock(>mnt_listmtx);
-   pause("vnacti", 1);
-