* Re: [PATCH] jbd2: use shrink_type type instead of bool type for __jbd2_journal_clean_checkpoint_list()
2024-04-01 1:16 [PATCH] jbd2: use shrink_type type instead of bool type for __jbd2_journal_clean_checkpoint_list() Ye Bin
@ 2024-04-01 2:17 ` Zhang Yi
2024-04-01 2:44 ` Darrick J. Wong
2024-04-04 0:18 ` Dave Chinner
2 siblings, 0 replies; 6+ messages in thread
From: Zhang Yi @ 2024-04-01 2:17 UTC (permalink / raw)
To: Ye Bin, tytso, adilger.kernel, linux-ext4; +Cc: linux-kernel, jack
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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] jbd2: use shrink_type type instead of bool type for __jbd2_journal_clean_checkpoint_list()
2024-04-01 1:16 [PATCH] jbd2: use shrink_type type instead of bool type for __jbd2_journal_clean_checkpoint_list() Ye Bin
2024-04-01 2:17 ` Zhang Yi
@ 2024-04-01 2:44 ` Darrick J. Wong
2024-04-01 6:33 ` yebin (H)
2024-04-04 0:18 ` Dave Chinner
2 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2024-04-01 2:44 UTC (permalink / raw)
To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, jack
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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] jbd2: use shrink_type type instead of bool type for __jbd2_journal_clean_checkpoint_list()
2024-04-01 2:44 ` Darrick J. Wong
@ 2024-04-01 6:33 ` yebin (H)
2024-04-02 12:55 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: yebin (H) @ 2024-04-01 6:33 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, jack
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
>>
>>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] jbd2: use shrink_type type instead of bool type for __jbd2_journal_clean_checkpoint_list()
2024-04-01 6:33 ` yebin (H)
@ 2024-04-02 12:55 ` Jan Kara
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2024-04-02 12:55 UTC (permalink / raw)
To: yebin (H)
Cc: Darrick J. Wong, tytso, adilger.kernel, linux-ext4, linux-kernel,
jack
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
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] jbd2: use shrink_type type instead of bool type for __jbd2_journal_clean_checkpoint_list()
2024-04-01 1:16 [PATCH] jbd2: use shrink_type type instead of bool type for __jbd2_journal_clean_checkpoint_list() Ye Bin
2024-04-01 2:17 ` Zhang Yi
2024-04-01 2:44 ` Darrick J. Wong
@ 2024-04-04 0:18 ` Dave Chinner
2 siblings, 0 replies; 6+ messages in thread
From: Dave Chinner @ 2024-04-04 0:18 UTC (permalink / raw)
To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, jack
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.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 6+ messages in thread