Message ID | 1573159954-27846-3-git-send-email-sandeen@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | avoid softlockups in various s_inodes iterators | expand |
On Thu, Nov 07, 2019 at 02:52:34PM -0600, Eric Sandeen wrote: > 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(). Umm... The critical part you've omitted here is that at this stage any final iput() done by fsnotify_sb_delete() (or anybody else, really) will forcibly evict the sucker there and then. So it's not as if any inodes were *added* to the evictable set by fsnotify_sb_delete() to be picked by evict_inodes() - any candidate is immediately disposed of. The crucial point is that SB_ACTIVE is already cleared by that stage - without that the patch would've been badly broken. That aside, both patches look sane. Could you update the commit message and resend the second one?
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);