improve jbd fsync batching
PATCH 1/1 improve jbd fsync batching
Hello,
This is a rework of the patch I did a few months ago, taking into account some comments from Andrew and using the new schedule_hrtimeout function (thanks Arjan!).
There is a flaw with the way jbd handles fsync batching. If we fsync() a file and we were not the last person to run fsync() on this fs then we automatically sleep for 1 jiffie in order to wait for new writers to join into the transaction before forcing the commit. The problem with this is that with really fast storage (ie a Clariion) the time it takes to commit a transaction to disk is way faster than 1 jiffie in most cases, so sleeping means waiting longer with nothing to do than if we just committed the transaction and kept going. Ric Wheeler noticed this when using fs_mark with more than 1 thread, the throughput would plummet as he added more threads.
This patch attempts to fix this problem by recording the average time in nanoseconds that it takes to commit a transaction to disk, and what time we started the transaction. If we run an fsync() and we have been running for less time than it takes to commit the transaction to disk, we sleep for the delta amount of time and then commit to disk. We acheive sub-jiffie sleeping using schedule_hrtimeout. This means that the wait time is auto-tuned to the speed of the underlying disk, instead of having this static timeout. I weighted the average according to somebody's comments (Andreas Dilger I think) in order to help normalize random outliers where we take way longer or way less time to commit than the average. I also have a min() check in there to make sure we don't sleep longer than a jiffie in case our storage is super slow, this was requested by Andrew.
I unfortunately do not have access to a Clariion, so I had to use a ramdisk to represent a super fast array. I tested with a SATA drive with barrier=1 to make sure there was no regression with local disks, I tested with a 4 way multipathed Apple Xserve RAID array and of course the ramdisk. I ran the following command
fs_mark -d /mnt/ext3-test -s 4096 -n 2000 -D 64 -t $i
where $i was 2, 4, 8, 16 and 32. I mkfs'ed the fs each time. Here are my results
type threads with patch without patch
sata 2 24.6 26.3
sata 4 49.2 48.1
sata 8 70.1 67.0
sata 16 104.0 94.1
sata 32 153.6 142.7
xserve 2 246.4 222.0
xserve 4 480.0 440.8
xserve 8 829.5 730.8
xserve 16 1172.7 1026.9
xserve 32 1816.3 1650.5
ramdisk 2 2538.3 1745.6
ramdisk 4 2942.3 661.9
ramdisk 8 2882.5 999.8
ramdisk 16 2738.7 1801.9
ramdisk 32 2541.9 2394.0
All comments are welcome. Thank you,
fs/jbd/commit.c +15 -0
@@ -306,6 +306,8 @@ void journal_commit_transaction(journal_t *journal)int flags;int err;unsigned long blocknr; +ktime_t start_time; +u64 commit_time;char *tagp =NULL ;journal_header_t *header;journal_block_tag_t *tag =NULL ;
@@ -418,6 +420,7 @@ void journal_commit_transaction(journal_t *journal) commit_transaction->t_state =T_FLUSH ; journal->j_committing_transaction = commit_transaction; journal->j_running_transaction =NULL ; + start_time =ktime_get (); commit_transaction->t_log_start = journal->j_head;wake_up (&journal->j_wait_transaction_locked);spin_unlock (&journal->j_state_lock);
@@ -913,6 +916,18 @@ restart_loop:J_ASSERT (commit_transaction == journal->j_committing_transaction); journal->j_commit_sequence = commit_transaction->t_tid; journal->j_committing_transaction =NULL ; + commit_time =ktime_to_ns (ktime_sub (ktime_get (), start_time)); + +/* +* weight the commit time higher than the average time so we don't +* react too strongly to vast changes in commit time +*/ +if (likely (journal->j_average_commit_time)) + journal->j_average_commit_time = (commit_time*3 + + journal->j_average_commit_time) /4 ; +else + journal->j_average_commit_time = commit_time; +spin_unlock (&journal->j_state_lock); if (commit_transaction->t_checkpoint_list == NULL &&
fs/jbd/transaction.c +22 -5
@@ -25,6 +25,7 @@#include <linux/timer.h> #include <linux/mm.h> #include <linux/highmem.h> +#include <linux/hrtimer.h> static void __journal_temp_unlink_buffer (struct journal_head *jh);
@@ -49,6 +50,7 @@ get_transaction(journal_t *journal, transaction_t *transaction) { transaction->t_journal = journal; transaction->t_state =T_RUNNING ; + transaction->t_start_time =ktime_get (); transaction->t_tid = journal->j_transaction_sequence++; transaction->t_expires = jiffies + journal->j_commit_interval;spin_lock_init (&transaction->t_handle_lock);
@@ -1371,7 +1373,7 @@ int journal_stop(handle_t *handle) {transaction_t *transaction = handle->h_transaction;journal_t *journal = transaction->t_journal; -int old_handle_count, err; +int err;pid_t pid;J_ASSERT (journal_current_handle () == handle);
@@ -1407,11 +1409,26 @@ int journal_stop(handle_t *handle)*/ pid = current->pid;if (handle->h_sync && journal->j_last_sync_writer != pid) { +u64 commit_time, trans_time; + journal->j_last_sync_writer = pid; -do { - old_handle_count = transaction->t_handle_count; -schedule_timeout_uninterruptible (1 ); - }while (old_handle_count != transaction->t_handle_count); + +spin_lock (&journal->j_state_lock); + commit_time = journal->j_average_commit_time; +spin_unlock (&journal->j_state_lock); + + trans_time =ktime_to_ns (ktime_sub (ktime_get (), + transaction->t_start_time)); + + commit_time =min_t (u64, commit_time, +1000 *jiffies_to_usecs (1 )); + +if (trans_time < commit_time) { +ktime_t expires =ktime_add_ns (ktime_get (), + commit_time); +set_current_state (TASK_UNINTERRUPTIBLE ); +schedule_hrtimeout (&expires,HRTIMER_MODE_ABS ); + } } current->journal_info =NULL ;
include/linux/jbd.h +7 -0
@@ -543,6 +543,11 @@ struct transaction_sunsigned long t_expires;/* +* When this transaction started, in nanoseconds [no locking] +*/ +ktime_t t_start_time; + +/* * How many handles used this transaction? [t_handle_lock] */ int t_handle_count;
@@ -800,6 +805,8 @@ struct journal_spid_t j_last_sync_writer; +u64 j_average_commit_time; +/* * An opaque pointer to fs-private information. ext3 puts its * superblock pointer here
Quoted 2 lines · Josef Bacik
I also have a
min()check in there to make sure we don't sleep longer than a jiffie in case our storage is super slow, this was requested by Andrew.
Is there a particular reason why 1 jiffie is considered the "right amount" of time to sleep, given this is a kernel config parameter and has nothing to do with the storage? Considering a seek time in the range of ~10ms this would only be right for HZ=100 and the wait would otherwise be too short to maximize batching within a single transaction.
Quoted 6 lines
type threads with patch without patch
sata 2 24.6 26.3
sata 4 49.2 48.1
sata 8 70.1 67.0
sata 16 104.0 94.1
sata 32 153.6 142.7
In the previous patch where this wasn't limited it had better performance even for the 2 thread case. With the current 1-jiffie wait it likely isn't long enough to batch every pair of operations and every other operation waits an extra amount before giving up too soon. Previous patch:
type threads patch unpatched sata 2 34.6 26.2 sata 4 58.0 48.0 sata 8 75.2 70.4 sata 16 101.1 89.6
I'd recommend changing the patch to have a maximum sleep time that has a fixed maximum number of milliseconds (15ms should be enough for even very old disks).
That said, this would be a minor enhancement and should NOT be considered a reason to delay this patch's inclusion into -mm or the ext4 tree.
PS - it should really go into jbd2 also
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
Quoted 10 lines · Andreas Dilger
Quoted 2 lines · Josef Bacik
I also have a
min()check in there to make sure we don't sleep longer than a jiffie in case our storage is super slow, this was requested by Andrew.Is there a particular reason why 1 jiffie is considered the "right amount" of time to sleep, given this is a kernel config parameter and has nothing to do with the storage? Considering a seek time in the range of ~10ms this would only be right for HZ=100 and the wait would otherwise be too short to maximize batching within a single transaction.
I wouldn't say "right amount", more of "traditional amount". If you have super slow storage this patch will not result in you waiting any longer than you did originally, which I think is what the concern was, that we not wait a super long time just because the disk is slow.
Quoted 18 lines
Quoted 6 lines
type threads with patch without patch
sata 2 24.6 26.3
sata 4 49.2 48.1
sata 8 70.1 67.0
sata 16 104.0 94.1
sata 32 153.6 142.7In the previous patch where this wasn't limited it had better performance even for the 2 thread case. With the current 1-jiffie wait it likely isn't long enough to batch every pair of operations and every other operation waits an extra amount before giving up too soon. Previous patch:
type threads patch unpatched sata 2 34.6 26.2 sata 4 58.0 48.0 sata 8 75.2 70.4 sata 16 101.1 89.6
Quoted 4 lines
I'd recommend changing the patch to have a maximum sleep time that has a fixed maximum number of milliseconds (15ms should be enough for even very old disks).
This stat gathering process has been very unscientific :), I just ran once and took that number. Sometimes the patched version would come out on top, sometimes it wouldn't. If I were to do this the way my stat teacher taught me I'm sure the patched/unpatched versions would come out to be relatively the same in the 2 thread case.
Quoted 6 lines
That said, this would be a minor enhancement and should NOT be considered a reason to delay this patch's inclusion into -mm or the ext4 tree.
PS - it should really go into jbd2 also
Yes I will be doing a jbd2 version of this patch provided there are no issues with this patch. Thanks much for the comments,
Josef
Quoted 9 lines · On Tue, 28 Oct 2008 15:38:05 -0600 Andreas Dilger <[email protected]> wrote:
Quoted 3 lines · Josef Bacik
I also have a
min()check in there to make sure we don't sleep longer than a jiffie in case our storage is super slow, this was requested by Andrew.Is there a particular reason why 1 jiffie is considered the "right amount" of time to sleep, given this is a kernel config parameter and has nothing to do with the storage? Considering a seek time in the range of ~10ms this would only be right for HZ=100 and the wait would
well... my disk does a 50 usec seek time or so.. so I don't mind ;-)
in fact it sounds awefully long to me.
Signature · 3 lines
Arjan van de Ven Intel Open Source Technology Centre For development, discussion and tips for power savings, visit http://www.lesswatts.org
Quoted 16 lines · Arjan van de Ven
Quoted 11 lines · On Tue, 28 Oct 2008 15:38:05 -0600 Andreas Dilger <[email protected]> wrote:
Quoted 4 lines · Josef Bacik
I also have a
min()check in there to make sure we don't sleep longer than a jiffie in case our storage is super slow, this was requested by Andrew.Is there a particular reason why 1 jiffie is considered the "right amount" of time to sleep, given this is a kernel config parameter and has nothing to do with the storage? Considering a seek time in the range of ~10ms this would only be right for HZ=100 and the wait would
Quoted 4 lines
well... my disk does a 50 usec seek time or so.. so I don't mind ;-)
in fact it sounds awefully long to me.
For small writes as well as reads?
If so, it would be great to test Josef's patch against your shiny new SSD :-)
ric
Quoted 38 lines · On Tue, 28 Oct 2008 16:16:15 -0400 Josef Bacik <[email protected]> wrote:
Hello,
This is a rework of the patch I did a few months ago, taking into account some comments from Andrew and using the new
schedule_hrtimeoutfunction (thanks Arjan!).There is a flaw with the way jbd handles fsync batching. If we
fsync()a file and we were not the last person to runfsync()on this fs then we automatically sleep for 1 jiffie in order to wait for new writers to join into the transaction before forcing the commit. The problem with this is that with really fast storage (ie a Clariion) the time it takes to commit a transaction to disk is way faster than 1 jiffie in most cases, so sleeping means waiting longer with nothing to do than if we just committed the transaction and kept going. Ric Wheeler noticed this when usingfs_markwith more than 1 thread, the throughput would plummet as he added more threads....
...
fs/jbd/transaction.c +2
-1@@ -49,6 +50,7 @@ get_transaction(journal_t *journal, transaction_t *transaction) { transaction->t_journal = journal; transaction->t_state =T_RUNNING ; + transaction->t_start_time =ktime_get (); transaction->t_tid = journal->j_transaction_sequence++; transaction->t_expires = jiffies + journal->j_commit_interval;spin_lock_init (&transaction->t_handle_lock);@@ -1371,7 +1373,7 @@ int journal_stop(handle_t *handle) {transaction_t *transaction = handle->h_transaction;journal_t *journal = transaction->t_journal; -int old_handle_count, err; +int err;pid_t pid;J_ASSERT (journal_current_handle () == handle);
fs/jbd/transaction.c int journal_stop(handle_t *handle) · line 1411
Quoted 3 lines
fs/jbd/transaction.c +0
-0@@ -1407,11 +1409,26 @@ int journal_stop(handle_t *handle)*/ pid = current->pid;if (handle->h_sync && journal->j_last_sync_writer != pid) {
It would be nice to have a comment here explaining the overall design. it's a bit opaque working that out from the raw implementation.
fs/jbd/transaction.c int journal_stop(handle_t *handle) · line 1418
Quoted 11 lines
fs/jbd/transaction.c +6
-4+u64 commit_time, trans_time; + journal->j_last_sync_writer = pid; -do { - old_handle_count = transaction->t_handle_count; -schedule_timeout_uninterruptible (1 ); - }while (old_handle_count != transaction->t_handle_count); + +spin_lock (&journal->j_state_lock); + commit_time = journal->j_average_commit_time; +spin_unlock (&journal->j_state_lock);
OK, the lock is needed on 32-bit machines, I guess.
fs/jbd/transaction.c int journal_stop(handle_t *handle) · line 1424
Quoted 5 lines
fs/jbd/transaction.c +5
-0+ trans_time =ktime_to_ns (ktime_sub (ktime_get (), + transaction->t_start_time)); + + commit_time =min_t (u64, commit_time, +1000 *jiffies_to_usecs (1 ));
OK. The multiplication of an unsigned by 1000 could overflow, but only if HZ is less than 0.25. I don't think we need worry about that ;)
fs/jbd/transaction.c int journal_stop(handle_t *handle) · line 1430
Quoted 5 lines
fs/jbd/transaction.c +5
-0+if (trans_time < commit_time) { +ktime_t expires =ktime_add_ns (ktime_get (), + commit_time); +set_current_state (TASK_UNINTERRUPTIBLE ); +schedule_hrtimeout (&expires,HRTIMER_MODE_ABS );
We should have schedule_hrtimeout_uninterruptible(), but we don't.
Quoted 21 lines
fs/jbd/transaction.c +1
-0+ } } current->journal_info =NULL ;include/linux/jbd.h +5
-0@@ -543,6 +543,11 @@ struct transaction_sunsigned long t_expires;/* +* When this transaction started, in nanoseconds [no locking] +*/ +ktime_t t_start_time; + +/* * How many handles used this transaction? [t_handle_lock] */ int t_handle_count;
include/linux/jbd.h struct journal_s · line 808
Quoted 4 lines
include/linux/jbd.h +1
-0@@ -800,6 +805,8 @@ struct journal_spid_t j_last_sync_writer; +u64 j_average_commit_time;
Every field in that structure is carefully documented (except for j_last_sync_writer - what vandal did that?)
please fix.
Quoted 103 lines · Andrew Morton
Quoted 38 lines · On Tue, 28 Oct 2008 16:16:15 -0400 Josef Bacik <[email protected]> wrote:
Hello,
This is a rework of the patch I did a few months ago, taking into account some comments from Andrew and using the new
schedule_hrtimeoutfunction (thanks Arjan!).There is a flaw with the way jbd handles fsync batching. If we
fsync()a file and we were not the last person to runfsync()on this fs then we automatically sleep for 1 jiffie in order to wait for new writers to join into the transaction before forcing the commit. The problem with this is that with really fast storage (ie a Clariion) the time it takes to commit a transaction to disk is way faster than 1 jiffie in most cases, so sleeping means waiting longer with nothing to do than if we just committed the transaction and kept going. Ric Wheeler noticed this when usingfs_markwith more than 1 thread, the throughput would plummet as he added more threads....
...
fs/jbd/transaction.c +2
-1@@ -49,6 +50,7 @@ get_transaction(journal_t *journal, transaction_t *transaction) { transaction->t_journal = journal; transaction->t_state =T_RUNNING ; + transaction->t_start_time =ktime_get (); transaction->t_tid = journal->j_transaction_sequence++; transaction->t_expires = jiffies + journal->j_commit_interval;spin_lock_init (&transaction->t_handle_lock);@@ -1371,7 +1373,7 @@ int journal_stop(handle_t *handle) {transaction_t *transaction = handle->h_transaction;journal_t *journal = transaction->t_journal; -int old_handle_count, err; +int err;pid_t pid;J_ASSERT (journal_current_handle () == handle);fs/jbd/transaction.c int journal_stop(handle_t *handle) · line 1411
Quoted 3 lines
fs/jbd/transaction.c +0
-0@@ -1407,11 +1409,26 @@ int journal_stop(handle_t *handle)*/ pid = current->pid;if (handle->h_sync && journal->j_last_sync_writer != pid) {It would be nice to have a comment here explaining the overall design. it's a bit opaque working that out from the raw implementation.
fs/jbd/transaction.c int journal_stop(handle_t *handle) · line 1418
Quoted 11 lines
fs/jbd/transaction.c +6
-4+u64 commit_time, trans_time; + journal->j_last_sync_writer = pid; -do { - old_handle_count = transaction->t_handle_count; -schedule_timeout_uninterruptible (1 ); - }while (old_handle_count != transaction->t_handle_count); + +spin_lock (&journal->j_state_lock); + commit_time = journal->j_average_commit_time; +spin_unlock (&journal->j_state_lock);OK, the lock is needed on 32-bit machines, I guess.
fs/jbd/transaction.c int journal_stop(handle_t *handle) · line 1424
Quoted 5 lines
fs/jbd/transaction.c +5
-0+ trans_time =ktime_to_ns (ktime_sub (ktime_get (), + transaction->t_start_time)); + + commit_time =min_t (u64, commit_time, +1000 *jiffies_to_usecs (1 ));OK. The multiplication of an unsigned by 1000 could overflow, but only if HZ is less than 0.25. I don't think we need worry about that ;)
fs/jbd/transaction.c int journal_stop(handle_t *handle) · line 1430
Quoted 5 lines
fs/jbd/transaction.c +5
-0+if (trans_time < commit_time) { +ktime_t expires =ktime_add_ns (ktime_get (), + commit_time); +set_current_state (TASK_UNINTERRUPTIBLE ); +schedule_hrtimeout (&expires,HRTIMER_MODE_ABS );We should have
schedule_hrtimeout_uninterruptible(), but we don't.Quoted 22 lines
fs/jbd/transaction.c +1
-0+ } } current->journal_info =NULL ;include/linux/jbd.h +5
-0@@ -543,6 +543,11 @@ struct transaction_sunsigned long t_expires;/* +* When this transaction started, in nanoseconds [no locking] +*/ +ktime_t t_start_time; + +/* * How many handles used this transaction? [t_handle_lock] */ int t_handle_count;@@ -800,6 +805,8 @@ struct journal_s
Quoted 8 lines
include/linux/jbd.h struct journal_s · line 808
Quoted 3 lines
include/linux/jbd.h +1
-0pid_t j_last_sync_writer; +u64 j_average_commit_time;Every field in that structure is carefully documented (except for
j_last_sync_writer- what vandal did that?)please fix.
I see you already pulled this into -mm, would you like me to repost with the same changelog and the patch updated, or just reply to this with the updated patch? Thanks,
Josef
Quoted 5 lines · On Mon, 3 Nov 2008 15:24:43 -0500 Josef Bacik <[email protected]> wrote:
Quoted 1 line
please fix.
I see you already pulled this into -mm, would you like me to repost with the same changelog and the patch updated, or just reply to this with the updated patch? Thanks,
Either works for me at this stage. If it's a replacement then I'll turn it into an incremental so I can see what changed, which takes me about 2.15 seconds.
If it had been a large patch or if it had been under test in someone's tree for a while then a replacement patch would be unwelcome. But for a small, fresh patch like this one it's no big deal either way.
Quoted 9 lines · Andrew Morton
Quoted 5 lines · On Mon, 3 Nov 2008 15:24:43 -0500 Josef Bacik <[email protected]> wrote:
Quoted 1 line
please fix.
I see you already pulled this into -mm, would you like me to repost with the same changelog and the patch updated, or just reply to this with the updated patch? Thanks,
Quoted 8 lines
Either works for me at this stage. If it's a replacement then I'll turn it into an incremental so I can see what changed, which takes me about 2.15 seconds.
If it had been a large patch or if it had been under test in someone's tree for a while then a replacement patch would be unwelcome. But for a small, fresh patch like this one it's no big deal either way.
Ok here is a replacement patch with the comments as requested, as well as a comment for j_last_sync_writer. Thank you,
Josef
fs/jbd/commit.c +15 -0
@@ -306,6 +306,8 @@ void journal_commit_transaction(journal_t *journal)int flags;int err;unsigned long blocknr; +ktime_t start_time; +u64 commit_time;char *tagp =NULL ;journal_header_t *header;journal_block_tag_t *tag =NULL ;
@@ -418,6 +420,7 @@ void journal_commit_transaction(journal_t *journal) commit_transaction->t_state =T_FLUSH ; journal->j_committing_transaction = commit_transaction; journal->j_running_transaction =NULL ; + start_time =ktime_get (); commit_transaction->t_log_start = journal->j_head;wake_up (&journal->j_wait_transaction_locked);spin_unlock (&journal->j_state_lock);
@@ -913,6 +916,18 @@ restart_loop:J_ASSERT (commit_transaction == journal->j_committing_transaction); journal->j_commit_sequence = commit_transaction->t_tid; journal->j_committing_transaction =NULL ; + commit_time =ktime_to_ns (ktime_sub (ktime_get (), start_time)); + +/* +* weight the commit time higher than the average time so we don't +* react too strongly to vast changes in commit time +*/ +if (likely (journal->j_average_commit_time)) + journal->j_average_commit_time = (commit_time*3 + + journal->j_average_commit_time) /4 ; +else + journal->j_average_commit_time = commit_time; +spin_unlock (&journal->j_state_lock); if (commit_transaction->t_checkpoint_list == NULL &&
fs/jbd/transaction.c +33 -5
@@ -25,6 +25,7 @@#include <linux/timer.h> #include <linux/mm.h> #include <linux/highmem.h> +#include <linux/hrtimer.h> static void __journal_temp_unlink_buffer (struct journal_head *jh);
@@ -49,6 +50,7 @@ get_transaction(journal_t *journal, transaction_t *transaction) { transaction->t_journal = journal; transaction->t_state =T_RUNNING ; + transaction->t_start_time =ktime_get (); transaction->t_tid = journal->j_transaction_sequence++; transaction->t_expires = jiffies + journal->j_commit_interval;spin_lock_init (&transaction->t_handle_lock);
@@ -1371,7 +1373,7 @@ int journal_stop(handle_t *handle) {transaction_t *transaction = handle->h_transaction;journal_t *journal = transaction->t_journal; -int old_handle_count, err; +int err;pid_t pid;J_ASSERT (journal_current_handle () == handle);
@@ -1400,6 +1402,17 @@ int journal_stop(handle_t *handle)* on IO anyway. Speeds up many-threaded, many-dir operations * by 30x or more... * +* We try and optimize the sleep time against what the underlying disk +* can do, instead of having a static sleep time. This is usefull for +* the case where our storage is so fast that it is more optimal to go +* ahead and force a flush and wait for the transaction to be committed +* than it is to wait for an arbitrary amount of time for new writers to +* join the transaction. We acheive this by measuring how long it takes +* to commit a transaction, and compare it with how long this +* transaction has been running, and if run time < commit time then we +* sleep for the delta and commit. This greatly helps super fast disks +* that would see slowdowns as more threads started doing fsyncs. +* * But don't do this if this process was the most recent one to * perform a synchronous write. We do this to detect the case where a * single process is doing a stream of sync writes. No point in waiting
@@ -1407,11 +1420,26 @@ int journal_stop(handle_t *handle)*/ pid = current->pid;if (handle->h_sync && journal->j_last_sync_writer != pid) { +u64 commit_time, trans_time; + journal->j_last_sync_writer = pid; -do { - old_handle_count = transaction->t_handle_count; -schedule_timeout_uninterruptible (1 ); - }while (old_handle_count != transaction->t_handle_count); + +spin_lock (&journal->j_state_lock); + commit_time = journal->j_average_commit_time; +spin_unlock (&journal->j_state_lock); + + trans_time =ktime_to_ns (ktime_sub (ktime_get (), + transaction->t_start_time)); + + commit_time =min_t (u64, commit_time, +1000 *jiffies_to_usecs (1 )); + +if (trans_time < commit_time) { +ktime_t expires =ktime_add_ns (ktime_get (), + commit_time); +set_current_state (TASK_UNINTERRUPTIBLE ); +schedule_hrtimeout (&expires,HRTIMER_MODE_ABS ); + } } current->journal_info =NULL ;
include/linux/jbd.h +15 -0
@@ -543,6 +543,11 @@ struct transaction_sunsigned long t_expires;/* +* When this transaction started, in nanoseconds [no locking] +*/ +ktime_t t_start_time; + +/* * How many handles used this transaction? [t_handle_lock] */ int t_handle_count;
@@ -798,9 +803,19 @@ struct journal_sstruct buffer_head **j_wbuf;int j_wbufsize; +/* +* this is the pid of the last person to run a synchronous operation +* through the journal. +*/ pid_t j_last_sync_writer;/* +* the average amount of time in nanoseconds it takes to commit a +* transaction to the disk. [j_state_lock] +*/ +u64 j_average_commit_time; + +/* * An opaque pointer to fs-private information. ext3 puts its * superblock pointer here */
Quoted 4 lines · Josef Bacik
I see you already pulled this into -mm, would you like me to repost with the same changelog and the patch updated, or just reply to this with the updated patch? Thanks,
Josef,
When you have a moment, any chance you could spin an ext4 version of the patch, too? Thanks!!
- Ted
Quoted 7 lines · Andrew Morton
fs/jbd/transaction.c int journal_stop(handle_t *handle) · line 1418
Quoted 3 lines · On Tue, 28 Oct 2008 16:16:15 -0400 Josef Bacik <[email protected]> wrote:
fs/jbd/transaction.c +3
-0+spin_lock (&journal->j_state_lock); + commit_time = journal->j_average_commit_time; +spin_unlock (&journal->j_state_lock);OK, the lock is needed on 32-bit machines, I guess.
Should we pessimize the 64-bit performance in that case, for 32-bit increasingly rare 32-bit platforms?
It might be useful to have a spin_{,un}lock_64bit_word() helper that evaluates to a no-op on plaforms that don't need a hammer to do an atomic 64-bit update.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
Quoted 11 lines · On Mon, 03 Nov 2008 22:24:28 -0700 Andreas Dilger <[email protected]> wrote:
Quoted 7 lines · Andrew Morton
fs/jbd/transaction.c int journal_stop(handle_t *handle) · line 1418
Quoted 3 lines · On Tue, 28 Oct 2008 16:16:15 -0400 Josef Bacik <[email protected]> wrote:
fs/jbd/transaction.c +3
-0+spin_lock (&journal->j_state_lock); + commit_time = journal->j_average_commit_time; +spin_unlock (&journal->j_state_lock);OK, the lock is needed on 32-bit machines, I guess.
Should we pessimize the 64-bit performance in that case, for 32-bit increasingly rare 32-bit platforms?
In general no.
But spinlocks also do memory ordering stuff on both 32- and 64-bit machines. Introducing differences there needs thinking about.
In this case it's fsync which is going to be monster slow anyway.