LINUX: Avoid d_invalidate() during afs_ShakeLooseVCaches()

With recent changes to d_invalidate's semantics (it returns void in Linux 3.11,
and always returns success in RHEL 7.4), it has become increasingly clear that
d_invalidate() is not the best function for use in our best-effort
(nondisruptive) attempt to free up vcaches that is afs_ShakeLooseVCaches().
The new d_invalidate() semantics always force the invalidation of a directory
dentry, which contradicts our desire to be nondisruptive, especially when
that directory is being used as the current working directory for a process.
Our call to d_invalidate(), intended to merely probe for whether a dentry
can be discarded without affecting other consumers, instead would cause
processes using that dentry as a CWD to receive ENOENT errors from getcwd().

A previous commit (c3bbf0b4444db88192eea4580ac9e9ca3de0d286) tried to address
this issue by calling d_prune_aliases() instead of d_invalidate(), but
d_prune_aliases() does not recursively descend into children of the given
dentry while pruning, leaving it an incomplete solution for our use-case.

To address these issues, modify the shakeloose routine TryEvictDentries() to
call shrink_dcache_parent() and maybe __d_drop() for directories, and
d_prune_aliases() for non-directories, instead of d_invalidate().  (Calls to
d_prune_aliases() for directories have already been removed by reverting commit
c3bbf0b4444db88192eea4580ac9e9ca3de0d286.)

Just like d_invalidate(), shrink_dcache_parent() has been around "forever"
(since pre-git v2.6.12).  Also like d_invalidate(), it "walks" the parent
dentry's subdirectories and "shrinks" (unhashes) unused dentries.  But unlike
d_invalidate(), shrink_dcache_parent() will not unhash an in-use dentry, and
has never changed its signature or semantics.

d_prune_aliases() has also been available "forever", and has also never changed
its signature or semantics.  The lack of recursive descent is not an issue for
non-directories, which cannot have such children.

[kaduk@mit.edu: apply review feedback to fix locking and avoid extraneous
changes, and reword commit message]

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

diff --git a/src/afs/LINUX/osi_vcache.c b/src/afs/LINUX/osi_vcache.c
index 1a5012c..19840d1 100644 (file)
--- a/src/afs/LINUX/osi_vcache.c
+++ b/src/afs/LINUX/osi_vcache.c
@@ -16,10 +16,9 @@
 #include "osi_compat.h"
 
 static void
-TryEvictDentries(struct vcache *avc)
+TryEvictDirDentries(struct inode *inode)
 {
     struct dentry *dentry;
-    struct inode *inode = AFSTOV(avc);
 #if defined(D_ALIAS_IS_HLIST) && !defined(HLIST_ITERATOR_NO_NODE)
     struct hlist_node *p;
 #endif
@@ -45,11 +44,44 @@ restart:
        afs_linux_dget(dentry);
 
        afs_d_alias_unlock(inode);
-       if (afs_d_invalidate(dentry) == -EBUSY) {
-           dput(dentry);
-           /* perhaps lock and try to continue? (use cur as head?) */
-           goto inuse;
+
+       /*
+        * Once we have dropped the d_alias lock (above), it is no longer safe
+        * to 'continue' our iteration over d_alias because the list may change
+        * out from under us.  Therefore, we must either leave the loop, or
+        * restart from the beginning.  To avoid looping forever, we must only
+        * restart if we know we've d_drop'd an alias.  In all other cases we
+        * must leave the loop.
+        */
+
+       /*
+        * For a long time we used d_invalidate() for this purpose, but
+        * using shrink_dcache_parent() and checking the refcount ourselves is
+        * better, for two reasons: it avoids causing ENOENT issues for the
+        * CWD in linux versions since 3.11, and it avoids dropping Linux
+        * submounts.
+        *
+        * For non-fakestat, AFS mountpoints look like directories and end up here.
+        */
+
+       shrink_dcache_parent(dentry);
+       spin_lock(&dentry->d_lock);
+       if (afs_dentry_count(dentry) > 1)       /* still has references */ {
+           if (dentry->d_inode != NULL) /* is not a negative dentry */ {
+               spin_unlock(&dentry->d_lock);
+               dput(dentry);
+               goto inuse;
+           }
        }
+       /*
+        * This is either a negative dentry, or a dentry with no references.
+        * Either way, it is okay to unhash it now.
+        * Do so under the d_lock (that is, via __d_drop() instead of d_drop())
+        * to avoid a race with another process picking up a reference.
+        */
+       __d_drop(dentry);
+       spin_unlock(&dentry->d_lock);
+
        dput(dentry);
        afs_d_alias_lock(inode);
        goto restart;
@@ -69,12 +101,17 @@ osi_TryEvictVCache(struct vcache *avc, int *slept, int defersleep)
     /* First, see if we can evict the inode from the dcache */
     if (defersleep && avc != afs_globalVp && VREFCOUNT(avc) > 1
        && avc->opens == 0) {
+       struct inode *ip = AFSTOV(avc);
+
        *slept = 1;
        AFS_FAST_HOLD(avc);
        ReleaseWriteLock(&afs_xvcache);
        AFS_GUNLOCK();
 
-       TryEvictDentries(avc);
+       if (S_ISDIR(ip->i_mode))
+           TryEvictDirDentries(ip);
+       else
+           d_prune_aliases(ip);
 
        AFS_GLOCK();
        ObtainWriteLock(&afs_xvcache, 733);