jbd2: fix checkpoint cleanup performance regression
commit 373ac521799d9e97061515aca6ec6621789036bb upstream. 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: stable@kernel.org Fixes: b98dba273a0e ("jbd2: remove journal_clean_one_cp_list()") Signed-off-by: Zhang Yi <yi.zhang@huawei.com> Reviewed-by: Jan Kara <jack@suse.cz> Link: https://lore.kernel.org/r/20230714025528.564988-2-yi.zhang@huaweicloud.com Signed-off-by: Theodore Ts'o <tytso@mit.edu> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
This commit is contained in:
parent
6ea18981bb
commit
c5f23305f8
@ -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
|
||||
|
Loading…
Reference in New Issue
Block a user