linux-ext4 - Re: [RFC PATCH] ext4: don't trap kswapd and allocating tasks on ext4 inode IO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <92681C1D-9812-4868-9A26-40CF0644BAB4@dilger.ca>
Date:   Tue, 16 May 2017 03:34:27 +0200
From:   Andreas Dilger <adilger@...ger.ca>
To:     Johannes Weiner <hannes@...xchg.org>
Cc:     Jan Kara <jack@...e.com>, Theodore Ts'o <tytso@....edu>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-team@...com
Subject: Re: [RFC PATCH] ext4: don't trap kswapd and allocating tasks on ext4
 inode IO

On May 15, 2017, at 5:46 PM, Johannes Weiner <hannes@...xchg.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






Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)