FBSD: Ignore VI_DOOMED vnodes 72/13972/6
authorAndrew Deason <adeason@dson.org>
Mon, 25 Nov 2019 04:36:17 +0000 (22:36 -0600)
committerBenjamin Kaduk <kaduk@mit.edu>
Fri, 29 May 2020 04:21:09 +0000 (00:21 -0400)
commit20dc2832268eb81d40e798da0d424c98cf26062c
treef2313301bb964cf51566d626c45c7116cab3e329
parent145c90bdbeeff4ea95acacd7dc110f0c6fcba281
FBSD: Ignore VI_DOOMED vnodes

Currently on FreeBSD, osi_TryEvictVCache calls vgone() for our vnode
after checking if the given vcache is in use. vgone() then calls our
VOP_RECLAIM operation, which calls afs_vop_reclaim, which calls
afs_FlushVCache to finally actually flush the vcache.

The current approach has at least the following major issues:

- In afs_vop_reclaim, we return success even if afs_FlushVCache()
  fails. This allows FreeBSD to reuse the vnode for another file, but
  the vnode is still being referenced by our vcache, which is
  referenced by the global VLRU and various other structures. This
  causes all kinds of weird errors, since we try to use the underlying
  vnode for different files.

- After the relevant checks in osi_TryEvictVCache are done, another
  thread can acquire a new reference to our vcache (this can happen
  while vgone() is running up until the vnode is locked). This new
  reference will cause afs_FlushVCache to fail.

- Our afs_vop_reclaim callback is called while the vnode is locked,
  and can acquire afs_xvcache. Other code locks the vnode while
  afs_xvcache is already held (such as afs_PutVCache -> vrele). This
  can lead to deadlocks if two threads try to run these codepaths for
  the same vnode at the same time.

- afs_vop_reclaim optionally acquires afs_xvcache based on the return
  value of CheckLock(&afs_xvcache). However, CheckLock just returns if
  that lock is locked by anyone, not if the current thread holds the
  lock. This can result in the rest of the function running without
  afs_xvcache actually being held if we drop AFS_GLOCK at any point.

- osi_TryEvictVCache() tries to vn_lock() the target vnode, but we may
  already have another vnode locked in the current thread. If the
  vnode we're trying to evict is a descendant of a vnode we already
  have locked, this can deadlock.

To fix these issues, make some changes to how our vcache management
works on FreeBSD:

- Do not allow anyone to hold a new reference on a VI_DOOMED vnode.
  We do this by checking for VI_DOOMED in osi_vnhold, and returning an
  error if VI_DOOMED is set.

- In afs_vop_reclaim, panic if afs_FlushVCache fails. With the new
  VI_DOOMED check, afs_FlushVCache show now never fail; and if it
  somehow does, panic'ing immediately is better than corrupting
  various structures and panic'ing later on.

- Move around some of the relevant locking in afs_vop_reclaim to fix
  the lock-related issues.

- In osi_TryEvictVCache, don't wait for the vnode lock (LK_NOWAIT);
  treat the vnode as "in use" if we can't immediately obtain the lock.

Thanks to tcreech@tcreech.com and kaduk@mit.edu for insight and help
investigating the relevant issues.

FIXES 135041

Change-Id: I23e94ecebbddc8c68a8f4ea918d64efd0f9f9dfd
Reviewed-on: https://gerrit.openafs.org/13972
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
src/afs/FBSD/osi_vcache.c
src/afs/FBSD/osi_vm.c
src/afs/FBSD/osi_vnodeops.c