| Message ID | 20240401011614.3650958-1-yebin10@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | jbd2: use shrink_type type instead of bool type for __jbd2_journal_clean_checkpoint_list() | expand |
On 2024/4/1 9:16, 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> Make sense, thanks for the cleanup. Reviewed-by: Zhang Yi <yi.zhang@huawei.com> > --- > fs/jbd2/checkpoint.c | 9 +++------ > fs/jbd2/commit.c | 2 +- > include/linux/jbd2.h | 4 +++- > 3 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c > index 1c97e64c4784..d6e8b80a4078 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 > * > @@ -476,17 +474,16 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, > * > * 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; > > 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 +524,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); >
On Mon, Apr 01, 2024 at 09:16:14AM +0800, 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> > --- > fs/jbd2/checkpoint.c | 9 +++------ > fs/jbd2/commit.c | 2 +- > include/linux/jbd2.h | 4 +++- > 3 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c > index 1c97e64c4784..d6e8b80a4078 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 > * > @@ -476,17 +474,16 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, > * > * 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; > > transaction = journal->j_checkpoint_transactions; > if (!transaction) > return; > > - type = destroy ? SHRINK_DESTROY : SHRINK_BUSY_STOP; What is supposed to happen if the caller passes in SHRINK_BUSY_SKIP? --D > last_transaction = transaction->t_cpprev; > next_transaction = transaction; > do { > @@ -527,7 +524,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/1 10:44, Darrick J. Wong wrote: > On Mon, Apr 01, 2024 at 09:16:14AM +0800, 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> >> --- >> fs/jbd2/checkpoint.c | 9 +++------ >> fs/jbd2/commit.c | 2 +- >> include/linux/jbd2.h | 4 +++- >> 3 files changed, 7 insertions(+), 8 deletions(-) >> >> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c >> index 1c97e64c4784..d6e8b80a4078 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 >> * >> @@ -476,17 +474,16 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, >> * >> * 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; >> >> transaction = journal->j_checkpoint_transactions; >> if (!transaction) >> return; >> >> - type = destroy ? SHRINK_DESTROY : SHRINK_BUSY_STOP; > What is supposed to happen if the caller passes in SHRINK_BUSY_SKIP? > > --D If SHRING_BUSY_SKIP is passed, the buffers in use will be skipped during traversal and release.Currently, SHRINKING_BUSY_SKIP is used in the memory reclamation process. >> last_transaction = transaction->t_cpprev; >> next_transaction = transaction; >> do { >> @@ -527,7 +524,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 Mon 01-04-24 14:33:25, yebin (H) wrote: > On 2024/4/1 10:44, Darrick J. Wong wrote: > > On Mon, Apr 01, 2024 at 09:16:14AM +0800, 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> > > > --- > > > fs/jbd2/checkpoint.c | 9 +++------ > > > fs/jbd2/commit.c | 2 +- > > > include/linux/jbd2.h | 4 +++- > > > 3 files changed, 7 insertions(+), 8 deletions(-) > > > > > > diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c > > > index 1c97e64c4784..d6e8b80a4078 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 > > > * > > > @@ -476,17 +474,16 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, > > > * > > > * 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) The comment above this function needs updating after this change. > > > { > > > transaction_t *transaction, *last_transaction, *next_transaction; > > > - enum shrink_type type; > > > bool released; > > > transaction = journal->j_checkpoint_transactions; > > > if (!transaction) > > > return; > > > - type = destroy ? SHRINK_DESTROY : SHRINK_BUSY_STOP; > > What is supposed to happen if the caller passes in SHRINK_BUSY_SKIP? > > > > --D > > If SHRING_BUSY_SKIP is passed, the buffers in use will be skipped during traversal > and release.Currently, SHRINKING_BUSY_SKIP is used in the memory reclamation process. I guess Darrick was wondering whether there's some usefulness in calling __jbd2_journal_clean_checkpoint_list() with SHRINKING_BUSY_SKIP. I agree it does no harm but as we have seen in the past, it just wastes CPU cycles scanning the buffer list in some cases so that's why we created SHRINKING_BUSY_STOP. I also agree the 'bool' argument isn't great and the enum is actually explaining more so I'm in favor of switching to enum but perhaps we can have WARN_ON_ONCE(type == SHRINKING_BUSY_SKIP) (perhaps with a short explanation in the comment above the function) to see if we accidentally didn't grow unexpected use. Honza
On Mon, Apr 01, 2024 at 09:16:14AM +0800, 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> > --- > fs/jbd2/checkpoint.c | 9 +++------ > fs/jbd2/commit.c | 2 +- > include/linux/jbd2.h | 4 +++- > 3 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c > index 1c97e64c4784..d6e8b80a4078 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}; So this is a local, internal definition, but .... > 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}; ... this exports it to the world. That's a problem, because the "SHRINK_*" namespace is owned by the memory management subsystem for controlling memory shrinkers. e.g. SHRINK_STOP and SHRINK_EMPTY are already defined and in wide use across the kernel in the cache shrinker infrastructure. IOWS, these new types needs to be prefixed to indicate they are JBD2 objects. i.e enum jbd2_shrink_type {JBD2_SHRINK_DESTROY, JBD2_.... }; So that people who are looking at memory shrinker stuff don't get horribly confused by jbd2 using shrinker namespaces for things that are completely unrelated to memory reclaim... -Dave.
diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c index 1c97e64c4784..d6e8b80a4078 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 * @@ -476,17 +474,16 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, * * 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; 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 +524,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 | 9 +++------ fs/jbd2/commit.c | 2 +- include/linux/jbd2.h | 4 +++- 3 files changed, 7 insertions(+), 8 deletions(-)