Skip to content

Commit 373ac52

Browse files
zhangyi089tytso
authored andcommitted
jbd2: fix checkpoint cleanup performance regression
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 Cc: [email protected] Fixes: b98dba2 ("jbd2: remove journal_clean_one_cp_list()") Signed-off-by: Zhang Yi <[email protected]> Reviewed-by: Jan Kara <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Theodore Ts'o <[email protected]>
1 parent 4eea9fb commit 373ac52

File tree

1 file changed

+14
-6
lines changed

1 file changed

+14
-6
lines changed

fs/jbd2/checkpoint.c

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,8 @@ int jbd2_cleanup_journal_tail(journal_t *journal)
349349

350350
/* Checkpoint list management */
351351

352+
enum shrink_type {SHRINK_DESTROY, SHRINK_BUSY_STOP, SHRINK_BUSY_SKIP};
353+
352354
/*
353355
* journal_shrink_one_cp_list
354356
*
@@ -360,7 +362,8 @@ int jbd2_cleanup_journal_tail(journal_t *journal)
360362
* Called with j_list_lock held.
361363
*/
362364
static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
363-
bool destroy, bool *released)
365+
enum shrink_type type,
366+
bool *released)
364367
{
365368
struct journal_head *last_jh;
366369
struct journal_head *next_jh = jh;
@@ -376,12 +379,15 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
376379
jh = next_jh;
377380
next_jh = jh->b_cpnext;
378381

379-
if (destroy) {
382+
if (type == SHRINK_DESTROY) {
380383
ret = __jbd2_journal_remove_checkpoint(jh);
381384
} else {
382385
ret = jbd2_journal_try_remove_checkpoint(jh);
383-
if (ret < 0)
384-
continue;
386+
if (ret < 0) {
387+
if (type == SHRINK_BUSY_SKIP)
388+
continue;
389+
break;
390+
}
385391
}
386392

387393
nr_freed++;
@@ -445,7 +451,7 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
445451
tid = transaction->t_tid;
446452

447453
freed = journal_shrink_one_cp_list(transaction->t_checkpoint_list,
448-
false, &released);
454+
SHRINK_BUSY_SKIP, &released);
449455
nr_freed += freed;
450456
(*nr_to_scan) -= min(*nr_to_scan, freed);
451457
if (*nr_to_scan == 0)
@@ -485,19 +491,21 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
485491
void __jbd2_journal_clean_checkpoint_list(journal_t *journal, bool destroy)
486492
{
487493
transaction_t *transaction, *last_transaction, *next_transaction;
494+
enum shrink_type type;
488495
bool released;
489496

490497
transaction = journal->j_checkpoint_transactions;
491498
if (!transaction)
492499
return;
493500

501+
type = destroy ? SHRINK_DESTROY : SHRINK_BUSY_STOP;
494502
last_transaction = transaction->t_cpprev;
495503
next_transaction = transaction;
496504
do {
497505
transaction = next_transaction;
498506
next_transaction = transaction->t_cpnext;
499507
journal_shrink_one_cp_list(transaction->t_checkpoint_list,
500-
destroy, &released);
508+
type, &released);
501509
/*
502510
* This function only frees up some memory if possible so we
503511
* dont have an obligation to finish processing. Bail out if

0 commit comments

Comments
 (0)