imporve jbd2 fsync batching
Subsystems (2)
PATCH 1/1 imporve jbd2 fsync batching
Hello,
This is the jbd2 version of the jbd fsync batching patch.
This patch removes the static sleep time in favor of a more self optimizing approach where we measure the average amount of time it takes to commit a transaction to disk and the ammount of time a transaction has been running. If somebody does a sync write or an fsync() traditionally we would sleep for 1 jiffies, which depending on the value of HZ could be a significant amount of time compared to how long it takes to commit a transaction to the underlying storage. With this patch instead of sleeping for a jiffie, we check to see if the amount of time this transaction has been running is less than the average commit time, and if it is we sleep for the delta using schedule_hrtimeout to give us a higher precision sleep time. This greatly benefits high end storage where you could end up sleeping for longer than it takes to commit the transaction and therefore sitting idle instead of allowing the transaction to be committed by keeping the sleep time to a minimum so you are sure to always be doing something. Thank you,
fs/jbd2/commit.c +14 -0
@@ -332,6 +332,8 @@ void jbd2_journal_commit_transaction(journal_t *journal)int flags;int err;unsigned long long blocknr; +ktime_t start_time; +u64 commit_time;char *tagp =NULL ;journal_header_t *header;journal_block_tag_t *tag =NULL ;
@@ -458,6 +460,7 @@ void jbd2_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);
@@ -972,6 +975,17 @@ 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 the 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/jbd2/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 __jbd2_journal_temp_unlink_buffer (struct journal_head *jh);
@@ -48,6 +49,7 @@ jbd2_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);
@@ -1193,7 +1195,7 @@ int jbd2_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);
@@ -1222,6 +1224,17 @@ int jbd2_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
@@ -1229,11 +1242,26 @@ int jbd2_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/jbd2.h +15 -0
@@ -637,6 +637,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;
@@ -938,8 +943,18 @@ struct journal_sstruct buffer_head **j_wbuf;int j_wbufsize; +/* +* this is the pid of hte 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 disk. [j_state_lock] +*/ +u64 j_average_commit_time; +/* This function is called when a transaction is closed */ void (*j_commit_callback)(journal_t *,transaction_t *);
Theodore Tso
maintainerThanks!!
Once tiny spelling nit, which I'll fix up before I put it in the ext4 patch queue.
fs/jbd2/transaction.c int jbd2_journal_stop(handle_t *handle) · line 1228
Quoted 6 lines · Josef Bacik
fs/jbd2/transaction.c +2
-0@@ -1222,6 +1224,17 @@ int jbd2_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
s/usefull/useful/
- Ted
Quoted 4 lines
fs/jbd2/transaction.c int jbd2_journal_stop(handle_t *handle) · line 1228
Quoted 2 lines
fs/jbd2/transaction.c +1
-0+* can do, instead of having a static sleep time. This is usefull for s/usefull/useful/
and:
+ * join the transaction. We acheive this by measuring how long it takes
s/acheive/achieve/
Signature · 1 line
Leroy
Quoted 11 lines · Josef Bacik
somebody does a sync write or an
fsync()traditionally we would sleep for 1 jiffies, which depending on the value of HZ could be a significant amount of time compared to how long it takes to commit a transaction to the underlying storage. With this patch instead of sleeping for a jiffie, we check to see if the amount of time this transaction has been running is less than the average commit time, and if it is we sleep for the delta usingschedule_hrtimeoutto give us a higher precision sleep time. This greatly benefits high end storage where you could end up sleeping for longer than it takes to commit the transaction and therefore sitting idle instead of allowing the transaction to be committed by keeping the sleep time to a minimum so you are sure to always be doing something.
There was no reply to my previous comments about making the maximum sleep time be a fixed value (e.g. 15ms) instead of having it arbitrarily based on the jiffies value, which may change between 1ms and 10ms.
I don't object to this being included in ext4, but I suspect it could do even better, or at least be more consistent than depending on the HZ value.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
Theodore Tso
maintainerQuoted 3 lines · Andreas Dilger
There was no reply to my previous comments about making the maximum sleep time be a fixed value (e.g. 15ms) instead of having it arbitrarily based on the jiffies value, which may change between 1ms and 10ms.
I know the whole point is to make it be self-tuning, but I wonder if we should have some knobs to control the min and max sleep times.
- Ted
Quoted 7 lines · Theodore Tso
Quoted 4 lines · Andreas Dilger
There was no reply to my previous comments about making the maximum sleep time be a fixed value (e.g. 15ms) instead of having it arbitrarily based on the jiffies value, which may change between 1ms and 10ms.
Quoted 5 lines
I know the whole point is to make it be self-tuning, but I wonder if we should have some knobs to control the min and max sleep times.
- Ted
I think that it could be useful to have a tunable knob for the max sleep time. It would certainly be capped at a much lower level for a RAM disk than it would be for a very large RAID volume...
Ric
Theodore Tso
maintainerPATCH 1/1 ext4: add fsync batch tuning knobs
Quoted 3 lines · Ric Wheeler
I think that it could be useful to have a tunable knob for the max sleep time. It would certainly be capped at a much lower level for a RAM disk than it would be for a very large RAID volume...
Here is a patch which implements tuning knobs for Josef's fsync batching patch, at least for ext4. I've implemented both a minimum and maximum batch time. I've also changed the default max batch time to be 15ms, independent of HZ setting.
It'd be great if someone could run some benchmarks to see what would be ideal ways to twiddle these knobs for different types of storage devices (and workloads, presumably).
- Ted
-------------
ext4: add fsync batch tuning knobs
From: "Theodore Ts'o" <[email protected]>
Add new mount options, min_batch_time and max_batch_time, which controls how long the jbd2 layer should wait for additional filesystem operations to get batched into a synchronous handle.
---
fs/ext4/ext4.h +7 -0
@@ -328,6 +328,7 @@ struct ext4_mount_options {uid_t s_resuid;gid_t s_resgid;unsigned long s_commit_interval; +u32 s_min_batch_time, s_max_batch_time;#ifdef CONFIG_QUOTA int s_jquota_fmt;char *s_qf_names[MAXQUOTAS ];
@@ -806,6 +807,12 @@ static inline int ext4_valid_inum(struct super_block *sb, unsigned long ino)#define EXT4_DEFM_JMODE_WBACK 0x0060/* +* Default journal batch times +*/ +#define EXT4_DEF_MIN_BATCH_TIME 0 +#define EXT4_DEF_MAX_BATCH_TIME 15000/* 15ms */ + +/* * Structure of a directory entry */ #define EXT4_NAME_LEN 255
fs/ext4/ext4_sb.h +2 -0
@@ -74,6 +74,8 @@ struct ext4_sb_info {struct journal_s *s_journal;struct list_head s_orphan;unsigned long s_commit_interval; +u32 s_max_batch_time; +u32 s_min_batch_time;struct block_device *journal_bdev;#ifdef CONFIG_JBD2_DEBUG struct timer_list turn_ro_timer;/* For turning read-only (crash simulation) */
fs/ext4/super.c +38 -7
96 lines not on this page · raw message
fs/jbd2/journal.c +2 -0
@@ -961,6 +961,8 @@ static journal_t * journal_init_common (void)spin_lock_init (&journal->j_state_lock); journal->j_commit_interval = (HZ *JBD2_DEFAULT_MAX_COMMIT_AGE ); + journal->j_min_batch_time =0 ; + journal->j_max_batch_time =15000 ;/* 15ms */ /* The journal is marked for error until we succeed with recovery! */ journal->j_flags =JBD2_ABORT ;
fs/jbd2/transaction.c +2 -1
@@ -1255,9 +1255,11 @@ int jbd2_journal_stop(handle_t *handle) trans_time =ktime_to_ns (ktime_sub (ktime_get (), transaction->t_start_time)); + commit_time =max_t (u64, commit_time, +1000 *journal->j_min_batch_time); commit_time = min_t(u64, commit_time, -1000 *jiffies_to_usecs (1 ));
Re: [PATCH] ext4: add fsync batch tuning knobs
On Tue, Nov 25, 2008 at 05:22:32AM -0500, Theodore Tso wrote:
....
...
Quoted 9 lines
fs/ext4/super.c +2
-0{Opt_bh, "bh"}, {Opt_commit, "commit=%u"}, + {Opt_min_batch_time, "min_batch_time=%u"}, + {Opt_max_batch_time, "max_batch_time=%u"}, {Opt_journal_update, "journal=update"}, {Opt_journal_inum, "journal=%u"}, {Opt_journal_dev, "journal_dev=%u"},@@ -1107,6 +1119,20 @@ static int parse_options(char *options, struct super_block *sb, option =JBD2_DEFAULT_MAX_COMMIT_AGE ;
fs/ext4/super.c static int parse_options(char *options, struct super_block *sb, · line 1129
Quoted 10 lines
fs/ext4/super.c +8
-0sbi->s_commit_interval =HZ * option;break ; +case Opt_max_batch_time: +if (match_int (&args[0 ], &option)) +return 0 ; +if (option <0 ) +return 0 ; +if (option ==0 ) + option =EXT4_DEF_MAX_BATCH_TIME ; + sbi->s_max_batch_time = option;
We need a break here .
fs/ext4/super.c static int parse_options(char *options, struct super_block *sb, · line 1135
Quoted 6 lines
fs/ext4/super.c +6
-0+case Opt_min_batch_time: +if (match_int (&args[0 ], &option)) +return 0 ; +if (option <0 ) +return 0 ; + sbi->s_min_batch_time = option;
same here
fs/ext4/super.c static int ext4_fill_super(struct super_block *sb, void *data, int silent) · line 1989
Quoted 13 lines
fs/ext4/super.c +3
-0case Opt_data_journal: data_opt =EXT4_MOUNT_JOURNAL_DATA ; goto datacheck;@@ -1955,6 +1981,9 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) sbi->s_resuid =le16_to_cpu (es->s_def_resuid); sbi->s_resgid =le16_to_cpu (es->s_def_resgid); + sbi->s_commit_interval =JBD2_DEFAULT_MAX_COMMIT_AGE *HZ ; + sbi->s_min_batch_time =EXT4_DEF_MIN_BATCH_TIME ; + sbi->s_max_batch_time =EXT4_DEF_MAX_BATCH_TIME ;set_opt (sbi->s_mount_opt,RESERVATION );set_opt (sbi->s_mount_opt,BARRIER );
....
..
The patch also needs documentation of new mount options
-aneesh
Quoted 13 lines · Andreas Dilger
Quoted 11 lines · Josef Bacik
somebody does a sync write or an
fsync()traditionally we would sleep for 1 jiffies, which depending on the value of HZ could be a significant amount of time compared to how long it takes to commit a transaction to the underlying storage. With this patch instead of sleeping for a jiffie, we check to see if the amount of time this transaction has been running is less than the average commit time, and if it is we sleep for the delta usingschedule_hrtimeoutto give us a higher precision sleep time. This greatly benefits high end storage where you could end up sleeping for longer than it takes to commit the transaction and therefore sitting idle instead of allowing the transaction to be committed by keeping the sleep time to a minimum so you are sure to always be doing something.
Quoted 7 lines
There was no reply to my previous comments about making the maximum sleep time be a fixed value (e.g. 15ms) instead of having it arbitrarily based on the jiffies value, which may change between 1ms and 10ms.
I don't object to this being included in ext4, but I suspect it could do even better, or at least be more consistent than depending on the HZ value.
Sorry I missed that comment. The reason I set the top to be 1 jiffies was because thats what it used to be, so in an effort to try and keep regressions from happening I just left it as the max. I'm fine with setting the max to some other more constant value. Thanks,
Josef
Theodore Tso
maintainerfs/jbd2/commit.c restart_loop: · line 986
Quoted 9 lines · Josef Bacik
fs/jbd2/commit.c +9
-0+ 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 the commit time +*/ +if (likely (journal->j_average_commit_time)) + journal->j_average_commit_time = (commit_time*3 + + journal->j_average_commit_time) /4 ;
Stupid question... if the goal is to not have the average commit time not react too strongly to sudden and vast changes to the commit time, would it be better to do this instead:
Review
Quoted 2 lines
(quoted diff) +2
-0+ journal->j_average_commit_time = (commit_time + + journal->j_average_commit_time*3) / 4;
- Ted
Quoted 15 lines · Theodore Ts'o
fs/jbd2/commit.c restart_loop: · line 986
Quoted 9 lines · Josef Bacik
fs/jbd2/commit.c +9
-0+ 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 the commit time +*/ +if (likely (journal->j_average_commit_time)) + journal->j_average_commit_time = (commit_time*3 + + journal->j_average_commit_time) /4 ;Stupid question... if the goal is to not have the average commit time not react too strongly to sudden and vast changes to the commit time, would it be better to do this instead:
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ journal->j_average_commit_time = (commit_time + + journal->j_average_commit_time*3) / 4;
Actually, yes. That is my fault, I'd suggested the original change to Josef.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
Theodore Tso
maintainerQuoted 9 lines · Andreas Dilger
Quoted 6 lines
Stupid question... if the goal is to not have the average commit time not react too strongly to sudden and vast changes to the commit time, would it be better to do this instead:
Review
Quoted 2 lines
(quoted diff) +2
-0+ journal->j_average_commit_time = (commit_time + + journal->j_average_commit_time*3) / 4;Actually, yes. That is my fault, I'd suggested the original change to Josef.
BTW, I've added a patch to display the average commit time it does vary wildly, especially on a laptop hard drive. While the system is idle, and the occasional commits need to wait for the hard drive to wake up, leads to a average commit time of around 80-140ms. If the disk is just getting lightly tickled, such that it never has a chance to go to sleep, the average commit time can drop down to around 20-25ms. If the hard drive is really busy, then the average commit time can go up to 40-50ms.
Increasing the weight as described below will slow down the move to these long-term averages, but at least for laptop or Green Star drives with power savings enabled, the average commit time does seem to vary in some non-inintuitive ways. Of course, if we are capping the max transaction time at 15ms, most of this will never be visible, but it would probably be interesting to test out this patch on a fast SSD or an expensive enterprise array to see whether are similar surprising variations in the average commit time.
- Ted
Quoted 20 lines · Theodore Tso
Quoted 9 lines · Andreas Dilger
Quoted 6 lines
Stupid question... if the goal is to not have the average commit time not react too strongly to sudden and vast changes to the commit time, would it be better to do this instead:
Review
Quoted 2 lines
(quoted diff) +2
-0+ journal->j_average_commit_time = (commit_time + + journal->j_average_commit_time*3) / 4;Actually, yes. That is my fault, I'd suggested the original change to Josef.
BTW, I've added a patch to display the average commit time it does vary wildly, especially on a laptop hard drive. While the system is idle, and the occasional commits need to wait for the hard drive to wake up, leads to a average commit time of around 80-140ms. If the disk is just getting lightly tickled, such that it never has a chance to go to sleep, the average commit time can drop down to around 20-25ms. If the hard drive is really busy, then the average commit time can go up to 40-50ms.
Quoted 9 lines
Increasing the weight as described below will slow down the move to these long-term averages, but at least for laptop or Green Star drives with power savings enabled, the average commit time does seem to vary in some non-inintuitive ways. Of course, if we are capping the max transaction time at 15ms, most of this will never be visible, but it would probably be interesting to test out this patch on a fast SSD or an expensive enterprise array to see whether are similar surprising variations in the average commit time.
I was printing out the commit time when I was testing this and with high end storage the commit time didn't vary all that much. With my SATA drive on a normal running box it didn't vary all that much either, it would drop down to like 1ms because of syslog, but other than that it would ramp up when there was load and it would slowly come back down as the load went away. Also do you want a new patch with that fix, or will you just fix it up? Thanks,
Josef