[PATCH] jbd2: Fix dbench4 performance regression for 'nobarrier' mounts

[PATCH] jbd2: Fix dbench4 performance regression for 'nobarrier' mounts

[Jan Kara]

Commit b685d3d65ac7 "block: treat REQ_FUA and REQ_PREFLUSH as
synchronous" removed REQ_SYNC flag from WRITE_FUA implementation. Since
JBD2 strips REQ_FUA and REQ_FLUSH flags from submitted IO when the
filesystem is mounted with nobarrier mount option, journal superblock
writes ended up being async writes after this patch and that caused
heavy performance regression for dbench4 benchmark with high number of
processes. In my test setup with HP RAID array with non-volatile write
cache and 32 GB ram, dbench4 runs with 8 processes regressed by ~25%.

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.

Fixes: b685d3d65ac791406e0dfd8779cc9b3707fea5a3
CC: stable@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/journal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 5adc2fb62b0f..e768126f6a72 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1348,7 +1348,7 @@ static int jbd2_write_superblock(journal_t *journal, int write_flags)
 	jbd2_superblock_csum_set(journal, sb);
 	get_bh(bh);
 	bh->b_end_io = end_buffer_write_sync;
-	ret = submit_bh(REQ_OP_WRITE, write_flags, bh);
+	ret = submit_bh(REQ_OP_WRITE, write_flags | REQ_SYNC, bh);
 	wait_on_buffer(bh);
 	if (buffer_write_io_error(bh)) {
 		clear_buffer_write_io_error(bh);
-- 
2.12.0

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

[Christoph Hellwig]

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?

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

But I guess we'll need a quick fix first, for that:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Theodore Ts'o]

On Fri, Apr 28, 2017 at 11:59:34AM +0200, Jan Kara wrote:
> Commit b685d3d65ac7 "block: treat REQ_FUA and REQ_PREFLUSH as
> synchronous" removed REQ_SYNC flag from WRITE_FUA implementation. Since
> JBD2 strips REQ_FUA and REQ_FLUSH flags from submitted IO when the
> filesystem is mounted with nobarrier mount option, journal superblock
> writes ended up being async writes after this patch and that caused
> heavy performance regression for dbench4 benchmark with high number of
> processes. In my test setup with HP RAID array with non-volatile write
> cache and 32 GB ram, dbench4 runs with 8 processes regressed by ~25%.
> 
> 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.
> 
> Fixes: b685d3d65ac791406e0dfd8779cc9b3707fea5a3
> CC: stable@vger.kernel.org
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

						- Ted

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

[Jan Kara]

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@lst.de>

Thanks!

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