| Message ID | 95fa94cbeb4bb0275430a6721a588bd738d5a9aa.1642044249.git.riteshh@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4/jbd2: inline_data fixes and some cleanups | expand |
On Thu 13-01-22 08:56:28, Ritesh Harjani wrote: > No functionality change as such in this patch. This only refactors the > common piece of code which waits for t_updates to finish into a common > function named as jbd2_journal_wait_updates(journal_t *) > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> Just one nit, otherwise. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> > @@ -1757,6 +1757,35 @@ static inline unsigned long jbd2_log_space_left(journal_t *journal) > return max_t(long, free, 0); > } > > +/* > + * Waits for any outstanding t_updates to finish. > + * This is called with write j_state_lock held. > + */ > +static inline void jbd2_journal_wait_updates(journal_t *journal) > +{ > + transaction_t *commit_transaction = journal->j_running_transaction; > + > + if (!commit_transaction) > + return; > + > + 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); > +} > + I don't think making this inline makes sence. Neither the commit code nor jbd2_journal_lock_updates() are so hot that it would warrant this large inline function... Honza
On 22/01/13 12:30PM, Jan Kara wrote: > On Thu 13-01-22 08:56:28, Ritesh Harjani wrote: > > No functionality change as such in this patch. This only refactors the > > common piece of code which waits for t_updates to finish into a common > > function named as jbd2_journal_wait_updates(journal_t *) > > > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> > > Just one nit, otherwise. Feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> > > > @@ -1757,6 +1757,35 @@ static inline unsigned long jbd2_log_space_left(journal_t *journal) > > return max_t(long, free, 0); > > } > > > > +/* > > + * Waits for any outstanding t_updates to finish. > > + * This is called with write j_state_lock held. > > + */ > > +static inline void jbd2_journal_wait_updates(journal_t *journal) > > +{ > > + transaction_t *commit_transaction = journal->j_running_transaction; > > + > > + if (!commit_transaction) > > + return; > > + > > + 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); > > +} > > + > > I don't think making this inline makes sence. Neither the commit code nor > jbd2_journal_lock_updates() are so hot that it would warrant this large > inline function... Yes, make sense. Thanks for the review. Will do the needful in v2. -ritesh > > Honza > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c index 3cc4ab2ba7f4..428364f107be 100644 --- a/fs/jbd2/commit.c +++ b/fs/jbd2/commit.c @@ -484,22 +484,9 @@ 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); + // waits for any t_updates to finish + jbd2_journal_wait_updates(journal); - 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); commit_transaction->t_state = T_SWITCH; write_unlock(&journal->j_state_lock); @@ -817,7 +804,7 @@ void jbd2_journal_commit_transaction(journal_t *journal) commit_transaction->t_state = T_COMMIT_DFLUSH; write_unlock(&journal->j_state_lock); - /* + /* * If the journal is not located on the file system device, * then we must flush the file system device before we issue * the commit record diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index 6a3caedd2285..89a955ab1557 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -449,7 +449,7 @@ static int start_this_handle(journal_t *journal, handle_t *handle, } /* OK, account for the buffers that this operation expects to - * use and add the handle to the running transaction. + * use and add the handle to the running transaction. */ update_t_max_wait(transaction, ts); handle->h_transaction = transaction; @@ -863,27 +863,9 @@ void jbd2_journal_lock_updates(journal_t *journal) write_lock(&journal->j_state_lock); } - /* Wait until there are no running updates */ - while (1) { - transaction_t *transaction = journal->j_running_transaction; + /* Wait until there are no running t_updates */ + jbd2_journal_wait_updates(journal); - if (!transaction) - break; - - spin_lock(&transaction->t_handle_lock); - prepare_to_wait(&journal->j_wait_updates, &wait, - TASK_UNINTERRUPTIBLE); - if (!atomic_read(&transaction->t_updates)) { - spin_unlock(&transaction->t_handle_lock); - finish_wait(&journal->j_wait_updates, &wait); - break; - } - spin_unlock(&transaction->t_handle_lock); - write_unlock(&journal->j_state_lock); - schedule(); - finish_wait(&journal->j_wait_updates, &wait); - write_lock(&journal->j_state_lock); - } write_unlock(&journal->j_state_lock); /* diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index f76598265896..34b051aa9009 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -594,7 +594,7 @@ struct transaction_s */ unsigned long t_log_start; - /* + /* * Number of buffers on the t_buffers list [j_list_lock, no locks * needed for jbd2 thread] */ @@ -1757,6 +1757,35 @@ static inline unsigned long jbd2_log_space_left(journal_t *journal) return max_t(long, free, 0); } +/* + * Waits for any outstanding t_updates to finish. + * This is called with write j_state_lock held. + */ +static inline void jbd2_journal_wait_updates(journal_t *journal) +{ + transaction_t *commit_transaction = journal->j_running_transaction; + + if (!commit_transaction) + return; + + 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); +} + /* * Definitions which augment the buffer_head layer */
No functionality change as such in this patch. This only refactors the common piece of code which waits for t_updates to finish into a common function named as jbd2_journal_wait_updates(journal_t *) Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> --- fs/jbd2/commit.c | 19 +++---------------- fs/jbd2/transaction.c | 24 +++--------------------- include/linux/jbd2.h | 31 ++++++++++++++++++++++++++++++- 3 files changed, 36 insertions(+), 38 deletions(-)