Here's a potential patch to remove all the locks of dentry->d_lock from
shrink_dcache_for_umount*(). Getting these locks ought to be unnecessary as
these functions are only called during superblock destruction and, as such,
should never see d_lock locked - I think.
However, this patch causes the assertion:
dentry_rcuwalk_barrier(dentry);
--> assert_spin_locked(&dentry->d_lock);
in __d_drop() to trigger.
In this particular instance (sb destruction) is this assertion actually
necessary, or can I just strip out all the locking from these functions and
rely on RCU freeing to prevent transit through the hash bucket chains from
going wrong? I think that it shouldn't be a problem, but I presume Nick
must've had his reasons for not getting rid of the locking himself.
David
---
From: David Howells <dhowells@xxxxxxxxxx>
Subject: [PATCH] VFS: Remove dentry->d_lock locking from shrink_dcache_for_umount*()
Locks of the dcache_lock were replaced by locks of dentry->d_lock in commits
such as:
2304450783dfde7b0b94ae234edd0dbffa865073
2fd6b7f50797f2e993eea59e0a0b8c6399c811dc
as part of the RCU-based pathwalk changes, despite the fact that the caller
(shrink_dcache_for_umount()) notes in the banner comment the reasons that
d_lock is not necessary in these functions:
/*
* destroy the dentries attached to a superblock on unmounting
* - we don't need to use dentry->d_lock because:
* - the superblock is detached from all mountings and open files, so the
* dentry trees will not be rearranged by the VFS
* - s_umount is write-locked, so the memory pressure shrinker will ignore
* any dentries belonging to this superblock that it comes across
* - the filesystem itself is no longer permitted to rearrange the dentries
* in this superblock
*/
So remove these locks. If the locks are actually necessary, then this banner
comment should be altered instead.
Note that the functions should be renamed - they operate during superblock
destruction rather than unmounting specifically.
The hash table chains are protected by 1-bit locks in the hash table heads, so
those shouldn't be a problem.
Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
Cc: Nick Piggin <npiggin@xxxxxxxxx>
---
fs/dcache.c | 11 -----------
1 files changed, 0 insertions(+), 11 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 37f72ee..def734f 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -878,10 +878,8 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
BUG_ON(!IS_ROOT(dentry));
/* detach this root from the system */
- spin_lock(&dentry->d_lock);
dentry_lru_del(dentry);
__d_drop(dentry);
- spin_unlock(&dentry->d_lock);
for (;;) {
/* descend to the first leaf in the current subtree */
@@ -890,16 +888,11 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
/* this is a branch with children - detach all of them
* from the system in one go */
- spin_lock(&dentry->d_lock);
list_for_each_entry(loop, &dentry->d_subdirs,
d_u.d_child) {
- spin_lock_nested(&loop->d_lock,
- DENTRY_D_LOCK_NESTED);
dentry_lru_del(loop);
__d_drop(loop);
- spin_unlock(&loop->d_lock);
}
- spin_unlock(&dentry->d_lock);
/* move to the first child */
dentry = list_entry(dentry->d_subdirs.next,
@@ -931,10 +924,8 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
list_del(&dentry->d_u.d_child);
} else {
parent = dentry->d_parent;
- spin_lock(&parent->d_lock);
parent->d_count--;
list_del(&dentry->d_u.d_child);
- spin_unlock(&parent->d_lock);
}
detached++;
@@ -983,9 +974,7 @@ void shrink_dcache_for_umount(struct super_block *sb)
dentry = sb->s_root;
sb->s_root = NULL;
- spin_lock(&dentry->d_lock);
dentry->d_count--;
- spin_unlock(&dentry->d_lock);
shrink_dcache_for_umount_subtree(dentry);
while (!hlist_bl_empty(&sb->s_anon)) {
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
