vfs: fix dentry shrink list corruption
Subsystems (2)
cover vfs: fix dentry shrink list corruption
On at least 3 different machines, we have seen dentry shrink list corruption with 3.14 stable kernel (see the end of this email), which I think is fixed by a set of patches from Al.
This bug is probably introduced by the following commit in 3.12:
commit dd1f6b2e43a53ee58eb87d5e623cf44e277d005d Author: Dave Chinner <[email protected]> Date: Wed Aug 28 10:17:55 2013 +1000 dcache: remove dentries from LRU before putting on dispose list
since previously we held the per sb lock. It is hard to reproduce this bug, because it is a race condition which happens when we update two contiguous dentries on a shrink list. Matt ran some ltp stress tests and doesn't see any problem with the patched kernel. And, 3.13 needs them too, but we have no time or resource to backport or test them on 3.13.
The first 7 patches are the original patchset from Al, the rest of patches are either needed for context or are folllow-up fixes, especially the last 2 patches. Of course all of them are merged in upstream, see each patch for details. Note, I don't get any conflict when backporting them to 3.14.
------------->
[45204.957357] ------------[ cut here ]------------ [45204.957370] WARNING: CPU: 6 PID: 26375 at lib/list_debug.c:33 __list_add+0x63/0xa7() [45204.957373] list_add corruption. prev->next should be next (ffff880c0205d280), but was ffff880cfa3d9d00. (prev=ffff8807c5ad7c08). [45204.957375] Modules linked in: cls_basic act_mirred cls_u32 veth
sch_ingress netconsole configfs ipv6 dm_multipath scsi_dh video sbs sbshc acpi_p
ad acpi_ipmi parport_pc lp parport tcp_diag inet_diag ipmi_devintf sg iTCO_wdt iTCO_vendor_support dcdbas igb i2c_algo_bit ptp pps_core wmi ipmi_si sb_edac hed edac_core ipmi_msghandler i2c_i801
ioatdma i2c_core microcode lpc_ich mfd_core dca shpchp ahci libahci
libata sd_mod scsi_mod
[45204.957421] CPU: 6 PID: 26375 Comm: python2.6 Not tainted 3.14.14 #1 [45204.957423] Hardware name: Dell Inc. PowerEdge C6220/03C9JJ, BIOS 1.2.1 05/27/2013 [45204.957426] 0000000000000000 ffff8807c5ad7ad0 ffffffff814a9b67 ffff8807c5ad7b18 [45204.957453] ffff8807c5ad7b08 ffffffff8105b901 ffffffff812787ff ffff8807c5ad7c08 [45204.957459] ffff880c0205d280 ffff880be8bbc8c0 ffff880850fd3680 ffff8807c5ad7b68 [45204.957465] Call Trace: [45204.957475] [<ffffffff814a9b67>] dump_stack+0x45/0x56 [45204.957482] [<ffffffff8105b901>] warn_slowpath_common+0x7f/0x98 [45204.957486] [<ffffffff812787ff>] ? __list_add+0x63/0xa7 [45204.957490] [<ffffffff8105b966>] warn_slowpath_fmt+0x4c/0x4e [45204.957494] [<ffffffff812787ff>] __list_add+0x63/0xa7 [45204.957500] [<ffffffff811630fa>] list_add+0xc/0xe [45204.957504] [<ffffffff81163146>] d_shrink_add+0x4a/0x5e [45204.957509] [<ffffffff81163b0b>] shrink_dentry_list+0xaa/0xbb [45204.957513] [<ffffffff81164485>] shrink_dcache_parent+0x50/0x60 [45204.957520] [<ffffffff811a5070>] proc_flush_task+0x14c/0x18b [45204.957526] [<ffffffff8105c49f>] release_task+0x30/0x36d [45204.957530] [<ffffffff8105dd43>] do_exit+0x809/0x8ee [45204.957537] [<ffffffff810afa48>] ? __unqueue_futex+0x53/0x56 [45204.957541] [<ffffffff8105dea2>] do_group_exit+0x44/0x9a [45204.957548] [<ffffffff81069cc7>] get_signal_to_deliver+0x44b/0x4bc [45204.957555] [<ffffffff8100224f>] do_signal+0x49/0x5d5 [45204.957561] [<ffffffff811442de>] ? kmem_cache_free+0x100/0x138 [45204.957565] [<ffffffff8105915f>] ? __mmdrop+0x82/0x89 [45204.957570] [<ffffffff81002804>] do_notify_resume+0x29/0x65 [45204.957576] [<ffffffff814b5662>] int_signal+0x12/0x17 [45204.957579] ---[ end trace 0decfbe793859fa2 ]--- [45204.959359] ------------[ cut here ]------------
PATCH 1/16 fold d_kill() and d_free()
From: Al Viro <[email protected]>
--- fs/dcache.c | 76 +++++++++++++++++++------------------------------------------ 1 file changed, 24 insertions(+), 52 deletions(-)
fs/dcache.c +24 -52
@@ -245,23 +245,6 @@ static void __d_free(struct rcu_head *head)kmem_cache_free (dentry_cache, dentry); } -/* -* no locks, please. -*/ -static void d_free (struct dentry *dentry) -{ -BUG_ON ((int )dentry->d_lockref.count >0 ); -this_cpu_dec (nr_dentry); -if (dentry->d_op && dentry->d_op->d_release) - dentry->d_op->d_release (dentry); - -/* if dentry was never visible to RCU, immediate free is OK */ -if (!(dentry->d_flags &DCACHE_RCUACCESS )) -__d_free (&dentry->d_u.d_rcu); -else -call_rcu (&dentry->d_u.d_rcu, __d_free); -} -/** * dentry_rcuwalk_barrier - invalidate in-progress rcu-walk lookups * @dentry: the target dentry
@@ -419,40 +402,6 @@ static void dentry_lru_del(struct dentry *dentry) }/** -* d_kill - kill dentry and return parent -* @dentry: dentry to kill -* @parent: parent dentry -* -* The dentry must already be unhashed and removed from the LRU. -* -* If this is the root of the dentry tree, return NULL. -* -* dentry->d_lock and parent->d_lock must be held by caller, and are dropped by -* d_kill. -*/ -static struct dentry *d_kill (struct dentry *dentry,struct dentry *parent) - __releases(dentry->d_lock) - __releases(parent->d_lock) - __releases(dentry->d_inode->i_lock) -{ -list_del (&dentry->d_u.d_child); -/* -* Inform d_walk() that we are no longer attached to the -* dentry tree -*/ - dentry->d_flags |=DCACHE_DENTRY_KILLED ; -if (parent) -spin_unlock (&parent->d_lock); -dentry_iput (dentry); -/* -* dentry_iput drops the locks, at which point nobody (except -* transient RCU lookups) can reach this dentry. -*/ -d_free (dentry); -return parent; -} - -/** * d_drop - drop a dentry * @dentry: dentry to drop *
@@ -545,7 +494,30 @@ dentry_kill(struct dentry *dentry, int unlock_on_failure)dentry_lru_del (dentry);/* if it was on the hash then remove it */ __d_drop (dentry); -return d_kill (dentry, parent); +list_del (&dentry->d_u.d_child); +/* +* Inform d_walk() that we are no longer attached to the +* dentry tree +*/ + dentry->d_flags |=DCACHE_DENTRY_KILLED ; +if (parent) +spin_unlock (&parent->d_lock); +dentry_iput (dentry); +/* +* dentry_iput drops the locks, at which point nobody (except +* transient RCU lookups) can reach this dentry. +*/ +BUG_ON ((int )dentry->d_lockref.count >0 ); +this_cpu_dec (nr_dentry); +if (dentry->d_op && dentry->d_op->d_release) + dentry->d_op->d_release (dentry); + +/* if dentry was never visible to RCU, immediate free is OK */ +if (!(dentry->d_flags &DCACHE_RCUACCESS )) +__d_free (&dentry->d_u.d_rcu); +else +call_rcu (&dentry->d_u.d_rcu, __d_free); +return parent; }/*
Signature · 1 line
1.8.3.1
Greg KH
maintainerRe: [Patch 3.14 stable 01/16] fold d_kill() and d_free()
Quoted 7 lines · Cong Wang
From: Al Viro <[email protected]>
Signed-off-by Al Viro <[email protected]>(cherry picked from commit 03b3b889e79cdb6b806fc0ba9be0d71c186bbfaa)--- fs/dcache.c | 76 +++++++++++++++++++------------------------------------------ 1 file changed, 24 insertions(+), 52 deletions(-)
This patch doesn't apply to the 3.14-stable tree anymore.
I know it's been a while, but could you respin this series and resend them if you think it's still needed for 3.14-stable?
thanks,
greg k-h
PATCH 1/1 fold d_kill() and d_free()
From: Vinson Lee <[email protected]>
This is a backport of 03b3b889e79cdb6b806fc0ba9be0d71c186bbfaa "fold d_kill() and d_free()" to stable kernel 3.14.
The other 15 patches (patches 2-16) from the "vfs: fix dentry shrink list corruption" stable request still apply on top of 3.14.43.
Al Viro (1):
fold d_kill() and d_free()
fs/dcache.c | 77 +++++++++++++++++++------------------------------------------ 1 file changed, 24 insertions(+), 53 deletions(-)
Signature · 1 line
2.1.0
PATCH 1/1 fold d_kill() and d_free()
From: Al Viro <[email protected]>
commit 03b3b889e79cdb6b806fc0ba9be0d71c186bbfaa upstream.
--- fs/dcache.c | 77 +++++++++++++++++++------------------------------------------ 1 file changed, 24 insertions(+), 53 deletions(-)
fs/dcache.c +24 -53
@@ -244,24 +244,6 @@ static void __d_free(struct rcu_head *head)kmem_cache_free (dentry_cache, dentry); } -/* -* no locks, please. -*/ -static void d_free (struct dentry *dentry) -{ -WARN_ON (!hlist_unhashed (&dentry->d_u.d_alias)); -BUG_ON ((int )dentry->d_lockref.count >0 ); -this_cpu_dec (nr_dentry); -if (dentry->d_op && dentry->d_op->d_release) - dentry->d_op->d_release (dentry); - -/* if dentry was never visible to RCU, immediate free is OK */ -if (!(dentry->d_flags &DCACHE_RCUACCESS )) -__d_free (&dentry->d_u.d_rcu); -else -call_rcu (&dentry->d_u.d_rcu, __d_free); -} -/** * dentry_rcuwalk_barrier - invalidate in-progress rcu-walk lookups * @dentry: the target dentry
@@ -419,40 +401,6 @@ static void dentry_lru_del(struct dentry *dentry) }/** -* d_kill - kill dentry and return parent -* @dentry: dentry to kill -* @parent: parent dentry -* -* The dentry must already be unhashed and removed from the LRU. -* -* If this is the root of the dentry tree, return NULL. -* -* dentry->d_lock and parent->d_lock must be held by caller, and are dropped by -* d_kill. -*/ -static struct dentry *d_kill (struct dentry *dentry,struct dentry *parent) - __releases(dentry->d_lock) - __releases(parent->d_lock) - __releases(dentry->d_inode->i_lock) -{ -__list_del_entry (&dentry->d_child); -/* -* Inform d_walk() that we are no longer attached to the -* dentry tree -*/ - dentry->d_flags |=DCACHE_DENTRY_KILLED ; -if (parent) -spin_unlock (&parent->d_lock); -dentry_iput (dentry); -/* -* dentry_iput drops the locks, at which point nobody (except -* transient RCU lookups) can reach this dentry. -*/ -d_free (dentry); -return parent; -} - -/** * d_drop - drop a dentry * @dentry: dentry to drop *
@@ -545,7 +493,30 @@ relock:dentry_lru_del (dentry);/* if it was on the hash then remove it */ __d_drop (dentry); -return d_kill (dentry, parent); +list_del (&dentry->d_child); +/* +* Inform d_walk() that we are no longer attached to the +* dentry tree +*/ + dentry->d_flags |=DCACHE_DENTRY_KILLED ; +if (parent) +spin_unlock (&parent->d_lock); +dentry_iput (dentry); +/* +* dentry_iput drops the locks, at which point nobody (except +* transient RCU lookups) can reach this dentry. +*/ +BUG_ON ((int )dentry->d_lockref.count >0 ); +this_cpu_dec (nr_dentry); +if (dentry->d_op && dentry->d_op->d_release) + dentry->d_op->d_release (dentry); + +/* if dentry was never visible to RCU, immediate free is OK */ +if (!(dentry->d_flags &DCACHE_RCUACCESS )) +__d_free (&dentry->d_u.d_rcu); +else +call_rcu (&dentry->d_u.d_rcu, __d_free); +return parent; }/*
Signature · 1 line
2.1.0
Greg KH
maintainerRe: [PATCH 3.14] fold d_kill() and d_free()
In reply to Vinson LeeQuoted 7 lines · Vinson Lee
From: Vinson Lee <[email protected]>
This is a backport of
03b3b889e79cdb6b806fc0ba9be0d71c186bbfaa"foldd_kill()andd_free()" to stable kernel 3.14.The other 15 patches (patches 2-16) from the "vfs: fix dentry shrink list corruption" stable request still apply on top of 3.14.43.
What stable request?
I don't see thos in my queue, what is this all supposed to be fixing?
Context please, I have none.
greg k-h
PATCH 2/16 fold try_prune_one_dentry()
In reply to Cong WangFrom: Al Viro <[email protected]>
--- fs/dcache.c | 75 +++++++++++++++++++++---------------------------------------- 1 file changed, 25 insertions(+), 50 deletions(-)
fs/dcache.c +25 -50
@@ -786,47 +786,9 @@ void d_prune_aliases(struct inode *inode) }EXPORT_SYMBOL (d_prune_aliases); -/* -* Try to throw away a dentry - free the inode, dput the parent. -* Requires dentry->d_lock is held, and dentry->d_count == 0. -* Releases dentry->d_lock. -* -* This may fail if locks cannot be acquired no problem, just try again. -*/ -static struct dentry *try_prune_one_dentry (struct dentry *dentry) - __releases(dentry->d_lock) -{ -struct dentry *parent; - - parent =dentry_kill (dentry,0 ); -/* -* If dentry_kill returns NULL, we have nothing more to do. -* if it returns the same dentry, trylocks failed. In either -* case, just loop again. -* -* Otherwise, we need to prune ancestors too. This is necessary -* to prevent quadratic behavior of shrink_dcache_parent(), but -* is also expected to be beneficial in reducing dentry cache -* fragmentation. -*/ -if (!parent) -return NULL ; -if (parent == dentry) -return dentry; - -/* Prune ancestors. */ - dentry = parent; -while (dentry) { -if (lockref_put_or_lock (&dentry->d_lockref)) -return NULL ; - dentry =dentry_kill (dentry,1 ); - } -return NULL ; -} -static void shrink_dentry_list (struct list_head *list) { -struct dentry *dentry; +struct dentry *dentry, *parent;rcu_read_lock ();for (;;) {
@@ -862,22 +824,35 @@ static void shrink_dentry_list(struct list_head *list) }rcu_read_unlock (); + parent =dentry_kill (dentry,0 );/* -* If 'try_to_prune()' returns a dentry, it will -* be the same one we passed in, and d_lock will -* have been held the whole time, so it will not -* have been added to any other lists. We failed -* to get the inode lock. -* -* We just add it back to the shrink list. +* If dentry_kill returns NULL, we have nothing more to do. */ - dentry =try_prune_one_dentry (dentry); - -rcu_read_lock (); -if (dentry) { +if (!parent) { +rcu_read_lock (); +continue ; + } +if (unlikely (parent == dentry)) { +/* +* trylocks have failed and d_lock has been held the +* whole time, so it could not have been added to any +* other lists. Just add it back to the shrink list. +*/ +rcu_read_lock ();d_shrink_add (dentry, list);spin_unlock (&dentry->d_lock); +continue ; } +/* +* We need to prune ancestors too. This is necessary to prevent +* quadratic behavior of shrink_dcache_parent(), but is also +* expected to be beneficial in reducing dentry cache +* fragmentation. +*/ + dentry = parent; +while (dentry && !lockref_put_or_lock (&dentry->d_lockref)) + dentry =dentry_kill (dentry,1 ); +rcu_read_lock (); }rcu_read_unlock (); }
Signature · 1 line
1.8.3.1
PATCH 3/16 new helper: dentry_free()
In reply to Cong WangFrom: Al Viro <[email protected]>
The part of old d_free() that dealt with actual freeing of dentry. Taken out of dentry_kill() into a separate function.
--- fs/dcache.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
fs/dcache.c +10 -5
@@ -245,6 +245,15 @@ static void __d_free(struct rcu_head *head)kmem_cache_free (dentry_cache, dentry); } +static void dentry_free (struct dentry *dentry) +{ +/* if dentry was never visible to RCU, immediate free is OK */ +if (!(dentry->d_flags &DCACHE_RCUACCESS )) +__d_free (&dentry->d_u.d_rcu); +else +call_rcu (&dentry->d_u.d_rcu, __d_free); +} +/** * dentry_rcuwalk_barrier - invalidate in-progress rcu-walk lookups * @dentry: the target dentry
@@ -512,11 +521,7 @@ dentry_kill(struct dentry *dentry, int unlock_on_failure)if (dentry->d_op && dentry->d_op->d_release) dentry->d_op->d_release (dentry); -/* if dentry was never visible to RCU, immediate free is OK */ -if (!(dentry->d_flags &DCACHE_RCUACCESS )) -__d_free (&dentry->d_u.d_rcu); -else -call_rcu (&dentry->d_u.d_rcu, __d_free); +dentry_free (dentry);return parent; }
Signature · 1 line
1.8.3.1
PATCH 4/16 expand the call of dentry_lru_del() in dentry_kill()
In reply to Cong WangFrom: Al Viro <[email protected]>
--- fs/dcache.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
fs/dcache.c +6 -1
@@ -500,7 +500,12 @@ dentry_kill(struct dentry *dentry, int unlock_on_failure)if ((dentry->d_flags &DCACHE_OP_PRUNE ) && !d_unhashed (dentry)) dentry->d_op->d_prune (dentry); -dentry_lru_del (dentry); +if (dentry->d_flags &DCACHE_LRU_LIST ) { +if (!(dentry->d_flags &DCACHE_SHRINK_LIST )) +d_lru_del (dentry); +else +d_shrink_del (dentry); + }/* if it was on the hash then remove it */ __d_drop (dentry);list_del (&dentry->d_u.d_child);
Signature · 1 line
1.8.3.1
PATCH 5/16 dentry_kill(): don't try to remove from shrink list
In reply to Cong WangFrom: Al Viro <[email protected]>
If the victim in on the shrink list, don't remove it from there. If shrink_dentry_list() manages to remove it from the list before we are done - fine, we'll just free it as usual. If not - mark it with new flag (DCACHE_MAY_FREE) and leave it there.
Eventually, shrink_dentry_list() will get to it, remove the sucker from shrink list and call dentry_kill(dentry, 0). Which is where we'll deal with freeing.
Since now dentry_kill(dentry, 0) may happen after or during dentry_kill(dentry, 1), we need to recognize that (by seeing DCACHE_DENTRY_KILLED already set), unlock everything and either free the sucker (in case DCACHE_MAY_FREE has been set) or leave it for ongoing dentry_kill(dentry, 1) to deal with.
--- fs/dcache.c | 27 +++++++++++++++++++-------- include/linux/dcache.h | 2 ++ 2 files changed, 21 insertions(+), 8 deletions(-)
fs/dcache.c +19 -8
@@ -467,7 +467,14 @@ dentry_kill(struct dentry *dentry, int unlock_on_failure) __releases(dentry->d_lock) {struct inode *inode; -struct dentry *parent; +struct dentry *parent =NULL ; +bool can_free = true; + +if (unlikely (dentry->d_flags &DCACHE_DENTRY_KILLED )) { + can_free = dentry->d_flags &DCACHE_MAY_FREE ; +spin_unlock (&dentry->d_lock); + goto out; + } inode = dentry->d_inode;if (inode && !spin_trylock (&inode->i_lock)) {
@@ -478,9 +485,7 @@ dentry_kill(struct dentry *dentry, int unlock_on_failure) }return dentry;/* try again with same dentry */ } -if (IS_ROOT (dentry)) - parent =NULL ; -else +if (!IS_ROOT (dentry)) parent = dentry->d_parent;if (parent && !spin_trylock (&parent->d_lock)) {if (inode)
@@ -503,8 +508,6 @@ dentry_kill(struct dentry *dentry, int unlock_on_failure)if (dentry->d_flags &DCACHE_LRU_LIST ) {if (!(dentry->d_flags &DCACHE_SHRINK_LIST ))d_lru_del (dentry); -else -d_shrink_del (dentry); }/* if it was on the hash then remove it */ __d_drop (dentry);
@@ -526,7 +529,15 @@ dentry_kill(struct dentry *dentry, int unlock_on_failure)if (dentry->d_op && dentry->d_op->d_release) dentry->d_op->d_release (dentry); -dentry_free (dentry); +spin_lock (&dentry->d_lock); +if (dentry->d_flags &DCACHE_SHRINK_LIST ) { + dentry->d_flags |=DCACHE_MAY_FREE ; + can_free = false; + } +spin_unlock (&dentry->d_lock); +out: +if (likely (can_free)) +dentry_free (dentry);return parent; }
@@ -828,7 +839,7 @@ static void shrink_dentry_list(struct list_head *list)* We found an inuse dentry which was not removed from * the LRU because of laziness during lookup. Do not free it. */ -if (dentry->d_lockref.count) { +if ((int )dentry->d_lockref.count >0 ) {spin_unlock (&dentry->d_lock);continue ; }
include/linux/dcache.h +2 -0
@@ -221,6 +221,8 @@ struct dentry_operations {#define DCACHE_SYMLINK_TYPE 0x00300000/* Symlink */ #define DCACHE_FILE_TYPE 0x00400000/* Other file type */ +#define DCACHE_MAY_FREE 0x00800000 +extern seqlock_t rename_lock;static inline int dname_external (const struct dentry *dentry)
Signature · 1 line
1.8.3.1
PATCH 6/16 don't remove from shrink list in select_collect()
In reply to Cong WangFrom: Al Viro <[email protected]>
If we find something already on a shrink list, just incrementdata->found and do nothing else. Loops in shrink_dcache_parent() and check_submounts_and_drop() will do the right thing - everything we did put into our list will be evicted and if there had been nothing, but data->found got non-zero, well, we have somebody else shrinking those guys; just try again.
--- fs/dcache.c | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-)
fs/dcache.c +10 -21
@@ -1228,34 +1228,23 @@ static enum d_walk_ret select_collect(void *_data, struct dentry *dentry)if (data->start == dentry) goto out; -/* -* move only zero ref count dentries to the dispose list. -* -* Those which are presently on the shrink list, being processed -* by shrink_dentry_list(), shouldn't be moved. Otherwise the -* loop in shrink_dcache_parent() might not make any progress -* and loop forever. -*/ -if (dentry->d_lockref.count) { -dentry_lru_del (dentry); - }else if (!(dentry->d_flags &DCACHE_SHRINK_LIST )) { -/* -* We can't use d_lru_shrink_move() because we -* need to get the global LRU lock and do the -* LRU accounting. -*/ -d_lru_del (dentry); -d_shrink_add (dentry, &data->dispose); +if (dentry->d_flags &DCACHE_SHRINK_LIST ) { data->found++; - ret =D_WALK_NORETRY ; + }else { +if (dentry->d_flags &DCACHE_LRU_LIST ) +d_lru_del (dentry); +if (!dentry->d_lockref.count) { +d_shrink_add (dentry, &data->dispose); + data->found++; + } }/* * We can return to the caller if we have found some (this * ensures forward progress). We'll be coming back to find * the rest. */ -if (data->found &&need_resched ()) - ret =D_WALK_QUIT ; +if (!list_empty (&data->dispose)) + ret =need_resched () ?D_WALK_QUIT :D_WALK_NORETRY ; out:return ret; }
Signature · 1 line
1.8.3.1
PATCH 7/16 more graceful recovery in umount_collect()
In reply to Cong WangFrom: Al Viro <[email protected]>
Start with shrink_dcache_parent(), then scan what remains.
First of all, BUG() is very much an overkill here; we are holding ->s_umount, and hitting BUG() means that a lot of interesting stuff will be hanging after that point (sync(2), for example). Moreover, in cases when there had been more than one leak, we'll be better off reporting all of them. And more than just the last component of pathname - %pd is there for just such uses...
That was the last user of dentry_lru_del(), so kill it off...
--- fs/dcache.c | 101 +++++++++++++++--------------------------------------------- 1 file changed, 25 insertions(+), 76 deletions(-)
fs/dcache.c +25 -76
@@ -394,22 +394,6 @@ static void dentry_lru_add(struct dentry *dentry)d_lru_add (dentry); } -/* -* Remove a dentry with references from the LRU. -* -* If we are on the shrink list, then we can get to try_prune_one_dentry() and -* lose our last reference through the parent walk. In this case, we need to -* remove ourselves from the shrink list, not the LRU. -*/ -static void dentry_lru_del (struct dentry *dentry) -{ -if (dentry->d_flags &DCACHE_LRU_LIST ) { -if (dentry->d_flags &DCACHE_SHRINK_LIST ) -return d_shrink_del (dentry); -d_lru_del (dentry); - } -} -/** * d_drop - drop a dentry * @dentry: dentry to drop
@@ -1274,45 +1258,35 @@ void shrink_dcache_parent(struct dentry *parent) }EXPORT_SYMBOL (shrink_dcache_parent); -static enum d_walk_ret umount_collect (void *_data,struct dentry *dentry) +static enum d_walk_ret umount_check (void *_data,struct dentry *dentry) { -struct select_data *data = _data; -enum d_walk_ret ret =D_WALK_CONTINUE ; +/* it has busy descendents; complain about those instead */ +if (!list_empty (&dentry->d_subdirs)) +return D_WALK_CONTINUE ; -if (dentry->d_lockref.count) { -dentry_lru_del (dentry); -if (likely (!list_empty (&dentry->d_subdirs))) - goto out; -if (dentry == data->start && dentry->d_lockref.count ==1 ) - goto out; -printk (KERN_ERR -"BUG: Dentry %p{i=%lx,n=%s}" -" still in use (%d)" -" [unmount of %s %s]\n" , +/* root with refcount 1 is fine */ +if (dentry == _data && dentry->d_lockref.count ==1 ) +return D_WALK_CONTINUE ; + +printk (KERN_ERR "BUG: Dentry %p{i=%lx,n=%pd} " +" still in use (%d) [unmount of %s %s]\n" , dentry, dentry->d_inode ? dentry->d_inode->i_ino :0UL , - dentry->d_name.name, + dentry, dentry->d_lockref.count, dentry->d_sb->s_type->name, dentry->d_sb->s_id); -BUG (); - }else if (!(dentry->d_flags &DCACHE_SHRINK_LIST )) { -/* -* We can't use d_lru_shrink_move() because we -* need to get the global LRU lock and do the -* LRU accounting. -*/ -if (dentry->d_flags &DCACHE_LRU_LIST ) -d_lru_del (dentry); -d_shrink_add (dentry, &data->dispose); - data->found++; - ret =D_WALK_NORETRY ; - } -out: -if (data->found &&need_resched ()) - ret =D_WALK_QUIT ; -return ret; +WARN_ON (1 ); +return D_WALK_CONTINUE ; +} + +static void do_one_tree (struct dentry *dentry) +{ +shrink_dcache_parent (dentry); +d_walk (dentry, dentry, umount_check,NULL ); +d_drop (dentry); +dput (dentry); }/*
@@ -1322,40 +1296,15 @@ void shrink_dcache_for_umount(struct super_block *sb) {struct dentry *dentry; -if (down_read_trylock (&sb->s_umount)) -BUG (); +WARN (down_read_trylock (&sb->s_umount),"s_umount should've been locked" ); dentry = sb->s_root; sb->s_root =NULL ; -for (;;) { -struct select_data data; - -INIT_LIST_HEAD (&data.dispose); - data.start = dentry; - data.found =0 ; - -d_walk (dentry, &data, umount_collect,NULL ); -if (!data.found) -break ; - -shrink_dentry_list (&data.dispose); -cond_resched (); - } -d_drop (dentry); -dput (dentry); +do_one_tree (dentry);while (!hlist_bl_empty (&sb->s_anon)) { -struct select_data data; - dentry = hlist_bl_entry(hlist_bl_first(&sb->s_anon), struct dentry, d_hash); - -INIT_LIST_HEAD (&data.dispose); - data.start =NULL ; - data.found =0 ; - -d_walk (dentry, &data, umount_collect,NULL ); -if (data.found) -shrink_dentry_list (&data.dispose); -cond_resched (); + dentry = dget(hlist_bl_entry(hlist_bl_first(&sb->s_anon), struct dentry, d_hash)); +do_one_tree (dentry); } }
Signature · 1 line
1.8.3.1
PATCH 8/16 dcache: don't need rcu in shrink_dentry_list()
In reply to Cong WangFrom: Miklos Szeredi <[email protected]>
Since now the shrink list is private and nobody can free the dentry while it is on the shrink list, we can remove RCU protection from this.
--- fs/dcache.c | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-)
fs/dcache.c +4 -23
@@ -795,23 +795,9 @@ static void shrink_dentry_list(struct list_head *list) {struct dentry *dentry, *parent; -rcu_read_lock (); -for (;;) { - dentry = list_entry_rcu(list->prev, struct dentry, d_lru); -if (&dentry->d_lru == list) -break ;/* empty */ - -/* -* Get the dentry lock, and re-verify that the dentry is -* this on the shrinking list. If it is, we know that -* DCACHE_SHRINK_LIST and DCACHE_LRU_LIST are set. -*/ +while (!list_empty (list)) { + dentry = list_entry(list->prev, struct dentry, d_lru);spin_lock (&dentry->d_lock); - if (dentry != list_entry(list->prev, struct dentry, d_lru)) { -spin_unlock (&dentry->d_lock); -continue ; - } -/* * The dispose list is isolated and dentries are not accounted * to the LRU here, so we can simply remove it from the list
@@ -827,23 +813,20 @@ static void shrink_dentry_list(struct list_head *list)spin_unlock (&dentry->d_lock);continue ; } -rcu_read_unlock (); parent =dentry_kill (dentry,0 );/* * If dentry_kill returns NULL, we have nothing more to do. */ -if (!parent) { -rcu_read_lock (); +if (!parent)continue ; - } +if (unlikely (parent == dentry)) {/* * trylocks have failed and d_lock has been held the * whole time, so it could not have been added to any * other lists. Just add it back to the shrink list. */ -rcu_read_lock ();d_shrink_add (dentry, list);spin_unlock (&dentry->d_lock);continue ;
@@ -857,9 +840,7 @@ static void shrink_dentry_list(struct list_head *list) dentry = parent;while (dentry && !lockref_put_or_lock (&dentry->d_lockref)) dentry =dentry_kill (dentry,1 ); -rcu_read_lock (); } -rcu_read_unlock (); } static enum lru_status
Signature · 1 line
1.8.3.1
PATCH 9/16 lift the "already marked killed" case into shrink_dentry_list()
In reply to Cong WangFrom: Al Viro <[email protected]>
It can happen only when dentry_kill() is called with unlock_on_failure equal to 0 - other callers had dentry pinned until the moment they've got ->d_lock and DCACHE_DENTRY_KILLED is set only after lockref_mark_dead().
IOW, only one of three call sites of dentry_kill() might end up reaching that code. Just move it there.
--- fs/dcache.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
fs/dcache.c +9 -6
@@ -454,12 +454,6 @@ dentry_kill(struct dentry *dentry, int unlock_on_failure)struct dentry *parent =NULL ;bool can_free = true; -if (unlikely (dentry->d_flags &DCACHE_DENTRY_KILLED )) { - can_free = dentry->d_flags &DCACHE_MAY_FREE ; -spin_unlock (&dentry->d_lock); - goto out; - } - inode = dentry->d_inode;if (inode && !spin_trylock (&inode->i_lock)) { relock:
@@ -814,6 +808,15 @@ static void shrink_dentry_list(struct list_head *list)continue ; } + +if (unlikely (dentry->d_flags &DCACHE_DENTRY_KILLED )) { +bool can_free = dentry->d_flags &DCACHE_MAY_FREE ; +spin_unlock (&dentry->d_lock); +if (can_free) +dentry_free (dentry); +continue ; + } + parent =dentry_kill (dentry,0 );/* * If dentry_kill returns NULL, we have nothing more to do.
Signature · 1 line
1.8.3.1
PATCH 10/16 split dentry_kill()
In reply to Cong WangFrom: Al Viro <[email protected]>
... into trylocks and everything else. The latter (actual killing) is __dentry_kill().
--- fs/dcache.c | 62 +++++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 36 insertions(+), 26 deletions(-)
fs/dcache.c +36 -26
@@ -440,36 +440,12 @@ void d_drop(struct dentry *dentry) }EXPORT_SYMBOL (d_drop); -/* -* Finish off a dentry we've decided to kill. -* dentry->d_lock must be held, returns with it unlocked. -* If ref is non-zero, then decrement the refcount too. -* Returns dentry requiring refcount drop, or NULL if we're done. -*/ -static struct dentry * -dentry_kill (struct dentry *dentry,int unlock_on_failure) - __releases(dentry->d_lock) +static void __dentry_kill (struct dentry *dentry) { -struct inode *inode;struct dentry *parent =NULL ;bool can_free = true; - - inode = dentry->d_inode; -if (inode && !spin_trylock (&inode->i_lock)) { -relock: -if (unlock_on_failure) { -spin_unlock (&dentry->d_lock); -cpu_relax (); - } -return dentry;/* try again with same dentry */ - }if (!IS_ROOT (dentry)) parent = dentry->d_parent; -if (parent && !spin_trylock (&parent->d_lock)) { -if (inode) -spin_unlock (&inode->i_lock); - goto relock; - }/* * The dentry is now unrecoverably dead to the world.
@@ -513,10 +489,44 @@ dentry_kill(struct dentry *dentry, int unlock_on_failure) can_free = false; }spin_unlock (&dentry->d_lock); -out:if (likely (can_free))dentry_free (dentry); +} + +/* +* Finish off a dentry we've decided to kill. +* dentry->d_lock must be held, returns with it unlocked. +* If ref is non-zero, then decrement the refcount too. +* Returns dentry requiring refcount drop, or NULL if we're done. +*/ +static struct dentry * +dentry_kill (struct dentry *dentry,int unlock_on_failure) + __releases(dentry->d_lock) +{ +struct inode *inode = dentry->d_inode; +struct dentry *parent =NULL ; + +if (inode &&unlikely (!spin_trylock (&inode->i_lock))) + goto failed; + +if (!IS_ROOT (dentry)) { + parent = dentry->d_parent; +if (unlikely (!spin_trylock (&parent->d_lock))) { +if (inode) +spin_unlock (&inode->i_lock); + goto failed; + } + } + +__dentry_kill (dentry);return parent; + +failed: +if (unlock_on_failure) { +spin_unlock (&dentry->d_lock); +cpu_relax (); + } +return dentry;/* try again with same dentry */ }/*
Signature · 1 line
1.8.3.1
PATCH 11/16 expand dentry_kill(dentry, 0) in shrink_dentry_list()
In reply to Cong WangFrom: Al Viro <[email protected]>
Result will be massaged to saner shape in the next commits. It is ugly, no questions - the point of that one is to be a provably equivalent transformation (and it might be worth splitting a bit more).
--- fs/dcache.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)
fs/dcache.c +17 -13
@@ -800,6 +800,7 @@ static void shrink_dentry_list(struct list_head *list)struct dentry *dentry, *parent;while (!list_empty (list)) { +struct inode *inode; dentry = list_entry(list->prev, struct dentry, d_lru);spin_lock (&dentry->d_lock);/*
@@ -827,23 +828,26 @@ static void shrink_dentry_list(struct list_head *list)continue ; } - parent =dentry_kill (dentry,0 ); -/* -* If dentry_kill returns NULL, we have nothing more to do. -*/ -if (!parent) -continue ; - -if (unlikely (parent == dentry)) { -/* -* trylocks have failed and d_lock has been held the -* whole time, so it could not have been added to any -* other lists. Just add it back to the shrink list. -*/ + inode = dentry->d_inode; +if (inode &&unlikely (!spin_trylock (&inode->i_lock))) {d_shrink_add (dentry, list);spin_unlock (&dentry->d_lock);continue ; } + + parent =NULL ; +if (!IS_ROOT (dentry)) { + parent = dentry->d_parent; +if (unlikely (!spin_trylock (&parent->d_lock))) { +if (inode) +spin_unlock (&inode->i_lock); +d_shrink_add (dentry, list); +spin_unlock (&dentry->d_lock); +continue ; + } + } + +__dentry_kill (dentry);/* * We need to prune ancestors too. This is necessary to prevent * quadratic behavior of shrink_dcache_parent(), but is also
Signature · 1 line
1.8.3.1
PATCH 12/16 shrink_dentry_list(): take parent's ->d_lock earlier
In reply to Cong WangFrom: Al Viro <[email protected]>
The cause of livelocks there is that we are taking ->d_lock on dentry and its parent in the wrong order, forcing us to use trylock on the parent's one. d_walk() takes them in the right order, and unfortunately it's not hard to create a situation when shrink_dentry_list() can't make progress since trylock keeps failing, and shrink_dcache_parent() or check_submounts_and_drop() keeps calling d_walk() disrupting the very shrink_dentry_list() it's waiting for.
Solution is straightforward - if that trylock fails, let's unlock the dentry itself and take locks in the right order. We need to stabilize ->d_parent without holding ->d_lock, but that's doable using RCU. And we'd better do that in the very beginning of the loop in shrink_dentry_list(), since the checks on refcount, etc. would need to be redone anyway.
That deals with a half of the problem - killing dentries on the shrink list itself. Another one (dropping their parents) is in the next commit.
locking parent is interesting - it would be easy to do rcu_read_lock(), lock whatever we think is a parent, lock dentry itself and check if the parent is still the right one. Except that we need to check that *before* locking the dentry, or we are risking taking ->d_lock out of order. Fortunately, once the D1 is locked, we can check if D2->d_parent is equal to D1 without the need to lock D2; D2->d_parent can start or stop pointing to D1 only under D1->d_lock, so taking D1->d_lock is enough. In other words, the right solution is rcu_read_lock/lock what looks like parent right now/check if it's still our parent/rcu_read_unlock/lock the child.
--- fs/dcache.c | 53 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 12 deletions(-)
fs/dcache.c +41 -12
@@ -529,6 +529,38 @@ dentry_kill(struct dentry *dentry, int unlock_on_failure)return dentry;/* try again with same dentry */ } +static inline struct dentry *lock_parent (struct dentry *dentry) +{ +struct dentry *parent = dentry->d_parent; +if (IS_ROOT (dentry)) +return NULL ; +if (likely (spin_trylock (&parent->d_lock))) +return parent; +spin_unlock (&dentry->d_lock); +rcu_read_lock (); +again: + parent =ACCESS_ONCE (dentry->d_parent); +spin_lock (&parent->d_lock); +/* +* We can't blindly lock dentry until we are sure +* that we won't violate the locking order. +* Any changes of dentry->d_parent must have +* been done with parent->d_lock held, so +* spin_lock() above is enough of a barrier +* for checking if it's still our child. +*/ +if (unlikely (parent != dentry->d_parent)) { +spin_unlock (&parent->d_lock); + goto again; + } +rcu_read_unlock (); +if (parent != dentry) +spin_lock (&dentry->d_lock); +else + parent =NULL ; +return parent; +} +/* * This is dput *
@@ -803,6 +835,8 @@ static void shrink_dentry_list(struct list_head *list)struct inode *inode; dentry = list_entry(list->prev, struct dentry, d_lru);spin_lock (&dentry->d_lock); + parent =lock_parent (dentry); +/* * The dispose list is isolated and dentries are not accounted * to the LRU here, so we can simply remove it from the list
@@ -816,6 +850,8 @@ static void shrink_dentry_list(struct list_head *list)*/ if ((int )dentry->d_lockref.count >0 ) {spin_unlock (&dentry->d_lock); +if (parent) +spin_unlock (&parent->d_lock);continue ; }
@@ -823,6 +859,8 @@ static void shrink_dentry_list(struct list_head *list)if (unlikely (dentry->d_flags &DCACHE_DENTRY_KILLED )) {bool can_free = dentry->d_flags &DCACHE_MAY_FREE ;spin_unlock (&dentry->d_lock); +if (parent) +spin_unlock (&parent->d_lock);if (can_free)dentry_free (dentry);continue ;
@@ -832,22 +870,13 @@ static void shrink_dentry_list(struct list_head *list)if (inode &&unlikely (!spin_trylock (&inode->i_lock))) {d_shrink_add (dentry, list);spin_unlock (&dentry->d_lock); +if (parent) +spin_unlock (&parent->d_lock);continue ; } - parent =NULL ; -if (!IS_ROOT (dentry)) { - parent = dentry->d_parent; -if (unlikely (!spin_trylock (&parent->d_lock))) { -if (inode) -spin_unlock (&inode->i_lock); -d_shrink_add (dentry, list); -spin_unlock (&dentry->d_lock); -continue ; - } - } -__dentry_kill (dentry); +/* * We need to prune ancestors too. This is necessary to prevent * quadratic behavior of shrink_dcache_parent(), but is also
Signature · 1 line
1.8.3.1
PATCH 13/16 dealing with the rest of shrink_dentry_list() livelock
In reply to Cong WangFrom: Al Viro <[email protected]>
We have the same problem with ->d_lock order in the inner loop, where we are dropping references to ancestors. Same solution, basically - instead of using dentry_kill() we use lock_parent() (introduced in the previous commit) to get that lock in a safe way, recheck ->d_count (in case if lock_parent() has ended up dropping and retaking ->d_lock and somebody managed to grab a reference during that window), trylock the inode->i_lock and use __dentry_kill() to do the rest.
--- fs/dcache.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)
fs/dcache.c +20 -2
@@ -884,8 +884,26 @@ static void shrink_dentry_list(struct list_head *list)* fragmentation. */ dentry = parent; -while (dentry && !lockref_put_or_lock (&dentry->d_lockref)) - dentry =dentry_kill (dentry,1 ); +while (dentry && !lockref_put_or_lock (&dentry->d_lockref)) { + parent =lock_parent (dentry); +if (dentry->d_lockref.count !=1 ) { + dentry->d_lockref.count--; +spin_unlock (&dentry->d_lock); +if (parent) +spin_unlock (&parent->d_lock); +break ; + } + inode = dentry->d_inode;/* can't be NULL */ +if (unlikely (!spin_trylock (&inode->i_lock))) { +spin_unlock (&dentry->d_lock); +if (parent) +spin_unlock (&parent->d_lock); +cpu_relax (); +continue ; + } +__dentry_kill (dentry); + dentry = parent; + } } }
Signature · 1 line
1.8.3.1
PATCH 14/16 dentry_kill() doesn't need the second argument now
In reply to Cong WangFrom: Al Viro <[email protected]>
it's 1 in the only remaining caller.
--- fs/dcache.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
fs/dcache.c +4 -7
@@ -499,8 +499,7 @@ static void __dentry_kill(struct dentry *dentry)* If ref is non-zero, then decrement the refcount too. * Returns dentry requiring refcount drop, or NULL if we're done. */ -static struct dentry * -dentry_kill (struct dentry *dentry,int unlock_on_failure) +static struct dentry *dentry_kill (struct dentry *dentry) __releases(dentry->d_lock) {struct inode *inode = dentry->d_inode;
@@ -522,10 +521,8 @@ dentry_kill(struct dentry *dentry, int unlock_on_failure)return parent; failed: -if (unlock_on_failure) { -spin_unlock (&dentry->d_lock); -cpu_relax (); - } +spin_unlock (&dentry->d_lock); +cpu_relax ();return dentry;/* try again with same dentry */ }
@@ -614,7 +611,7 @@ void dput(struct dentry *dentry)return ; kill_it: - dentry =dentry_kill (dentry,1 ); + dentry =dentry_kill (dentry);if (dentry) goto repeat; }
Signature · 1 line
1.8.3.1
PATCH 15/16 dcache: add missing lockdep annotation
In reply to Cong WangFrom: Linus Torvalds <[email protected]>
lock_parent() very much on purpose does nested locking of dentries, and is careful to maintain the right order (lock parent first). But because it didn't annotate the nested locking order, lockdep thought it might be a deadlock on d_lock, and complained.
Add the proper annotation for the inner locking of the child dentry to make lockdep happy.
Introduced by commit 046b961b45f9 ("shrink_dentry_list(): take parent's ->d_lock earlier").
--- fs/dcache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/dcache.c +1 -1
@@ -552,7 +552,7 @@ static inline struct dentry *lock_parent(struct dentry *dentry) }rcu_read_unlock ();if (parent != dentry) -spin_lock (&dentry->d_lock); +spin_lock_nested (&dentry->d_lock,DENTRY_D_LOCK_NESTED );else parent =NULL ;return parent;
Signature · 1 line
1.8.3.1
PATCH 16/16 lock_parent: don't step on stale ->d_parent of all-but-freed one
In reply to Cong WangFrom: Al Viro <[email protected]>
Dentry that had been through (or into) __dentry_kill() might be seen by shrink_dentry_list(); that's normal, it'll be taken off the shrink list and freed if __dentry_kill() has already finished. The problem is, its ->d_parent might be pointing to already freed dentry, so lock_parent() needs to be careful.
We need to check that dentry hasn't already gone into __dentry_kill() *and* grab rcu_read_lock() before dropping ->d_lock - the latter makes sure that whatever we see in ->d_parent after dropping ->d_lock it won't be freed until we drop rcu_read_lock().
--- fs/dcache.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
fs/dcache.c +3 -1
@@ -531,10 +531,12 @@ static inline struct dentry *lock_parent(struct dentry *dentry)struct dentry *parent = dentry->d_parent;if (IS_ROOT (dentry))return NULL ; +if (unlikely ((int )dentry->d_lockref.count <0 )) +return NULL ;if (likely (spin_trylock (&parent->d_lock)))return parent; -spin_unlock (&dentry->d_lock);rcu_read_lock (); +spin_unlock (&dentry->d_lock); again: parent =ACCESS_ONCE (dentry->d_parent);spin_lock (&parent->d_lock);
Signature · 1 line
1.8.3.1
Greg KH
maintainerQuoted 18 lines · Cong Wang
On at least 3 different machines, we have seen dentry shrink list corruption with 3.14 stable kernel (see the end of this email), which I think is fixed by a set of patches from Al.
This bug is probably introduced by the following commit in 3.12:
commit dd1f6b2e43a53ee58eb87d5e623cf44e277d005d Author: Dave Chinner <[email protected]> Date: Wed Aug 28 10:17:55 2013 +1000 dcache: remove dentries from LRU before putting on dispose listsince previously we held the per sb lock. It is hard to reproduce this bug, because it is a race condition which happens when we update two contiguous dentries on a shrink list. Matt ran some ltp stress tests and doesn't see any problem with the patched kernel. And, 3.13 needs them too, but we have no time or resource to backport or test them on 3.13.
Quoted 7 lines
The first 7 patches are the original patchset from Al, the rest of patches are either needed for context or are folllow-up fixes, especially the last 2 patches. Of course all of them are merged in upstream, see each patch for details. Note, I don't get any conflict when backporting them to 3.14.
Signed-off-by Cong Wang <[email protected]>Tested-by Matthew Mullins <[email protected]>
I'm not ignoring these, just waiting for the other major 3.14-stable patches that I've merged over the past few releases to settle down and ensure they work properly. It will probably be a few more weeks before I can get to these, don't worry, they aren't lost.
thanks,
greg k-h
Quoted 6 lines · Greg KH
I'm not ignoring these, just waiting for the other major 3.14-stable patches that I've merged over the past few releases to settle down and ensure they work properly. It will probably be a few more weeks before I can get to these, don't worry, they aren't lost.
Awesome! Thanks for taking care of them!