| Message ID | 20111222210007.ca8fb54f.toshi.okajima@jp.fujitsu.com |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 22, 2011 at 8:00 PM, Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> wrote: > Remove all t_handle_lock statements because they are not necessary anymore. > > Because there is read_lock(&journal->j_state_lock) or > write_lock(&journal->j_state_lock) on all the forward codes beyond the place > which needs a spin_lock(&transaction->t_handle_lock). > > Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> > Cc: > --- > fs/jbd2/commit.c | 4 ---- > fs/jbd2/transaction.c | 18 +++--------------- > include/linux/jbd2.h | 8 -------- > 3 files changed, 3 insertions(+), 27 deletions(-) > > diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c > index 68d704d..1030d47 100644 > --- a/fs/jbd2/commit.c > +++ b/fs/jbd2/commit.c > @@ -364,22 +364,18 @@ void jbd2_journal_commit_transaction(journal_t *journal) > stats.run.rs_running = jbd2_time_diff(commit_transaction->t_start, > stats.run.rs_locked); > > - spin_lock(&commit_transaction->t_handle_lock); > while (atomic_read(&commit_transaction->t_updates)) { > DEFINE_WAIT(wait); > > prepare_to_wait(&journal->j_wait_updates, &wait, > TASK_UNINTERRUPTIBLE); > if (atomic_read(&commit_transaction->t_updates)) { > - spin_unlock(&commit_transaction->t_handle_lock); > write_unlock(&journal->j_state_lock); > schedule(); > write_lock(&journal->j_state_lock); > - spin_lock(&commit_transaction->t_handle_lock); > } > finish_wait(&journal->j_wait_updates, &wait); > } > - spin_unlock(&commit_transaction->t_handle_lock); > > J_ASSERT (atomic_read(&commit_transaction->t_outstanding_credits) <= > journal->j_max_transaction_buffers); > diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c > index 76f2eca..7f84e3f 100644 > --- a/fs/jbd2/transaction.c > +++ b/fs/jbd2/transaction.c > @@ -56,7 +56,6 @@ jbd2_get_transaction(journal_t *journal, transaction_t *transaction) > 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); > atomic_set(&transaction->t_updates, 0); > atomic_set(&transaction->t_outstanding_credits, 0); > atomic_set(&transaction->t_handle_count, 0); > @@ -100,10 +99,8 @@ static inline void update_t_max_wait(transaction_t *transaction, > if (jbd2_journal_enable_debug && > time_after(transaction->t_start, ts)) { > ts = jbd2_time_diff(ts, transaction->t_start); > - spin_lock(&transaction->t_handle_lock); > if (ts > transaction->t_max_wait) > transaction->t_max_wait = ts; > - spin_unlock(&transaction->t_handle_lock); Hi Toshiyuki, I think you removed too much. t_handle_lock is needed here to protect t_max_wait. I meant just removing t_handle_lock from where I commented in your patch post out last time. Yongqiang. > } > #endif > } > @@ -401,19 +398,18 @@ int jbd2_journal_extend(handle_t *handle, int nblocks) > goto error_out; > } > > - spin_lock(&transaction->t_handle_lock); > wanted = atomic_read(&transaction->t_outstanding_credits) + nblocks; > > if (wanted > journal->j_max_transaction_buffers) { > jbd_debug(3, "denied handle %p %d blocks: " > "transaction too large\n", handle, nblocks); > - goto unlock; > + goto error_out; > } > > if (wanted > __jbd2_log_space_left(journal)) { > jbd_debug(3, "denied handle %p %d blocks: " > "insufficient log space\n", handle, nblocks); > - goto unlock; > + goto error_out; > } > > handle->h_buffer_credits += nblocks; > @@ -421,8 +417,6 @@ int jbd2_journal_extend(handle_t *handle, int nblocks) > result = 0; > > jbd_debug(3, "extended handle %p by %d\n", handle, nblocks); > -unlock: > - spin_unlock(&transaction->t_handle_lock); > error_out: > read_unlock(&journal->j_state_lock); > out: > @@ -464,12 +458,10 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, gfp_t gfp_mask) > J_ASSERT(journal_current_handle() == handle); > > read_lock(&journal->j_state_lock); > - spin_lock(&transaction->t_handle_lock); > atomic_sub(handle->h_buffer_credits, > &transaction->t_outstanding_credits); > if (atomic_dec_and_test(&transaction->t_updates)) > wake_up(&journal->j_wait_updates); > - spin_unlock(&transaction->t_handle_lock); > > jbd_debug(2, "restarting handle %p\n", handle); > tid = transaction->t_tid; > @@ -516,14 +508,10 @@ void jbd2_journal_lock_updates(journal_t *journal) > if (!transaction) > break; > > - spin_lock(&transaction->t_handle_lock); > - if (!atomic_read(&transaction->t_updates)) { > - spin_unlock(&transaction->t_handle_lock); > + if (!atomic_read(&transaction->t_updates)) > break; > - } > prepare_to_wait(&journal->j_wait_updates, &wait, > TASK_UNINTERRUPTIBLE); > - spin_unlock(&transaction->t_handle_lock); > write_unlock(&journal->j_state_lock); > schedule(); > finish_wait(&journal->j_wait_updates, &wait); > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h > index 2092ea2..55f7a8c 100644 > --- a/include/linux/jbd2.h > +++ b/include/linux/jbd2.h > @@ -440,9 +440,6 @@ struct transaction_chp_stats_s { > * ->j_list_lock > * > * j_state_lock > - * ->t_handle_lock > - * > - * j_state_lock > * ->j_list_lock (journal_unmap_buffer) > * > */ > @@ -538,11 +535,6 @@ struct transaction_s > struct list_head t_inode_list; > > /* > - * Protects info related to handles > - */ > - spinlock_t t_handle_lock; > - > - /* > * Longest time some handle had to wait for running transaction > */ > unsigned long t_max_wait; > -- > 1.5.5.6
Hi Yongqiang, (2011/12/22 23:07), Yongqiang Yang wrote: > On Thu, Dec 22, 2011 at 8:00 PM, Toshiyuki Okajima > <toshi.okajima@jp.fujitsu.com> wrote: >> Remove all t_handle_lock statements because they are not necessary anymore. >> >> Because there is read_lock(&journal->j_state_lock) or >> write_lock(&journal->j_state_lock) on all the forward codes beyond the place >> which needs a spin_lock(&transaction->t_handle_lock). >> >> Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> >> Cc: >> --- >> fs/jbd2/commit.c | 4 ---- >> fs/jbd2/transaction.c | 18 +++--------------- >> include/linux/jbd2.h | 8 -------- >> 3 files changed, 3 insertions(+), 27 deletions(-) >> >> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c >> index 68d704d..1030d47 100644 >> --- a/fs/jbd2/commit.c >> +++ b/fs/jbd2/commit.c >> @@ -364,22 +364,18 @@ void jbd2_journal_commit_transaction(journal_t *journal) >> stats.run.rs_running = jbd2_time_diff(commit_transaction->t_start, >> stats.run.rs_locked); >> >> - spin_lock(&commit_transaction->t_handle_lock); >> while (atomic_read(&commit_transaction->t_updates)) { >> DEFINE_WAIT(wait); >> >> prepare_to_wait(&journal->j_wait_updates, &wait, >> TASK_UNINTERRUPTIBLE); >> if (atomic_read(&commit_transaction->t_updates)) { >> - spin_unlock(&commit_transaction->t_handle_lock); >> write_unlock(&journal->j_state_lock); >> schedule(); >> write_lock(&journal->j_state_lock); >> - spin_lock(&commit_transaction->t_handle_lock); >> } >> finish_wait(&journal->j_wait_updates, &wait); >> } >> - spin_unlock(&commit_transaction->t_handle_lock); >> >> J_ASSERT (atomic_read(&commit_transaction->t_outstanding_credits) <= >> journal->j_max_transaction_buffers); >> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c >> index 76f2eca..7f84e3f 100644 >> --- a/fs/jbd2/transaction.c >> +++ b/fs/jbd2/transaction.c >> @@ -56,7 +56,6 @@ jbd2_get_transaction(journal_t *journal, transaction_t *transaction) >> 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); >> atomic_set(&transaction->t_updates, 0); >> atomic_set(&transaction->t_outstanding_credits, 0); >> atomic_set(&transaction->t_handle_count, 0); >> @@ -100,10 +99,8 @@ static inline void update_t_max_wait(transaction_t *transaction, >> if (jbd2_journal_enable_debug && >> time_after(transaction->t_start, ts)) { >> ts = jbd2_time_diff(ts, transaction->t_start); >> - spin_lock(&transaction->t_handle_lock); >> if (ts > transaction->t_max_wait) >> transaction->t_max_wait = ts; >> - spin_unlock(&transaction->t_handle_lock); > Hi Toshiyuki, > > I think you removed too much. t_handle_lock is needed here to > protect t_max_wait. I meant just removing t_handle_lock from where I > commented in your patch post out last time. Thanks for your comment. OK, I understand. This function tries to protect itself by read_lock(&journal->j_state_lock), but by only read_lock(&journal->j_state_lock) cannot protect this critical code section. 96 static inline void update_t_max_wait(transaction_t *transaction, 97 unsigned long ts) 98 { 99 #ifdef CONFIG_JBD2_DEBUG 100 if (jbd2_journal_enable_debug && 101 time_after(transaction->t_start, ts)) { 102 ts = jbd2_time_diff(ts, transaction->t_start); 103 spin_lock(&transaction->t_handle_lock); 104 if (ts > transaction->t_max_wait) 105 transaction->t_max_wait = ts; 106 spin_unlock(&transaction->t_handle_lock); 107 } 108 #endif 109 } I will delete the part of my patch for update_t_max_wait(). Due to the same reason, - jbd2_journal_extend - jbd2_journal_restart cannot be protected by my patch. So, I will delete these parts, too. Best Regards, Toshiyuki Okajima > > Yongqiang. >> } >> #endif >> } >> @@ -401,19 +398,18 @@ int jbd2_journal_extend(handle_t *handle, int nblocks) >> goto error_out; >> } >> >> - spin_lock(&transaction->t_handle_lock); >> wanted = atomic_read(&transaction->t_outstanding_credits) + nblocks; >> >> if (wanted > journal->j_max_transaction_buffers) { >> jbd_debug(3, "denied handle %p %d blocks: " >> "transaction too large\n", handle, nblocks); >> - goto unlock; >> + goto error_out; >> } >> >> if (wanted > __jbd2_log_space_left(journal)) { >> jbd_debug(3, "denied handle %p %d blocks: " >> "insufficient log space\n", handle, nblocks); >> - goto unlock; >> + goto error_out; >> } >> >> handle->h_buffer_credits += nblocks; >> @@ -421,8 +417,6 @@ int jbd2_journal_extend(handle_t *handle, int nblocks) >> result = 0; >> >> jbd_debug(3, "extended handle %p by %d\n", handle, nblocks); >> -unlock: >> - spin_unlock(&transaction->t_handle_lock); >> error_out: >> read_unlock(&journal->j_state_lock); >> out: >> @@ -464,12 +458,10 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, gfp_t gfp_mask) >> J_ASSERT(journal_current_handle() == handle); >> >> read_lock(&journal->j_state_lock); >> - spin_lock(&transaction->t_handle_lock); >> atomic_sub(handle->h_buffer_credits, >> &transaction->t_outstanding_credits); >> if (atomic_dec_and_test(&transaction->t_updates)) >> wake_up(&journal->j_wait_updates); >> - spin_unlock(&transaction->t_handle_lock); >> >> jbd_debug(2, "restarting handle %p\n", handle); >> tid = transaction->t_tid; >> @@ -516,14 +508,10 @@ void jbd2_journal_lock_updates(journal_t *journal) >> if (!transaction) >> break; >> >> - spin_lock(&transaction->t_handle_lock); >> - if (!atomic_read(&transaction->t_updates)) { >> - spin_unlock(&transaction->t_handle_lock); >> + if (!atomic_read(&transaction->t_updates)) >> break; >> - } >> prepare_to_wait(&journal->j_wait_updates, &wait, >> TASK_UNINTERRUPTIBLE); >> - spin_unlock(&transaction->t_handle_lock); >> write_unlock(&journal->j_state_lock); >> schedule(); >> finish_wait(&journal->j_wait_updates, &wait); >> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h >> index 2092ea2..55f7a8c 100644 >> --- a/include/linux/jbd2.h >> +++ b/include/linux/jbd2.h >> @@ -440,9 +440,6 @@ struct transaction_chp_stats_s { >> * ->j_list_lock >> * >> * j_state_lock >> - * ->t_handle_lock >> - * >> - * j_state_lock >> * ->j_list_lock (journal_unmap_buffer) >> * >> */ >> @@ -538,11 +535,6 @@ struct transaction_s >> struct list_head t_inode_list; >> >> /* >> - * Protects info related to handles >> - */ >> - spinlock_t t_handle_lock; >> - >> - /* >> * Longest time some handle had to wait for running transaction >> */ >> unsigned long t_max_wait; >> -- >> 1.5.5.6 > > > -- 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 68d704d..1030d47 100644 --- a/fs/jbd2/commit.c +++ b/fs/jbd2/commit.c @@ -364,22 +364,18 @@ void jbd2_journal_commit_transaction(journal_t *journal) stats.run.rs_running = jbd2_time_diff(commit_transaction->t_start, stats.run.rs_locked); - spin_lock(&commit_transaction->t_handle_lock); while (atomic_read(&commit_transaction->t_updates)) { DEFINE_WAIT(wait); prepare_to_wait(&journal->j_wait_updates, &wait, TASK_UNINTERRUPTIBLE); if (atomic_read(&commit_transaction->t_updates)) { - spin_unlock(&commit_transaction->t_handle_lock); write_unlock(&journal->j_state_lock); schedule(); write_lock(&journal->j_state_lock); - spin_lock(&commit_transaction->t_handle_lock); } finish_wait(&journal->j_wait_updates, &wait); } - spin_unlock(&commit_transaction->t_handle_lock); J_ASSERT (atomic_read(&commit_transaction->t_outstanding_credits) <= journal->j_max_transaction_buffers); diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index 76f2eca..7f84e3f 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -56,7 +56,6 @@ jbd2_get_transaction(journal_t *journal, transaction_t *transaction) 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); atomic_set(&transaction->t_updates, 0); atomic_set(&transaction->t_outstanding_credits, 0); atomic_set(&transaction->t_handle_count, 0); @@ -100,10 +99,8 @@ static inline void update_t_max_wait(transaction_t *transaction, if (jbd2_journal_enable_debug && time_after(transaction->t_start, ts)) { ts = jbd2_time_diff(ts, transaction->t_start); - spin_lock(&transaction->t_handle_lock); if (ts > transaction->t_max_wait) transaction->t_max_wait = ts; - spin_unlock(&transaction->t_handle_lock); } #endif } @@ -401,19 +398,18 @@ int jbd2_journal_extend(handle_t *handle, int nblocks) goto error_out; } - spin_lock(&transaction->t_handle_lock); wanted = atomic_read(&transaction->t_outstanding_credits) + nblocks; if (wanted > journal->j_max_transaction_buffers) { jbd_debug(3, "denied handle %p %d blocks: " "transaction too large\n", handle, nblocks); - goto unlock; + goto error_out; } if (wanted > __jbd2_log_space_left(journal)) { jbd_debug(3, "denied handle %p %d blocks: " "insufficient log space\n", handle, nblocks); - goto unlock; + goto error_out; } handle->h_buffer_credits += nblocks; @@ -421,8 +417,6 @@ int jbd2_journal_extend(handle_t *handle, int nblocks) result = 0; jbd_debug(3, "extended handle %p by %d\n", handle, nblocks); -unlock: - spin_unlock(&transaction->t_handle_lock); error_out: read_unlock(&journal->j_state_lock); out: @@ -464,12 +458,10 @@ int jbd2__journal_restart(handle_t *handle, int nblocks, gfp_t gfp_mask) J_ASSERT(journal_current_handle() == handle); read_lock(&journal->j_state_lock); - spin_lock(&transaction->t_handle_lock); atomic_sub(handle->h_buffer_credits, &transaction->t_outstanding_credits); if (atomic_dec_and_test(&transaction->t_updates)) wake_up(&journal->j_wait_updates); - spin_unlock(&transaction->t_handle_lock); jbd_debug(2, "restarting handle %p\n", handle); tid = transaction->t_tid; @@ -516,14 +508,10 @@ void jbd2_journal_lock_updates(journal_t *journal) if (!transaction) break; - spin_lock(&transaction->t_handle_lock); - if (!atomic_read(&transaction->t_updates)) { - spin_unlock(&transaction->t_handle_lock); + if (!atomic_read(&transaction->t_updates)) break; - } prepare_to_wait(&journal->j_wait_updates, &wait, TASK_UNINTERRUPTIBLE); - spin_unlock(&transaction->t_handle_lock); write_unlock(&journal->j_state_lock); schedule(); finish_wait(&journal->j_wait_updates, &wait); diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index 2092ea2..55f7a8c 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -440,9 +440,6 @@ struct transaction_chp_stats_s { * ->j_list_lock * * j_state_lock - * ->t_handle_lock - * - * j_state_lock * ->j_list_lock (journal_unmap_buffer) * */ @@ -538,11 +535,6 @@ struct transaction_s struct list_head t_inode_list; /* - * Protects info related to handles - */ - spinlock_t t_handle_lock; - - /* * Longest time some handle had to wait for running transaction */ unsigned long t_max_wait;
Remove all t_handle_lock statements because they are not necessary anymore. Because there is read_lock(&journal->j_state_lock) or write_lock(&journal->j_state_lock) on all the forward codes beyond the place which needs a spin_lock(&transaction->t_handle_lock). Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> Cc: --- fs/jbd2/commit.c | 4 ---- fs/jbd2/transaction.c | 18 +++--------------- include/linux/jbd2.h | 8 -------- 3 files changed, 3 insertions(+), 27 deletions(-)