Re: [PATCH 1/3] jbd2: fix checkpoint cleanup performance regression

[Jan Kara <jack@suse.cz>]

On Fri 14-07-23 10:55:26, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> 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 <yi.zhang@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								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 <jack@suse.com>
SUSE Labs, CR