FBSD: Avoid recursive osi_VM_StoreAllSegments lock 00/14000/6
authorAndrew Deason <adeason@dson.org>
Wed, 1 Jan 2020 23:09:24 +0000 (17:09 -0600)
committerBenjamin Kaduk <kaduk@mit.edu>
Thu, 11 Mar 2021 22:37:53 +0000 (17:37 -0500)
Currently, osi_VM_StoreAllSegments calls vget() for the given vnode,
which requires locking the vnode. However, the vnode should already be
locked. For example, when called from the close syscall, we reach this
function via: vn_close1 -> afs_vop_close -> afs_close ->
afs_StoreOnLastReference -> afs_StoreAllSegments ->
osi_VM_StoreAllSegments. This causes a panic like so:

    kernel: panic: lockmgr_xlock_hard: recursing on non recursive lockmgr 0x[...] @ /usr/src/sys/kern/vfs_subr.c:2730

We can also reach this code path from the BOP_STORE background
operation (BStore -> afs_StoreOnLastReference -> afs_StoreAllSegments
-> osi_VM_StoreAllSegments), initiated from afs_close(), which has the
vnode locked. In this case, we won't be recursively locking the vnode,
since the process calling afs_close() is the one that holds the lock,
and the background thread is the process trying to lock the vnode
again. So we'll just deadlock.

From the comments in this function, it seems like locking the vnode at
all in here is unnecessary, since the vnode should be locked from the
higher-level functions anyway. So just skip the vget and all of the
related looping retry logic. As a result, this function can now become
somewhat simplified.

Change-Id: Ic5a18de46e51dc86190207163ad0fe73bc03cbd7
Reviewed-on: https://gerrit.openafs.org/14000
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>

src/afs/FBSD/osi_vm.c

index 780ae12..d02c533 100644 (file)
@@ -110,41 +110,30 @@ osi_VM_StoreAllSegments(struct vcache *avc)
 {
     struct vnode *vp;
     struct vm_object *obj;
-    int anyio, tries;
 
-    ReleaseWriteLock(&avc->lock);
-    AFS_GUNLOCK();
-    tries = 5;
     vp = AFSTOV(avc);
-
     /*
-     * I don't understand this.  Why not just call vm_object_page_clean()
-     * and be done with it?  I particularly don't understand why we're calling
-     * vget() here.  Is there some reason to believe that the vnode might
-     * be being recycled at this point?  I don't think there's any need for
-     * this loop, either -- if we keep the vnode locked all the time,
-     * that and the object lock will prevent any new pages from appearing.
-     * The loop is what causes the race condition.  -GAW
+     * VOP_ISLOCKED may return LK_EXCLOTHER here, since we may be running in a
+     * BOP_STORE background operation, and so we're running in a different
+     * thread than the actual syscall that has the vnode locked. So we cannot
+     * just call ASSERT_VOP_LOCKED (since that will fail if VOP_ISLOCKED
+     * returns LK_EXCLOTHER), and instead we just have our own assert here.
      */
-    do {
-       anyio = 0;
-       
-       obj = vp->v_object;
-       if (obj != NULL && obj->flags & OBJ_MIGHTBEDIRTY) {
-           if (!vget(vp, LK_EXCLUSIVE | LK_RETRY, curthread)) {
-                   obj = vp->v_object;
-                   if (obj != NULL) {
-                       AFS_VM_OBJECT_WLOCK(obj);
-                       vm_object_page_clean(obj, 0, 0, OBJPC_SYNC);
-                       AFS_VM_OBJECT_WUNLOCK(obj);
-                       anyio = 1;
-                   }
-                   vput(vp);
-               }
-           }
-    } while (anyio && (--tries > 0));
-    AFS_GLOCK();
-    ObtainWriteLock(&avc->lock, 94);
+    osi_Assert(VOP_ISLOCKED(vp) != 0);
+
+    obj = vp->v_object;
+
+    if (obj != NULL && (obj->flags & OBJ_MIGHTBEDIRTY) != 0) {
+       ReleaseWriteLock(&avc->lock);
+       AFS_GUNLOCK();
+
+       AFS_VM_OBJECT_WLOCK(obj);
+       vm_object_page_clean(obj, 0, 0, OBJPC_SYNC);
+       AFS_VM_OBJECT_WUNLOCK(obj);
+
+       AFS_GLOCK();
+       ObtainWriteLock(&avc->lock, 94);
+    }
 }
 
 /* Try to invalidate pages, for "fs flush" or "fs flushv"; or