| Message ID | 20081028201614.GA21600@unused.rdu.redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Oct 28, 2008 at 03:38:05PM -0600, Andreas Dilger wrote: > On Oct 28, 2008 16:16 -0400, Josef Bacik wrote: > > 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. > > 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). > 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct 28, 2008 16:16 -0400, Josef Bacik wrote: > 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. > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 28 Oct 2008 15:38:05 -0600 Andreas Dilger <adilger@sun.com> wrote: > On Oct 28, 2008 16:16 -0400, Josef Bacik wrote: > > 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.
Arjan van de Ven wrote: > On Tue, 28 Oct 2008 15:38:05 -0600 > Andreas Dilger <adilger@sun.com> wrote: > > >> On Oct 28, 2008 16:16 -0400, Josef Bacik wrote: >> >>> 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. > For small writes as well as reads? If so, it would be great to test Josef's patch against your shiny new SSD :-) ric -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 03, 2008 at 12:27:29PM -0800, Andrew Morton wrote: > On Tue, 28 Oct 2008 16:16:15 -0400 > Josef Bacik <jbacik@redhat.com> 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_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. > > > > ... > > > > ... > > > > @@ -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) { > > 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. > > > + 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. > > > > + 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 ;) > > > > + 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. > > > + } > > } > > > > current->journal_info = NULL; > > diff --git a/include/linux/jbd.h b/include/linux/jbd.h > > index 346e2b8..d842230 100644 > > --- a/include/linux/jbd.h > > +++ b/include/linux/jbd.h > > @@ -543,6 +543,11 @@ struct transaction_s > > unsigned 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 > > > > pid_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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 28 Oct 2008 16:16:15 -0400 Josef Bacik <jbacik@redhat.com> 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_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. > > ... > > ... > > @@ -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) { 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. > + 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. > + 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 ;) > + 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. > + } > } > > current->journal_info = NULL; > diff --git a/include/linux/jbd.h b/include/linux/jbd.h > index 346e2b8..d842230 100644 > --- a/include/linux/jbd.h > +++ b/include/linux/jbd.h > @@ -543,6 +543,11 @@ struct transaction_s > unsigned 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 > > pid_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. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 3 Nov 2008 15:24:43 -0500 Josef Bacik <jbacik@redhat.com> wrote: > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 03, 2008 at 03:24:43PM -0500, Josef Bacik wrote: > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Nov 03, 2008 12:27 -0800, Andrew Morton wrote: > On Tue, 28 Oct 2008 16:16:15 -0400 > Josef Bacik <jbacik@redhat.com> wrote: > > + 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. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 03 Nov 2008 22:24:28 -0700 Andreas Dilger <adilger@sun.com> wrote: > On Nov 03, 2008 12:27 -0800, Andrew Morton wrote: > > On Tue, 28 Oct 2008 16:16:15 -0400 > > Josef Bacik <jbacik@redhat.com> wrote: > > > + 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. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/jbd/commit.c b/fs/jbd/commit.c index 25719d9..3fbffb1 100644 --- a/fs/jbd/commit.c +++ b/fs/jbd/commit.c @@ -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 && diff --git a/fs/jbd/transaction.c b/fs/jbd/transaction.c index d15cd6e..59dd181 100644 --- a/fs/jbd/transaction.c +++ b/fs/jbd/transaction.c @@ -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; diff --git a/include/linux/jbd.h b/include/linux/jbd.h index 346e2b8..d842230 100644 --- a/include/linux/jbd.h +++ b/include/linux/jbd.h @@ -543,6 +543,11 @@ struct transaction_s unsigned 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 pid_t j_last_sync_writer; + u64 j_average_commit_time; + /* * An opaque pointer to fs-private information. ext3 puts its * superblock pointer here
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, Signed-off-by: Josef Bacik <jbacik@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html