* [PATCH] dcache: Add cond_resched in shrink_dentry_list
@ 2018-03-22 9:39 Nikolay Borisov
2018-03-22 11:42 ` [PATCH v2] " Nikolay Borisov
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Nikolay Borisov @ 2018-03-22 9:39 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, rgoldwyn, jeffm, dbueso, Nikolay Borisov
As previously [1] reported it's possible to call shrink_dentry_list
with a large number of dentries (> 10000). This, in turn, could
trigger the softlockup detector and possibly trigger a panic.
In addition to the unmount path being vulnerable to this scenario,
at SuSE we've observed similar situation happening during process
exit on processes that touch a lot of dentries. Here is an excerpt
from a crash dump. The number after the colon are the number of
dentries on the list passed to shrink_dentry_list:
PID 99760: 10722
PID 107530: 215
PID 108809: 24134
PID 108877: 21331
PID 141708: 16487
So we want to kill between 15k-25k dentries without yielding.
And one possible call stack looks like:
4 [ffff8839ece41db0] _raw_spin_lock at ffffffff8152a5f8
5 [ffff8839ece41db0] evict at ffffffff811c3026
6 [ffff8839ece41dd0] __dentry_kill at ffffffff811bf258
7 [ffff8839ece41df0] shrink_dentry_list at ffffffff811bf593
8 [ffff8839ece41e18] shrink_dcache_parent at ffffffff811bf830
9 [ffff8839ece41e50] proc_flush_task at ffffffff8120dd61
10 [ffff8839ece41ec0] release_task at ffffffff81059ebd
11 [ffff8839ece41f08] do_exit at ffffffff8105b8ce
12 [ffff8839ece41f78] sys_exit at ffffffff8105bd53
13 [ffff8839ece41f80] system_call_fastpath at ffffffff81532909
While some of the callers of shrink_dentry_list do use cond_resched,
this is not sufficient to prevent softlockups. So just move
cond_resched into shrink_dentry_list from its callers.
[1] https://patchwork.kernel.org/patch/8642031/
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/dcache.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 8945e6cabd93..2fcb5b0fa581 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -982,6 +982,9 @@ static void shrink_dentry_list(struct list_head *list)
while (!list_empty(list)) {
struct inode *inode;
+
+ con_resched();
+
dentry = list_entry(list->prev, struct dentry, d_lru);
spin_lock(&dentry->d_lock);
parent = lock_parent(dentry);
@@ -1177,7 +1180,6 @@ void shrink_dcache_sb(struct super_block *sb)
this_cpu_sub(nr_dentry_unused, freed);
shrink_dentry_list(&dispose);
- cond_resched();
} while (list_lru_count(&sb->s_dentry_lru) > 0);
}
EXPORT_SYMBOL(shrink_dcache_sb);
@@ -1459,7 +1461,6 @@ void shrink_dcache_parent(struct dentry *parent)
break;
shrink_dentry_list(&data.dispose);
- cond_resched();
}
}
EXPORT_SYMBOL(shrink_dcache_parent);
@@ -1586,7 +1587,6 @@ void d_invalidate(struct dentry *dentry)
detach_mounts(data.mountpoint);
dput(data.mountpoint);
}
- cond_resched();
}
}
EXPORT_SYMBOL(d_invalidate);
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2] dcache: Add cond_resched in shrink_dentry_list
2018-03-22 9:39 [PATCH] dcache: Add cond_resched in shrink_dentry_list Nikolay Borisov
@ 2018-03-22 11:42 ` Nikolay Borisov
2018-03-22 15:33 ` Davidlohr Bueso
2018-03-25 1:34 ` [PATCH] " kbuild test robot
2018-03-25 1:58 ` kbuild test robot
2 siblings, 1 reply; 5+ messages in thread
From: Nikolay Borisov @ 2018-03-22 11:42 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, rgoldwyn, jeffm, dbueso, Nikolay Borisov
As previously [1] reported it's possible to call shrink_dentry_list
with a large number of dentries (> 10000). This, in turn, could
trigger the softlockup detector and possibly trigger a panic.
In addition to the unmount path being vulnerable to this scenario,
at SuSE we've observed similar situation happening during process
exit on processes that touch a lot of dentries. Here is an excerpt
from a crash dump. The number after the colon are the number of
dentries on the list passed to shrink_dentry_list:
PID 99760: 10722
PID 107530: 215
PID 108809: 24134
PID 108877: 21331
PID 141708: 16487
So we want to kill between 15k-25k dentries without yielding.
And one possible call stack looks like:
4 [ffff8839ece41db0] _raw_spin_lock at ffffffff8152a5f8
5 [ffff8839ece41db0] evict at ffffffff811c3026
6 [ffff8839ece41dd0] __dentry_kill at ffffffff811bf258
7 [ffff8839ece41df0] shrink_dentry_list at ffffffff811bf593
8 [ffff8839ece41e18] shrink_dcache_parent at ffffffff811bf830
9 [ffff8839ece41e50] proc_flush_task at ffffffff8120dd61
10 [ffff8839ece41ec0] release_task at ffffffff81059ebd
11 [ffff8839ece41f08] do_exit at ffffffff8105b8ce
12 [ffff8839ece41f78] sys_exit at ffffffff8105bd53
13 [ffff8839ece41f80] system_call_fastpath at ffffffff81532909
While some of the callers of shrink_dentry_list do use cond_resched,
this is not sufficient to prevent softlockups. So just move
cond_resched into shrink_dentry_list from its callers.
[1] https://patchwork.kernel.org/patch/8642031/
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
V2:
* Fix typo in conD_resched
* Actually compile test it
fs/dcache.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 8945e6cabd93..d9f3a53b5898 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -982,6 +982,9 @@ static void shrink_dentry_list(struct list_head *list)
while (!list_empty(list)) {
struct inode *inode;
+
+ cond_resched();
+
dentry = list_entry(list->prev, struct dentry, d_lru);
spin_lock(&dentry->d_lock);
parent = lock_parent(dentry);
@@ -1177,7 +1180,6 @@ void shrink_dcache_sb(struct super_block *sb)
this_cpu_sub(nr_dentry_unused, freed);
shrink_dentry_list(&dispose);
- cond_resched();
} while (list_lru_count(&sb->s_dentry_lru) > 0);
}
EXPORT_SYMBOL(shrink_dcache_sb);
@@ -1459,7 +1461,6 @@ void shrink_dcache_parent(struct dentry *parent)
break;
shrink_dentry_list(&data.dispose);
- cond_resched();
}
}
EXPORT_SYMBOL(shrink_dcache_parent);
@@ -1586,7 +1587,6 @@ void d_invalidate(struct dentry *dentry)
detach_mounts(data.mountpoint);
dput(data.mountpoint);
}
- cond_resched();
}
}
EXPORT_SYMBOL(d_invalidate);
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] dcache: Add cond_resched in shrink_dentry_list
2018-03-22 11:42 ` [PATCH v2] " Nikolay Borisov
@ 2018-03-22 15:33 ` Davidlohr Bueso
0 siblings, 0 replies; 5+ messages in thread
From: Davidlohr Bueso @ 2018-03-22 15:33 UTC (permalink / raw)
To: Nikolay Borisov, viro; +Cc: linux-fsdevel, rgoldwyn, jeffm, akpm
Cc'ing akpm.
On Thu, 2018-03-22 at 13:42 +0200, Nikolay Borisov wrote:
> As previously [1] reported it's possible to call shrink_dentry_list
> with a large number of dentries (> 10000). This, in turn, could
> trigger the softlockup detector and possibly trigger a panic.
> In addition to the unmount path being vulnerable to this scenario,
> at SuSE we've observed similar situation happening during process
> exit on processes that touch a lot of dentries. Here is an excerpt
> from a crash dump. The number after the colon are the number of
> dentries on the list passed to shrink_dentry_list:
>
> PID 99760: 10722
> PID 107530: 215
> PID 108809: 24134
> PID 108877: 21331
> PID 141708: 16487
>
> So we want to kill between 15k-25k dentries without yielding.
>
> And one possible call stack looks like:
>
> 4 [ffff8839ece41db0] _raw_spin_lock at ffffffff8152a5f8
> 5 [ffff8839ece41db0] evict at ffffffff811c3026
> 6 [ffff8839ece41dd0] __dentry_kill at ffffffff811bf258
> 7 [ffff8839ece41df0] shrink_dentry_list at ffffffff811bf593
> 8 [ffff8839ece41e18] shrink_dcache_parent at ffffffff811bf830
> 9 [ffff8839ece41e50] proc_flush_task at ffffffff8120dd61
> 10 [ffff8839ece41ec0] release_task at ffffffff81059ebd
> 11 [ffff8839ece41f08] do_exit at ffffffff8105b8ce
> 12 [ffff8839ece41f78] sys_exit at ffffffff8105bd53
> 13 [ffff8839ece41f80] system_call_fastpath at ffffffff81532909
>
> While some of the callers of shrink_dentry_list do use cond_resched,
> this is not sufficient to prevent softlockups. So just move
> cond_resched into shrink_dentry_list from its callers.
>
> [1] https://patchwork.kernel.org/patch/8642031/
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>
> V2:
> * Fix typo in conD_resched
> * Actually compile test it
> fs/dcache.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 8945e6cabd93..d9f3a53b5898 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -982,6 +982,9 @@ static void shrink_dentry_list(struct list_head
> *list)
>
> while (!list_empty(list)) {
> struct inode *inode;
> +
> + cond_resched();
> +
> dentry = list_entry(list->prev, struct dentry,
> d_lru);
> spin_lock(&dentry->d_lock);
> parent = lock_parent(dentry);
> @@ -1177,7 +1180,6 @@ void shrink_dcache_sb(struct super_block *sb)
>
> this_cpu_sub(nr_dentry_unused, freed);
> shrink_dentry_list(&dispose);
> - cond_resched();
> } while (list_lru_count(&sb->s_dentry_lru) > 0);
> }
> EXPORT_SYMBOL(shrink_dcache_sb);
> @@ -1459,7 +1461,6 @@ void shrink_dcache_parent(struct dentry
> *parent)
> break;
>
> shrink_dentry_list(&data.dispose);
> - cond_resched();
> }
> }
> EXPORT_SYMBOL(shrink_dcache_parent);
> @@ -1586,7 +1587,6 @@ void d_invalidate(struct dentry *dentry)
> detach_mounts(data.mountpoint);
> dput(data.mountpoint);
> }
> - cond_resched();
I was wondering about whether not dropping this one was safe because of
the possible call to __detach_mounts(). But I would assume that the
amount of mount point entries for a dentry is quite low making that
cond_resched() really for shrink_dentry_list(); so yeah removing it
makes sense.
> }
> }
> EXPORT_SYMBOL(d_invalidate);
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dcache: Add cond_resched in shrink_dentry_list
2018-03-22 9:39 [PATCH] dcache: Add cond_resched in shrink_dentry_list Nikolay Borisov
2018-03-22 11:42 ` [PATCH v2] " Nikolay Borisov
@ 2018-03-25 1:34 ` kbuild test robot
2018-03-25 1:58 ` kbuild test robot
2 siblings, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2018-03-25 1:34 UTC (permalink / raw)
To: Nikolay Borisov
Cc: kbuild-all, viro, linux-fsdevel, rgoldwyn, jeffm, dbueso,
Nikolay Borisov
[-- Attachment #1: Type: text/plain, Size: 3748 bytes --]
Hi Nikolay,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6]
[cannot apply to next-20180323]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Nikolay-Borisov/dcache-Add-cond_resched-in-shrink_dentry_list/20180325-085028
config: i386-randconfig-x013-201812 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
fs/dcache.c: In function 'shrink_dentry_list':
>> fs/dcache.c:986:3: error: implicit declaration of function 'con_resched'; did you mean 'cond_resched'? [-Werror=implicit-function-declaration]
con_resched();
^~~~~~~~~~~
cond_resched
cc1: some warnings being treated as errors
vim +986 fs/dcache.c
978
979 static void shrink_dentry_list(struct list_head *list)
980 {
981 struct dentry *dentry, *parent;
982
983 while (!list_empty(list)) {
984 struct inode *inode;
985
> 986 con_resched();
987
988 dentry = list_entry(list->prev, struct dentry, d_lru);
989 spin_lock(&dentry->d_lock);
990 parent = lock_parent(dentry);
991
992 /*
993 * The dispose list is isolated and dentries are not accounted
994 * to the LRU here, so we can simply remove it from the list
995 * here regardless of whether it is referenced or not.
996 */
997 d_shrink_del(dentry);
998
999 /*
1000 * We found an inuse dentry which was not removed from
1001 * the LRU because of laziness during lookup. Do not free it.
1002 */
1003 if (dentry->d_lockref.count > 0) {
1004 spin_unlock(&dentry->d_lock);
1005 if (parent)
1006 spin_unlock(&parent->d_lock);
1007 continue;
1008 }
1009
1010
1011 if (unlikely(dentry->d_flags & DCACHE_DENTRY_KILLED)) {
1012 bool can_free = dentry->d_flags & DCACHE_MAY_FREE;
1013 spin_unlock(&dentry->d_lock);
1014 if (parent)
1015 spin_unlock(&parent->d_lock);
1016 if (can_free)
1017 dentry_free(dentry);
1018 continue;
1019 }
1020
1021 inode = dentry->d_inode;
1022 if (inode && unlikely(!spin_trylock(&inode->i_lock))) {
1023 d_shrink_add(dentry, list);
1024 spin_unlock(&dentry->d_lock);
1025 if (parent)
1026 spin_unlock(&parent->d_lock);
1027 continue;
1028 }
1029
1030 __dentry_kill(dentry);
1031
1032 /*
1033 * We need to prune ancestors too. This is necessary to prevent
1034 * quadratic behavior of shrink_dcache_parent(), but is also
1035 * expected to be beneficial in reducing dentry cache
1036 * fragmentation.
1037 */
1038 dentry = parent;
1039 while (dentry && !lockref_put_or_lock(&dentry->d_lockref)) {
1040 parent = lock_parent(dentry);
1041 if (dentry->d_lockref.count != 1) {
1042 dentry->d_lockref.count--;
1043 spin_unlock(&dentry->d_lock);
1044 if (parent)
1045 spin_unlock(&parent->d_lock);
1046 break;
1047 }
1048 inode = dentry->d_inode; /* can't be NULL */
1049 if (unlikely(!spin_trylock(&inode->i_lock))) {
1050 spin_unlock(&dentry->d_lock);
1051 if (parent)
1052 spin_unlock(&parent->d_lock);
1053 cpu_relax();
1054 continue;
1055 }
1056 __dentry_kill(dentry);
1057 dentry = parent;
1058 }
1059 }
1060 }
1061
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25981 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dcache: Add cond_resched in shrink_dentry_list
2018-03-22 9:39 [PATCH] dcache: Add cond_resched in shrink_dentry_list Nikolay Borisov
2018-03-22 11:42 ` [PATCH v2] " Nikolay Borisov
2018-03-25 1:34 ` [PATCH] " kbuild test robot
@ 2018-03-25 1:58 ` kbuild test robot
2 siblings, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2018-03-25 1:58 UTC (permalink / raw)
To: Nikolay Borisov
Cc: kbuild-all, viro, linux-fsdevel, rgoldwyn, jeffm, dbueso,
Nikolay Borisov
[-- Attachment #1: Type: text/plain, Size: 3703 bytes --]
Hi Nikolay,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6]
[cannot apply to next-20180323]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Nikolay-Borisov/dcache-Add-cond_resched-in-shrink_dentry_list/20180325-085028
config: i386-randconfig-a0-201812 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
fs/dcache.c: In function 'shrink_dentry_list':
>> fs/dcache.c:986:3: error: implicit declaration of function 'con_resched' [-Werror=implicit-function-declaration]
con_resched();
^
cc1: some warnings being treated as errors
vim +/con_resched +986 fs/dcache.c
978
979 static void shrink_dentry_list(struct list_head *list)
980 {
981 struct dentry *dentry, *parent;
982
983 while (!list_empty(list)) {
984 struct inode *inode;
985
> 986 con_resched();
987
988 dentry = list_entry(list->prev, struct dentry, d_lru);
989 spin_lock(&dentry->d_lock);
990 parent = lock_parent(dentry);
991
992 /*
993 * The dispose list is isolated and dentries are not accounted
994 * to the LRU here, so we can simply remove it from the list
995 * here regardless of whether it is referenced or not.
996 */
997 d_shrink_del(dentry);
998
999 /*
1000 * We found an inuse dentry which was not removed from
1001 * the LRU because of laziness during lookup. Do not free it.
1002 */
1003 if (dentry->d_lockref.count > 0) {
1004 spin_unlock(&dentry->d_lock);
1005 if (parent)
1006 spin_unlock(&parent->d_lock);
1007 continue;
1008 }
1009
1010
1011 if (unlikely(dentry->d_flags & DCACHE_DENTRY_KILLED)) {
1012 bool can_free = dentry->d_flags & DCACHE_MAY_FREE;
1013 spin_unlock(&dentry->d_lock);
1014 if (parent)
1015 spin_unlock(&parent->d_lock);
1016 if (can_free)
1017 dentry_free(dentry);
1018 continue;
1019 }
1020
1021 inode = dentry->d_inode;
1022 if (inode && unlikely(!spin_trylock(&inode->i_lock))) {
1023 d_shrink_add(dentry, list);
1024 spin_unlock(&dentry->d_lock);
1025 if (parent)
1026 spin_unlock(&parent->d_lock);
1027 continue;
1028 }
1029
1030 __dentry_kill(dentry);
1031
1032 /*
1033 * We need to prune ancestors too. This is necessary to prevent
1034 * quadratic behavior of shrink_dcache_parent(), but is also
1035 * expected to be beneficial in reducing dentry cache
1036 * fragmentation.
1037 */
1038 dentry = parent;
1039 while (dentry && !lockref_put_or_lock(&dentry->d_lockref)) {
1040 parent = lock_parent(dentry);
1041 if (dentry->d_lockref.count != 1) {
1042 dentry->d_lockref.count--;
1043 spin_unlock(&dentry->d_lock);
1044 if (parent)
1045 spin_unlock(&parent->d_lock);
1046 break;
1047 }
1048 inode = dentry->d_inode; /* can't be NULL */
1049 if (unlikely(!spin_trylock(&inode->i_lock))) {
1050 spin_unlock(&dentry->d_lock);
1051 if (parent)
1052 spin_unlock(&parent->d_lock);
1053 cpu_relax();
1054 continue;
1055 }
1056 __dentry_kill(dentry);
1057 dentry = parent;
1058 }
1059 }
1060 }
1061
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30662 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-03-25 2:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-22 9:39 [PATCH] dcache: Add cond_resched in shrink_dentry_list Nikolay Borisov
2018-03-22 11:42 ` [PATCH v2] " Nikolay Borisov
2018-03-22 15:33 ` Davidlohr Bueso
2018-03-25 1:34 ` [PATCH] " kbuild test robot
2018-03-25 1:58 ` kbuild test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).