| Message ID | 20240407065355.1528580-2-yebin10@huawei.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | jbd2: use shrink_type type instead of bool type for __jbd2_journal_clean_checkpoint_list() | expand |
On Sun 07-04-24 14:53:54, Ye Bin wrote: > "enum shrink_type" can clearly express the meaning of the parameter of > __jbd2_journal_clean_checkpoint_list(), and there is no need to use the > bool type. > > Signed-off-by: Ye Bin <yebin10@huawei.com> Looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/jbd2/checkpoint.c | 16 +++++++++------- > fs/jbd2/commit.c | 2 +- > include/linux/jbd2.h | 4 +++- > 3 files changed, 13 insertions(+), 9 deletions(-) > > diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c > index 1c97e64c4784..80c0ab98bc63 100644 > --- a/fs/jbd2/checkpoint.c > +++ b/fs/jbd2/checkpoint.c > @@ -337,8 +337,6 @@ int jbd2_cleanup_journal_tail(journal_t *journal) > > /* Checkpoint list management */ > > -enum shrink_type {SHRINK_DESTROY, SHRINK_BUSY_STOP, SHRINK_BUSY_SKIP}; > - > /* > * journal_shrink_one_cp_list > * > @@ -472,21 +470,25 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, > * journal_clean_checkpoint_list > * > * Find all the written-back checkpoint buffers in the journal and release them. > - * If 'destroy' is set, release all buffers unconditionally. > + * If 'type' is SHRINK_DESTROY, release all buffers unconditionally. If 'type' > + * is SHRINK_BUSY_STOP, will stop release buffers if encounters a busy buffer. > + * To avoid wasting CPU cycles scanning the buffer list in some cases, don't > + * pass SHRINK_BUSY_SKIP 'type' for this function. > * > * Called with j_list_lock held. > */ > -void __jbd2_journal_clean_checkpoint_list(journal_t *journal, bool destroy) > +void __jbd2_journal_clean_checkpoint_list(journal_t *journal, > + enum shrink_type type) > { > transaction_t *transaction, *last_transaction, *next_transaction; > - enum shrink_type type; > bool released; > > + WARN_ON_ONCE(type == SHRINK_BUSY_SKIP); > + > transaction = journal->j_checkpoint_transactions; > if (!transaction) > return; > > - type = destroy ? SHRINK_DESTROY : SHRINK_BUSY_STOP; > last_transaction = transaction->t_cpprev; > next_transaction = transaction; > do { > @@ -527,7 +529,7 @@ void jbd2_journal_destroy_checkpoint(journal_t *journal) > spin_unlock(&journal->j_list_lock); > break; > } > - __jbd2_journal_clean_checkpoint_list(journal, true); > + __jbd2_journal_clean_checkpoint_list(journal, SHRINK_DESTROY); > spin_unlock(&journal->j_list_lock); > cond_resched(); > } > diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c > index 5e122586e06e..78ebd04ac97d 100644 > --- a/fs/jbd2/commit.c > +++ b/fs/jbd2/commit.c > @@ -501,7 +501,7 @@ void jbd2_journal_commit_transaction(journal_t *journal) > * frees some memory > */ > spin_lock(&journal->j_list_lock); > - __jbd2_journal_clean_checkpoint_list(journal, false); > + __jbd2_journal_clean_checkpoint_list(journal, SHRINK_BUSY_STOP); > spin_unlock(&journal->j_list_lock); > > jbd2_debug(3, "JBD2: commit phase 1\n"); > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h > index 971f3e826e15..58a961999d70 100644 > --- a/include/linux/jbd2.h > +++ b/include/linux/jbd2.h > @@ -1434,7 +1434,9 @@ void jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block); > extern void jbd2_journal_commit_transaction(journal_t *); > > /* Checkpoint list management */ > -void __jbd2_journal_clean_checkpoint_list(journal_t *journal, bool destroy); > +enum shrink_type {SHRINK_DESTROY, SHRINK_BUSY_STOP, SHRINK_BUSY_SKIP}; > + > +void __jbd2_journal_clean_checkpoint_list(journal_t *journal, enum shrink_type type); > unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, unsigned long *nr_to_scan); > int __jbd2_journal_remove_checkpoint(struct journal_head *); > int jbd2_journal_try_remove_checkpoint(struct journal_head *jh); > -- > 2.31.1 >
On 2024/4/7 14:53, Ye Bin wrote: > "enum shrink_type" can clearly express the meaning of the parameter of > __jbd2_journal_clean_checkpoint_list(), and there is no need to use the > bool type. > > Signed-off-by: Ye Bin <yebin10@huawei.com> Looks good to me. Reviewed-by: Zhang Yi <yi.zhang@huawei.com> > --- > fs/jbd2/checkpoint.c | 16 +++++++++------- > fs/jbd2/commit.c | 2 +- > include/linux/jbd2.h | 4 +++- > 3 files changed, 13 insertions(+), 9 deletions(-) > > diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c > index 1c97e64c4784..80c0ab98bc63 100644 > --- a/fs/jbd2/checkpoint.c > +++ b/fs/jbd2/checkpoint.c > @@ -337,8 +337,6 @@ int jbd2_cleanup_journal_tail(journal_t *journal) > > /* Checkpoint list management */ > > -enum shrink_type {SHRINK_DESTROY, SHRINK_BUSY_STOP, SHRINK_BUSY_SKIP}; > - > /* > * journal_shrink_one_cp_list > * > @@ -472,21 +470,25 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, > * journal_clean_checkpoint_list > * > * Find all the written-back checkpoint buffers in the journal and release them. > - * If 'destroy' is set, release all buffers unconditionally. > + * If 'type' is SHRINK_DESTROY, release all buffers unconditionally. If 'type' > + * is SHRINK_BUSY_STOP, will stop release buffers if encounters a busy buffer. > + * To avoid wasting CPU cycles scanning the buffer list in some cases, don't > + * pass SHRINK_BUSY_SKIP 'type' for this function. > * > * Called with j_list_lock held. > */ > -void __jbd2_journal_clean_checkpoint_list(journal_t *journal, bool destroy) > +void __jbd2_journal_clean_checkpoint_list(journal_t *journal, > + enum shrink_type type) > { > transaction_t *transaction, *last_transaction, *next_transaction; > - enum shrink_type type; > bool released; > > + WARN_ON_ONCE(type == SHRINK_BUSY_SKIP); > + > transaction = journal->j_checkpoint_transactions; > if (!transaction) > return; > > - type = destroy ? SHRINK_DESTROY : SHRINK_BUSY_STOP; > last_transaction = transaction->t_cpprev; > next_transaction = transaction; > do { > @@ -527,7 +529,7 @@ void jbd2_journal_destroy_checkpoint(journal_t *journal) > spin_unlock(&journal->j_list_lock); > break; > } > - __jbd2_journal_clean_checkpoint_list(journal, true); > + __jbd2_journal_clean_checkpoint_list(journal, SHRINK_DESTROY); > spin_unlock(&journal->j_list_lock); > cond_resched(); > } > diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c > index 5e122586e06e..78ebd04ac97d 100644 > --- a/fs/jbd2/commit.c > +++ b/fs/jbd2/commit.c > @@ -501,7 +501,7 @@ void jbd2_journal_commit_transaction(journal_t *journal) > * frees some memory > */ > spin_lock(&journal->j_list_lock); > - __jbd2_journal_clean_checkpoint_list(journal, false); > + __jbd2_journal_clean_checkpoint_list(journal, SHRINK_BUSY_STOP); > spin_unlock(&journal->j_list_lock); > > jbd2_debug(3, "JBD2: commit phase 1\n"); > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h > index 971f3e826e15..58a961999d70 100644 > --- a/include/linux/jbd2.h > +++ b/include/linux/jbd2.h > @@ -1434,7 +1434,9 @@ void jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block); > extern void jbd2_journal_commit_transaction(journal_t *); > > /* Checkpoint list management */ > -void __jbd2_journal_clean_checkpoint_list(journal_t *journal, bool destroy); > +enum shrink_type {SHRINK_DESTROY, SHRINK_BUSY_STOP, SHRINK_BUSY_SKIP}; > + > +void __jbd2_journal_clean_checkpoint_list(journal_t *journal, enum shrink_type type); > unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, unsigned long *nr_to_scan); > int __jbd2_journal_remove_checkpoint(struct journal_head *); > int jbd2_journal_try_remove_checkpoint(struct journal_head *jh); >
diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c index 1c97e64c4784..80c0ab98bc63 100644 --- a/fs/jbd2/checkpoint.c +++ b/fs/jbd2/checkpoint.c @@ -337,8 +337,6 @@ int jbd2_cleanup_journal_tail(journal_t *journal) /* Checkpoint list management */ -enum shrink_type {SHRINK_DESTROY, SHRINK_BUSY_STOP, SHRINK_BUSY_SKIP}; - /* * journal_shrink_one_cp_list * @@ -472,21 +470,25 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, * journal_clean_checkpoint_list * * Find all the written-back checkpoint buffers in the journal and release them. - * If 'destroy' is set, release all buffers unconditionally. + * If 'type' is SHRINK_DESTROY, release all buffers unconditionally. If 'type' + * is SHRINK_BUSY_STOP, will stop release buffers if encounters a busy buffer. + * To avoid wasting CPU cycles scanning the buffer list in some cases, don't + * pass SHRINK_BUSY_SKIP 'type' for this function. * * Called with j_list_lock held. */ -void __jbd2_journal_clean_checkpoint_list(journal_t *journal, bool destroy) +void __jbd2_journal_clean_checkpoint_list(journal_t *journal, + enum shrink_type type) { transaction_t *transaction, *last_transaction, *next_transaction; - enum shrink_type type; bool released; + WARN_ON_ONCE(type == SHRINK_BUSY_SKIP); + transaction = journal->j_checkpoint_transactions; if (!transaction) return; - type = destroy ? SHRINK_DESTROY : SHRINK_BUSY_STOP; last_transaction = transaction->t_cpprev; next_transaction = transaction; do { @@ -527,7 +529,7 @@ void jbd2_journal_destroy_checkpoint(journal_t *journal) spin_unlock(&journal->j_list_lock); break; } - __jbd2_journal_clean_checkpoint_list(journal, true); + __jbd2_journal_clean_checkpoint_list(journal, SHRINK_DESTROY); spin_unlock(&journal->j_list_lock); cond_resched(); } diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c index 5e122586e06e..78ebd04ac97d 100644 --- a/fs/jbd2/commit.c +++ b/fs/jbd2/commit.c @@ -501,7 +501,7 @@ void jbd2_journal_commit_transaction(journal_t *journal) * frees some memory */ spin_lock(&journal->j_list_lock); - __jbd2_journal_clean_checkpoint_list(journal, false); + __jbd2_journal_clean_checkpoint_list(journal, SHRINK_BUSY_STOP); spin_unlock(&journal->j_list_lock); jbd2_debug(3, "JBD2: commit phase 1\n"); diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index 971f3e826e15..58a961999d70 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -1434,7 +1434,9 @@ void jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block); extern void jbd2_journal_commit_transaction(journal_t *); /* Checkpoint list management */ -void __jbd2_journal_clean_checkpoint_list(journal_t *journal, bool destroy); +enum shrink_type {SHRINK_DESTROY, SHRINK_BUSY_STOP, SHRINK_BUSY_SKIP}; + +void __jbd2_journal_clean_checkpoint_list(journal_t *journal, enum shrink_type type); unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, unsigned long *nr_to_scan); int __jbd2_journal_remove_checkpoint(struct journal_head *); int jbd2_journal_try_remove_checkpoint(struct journal_head *jh);
"enum shrink_type" can clearly express the meaning of the parameter of __jbd2_journal_clean_checkpoint_list(), and there is no need to use the bool type. Signed-off-by: Ye Bin <yebin10@huawei.com> --- fs/jbd2/checkpoint.c | 16 +++++++++------- fs/jbd2/commit.c | 2 +- include/linux/jbd2.h | 4 +++- 3 files changed, 13 insertions(+), 9 deletions(-)