Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-16 Thread Khazhismel Kumykov
On Sun, Apr 15, 2018 at 3:34 PM, Al Viro wrote: > On Sun, Apr 15, 2018 at 10:54:55PM +0100, Al Viro wrote: >> On Sun, Apr 15, 2018 at 09:40:54PM +0100, Al Viro wrote: >> >> > BTW, the current placement of cond_resched() looks bogus; suppose we >> > have collected a lot of victims and ran into need

Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-15 Thread Al Viro
On Sun, Apr 15, 2018 at 10:54:55PM +0100, Al Viro wrote: > On Sun, Apr 15, 2018 at 09:40:54PM +0100, Al Viro wrote: > > > BTW, the current placement of cond_resched() looks bogus; suppose we > > have collected a lot of victims and ran into need_resched(). We leave > > d_walk() and call shrink_den

Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-15 Thread Al Viro
On Sun, Apr 15, 2018 at 09:40:54PM +0100, Al Viro wrote: > BTW, the current placement of cond_resched() looks bogus; suppose we > have collected a lot of victims and ran into need_resched(). We leave > d_walk() and call shrink_dentry_list(). At that point there's a lot > of stuff on our shrink l

Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-15 Thread Al Viro
On Sun, Apr 15, 2018 at 11:34:17AM -0700, Linus Torvalds wrote: > No, it's obviously not type-safe, but at least it's _legible_, and is > a pattern, while that "let's randomly just do a cast in the middle of > the code" is just nasty. Sure, no problem... I really wish there was a way to say voi

Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-15 Thread Linus Torvalds
On Sat, Apr 14, 2018 at 5:51 PM, Al Viro wrote: >> if (!list_empty(&data.select.found)) That was obviously meant to be just if (data.select.found) I had just cut-and-pasted a bit too much. > You would have to do the same in check_and_drop() as well, > and that b

Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-15 Thread Al Viro
On Sun, Apr 15, 2018 at 03:39:44AM +0100, Al Viro wrote: > I really wonder if we should just do the following in > d_invalidate(): > * grab ->d_lock on victim, check if it's unhashed, > unlock and bugger off if it is. Otherwise, unhash and unlock. > >From that point on any d_set_mounted() i

Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-14 Thread Al Viro
On Sun, Apr 15, 2018 at 01:51:07AM +0100, Al Viro wrote: > On Sat, Apr 14, 2018 at 02:47:21PM -0700, Linus Torvalds wrote: > > On Sat, Apr 14, 2018 at 1:58 PM, Al Viro wrote: > > > > > > That breaks d_invalidate(), unfortunately. Look at the termination > > > conditions in the loop there... > >

Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-14 Thread Al Viro
On Sat, Apr 14, 2018 at 02:47:21PM -0700, Linus Torvalds wrote: > On Sat, Apr 14, 2018 at 1:58 PM, Al Viro wrote: > > > > That breaks d_invalidate(), unfortunately. Look at the termination > > conditions in the loop there... > > Ugh. I was going to say "but that doesn't even use select_collect()

Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-14 Thread Linus Torvalds
On Sat, Apr 14, 2018 at 1:58 PM, Al Viro wrote: > > That breaks d_invalidate(), unfortunately. Look at the termination > conditions in the loop there... Ugh. I was going to say "but that doesn't even use select_collect()", but yeah, detach_and_collect() calls it. It would be easy enough to just

Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-14 Thread Al Viro
On Sat, Apr 14, 2018 at 09:36:23AM -0700, Linus Torvalds wrote: > But it does *not* make sense for the case where we've hit a dentry > that is already on the shrink list. Sure, we'll continue to gather all > the other dentries, but if there is concurrent shrinking, shouldn't we > give up the CPU mo

Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-14 Thread Linus Torvalds
On Sat, Apr 14, 2018 at 1:02 AM, Al Viro wrote: > > "Bail out" is definitely a bad idea, "sleep"... what on? Especially > since there might be several evictions we are overlapping with... Well, one thing that should be looked at is the return condition from select_collect() that shrink_dcache_pa

Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-14 Thread Al Viro
On Sat, Apr 14, 2018 at 10:00:29AM +0300, Nikolay Borisov wrote: > > > On 14.04.2018 00:14, Andrew Morton wrote: > > On Fri, 13 Apr 2018 13:28:23 -0700 Khazhismel Kumykov > > wrote: > > > >> shrink_dcache_parent may spin waiting for a parallel shrink_dentry_list. > >> In this case we may have

Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-14 Thread Nikolay Borisov
On 14.04.2018 00:14, Andrew Morton wrote: > On Fri, 13 Apr 2018 13:28:23 -0700 Khazhismel Kumykov > wrote: > >> shrink_dcache_parent may spin waiting for a parallel shrink_dentry_list. >> In this case we may have 0 dentries to dispose, so we will never >> schedule out while waiting for the par

Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-13 Thread David Rientjes
On Fri, 13 Apr 2018, Khazhismel Kumykov wrote: > shrink_dcache_parent may spin waiting for a parallel shrink_dentry_list. > In this case we may have 0 dentries to dispose, so we will never > schedule out while waiting for the parallel shrink_dentry_list to > complete. > > Tested that this fixes s

Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-13 Thread Andrew Morton
On Fri, 13 Apr 2018 13:28:23 -0700 Khazhismel Kumykov wrote: > shrink_dcache_parent may spin waiting for a parallel shrink_dentry_list. > In this case we may have 0 dentries to dispose, so we will never > schedule out while waiting for the parallel shrink_dentry_list to > complete. > > Tested th

[PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache_parent()

2018-04-13 Thread Khazhismel Kumykov
shrink_dcache_parent may spin waiting for a parallel shrink_dentry_list. In this case we may have 0 dentries to dispose, so we will never schedule out while waiting for the parallel shrink_dentry_list to complete. Tested that this fixes syzbot reports of stalls in shrink_dcache_parent() Fixes: 32