| Message ID | 1359654788-6078-3-git-send-email-tytso@mit.edu |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Thu, Jan 31, 2013 at 12:53:07PM -0500, Theodore Ts'o wrote: > Once a transaction has been requested to be committed, don't let any > other handles start under that transaction, and don't allow any > pending transactions to be extended (i.e., in the case of > unlink/ftruncate). > > The idea is once the transaction has had log_start_commit() called on > it, at least one thread is blocked waiting for that transaction to > commit, and over time, more and more threads will end up getting > blocked. In order to avoid high variability in file system operations > getting blocked behind the a blocked start_this_handle(), we should > try to get the commit started as soon as possible. I'm going to drop this patch because thanks to some performance measurement works by Barry Marson at Red Hat, it shows that this patch apparently makes things worse by 17% with AIM7. At a guess, it looks like some AIM7 has enough threads competing for the CPU that it can take a good 80ms or more before kjournald can get scheduled, and then start locking down the transaction. During those 80ms, a number of short transactions get in and out and make forward progress on the benchmark. However, there are some "long pole" handles that will take up to 100-250ms to complete, and during that time, nothing else can get done. By starting to lock down the transaction as soon as the commit is requested, instead of waiting until the kjournald thread can be scheduled, we end up limiting forward progress made by the quick "in and out" handles, and this far outweighs the benefits of stopping a long-lived handle from getting started. It may be that after we do some work to conclusively identify what these "long pole" handles might be --- I strongly suspect they come from truncate/unlink calls, but we need to make sure. If in fact they are coming from truncate/unlink calls, it might be possible to let a truncate processing stop its handle once it notices that we are requesting that a journal commit should start, so we can lower the average amount of time start_this_handle() gets blocked waiting for a commit to complete. Thanks again to Barry for doing the benchmarking work! As always, people who provide us with benchmarking support provide an absolutely invaluable service. - 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
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c index 3091d42..fd2ac94 100644 --- a/fs/jbd2/commit.c +++ b/fs/jbd2/commit.c @@ -424,7 +424,8 @@ void jbd2_journal_commit_transaction(journal_t *journal) J_ASSERT(journal->j_committing_transaction == NULL); commit_transaction = journal->j_running_transaction; - J_ASSERT(commit_transaction->t_state == T_RUNNING); + J_ASSERT(commit_transaction->t_state == T_REQUESTED || + commit_transaction->t_state == T_RUNNING); trace_jbd2_start_commit(journal, commit_transaction); jbd_debug(1, "JBD2: starting commit of transaction %d\n", diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 1a80e31..c22773b 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -533,6 +533,7 @@ int __jbd2_log_start_commit(journal_t *journal, tid_t target) jbd_debug(1, "JBD2: requesting commit %d/%d\n", journal->j_commit_request, journal->j_commit_sequence); + journal->j_running_transaction->t_state = T_REQUESTED; wake_up(&journal->j_wait_commit); return 1; } else if (!tid_geq(journal->j_commit_request, target)) diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index df9f297..6daff29 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -224,7 +224,8 @@ repeat: * If the current transaction is locked down for commit, wait for the * lock to be released. */ - if (transaction->t_state == T_LOCKED) { + if ((transaction->t_state == T_LOCKED) || + (transaction->t_state == T_REQUESTED)) { DEFINE_WAIT(wait); prepare_to_wait(&journal->j_wait_transaction_locked, @@ -2179,7 +2180,8 @@ void __jbd2_journal_refile_buffer(struct journal_head *jh) else jlist = BJ_Reserved; __jbd2_journal_file_buffer(jh, jh->b_transaction, jlist); - J_ASSERT_JH(jh, jh->b_transaction->t_state == T_RUNNING); + J_ASSERT_JH(jh, (jh->b_transaction->t_state == T_RUNNING || + jh->b_transaction->t_state == T_REQUESTED)); if (was_dirty) set_buffer_jbddirty(bh); diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index e30b663..920a8d0 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -493,6 +493,7 @@ struct transaction_s */ enum { T_RUNNING, + T_REQUESTED, T_LOCKED, T_FLUSH, T_COMMIT,
Once a transaction has been requested to be committed, don't let any other handles start under that transaction, and don't allow any pending transactions to be extended (i.e., in the case of unlink/ftruncate). The idea is once the transaction has had log_start_commit() called on it, at least one thread is blocked waiting for that transaction to commit, and over time, more and more threads will end up getting blocked. In order to avoid high variability in file system operations getting blocked behind the a blocked start_this_handle(), we should try to get the commit started as soon as possible. Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> --- fs/jbd2/commit.c | 3 ++- fs/jbd2/journal.c | 1 + fs/jbd2/transaction.c | 6 ++++-- include/linux/jbd2.h | 1 + 4 files changed, 8 insertions(+), 3 deletions(-)