linux-ext4 - Re: [PATCH] jbd2: Fix dbench4 performance regression for 'nobarrier' mounts

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20170502093654.GC13916@quack2.suse.cz>
Date:   Tue, 2 May 2017 11:36:54 +0200
From:   Jan Kara <jack@...e.cz>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     Jan Kara <jack@...e.cz>, Ted Tso <tytso@....edu>,
        linux-ext4@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] jbd2: Fix dbench4 performance regression for 'nobarrier'
 mounts

On Fri 28-04-17 08:03:24, Christoph Hellwig wrote:
> On Fri, Apr 28, 2017 at 11:59:34AM +0200, Jan Kara wrote:
> > Fix the problem by making sure journal superblock writes are always
> > treated as synchronous since they generally block progress of the
> > journalling machinery and thus the whole filesystem.
> 
> The callchains leading down to jbd2_write_superblock looks a little
> suspicious to me.  It seems like jbd2_journal_commit_transaction
> will actually call without FUA in the JBD2_FLUSHED case. Is that
> really intentional, and if yes should it be documented?

I guess you mean this:

                /*
                 * We hold j_checkpoint_mutex so tail cannot change under us.
                 * We don't need any special data guarantees for writing sb
                 * since journal is empty and it is ok for write to be
                 * flushed only with transaction commit.
                 */
                jbd2_journal_update_sb_log_tail(journal,
                                                journal->j_tail_sequence,
                                                journal->j_tail,
                                                REQ_SYNC);

And yes, omitting REQ_FUA is intentional and the comment mentions it as "We
don't need any special data guarantees...". Maybe I could add there an
explicit mentioning of REQ_FUA and REQ_PREFLUSH so that it is clearer what
we are talking about.

> Except for that it would seem more useful to move to a "bool preflush"
> argument passed down.

Well, we can call jbd2_write_superblock() with REQ_FUA, REQ_PREFLUSH |
REQ_FUA, REQ_SYNC. So one bool argument won't be enough. However I do agree
that it would be cleaner to pass REQ_SYNC directly from all the places
which set some flags which are eventually passed down to
jbd2_write_superblock(). I'll create a cleanup patch for that.

> But I guess we'll need a quick fix first, for that:
> 
> Reviewed-by: Christoph Hellwig <hch@....de>

Thanks!

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