Re: [PATCH v2] dcache: Add cond_resched in shrink_dentry_list

[Davidlohr Bueso <dbueso@suse.com>]

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