Patchwork [RFC] fs: ext4: don't trap kswapd and allocating tasks on ext4 inode IO

login
register
mail settings
Submitter Johannes Weiner
Date May 15, 2017, 3:46 p.m.
Message ID <20170515154634.19733-1-hannes@cmpxchg.org>
Download mbox | patch
Permalink /patch/9727493/
State New
Headers show

Comments

Johannes Weiner - May 15, 2017, 3:46 p.m.
We have observed across several workloads situations where kswapd and
direct reclaimers get stuck in the inode shrinker of the ext4 / mount,
causing allocation latencies across tasks in the system, while there
are dozens of gigabytes of clean page cache covering multiple disks.

The stack traces of such an instance looks like this:

[<ffffffff812b3225>] jbd2_log_wait_commit+0x95/0x110
[<ffffffff812b4f29>] jbd2_complete_transaction+0x59/0x90
[<ffffffff812668da>] ext4_evict_inode+0x2da/0x480
[<ffffffff811f2230>] evict+0xc0/0x190
[<ffffffff811f2339>] dispose_list+0x39/0x50
[<ffffffff811f323b>] prune_icache_sb+0x4b/0x60
[<ffffffff811dba71>] super_cache_scan+0x141/0x190
[<ffffffff8116e755>] shrink_slab+0x235/0x440
[<ffffffff81172b48>] shrink_zone+0x268/0x2d0
[<ffffffff81172f04>] do_try_to_free_pages+0x164/0x410
[<ffffffff81173265>] try_to_free_pages+0xb5/0x160
[<ffffffff811656b6>] __alloc_pages_nodemask+0x636/0xb30
[<ffffffff811acac8>] alloc_pages_current+0x88/0x120
[<ffffffff816d4e46>] skb_page_frag_refill+0xc6/0xf0
[<ffffffff816d4e8d>] sk_page_frag_refill+0x1d/0x80
[<ffffffff8173f86b>] tcp_sendmsg+0x28b/0xb10
[<ffffffff81769727>] inet_sendmsg+0x67/0xa0
[<ffffffff816d0488>] sock_sendmsg+0x38/0x50
[<ffffffff816d0518>] sock_write_iter+0x78/0xd0
[<ffffffff811d774e>] do_iter_readv_writev+0x5e/0xa0
[<ffffffff811d8468>] do_readv_writev+0x178/0x210
[<ffffffff811d871c>] vfs_writev+0x3c/0x50
[<ffffffff811d8782>] do_writev+0x52/0xd0
[<ffffffff811d9810>] SyS_writev+0x10/0x20
[<ffffffff81002910>] do_syscall_64+0x50/0xa0
[<ffffffff817eed3c>] return_from_SYSCALL_64+0x0/0x6a
[<ffffffffffffffff>] 0xffffffffffffffff

The inode shrinker has provisions to skip any inodes that require
writeback, to avoid tarpitting the entire system behind a single
object when there are many other pools to recycle memory from. But
that logic doesn't cover the situation where an ext4 inode is clean
but journaled and tied to a commit that yet needs to hit the platter.

Add a superblock operation that lets the generic inode shrinker query
the filesystem whether evicting a given inode will require any IO; add
an ext4 implementation that checks whether the journal is caught up to
the commit id associated with the inode.

Fixes: 2d859db3e4a8 ("ext4: fix data corruption in inodes with journalled data")
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 fs/ext4/ext4.h       |  1 +
 fs/ext4/inode.c      | 17 +++++++++++++++++
 fs/ext4/super.c      |  1 +
 fs/inode.c           |  7 +++++++
 fs/jbd2/journal.c    |  9 ++++++++-
 include/linux/fs.h   |  1 +
 include/linux/jbd2.h |  1 +
 7 files changed, 36 insertions(+), 1 deletion(-)
Andreas Dilger - May 16, 2017, 1:34 a.m.
On May 15, 2017, at 5:46 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> 
> We have observed across several workloads situations where kswapd and
> direct reclaimers get stuck in the inode shrinker of the ext4 / mount,
> causing allocation latencies across tasks in the system, while there
> are dozens of gigabytes of clean page cache covering multiple disks.
> 
> The inode shrinker has provisions to skip any inodes that require
> writeback, to avoid tarpitting the entire system behind a single
> object when there are many other pools to recycle memory from. But
> that logic doesn't cover the situation where an ext4 inode is clean
> but journaled and tied to a commit that yet needs to hit the platter.
> 
> Add a superblock operation that lets the generic inode shrinker query
> the filesystem whether evicting a given inode will require any IO; add
> an ext4 implementation that checks whether the journal is caught up to
> the commit id associated with the inode.
[snip]

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 5834c4d76be8..4cb6cf932d9a 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -182,6 +182,23 @@ int ext4_truncate_restart_trans(handle_t *handle, struct inode *inode,
> 	return ret;
> }
> 
> +int ext4_evict_needs_io(struct inode *inode)
> +{
> +	if (inode->i_nlink &&
> +	    inode->i_ino != EXT4_JOURNAL_INO &&
> +	    ext4_should_journal_data(inode) &&
> +	    (S_ISLNK(inode->i_mode) || S_ISREG(inode->i_mode))) {
> +		journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
> +		tid_t commit_tid = EXT4_I(inode)->i_datasync_tid;
> +		int err;
> +
> +		err = __jbd2_complete_transaction(journal, commit_tid, false);

From a code style point of view, having a function exported with a name like
"__jbd2..." is probably bad.  An argument of "false" is meaningless at the
caller here.  It would be better to have a function name something like
jbd2_complete_transaction_nowait() exported, so that it is more clear what
this function does.

It would be even better to add a comment block for this function that
explains that the caller should use "jbd2_log_wait_commit(journal, tid)"
if it wants to wait for this transaction to complete commit.

Cheers, Andreas
Jan Kara - May 16, 2017, 2:36 p.m.
On Mon 15-05-17 11:46:34, Johannes Weiner wrote:
> We have observed across several workloads situations where kswapd and
> direct reclaimers get stuck in the inode shrinker of the ext4 / mount,
> causing allocation latencies across tasks in the system, while there
> are dozens of gigabytes of clean page cache covering multiple disks.
> 
> The stack traces of such an instance looks like this:
> 
> [<ffffffff812b3225>] jbd2_log_wait_commit+0x95/0x110
> [<ffffffff812b4f29>] jbd2_complete_transaction+0x59/0x90
> [<ffffffff812668da>] ext4_evict_inode+0x2da/0x480
> [<ffffffff811f2230>] evict+0xc0/0x190
> [<ffffffff811f2339>] dispose_list+0x39/0x50
> [<ffffffff811f323b>] prune_icache_sb+0x4b/0x60
> [<ffffffff811dba71>] super_cache_scan+0x141/0x190
> [<ffffffff8116e755>] shrink_slab+0x235/0x440
> [<ffffffff81172b48>] shrink_zone+0x268/0x2d0
> [<ffffffff81172f04>] do_try_to_free_pages+0x164/0x410
> [<ffffffff81173265>] try_to_free_pages+0xb5/0x160
> [<ffffffff811656b6>] __alloc_pages_nodemask+0x636/0xb30
> [<ffffffff811acac8>] alloc_pages_current+0x88/0x120
> [<ffffffff816d4e46>] skb_page_frag_refill+0xc6/0xf0
> [<ffffffff816d4e8d>] sk_page_frag_refill+0x1d/0x80
> [<ffffffff8173f86b>] tcp_sendmsg+0x28b/0xb10
> [<ffffffff81769727>] inet_sendmsg+0x67/0xa0
> [<ffffffff816d0488>] sock_sendmsg+0x38/0x50
> [<ffffffff816d0518>] sock_write_iter+0x78/0xd0
> [<ffffffff811d774e>] do_iter_readv_writev+0x5e/0xa0
> [<ffffffff811d8468>] do_readv_writev+0x178/0x210
> [<ffffffff811d871c>] vfs_writev+0x3c/0x50
> [<ffffffff811d8782>] do_writev+0x52/0xd0
> [<ffffffff811d9810>] SyS_writev+0x10/0x20
> [<ffffffff81002910>] do_syscall_64+0x50/0xa0
> [<ffffffff817eed3c>] return_from_SYSCALL_64+0x0/0x6a
> [<ffffffffffffffff>] 0xffffffffffffffff
> 
> The inode shrinker has provisions to skip any inodes that require
> writeback, to avoid tarpitting the entire system behind a single
> object when there are many other pools to recycle memory from. But
> that logic doesn't cover the situation where an ext4 inode is clean
> but journaled and tied to a commit that yet needs to hit the platter.
> 
> Add a superblock operation that lets the generic inode shrinker query
> the filesystem whether evicting a given inode will require any IO; add
> an ext4 implementation that checks whether the journal is caught up to
> the commit id associated with the inode.
> 
> Fixes: 2d859db3e4a8 ("ext4: fix data corruption in inodes with journalled data")
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

OK. I have to say I'm somewhat surprised you use data journalling on some
of your files / filesystems but whatever - maybe these are long symlink
after all which would make sense. And I'm actually doubly surprised you can
see these stack traces as these days inode_lru_isolate() checks
inode->i_data.nrpages and uncommitted pages cannot be evicted from
pagecache (ext4_releasepage() will refuse to free them) so I don't see how
such inode can get to dispose_list(). But maybe the inode doesn't really
have any pages and i_datasync_tid just happens to be set to the current
transaction because it is initialized that way and we are evicting inode
that was recently read from disk.

Anyway if you add: "&& inode->i_data.nrpages" to the test in
ext4_evict_inode() do the stalls go away?

								Honza
Johannes Weiner - May 16, 2017, 3:41 p.m.
On Tue, May 16, 2017 at 04:36:45PM +0200, Jan Kara wrote:
> On Mon 15-05-17 11:46:34, Johannes Weiner wrote:
> > We have observed across several workloads situations where kswapd and
> > direct reclaimers get stuck in the inode shrinker of the ext4 / mount,
> > causing allocation latencies across tasks in the system, while there
> > are dozens of gigabytes of clean page cache covering multiple disks.
> > 
> > The stack traces of such an instance looks like this:
> > 
> > [<ffffffff812b3225>] jbd2_log_wait_commit+0x95/0x110
> > [<ffffffff812b4f29>] jbd2_complete_transaction+0x59/0x90
> > [<ffffffff812668da>] ext4_evict_inode+0x2da/0x480
> > [<ffffffff811f2230>] evict+0xc0/0x190
> > [<ffffffff811f2339>] dispose_list+0x39/0x50
> > [<ffffffff811f323b>] prune_icache_sb+0x4b/0x60
> > [<ffffffff811dba71>] super_cache_scan+0x141/0x190
> > [<ffffffff8116e755>] shrink_slab+0x235/0x440
> > [<ffffffff81172b48>] shrink_zone+0x268/0x2d0
> > [<ffffffff81172f04>] do_try_to_free_pages+0x164/0x410
> > [<ffffffff81173265>] try_to_free_pages+0xb5/0x160
> > [<ffffffff811656b6>] __alloc_pages_nodemask+0x636/0xb30
> > [<ffffffff811acac8>] alloc_pages_current+0x88/0x120
> > [<ffffffff816d4e46>] skb_page_frag_refill+0xc6/0xf0
> > [<ffffffff816d4e8d>] sk_page_frag_refill+0x1d/0x80
> > [<ffffffff8173f86b>] tcp_sendmsg+0x28b/0xb10
> > [<ffffffff81769727>] inet_sendmsg+0x67/0xa0
> > [<ffffffff816d0488>] sock_sendmsg+0x38/0x50
> > [<ffffffff816d0518>] sock_write_iter+0x78/0xd0
> > [<ffffffff811d774e>] do_iter_readv_writev+0x5e/0xa0
> > [<ffffffff811d8468>] do_readv_writev+0x178/0x210
> > [<ffffffff811d871c>] vfs_writev+0x3c/0x50
> > [<ffffffff811d8782>] do_writev+0x52/0xd0
> > [<ffffffff811d9810>] SyS_writev+0x10/0x20
> > [<ffffffff81002910>] do_syscall_64+0x50/0xa0
> > [<ffffffff817eed3c>] return_from_SYSCALL_64+0x0/0x6a
> > [<ffffffffffffffff>] 0xffffffffffffffff
> > 
> > The inode shrinker has provisions to skip any inodes that require
> > writeback, to avoid tarpitting the entire system behind a single
> > object when there are many other pools to recycle memory from. But
> > that logic doesn't cover the situation where an ext4 inode is clean
> > but journaled and tied to a commit that yet needs to hit the platter.
> > 
> > Add a superblock operation that lets the generic inode shrinker query
> > the filesystem whether evicting a given inode will require any IO; add
> > an ext4 implementation that checks whether the journal is caught up to
> > the commit id associated with the inode.
> > 
> > Fixes: 2d859db3e4a8 ("ext4: fix data corruption in inodes with journalled data")
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> OK. I have to say I'm somewhat surprised you use data journalling on some
> of your files / filesystems but whatever - maybe these are long symlink
> after all which would make sense.

The filesystem is actually mounted data=ordered and we didn't catch
anyone in userspace enabling journaling on individual inodes. So we
assumed this must be from symlinks.

> And I'm actually doubly surprised you can see these stack traces as
> these days inode_lru_isolate() checks inode->i_data.nrpages and
> uncommitted pages cannot be evicted from pagecache
> (ext4_releasepage() will refuse to free them) so I don't see how
> such inode can get to dispose_list(). But maybe the inode doesn't
> really have any pages and i_datasync_tid just happens to be set to
> the current transaction because it is initialized that way and we
> are evicting inode that was recently read from disk.

Hm, we're running 4.6, but that already has the nrpages check in
inode_lru_isolate(). There couldn't be any pages in those inodes by
the time the shrinker gets to them.

> Anyway if you add: "&& inode->i_data.nrpages" to the test in
> ext4_evict_inode() do the stalls go away?

Want me to still test this?

Patch

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 8e8046104f4d..b552978fc31f 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2463,6 +2463,7 @@  extern struct inode *ext4_iget_normal(struct super_block *, unsigned long);
 extern int  ext4_write_inode(struct inode *, struct writeback_control *);
 extern int  ext4_setattr(struct dentry *, struct iattr *);
 extern int  ext4_getattr(const struct path *, struct kstat *, u32, unsigned int);
+extern int ext4_evict_needs_io(struct inode *);
 extern void ext4_evict_inode(struct inode *);
 extern void ext4_clear_inode(struct inode *);
 extern int  ext4_file_getattr(const struct path *, struct kstat *, u32, unsigned int);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 5834c4d76be8..4cb6cf932d9a 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -182,6 +182,23 @@  int ext4_truncate_restart_trans(handle_t *handle, struct inode *inode,
 	return ret;
 }
 
+int ext4_evict_needs_io(struct inode *inode)
+{
+	if (inode->i_nlink &&
+	    inode->i_ino != EXT4_JOURNAL_INO &&
+	    ext4_should_journal_data(inode) &&
+	    (S_ISLNK(inode->i_mode) || S_ISREG(inode->i_mode))) {
+		journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
+		tid_t commit_tid = EXT4_I(inode)->i_datasync_tid;
+		int err;
+
+		err = __jbd2_complete_transaction(journal, commit_tid, false);
+		if (err == -EAGAIN)
+			return 1;
+	}
+	return 0;
+}
+
 /*
  * Called at the last iput() if i_nlink is zero.
  */
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c90edf09b0c3..e0e7cff589eb 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1287,6 +1287,7 @@  static const struct super_operations ext4_sops = {
 	.write_inode	= ext4_write_inode,
 	.dirty_inode	= ext4_dirty_inode,
 	.drop_inode	= ext4_drop_inode,
+	.evict_needs_io	= ext4_evict_needs_io,
 	.evict_inode	= ext4_evict_inode,
 	.put_super	= ext4_put_super,
 	.sync_fs	= ext4_sync_fs,
diff --git a/fs/inode.c b/fs/inode.c
index db5914783a71..730d2034b8a1 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -703,6 +703,7 @@  static enum lru_status inode_lru_isolate(struct list_head *item,
 {
 	struct list_head *freeable = arg;
 	struct inode	*inode = container_of(item, struct inode, i_lru);
+	const struct super_operations *op = inode->i_sb->s_op;
 
 	/*
 	 * we are inverting the lru lock/inode->i_lock here, so use a trylock.
@@ -723,6 +724,12 @@  static enum lru_status inode_lru_isolate(struct list_head *item,
 		return LRU_REMOVED;
 	}
 
+	/* If eviction may block on IO, defer for now */
+	if (op->evict_needs_io && !op->evict_needs_io(inode)) {
+		spin_unlock(&inode->i_lock);
+		return LRU_ROTATE;
+	}
+
 	/* recently referenced inodes get one more pass */
 	if (inode->i_state & I_REFERENCED) {
 		inode->i_state &= ~I_REFERENCED;
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 5a0245e36240..e353f3a0abf5 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -745,7 +745,7 @@  int jbd2_log_wait_commit(journal_t *journal, tid_t tid)
  * the transaction id is stale, it is by definition already completed,
  * so just return SUCCESS.
  */
-int jbd2_complete_transaction(journal_t *journal, tid_t tid)
+int __jbd2_complete_transaction(journal_t *journal, tid_t tid, bool wait)
 {
 	int	need_to_wait = 1;
 
@@ -765,8 +765,15 @@  int jbd2_complete_transaction(journal_t *journal, tid_t tid)
 	if (!need_to_wait)
 		return 0;
 wait_commit:
+	if (!wait)
+		return -EAGAIN;
 	return jbd2_log_wait_commit(journal, tid);
 }
+
+int jbd2_complete_transaction(journal_t *journal, tid_t tid)
+{
+	return __jbd2_complete_transaction(journal, tid, true);
+}
 EXPORT_SYMBOL(jbd2_complete_transaction);
 
 /*
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 26488b419965..3a895f31afa7 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1777,6 +1777,7 @@  struct super_operations {
    	void (*dirty_inode) (struct inode *, int flags);
 	int (*write_inode) (struct inode *, struct writeback_control *wbc);
 	int (*drop_inode) (struct inode *);
+	int (*evict_needs_io) (struct inode *);
 	void (*evict_inode) (struct inode *);
 	void (*put_super) (struct super_block *);
 	int (*sync_fs)(struct super_block *sb, int wait);
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 606b6bce3a5b..3b902bed6e8b 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1367,6 +1367,7 @@  int jbd2_log_start_commit(journal_t *journal, tid_t tid);
 int __jbd2_log_start_commit(journal_t *journal, tid_t tid);
 int jbd2_journal_start_commit(journal_t *journal, tid_t *tid);
 int jbd2_log_wait_commit(journal_t *journal, tid_t tid);
+int __jbd2_complete_transaction(journal_t *journal, tid_t tid, bool wait);
 int jbd2_complete_transaction(journal_t *journal, tid_t tid);
 int jbd2_log_do_checkpoint(journal_t *journal);
 int jbd2_trans_will_send_data_barrier(journal_t *journal, tid_t tid);