Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 5 May 2023 15:13:00 +0200
From:   Jan Kara <jack@suse.cz>
To:     Zhang Yi <yi.zhang@huaweicloud.com>
Cc:     linux-ext4@vger.kernel.org, tytso@mit.edu,
        adilger.kernel@dilger.ca, jack@suse.cz, yi.zhang@huawei.com,
        yukuai3@huawei.com, chengzhihao1@huawei.com
Subject: Re: [PATCH v2 2/3] jbd2: remove t_checkpoint_io_list
Message-ID: <20230505131300.57p23wh53m3iabod@quack3>
References: <20230505123219.4135141-1-yi.zhang@huaweicloud.com>
 <20230505123219.4135141-2-yi.zhang@huaweicloud.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230505123219.4135141-2-yi.zhang@huaweicloud.com>
Precedence: bulk

On Fri 05-05-23 20:32:18, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Since t_checkpoint_io_list was stop using in jbd2_log_do_checkpoint()
> now, it's time to remove the whole t_checkpoint_io_list logic.
> 
> 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 | 42 ++----------------------------------------
>  fs/jbd2/commit.c     |  3 +--
>  include/linux/jbd2.h |  6 ------
>  3 files changed, 3 insertions(+), 48 deletions(-)
> 
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index ae1ebfb8bc86..2b62154e9f1e 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -27,7 +27,7 @@
>   *
>   * Called with j_list_lock held.
>   */
> -static inline void __buffer_unlink_first(struct journal_head *jh)
> +static inline void __buffer_unlink(struct journal_head *jh)
>  {
>  	transaction_t *transaction = jh->b_cp_transaction;
>  
> @@ -40,23 +40,6 @@ static inline void __buffer_unlink_first(struct journal_head *jh)
>  	}
>  }
>  
> -/*
> - * Unlink a buffer from a transaction checkpoint(io) list.
> - *
> - * Called with j_list_lock held.
> - */
> -static inline void __buffer_unlink(struct journal_head *jh)
> -{
> -	transaction_t *transaction = jh->b_cp_transaction;
> -
> -	__buffer_unlink_first(jh);
> -	if (transaction->t_checkpoint_io_list == jh) {
> -		transaction->t_checkpoint_io_list = jh->b_cpnext;
> -		if (transaction->t_checkpoint_io_list == jh)
> -			transaction->t_checkpoint_io_list = NULL;
> -	}
> -}
> -
>  /*
>   * Check a checkpoint buffer could be release or not.
>   *
> @@ -499,15 +482,6 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
>  			break;
>  		if (need_resched() || spin_needbreak(&journal->j_list_lock))
>  			break;
> -		if (released)
> -			continue;
> -
> -		nr_freed += journal_shrink_one_cp_list(transaction->t_checkpoint_io_list,
> -						       nr_to_scan, &released);
> -		if (*nr_to_scan == 0)
> -			break;
> -		if (need_resched() || spin_needbreak(&journal->j_list_lock))
> -			break;
>  	} while (transaction != last_transaction);
>  
>  	if (transaction != last_transaction) {
> @@ -562,17 +536,6 @@ void __jbd2_journal_clean_checkpoint_list(journal_t *journal, bool destroy)
>  		 */
>  		if (need_resched())
>  			return;
> -		if (ret)
> -			continue;
> -		/*
> -		 * It is essential that we are as careful as in the case of
> -		 * t_checkpoint_list with removing the buffer from the list as
> -		 * we can possibly see not yet submitted buffers on io_list
> -		 */
> -		ret = journal_clean_one_cp_list(transaction->
> -				t_checkpoint_io_list, destroy);
> -		if (need_resched())
> -			return;
>  		/*
>  		 * Stop scanning if we couldn't free the transaction. This
>  		 * avoids pointless scanning of transactions which still
> @@ -657,7 +620,7 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
>  	jbd2_journal_put_journal_head(jh);
>  
>  	/* Is this transaction empty? */
> -	if (transaction->t_checkpoint_list || transaction->t_checkpoint_io_list)
> +	if (transaction->t_checkpoint_list)
>  		return 0;
>  
>  	/*
> @@ -749,7 +712,6 @@ void __jbd2_journal_drop_transaction(journal_t *journal, transaction_t *transact
>  	J_ASSERT(transaction->t_forget == NULL);
>  	J_ASSERT(transaction->t_shadow_list == NULL);
>  	J_ASSERT(transaction->t_checkpoint_list == NULL);
> -	J_ASSERT(transaction->t_checkpoint_io_list == NULL);
>  	J_ASSERT(atomic_read(&transaction->t_updates) == 0);
>  	J_ASSERT(journal->j_committing_transaction != transaction);
>  	J_ASSERT(journal->j_running_transaction != transaction);
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index b33155dd7001..1073259902a6 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -1141,8 +1141,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  	spin_lock(&journal->j_list_lock);
>  	commit_transaction->t_state = T_FINISHED;
>  	/* Check if the transaction can be dropped now that we are finished */
> -	if (commit_transaction->t_checkpoint_list == NULL &&
> -	    commit_transaction->t_checkpoint_io_list == NULL) {
> +	if (commit_transaction->t_checkpoint_list == NULL) {
>  		__jbd2_journal_drop_transaction(journal, commit_transaction);
>  		jbd2_journal_free_transaction(commit_transaction);
>  	}
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index f619bae1dcc5..91a2cf4bc575 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -622,12 +622,6 @@ struct transaction_s
>  	 */
>  	struct journal_head	*t_checkpoint_list;
>  
> -	/*
> -	 * Doubly-linked circular list of all buffers submitted for IO while
> -	 * checkpointing. [j_list_lock]
> -	 */
> -	struct journal_head	*t_checkpoint_io_list;
> -
>  	/*
>  	 * Doubly-linked circular list of metadata buffers being
>  	 * shadowed by log IO.  The IO buffers on the iobuf list and
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR