From: Zhang Yi <[email protected]>
These patch set includes 2 fixes and 1 cleanup about "jbd2: fix several
checkpoint inconsistent issues" patch set[1] recently applied in dev
branch. The first patch fix a performance regression introduced while
merging journal_shrink_one_cp_list(). The second one add a missing
check before dropping checkpoint buffer that could lead to filesystem
inconsistent. The last patch which is from Yang Li, remove the unused
__cp_buffer_busy() helper. Please see the corresponding patch for
details.
[1] https://lore.kernel.org/linux-ext4/[email protected]/T/#t
Thanks,
Yi.
Yang Li (1):
jbd2: remove unused function '__cp_buffer_busy'
Zhang Yi (1):
jbd2: fix checkpoint cleanup performance regression
Zhihao Cheng (1):
jbd2: Check 'jh->b_transaction' before remove it from checkpoint
fs/jbd2/checkpoint.c | 34 ++++++++++++++++------------------
1 file changed, 16 insertions(+), 18 deletions(-)
--
2.39.2
From: Zhang Yi <[email protected]>
journal_clean_one_cp_list() has been merged into
journal_shrink_one_cp_list(), but do chekpoint buffer cleanup from the
committing process is just a best effort, it should stop scan once it
meet a busy buffer, or else it will cause a lot of invalid buffer scan
and checks. We catch a performance regression when doing fs_mark tests
below.
Test cmd:
./fs_mark -d scratch -s 1024 -n 10000 -t 1 -D 100 -N 100
Before merging checkpoint buffer cleanup:
FSUse% Count Size Files/sec App Overhead
95 10000 1024 8304.9 49033
After merging checkpoint buffer cleanup:
FSUse% Count Size Files/sec App Overhead
95 10000 1024 7649.0 50012
FSUse% Count Size Files/sec App Overhead
95 10000 1024 2107.1 50871
After merging checkpoint buffer cleanup, the total loop count in
journal_shrink_one_cp_list() could be up to 6,261,600+ (50,000+ ~
100,000+ in general), most of them are invalid. This patch fix it
through passing 'shrink_type' into journal_shrink_one_cp_list() and add
a new 'SHRINK_BUSY_STOP' to indicate it should stop once meet a busy
buffer. After fix, the loop count descending back to 10,000+.
After this fix:
FSUse% Count Size Files/sec App Overhead
95 10000 1024 8558.4 49109
Fixes: b98dba273a0e ("jbd2: remove journal_clean_one_cp_list()")
Signed-off-by: Zhang Yi <[email protected]>
---
fs/jbd2/checkpoint.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
index 9ec91017a7f3..936c6d758a65 100644
--- a/fs/jbd2/checkpoint.c
+++ b/fs/jbd2/checkpoint.c
@@ -349,6 +349,8 @@ 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
*
@@ -360,7 +362,8 @@ int jbd2_cleanup_journal_tail(journal_t *journal)
* Called with j_list_lock held.
*/
static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
- bool destroy, bool *released)
+ enum shrink_type type,
+ bool *released)
{
struct journal_head *last_jh;
struct journal_head *next_jh = jh;
@@ -376,12 +379,15 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
jh = next_jh;
next_jh = jh->b_cpnext;
- if (destroy) {
+ if (type == SHRINK_DESTROY) {
ret = __jbd2_journal_remove_checkpoint(jh);
} else {
ret = jbd2_journal_try_remove_checkpoint(jh);
- if (ret < 0)
- continue;
+ if (ret < 0) {
+ if (type == SHRINK_BUSY_SKIP)
+ continue;
+ break;
+ }
}
nr_freed++;
@@ -445,7 +451,7 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
tid = transaction->t_tid;
freed = journal_shrink_one_cp_list(transaction->t_checkpoint_list,
- false, &released);
+ SHRINK_BUSY_SKIP, &released);
nr_freed += freed;
(*nr_to_scan) -= min(*nr_to_scan, freed);
if (*nr_to_scan == 0)
@@ -485,19 +491,21 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
void __jbd2_journal_clean_checkpoint_list(journal_t *journal, bool destroy)
{
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 {
transaction = next_transaction;
next_transaction = transaction->t_cpnext;
journal_shrink_one_cp_list(transaction->t_checkpoint_list,
- destroy, &released);
+ type, &released);
/*
* This function only frees up some memory if possible so we
* dont have an obligation to finish processing. Bail out if
--
2.39.2
On Fri 14-07-23 10:55:26, Zhang Yi wrote:
> From: Zhang Yi <[email protected]>
>
> journal_clean_one_cp_list() has been merged into
> journal_shrink_one_cp_list(), but do chekpoint buffer cleanup from the
> committing process is just a best effort, it should stop scan once it
> meet a busy buffer, or else it will cause a lot of invalid buffer scan
> and checks. We catch a performance regression when doing fs_mark tests
> below.
>
> Test cmd:
> ./fs_mark -d scratch -s 1024 -n 10000 -t 1 -D 100 -N 100
>
> Before merging checkpoint buffer cleanup:
> FSUse% Count Size Files/sec App Overhead
> 95 10000 1024 8304.9 49033
>
> After merging checkpoint buffer cleanup:
> FSUse% Count Size Files/sec App Overhead
> 95 10000 1024 7649.0 50012
> FSUse% Count Size Files/sec App Overhead
> 95 10000 1024 2107.1 50871
>
> After merging checkpoint buffer cleanup, the total loop count in
> journal_shrink_one_cp_list() could be up to 6,261,600+ (50,000+ ~
> 100,000+ in general), most of them are invalid. This patch fix it
> through passing 'shrink_type' into journal_shrink_one_cp_list() and add
> a new 'SHRINK_BUSY_STOP' to indicate it should stop once meet a busy
> buffer. After fix, the loop count descending back to 10,000+.
>
> After this fix:
> FSUse% Count Size Files/sec App Overhead
> 95 10000 1024 8558.4 49109
>
> Fixes: b98dba273a0e ("jbd2: remove journal_clean_one_cp_list()")
> Signed-off-by: Zhang Yi <[email protected]>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/jbd2/checkpoint.c | 20 ++++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index 9ec91017a7f3..936c6d758a65 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -349,6 +349,8 @@ 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
> *
> @@ -360,7 +362,8 @@ int jbd2_cleanup_journal_tail(journal_t *journal)
> * Called with j_list_lock held.
> */
> static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
> - bool destroy, bool *released)
> + enum shrink_type type,
> + bool *released)
> {
> struct journal_head *last_jh;
> struct journal_head *next_jh = jh;
> @@ -376,12 +379,15 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
> jh = next_jh;
> next_jh = jh->b_cpnext;
>
> - if (destroy) {
> + if (type == SHRINK_DESTROY) {
> ret = __jbd2_journal_remove_checkpoint(jh);
> } else {
> ret = jbd2_journal_try_remove_checkpoint(jh);
> - if (ret < 0)
> - continue;
> + if (ret < 0) {
> + if (type == SHRINK_BUSY_SKIP)
> + continue;
> + break;
> + }
> }
>
> nr_freed++;
> @@ -445,7 +451,7 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
> tid = transaction->t_tid;
>
> freed = journal_shrink_one_cp_list(transaction->t_checkpoint_list,
> - false, &released);
> + SHRINK_BUSY_SKIP, &released);
> nr_freed += freed;
> (*nr_to_scan) -= min(*nr_to_scan, freed);
> if (*nr_to_scan == 0)
> @@ -485,19 +491,21 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
> void __jbd2_journal_clean_checkpoint_list(journal_t *journal, bool destroy)
> {
> 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 {
> transaction = next_transaction;
> next_transaction = transaction->t_cpnext;
> journal_shrink_one_cp_list(transaction->t_checkpoint_list,
> - destroy, &released);
> + type, &released);
> /*
> * This function only frees up some memory if possible so we
> * dont have an obligation to finish processing. Bail out if
> --
> 2.39.2
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Fri, 14 Jul 2023 10:55:25 +0800, Zhang Yi wrote:
> These patch set includes 2 fixes and 1 cleanup about "jbd2: fix several
> checkpoint inconsistent issues" patch set[1] recently applied in dev
> branch. The first patch fix a performance regression introduced while
> merging journal_shrink_one_cp_list(). The second one add a missing
> check before dropping checkpoint buffer that could lead to filesystem
> inconsistent. The last patch which is from Yang Li, remove the unused
> __cp_buffer_busy() helper. Please see the corresponding patch for
> details.
>
> [...]
Applied, thanks!
[1/3] jbd2: fix checkpoint cleanup performance regression
commit: 373ac521799d9e97061515aca6ec6621789036bb
[2/3] jbd2: Check 'jh->b_transaction' before remove it from checkpoint
commit: 590a809ff743e7bd890ba5fb36bc38e20a36de53
[3/3] jbd2: remove unused function '__cp_buffer_busy'
commit: 5f02a30eac5cc1c081cbdb42d19fd0ded00b0618
Best regards,
--
Theodore Ts'o <[email protected]>