From 4feec06c7bdc7102ae654cff20eb02650ec32800 Mon Sep 17 00:00:00 2001 From: Mark Vitale Date: Thu, 30 Nov 2017 20:26:46 -0500 Subject: [PATCH] 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] Reviewed-on: https://gerrit.openafs.org/12830 Reviewed-by: Benjamin Kaduk Tested-by: BuildBot (cherry picked from commit afbc199f152cc06edc877333f229604c28638d07) Change-Id: I6d37e5584b57dcbb056385a79f67b92a363e08d2 Reviewed-on: https://gerrit.openafs.org/12851 Tested-by: BuildBot Tested-by: Benjamin Kaduk Reviewed-by: Benjamin Kaduk --- src/afs/LINUX/osi_vcache.c | 51 ++++++++++++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 7 deletions(-) diff --git a/src/afs/LINUX/osi_vcache.c b/src/afs/LINUX/osi_vcache.c index 1a5012c95..19840d1ad 100644 --- 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); -- 2.39.5