[PATCH] fs: avoid softlockups in s_inodes iterators

[PATCH] fs: avoid softlockups in s_inodes iterators

[Eric Sandeen]

Anything that walks all inodes on sb->s_inodes list without rescheduling
risks softlockups.

Previous efforts were made in 2 functions, see:

c27d82f fs/drop_caches.c: avoid softlockups in drop_pagecache_sb()
ac05fbb inode: don't softlockup when evicting inodes

but there hasn't been an audit of all walkers, so do that now.  This
also consistently moves the cond_resched() calls to the bottom of each
loop.

One remains: remove_dquot_ref(), because I'm not quite sure how to deal
with that one w/o taking the i_lock.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/drop_caches.c b/fs/drop_caches.c
index d31b6c72b476..dc1a1d5d825b 100644
--- a/fs/drop_caches.c
+++ b/fs/drop_caches.c
@@ -35,11 +35,11 @@ static void drop_pagecache_sb(struct super_block *sb, void *unused)
 		spin_unlock(&inode->i_lock);
 		spin_unlock(&sb->s_inode_list_lock);
 
-		cond_resched();
 		invalidate_mapping_pages(inode->i_mapping, 0, -1);
 		iput(toput_inode);
 		toput_inode = inode;
 
+		cond_resched();
 		spin_lock(&sb->s_inode_list_lock);
 	}
 	spin_unlock(&sb->s_inode_list_lock);
diff --git a/fs/inode.c b/fs/inode.c
index fef457a42882..b0c789bb3dba 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -676,6 +676,7 @@ int invalidate_inodes(struct super_block *sb, bool kill_dirty)
 	struct inode *inode, *next;
 	LIST_HEAD(dispose);
 
+again:
 	spin_lock(&sb->s_inode_list_lock);
 	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) {
 		spin_lock(&inode->i_lock);
@@ -698,6 +699,13 @@ int invalidate_inodes(struct super_block *sb, bool kill_dirty)
 		inode_lru_list_del(inode);
 		spin_unlock(&inode->i_lock);
 		list_add(&inode->i_lru, &dispose);
+
+		if (need_resched()) {
+			spin_unlock(&sb->s_inode_list_lock);
+			cond_resched();
+			dispose_list(&dispose);
+			goto again;
+		}
 	}
 	spin_unlock(&sb->s_inode_list_lock);
 
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 2ecef6155fc0..15f77cbbd188 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -67,22 +67,19 @@ static void fsnotify_unmount_inodes(struct super_block *sb)
 		spin_unlock(&inode->i_lock);
 		spin_unlock(&sb->s_inode_list_lock);
 
-		if (iput_inode)
-			iput(iput_inode);
-
+		iput(iput_inode);
 		/* for each watch, send FS_UNMOUNT and then remove it */
 		fsnotify(inode, FS_UNMOUNT, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
-
 		fsnotify_inode_delete(inode);
 
 		iput_inode = inode;
 
+		cond_resched();
 		spin_lock(&sb->s_inode_list_lock);
 	}
 	spin_unlock(&sb->s_inode_list_lock);
+	iput(iput_inode);
 
-	if (iput_inode)
-		iput(iput_inode);
 	/* Wait for outstanding inode references from connectors */
 	wait_var_event(&sb->s_fsnotify_inode_refs,
 		       !atomic_long_read(&sb->s_fsnotify_inode_refs));
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 6e826b454082..4a085b3c7cac 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -985,6 +985,7 @@ static int add_dquot_ref(struct super_block *sb, int type)
 		 * later.
 		 */
 		old_inode = inode;
+		cond_resched();
 		spin_lock(&sb->s_inode_list_lock);
 	}
 	spin_unlock(&sb->s_inode_list_lock);

Re: [PATCH] fs: avoid softlockups in s_inodes iterators

[Josef Bacik]

On Fri, Oct 11, 2019 at 11:49:38AM -0500, Eric Sandeen wrote:
> Anything that walks all inodes on sb->s_inodes list without rescheduling
> risks softlockups.
> 
> Previous efforts were made in 2 functions, see:
> 
> c27d82f fs/drop_caches.c: avoid softlockups in drop_pagecache_sb()
> ac05fbb inode: don't softlockup when evicting inodes
> 
> but there hasn't been an audit of all walkers, so do that now.  This
> also consistently moves the cond_resched() calls to the bottom of each
> loop.
> 
> One remains: remove_dquot_ref(), because I'm not quite sure how to deal
> with that one w/o taking the i_lock.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

You've got iput cleanups in here and cond_resched()'s.  I feel like this is a
missed opportunity to pad your patch count.  Thanks,

Josef

Re: [PATCH] fs: avoid softlockups in s_inodes iterators

[Eric Sandeen]

On 10/11/19 12:29 PM, Josef Bacik wrote:
> On Fri, Oct 11, 2019 at 11:49:38AM -0500, Eric Sandeen wrote:
>> Anything that walks all inodes on sb->s_inodes list without rescheduling
>> risks softlockups.
>>
>> Previous efforts were made in 2 functions, see:
>>
>> c27d82f fs/drop_caches.c: avoid softlockups in drop_pagecache_sb()
>> ac05fbb inode: don't softlockup when evicting inodes
>>
>> but there hasn't been an audit of all walkers, so do that now.  This
>> also consistently moves the cond_resched() calls to the bottom of each
>> loop.
>>
>> One remains: remove_dquot_ref(), because I'm not quite sure how to deal
>> with that one w/o taking the i_lock.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> You've got iput cleanups in here and cond_resched()'s.  I feel like this is a
> missed opportunity to pad your patch count.  Thanks,

yeah, I was going to suggest that I could split it out into 3
(move cond_rescheds, clean up iputs, add new rescheds) if there was a
request.  But it seemed a bit ridiculously granular.  Find by me
if desired, tho.

So, was that a request?

-Eric

Re: [PATCH] fs: avoid softlockups in s_inodes iterators

[Josef Bacik]

On Fri, Oct 11, 2019 at 12:34:45PM -0500, Eric Sandeen wrote:
> On 10/11/19 12:29 PM, Josef Bacik wrote:
> > On Fri, Oct 11, 2019 at 11:49:38AM -0500, Eric Sandeen wrote:
> >> Anything that walks all inodes on sb->s_inodes list without rescheduling
> >> risks softlockups.
> >>
> >> Previous efforts were made in 2 functions, see:
> >>
> >> c27d82f fs/drop_caches.c: avoid softlockups in drop_pagecache_sb()
> >> ac05fbb inode: don't softlockup when evicting inodes
> >>
> >> but there hasn't been an audit of all walkers, so do that now.  This
> >> also consistently moves the cond_resched() calls to the bottom of each
> >> loop.
> >>
> >> One remains: remove_dquot_ref(), because I'm not quite sure how to deal
> >> with that one w/o taking the i_lock.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > 
> > You've got iput cleanups in here and cond_resched()'s.  I feel like this is a
> > missed opportunity to pad your patch count.  Thanks,
> 
> yeah, I was going to suggest that I could split it out into 3
> (move cond_rescheds, clean up iputs, add new rescheds) if there was a
> request.  But it seemed a bit ridiculously granular.  Find by me
> if desired, tho.
> 
> So, was that a request?

I think just two patches, one for the iputs and one for the resched changes.
Thanks,

Josef

Re: [PATCH] fs: avoid softlockups in s_inodes iterators

[Matthew Wilcox]

On Fri, Oct 11, 2019 at 11:49:38AM -0500, Eric Sandeen wrote:
> @@ -698,6 +699,13 @@ int invalidate_inodes(struct super_block *sb, bool kill_dirty)
>  		inode_lru_list_del(inode);
>  		spin_unlock(&inode->i_lock);
>  		list_add(&inode->i_lru, &dispose);
> +
> +		if (need_resched()) {
> +			spin_unlock(&sb->s_inode_list_lock);
> +			cond_resched();
> +			dispose_list(&dispose);
> +			goto again;
> +		}
>  	}
>  	spin_unlock(&sb->s_inode_list_lock);
>  

Is this equivalent to:

+		cond_resched_lock(&sb->s_inode_list_lock));

or is disposing of the list a crucial part here?

Re: [PATCH] fs: avoid softlockups in s_inodes iterators

[Eric Sandeen]

On 10/11/19 1:32 PM, Matthew Wilcox wrote:
> On Fri, Oct 11, 2019 at 11:49:38AM -0500, Eric Sandeen wrote:
>> @@ -698,6 +699,13 @@ int invalidate_inodes(struct super_block *sb, bool kill_dirty)
>>  		inode_lru_list_del(inode);
>>  		spin_unlock(&inode->i_lock);
>>  		list_add(&inode->i_lru, &dispose);
>> +
>> +		if (need_resched()) {
>> +			spin_unlock(&sb->s_inode_list_lock);
>> +			cond_resched();
>> +			dispose_list(&dispose);
>> +			goto again;
>> +		}
>>  	}
>>  	spin_unlock(&sb->s_inode_list_lock);
>>  
> 
> Is this equivalent to:
> 
> +		cond_resched_lock(&sb->s_inode_list_lock));
> 
> or is disposing of the list a crucial part here?

I think we need to dispose, or we'll start with the entire ~unmodified list again after the goto:

-Eric

Re: [PATCH] fs: avoid softlockups in s_inodes iterators

[Eric Sandeen]

On 10/11/19 1:45 PM, Eric Sandeen wrote:
> On 10/11/19 1:32 PM, Matthew Wilcox wrote:
>> On Fri, Oct 11, 2019 at 11:49:38AM -0500, Eric Sandeen wrote:
>>> @@ -698,6 +699,13 @@ int invalidate_inodes(struct super_block *sb, bool kill_dirty)
>>>  		inode_lru_list_del(inode);
>>>  		spin_unlock(&inode->i_lock);
>>>  		list_add(&inode->i_lru, &dispose);
>>> +
>>> +		if (need_resched()) {
>>> +			spin_unlock(&sb->s_inode_list_lock);
>>> +			cond_resched();
>>> +			dispose_list(&dispose);
>>> +			goto again;
>>> +		}
>>>  	}
>>>  	spin_unlock(&sb->s_inode_list_lock);
>>>  
>>
>> Is this equivalent to:
>>
>> +		cond_resched_lock(&sb->s_inode_list_lock));
>>
>> or is disposing of the list a crucial part here?
> 
> I think we need to dispose, or we'll start with the entire ~unmodified list again after the goto:

Oh, if you meant in lieu of the goto, we can't drop that lock and
expect to pick up our traversal where we left off, can we?

-Eric

Re: [PATCH] fs: avoid softlockups in s_inodes iterators

[Dave Chinner]

On Fri, Oct 11, 2019 at 04:14:02PM -0500, Eric Sandeen wrote:
> 
> 
> On 10/11/19 1:45 PM, Eric Sandeen wrote:
> > On 10/11/19 1:32 PM, Matthew Wilcox wrote:
> >> On Fri, Oct 11, 2019 at 11:49:38AM -0500, Eric Sandeen wrote:
> >>> @@ -698,6 +699,13 @@ int invalidate_inodes(struct super_block *sb, bool kill_dirty)
> >>>  		inode_lru_list_del(inode);
> >>>  		spin_unlock(&inode->i_lock);
> >>>  		list_add(&inode->i_lru, &dispose);
> >>> +
> >>> +		if (need_resched()) {
> >>> +			spin_unlock(&sb->s_inode_list_lock);
> >>> +			cond_resched();
> >>> +			dispose_list(&dispose);
> >>> +			goto again;
> >>> +		}
> >>>  	}
> >>>  	spin_unlock(&sb->s_inode_list_lock);
> >>>  
> >>
> >> Is this equivalent to:
> >>
> >> +		cond_resched_lock(&sb->s_inode_list_lock));
> >>
> >> or is disposing of the list a crucial part here?
> > 
> > I think we need to dispose, or we'll start with the entire ~unmodified list again after the goto:
> 
> Oh, if you meant in lieu of the goto, we can't drop that lock and
> expect to pick up our traversal where we left off, can we?

No, we can't. Unless you're doing the iget/iput game (which we can't
here!) the moment the s_inode_list_lock is dropped we cannot rely on
the inode or it's next pointer to still be valid. Hence we have to
restart the traversal. And we dispose of the list before restarting
because there's nothing to gain by waiting until we've done the
entire sb inode list walk (could be hundreds of millions of inodes)
before we start actually freeing them....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] fs: avoid softlockups in s_inodes iterators

[Jan Kara]

On Fri 11-10-19 11:49:38, Eric Sandeen wrote:
> One remains: remove_dquot_ref(), because I'm not quite sure how to deal
> with that one w/o taking the i_lock.

Yeah, that will be somewhat tricky. But I think we can modify the standard
iget-iput dance like:

		if (need_resched()) {
			spin_lock(&inode->i_lock);
			if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) {
				/*
				 * Cannot break on this inode, need to do one
				 * more.
				 */
				spin_unlock(&inode->i_lock);
				continue;
			}
			__iget(inode);
			spin_unlock(&inode->i_lock);
			spin_unlock(&sb->s_inode_list_lock);
			iput(put_inode);
			put_inode = inode;
			cond_resched();
			spin_lock(&sb->s_inode_list_lock);
		}
	...
	iput(put_inode);

Will you transform this into a proper patch in your series or should I do
it?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR