Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] avoid softlockups in various s_inodes iterators
@ 2019-11-07 19:42 Eric Sandeen
  2019-11-07 19:47 ` [PATCH 1/2] fs: avoid softlockups in " Eric Sandeen
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Eric Sandeen @ 2019-11-07 19:42 UTC (permalink / raw)
  To: fsdevel; +Cc: Al Viro

2 patches to make sure we either schedule in an s_inodes walking
loop, or do our best to limit the size of the walk, to avoid soft
lockups.

-Eric


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/2] fs: avoid softlockups in s_inodes iterators
  2019-11-07 19:42 [PATCH 0/2] avoid softlockups in various s_inodes iterators Eric Sandeen
@ 2019-11-07 19:47 ` " Eric Sandeen
  2019-11-07 19:48 ` [PATCH 2/2] fs: call fsnotify_sb_delete after evict_inodes Eric Sandeen
  2019-11-07 19:58 ` [PATCH 0/2] avoid softlockups in various s_inodes iterators Eric Sandeen
  2 siblings, 0 replies; 5+ messages in thread
From: Eric Sandeen @ 2019-11-07 19:47 UTC (permalink / raw)
  To: fsdevel; +Cc: Al Viro

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 in cases where it already exists.

One loop 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>
Reviewed-by: Jan Kara <jack@suse.cz>
---

V2: Drop unrelated iput cleanups in fsnotify

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..ac9eb273e28c 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -77,6 +77,7 @@ static void fsnotify_unmount_inodes(struct super_block *sb)
 
         iput_inode = inode;
 
+        cond_resched();
         spin_lock(&sb->s_inode_list_lock);
     }
     spin_unlock(&sb->s_inode_list_lock);
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);



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/2] fs: call fsnotify_sb_delete after evict_inodes
  2019-11-07 19:42 [PATCH 0/2] avoid softlockups in various s_inodes iterators Eric Sandeen
  2019-11-07 19:47 ` [PATCH 1/2] fs: avoid softlockups in " Eric Sandeen
@ 2019-11-07 19:48 ` Eric Sandeen
  2019-11-07 19:58 ` [PATCH 0/2] avoid softlockups in various s_inodes iterators Eric Sandeen
  2 siblings, 0 replies; 5+ messages in thread
From: Eric Sandeen @ 2019-11-07 19:48 UTC (permalink / raw)
  To: fsdevel; +Cc: Al Viro

When a filesystem is unmounted, we currently call fsnotify_sb_delete()
before evict_inodes(), which means that fsnotify_unmount_inodes()
must iterate over all inodes on the superblock, even though it will
only act on inodes with a refcount.  This is inefficient and can lead
to livelocks as it iterates over many unrefcounted inodes.

However, since fsnotify_sb_delete() and evict_inodes() are working
on orthogonal sets of inodes (fsnotify_sb_delete() only cares about
nonzero refcount, and evict_inodes() only cares about zero refcount),
we can swap the order of the calls.  The fsnotify call will then have
a much smaller list to walk (any refcounted inodes).

This should speed things up overall, and avoid livelocks in 
fsnotify_unmount_inodes().

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index ac9eb273e28c..426f03b6e660 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -57,6 +57,10 @@ static void fsnotify_unmount_inodes(struct super_block *sb)
 		 * doing an __iget/iput with SB_ACTIVE clear would actually
 		 * evict all inodes with zero i_count from icache which is
 		 * unnecessarily violent and may in fact be illegal to do.
+		 *
+		 * However, we should have been called /after/ evict_inodes
+		 * removed all zero refcount inodes, in any case.  Test to
+		 * be sure.
 		 */
 		if (!atomic_read(&inode->i_count)) {
 			spin_unlock(&inode->i_lock);
diff --git a/fs/super.c b/fs/super.c
index cfadab2cbf35..cd352530eca9 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -448,10 +448,12 @@ void generic_shutdown_super(struct super_block *sb)
 		sync_filesystem(sb);
 		sb->s_flags &= ~SB_ACTIVE;
 
-		fsnotify_sb_delete(sb);
 		cgroup_writeback_umount();
 
+		/* evict all inodes with zero refcount */
 		evict_inodes(sb);
+		/* only nonzero refcount inodes can have marks */
+		fsnotify_sb_delete(sb);
 
 		if (sb->s_dio_done_wq) {
 			destroy_workqueue(sb->s_dio_done_wq);




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 0/2] avoid softlockups in various s_inodes iterators
  2019-11-07 19:42 [PATCH 0/2] avoid softlockups in various s_inodes iterators Eric Sandeen
  2019-11-07 19:47 ` [PATCH 1/2] fs: avoid softlockups in " Eric Sandeen
  2019-11-07 19:48 ` [PATCH 2/2] fs: call fsnotify_sb_delete after evict_inodes Eric Sandeen
@ 2019-11-07 19:58 ` Eric Sandeen
  2 siblings, 0 replies; 5+ messages in thread
From: Eric Sandeen @ 2019-11-07 19:58 UTC (permalink / raw)
  To: fsdevel; +Cc: Al Viro

On 11/7/19 1:42 PM, Eric Sandeen wrote:
> 2 patches to make sure we either schedule in an s_inodes walking
> loop, or do our best to limit the size of the walk, to avoid soft
> lockups.
> 
> -Eric
> 

Something mangled that sorry, let me resend it sanely...

-Eric


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/2] fs: call fsnotify_sb_delete after evict_inodes
  2019-11-07 20:52 [PATCH 0/2 V2] " Eric Sandeen
@ 2019-11-07 20:52 ` Eric Sandeen
  0 siblings, 0 replies; 5+ messages in thread
From: Eric Sandeen @ 2019-11-07 20:52 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: viro

When a filesystem is unmounted, we currently call fsnotify_sb_delete()
before evict_inodes(), which means that fsnotify_unmount_inodes()
must iterate over all inodes on the superblock, even though it will
only act on inodes with a refcount.  This is inefficient and can lead
to livelocks as it iterates over many unrefcounted inodes.

However, since fsnotify_sb_delete() and evict_inodes() are working
on orthogonal sets of inodes (fsnotify_sb_delete() only cares about
nonzero refcount, and evict_inodes() only cares about zero refcount),
we can swap the order of the calls.  The fsnotify call will then have
a much smaller list to walk (any refcounted inodes).

This should speed things up overall, and avoid livelocks in
fsnotify_unmount_inodes().

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/notify/fsnotify.c | 3 +++
 fs/super.c           | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index ac9eb27..f44e39c 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -57,6 +57,9 @@ static void fsnotify_unmount_inodes(struct super_block *sb)
 		 * doing an __iget/iput with SB_ACTIVE clear would actually
 		 * evict all inodes with zero i_count from icache which is
 		 * unnecessarily violent and may in fact be illegal to do.
+		 * However, we should have been called /after/ evict_inodes
+		 * removed all zero refcount inodes, in any case.  Test to
+		 * be sure.
 		 */
 		if (!atomic_read(&inode->i_count)) {
 			spin_unlock(&inode->i_lock);
diff --git a/fs/super.c b/fs/super.c
index cfadab2..cd35253 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -448,10 +448,12 @@ void generic_shutdown_super(struct super_block *sb)
 		sync_filesystem(sb);
 		sb->s_flags &= ~SB_ACTIVE;
 
-		fsnotify_sb_delete(sb);
 		cgroup_writeback_umount();
 
+		/* evict all inodes with zero refcount */
 		evict_inodes(sb);
+		/* only nonzero refcount inodes can have marks */
+		fsnotify_sb_delete(sb);
 
 		if (sb->s_dio_done_wq) {
 			destroy_workqueue(sb->s_dio_done_wq);
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-07 19:42 [PATCH 0/2] avoid softlockups in various s_inodes iterators Eric Sandeen
2019-11-07 19:47 ` [PATCH 1/2] fs: avoid softlockups in " Eric Sandeen
2019-11-07 19:48 ` [PATCH 2/2] fs: call fsnotify_sb_delete after evict_inodes Eric Sandeen
2019-11-07 19:58 ` [PATCH 0/2] avoid softlockups in various s_inodes iterators Eric Sandeen
2019-11-07 20:52 [PATCH 0/2 V2] " Eric Sandeen
2019-11-07 20:52 ` [PATCH 2/2] fs: call fsnotify_sb_delete after evict_inodes Eric Sandeen

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git