kernel building regression on 64 LCPUs machine

performance bug Alex,Shi 7 participants 38 messages Jan 19, 2011 – Mar 28, 2011

Subsystems (3)

ext4 file system OCFS2 Journalling layer for block devices (JBD2)

lore

Share & tools

lore

Alex,Shi

author

2011-01-19 01:56 UTC #0 lore raw

Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.

iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.

we didn't test deadline IO mode, just test cfq. seems insert write request into sync queue effect much read performance, but we don't know details. What's your comments of this?

iostat of .37 kernel:
rrqm/s wrqm/s r/s w/s rMB/s wMB/s avgrq-sz avgqu-sz await svctm %util
22.5772 96.46 92.3742 14.747 1.0048 0.439474 34.8557 0.18078 3.8076 0.30447 2.94302 iostat of commit reverted .37:
26.6223 80.21 107.875 6.03538 1.51415 0 41.3275 0.153385 3.80569 0.377231 3.22323

vmstat report show, read bandwidth dropping:
vmstat of .37:
r b swpd free buff cache si so bi bo in cs us sy id wa st
3.4 52.6 0.0 64303937.0 16466.7 121544.5 0.0 0.0 2102.7 1914.6 7414.1 3185.7 2.0 1.0 80.3 16.7 0.0 vmstat of revert all from .37
2.2 35.8 0.0 64306767.4 17265.6 126101.2 0.0 0.0 2415.8 1619.1 8532.2 3556.2 2.5 1.1 83.0 13.3 0.0

Regards
Alex

===

fs/jbd/commit.c +1 -1

index 34a4861..27ac2f3 100644
@@ -294,7 +294,7 @@ void journal_commit_transaction(journal_t *journal)
 	int first_tag = 0;
 	int tag_flag;
 	int i;
-	int write_op = WRITE_SYNC;
+	int write_op = WRITE;
 
 	/*
 	 * First job: lock down the current transaction and wait for

fs/jbd2/commit.c +1 -1

index f3ad159..69ff08e 100644
@@ -329,7 +329,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	int tag_bytes = journal_tag_bytes(journal);
 	struct buffer_head *cbh = NULL; /* For transactional checksums */
 	__u32 crc32_sum = ~0;
-	int write_op = WRITE_SYNC;
+	int write_op = WRITE;
 
 	/*
 	 * First job: lock down the current transaction and wait for

Shaohua Li

2011-01-19 02:03 UTC #1 lore raw

add Jan and Theodore to the loop.

Quoted 11 lines · Alex

Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.

since WRITE_SYNC_PLUG doesn't work, this isn't a simple no-write-merge issue.

Quoted 49 lines

we didn't test deadline IO mode, just test cfq. seems insert write request into sync queue effect much read performance, but we don't know details. What's your comments of this?
iostat of .37 kernel:
rrqm/s wrqm/s r/s w/s rMB/s wMB/s avgrq-sz avgqu-sz await svctm %util
22.5772 96.46 92.3742 14.747 1.0048 0.439474 34.8557 0.18078 3.8076 0.30447 2.94302 iostat of commit reverted .37:
26.6223 80.21 107.875 6.03538 1.51415 0 41.3275 0.153385 3.80569 0.377231 3.22323
vmstat report show, read bandwidth dropping:
vmstat of .37:
r b swpd free buff cache si so bi bo in cs us sy id wa st
3.4 52.6 0.0 64303937.0 16466.7 121544.5 0.0 0.0 2102.7 1914.6 7414.1 3185.7 2.0 1.0 80.3 16.7 0.0 vmstat of revert all from .37
2.2 35.8 0.0 64306767.4 17265.6 126101.2 0.0 0.0 2415.8 1619.1 8532.2 3556.2 2.5 1.1 83.0 13.3 0.0
Regards
Alex
===
fs/jbd/commit.c +1 -1
index 34a4861..27ac2f3 100644
@@ -294,7 +294,7 @@ void journal_commit_transaction(journal_t *journal)
 	int first_tag = 0;
 	int tag_flag;
 	int i;
-	int write_op = WRITE_SYNC;
+	int write_op = WRITE;
 
 	/*
 	 * First job: lock down the current transaction and wait for
fs/jbd2/commit.c +1 -1
index f3ad159..69ff08e 100644
@@ -329,7 +329,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	int tag_bytes = journal_tag_bytes(journal);
 	struct buffer_head *cbh = NULL; /* For transactional checksums */
 	__u32 crc32_sum = ~0;
-	int write_op = WRITE_SYNC;
+	int write_op = WRITE;
 
 	/*
 	 * First job: lock down the current transaction and wait for

Jan Kara

reviewer

2011-01-19 12:56 UTC #2 lore raw

Quoted 1 line · Shaohua Li

add Jan and Theodore to the loop.

Thanks.

Quoted 8 lines · Alex

Quoted 7 lines
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.

Quoted 9 lines

Quoted 4 lines
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
since WRITE_SYNC_PLUG doesn't work, this isn't a simple no-write-merge issue.
Quoted 3 lines
we didn't test deadline IO mode, just test cfq. seems insert write request into sync queue effect much read performance, but we don't know details. What's your comments of this?

Indeed it seems that the optimization of the case where we wait for the
transaction is negatively impacting the performance when we are not. Does patch below help for your load? It refines the logic when WRITE_SYNC is needed (of course, we should also test whether the patch works for fsync heavy loads as well).
The patch is mostly a proof of concept and only lightly tested so be
careful...

Honza

Signature · 3 lines

Jan Kara <[email protected]>
SUSE Labs, CR
---

Quoted 1 line

From 6060f56ac919ac6b146057787af219bbf2503ea0 Mon Sep 17 00:00:00 2001

From: Jan Kara <[email protected]>
Date: Wed, 19 Jan 2011 13:45:04 +0100
Subject: [PATCH] jbd2: Refine commit writeout logic

Currently we write out all journal buffers in WRITE_SYNC mode. This improves performance for fsync heavy workloads but hinders performance when writes are mostly asynchronous. So add possibility for callers starting a transaction commit to specify whether they are going to wait for the commit and submit journal writes in WRITE_SYNC mode only in that case.

Signed-off-by Jan Kara <[email protected]>

---
 fs/ext4/fsync.c       |    2 +-
 fs/ext4/super.c       |    2 +-
 fs/jbd2/checkpoint.c  |    2 +-
 fs/jbd2/commit.c      |    4 ++--
 fs/jbd2/journal.c     |   19 ++++++++++---------
 fs/jbd2/transaction.c |   13 ++++++-------
 fs/ocfs2/aops.c       |    2 +-
 fs/ocfs2/super.c      |    2 +-
 include/linux/jbd2.h  |   18 ++++++++++--------
 9 files changed, 33 insertions(+), 31 deletions(-)

fs/ext4/fsync.c +1 -1

index 7829b28..19434da 100644
@@ -198,7 +198,7 @@ int ext4_sync_file(struct file *file, int datasync)
 		return ext4_force_commit(inode->i_sb);
 
 	commit_tid = datasync ? ei->i_datasync_tid : ei->i_sync_tid;
-	if (jbd2_log_start_commit(journal, commit_tid)) {
+	if (jbd2_log_start_commit(journal, commit_tid, true)) {
 		/*
 		 * When the journal is on a different device than the
 		 * fs data disk, we need to issue the barrier in

fs/ext4/super.c +1 -1

index 0f10ccd..eee8486 100644
@@ -4121,7 +4121,7 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
 
 	trace_ext4_sync_fs(sb, wait);
 	flush_workqueue(sbi->dio_unwritten_wq);
-	if (jbd2_journal_start_commit(sbi->s_journal, &target)) {
+	if (jbd2_journal_start_commit(sbi->s_journal, &target, true)) {
 		if (wait)
 			jbd2_log_wait_commit(sbi->s_journal, target);
 	}

fs/jbd2/checkpoint.c +1 -1

index 6a79fd0..3436d53 100644
@@ -309,7 +309,7 @@ static int __process_buffer(journal_t *journal, struct journal_head *jh,
 			       "Waiting for Godot: block %llu\n",
 			       journal->j_devname,
 			       (unsigned long long) bh->b_blocknr);
-		jbd2_log_start_commit(journal, tid);
+		jbd2_log_start_commit(journal, tid, true);
 		jbd2_log_wait_commit(journal, tid);
 		ret = 1;
 	} else if (!buffer_dirty(bh)) {

fs/jbd2/commit.c +2 -2

index f3ad159..19973eb 100644
@@ -329,7 +329,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	int tag_bytes = journal_tag_bytes(journal);
 	struct buffer_head *cbh = NULL; /* For transactional checksums */
 	__u32 crc32_sum = ~0;
-	int write_op = WRITE_SYNC;
+	int write_op = WRITE;
 
 	/*
 	 * First job: lock down the current transaction and wait for

@@ -368,7 +368,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	 * we unplug the device. We don't do explicit unplugging in here,
 	 * instead we rely on sync_buffer() doing the unplug for us.
 	 */
-	if (commit_transaction->t_synchronous_commit)
+	if (tid_geq(journal->j_commit_waited, commit_transaction->t_tid))
 		write_op = WRITE_SYNC_PLUG;
 	trace_jbd2_commit_locking(journal, commit_transaction);
 	stats.run.rs_wait = commit_transaction->t_max_wait;

fs/jbd2/journal.c +10 -9

index 9e46869..e278fa2 100644
@@ -475,7 +475,7 @@ int __jbd2_log_space_left(journal_t *journal)
 /*
  * Called under j_state_lock.  Returns true if a transaction commit was started.
  */
-int __jbd2_log_start_commit(journal_t *journal, tid_t target)
+int __jbd2_log_start_commit(journal_t *journal, tid_t target, bool will_wait)
 {
 	/*
 	 * Are we already doing a recent enough commit?

@@ -485,7 +485,8 @@ int __jbd2_log_start_commit(journal_t *journal, tid_t target)
 		 * We want a new commit: OK, mark the request and wakeup the
 		 * commit thread.  We do _not_ do the commit ourselves.
 		 */
-
+		if (will_wait && !tid_geq(journal->j_commit_waited, target))
+			journal->j_commit_waited = target;
 		journal->j_commit_request = target;
 		jbd_debug(1, "JBD: requesting commit %d/%d\n",
 			  journal->j_commit_request,

@@ -496,12 +497,12 @@ int __jbd2_log_start_commit(journal_t *journal, tid_t target)
 	return 0;
 }
 
-int jbd2_log_start_commit(journal_t *journal, tid_t tid)
+int jbd2_log_start_commit(journal_t *journal, tid_t tid, bool will_wait)
 {
 	int ret;
 
 	write_lock(&journal->j_state_lock);
-	ret = __jbd2_log_start_commit(journal, tid);
+	ret = __jbd2_log_start_commit(journal, tid, will_wait);
 	write_unlock(&journal->j_state_lock);
 	return ret;
 }

@@ -524,7 +525,7 @@ int jbd2_journal_force_commit_nested(journal_t *journal)
 	read_lock(&journal->j_state_lock);
 	if (journal->j_running_transaction && !current->journal_info) {
 		transaction = journal->j_running_transaction;
-		__jbd2_log_start_commit(journal, transaction->t_tid);
+		__jbd2_log_start_commit(journal, transaction->t_tid, true);
 	} else if (journal->j_committing_transaction)
 		transaction = journal->j_committing_transaction;

@@ -544,7 +545,7 @@ int jbd2_journal_force_commit_nested(journal_t *journal)
  * if a transaction is going to be committed (or is currently already
  * committing), and fills its tid in at *ptid
  */
-int jbd2_journal_start_commit(journal_t *journal, tid_t *ptid)
+int jbd2_journal_start_commit(journal_t *journal, tid_t *ptid, bool will_wait)
 {
 	int ret = 0;

@@ -552,7 +553,7 @@ int jbd2_journal_start_commit(journal_t *journal, tid_t *ptid)
 	if (journal->j_running_transaction) {
 		tid_t tid = journal->j_running_transaction->t_tid;
 
-		__jbd2_log_start_commit(journal, tid);
+		__jbd2_log_start_commit(journal, tid, will_wait);
 		/* There's a running transaction and we've just made sure
 		 * it's commit has been scheduled. */
 		if (ptid)

@@ -1559,7 +1560,7 @@ int jbd2_journal_flush(journal_t *journal)
 	/* Force everything buffered to the log... */
 	if (journal->j_running_transaction) {
 		transaction = journal->j_running_transaction;
-		__jbd2_log_start_commit(journal, transaction->t_tid);
+		__jbd2_log_start_commit(journal, transaction->t_tid, true);
 	} else if (journal->j_committing_transaction)
 		transaction = journal->j_committing_transaction;

@@ -1675,7 +1676,7 @@ void __jbd2_journal_abort_hard(journal_t *journal)
 	journal->j_flags |= JBD2_ABORT;
 	transaction = journal->j_running_transaction;
 	if (transaction)
-		__jbd2_log_start_commit(journal, transaction->t_tid);
+		__jbd2_log_start_commit(journal, transaction->t_tid, false);
 	write_unlock(&journal->j_state_lock);
 }

fs/jbd2/transaction.c +6 -7

index 3948932..3d5215e 100644
@@ -222,7 +222,7 @@ repeat:
 		atomic_sub(nblocks, &transaction->t_outstanding_credits);
 		prepare_to_wait(&journal->j_wait_transaction_locked, &wait,
 				TASK_UNINTERRUPTIBLE);
-		__jbd2_log_start_commit(journal, transaction->t_tid);
+		__jbd2_log_start_commit(journal, transaction->t_tid, false);
 		read_unlock(&journal->j_state_lock);
 		schedule();
 		finish_wait(&journal->j_wait_transaction_locked, &wait);

@@ -465,7 +465,7 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, int gfp_mask)
 	spin_unlock(&transaction->t_handle_lock);
 
 	jbd_debug(2, "restarting handle %p\n", handle);
-	__jbd2_log_start_commit(journal, transaction->t_tid);
+	__jbd2_log_start_commit(journal, transaction->t_tid, false);
 	read_unlock(&journal->j_state_lock);
 
 	lock_map_release(&handle->h_lockdep_map);

@@ -1361,8 +1361,6 @@ int jbd2_journal_stop(handle_t *handle)
 		}
 	}
 
-	if (handle->h_sync)
-		transaction->t_synchronous_commit = 1;
 	current->journal_info = NULL;
 	atomic_sub(handle->h_buffer_credits,
 		   &transaction->t_outstanding_credits);

@@ -1383,15 +1381,16 @@ int jbd2_journal_stop(handle_t *handle)
 
 		jbd_debug(2, "transaction too old, requesting commit for "
 					"handle %p\n", handle);
-		/* This is non-blocking */
-		jbd2_log_start_commit(journal, transaction->t_tid);
-
 		/*
 		 * Special case: JBD2_SYNC synchronous updates require us
 		 * to wait for the commit to complete.
 		 */
 		if (handle->h_sync && !(current->flags & PF_MEMALLOC))
 			wait_for_commit = 1;
+
+		/* This is non-blocking */
+		jbd2_log_start_commit(journal, transaction->t_tid,
+				      wait_for_commit);
 	}
 
 	/*

fs/ocfs2/aops.c +1 -1

index 1fbb0e2..d493f32 100644
@@ -1659,7 +1659,7 @@ static int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
 		goto out;
 	}
 
-	if (jbd2_journal_start_commit(osb->journal->j_journal, &target)) {
+	if (jbd2_journal_start_commit(osb->journal->j_journal, &target, true)) {
 		jbd2_log_wait_commit(osb->journal->j_journal, target);
 		ret = 1;
 	}

fs/ocfs2/super.c +1 -1

index 31c3ffd..aab448c 100644
@@ -414,7 +414,7 @@ static int ocfs2_sync_fs(struct super_block *sb, int wait)
 	}
 
 	if (jbd2_journal_start_commit(OCFS2_SB(sb)->journal->j_journal,
-				      &target)) {
+				      &target, wait)) {
 		if (wait)
 			jbd2_log_wait_commit(OCFS2_SB(sb)->journal->j_journal,
 					     target);

include/linux/jbd2.h +10 -8

index 27e79c2..46aaf45 100644
@@ -631,11 +631,6 @@ struct transaction_s
 	 */
 	atomic_t		t_handle_count;
 
-	/*
-	 * This transaction is being forced and some process is
-	 * waiting for it to finish.
-	 */
-	unsigned int t_synchronous_commit:1;
 	unsigned int t_flushed_data_blocks:1;
 
 	/*

@@ -900,6 +895,13 @@ struct journal_s
 	tid_t			j_commit_request;
 
 	/*
+	 * Sequence number of the most recent transaction someone is waiting
+	 * for to commit.
+	 * [j_state_lock]
+	 */
+	tid_t			j_commit_waited;
+
+	/*
 	 * Journal uuid: identifies the object (filesystem, LVM volume etc)
 	 * backed by this journal.  This will eventually be replaced by an array
 	 * of uuids, allowing us to index multiple devices within a single

@@ -1200,9 +1202,9 @@ extern void	jbd2_journal_switch_revoke_table(journal_t *journal);
  */
 
 int __jbd2_log_space_left(journal_t *); /* Called with journal locked */
-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_start_commit(journal_t *journal, tid_t tid, bool will_wait);
+int __jbd2_log_start_commit(journal_t *journal, tid_t tid, bool will_wait);
+int jbd2_journal_start_commit(journal_t *journal, tid_t *tid, bool will_wait);
 int jbd2_journal_force_commit_nested(journal_t *journal);
 int jbd2_log_wait_commit(journal_t *journal, tid_t tid);
 int jbd2_log_do_checkpoint(journal_t *journal);

Signature · 1 line

1.7.1

Alex,Shi

author

2011-01-20 07:53 UTC #3 lore raw

Quoted 21 lines · Jan Kara

Quoted 1 line · Shaohua Li
add Jan and Theodore to the loop.
Thanks.
Quoted 8 lines · Alex
Quoted 7 lines
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.
Quoted 9 lines
Quoted 4 lines
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
since WRITE_SYNC_PLUG doesn't work, this isn't a simple no-write-merge issue.
Quoted 3 lines
we didn't test deadline IO mode, just test cfq. seems insert write request into sync queue effect much read performance, but we don't know details. What's your comments of this?

Quoted 8 lines

Indeed it seems that the optimization of the case where we wait for the
transaction is negatively impacting the performance when we are not. Does patch below help for your load? It refines the logic when WRITE_SYNC is needed (of course, we should also test whether the patch works for fsync heavy loads as well).
The patch is mostly a proof of concept and only lightly tested so be
careful...

I tested the patch after remove t_synchronous_commit lines in include/trace/events/jbd2.h on 2.6.38-rc1 kernel. but did not find clear improvement on kbuild.

Vivek Goyal

2011-01-20 15:17 UTC #4 lore raw

In reply to Shaohua Li

Quoted 10 lines · Shaohua Li

add Jan and Theodore to the loop.
Quoted 7 lines · Alex
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.

Quoted 6 lines

Quoted 4 lines
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
since WRITE_SYNC_PLUG doesn't work, this isn't a simple no-write-merge issue.

Yep, it does sound like reduce write merging. But moving journal commits back to WRITE, then fsync performance will drop as there will be idling introduced between fsync thread and journalling thread. So that does not sound like a good idea either.

Secondly, in presence of mixed workload (some other sync read happening) WRITES can get less bandwidth and sync workload much more. So by marking journal commits as WRITES you might increase the delay there in completion in presence of other sync workload.

So Jan Kara's approach makes sense that if somebody is waiting on commit then make it WRITE_SYNC otherwise make it WRITE. Not sure why did it not work for you. Is it possible to run some traces and do more debugging that figure out what's happening.

Thanks
Vivek

Shaohua Li

2011-01-21 07:17 UTC #5 lore raw

Quoted 28 lines · Vivek Goyal

Quoted 10 lines · Shaohua Li
add Jan and Theodore to the loop.
Quoted 7 lines · Alex
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.
Quoted 6 lines
Quoted 4 lines
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
since WRITE_SYNC_PLUG doesn't work, this isn't a simple no-write-merge issue.
Yep, it does sound like reduce write merging. But moving journal commits back to WRITE, then fsync performance will drop as there will be idling introduced between fsync thread and journalling thread. So that does not sound like a good idea either.
Secondly, in presence of mixed workload (some other sync read happening) WRITES can get less bandwidth and sync workload much more. So by marking journal commits as WRITES you might increase the delay there in completion in presence of other sync workload.

Quoted 4 lines

So Jan Kara's approach makes sense that if somebody is waiting on commit then make it WRITE_SYNC otherwise make it WRITE. Not sure why did it not work for you. Is it possible to run some traces and do more debugging that figure out what's happening.

I'll debug and see if I can find anything.

Thanks,
Shaohua

Shaohua Li

2011-01-26 08:15 UTC #6 lore raw

In reply to Vivek Goyal

Quoted 28 lines · Vivek Goyal

Quoted 10 lines · Shaohua Li
add Jan and Theodore to the loop.
Quoted 7 lines · Alex
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.
Quoted 6 lines
Quoted 4 lines
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
since WRITE_SYNC_PLUG doesn't work, this isn't a simple no-write-merge issue.
Yep, it does sound like reduce write merging. But moving journal commits back to WRITE, then fsync performance will drop as there will be idling introduced between fsync thread and journalling thread. So that does not sound like a good idea either.
Secondly, in presence of mixed workload (some other sync read happening) WRITES can get less bandwidth and sync workload much more. So by marking journal commits as WRITES you might increase the delay there in completion in presence of other sync workload.

Quoted 4 lines

So Jan Kara's approach makes sense that if somebody is waiting on commit then make it WRITE_SYNC otherwise make it WRITE. Not sure why did it not work for you. Is it possible to run some traces and do more debugging that figure out what's happening.

Sorry for the long delay.

Looks fedora enables ccache by default. While our kbuild test is on ext4 disk but rootfs is on ext3 where ccache cache files live. Jan's patch only covers ext4, maybe this is the reason.
I changed jbd to use WRITE for journal_commit_transaction. With the change and Jan's patch, the test seems fine.

Jan,
can you send a patch with similar change for ext3? So we can do more tests.

Thanks,
Shaohua

Alex,Shi

author

2011-02-12 09:22 UTC #7 lore raw

Quoted 33 lines · Shaohua

Quoted 28 lines · Vivek Goyal
Quoted 10 lines · Shaohua Li
add Jan and Theodore to the loop.
Quoted 7 lines · Alex
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.
Quoted 6 lines
Quoted 4 lines
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
since WRITE_SYNC_PLUG doesn't work, this isn't a simple no-write-merge issue.
Yep, it does sound like reduce write merging. But moving journal commits back to WRITE, then fsync performance will drop as there will be idling introduced between fsync thread and journalling thread. So that does not sound like a good idea either.
Secondly, in presence of mixed workload (some other sync read happening) WRITES can get less bandwidth and sync workload much more. So by marking journal commits as WRITES you might increase the delay there in completion in presence of other sync workload.
Quoted 4 lines
So Jan Kara's approach makes sense that if somebody is waiting on commit then make it WRITE_SYNC otherwise make it WRITE. Not sure why did it not work for you. Is it possible to run some traces and do more debugging that figure out what's happening.

Quoted 7 lines

Sorry for the long delay.
Looks fedora enables ccache by default. While our kbuild test is on ext4 disk but rootfs is on ext3 where ccache cache files live. Jan's patch only covers ext4, maybe this is the reason.
I changed jbd to use WRITE for journal_commit_transaction. With the change and Jan's patch, the test seems fine.

Let me clarify the bug situation again.
With the following scenarios, the regression is clear.
1, ccache_dir setup at rootfs that format is ext3 on /dev/sda1; 2, kbuild on /dev/sdb1 with ext4.
but if we disable the ccache, only do kbuild on sdb1 with ext4. There is no regressions whenever with or without Jan's patch.
So, problem focus on the ccache scenario, (from fedora 11, ccache is default setting).

If we compare the vmstat output with or without ccache, there is too many write when ccache enabled. According the result, it should to do some tunning on ext3 fs.

vmstat average output per 10 seconds, without ccache
procs -----------memory---------- ---swap-- -----io---- --system-- -----cpu-----
r b swpd free buff cache si so bi bo in cs us sy id wa st
26.8 0.5 0.0 63930192.3 9677.0 96544.9 0.0 0.0 2486.9 337.9 17729.9 4496.4 17.5 2.5 79.8 0.2 0.0

vmstat average output per 10 seconds, with ccache
procs -----------memory---------- ---swap-- -----io---- --system-- -----cpu-----
r b swpd free buff cache si so bi bo in cs us sy id wa st
2.4 40.7 0.0 64316231.0 17260.6 119533.8 0.0 0.0 2477.6 1493.1 8606.4 3565.2 2.5 1.1 83.0 13.5 0.0

Quoted 6 lines

Jan,
can you send a patch with similar change for ext3? So we can do more tests.
Thanks,
Shaohua

Corrado Zoccolo

2011-02-12 18:26 UTC #8 lore raw

Quoted 50 lines · Alex,Shi

Quoted 33 lines · Shaohua
Quoted 28 lines · Vivek Goyal
Quoted 10 lines · Shaohua Li
add Jan and Theodore to the loop.
Quoted 7 lines · Alex
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.
Quoted 6 lines
Quoted 4 lines
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
since WRITE_SYNC_PLUG doesn't work, this isn't a simple no-write-merge issue.
Yep, it does sound like reduce write merging. But moving journal commits back to WRITE, then fsync performance will drop as there will be idling introduced between fsync thread and journalling thread. So that does not sound like a good idea either.
Secondly, in presence of mixed workload (some other sync read happening) WRITES can get less bandwidth and sync workload much more. So by marking journal commits as WRITES you might increase the delay there in completion in presence of other sync workload.
Quoted 4 lines
So Jan Kara's approach makes sense that if somebody is waiting on commit then make it WRITE_SYNC otherwise make it WRITE. Not sure why did it not work for you. Is it possible to run some traces and do more debugging that figure out what's happening.
Quoted 7 lines
Sorry for the long delay.
Looks fedora enables ccache by default. While our kbuild test is on ext4 disk but rootfs is on ext3 where ccache cache files live. Jan's patch only covers ext4, maybe this is the reason.
I changed jbd to use WRITE for journal_commit_transaction. With the change and Jan's patch, the test seems fine.
Let me clarify the bug situation again.
With the following scenarios, the regression is clear.
1, ccache_dir setup at rootfs that format is ext3 on /dev/sda1; 2, kbuild on /dev/sdb1 with ext4.
but if we disable the ccache, only do kbuild on sdb1 with ext4. There is no regressions whenever with or without Jan's patch.
So, problem focus on the ccache scenario, (from fedora 11, ccache is default setting).

Quoted 3 lines

If we compare the vmstat output with or without ccache, there is too many write when ccache enabled. According the result, it should to do some tunning on ext3 fs.

Is ext3 configured with data ordered or writeback?
I think ccache might be performing fsyncs, and this is a bad workload for ext3, especially in ordered mode.
It might be that my patch introduced a regression in ext3 fsync performance, but I don't understand how reverting only the change in jbd2 (that is the ext4 specific journaling daemon) could restore it. The two partitions are on different disks, so each one should be isolated from the I/O perspective (do they share a single controller?). The only interaction I see happens at the VM level, since changing performance of any of the two changes the rate at which pages can be cleaned.

Corrado

Quoted 23 lines

vmstat average output per 10 seconds, without ccache
procs -----------memory---------- ---swap-- -----io---- --system-- -----cpu----- r b swpd free buff cache si so bi bo in cs us sy id wa st 26.8 0.5 0.0 63930192.3 9677.0 96544.9 0.0 0.0 2486.9 337.9 17729.9 4496.4 17.5 2.5 79.8 0.2 0.0
vmstat average output per 10 seconds, with ccache
procs -----------memory---------- ---swap-- -----io---- --system-- -----cpu-----
r b swpd free buff cache si so bi bo in cs us sy id wa st 2.4 40.7 0.0 64316231.0 17260.6 119533.8 0.0 0.0 2477.6 1493.1 8606.4 3565.2 2.5 1.1 83.0 13.5 0.0
Quoted 6 lines
Jan,
can you send a patch with similar change for ext3? So we can do more tests.
Thanks,
Shaohua

Signature · 10 lines

__________________________________________________________________________

dott. Corrado Zoccolo                          mailto:[email protected]
PhD - Department of Computer Science - University of Pisa, Italy
--------------------------------------------------------------------------
The self-confidence of a warrior is not the self-confidence of the average
man. The average man seeks certainty in the eyes of the onlooker and calls
that self-confidence. The warrior seeks impeccability in his own eyes and
calls that humbleness.
                               Tales of Power - C. Castaneda

Alex,Shi

author

2011-02-14 02:25 UTC #9 lore raw

Quoted 51 lines · Corrado Zoccolo

Quoted 50 lines · Alex,Shi
Quoted 33 lines · Shaohua
Quoted 28 lines · Vivek Goyal
Quoted 10 lines · Shaohua Li
add Jan and Theodore to the loop.
Quoted 7 lines · Alex
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.
Quoted 6 lines
Quoted 4 lines
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
since WRITE_SYNC_PLUG doesn't work, this isn't a simple no-write-merge issue.
Yep, it does sound like reduce write merging. But moving journal commits back to WRITE, then fsync performance will drop as there will be idling introduced between fsync thread and journalling thread. So that does not sound like a good idea either.
Secondly, in presence of mixed workload (some other sync read happening) WRITES can get less bandwidth and sync workload much more. So by marking journal commits as WRITES you might increase the delay there in completion in presence of other sync workload.
Quoted 4 lines
So Jan Kara's approach makes sense that if somebody is waiting on commit then make it WRITE_SYNC otherwise make it WRITE. Not sure why did it not work for you. Is it possible to run some traces and do more debugging that figure out what's happening.
Quoted 7 lines
Sorry for the long delay.
Looks fedora enables ccache by default. While our kbuild test is on ext4 disk but rootfs is on ext3 where ccache cache files live. Jan's patch only covers ext4, maybe this is the reason.
I changed jbd to use WRITE for journal_commit_transaction. With the change and Jan's patch, the test seems fine.
Let me clarify the bug situation again.
With the following scenarios, the regression is clear.
1, ccache_dir setup at rootfs that format is ext3 on /dev/sda1; 2, kbuild on /dev/sdb1 with ext4.
but if we disable the ccache, only do kbuild on sdb1 with ext4. There is no regressions whenever with or without Jan's patch.
So, problem focus on the ccache scenario, (from fedora 11, ccache is default setting).

Quoted 4 lines

Quoted 3 lines
If we compare the vmstat output with or without ccache, there is too many write when ccache enabled. According the result, it should to do some tunning on ext3 fs.
Is ext3 configured with data ordered or writeback?

The ext3 on sda and ext4 on sdb are both used 'ordered' mounting mode.

Quoted 8 lines

I think ccache might be performing fsyncs, and this is a bad workload for ext3, especially in ordered mode.
It might be that my patch introduced a regression in ext3 fsync performance, but I don't understand how reverting only the change in jbd2 (that is the ext4 specific journaling daemon) could restore it. The two partitions are on different disks, so each one should be isolated from the I/O perspective (do they share a single controller?).

No, sda/sdb use separated controller.

Quoted 31 lines

The only interaction I see happens at the VM level,
since changing performance of any of the two changes the rate at which pages can be cleaned.
Corrado
Quoted 23 lines
vmstat average output per 10 seconds, without ccache
procs -----------memory---------- ---swap-- -----io---- --system-- -----cpu-----
r b swpd free buff cache si so bi bo in cs us sy id wa st
26.8 0.5 0.0 63930192.3 9677.0 96544.9 0.0 0.0 2486.9 337.9 17729.9 4496.4 17.5 2.5 79.8 0.2 0.0
vmstat average output per 10 seconds, with ccache
procs -----------memory---------- ---swap-- -----io---- --system-- -----cpu-----
r b swpd free buff cache si so bi bo in cs us sy id wa st
2.4 40.7 0.0 64316231.0 17260.6 119533.8 0.0 0.0 2477.6 1493.1 8606.4 3565.2 2.5 1.1 83.0 13.5 0.0
Quoted 6 lines
Jan,
can you send a patch with similar change for ext3? So we can do more tests.
Thanks,
Shaohua

Shaohua Li

2011-02-15 01:10 UTC #10 lore raw

Quoted 68 lines · Alex

Quoted 51 lines · Corrado Zoccolo
Quoted 50 lines · Alex,Shi
Quoted 33 lines · Shaohua
Quoted 28 lines · Vivek Goyal
Quoted 10 lines · Shaohua Li
add Jan and Theodore to the loop.
Quoted 7 lines · Alex
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.
Quoted 6 lines
Quoted 4 lines
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
since WRITE_SYNC_PLUG doesn't work, this isn't a simple no-write-merge issue.
Yep, it does sound like reduce write merging. But moving journal commits back to WRITE, then fsync performance will drop as there will be idling introduced between fsync thread and journalling thread. So that does not sound like a good idea either.
Secondly, in presence of mixed workload (some other sync read happening) WRITES can get less bandwidth and sync workload much more. So by marking journal commits as WRITES you might increase the delay there in completion in presence of other sync workload.
Quoted 4 lines
So Jan Kara's approach makes sense that if somebody is waiting on commit then make it WRITE_SYNC otherwise make it WRITE. Not sure why did it not work for you. Is it possible to run some traces and do more debugging that figure out what's happening.
Quoted 7 lines
Sorry for the long delay.
Looks fedora enables ccache by default. While our kbuild test is on ext4 disk but rootfs is on ext3 where ccache cache files live. Jan's patch only covers ext4, maybe this is the reason.
I changed jbd to use WRITE for journal_commit_transaction. With the change and Jan's patch, the test seems fine.
Let me clarify the bug situation again.
With the following scenarios, the regression is clear.
1, ccache_dir setup at rootfs that format is ext3 on /dev/sda1; 2, kbuild on /dev/sdb1 with ext4.
but if we disable the ccache, only do kbuild on sdb1 with ext4. There is no regressions whenever with or without Jan's patch.
So, problem focus on the ccache scenario, (from fedora 11, ccache is default setting).
Quoted 4 lines
Quoted 3 lines
If we compare the vmstat output with or without ccache, there is too many write when ccache enabled. According the result, it should to do some tunning on ext3 fs.
Is ext3 configured with data ordered or writeback?
The ext3 on sda and ext4 on sdb are both used 'ordered' mounting mode.
Quoted 8 lines
I think ccache might be performing fsyncs, and this is a bad workload for ext3, especially in ordered mode.
It might be that my patch introduced a regression in ext3 fsync performance, but I don't understand how reverting only the change in jbd2 (that is the ext4 specific journaling daemon) could restore it. The two partitions are on different disks, so each one should be isolated from the I/O perspective (do they share a single controller?).

Quoted 23 lines

No, sda/sdb use separated controller.
Quoted 21 lines
The only interaction I see happens at the VM level,
since changing performance of any of the two changes the rate at which pages can be cleaned.
Corrado
Quoted 16 lines
vmstat average output per 10 seconds, without ccache
procs -----------memory---------- ---swap-- -----io---- --system-- -----cpu-----
r b swpd free buff cache si so bi bo in cs us sy id wa st
26.8 0.5 0.0 63930192.3 9677.0 96544.9 0.0 0.0 2486.9 337.9 17729.9 4496.4 17.5 2.5 79.8 0.2 0.0
vmstat average output per 10 seconds, with ccache
procs -----------memory---------- ---swap-- -----io---- --system-- -----cpu-----
r b swpd free buff cache si so bi bo in cs us sy id wa st
2.4 40.7 0.0 64316231.0 17260.6 119533.8 0.0 0.0 2477.6 1493.1 8606.4 3565.2 2.5 1.1 83.0 13.5 0.0
Quoted 3 lines
Jan,
can you send a patch with similar change for ext3? So we can do more tests.

Hi Jan,
can you send a patch with both ext3 and ext4 changes? Our test shows your patch has positive effect, but need confirm with the ext3 change.

Thanks,
Shaohua

Jan Kara

reviewer

2011-02-21 16:49 UTC #11 lore raw

Quoted 90 lines · Shaohua Li

Quoted 89 lines · Alex
Quoted 51 lines · Corrado Zoccolo
Quoted 50 lines · Alex,Shi
Quoted 33 lines · Shaohua
Quoted 28 lines · Vivek Goyal
Quoted 10 lines · Shaohua Li
add Jan and Theodore to the loop.
Quoted 7 lines · Alex
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.
Quoted 6 lines
Quoted 4 lines
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
since WRITE_SYNC_PLUG doesn't work, this isn't a simple no-write-merge issue.
Yep, it does sound like reduce write merging. But moving journal commits back to WRITE, then fsync performance will drop as there will be idling introduced between fsync thread and journalling thread. So that does not sound like a good idea either.
Secondly, in presence of mixed workload (some other sync read happening) WRITES can get less bandwidth and sync workload much more. So by marking journal commits as WRITES you might increase the delay there in completion in presence of other sync workload.
Quoted 4 lines
So Jan Kara's approach makes sense that if somebody is waiting on commit then make it WRITE_SYNC otherwise make it WRITE. Not sure why did it not work for you. Is it possible to run some traces and do more debugging that figure out what's happening.
Quoted 7 lines
Sorry for the long delay.
Looks fedora enables ccache by default. While our kbuild test is on ext4 disk but rootfs is on ext3 where ccache cache files live. Jan's patch only covers ext4, maybe this is the reason.
I changed jbd to use WRITE for journal_commit_transaction. With the change and Jan's patch, the test seems fine.
Let me clarify the bug situation again.
With the following scenarios, the regression is clear.
1, ccache_dir setup at rootfs that format is ext3 on /dev/sda1; 2, kbuild on /dev/sdb1 with ext4.
but if we disable the ccache, only do kbuild on sdb1 with ext4. There is no regressions whenever with or without Jan's patch.
So, problem focus on the ccache scenario, (from fedora 11, ccache is default setting).
Quoted 4 lines
Quoted 3 lines
If we compare the vmstat output with or without ccache, there is too many write when ccache enabled. According the result, it should to do some tunning on ext3 fs.
Is ext3 configured with data ordered or writeback?
The ext3 on sda and ext4 on sdb are both used 'ordered' mounting mode.
Quoted 8 lines
I think ccache might be performing fsyncs, and this is a bad workload for ext3, especially in ordered mode.
It might be that my patch introduced a regression in ext3 fsync performance, but I don't understand how reverting only the change in jbd2 (that is the ext4 specific journaling daemon) could restore it. The two partitions are on different disks, so each one should be isolated from the I/O perspective (do they share a single controller?).
No, sda/sdb use separated controller.
Quoted 19 lines
The only interaction I see happens at the VM level,
since changing performance of any of the two changes the rate at which pages can be cleaned.
Corrado
Quoted 14 lines
vmstat average output per 10 seconds, without ccache
procs -----------memory---------- ---swap-- -----io---- --system-- -----cpu-----
r b swpd free buff cache si so bi bo in cs us sy id wa st
26.8 0.5 0.0 63930192.3 9677.0 96544.9 0.0 0.0 2486.9 337.9 17729.9 4496.4 17.5 2.5 79.8 0.2 0.0
vmstat average output per 10 seconds, with ccache
procs -----------memory---------- ---swap-- -----io---- --system-- -----cpu-----
r b swpd free buff cache si so bi bo in cs us sy id wa st
2.4 40.7 0.0 64316231.0 17260.6 119533.8 0.0 0.0 2477.6 1493.1 8606.4 3565.2 2.5 1.1 83.0 13.5 0.0
Quoted 1 line

Quoted 5 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Jan,
can you send a patch with similar change for ext3? So we can do more tests.
Hi Jan,
can you send a patch with both ext3 and ext4 changes? Our test shows your patch has positive effect, but need confirm with the ext3 change.

Sure. Patches for both ext3 & ext4 are attached. Sorry, it took me a
while to get to this.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 1 line

From 5674f84e70db8274d5aac56a41439ea3b3bcb46e Mon Sep 17 00:00:00 2001

From: Jan Kara <[email protected]>
Date: Wed, 19 Jan 2011 13:45:04 +0100
Subject: [PATCH 1/2] jbd2: Refine commit writeout logic

Signed-off-by Jan Kara <[email protected]>

---
 fs/ext4/fsync.c       |    2 +-
 fs/ext4/super.c       |    2 +-
 fs/jbd2/checkpoint.c  |    2 +-
 fs/jbd2/commit.c      |    4 ++--
 fs/jbd2/journal.c     |   19 ++++++++++---------
 fs/jbd2/transaction.c |   13 ++++++-------
 fs/ocfs2/aops.c       |    2 +-
 fs/ocfs2/super.c      |    2 +-
 include/linux/jbd2.h  |   18 ++++++++++--------
 9 files changed, 33 insertions(+), 31 deletions(-)

fs/ext4/fsync.c +1 -1

index 7829b28..19434da 100644
@@ -198,7 +198,7 @@ int ext4_sync_file(struct file *file, int datasync)
 		return ext4_force_commit(inode->i_sb);
 
 	commit_tid = datasync ? ei->i_datasync_tid : ei->i_sync_tid;
-	if (jbd2_log_start_commit(journal, commit_tid)) {
+	if (jbd2_log_start_commit(journal, commit_tid, true)) {
 		/*
 		 * When the journal is on a different device than the
 		 * fs data disk, we need to issue the barrier in

fs/ext4/super.c +1 -1

index 48ce561..0aeb877 100644
@@ -4113,7 +4113,7 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
 
 	trace_ext4_sync_fs(sb, wait);
 	flush_workqueue(sbi->dio_unwritten_wq);
-	if (jbd2_journal_start_commit(sbi->s_journal, &target)) {
+	if (jbd2_journal_start_commit(sbi->s_journal, &target, true)) {
 		if (wait)
 			jbd2_log_wait_commit(sbi->s_journal, target);
 	}

fs/jbd2/checkpoint.c +1 -1

index 6a79fd0..3436d53 100644
@@ -309,7 +309,7 @@ static int __process_buffer(journal_t *journal, struct journal_head *jh,
 			       "Waiting for Godot: block %llu\n",
 			       journal->j_devname,
 			       (unsigned long long) bh->b_blocknr);
-		jbd2_log_start_commit(journal, tid);
+		jbd2_log_start_commit(journal, tid, true);
 		jbd2_log_wait_commit(journal, tid);
 		ret = 1;
 	} else if (!buffer_dirty(bh)) {

fs/jbd2/commit.c +2 -2

index f3ad159..19973eb 100644
@@ -329,7 +329,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	int tag_bytes = journal_tag_bytes(journal);
 	struct buffer_head *cbh = NULL; /* For transactional checksums */
 	__u32 crc32_sum = ~0;
-	int write_op = WRITE_SYNC;
+	int write_op = WRITE;
 
 	/*
 	 * First job: lock down the current transaction and wait for

@@ -368,7 +368,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	 * we unplug the device. We don't do explicit unplugging in here,
 	 * instead we rely on sync_buffer() doing the unplug for us.
 	 */
-	if (commit_transaction->t_synchronous_commit)
+	if (tid_geq(journal->j_commit_waited, commit_transaction->t_tid))
 		write_op = WRITE_SYNC_PLUG;
 	trace_jbd2_commit_locking(journal, commit_transaction);
 	stats.run.rs_wait = commit_transaction->t_max_wait;

fs/jbd2/journal.c +10 -9

index 9e46869..e278fa2 100644
@@ -475,7 +475,7 @@ int __jbd2_log_space_left(journal_t *journal)
 /*
  * Called under j_state_lock.  Returns true if a transaction commit was started.
  */
-int __jbd2_log_start_commit(journal_t *journal, tid_t target)
+int __jbd2_log_start_commit(journal_t *journal, tid_t target, bool will_wait)
 {
 	/*
 	 * Are we already doing a recent enough commit?

@@ -485,7 +485,8 @@ int __jbd2_log_start_commit(journal_t *journal, tid_t target)
 		 * We want a new commit: OK, mark the request and wakeup the
 		 * commit thread.  We do _not_ do the commit ourselves.
 		 */
-
+		if (will_wait && !tid_geq(journal->j_commit_waited, target))
+			journal->j_commit_waited = target;
 		journal->j_commit_request = target;
 		jbd_debug(1, "JBD: requesting commit %d/%d\n",
 			  journal->j_commit_request,

@@ -496,12 +497,12 @@ int __jbd2_log_start_commit(journal_t *journal, tid_t target)
 	return 0;
 }
 
-int jbd2_log_start_commit(journal_t *journal, tid_t tid)
+int jbd2_log_start_commit(journal_t *journal, tid_t tid, bool will_wait)
 {
 	int ret;
 
 	write_lock(&journal->j_state_lock);
-	ret = __jbd2_log_start_commit(journal, tid);
+	ret = __jbd2_log_start_commit(journal, tid, will_wait);
 	write_unlock(&journal->j_state_lock);
 	return ret;
 }

@@ -524,7 +525,7 @@ int jbd2_journal_force_commit_nested(journal_t *journal)
 	read_lock(&journal->j_state_lock);
 	if (journal->j_running_transaction && !current->journal_info) {
 		transaction = journal->j_running_transaction;
-		__jbd2_log_start_commit(journal, transaction->t_tid);
+		__jbd2_log_start_commit(journal, transaction->t_tid, true);
 	} else if (journal->j_committing_transaction)
 		transaction = journal->j_committing_transaction;

@@ -544,7 +545,7 @@ int jbd2_journal_force_commit_nested(journal_t *journal)
  * if a transaction is going to be committed (or is currently already
  * committing), and fills its tid in at *ptid
  */
-int jbd2_journal_start_commit(journal_t *journal, tid_t *ptid)
+int jbd2_journal_start_commit(journal_t *journal, tid_t *ptid, bool will_wait)
 {
 	int ret = 0;

@@ -552,7 +553,7 @@ int jbd2_journal_start_commit(journal_t *journal, tid_t *ptid)
 	if (journal->j_running_transaction) {
 		tid_t tid = journal->j_running_transaction->t_tid;
 
-		__jbd2_log_start_commit(journal, tid);
+		__jbd2_log_start_commit(journal, tid, will_wait);
 		/* There's a running transaction and we've just made sure
 		 * it's commit has been scheduled. */
 		if (ptid)

@@ -1559,7 +1560,7 @@ int jbd2_journal_flush(journal_t *journal)
 	/* Force everything buffered to the log... */
 	if (journal->j_running_transaction) {
 		transaction = journal->j_running_transaction;
-		__jbd2_log_start_commit(journal, transaction->t_tid);
+		__jbd2_log_start_commit(journal, transaction->t_tid, true);
 	} else if (journal->j_committing_transaction)
 		transaction = journal->j_committing_transaction;

@@ -1675,7 +1676,7 @@ void __jbd2_journal_abort_hard(journal_t *journal)
 	journal->j_flags |= JBD2_ABORT;
 	transaction = journal->j_running_transaction;
 	if (transaction)
-		__jbd2_log_start_commit(journal, transaction->t_tid);
+		__jbd2_log_start_commit(journal, transaction->t_tid, false);
 	write_unlock(&journal->j_state_lock);
 }

fs/jbd2/transaction.c +6 -7

index faad2bd..c48e6e8 100644
@@ -222,7 +222,7 @@ repeat:
 		atomic_sub(nblocks, &transaction->t_outstanding_credits);
 		prepare_to_wait(&journal->j_wait_transaction_locked, &wait,
 				TASK_UNINTERRUPTIBLE);
-		__jbd2_log_start_commit(journal, transaction->t_tid);
+		__jbd2_log_start_commit(journal, transaction->t_tid, false);
 		read_unlock(&journal->j_state_lock);
 		schedule();
 		finish_wait(&journal->j_wait_transaction_locked, &wait);

@@ -465,7 +465,7 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, int gfp_mask)
 	spin_unlock(&transaction->t_handle_lock);
 
 	jbd_debug(2, "restarting handle %p\n", handle);
-	__jbd2_log_start_commit(journal, transaction->t_tid);
+	__jbd2_log_start_commit(journal, transaction->t_tid, false);
 	read_unlock(&journal->j_state_lock);
 
 	lock_map_release(&handle->h_lockdep_map);

@@ -1361,8 +1361,6 @@ int jbd2_journal_stop(handle_t *handle)
 		}
 	}
 
-	if (handle->h_sync)
-		transaction->t_synchronous_commit = 1;
 	current->journal_info = NULL;
 	atomic_sub(handle->h_buffer_credits,
 		   &transaction->t_outstanding_credits);

@@ -1383,15 +1381,16 @@ int jbd2_journal_stop(handle_t *handle)
 
 		jbd_debug(2, "transaction too old, requesting commit for "
 					"handle %p\n", handle);
-		/* This is non-blocking */
-		jbd2_log_start_commit(journal, transaction->t_tid);
-
 		/*
 		 * Special case: JBD2_SYNC synchronous updates require us
 		 * to wait for the commit to complete.
 		 */
 		if (handle->h_sync && !(current->flags & PF_MEMALLOC))
 			wait_for_commit = 1;
+
+		/* This is non-blocking */
+		jbd2_log_start_commit(journal, transaction->t_tid,
+				      wait_for_commit);
 	}
 
 	/*

fs/ocfs2/aops.c +1 -1

index 1fbb0e2..d493f32 100644
@@ -1659,7 +1659,7 @@ static int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
 		goto out;
 	}
 
-	if (jbd2_journal_start_commit(osb->journal->j_journal, &target)) {
+	if (jbd2_journal_start_commit(osb->journal->j_journal, &target, true)) {
 		jbd2_log_wait_commit(osb->journal->j_journal, target);
 		ret = 1;
 	}

fs/ocfs2/super.c +1 -1

index 38f986d..45d9f82 100644
@@ -414,7 +414,7 @@ static int ocfs2_sync_fs(struct super_block *sb, int wait)
 	}
 
 	if (jbd2_journal_start_commit(OCFS2_SB(sb)->journal->j_journal,
-				      &target)) {
+				      &target, wait)) {
 		if (wait)
 			jbd2_log_wait_commit(OCFS2_SB(sb)->journal->j_journal,
 					     target);

include/linux/jbd2.h +10 -8

index 27e79c2..46aaf45 100644
@@ -631,11 +631,6 @@ struct transaction_s
 	 */
 	atomic_t		t_handle_count;
 
-	/*
-	 * This transaction is being forced and some process is
-	 * waiting for it to finish.
-	 */
-	unsigned int t_synchronous_commit:1;
 	unsigned int t_flushed_data_blocks:1;
 
 	/*

@@ -900,6 +895,13 @@ struct journal_s
 	tid_t			j_commit_request;
 
 	/*
+	 * Sequence number of the most recent transaction someone is waiting
+	 * for to commit.
+	 * [j_state_lock]
+	 */
+	tid_t			j_commit_waited;
+
+	/*
 	 * Journal uuid: identifies the object (filesystem, LVM volume etc)
 	 * backed by this journal.  This will eventually be replaced by an array
 	 * of uuids, allowing us to index multiple devices within a single

@@ -1200,9 +1202,9 @@ extern void	jbd2_journal_switch_revoke_table(journal_t *journal);
  */
 
 int __jbd2_log_space_left(journal_t *); /* Called with journal locked */
-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_start_commit(journal_t *journal, tid_t tid, bool will_wait);
+int __jbd2_log_start_commit(journal_t *journal, tid_t tid, bool will_wait);
+int jbd2_journal_start_commit(journal_t *journal, tid_t *tid, bool will_wait);
 int jbd2_journal_force_commit_nested(journal_t *journal);
 int jbd2_log_wait_commit(journal_t *journal, tid_t tid);
 int jbd2_log_do_checkpoint(journal_t *journal);

Signature · 1 line

1.7.1

Quoted 1 line

From ff2be0c11564d1253d9420ee0f805f2ba8f62e9f Mon Sep 17 00:00:00 2001

From: Jan Kara <[email protected]>
Date: Mon, 21 Feb 2011 17:25:37 +0100
Subject: [PATCH 2/2] jbd: Refine commit writeout logic

Signed-off-by Jan Kara <[email protected]>

---
 fs/ext3/fsync.c      |    2 +-
 fs/ext3/super.c      |    2 +-
 fs/jbd/checkpoint.c  |    2 +-
 fs/jbd/commit.c      |    4 ++--
 fs/jbd/journal.c     |   19 ++++++++++---------
 fs/jbd/transaction.c |    9 ++++-----
 include/linux/jbd.h  |   21 ++++++++++++---------
 7 files changed, 31 insertions(+), 28 deletions(-)

fs/ext3/fsync.c +1 -1

index 09b13bb..5396dd6 100644
@@ -81,7 +81,7 @@ int ext3_sync_file(struct file *file, int datasync)
 	if (test_opt(inode->i_sb, BARRIER) &&
 	    !journal_trans_will_send_data_barrier(journal, commit_tid))
 		needs_barrier = 1;
-	log_start_commit(journal, commit_tid);
+	log_start_commit(journal, commit_tid, true);
 	ret = log_wait_commit(journal, commit_tid);
 
 	/*

fs/ext3/super.c +1 -1

index 85c8cc8..58a4424 100644
@@ -2497,7 +2497,7 @@ static int ext3_sync_fs(struct super_block *sb, int wait)
 {
 	tid_t target;
 
-	if (journal_start_commit(EXT3_SB(sb)->s_journal, &target)) {
+	if (journal_start_commit(EXT3_SB(sb)->s_journal, &target, true)) {
 		if (wait)
 			log_wait_commit(EXT3_SB(sb)->s_journal, target);
 	}

fs/jbd/checkpoint.c +1 -1

index e4b87bc..26ca2c3 100644
@@ -297,7 +297,7 @@ static int __process_buffer(journal_t *journal, struct journal_head *jh,
 
 		spin_unlock(&journal->j_list_lock);
 		jbd_unlock_bh_state(bh);
-		log_start_commit(journal, tid);
+		log_start_commit(journal, tid, true);
 		log_wait_commit(journal, tid);
 		ret = 1;
 	} else if (!buffer_dirty(bh)) {

fs/jbd/commit.c +2 -2

index 34a4861..ac188cb 100644
@@ -294,7 +294,7 @@ void journal_commit_transaction(journal_t *journal)
 	int first_tag = 0;
 	int tag_flag;
 	int i;
-	int write_op = WRITE_SYNC;
+	int write_op = WRITE;
 
 	/*
 	 * First job: lock down the current transaction and wait for

@@ -332,7 +332,7 @@ void journal_commit_transaction(journal_t *journal)
 	 * we unplug the device. We don't do explicit unplugging in here,
 	 * instead we rely on sync_buffer() doing the unplug for us.
 	 */
-	if (commit_transaction->t_synchronous_commit)
+	if (tid_geq(journal->j_commit_waited, commit_transaction->t_tid))
 		write_op = WRITE_SYNC_PLUG;
 	spin_lock(&commit_transaction->t_handle_lock);
 	while (commit_transaction->t_updates) {

fs/jbd/journal.c +10 -9

index da1b5e4..5743b4c 100644
@@ -434,7 +434,7 @@ int __log_space_left(journal_t *journal)
 /*
  * Called under j_state_lock.  Returns true if a transaction commit was started.
  */
-int __log_start_commit(journal_t *journal, tid_t target)
+int __log_start_commit(journal_t *journal, tid_t target, bool will_wait)
 {
 	/*
 	 * Are we already doing a recent enough commit?

@@ -444,7 +444,8 @@ int __log_start_commit(journal_t *journal, tid_t target)
 		 * We want a new commit: OK, mark the request and wakeup the
 		 * commit thread.  We do _not_ do the commit ourselves.
 		 */
-
+		if (will_wait && !tid_geq(journal->j_commit_waited, target))
+			journal->j_commit_waited = target;
 		journal->j_commit_request = target;
 		jbd_debug(1, "JBD: requesting commit %d/%d\n",
 			  journal->j_commit_request,

@@ -455,12 +456,12 @@ int __log_start_commit(journal_t *journal, tid_t target)
 	return 0;
 }
 
-int log_start_commit(journal_t *journal, tid_t tid)
+int log_start_commit(journal_t *journal, tid_t tid, bool will_wait)
 {
 	int ret;
 
 	spin_lock(&journal->j_state_lock);
-	ret = __log_start_commit(journal, tid);
+	ret = __log_start_commit(journal, tid, will_wait);
 	spin_unlock(&journal->j_state_lock);
 	return ret;
 }

@@ -483,7 +484,7 @@ int journal_force_commit_nested(journal_t *journal)
 	spin_lock(&journal->j_state_lock);
 	if (journal->j_running_transaction && !current->journal_info) {
 		transaction = journal->j_running_transaction;
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid, true);
 	} else if (journal->j_committing_transaction)
 		transaction = journal->j_committing_transaction;

@@ -503,7 +504,7 @@ int journal_force_commit_nested(journal_t *journal)
  * if a transaction is going to be committed (or is currently already
  * committing), and fills its tid in at *ptid
  */
-int journal_start_commit(journal_t *journal, tid_t *ptid)
+int journal_start_commit(journal_t *journal, tid_t *ptid, bool will_wait)
 {
 	int ret = 0;

@@ -511,7 +512,7 @@ int journal_start_commit(journal_t *journal, tid_t *ptid)
 	if (journal->j_running_transaction) {
 		tid_t tid = journal->j_running_transaction->t_tid;
 
-		__log_start_commit(journal, tid);
+		__log_start_commit(journal, tid, will_wait);
 		/* There's a running transaction and we've just made sure
 		 * it's commit has been scheduled. */
 		if (ptid)

@@ -1439,7 +1440,7 @@ int journal_flush(journal_t *journal)
 	/* Force everything buffered to the log... */
 	if (journal->j_running_transaction) {
 		transaction = journal->j_running_transaction;
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid, true);
 	} else if (journal->j_committing_transaction)
 		transaction = journal->j_committing_transaction;

@@ -1573,7 +1574,7 @@ static void __journal_abort_hard(journal_t *journal)
 	journal->j_flags |= JFS_ABORT;
 	transaction = journal->j_running_transaction;
 	if (transaction)
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid, false);
 	spin_unlock(&journal->j_state_lock);
 }

fs/jbd/transaction.c +4 -5

index 5b2e4c3..a12c0a3 100644
@@ -178,7 +178,7 @@ repeat_locked:
 		spin_unlock(&transaction->t_handle_lock);
 		prepare_to_wait(&journal->j_wait_transaction_locked, &wait,
 				TASK_UNINTERRUPTIBLE);
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid, false);
 		spin_unlock(&journal->j_state_lock);
 		schedule();
 		finish_wait(&journal->j_wait_transaction_locked, &wait);

@@ -411,7 +411,7 @@ int journal_restart(handle_t *handle, int nblocks)
 	spin_unlock(&transaction->t_handle_lock);
 
 	jbd_debug(2, "restarting handle %p\n", handle);
-	__log_start_commit(journal, transaction->t_tid);
+	__log_start_commit(journal, transaction->t_tid, false);
 	spin_unlock(&journal->j_state_lock);
 
 	lock_map_release(&handle->h_lockdep_map);

@@ -1431,8 +1431,6 @@ int journal_stop(handle_t *handle)
 		}
 	}
 
-	if (handle->h_sync)
-		transaction->t_synchronous_commit = 1;
 	current->journal_info = NULL;
 	spin_lock(&journal->j_state_lock);
 	spin_lock(&transaction->t_handle_lock);

@@ -1463,7 +1461,8 @@ int journal_stop(handle_t *handle)
 		jbd_debug(2, "transaction too old, requesting commit for "
 					"handle %p\n", handle);
 		/* This is non-blocking */
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid,
+			handle->h_sync && !(current->flags & PF_MEMALLOC));
 		spin_unlock(&journal->j_state_lock);
 
 		/*

include/linux/jbd.h +12 -9

index e069650..c38f73e 100644
@@ -541,12 +541,6 @@ struct transaction_s
 	 * How many handles used this transaction? [t_handle_lock]
 	 */
 	int t_handle_count;
-
-	/*
-	 * This transaction is being forced and some process is
-	 * waiting for it to finish.
-	 */
-	unsigned int t_synchronous_commit:1;
 };
 
 /**

@@ -594,6 +588,8 @@ struct transaction_s
  *  transaction
  * @j_commit_request: Sequence number of the most recent transaction wanting
  *     commit
+ * @j_commit_waited: Sequence number of the most recent transaction someone
+ *     is waiting for to commit.
  * @j_uuid: Uuid of client object.
  * @j_task: Pointer to the current commit thread for this journal
  * @j_max_transaction_buffers:  Maximum number of metadata buffers to allow in a

@@ -762,6 +758,13 @@ struct journal_s
 	tid_t			j_commit_request;
 
 	/*
+	 * Sequence number of the most recent transaction someone is waiting
+	 * for to commit.
+	 * [j_state_lock]
+	 */
+	tid_t                   j_commit_waited;
+
+	/*
 	 * Journal uuid: identifies the object (filesystem, LVM volume etc)
 	 * backed by this journal.  This will eventually be replaced by an array
 	 * of uuids, allowing us to index multiple devices within a single

@@ -985,9 +988,9 @@ extern void	journal_switch_revoke_table(journal_t *journal);
  */
 
 int __log_space_left(journal_t *); /* Called with journal locked */
-int log_start_commit(journal_t *journal, tid_t tid);
-int __log_start_commit(journal_t *journal, tid_t tid);
-int journal_start_commit(journal_t *journal, tid_t *tid);
+int log_start_commit(journal_t *journal, tid_t tid, bool will_wait);
+int __log_start_commit(journal_t *journal, tid_t tid, bool will_wait);
+int journal_start_commit(journal_t *journal, tid_t *tid, bool will_wait);
 int journal_force_commit_nested(journal_t *journal);
 int log_wait_commit(journal_t *journal, tid_t tid);
 int log_do_checkpoint(journal_t *journal);

Signature · 1 line

1.7.1

Alex,Shi

author

2011-02-23 08:23 UTC #12 lore raw

Though these patches can not totally recovered the problem, but they are
quite helpful with ccache enabled situation. It increase 10% performance
on 38-rc1 kernel.
I have tried to enabled they to latest rc6 kernel but failed. the vmstat output is here:
with patches:
r b swpd free buff cache si so bi bo in cs us sy id wa st
1.5 24.7 0.0 64316199.8 17240.8 153376.9 0.0 0.0 1777.1 1788.0 6479.0 2605.3 1.7 0.9 89.1 8.2 0.0 original 38-rc1 kernel:
2.4 32.3 0.0 63653302.9 17170.6 153125.3 0.0 0.0 1579.7 1834.1 6016.4 2407.0 1.5 0.7 86.6 10.1 0.0

It reduce write blocks clearly.

Quoted 6 lines

Quoted 2 lines
can you send a patch with both ext3 and ext4 changes? Our test shows your patch has positive effect, but need confirm with the ext3 change.
Sure. Patches for both ext3 & ext4 are attached. Sorry, it took me a
while to get to this.
Honza

Jan Kara

reviewer

2011-02-24 12:13 UTC #13 lore raw

Quoted 3 lines · Alex,Shi

Though these patches can not totally recovered the problem, but they are quite helpful with ccache enabled situation. It increase 10% performance on 38-rc1 kernel.

OK and what was the original performance drop with WRITE_SYNC change?

Quoted 2 lines

I have tried to enabled they to latest rc6 kernel but failed. the vmstat output is here:
with patches:

I'm attaching patches rebased on top of latest Linus's tree.
Corrado, could you possibly run your fsync-heavy tests so that we see whether there isn't negative impact of my patches on your fsync-heavy workload? Thanks.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 1 line

From 555d33d45bd38a8da6b187a15ad5eb9e9c4f1ab9 Mon Sep 17 00:00:00 2001

From: Jan Kara <[email protected]>
Date: Wed, 19 Jan 2011 13:45:04 +0100
Subject: [PATCH 1/2] jbd2: Refine commit writeout logic

Signed-off-by Jan Kara <[email protected]>

---
 fs/ext4/fsync.c       |    2 +-
 fs/ext4/super.c       |    2 +-
 fs/jbd2/checkpoint.c  |    2 +-
 fs/jbd2/commit.c      |    4 ++--
 fs/jbd2/journal.c     |   19 ++++++++++---------
 fs/jbd2/transaction.c |   15 +++++++++------
 fs/ocfs2/aops.c       |    2 +-
 fs/ocfs2/super.c      |    2 +-
 include/linux/jbd2.h  |   18 ++++++++++--------
 9 files changed, 36 insertions(+), 30 deletions(-)

fs/ext4/fsync.c +1 -1

index 7829b28..19434da 100644
@@ -198,7 +198,7 @@ int ext4_sync_file(struct file *file, int datasync)
 		return ext4_force_commit(inode->i_sb);
 
 	commit_tid = datasync ? ei->i_datasync_tid : ei->i_sync_tid;
-	if (jbd2_log_start_commit(journal, commit_tid)) {
+	if (jbd2_log_start_commit(journal, commit_tid, true)) {
 		/*
 		 * When the journal is on a different device than the
 		 * fs data disk, we need to issue the barrier in

fs/ext4/super.c +1 -1

index f6a318f..bfce0d6 100644
@@ -4115,7 +4115,7 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
 
 	trace_ext4_sync_fs(sb, wait);
 	flush_workqueue(sbi->dio_unwritten_wq);
-	if (jbd2_journal_start_commit(sbi->s_journal, &target)) {
+	if (jbd2_journal_start_commit(sbi->s_journal, &target, true)) {
 		if (wait)
 			jbd2_log_wait_commit(sbi->s_journal, target);
 	}

fs/jbd2/checkpoint.c +1 -1

index 6a79fd0..3436d53 100644
@@ -309,7 +309,7 @@ static int __process_buffer(journal_t *journal, struct journal_head *jh,
 			       "Waiting for Godot: block %llu\n",
 			       journal->j_devname,
 			       (unsigned long long) bh->b_blocknr);
-		jbd2_log_start_commit(journal, tid);
+		jbd2_log_start_commit(journal, tid, true);
 		jbd2_log_wait_commit(journal, tid);
 		ret = 1;
 	} else if (!buffer_dirty(bh)) {

fs/jbd2/commit.c +2 -2

index f3ad159..19973eb 100644
@@ -329,7 +329,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	int tag_bytes = journal_tag_bytes(journal);
 	struct buffer_head *cbh = NULL; /* For transactional checksums */
 	__u32 crc32_sum = ~0;
-	int write_op = WRITE_SYNC;
+	int write_op = WRITE;
 
 	/*
 	 * First job: lock down the current transaction and wait for

@@ -368,7 +368,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	 * we unplug the device. We don't do explicit unplugging in here,
 	 * instead we rely on sync_buffer() doing the unplug for us.
 	 */
-	if (commit_transaction->t_synchronous_commit)
+	if (tid_geq(journal->j_commit_waited, commit_transaction->t_tid))
 		write_op = WRITE_SYNC_PLUG;
 	trace_jbd2_commit_locking(journal, commit_transaction);
 	stats.run.rs_wait = commit_transaction->t_max_wait;

fs/jbd2/journal.c +10 -9

index 97e7346..04835d6 100644
@@ -476,7 +476,7 @@ int __jbd2_log_space_left(journal_t *journal)
  * Called with j_state_lock locked for writing.
  * Returns true if a transaction commit was started.
  */
-int __jbd2_log_start_commit(journal_t *journal, tid_t target)
+int __jbd2_log_start_commit(journal_t *journal, tid_t target, bool will_wait)
 {
 	/*
 	 * Are we already doing a recent enough commit?

@@ -486,7 +486,8 @@ int __jbd2_log_start_commit(journal_t *journal, tid_t target)
 		 * We want a new commit: OK, mark the request and wakeup the
 		 * commit thread.  We do _not_ do the commit ourselves.
 		 */
-
+		if (will_wait && !tid_geq(journal->j_commit_waited, target))
+			journal->j_commit_waited = target;
 		journal->j_commit_request = target;
 		jbd_debug(1, "JBD: requesting commit %d/%d\n",
 			  journal->j_commit_request,

@@ -497,12 +498,12 @@ int __jbd2_log_start_commit(journal_t *journal, tid_t target)
 	return 0;
 }
 
-int jbd2_log_start_commit(journal_t *journal, tid_t tid)
+int jbd2_log_start_commit(journal_t *journal, tid_t tid, bool will_wait)
 {
 	int ret;
 
 	write_lock(&journal->j_state_lock);
-	ret = __jbd2_log_start_commit(journal, tid);
+	ret = __jbd2_log_start_commit(journal, tid, will_wait);
 	write_unlock(&journal->j_state_lock);
 	return ret;
 }

@@ -539,7 +540,7 @@ int jbd2_journal_force_commit_nested(journal_t *journal)
 	tid = transaction->t_tid;
 	read_unlock(&journal->j_state_lock);
 	if (need_to_start)
-		jbd2_log_start_commit(journal, tid);
+		jbd2_log_start_commit(journal, tid, true);
 	jbd2_log_wait_commit(journal, tid);
 	return 1;
 }

@@ -549,7 +550,7 @@ int jbd2_journal_force_commit_nested(journal_t *journal)
  * if a transaction is going to be committed (or is currently already
  * committing), and fills its tid in at *ptid
  */
-int jbd2_journal_start_commit(journal_t *journal, tid_t *ptid)
+int jbd2_journal_start_commit(journal_t *journal, tid_t *ptid, bool will_wait)
 {
 	int ret = 0;

@@ -557,7 +558,7 @@ int jbd2_journal_start_commit(journal_t *journal, tid_t *ptid)
 	if (journal->j_running_transaction) {
 		tid_t tid = journal->j_running_transaction->t_tid;
 
-		__jbd2_log_start_commit(journal, tid);
+		__jbd2_log_start_commit(journal, tid, will_wait);
 		/* There's a running transaction and we've just made sure
 		 * it's commit has been scheduled. */
 		if (ptid)

@@ -1564,7 +1565,7 @@ int jbd2_journal_flush(journal_t *journal)
 	/* Force everything buffered to the log... */
 	if (journal->j_running_transaction) {
 		transaction = journal->j_running_transaction;
-		__jbd2_log_start_commit(journal, transaction->t_tid);
+		__jbd2_log_start_commit(journal, transaction->t_tid, true);
 	} else if (journal->j_committing_transaction)
 		transaction = journal->j_committing_transaction;

@@ -1680,7 +1681,7 @@ void __jbd2_journal_abort_hard(journal_t *journal)
 	journal->j_flags |= JBD2_ABORT;
 	transaction = journal->j_running_transaction;
 	if (transaction)
-		__jbd2_log_start_commit(journal, transaction->t_tid);
+		__jbd2_log_start_commit(journal, transaction->t_tid, false);
 	write_unlock(&journal->j_state_lock);
 }

fs/jbd2/transaction.c +9 -6

index 1d11910..340dd5e 100644
@@ -226,7 +226,7 @@ repeat:
 		need_to_start = !tid_geq(journal->j_commit_request, tid);
 		read_unlock(&journal->j_state_lock);
 		if (need_to_start)
-			jbd2_log_start_commit(journal, tid);
+			jbd2_log_start_commit(journal, tid, false);
 		schedule();
 		finish_wait(&journal->j_wait_transaction_locked, &wait);
 		goto repeat;

@@ -469,8 +469,12 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, int gfp_mask)
 	spin_unlock(&transaction->t_handle_lock);
 
 	jbd_debug(2, "restarting handle %p\n", handle);
+<<<<<<< HEAD
 	tid = transaction->t_tid;
 	need_to_start = !tid_geq(journal->j_commit_request, tid);
+=======
+	__jbd2_log_start_commit(journal, transaction->t_tid, false);
+>>>>>>> jbd2: Refine commit writeout logic
 	read_unlock(&journal->j_state_lock);
 	if (need_to_start)
 		jbd2_log_start_commit(journal, tid);

@@ -1368,8 +1372,6 @@ int jbd2_journal_stop(handle_t *handle)
 		}
 	}
 
-	if (handle->h_sync)
-		transaction->t_synchronous_commit = 1;
 	current->journal_info = NULL;
 	atomic_sub(handle->h_buffer_credits,
 		   &transaction->t_outstanding_credits);

@@ -1390,15 +1392,16 @@ int jbd2_journal_stop(handle_t *handle)
 
 		jbd_debug(2, "transaction too old, requesting commit for "
 					"handle %p\n", handle);
-		/* This is non-blocking */
-		jbd2_log_start_commit(journal, transaction->t_tid);
-
 		/*
 		 * Special case: JBD2_SYNC synchronous updates require us
 		 * to wait for the commit to complete.
 		 */
 		if (handle->h_sync && !(current->flags & PF_MEMALLOC))
 			wait_for_commit = 1;
+
+		/* This is non-blocking */
+		jbd2_log_start_commit(journal, transaction->t_tid,
+				      wait_for_commit);
 	}
 
 	/*

fs/ocfs2/aops.c +1 -1

index 1fbb0e2..d493f32 100644
@@ -1659,7 +1659,7 @@ static int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
 		goto out;
 	}
 
-	if (jbd2_journal_start_commit(osb->journal->j_journal, &target)) {
+	if (jbd2_journal_start_commit(osb->journal->j_journal, &target, true)) {
 		jbd2_log_wait_commit(osb->journal->j_journal, target);
 		ret = 1;
 	}

fs/ocfs2/super.c +1 -1

index 38f986d..45d9f82 100644
@@ -414,7 +414,7 @@ static int ocfs2_sync_fs(struct super_block *sb, int wait)
 	}
 
 	if (jbd2_journal_start_commit(OCFS2_SB(sb)->journal->j_journal,
-				      &target)) {
+				      &target, wait)) {
 		if (wait)
 			jbd2_log_wait_commit(OCFS2_SB(sb)->journal->j_journal,
 					     target);

include/linux/jbd2.h +10 -8

index 27e79c2..46aaf45 100644
@@ -631,11 +631,6 @@ struct transaction_s
 	 */
 	atomic_t		t_handle_count;
 
-	/*
-	 * This transaction is being forced and some process is
-	 * waiting for it to finish.
-	 */
-	unsigned int t_synchronous_commit:1;
 	unsigned int t_flushed_data_blocks:1;
 
 	/*

@@ -900,6 +895,13 @@ struct journal_s
 	tid_t			j_commit_request;
 
 	/*
+	 * Sequence number of the most recent transaction someone is waiting
+	 * for to commit.
+	 * [j_state_lock]
+	 */
+	tid_t			j_commit_waited;
+
+	/*
 	 * Journal uuid: identifies the object (filesystem, LVM volume etc)
 	 * backed by this journal.  This will eventually be replaced by an array
 	 * of uuids, allowing us to index multiple devices within a single

@@ -1200,9 +1202,9 @@ extern void	jbd2_journal_switch_revoke_table(journal_t *journal);
  */
 
 int __jbd2_log_space_left(journal_t *); /* Called with journal locked */
-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_start_commit(journal_t *journal, tid_t tid, bool will_wait);
+int __jbd2_log_start_commit(journal_t *journal, tid_t tid, bool will_wait);
+int jbd2_journal_start_commit(journal_t *journal, tid_t *tid, bool will_wait);
 int jbd2_journal_force_commit_nested(journal_t *journal);
 int jbd2_log_wait_commit(journal_t *journal, tid_t tid);
 int jbd2_log_do_checkpoint(journal_t *journal);

Signature · 1 line

1.7.1

Quoted 1 line

From 3e4f42155513df7b6d36584e7b8383b86f425467 Mon Sep 17 00:00:00 2001

From: Jan Kara <[email protected]>
Date: Mon, 21 Feb 2011 17:25:37 +0100
Subject: [PATCH 2/2] jbd: Refine commit writeout logic

Signed-off-by Jan Kara <[email protected]>

---
 fs/ext3/fsync.c      |    2 +-
 fs/ext3/super.c      |    2 +-
 fs/jbd/checkpoint.c  |    2 +-
 fs/jbd/commit.c      |    4 ++--
 fs/jbd/journal.c     |   19 ++++++++++---------
 fs/jbd/transaction.c |    9 ++++-----
 include/linux/jbd.h  |   21 ++++++++++++---------
 7 files changed, 31 insertions(+), 28 deletions(-)

fs/ext3/fsync.c +1 -1

index 09b13bb..5396dd6 100644
@@ -81,7 +81,7 @@ int ext3_sync_file(struct file *file, int datasync)
 	if (test_opt(inode->i_sb, BARRIER) &&
 	    !journal_trans_will_send_data_barrier(journal, commit_tid))
 		needs_barrier = 1;
-	log_start_commit(journal, commit_tid);
+	log_start_commit(journal, commit_tid, true);
 	ret = log_wait_commit(journal, commit_tid);
 
 	/*

fs/ext3/super.c +1 -1

index 85c8cc8..58a4424 100644
@@ -2497,7 +2497,7 @@ static int ext3_sync_fs(struct super_block *sb, int wait)
 {
 	tid_t target;
 
-	if (journal_start_commit(EXT3_SB(sb)->s_journal, &target)) {
+	if (journal_start_commit(EXT3_SB(sb)->s_journal, &target, true)) {
 		if (wait)
 			log_wait_commit(EXT3_SB(sb)->s_journal, target);
 	}

fs/jbd/checkpoint.c +1 -1

index e4b87bc..26ca2c3 100644
@@ -297,7 +297,7 @@ static int __process_buffer(journal_t *journal, struct journal_head *jh,
 
 		spin_unlock(&journal->j_list_lock);
 		jbd_unlock_bh_state(bh);
-		log_start_commit(journal, tid);
+		log_start_commit(journal, tid, true);
 		log_wait_commit(journal, tid);
 		ret = 1;
 	} else if (!buffer_dirty(bh)) {

fs/jbd/commit.c +2 -2

index 34a4861..ac188cb 100644
@@ -294,7 +294,7 @@ void journal_commit_transaction(journal_t *journal)
 	int first_tag = 0;
 	int tag_flag;
 	int i;
-	int write_op = WRITE_SYNC;
+	int write_op = WRITE;
 
 	/*
 	 * First job: lock down the current transaction and wait for

@@ -332,7 +332,7 @@ void journal_commit_transaction(journal_t *journal)
 	 * we unplug the device. We don't do explicit unplugging in here,
 	 * instead we rely on sync_buffer() doing the unplug for us.
 	 */
-	if (commit_transaction->t_synchronous_commit)
+	if (tid_geq(journal->j_commit_waited, commit_transaction->t_tid))
 		write_op = WRITE_SYNC_PLUG;
 	spin_lock(&commit_transaction->t_handle_lock);
 	while (commit_transaction->t_updates) {

fs/jbd/journal.c +10 -9

index da1b5e4..5743b4c 100644
@@ -434,7 +434,7 @@ int __log_space_left(journal_t *journal)
 /*
  * Called under j_state_lock.  Returns true if a transaction commit was started.
  */
-int __log_start_commit(journal_t *journal, tid_t target)
+int __log_start_commit(journal_t *journal, tid_t target, bool will_wait)
 {
 	/*
 	 * Are we already doing a recent enough commit?

@@ -444,7 +444,8 @@ int __log_start_commit(journal_t *journal, tid_t target)
 		 * We want a new commit: OK, mark the request and wakeup the
 		 * commit thread.  We do _not_ do the commit ourselves.
 		 */
-
+		if (will_wait && !tid_geq(journal->j_commit_waited, target))
+			journal->j_commit_waited = target;
 		journal->j_commit_request = target;
 		jbd_debug(1, "JBD: requesting commit %d/%d\n",
 			  journal->j_commit_request,

@@ -455,12 +456,12 @@ int __log_start_commit(journal_t *journal, tid_t target)
 	return 0;
 }
 
-int log_start_commit(journal_t *journal, tid_t tid)
+int log_start_commit(journal_t *journal, tid_t tid, bool will_wait)
 {
 	int ret;
 
 	spin_lock(&journal->j_state_lock);
-	ret = __log_start_commit(journal, tid);
+	ret = __log_start_commit(journal, tid, will_wait);
 	spin_unlock(&journal->j_state_lock);
 	return ret;
 }

@@ -483,7 +484,7 @@ int journal_force_commit_nested(journal_t *journal)
 	spin_lock(&journal->j_state_lock);
 	if (journal->j_running_transaction && !current->journal_info) {
 		transaction = journal->j_running_transaction;
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid, true);
 	} else if (journal->j_committing_transaction)
 		transaction = journal->j_committing_transaction;

@@ -503,7 +504,7 @@ int journal_force_commit_nested(journal_t *journal)
  * if a transaction is going to be committed (or is currently already
  * committing), and fills its tid in at *ptid
  */
-int journal_start_commit(journal_t *journal, tid_t *ptid)
+int journal_start_commit(journal_t *journal, tid_t *ptid, bool will_wait)
 {
 	int ret = 0;

@@ -511,7 +512,7 @@ int journal_start_commit(journal_t *journal, tid_t *ptid)
 	if (journal->j_running_transaction) {
 		tid_t tid = journal->j_running_transaction->t_tid;
 
-		__log_start_commit(journal, tid);
+		__log_start_commit(journal, tid, will_wait);
 		/* There's a running transaction and we've just made sure
 		 * it's commit has been scheduled. */
 		if (ptid)

@@ -1439,7 +1440,7 @@ int journal_flush(journal_t *journal)
 	/* Force everything buffered to the log... */
 	if (journal->j_running_transaction) {
 		transaction = journal->j_running_transaction;
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid, true);
 	} else if (journal->j_committing_transaction)
 		transaction = journal->j_committing_transaction;

@@ -1573,7 +1574,7 @@ static void __journal_abort_hard(journal_t *journal)
 	journal->j_flags |= JFS_ABORT;
 	transaction = journal->j_running_transaction;
 	if (transaction)
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid, false);
 	spin_unlock(&journal->j_state_lock);
 }

fs/jbd/transaction.c +4 -5

index 5b2e4c3..a12c0a3 100644
@@ -178,7 +178,7 @@ repeat_locked:
 		spin_unlock(&transaction->t_handle_lock);
 		prepare_to_wait(&journal->j_wait_transaction_locked, &wait,
 				TASK_UNINTERRUPTIBLE);
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid, false);
 		spin_unlock(&journal->j_state_lock);
 		schedule();
 		finish_wait(&journal->j_wait_transaction_locked, &wait);

@@ -411,7 +411,7 @@ int journal_restart(handle_t *handle, int nblocks)
 	spin_unlock(&transaction->t_handle_lock);
 
 	jbd_debug(2, "restarting handle %p\n", handle);
-	__log_start_commit(journal, transaction->t_tid);
+	__log_start_commit(journal, transaction->t_tid, false);
 	spin_unlock(&journal->j_state_lock);
 
 	lock_map_release(&handle->h_lockdep_map);

@@ -1431,8 +1431,6 @@ int journal_stop(handle_t *handle)
 		}
 	}
 
-	if (handle->h_sync)
-		transaction->t_synchronous_commit = 1;
 	current->journal_info = NULL;
 	spin_lock(&journal->j_state_lock);
 	spin_lock(&transaction->t_handle_lock);

@@ -1463,7 +1461,8 @@ int journal_stop(handle_t *handle)
 		jbd_debug(2, "transaction too old, requesting commit for "
 					"handle %p\n", handle);
 		/* This is non-blocking */
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid,
+			handle->h_sync && !(current->flags & PF_MEMALLOC));
 		spin_unlock(&journal->j_state_lock);
 
 		/*

include/linux/jbd.h +12 -9

index e069650..c38f73e 100644
@@ -541,12 +541,6 @@ struct transaction_s
 	 * How many handles used this transaction? [t_handle_lock]
 	 */
 	int t_handle_count;
-
-	/*
-	 * This transaction is being forced and some process is
-	 * waiting for it to finish.
-	 */
-	unsigned int t_synchronous_commit:1;
 };
 
 /**

@@ -594,6 +588,8 @@ struct transaction_s
  *  transaction
  * @j_commit_request: Sequence number of the most recent transaction wanting
  *     commit
+ * @j_commit_waited: Sequence number of the most recent transaction someone
+ *     is waiting for to commit.
  * @j_uuid: Uuid of client object.
  * @j_task: Pointer to the current commit thread for this journal
  * @j_max_transaction_buffers:  Maximum number of metadata buffers to allow in a

@@ -762,6 +758,13 @@ struct journal_s
 	tid_t			j_commit_request;
 
 	/*
+	 * Sequence number of the most recent transaction someone is waiting
+	 * for to commit.
+	 * [j_state_lock]
+	 */
+	tid_t                   j_commit_waited;
+
+	/*
 	 * Journal uuid: identifies the object (filesystem, LVM volume etc)
 	 * backed by this journal.  This will eventually be replaced by an array
 	 * of uuids, allowing us to index multiple devices within a single

@@ -985,9 +988,9 @@ extern void	journal_switch_revoke_table(journal_t *journal);
  */
 
 int __log_space_left(journal_t *); /* Called with journal locked */
-int log_start_commit(journal_t *journal, tid_t tid);
-int __log_start_commit(journal_t *journal, tid_t tid);
-int journal_start_commit(journal_t *journal, tid_t *tid);
+int log_start_commit(journal_t *journal, tid_t tid, bool will_wait);
+int __log_start_commit(journal_t *journal, tid_t tid, bool will_wait);
+int journal_start_commit(journal_t *journal, tid_t *tid, bool will_wait);
 int journal_force_commit_nested(journal_t *journal);
 int log_wait_commit(journal_t *journal, tid_t tid);
 int log_do_checkpoint(journal_t *journal);

Signature · 1 line

1.7.1

Alex Shi

author

2011-02-25 00:52 UTC #14 lore raw

Quoted 8 lines · Jan Kara

Quoted 4 lines · Alex,Shi
Though these patches can not totally recovered the problem, but they are quite helpful with ccache enabled situation. It increase 10% performance on 38-rc1 kernel.
OK and what was the original performance drop with WRITE_SYNC change?

The original drop is 30%.

Quoted 10 lines

Quoted 3 lines
I have tried to enabled they to latest rc6 kernel but failed. the vmstat output is here: with patches:
I'm attaching patches rebased on top of latest Linus's tree.
Corrado, could you possibly run your fsync-heavy tests so that we see whether there isn't negative impact of my patches on your fsync-heavy workload? Thanks.
Honza

Corrado Zoccolo

2011-02-26 14:45 UTC #15 lore raw

In reply to Jan Kara

Quoted 4 lines · Jan Kara

Quoted 3 lines · Alex,Shi
Though these patches can not totally recovered the problem, but they are quite helpful with ccache enabled situation. It increase 10% performance on 38-rc1 kernel.

Quoted 8 lines

OK and what was the original performance drop with WRITE_SYNC change?
Quoted 2 lines
I have tried to enabled they to latest rc6 kernel but failed. the vmstat output is here:
with patches:
I'm attaching patches rebased on top of latest Linus's tree. Corrado, could you possibly run your fsync-heavy tests so that we see whether there isn't negative impact of my patches on your fsync-heavy workload? Thanks.

The workload was actually Jeff's, and the stalls that my change tried to mitigate showed up on his enterprise class storage. Adding him so he can test it.

Corrado

Quoted 5 lines

Honza --
Jan Kara <[email protected]>
SUSE Labs, CR

Signature · 10 lines

__________________________________________________________________________

dott. Corrado Zoccolo                          mailto:[email protected]
PhD - Department of Computer Science - University of Pisa, Italy
--------------------------------------------------------------------------
The self-confidence of a warrior is not the self-confidence of the average
man. The average man seeks certainty in the eyes of the onlooker and calls
that self-confidence. The warrior seeks impeccability in his own eyes and
calls that humbleness.
                               Tales of Power - C. Castaneda

Jeff Moyer

2011-03-01 19:56 UTC #16 lore raw

Quoted 9 lines · Corrado Zoccolo

Quoted 8 lines · Jan Kara
Quoted 3 lines · Alex,Shi
Though these patches can not totally recovered the problem, but they are quite helpful with ccache enabled situation. It increase 10% performance on 38-rc1 kernel.
OK and what was the original performance drop with WRITE_SYNC change?
Quoted 2 lines
I have tried to enabled they to latest rc6 kernel but failed. the vmstat output is here:
with patches:

Quoted 7 lines

Quoted 4 lines
I'm attaching patches rebased on top of latest Linus's tree. Corrado, could you possibly run your fsync-heavy tests so that we see whether there isn't negative impact of my patches on your fsync-heavy workload? Thanks.
The workload was actually Jeff's, and the stalls that my change tried to mitigate showed up on his enterprise class storage. Adding him so he can test it.

Sorry for the late reply. You can use either fs_mark or iozone to generate an fsync-heavy workload. The test I did was to mix this with a sequential reader. If you can point me at patches, I should be able to test this.

Cheers,
Jeff

Jan Kara

reviewer

2011-03-02 09:42 UTC #17 lore raw

Quoted 15 lines · Jeff Moyer

Quoted 13 lines · Corrado Zoccolo
Quoted 12 lines · Jan Kara
Quoted 3 lines · Alex,Shi
Though these patches can not totally recovered the problem, but they are quite helpful with ccache enabled situation. It increase 10% performance on 38-rc1 kernel.
OK and what was the original performance drop with WRITE_SYNC change?
Quoted 2 lines
I have tried to enabled they to latest rc6 kernel but failed. the vmstat output is here:
with patches:
I'm attaching patches rebased on top of latest Linus's tree. Corrado, could you possibly run your fsync-heavy tests so that we see whether there isn't negative impact of my patches on your fsync-heavy workload? Thanks.

Quoted 8 lines

Quoted 3 lines
The workload was actually Jeff's, and the stalls that my change tried to mitigate showed up on his enterprise class storage. Adding him so he can test it.
Sorry for the late reply. You can use either fs_mark or iozone to generate an fsync-heavy workload. The test I did was to mix this with a sequential reader. If you can point me at patches, I should be able to test this.

The latest version of patches is attached to:
https://lkml.org/lkml/2011/2/24/125

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Jeff Moyer

2011-03-02 16:14 UTC #18 lore raw

Quoted 20 lines · Jan Kara

Quoted 19 lines · Jeff Moyer
Quoted 16 lines · Corrado Zoccolo
Quoted 4 lines · Jan Kara
Quoted 3 lines · Alex,Shi
Though these patches can not totally recovered the problem, but they are quite helpful with ccache enabled situation. It increase 10% performance on 38-rc1 kernel.
Quoted 8 lines
OK and what was the original performance drop with WRITE_SYNC change?
Quoted 2 lines
I have tried to enabled they to latest rc6 kernel but failed. the vmstat output is here:
with patches:
I'm attaching patches rebased on top of latest Linus's tree. Corrado, could you possibly run your fsync-heavy tests so that we see whether there isn't negative impact of my patches on your fsync-heavy workload? Thanks.
The workload was actually Jeff's, and the stalls that my change tried to mitigate showed up on his enterprise class storage. Adding him so he can test it.

Quoted 6 lines

Quoted 4 lines
Sorry for the late reply. You can use either fs_mark or iozone to generate an fsync-heavy workload. The test I did was to mix this with a sequential reader. If you can point me at patches, I should be able to test this.
The latest version of patches is attached to:
https://lkml.org/lkml/2011/2/24/125

Perhaps you should fix up the merge conflicts, first? ;-)

+<<<<<<< HEAD

        tid = transaction->t_tid;
        need_to_start = !tid_geq(journal->j_commit_request, tid);
+=======
+       __jbd2_log_start_commit(journal, transaction->t_tid, false);

+>>>>>>> jbd2: Refine commit writeout logic

Jan Kara

reviewer

2011-03-02 21:17 UTC #19 lore raw

Quoted 28 lines · Jeff Moyer

Quoted 20 lines · Jan Kara
Quoted 19 lines · Jeff Moyer
Quoted 16 lines · Corrado Zoccolo
Quoted 4 lines · Jan Kara
Quoted 3 lines · Alex,Shi
Though these patches can not totally recovered the problem, but they are quite helpful with ccache enabled situation. It increase 10% performance on 38-rc1 kernel.
Quoted 8 lines
OK and what was the original performance drop with WRITE_SYNC change?
Quoted 2 lines
I have tried to enabled they to latest rc6 kernel but failed. the vmstat output is here:
with patches:
I'm attaching patches rebased on top of latest Linus's tree. Corrado, could you possibly run your fsync-heavy tests so that we see whether there isn't negative impact of my patches on your fsync-heavy workload? Thanks.
The workload was actually Jeff's, and the stalls that my change tried to mitigate showed up on his enterprise class storage. Adding him so he can test it.
Quoted 6 lines
Quoted 4 lines
Sorry for the late reply. You can use either fs_mark or iozone to generate an fsync-heavy workload. The test I did was to mix this with a sequential reader. If you can point me at patches, I should be able to test this.
The latest version of patches is attached to:
https://lkml.org/lkml/2011/2/24/125

Quoted 8 lines

Perhaps you should fix up the merge conflicts, first? ;-)
+<<<<<<< HEAD
        tid = transaction->t_tid;
        need_to_start = !tid_geq(journal->j_commit_request, tid);
+=======
+       __jbd2_log_start_commit(journal, transaction->t_tid, false);
+>>>>>>> jbd2: Refine commit writeout logic

Doh, how embarrassing ;). Attached is a new version which compiles and
seems to run OK.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 1 line

From 2d4018f7fe647a440f488d71cf4ceffccaecb02e Mon Sep 17 00:00:00 2001

From: Jan Kara <[email protected]>
Date: Wed, 19 Jan 2011 13:45:04 +0100
Subject: [PATCH 1/2] jbd2: Refine commit writeout logic

Signed-off-by Jan Kara <[email protected]>

---
 fs/ext4/fsync.c       |    2 +-
 fs/ext4/super.c       |    2 +-
 fs/jbd2/checkpoint.c  |    2 +-
 fs/jbd2/commit.c      |    4 ++--
 fs/jbd2/journal.c     |   19 ++++++++++---------
 fs/jbd2/transaction.c |   13 ++++++-------
 fs/ocfs2/aops.c       |    2 +-
 fs/ocfs2/super.c      |    2 +-
 include/linux/jbd2.h  |   18 ++++++++++--------
 9 files changed, 33 insertions(+), 31 deletions(-)

fs/ext4/fsync.c +1 -1

index 7829b28..19434da 100644
@@ -198,7 +198,7 @@ int ext4_sync_file(struct file *file, int datasync)
 		return ext4_force_commit(inode->i_sb);
 
 	commit_tid = datasync ? ei->i_datasync_tid : ei->i_sync_tid;
-	if (jbd2_log_start_commit(journal, commit_tid)) {
+	if (jbd2_log_start_commit(journal, commit_tid, true)) {
 		/*
 		 * When the journal is on a different device than the
 		 * fs data disk, we need to issue the barrier in

fs/ext4/super.c +1 -1

index f6a318f..bfce0d6 100644
@@ -4115,7 +4115,7 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
 
 	trace_ext4_sync_fs(sb, wait);
 	flush_workqueue(sbi->dio_unwritten_wq);
-	if (jbd2_journal_start_commit(sbi->s_journal, &target)) {
+	if (jbd2_journal_start_commit(sbi->s_journal, &target, true)) {
 		if (wait)
 			jbd2_log_wait_commit(sbi->s_journal, target);
 	}

fs/jbd2/checkpoint.c +1 -1

index 6a79fd0..3436d53 100644
@@ -309,7 +309,7 @@ static int __process_buffer(journal_t *journal, struct journal_head *jh,
 			       "Waiting for Godot: block %llu\n",
 			       journal->j_devname,
 			       (unsigned long long) bh->b_blocknr);
-		jbd2_log_start_commit(journal, tid);
+		jbd2_log_start_commit(journal, tid, true);
 		jbd2_log_wait_commit(journal, tid);
 		ret = 1;
 	} else if (!buffer_dirty(bh)) {

fs/jbd2/commit.c +2 -2

index f3ad159..19973eb 100644
@@ -329,7 +329,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	int tag_bytes = journal_tag_bytes(journal);
 	struct buffer_head *cbh = NULL; /* For transactional checksums */
 	__u32 crc32_sum = ~0;
-	int write_op = WRITE_SYNC;
+	int write_op = WRITE;
 
 	/*
 	 * First job: lock down the current transaction and wait for

@@ -368,7 +368,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	 * we unplug the device. We don't do explicit unplugging in here,
 	 * instead we rely on sync_buffer() doing the unplug for us.
 	 */
-	if (commit_transaction->t_synchronous_commit)
+	if (tid_geq(journal->j_commit_waited, commit_transaction->t_tid))
 		write_op = WRITE_SYNC_PLUG;
 	trace_jbd2_commit_locking(journal, commit_transaction);
 	stats.run.rs_wait = commit_transaction->t_max_wait;

fs/jbd2/journal.c +10 -9

index 97e7346..04835d6 100644
@@ -476,7 +476,7 @@ int __jbd2_log_space_left(journal_t *journal)
  * Called with j_state_lock locked for writing.
  * Returns true if a transaction commit was started.
  */
-int __jbd2_log_start_commit(journal_t *journal, tid_t target)
+int __jbd2_log_start_commit(journal_t *journal, tid_t target, bool will_wait)
 {
 	/*
 	 * Are we already doing a recent enough commit?

@@ -486,7 +486,8 @@ int __jbd2_log_start_commit(journal_t *journal, tid_t target)
 		 * We want a new commit: OK, mark the request and wakeup the
 		 * commit thread.  We do _not_ do the commit ourselves.
 		 */
-
+		if (will_wait && !tid_geq(journal->j_commit_waited, target))
+			journal->j_commit_waited = target;
 		journal->j_commit_request = target;
 		jbd_debug(1, "JBD: requesting commit %d/%d\n",
 			  journal->j_commit_request,

@@ -497,12 +498,12 @@ int __jbd2_log_start_commit(journal_t *journal, tid_t target)
 	return 0;
 }
 
-int jbd2_log_start_commit(journal_t *journal, tid_t tid)
+int jbd2_log_start_commit(journal_t *journal, tid_t tid, bool will_wait)
 {
 	int ret;
 
 	write_lock(&journal->j_state_lock);
-	ret = __jbd2_log_start_commit(journal, tid);
+	ret = __jbd2_log_start_commit(journal, tid, will_wait);
 	write_unlock(&journal->j_state_lock);
 	return ret;
 }

@@ -539,7 +540,7 @@ int jbd2_journal_force_commit_nested(journal_t *journal)
 	tid = transaction->t_tid;
 	read_unlock(&journal->j_state_lock);
 	if (need_to_start)
-		jbd2_log_start_commit(journal, tid);
+		jbd2_log_start_commit(journal, tid, true);
 	jbd2_log_wait_commit(journal, tid);
 	return 1;
 }

@@ -549,7 +550,7 @@ int jbd2_journal_force_commit_nested(journal_t *journal)
  * if a transaction is going to be committed (or is currently already
  * committing), and fills its tid in at *ptid
  */
-int jbd2_journal_start_commit(journal_t *journal, tid_t *ptid)
+int jbd2_journal_start_commit(journal_t *journal, tid_t *ptid, bool will_wait)
 {
 	int ret = 0;

@@ -557,7 +558,7 @@ int jbd2_journal_start_commit(journal_t *journal, tid_t *ptid)
 	if (journal->j_running_transaction) {
 		tid_t tid = journal->j_running_transaction->t_tid;
 
-		__jbd2_log_start_commit(journal, tid);
+		__jbd2_log_start_commit(journal, tid, will_wait);
 		/* There's a running transaction and we've just made sure
 		 * it's commit has been scheduled. */
 		if (ptid)

@@ -1564,7 +1565,7 @@ int jbd2_journal_flush(journal_t *journal)
 	/* Force everything buffered to the log... */
 	if (journal->j_running_transaction) {
 		transaction = journal->j_running_transaction;
-		__jbd2_log_start_commit(journal, transaction->t_tid);
+		__jbd2_log_start_commit(journal, transaction->t_tid, true);
 	} else if (journal->j_committing_transaction)
 		transaction = journal->j_committing_transaction;

@@ -1680,7 +1681,7 @@ void __jbd2_journal_abort_hard(journal_t *journal)
 	journal->j_flags |= JBD2_ABORT;
 	transaction = journal->j_running_transaction;
 	if (transaction)
-		__jbd2_log_start_commit(journal, transaction->t_tid);
+		__jbd2_log_start_commit(journal, transaction->t_tid, false);
 	write_unlock(&journal->j_state_lock);
 }

fs/jbd2/transaction.c +6 -7

index 1d11910..2211dae 100644
@@ -226,7 +226,7 @@ repeat:
 		need_to_start = !tid_geq(journal->j_commit_request, tid);
 		read_unlock(&journal->j_state_lock);
 		if (need_to_start)
-			jbd2_log_start_commit(journal, tid);
+			jbd2_log_start_commit(journal, tid, false);
 		schedule();
 		finish_wait(&journal->j_wait_transaction_locked, &wait);
 		goto repeat;

@@ -473,7 +473,7 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, int gfp_mask)
 	need_to_start = !tid_geq(journal->j_commit_request, tid);
 	read_unlock(&journal->j_state_lock);
 	if (need_to_start)
-		jbd2_log_start_commit(journal, tid);
+		jbd2_log_start_commit(journal, tid, false);
 
 	lock_map_release(&handle->h_lockdep_map);
 	handle->h_buffer_credits = nblocks;

@@ -1368,8 +1368,6 @@ int jbd2_journal_stop(handle_t *handle)
 		}
 	}
 
-	if (handle->h_sync)
-		transaction->t_synchronous_commit = 1;
 	current->journal_info = NULL;
 	atomic_sub(handle->h_buffer_credits,
 		   &transaction->t_outstanding_credits);

@@ -1390,15 +1388,16 @@ int jbd2_journal_stop(handle_t *handle)
 
 		jbd_debug(2, "transaction too old, requesting commit for "
 					"handle %p\n", handle);
-		/* This is non-blocking */
-		jbd2_log_start_commit(journal, transaction->t_tid);
-
 		/*
 		 * Special case: JBD2_SYNC synchronous updates require us
 		 * to wait for the commit to complete.
 		 */
 		if (handle->h_sync && !(current->flags & PF_MEMALLOC))
 			wait_for_commit = 1;
+
+		/* This is non-blocking */
+		jbd2_log_start_commit(journal, transaction->t_tid,
+				      wait_for_commit);
 	}
 
 	/*

fs/ocfs2/aops.c +1 -1

index 1fbb0e2..d493f32 100644
@@ -1659,7 +1659,7 @@ static int ocfs2_try_to_free_truncate_log(struct ocfs2_super *osb,
 		goto out;
 	}
 
-	if (jbd2_journal_start_commit(osb->journal->j_journal, &target)) {
+	if (jbd2_journal_start_commit(osb->journal->j_journal, &target, true)) {
 		jbd2_log_wait_commit(osb->journal->j_journal, target);
 		ret = 1;
 	}

fs/ocfs2/super.c +1 -1

index 38f986d..45d9f82 100644
@@ -414,7 +414,7 @@ static int ocfs2_sync_fs(struct super_block *sb, int wait)
 	}
 
 	if (jbd2_journal_start_commit(OCFS2_SB(sb)->journal->j_journal,
-				      &target)) {
+				      &target, wait)) {
 		if (wait)
 			jbd2_log_wait_commit(OCFS2_SB(sb)->journal->j_journal,
 					     target);

include/linux/jbd2.h +10 -8

index 27e79c2..46aaf45 100644
@@ -631,11 +631,6 @@ struct transaction_s
 	 */
 	atomic_t		t_handle_count;
 
-	/*
-	 * This transaction is being forced and some process is
-	 * waiting for it to finish.
-	 */
-	unsigned int t_synchronous_commit:1;
 	unsigned int t_flushed_data_blocks:1;
 
 	/*

@@ -900,6 +895,13 @@ struct journal_s
 	tid_t			j_commit_request;
 
 	/*
+	 * Sequence number of the most recent transaction someone is waiting
+	 * for to commit.
+	 * [j_state_lock]
+	 */
+	tid_t			j_commit_waited;
+
+	/*
 	 * Journal uuid: identifies the object (filesystem, LVM volume etc)
 	 * backed by this journal.  This will eventually be replaced by an array
 	 * of uuids, allowing us to index multiple devices within a single

@@ -1200,9 +1202,9 @@ extern void	jbd2_journal_switch_revoke_table(journal_t *journal);
  */
 
 int __jbd2_log_space_left(journal_t *); /* Called with journal locked */
-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_start_commit(journal_t *journal, tid_t tid, bool will_wait);
+int __jbd2_log_start_commit(journal_t *journal, tid_t tid, bool will_wait);
+int jbd2_journal_start_commit(journal_t *journal, tid_t *tid, bool will_wait);
 int jbd2_journal_force_commit_nested(journal_t *journal);
 int jbd2_log_wait_commit(journal_t *journal, tid_t tid);
 int jbd2_log_do_checkpoint(journal_t *journal);

Signature · 1 line

1.7.1

Quoted 1 line

From 4334f23305e74c6b050c701d97803790dc04cbe0 Mon Sep 17 00:00:00 2001

From: Jan Kara <[email protected]>
Date: Mon, 21 Feb 2011 17:25:37 +0100
Subject: [PATCH 2/2] jbd: Refine commit writeout logic

Signed-off-by Jan Kara <[email protected]>

---
 fs/ext3/fsync.c      |    2 +-
 fs/ext3/super.c      |    2 +-
 fs/jbd/checkpoint.c  |    2 +-
 fs/jbd/commit.c      |    4 ++--
 fs/jbd/journal.c     |   19 ++++++++++---------
 fs/jbd/transaction.c |    9 ++++-----
 include/linux/jbd.h  |   21 ++++++++++++---------
 7 files changed, 31 insertions(+), 28 deletions(-)

fs/ext3/fsync.c +1 -1

index 09b13bb..5396dd6 100644
@@ -81,7 +81,7 @@ int ext3_sync_file(struct file *file, int datasync)
 	if (test_opt(inode->i_sb, BARRIER) &&
 	    !journal_trans_will_send_data_barrier(journal, commit_tid))
 		needs_barrier = 1;
-	log_start_commit(journal, commit_tid);
+	log_start_commit(journal, commit_tid, true);
 	ret = log_wait_commit(journal, commit_tid);
 
 	/*

fs/ext3/super.c +1 -1

index 85c8cc8..58a4424 100644
@@ -2497,7 +2497,7 @@ static int ext3_sync_fs(struct super_block *sb, int wait)
 {
 	tid_t target;
 
-	if (journal_start_commit(EXT3_SB(sb)->s_journal, &target)) {
+	if (journal_start_commit(EXT3_SB(sb)->s_journal, &target, true)) {
 		if (wait)
 			log_wait_commit(EXT3_SB(sb)->s_journal, target);
 	}

fs/jbd/checkpoint.c +1 -1

index e4b87bc..26ca2c3 100644
@@ -297,7 +297,7 @@ static int __process_buffer(journal_t *journal, struct journal_head *jh,
 
 		spin_unlock(&journal->j_list_lock);
 		jbd_unlock_bh_state(bh);
-		log_start_commit(journal, tid);
+		log_start_commit(journal, tid, true);
 		log_wait_commit(journal, tid);
 		ret = 1;
 	} else if (!buffer_dirty(bh)) {

fs/jbd/commit.c +2 -2

index 34a4861..ac188cb 100644
@@ -294,7 +294,7 @@ void journal_commit_transaction(journal_t *journal)
 	int first_tag = 0;
 	int tag_flag;
 	int i;
-	int write_op = WRITE_SYNC;
+	int write_op = WRITE;
 
 	/*
 	 * First job: lock down the current transaction and wait for

@@ -332,7 +332,7 @@ void journal_commit_transaction(journal_t *journal)
 	 * we unplug the device. We don't do explicit unplugging in here,
 	 * instead we rely on sync_buffer() doing the unplug for us.
 	 */
-	if (commit_transaction->t_synchronous_commit)
+	if (tid_geq(journal->j_commit_waited, commit_transaction->t_tid))
 		write_op = WRITE_SYNC_PLUG;
 	spin_lock(&commit_transaction->t_handle_lock);
 	while (commit_transaction->t_updates) {

fs/jbd/journal.c +10 -9

index da1b5e4..5743b4c 100644
@@ -434,7 +434,7 @@ int __log_space_left(journal_t *journal)
 /*
  * Called under j_state_lock.  Returns true if a transaction commit was started.
  */
-int __log_start_commit(journal_t *journal, tid_t target)
+int __log_start_commit(journal_t *journal, tid_t target, bool will_wait)
 {
 	/*
 	 * Are we already doing a recent enough commit?

@@ -444,7 +444,8 @@ int __log_start_commit(journal_t *journal, tid_t target)
 		 * We want a new commit: OK, mark the request and wakeup the
 		 * commit thread.  We do _not_ do the commit ourselves.
 		 */
-
+		if (will_wait && !tid_geq(journal->j_commit_waited, target))
+			journal->j_commit_waited = target;
 		journal->j_commit_request = target;
 		jbd_debug(1, "JBD: requesting commit %d/%d\n",
 			  journal->j_commit_request,

@@ -455,12 +456,12 @@ int __log_start_commit(journal_t *journal, tid_t target)
 	return 0;
 }
 
-int log_start_commit(journal_t *journal, tid_t tid)
+int log_start_commit(journal_t *journal, tid_t tid, bool will_wait)
 {
 	int ret;
 
 	spin_lock(&journal->j_state_lock);
-	ret = __log_start_commit(journal, tid);
+	ret = __log_start_commit(journal, tid, will_wait);
 	spin_unlock(&journal->j_state_lock);
 	return ret;
 }

@@ -483,7 +484,7 @@ int journal_force_commit_nested(journal_t *journal)
 	spin_lock(&journal->j_state_lock);
 	if (journal->j_running_transaction && !current->journal_info) {
 		transaction = journal->j_running_transaction;
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid, true);
 	} else if (journal->j_committing_transaction)
 		transaction = journal->j_committing_transaction;

@@ -503,7 +504,7 @@ int journal_force_commit_nested(journal_t *journal)
  * if a transaction is going to be committed (or is currently already
  * committing), and fills its tid in at *ptid
  */
-int journal_start_commit(journal_t *journal, tid_t *ptid)
+int journal_start_commit(journal_t *journal, tid_t *ptid, bool will_wait)
 {
 	int ret = 0;

@@ -511,7 +512,7 @@ int journal_start_commit(journal_t *journal, tid_t *ptid)
 	if (journal->j_running_transaction) {
 		tid_t tid = journal->j_running_transaction->t_tid;
 
-		__log_start_commit(journal, tid);
+		__log_start_commit(journal, tid, will_wait);
 		/* There's a running transaction and we've just made sure
 		 * it's commit has been scheduled. */
 		if (ptid)

@@ -1439,7 +1440,7 @@ int journal_flush(journal_t *journal)
 	/* Force everything buffered to the log... */
 	if (journal->j_running_transaction) {
 		transaction = journal->j_running_transaction;
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid, true);
 	} else if (journal->j_committing_transaction)
 		transaction = journal->j_committing_transaction;

@@ -1573,7 +1574,7 @@ static void __journal_abort_hard(journal_t *journal)
 	journal->j_flags |= JFS_ABORT;
 	transaction = journal->j_running_transaction;
 	if (transaction)
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid, false);
 	spin_unlock(&journal->j_state_lock);
 }

fs/jbd/transaction.c +4 -5

index 5b2e4c3..a12c0a3 100644
@@ -178,7 +178,7 @@ repeat_locked:
 		spin_unlock(&transaction->t_handle_lock);
 		prepare_to_wait(&journal->j_wait_transaction_locked, &wait,
 				TASK_UNINTERRUPTIBLE);
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid, false);
 		spin_unlock(&journal->j_state_lock);
 		schedule();
 		finish_wait(&journal->j_wait_transaction_locked, &wait);

@@ -411,7 +411,7 @@ int journal_restart(handle_t *handle, int nblocks)
 	spin_unlock(&transaction->t_handle_lock);
 
 	jbd_debug(2, "restarting handle %p\n", handle);
-	__log_start_commit(journal, transaction->t_tid);
+	__log_start_commit(journal, transaction->t_tid, false);
 	spin_unlock(&journal->j_state_lock);
 
 	lock_map_release(&handle->h_lockdep_map);

@@ -1431,8 +1431,6 @@ int journal_stop(handle_t *handle)
 		}
 	}
 
-	if (handle->h_sync)
-		transaction->t_synchronous_commit = 1;
 	current->journal_info = NULL;
 	spin_lock(&journal->j_state_lock);
 	spin_lock(&transaction->t_handle_lock);

@@ -1463,7 +1461,8 @@ int journal_stop(handle_t *handle)
 		jbd_debug(2, "transaction too old, requesting commit for "
 					"handle %p\n", handle);
 		/* This is non-blocking */
-		__log_start_commit(journal, transaction->t_tid);
+		__log_start_commit(journal, transaction->t_tid,
+			handle->h_sync && !(current->flags & PF_MEMALLOC));
 		spin_unlock(&journal->j_state_lock);
 
 		/*

include/linux/jbd.h +12 -9

index e069650..c38f73e 100644
@@ -541,12 +541,6 @@ struct transaction_s
 	 * How many handles used this transaction? [t_handle_lock]
 	 */
 	int t_handle_count;
-
-	/*
-	 * This transaction is being forced and some process is
-	 * waiting for it to finish.
-	 */
-	unsigned int t_synchronous_commit:1;
 };
 
 /**

@@ -594,6 +588,8 @@ struct transaction_s
  *  transaction
  * @j_commit_request: Sequence number of the most recent transaction wanting
  *     commit
+ * @j_commit_waited: Sequence number of the most recent transaction someone
+ *     is waiting for to commit.
  * @j_uuid: Uuid of client object.
  * @j_task: Pointer to the current commit thread for this journal
  * @j_max_transaction_buffers:  Maximum number of metadata buffers to allow in a

@@ -762,6 +758,13 @@ struct journal_s
 	tid_t			j_commit_request;
 
 	/*
+	 * Sequence number of the most recent transaction someone is waiting
+	 * for to commit.
+	 * [j_state_lock]
+	 */
+	tid_t                   j_commit_waited;
+
+	/*
 	 * Journal uuid: identifies the object (filesystem, LVM volume etc)
 	 * backed by this journal.  This will eventually be replaced by an array
 	 * of uuids, allowing us to index multiple devices within a single

@@ -985,9 +988,9 @@ extern void	journal_switch_revoke_table(journal_t *journal);
  */
 
 int __log_space_left(journal_t *); /* Called with journal locked */
-int log_start_commit(journal_t *journal, tid_t tid);
-int __log_start_commit(journal_t *journal, tid_t tid);
-int journal_start_commit(journal_t *journal, tid_t *tid);
+int log_start_commit(journal_t *journal, tid_t tid, bool will_wait);
+int __log_start_commit(journal_t *journal, tid_t tid, bool will_wait);
+int journal_start_commit(journal_t *journal, tid_t *tid, bool will_wait);
 int journal_force_commit_nested(journal_t *journal);
 int log_wait_commit(journal_t *journal, tid_t tid);
 int log_do_checkpoint(journal_t *journal);

Signature · 1 line

1.7.1

Jeff Moyer

2011-03-02 21:21 UTC #20 lore raw

Quoted 37 lines · Jan Kara

Quoted 28 lines · Jeff Moyer
Quoted 20 lines · Jan Kara
Quoted 19 lines · Jeff Moyer
Quoted 16 lines · Corrado Zoccolo
Quoted 4 lines · Jan Kara
Quoted 3 lines · Alex,Shi
Though these patches can not totally recovered the problem, but they are quite helpful with ccache enabled situation. It increase 10% performance on 38-rc1 kernel.
Quoted 8 lines
OK and what was the original performance drop with WRITE_SYNC change?
Quoted 2 lines
I have tried to enabled they to latest rc6 kernel but failed. the vmstat output is here:
with patches:
I'm attaching patches rebased on top of latest Linus's tree. Corrado, could you possibly run your fsync-heavy tests so that we see whether there isn't negative impact of my patches on your fsync-heavy workload? Thanks.
The workload was actually Jeff's, and the stalls that my change tried to mitigate showed up on his enterprise class storage. Adding him so he can test it.
Quoted 6 lines
Quoted 4 lines
Sorry for the late reply. You can use either fs_mark or iozone to generate an fsync-heavy workload. The test I did was to mix this with a sequential reader. If you can point me at patches, I should be able to test this.
The latest version of patches is attached to:
https://lkml.org/lkml/2011/2/24/125
Quoted 8 lines
Perhaps you should fix up the merge conflicts, first? ;-)
+<<<<<<< HEAD
        tid = transaction->t_tid;
        need_to_start = !tid_geq(journal->j_commit_request, tid);
+=======
+       __jbd2_log_start_commit(journal, transaction->t_tid, false);
+>>>>>>> jbd2: Refine commit writeout logic

Quoted 4 lines

Doh, how embarrassing ;). Attached is a new version which compiles and
seems to run OK.
Honza

Thanks, Jan. I should have results for you tomorrow.

Cheers,
Jeff

Jeff Moyer

2011-03-03 01:14 UTC #21 lore raw

In reply to Jan Kara

Hi, Jan,

So, the results are in. The test workload is an fs_mark process writing out 64k files and fsyncing each file after it's written. Concurrently with this is a fio job running a buffered sequential reader (bsr). Each data point is the average of 10 runs, after throwing out the first run. File system mount options are left at their defaults, which means that barriers are on. The storage is an HP EVA, connected to the host via a single 4Gb FC path.

ext3 looks marginally better with your patches. We get better files/sec AND better throughput from the buffered reader. For ext4, the results are less encouraging. We see a drop in files/sec, and an increase in throughput for the sequential reader. So, the fsync-ing is being starved a bit more than before.

        ||       ext3        ||       ext4        ||
        || fs_mark | fio bsr || fs_mark | fio bsr ||
--------++---------+---------++---------+---------||
vanilla || 517.535 | 178187  || 408.547 | 277130  ||
patched || 540.34  | 182312  || 342.813 | 294655  ||

====================================================
%diff || +4.4% | +2.3% || -16.1% | +6.3% ||

I'm tired right now, but I'll have a look at your ext4 patch in the morning and see if I can come up with a good reason for this drop.

Let me know if you have any questions.

Cheers,
Jeff

Jan Kara

reviewer

2011-03-04 15:32 UTC #22 lore raw

Hi Jeff,

Quoted 7 lines · Jeff Moyer

So, the results are in. The test workload is an fs_mark process writing out 64k files and fsyncing each file after it's written. Concurrently with this is a fio job running a buffered sequential reader (bsr). Each data point is the average of 10 runs, after throwing out the first run. File system mount options are left at their defaults, which means that barriers are on. The storage is an HP EVA, connected to the host via a single 4Gb FC path.

Thanks a lot for testing! BTW: fs_mark runs in a single thread or do you
use more threads?

Quoted 6 lines

ext3 looks marginally better with your patches. We get better files/sec AND better throughput from the buffered reader. For ext4, the results are less encouraging. We see a drop in files/sec, and an increase in throughput for the sequential reader. So, the fsync-ing is being starved a bit more than before.

Quoted 7 lines

        ||       ext3        ||       ext4        ||
        || fs_mark | fio bsr || fs_mark | fio bsr ||
--------++---------+---------++---------+---------||
vanilla || 517.535 | 178187  || 408.547 | 277130  ||
patched || 540.34  | 182312  || 342.813 | 294655  ||

====================================================
%diff || +4.4% | +2.3% || -16.1% | +6.3% ||

Interesting. I'm surprised ext3 and ext4 results differ this much. I'm more than happy with ext3 results since I just wanted to verify that fsync load doesn't degrade too much with the improved logic preferring non-fsync load more than we used to.

I'm not so happy with ext4 results. The difference between ext3 and ext4 might be that amount of data written by kjournald in ext3 is considerably larger if it ends up pushing out data (because of data=ordered mode) as well. With ext4, all data are written by filemap_fdatawrite() from fsync because of delayed allocation. And thus maybe for ext4 WRITE_SYNC_PLUG is hurting us with your fast storage and small amount of written data? With WRITE_SYNC, data would be already on it's way to storage before we get to wait for them...

Or it could be that we really send more data in WRITE mode rather than in WRITE_SYNC mode with the patch on ext4 (that should be verifiable with blktrace). But I wonder how that could happen...

Bye
Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Jeff Moyer

2011-03-04 15:40 UTC #23 lore raw

Quoted 11 lines · Jan Kara

Hi Jeff,
Quoted 7 lines · Jeff Moyer
So, the results are in. The test workload is an fs_mark process writing out 64k files and fsyncing each file after it's written. Concurrently with this is a fio job running a buffered sequential reader (bsr). Each data point is the average of 10 runs, after throwing out the first run. File system mount options are left at their defaults, which means that barriers are on. The storage is an HP EVA, connected to the host via a single 4Gb FC path.
Thanks a lot for testing! BTW: fs_mark runs in a single thread or do you
use more threads?

I use a single fs_mark thread. FWIW, I also tested just fs_mark, and those numbers look good.

Quoted 27 lines

Quoted 6 lines
ext3 looks marginally better with your patches. We get better files/sec AND better throughput from the buffered reader. For ext4, the results are less encouraging. We see a drop in files/sec, and an increase in throughput for the sequential reader. So, the fsync-ing is being starved a bit more than before.
Quoted 7 lines
        ||       ext3        ||       ext4        ||
        || fs_mark | fio bsr || fs_mark | fio bsr ||
--------++---------+---------++---------+---------||
vanilla || 517.535 | 178187  || 408.547 | 277130  ||
patched || 540.34  | 182312  || 342.813 | 294655  ||
====================================================
%diff || +4.4% | +2.3% || -16.1% | +6.3% ||
Interesting. I'm surprised ext3 and ext4 results differ this much. I'm more than happy with ext3 results since I just wanted to verify that fsync load doesn't degrade too much with the improved logic preferring non-fsync load more than we used to.
I'm not so happy with ext4 results. The difference between ext3 and ext4 might be that amount of data written by kjournald in ext3 is considerably larger if it ends up pushing out data (because of data=ordered mode) as well. With ext4, all data are written by filemap_fdatawrite() from fsync because of delayed allocation. And thus maybe for ext4 WRITE_SYNC_PLUG is hurting us with your fast storage and small amount of written data? With WRITE_SYNC, data would be already on it's way to storage before we get to wait for them...

Quoted 3 lines

Or it could be that we really send more data in WRITE mode rather than in WRITE_SYNC mode with the patch on ext4 (that should be verifiable with blktrace). But I wonder how that could happen...

Yeah, I've collected blktrace data and I'll get to evaluating that. Sorry, I ran out of time yesterday.

Cheers,
Jeff

Jeff Moyer

2011-03-04 15:50 UTC #24 lore raw

In reply to Jan Kara

Quoted 9 lines · Jan Kara

I'm not so happy with ext4 results. The difference between ext3 and ext4 might be that amount of data written by kjournald in ext3 is considerably larger if it ends up pushing out data (because of data=ordered mode) as well. With ext4, all data are written by filemap_fdatawrite() from fsync because of delayed allocation. And thus maybe for ext4 WRITE_SYNC_PLUG is hurting us with your fast storage and small amount of written data? With WRITE_SYNC, data would be already on it's way to storage before we get to wait for them...

Quoted 3 lines

Or it could be that we really send more data in WRITE mode rather than in WRITE_SYNC mode with the patch on ext4 (that should be verifiable with blktrace). But I wonder how that could happen...

It looks like this is the case, the I/O isn't coming down as synchronous. I'm seeing a lot of writes, very few write sync's, which means that the write stream will be preempted by the incoming reads.

Time to audit that fsync path and make sure it's marked properly, I guess.

Cheers,
Jeff

Jeff Moyer

2011-03-04 18:28 UTC #25 lore raw

Quoted 15 lines · Jeff Moyer

Quoted 9 lines · Jan Kara
I'm not so happy with ext4 results. The difference between ext3 and ext4 might be that amount of data written by kjournald in ext3 is considerably larger if it ends up pushing out data (because of data=ordered mode) as well. With ext4, all data are written by filemap_fdatawrite() from fsync because of delayed allocation. And thus maybe for ext4 WRITE_SYNC_PLUG is hurting us with your fast storage and small amount of written data? With WRITE_SYNC, data would be already on it's way to storage before we get to wait for them...
Quoted 3 lines
Or it could be that we really send more data in WRITE mode rather than in WRITE_SYNC mode with the patch on ext4 (that should be verifiable with blktrace). But I wonder how that could happen...

Quoted 6 lines

It looks like this is the case, the I/O isn't coming down as synchronous. I'm seeing a lot of writes, very few write sync's, which means that the write stream will be preempted by the incoming reads.
Time to audit that fsync path and make sure it's marked properly, I guess.

OK, I spoke too soon. Here's the blktrace summary information (I re-ran the tests using 3 samples, the blktrace is from the last run of the three in each case):

Vanilla
-------
fs_mark: 307.288 files/sec
fio: 286509 KB/s

Total (sde):

 Reads Queued:     341,558,   84,994MiB  Writes Queued:       1,561K,    6,244MiB
 Read Dispatches:  341,493,   84,994MiB  Write Dispatches:  648,046,    6,244MiB
 Reads Requeued:         0               Writes Requeued:        27
 Reads Completed:  341,491,   84,994MiB  Writes Completed:  648,021,    6,244MiB
 Read Merges:           65,    2,780KiB  Write Merges:      913,076,    3,652MiB
 IO unplugs:       578,102               Timer unplugs:           0

Throughput (R/W): 282,797KiB/s / 20,776KiB/s
Events (sde): 16,724,303 entries

Patched
-------
fs_mark: 278.587 files/sec
fio: 298007 KB/s

Total (sde):

 Reads Queued:     345,407,   86,834MiB  Writes Queued:       1,566K,    6,264MiB
 Read Dispatches:  345,391,   86,834MiB  Write Dispatches:  327,404,    6,264MiB
 Reads Requeued:         0               Writes Requeued:        33
 Reads Completed:  345,391,   86,834MiB  Writes Completed:  327,371,    6,264MiB
 Read Merges:           16,    1,576KiB  Write Merges:        1,238K,    4,954MiB
 IO unplugs:       580,308               Timer unplugs:           0

Throughput (R/W): 288,771KiB/s / 20,832KiB/s
Events (sde): 14,030,610 entries

So, it appears we flush out writes much more aggressively without the patch in place. I'm not sure why the write bandwidth looks to be higher in the patched case... odd.

Cheers,
Jeff

Alex,Shi

author

2011-03-22 07:36 UTC #26 lore raw

Quoted 57 lines · Jeff Moyer

Quoted 15 lines · Jeff Moyer
Quoted 9 lines · Jan Kara
I'm not so happy with ext4 results. The difference between ext3 and ext4 might be that amount of data written by kjournald in ext3 is considerably larger if it ends up pushing out data (because of data=ordered mode) as well. With ext4, all data are written by filemap_fdatawrite() from fsync because of delayed allocation. And thus maybe for ext4 WRITE_SYNC_PLUG is hurting us with your fast storage and small amount of written data? With WRITE_SYNC, data would be already on it's way to storage before we get to wait for them...
Quoted 3 lines
Or it could be that we really send more data in WRITE mode rather than in WRITE_SYNC mode with the patch on ext4 (that should be verifiable with blktrace). But I wonder how that could happen...
Quoted 6 lines
It looks like this is the case, the I/O isn't coming down as synchronous. I'm seeing a lot of writes, very few write sync's, which means that the write stream will be preempted by the incoming reads.
Time to audit that fsync path and make sure it's marked properly, I guess.
OK, I spoke too soon. Here's the blktrace summary information (I re-ran the tests using 3 samples, the blktrace is from the last run of the three in each case):
Vanilla
-------
fs_mark: 307.288 files/sec
fio: 286509 KB/s
Total (sde):
 Reads Queued:     341,558,   84,994MiB  Writes Queued:       1,561K,    6,244MiB
 Read Dispatches:  341,493,   84,994MiB  Write Dispatches:  648,046,    6,244MiB
 Reads Requeued:         0               Writes Requeued:        27
 Reads Completed:  341,491,   84,994MiB  Writes Completed:  648,021,    6,244MiB
 Read Merges:           65,    2,780KiB  Write Merges:      913,076,    3,652MiB
 IO unplugs:       578,102               Timer unplugs:           0
Throughput (R/W): 282,797KiB/s / 20,776KiB/s
Events (sde): 16,724,303 entries
Patched
-------
fs_mark: 278.587 files/sec
fio: 298007 KB/s
Total (sde):
 Reads Queued:     345,407,   86,834MiB  Writes Queued:       1,566K,    6,264MiB
 Read Dispatches:  345,391,   86,834MiB  Write Dispatches:  327,404,    6,264MiB
 Reads Requeued:         0               Writes Requeued:        33
 Reads Completed:  345,391,   86,834MiB  Writes Completed:  327,371,    6,264MiB
 Read Merges:           16,    1,576KiB  Write Merges:        1,238K,    4,954MiB
 IO unplugs:       580,308               Timer unplugs:           0

Quoted 7 lines

Throughput (R/W): 288,771KiB/s / 20,832KiB/s
Events (sde): 14,030,610 entries
So, it appears we flush out writes much more aggressively without the patch in place. I'm not sure why the write bandwidth looks to be higher in the patched case... odd.

Jan:
Do you have new idea on this?

Jan Kara

reviewer

2011-03-22 16:14 UTC #27 lore raw

Quoted 61 lines · Alex,Shi

Quoted 60 lines · Jeff Moyer
Quoted 15 lines · Jeff Moyer
Quoted 9 lines · Jan Kara
I'm not so happy with ext4 results. The difference between ext3 and ext4 might be that amount of data written by kjournald in ext3 is considerably larger if it ends up pushing out data (because of data=ordered mode) as well. With ext4, all data are written by filemap_fdatawrite() from fsync because of delayed allocation. And thus maybe for ext4 WRITE_SYNC_PLUG is hurting us with your fast storage and small amount of written data? With WRITE_SYNC, data would be already on it's way to storage before we get to wait for them...
Quoted 3 lines
Or it could be that we really send more data in WRITE mode rather than in WRITE_SYNC mode with the patch on ext4 (that should be verifiable with blktrace). But I wonder how that could happen...
Quoted 6 lines
It looks like this is the case, the I/O isn't coming down as synchronous. I'm seeing a lot of writes, very few write sync's, which means that the write stream will be preempted by the incoming reads.
Time to audit that fsync path and make sure it's marked properly, I guess.
OK, I spoke too soon. Here's the blktrace summary information (I re-ran the tests using 3 samples, the blktrace is from the last run of the three in each case):
Vanilla
-------
fs_mark: 307.288 files/sec
fio: 286509 KB/s
Total (sde):
 Reads Queued:     341,558,   84,994MiB  Writes Queued:       1,561K,    6,244MiB
 Read Dispatches:  341,493,   84,994MiB  Write Dispatches:  648,046,    6,244MiB
 Reads Requeued:         0               Writes Requeued:        27
 Reads Completed:  341,491,   84,994MiB  Writes Completed:  648,021,    6,244MiB
 Read Merges:           65,    2,780KiB  Write Merges:      913,076,    3,652MiB
 IO unplugs:       578,102               Timer unplugs:           0
Throughput (R/W): 282,797KiB/s / 20,776KiB/s
Events (sde): 16,724,303 entries
Patched
-------
fs_mark: 278.587 files/sec
fio: 298007 KB/s
Total (sde):
 Reads Queued:     345,407,   86,834MiB  Writes Queued:       1,566K,    6,264MiB
 Read Dispatches:  345,391,   86,834MiB  Write Dispatches:  327,404,    6,264MiB
 Reads Requeued:         0               Writes Requeued:        33
 Reads Completed:  345,391,   86,834MiB  Writes Completed:  327,371,    6,264MiB
 Read Merges:           16,    1,576KiB  Write Merges:        1,238K,    4,954MiB
 IO unplugs:       580,308               Timer unplugs:           0
Throughput (R/W): 288,771KiB/s / 20,832KiB/s
Events (sde): 14,030,610 entries

Quoted 6 lines

Quoted 3 lines
So, it appears we flush out writes much more aggressively without the patch in place. I'm not sure why the write bandwidth looks to be higher in the patched case... odd.
Jan:
Do you have new idea on this?

I was looking at the block traces for quite some time but I couldn't find
the reason why fs_mark is slower with my patch. Actually, looking at the data now, I don't even understand how fs_mark can report lower files/sec values.

Both block traces were taken for 300 seconds. From the above stats, we see that on both kernels, we wrote 6.2 GB over that time. Looking at more detailed stats I made, fs_mark processes wrote 4094 MB on vanilla kernel and 4107 MB on the patched kernel. Given that they just sequentially create and fsync 64 KB files, files/sec ratio should be about the same with both kernels. So I'm really missing how fs_mark arrives at different files/sec values or how with such different values it happens that the amount written is actually the same. Anyone has any idea?

Looking at how fs_mark works and at differences in trace files - could it be that the difference is caused by a difference in how log files each fs_mark thread is writing are flushed? Or possibly by IO caused unlinking of created files somehow leaking to the time of the next measured fs_mark run in one case and not the other one? Jeff, I suppose the log files of fs_mark processes are on the same device as the test directory, aren't they - that might explain the flusher thread doing IO? The patch below should limit the interactions. If you have time to test fs_mark with this patch applied - does it make any difference?

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

fs_mark.c renamed from fs_mark.c.orig +5 -0

@@ -1353,6 +1353,11 @@ int main(int argc, char **argv, char **e
 		print_iteration_stats(log_file_fp, &iteration_stats,
 				      files_written);
 		loops_done++;
+		/*
+		 * Flush dirty data to avoid interaction between unlink / log
+		 * file handling and the next iteration
+		 */
+		sync();
 
 	} while (do_fill_fs || (loop_count > loops_done));

Jeff Moyer

2011-03-22 17:46 UTC #28 lore raw

Quoted 3 lines · Jan Kara

Jeff, I suppose the log files of fs_mark processes are on the same device as the test directory, aren't they - that might explain the flusher thread doing IO? The patch below

No. My test environments are sane. The file system is on a completely separate disk and it is unmounted and remounted in between each run. That should take care of any lost flushes.

Did you receive my last couple of private emails commenting on the trace data?

Cheers,
Jeff

Alex,Shi

author

2011-03-24 06:43 UTC #29 lore raw

Quoted 9 lines · Jeff Moyer

Quoted 3 lines · Jan Kara
Jeff, I suppose the log files of fs_mark processes are on the same device as the test directory, aren't they - that might explain the flusher thread doing IO? The patch below
No. My test environments are sane. The file system is on a completely separate disk and it is unmounted and remounted in between each run. That should take care of any lost flushes.

Jeff, why not give a try with Jan's fs_mark patch? Maybe the result show something change. :)

Quoted 6 lines

Did you receive my last couple of private emails commenting on the trace data?
Cheers,
Jeff

Jan Kara

reviewer

2011-03-28 19:49 UTC #30 lore raw

In reply to Jeff Moyer

Quoted 9 lines · Jeff Moyer

Quoted 3 lines · Jan Kara
Jeff, I suppose the log files of fs_mark processes are on the same device as the test directory, aren't they - that might explain the flusher thread doing IO? The patch below
No. My test environments are sane. The file system is on a completely separate disk and it is unmounted and remounted in between each run. That should take care of any lost flushes.

OK, I understand but let me clear out one thing. What fs_mark seems to be
doing is:
for given number of iterations { (-L option, default 1)
fork each thread {
start timer
for i = 1 .. 10000 { (could be changed by option -n)
write file i
}
stop timer
for i = 1 .. 10000 {
unlink file i
}
write statistics to log file
exit
}
read all log files, write combined results to another log file
}

I see from blktrace data that indeed you are running more than one iteration of the fs_mark process so the problem I was wondering about is whether unlinking or writing of log files cannot interefere with the IO happening while the timer is running.

Now log files are rather small so I don't really think they cause any problem but they could be the data flusher thread writes - they are stored in the current directory (unless you use -l option). So I my question was aiming at where these files are stored - do you specify -l option and if not, what is the current directory of fs_mark process in your setup?

I think unlinks could possibly cause problems and that's why I suggested we add sync() to the parent process before it runs the next iteration...

Quoted 2 lines

Did you receive my last couple of private emails commenting on the trace data?

Yes, I did get them. I'll reply to them in a while.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Ted Ts'o

maintainer

2011-01-19 14:32 UTC #31 lore raw

In reply to Alex,Shi

Quoted 3 lines · Alex,Shi

Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system.

What kerenl version were you comparing against? And what were you using to benchmark this performance drop? I'm curious what your workload was; I'm guesisng it was some kind of mixed read/write workload? Were there any fsync()'s involved?

Thanks, regards,

- Ted

Shaohua Li

2011-01-20 02:12 UTC #32 lore raw

Quoted 9 lines · Ted Ts'o

Quoted 3 lines · Alex,Shi
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system.
What kerenl version were you comparing against? And what were you using to benchmark this performance drop? I'm curious what your workload was; I'm guesisng it was some kind of mixed read/write workload? Were there any fsync()'s involved?

2.6.36 vs 2.6.37. this exists in 2.6.37-rc1. it's a kbuild, no a mixed read/write workload. kbuild hasn't fsync involved

Thanks,
Shaohua

Corrado Zoccolo

2011-01-21 07:23 UTC #33 lore raw

In reply to Alex,Shi

Quoted 16 lines · Alex,Shi

Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
we didn't test deadline IO mode, just test cfq. seems insert write request into sync queue effect much read performance, but we don't know details. What's your comments of this?

Quoted 7 lines

iostat of .37 kernel:
rrqm/s wrqm/s r/s w/s rMB/s wMB/s avgrq-sz avgqu-sz await svctm %util 22.5772 96.46 92.3742 14.747 1.0048 0.439474 34.8557 0.18078 3.8076 0.30447 2.94302 iostat of commit reverted .37:
26.6223 80.21 107.875 6.03538 1.51415 0 41.3275 0.153385 3.80569 0.377231 3.22323
From these numbers, it seems to me that with the patch reverted, the

write bandwidth is really low, and probably you are keeping most written files in the buffer cache during the whole compile, while the non-reverted kernel is making progress in writing out the files. So the 'improved' read bandwidth is due to unfairness w.r.t. writes. Does the result change if you add a final sync and time that as well, in order to see the full time to make it on disk?

I think that in a more extreme test where you end up filling all the buffer cache with written data, you will see much longer stalls with the revert than without.

Thanks,
Corrado

Quoted 41 lines

vmstat report show, read bandwidth dropping:
vmstat of .37:
r b swpd free buff cache si so bi bo in cs us sy id wa st 3.4 52.6 0.0 64303937.0 16466.7 121544.5 0.0 0.0 2102.7 1914.6 7414.1 3185.7 2.0 1.0 80.3 16.7 0.0 vmstat of revert all from .37
2.2 35.8 0.0 64306767.4 17265.6 126101.2 0.0 0.0 2415.8 1619.1 8532.2 3556.2 2.5 1.1 83.0 13.3 0.0
Regards
Alex
===
fs/jbd/commit.c +1 -1
index 34a4861..27ac2f3 100644
@@ -294,7 +294,7 @@ void journal_commit_transaction(journal_t *journal)
        int first_tag = 0;
        int tag_flag;
        int i;
-       int write_op = WRITE_SYNC;
+       int write_op = WRITE;
 
        /*
         * First job: lock down the current transaction and wait for
fs/jbd2/commit.c +0 -0
index f3ad159..69ff08e 100644
@@ -329,7 +329,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
        int tag_bytes = journal_tag_bytes(journal);
struct buffer_head *cbh = NULL; /* For transactional checksums */ __u32 crc32_sum = ~0;
- int write_op = WRITE_SYNC;
+ int write_op = WRITE;
/*
* First job: lock down the current transaction and wait for

Signature · 10 lines

__________________________________________________________________________

dott. Corrado Zoccolo                          mailto:[email protected]
PhD - Department of Computer Science - University of Pisa, Italy
--------------------------------------------------------------------------
The self-confidence of a warrior is not the self-confidence of the average
man. The average man seeks certainty in the eyes of the onlooker and calls
that self-confidence. The warrior seeks impeccability in his own eyes and
calls that humbleness.
                               Tales of Power - C. Castaneda

Alex,Shi

author

2011-01-21 07:48 UTC #34 lore raw

Quoted 23 lines · Corrado Zoccolo

Quoted 16 lines · Alex,Shi
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
we didn't test deadline IO mode, just test cfq. seems insert write request into sync queue effect much read performance, but we don't know details. What's your comments of this?
Quoted 5 lines
iostat of .37 kernel:
rrqm/s wrqm/s r/s w/s rMB/s wMB/s avgrq-sz avgqu-sz await svctm %util
22.5772 96.46 92.3742 14.747 1.0048 0.439474 34.8557 0.18078 3.8076 0.30447 2.94302 iostat of commit reverted .37:
26.6223 80.21 107.875 6.03538 1.51415 0 41.3275 0.153385 3.80569 0.377231 3.22323

Quoted 7 lines

From these numbers, it seems to me that with the patch reverted, the write bandwidth is really low, and probably you are keeping most written files in the buffer cache during the whole compile, while the non-reverted kernel is making progress in writing out the files. So the 'improved' read bandwidth is due to unfairness w.r.t. writes. Does the result change if you add a final sync and time that as well, in order to see the full time to make it on disk?

Agree with your guess, but kbuild is such kind of benchmark, we can not change its behavior. :(

Quoted 4 lines

I think that in a more extreme test where you end up filling all the buffer cache with written data, you will see much longer stalls with the revert than without.

Have to do this? and if so, it is not kbuild. :)

BTW, the Jan's patch has a little improvement on kbuild. In many time testing, it seems about 3% improving.

The average iostat output of Jan's patch:
rrqm/s wrqm/s r/s w/s rMB/s wMB/s avgrq-sz avgqu-sz await svctm %util
23.419726 87.450685 96.164521 6.748493 1.046438 0.370137 45.182192 0.200685 6.848767 0.394110 3.072192

Quoted 48 lines

Thanks,
Corrado

Quoted 41 lines

Regards
Alex

===

fs/jbd/commit.c +1 -1

index 34a4861..27ac2f3 100644
@@ -294,7 +294,7 @@ void journal_commit_transaction(journal_t *journal)
       int first_tag = 0;
       int tag_flag;
       int i;
-       int write_op = WRITE_SYNC;
+       int write_op = WRITE;
 
       /*
        * First job: lock down the current transaction and wait for

fs/jbd2/commit.c +1 -1

index f3ad159..69ff08e 100644
@@ -329,7 +329,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
       int tag_bytes = journal_tag_bytes(journal);
       struct buffer_head *cbh = NULL; /* For transactional checksums */
       __u32 crc32_sum = ~0;
-       int write_op = WRITE_SYNC;
+       int write_op = WRITE;
 
       /*
        * First job: lock down the current transaction and wait for

Alex,Shi

author

2011-01-21 07:53 UTC #35 lore raw

Sorry for forgetting Jan and tytso. added again.

Quoted 99 lines · Alex,Shi

Quoted 23 lines · Corrado Zoccolo
Quoted 16 lines · Alex,Shi
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
we didn't test deadline IO mode, just test cfq. seems insert write request into sync queue effect much read performance, but we don't know details. What's your comments of this?
Quoted 5 lines
iostat of .37 kernel:
rrqm/s wrqm/s r/s w/s rMB/s wMB/s avgrq-sz avgqu-sz await svctm %util
22.5772 96.46 92.3742 14.747 1.0048 0.439474 34.8557 0.18078 3.8076 0.30447 2.94302 iostat of commit reverted .37:
26.6223 80.21 107.875 6.03538 1.51415 0 41.3275 0.153385 3.80569 0.377231 3.22323
Quoted 7 lines
From these numbers, it seems to me that with the patch reverted, the write bandwidth is really low, and probably you are keeping most written files in the buffer cache during the whole compile, while the non-reverted kernel is making progress in writing out the files. So the 'improved' read bandwidth is due to unfairness w.r.t. writes. Does the result change if you add a final sync and time that as well, in order to see the full time to make it on disk?
Agree with your guess, but kbuild is such kind of benchmark, we can not change its behavior. :(
Quoted 4 lines
I think that in a more extreme test where you end up filling all the buffer cache with written data, you will see much longer stalls with the revert than without.
Have to do this? and if so, it is not kbuild. :)
BTW, the Jan's patch has a little improvement on kbuild. In many time testing, it seems about 3% improving.
The average iostat output of Jan's patch:
rrqm/s wrqm/s r/s w/s rMB/s wMB/s avgrq-sz avgqu-sz await svctm %util
23.419726 87.450685 96.164521 6.748493 1.046438 0.370137 45.182192 0.200685 6.848767 0.394110 3.072192
Quoted 48 lines
Thanks,
Corrado
Quoted 41 lines
vmstat report show, read bandwidth dropping:
vmstat of .37:
r b swpd free buff cache si so bi bo in cs us sy id wa st
3.4 52.6 0.0 64303937.0 16466.7 121544.5 0.0 0.0 2102.7 1914.6 7414.1 3185.7 2.0 1.0 80.3 16.7 0.0 vmstat of revert all from .37
2.2 35.8 0.0 64306767.4 17265.6 126101.2 0.0 0.0 2415.8 1619.1 8532.2 3556.2 2.5 1.1 83.0 13.3 0.0
Regards
Alex
===
fs/jbd/commit.c +1 -1
index 34a4861..27ac2f3 100644
@@ -294,7 +294,7 @@ void journal_commit_transaction(journal_t *journal)
       int first_tag = 0;
       int tag_flag;
       int i;
-       int write_op = WRITE_SYNC;
+       int write_op = WRITE;
 
       /*
        * First job: lock down the current transaction and wait for
fs/jbd2/commit.c +1 -1
index f3ad159..69ff08e 100644
@@ -329,7 +329,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
       int tag_bytes = journal_tag_bytes(journal);
       struct buffer_head *cbh = NULL; /* For transactional checksums */
       __u32 crc32_sum = ~0;
-       int write_op = WRITE_SYNC;
+       int write_op = WRITE;
 
       /*
        * First job: lock down the current transaction and wait for

Corrado Zoccolo

2011-01-21 08:13 UTC #36 lore raw

Quoted 37 lines · Alex,Shi

Sorry for forgetting Jan and tytso. added again.
Quoted 34 lines · Alex,Shi
Quoted 23 lines · Corrado Zoccolo
Quoted 16 lines · Alex,Shi
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
we didn't test deadline IO mode, just test cfq. seems insert write request into sync queue effect much read performance, but we don't know details. What's your comments of this?
Quoted 5 lines
iostat of .37 kernel:
rrqm/s wrqm/s r/s w/s rMB/s wMB/s avgrq-sz avgqu-sz await svctm %util 22.5772 96.46 92.3742 14.747 1.0048 0.439474 34.8557 0.18078 3.8076 0.30447 2.94302 iostat of commit reverted .37:
26.6223 80.21 107.875 6.03538 1.51415 0 41.3275 0.153385 3.80569 0.377231 3.22323
Quoted 7 lines
From these numbers, it seems to me that with the patch reverted, the write bandwidth is really low, and probably you are keeping most written files in the buffer cache during the whole compile, while the non-reverted kernel is making progress in writing out the files. So the 'improved' read bandwidth is due to unfairness w.r.t. writes. Does the result change if you add a final sync and time that as well, in order to see the full time to make it on disk?
Agree with your guess, but kbuild is such kind of benchmark, we can not change its behavior. :(

Why not? Benchmarks should try to model real workloads, and after I build a kernel, I run lilo, that performs a sync (and typically takes several seconds). If I used grub, I would pay the sync while unmounting the filesystem, but I would experience the multi-second delay anyway.

Quoted 8 lines

Quoted 8 lines
Quoted 6 lines
I think that in a more extreme test where you end up filling all the buffer cache with written data, you will see much longer stalls with the revert than without.
Have to do this? and if so, it is not kbuild. :)

It might be, on a machine with fewer ram. Can you try running with just 1G of ram?

Quoted 3 lines

Quoted 3 lines
BTW, the Jan's patch has a little improvement on kbuild. In many time testing, it seems about 3% improving.

The fact is, does it improve (or at least is a noop) also on constrained memory systems?

Quoted 56 lines

Quoted 53 lines
The average iostat output of Jan's patch:
rrqm/s wrqm/s r/s w/s rMB/s wMB/s avgrq-sz avgqu-sz await svctm %util 23.419726 87.450685 96.164521 6.748493 1.046438 0.370137 45.182192 0.200685 6.848767 0.394110 3.072192
Quoted 48 lines
Thanks,
Corrado
Quoted 41 lines
vmstat report show, read bandwidth dropping:
vmstat of .37:
r b swpd free buff cache si so bi bo in cs us sy id wa st 3.4 52.6 0.0 64303937.0 16466.7 121544.5 0.0 0.0 2102.7 1914.6 7414.1 3185.7 2.0 1.0 80.3 16.7 0.0 vmstat of revert all from .37
2.2 35.8 0.0 64306767.4 17265.6 126101.2 0.0 0.0 2415.8 1619.1 8532.2 3556.2 2.5 1.1 83.0 13.3 0.0
Regards
Alex
===
fs/jbd/commit.c +1 -1
index 34a4861..27ac2f3 100644
@@ -294,7 +294,7 @@ void journal_commit_transaction(journal_t *journal)
        int first_tag = 0;
        int tag_flag;
        int i;
-       int write_op = WRITE_SYNC;
+       int write_op = WRITE;
 
        /*
         * First job: lock down the current transaction and wait for
fs/jbd2/commit.c +0 -0
index f3ad159..69ff08e 100644
@@ -329,7 +329,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
        int tag_bytes = journal_tag_bytes(journal);
struct buffer_head *cbh = NULL; /* For transactional checksums */ __u32 crc32_sum = ~0;
- int write_op = WRITE_SYNC;
+ int write_op = WRITE;
/*
* First job: lock down the current transaction and wait for

Signature · 10 lines

__________________________________________________________________________

dott. Corrado Zoccolo                          mailto:[email protected]
PhD - Department of Computer Science - University of Pisa, Italy
--------------------------------------------------------------------------
The self-confidence of a warrior is not the self-confidence of the average
man. The average man seeks certainty in the eyes of the onlooker and calls
that self-confidence. The warrior seeks impeccability in his own eyes and
calls that humbleness.
                               Tales of Power - C. Castaneda

Shaohua Li

2011-01-21 08:20 UTC #37 lore raw

In reply to Corrado Zoccolo

Quoted 31 lines · Corrado Zoccolo

Quoted 16 lines · Alex,Shi
Shaohua and I tested kernel building performance on latest kernel. and found it is drop about 15% on our 64 LCPUs NHM-EX machine on ext4 file system. We find this performance dropping is due to commit 749ef9f8423054e326f. If we revert this patch or just change the WRITE_SYNC back to WRITE in jbd2/commit.c file. the performance can be recovered.
iostat report show with the commit, read request merge number increased and write request merge dropped. The total request size increased and queue length dropped. So we tested another patch: only change WRITE_SYNC to WRITE_SYNC_PLUG in jbd2/commit.c, but nothing effected.
we didn't test deadline IO mode, just test cfq. seems insert write request into sync queue effect much read performance, but we don't know details. What's your comments of this?
Quoted 5 lines
iostat of .37 kernel:
rrqm/s wrqm/s r/s w/s rMB/s wMB/s avgrq-sz avgqu-sz await svctm %util
22.5772 96.46 92.3742 14.747 1.0048 0.439474 34.8557 0.18078 3.8076 0.30447 2.94302 iostat of commit reverted .37:
26.6223 80.21 107.875 6.03538 1.51415 0 41.3275 0.153385 3.80569 0.377231 3.22323
From these numbers, it seems to me that with the patch reverted, the write bandwidth is really low, and probably you are keeping most written files in the buffer cache during the whole compile, while the non-reverted kernel is making progress in writing out the files. So the 'improved' read bandwidth is due to unfairness w.r.t. writes. Does the result change if you add a final sync and time that as well, in order to see the full time to make it on disk?

Quoted 3 lines

I think that in a more extreme test where you end up filling all the buffer cache with written data, you will see much longer stalls with the revert than without.

Not just bandwidth issue. cpu might have more idle. so I bet even the 'kbuild + sync' workload still has regression

And the no-write-merge of WRITE_SYNC isn't good, at least we should use WRITE_SYNC_PLUG.

Thanks,
Shaohua

Quoted 44 lines

Quoted 41 lines

Regards
Alex

===

fs/jbd/commit.c +1 -1

index 34a4861..27ac2f3 100644
@@ -294,7 +294,7 @@ void journal_commit_transaction(journal_t *journal)
       int first_tag = 0;
       int tag_flag;
       int i;
-       int write_op = WRITE_SYNC;
+       int write_op = WRITE;
 
       /*
        * First job: lock down the current transaction and wait for

fs/jbd2/commit.c +1 -1

index f3ad159..69ff08e 100644
@@ -329,7 +329,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
       int tag_bytes = journal_tag_bytes(journal);
       struct buffer_head *cbh = NULL; /* For transactional checksums */
       __u32 crc32_sum = ~0;
-       int write_op = WRITE_SYNC;
+       int write_op = WRITE;
 
       /*
        * First job: lock down the current transaction and wait for