Re: [PATCH v2 4/6] jbd2: Fix wrongly judgement for buffer head removing while doing checkpoint

[Jan Kara <jack@suse.cz>]

On Tue 06-06-23 14:14:45, Zhang Yi wrote:
> From: Zhihao Cheng <chengzhihao1@huawei.com>
> 
> Following process,
> 
> jbd2_journal_commit_transaction
> // there are several dirty buffer heads in transaction->t_checkpoint_list
>           P1                   wb_workfn
> jbd2_log_do_checkpoint
>  if (buffer_locked(bh)) // false
>                             __block_write_full_page
>                              trylock_buffer(bh)
>                              test_clear_buffer_dirty(bh)
>  if (!buffer_dirty(bh))
>   __jbd2_journal_remove_checkpoint(jh)
>    if (buffer_write_io_error(bh)) // false
>                              >> bh IO error occurs <<
>  jbd2_cleanup_journal_tail
>   __jbd2_update_log_tail
>    jbd2_write_superblock
>    // The bh won't be replayed in next mount.
> , which could corrupt the ext4 image, fetch a reproducer in [Link].
> 
> Since writeback process clears buffer dirty after locking buffer head,
> we can fix it by try locking buffer and check dirtiness while buffer is
> locked, the buffer head can be removed if it is neither dirty nor locked.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217490
> Fixes: 470decc613ab ("[PATCH] jbd2: initial copy of files from jbd")
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> 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 | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index 3eb5b01a7e84..32f86bfbca69 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -204,20 +204,6 @@ int jbd2_log_do_checkpoint(journal_t *journal)
>  		jh = transaction->t_checkpoint_list;
>  		bh = jh2bh(jh);
>  
> -		/*
> -		 * The buffer may be writing back, or flushing out in the
> -		 * last couple of cycles, or re-adding into a new transaction,
> -		 * need to check it again until it's unlocked.
> -		 */
> -		if (buffer_locked(bh)) {
> -			get_bh(bh);
> -			spin_unlock(&journal->j_list_lock);
> -			wait_on_buffer(bh);
> -			/* the journal_head may have gone by now */
> -			BUFFER_TRACE(bh, "brelse");
> -			__brelse(bh);
> -			goto retry;
> -		}
>  		if (jh->b_transaction != NULL) {
>  			transaction_t *t = jh->b_transaction;
>  			tid_t tid = t->t_tid;
> @@ -252,7 +238,22 @@ int jbd2_log_do_checkpoint(journal_t *journal)
>  			spin_lock(&journal->j_list_lock);
>  			goto restart;
>  		}
> -		if (!buffer_dirty(bh)) {
> +		if (!trylock_buffer(bh)) {
> +			/*
> +			 * The buffer is locked, it may be writing back, or
> +			 * flushing out in the last couple of cycles, or
> +			 * re-adding into a new transaction, need to check
> +			 * it again until it's unlocked.
> +			 */
> +			get_bh(bh);
> +			spin_unlock(&journal->j_list_lock);
> +			wait_on_buffer(bh);
> +			/* the journal_head may have gone by now */
> +			BUFFER_TRACE(bh, "brelse");
> +			__brelse(bh);
> +			goto retry;
> +		} else if (!buffer_dirty(bh)) {
> +			unlock_buffer(bh);
>  			BUFFER_TRACE(bh, "remove from checkpoint");
>  			/*
>  			 * If the transaction was released or the checkpoint
> @@ -262,6 +263,7 @@ int jbd2_log_do_checkpoint(journal_t *journal)
>  			    !transaction->t_checkpoint_list)
>  				goto out;
>  		} else {
> +			unlock_buffer(bh);
>  			/*
>  			 * We are about to write the buffer, it could be
>  			 * raced by some other transaction shrink or buffer
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR