Re: [PATCH v3 1/4] jbd2: make sure dirty flag is cleared while revorking a buffer which belongs to older transaction

[Jan Kara <jack@suse.cz>]

On Fri 25-01-19 20:30:53, zhangyi (F) wrote:
> Now, we capture a data corruption problem on ext4 while we're truncating
> an extent index block. Imaging that if we are revoking a buffer which
> has been journaled by the committing transaction, the buffer's jbddirty
> flag will not be cleared in jbd2_journal_forget(), so the commit code
> will set the buffer dirty flag again after refile the buffer.
> 
> fsx                               kjournald2
>                                   jbd2_journal_commit_transaction
> jbd2_journal_revoke                commit phase 1~5...
>  jbd2_journal_forget
>    belongs to older transaction    commit phase 6
>    jbddirty not clear               __jbd2_journal_refile_buffer
>                                      __jbd2_journal_unfile_buffer
>                                       test_clear_buffer_jbddirty
>                                        mark_buffer_dirty
> 
> Finally, if the freed extent index block was allocated again as data
> block by some other files, it may corrupt the file data after writing
> cached pages later, such as during unmount time. (In general,
> clean_bdev_aliases() related helpers should be invoked after
> re-allocation to prevent the above corruption, but unfortunately we
> missed it when zeroout the head of extra extent blocks in
> ext4_ext_handle_unwritten_extents()).
> 
> This patch mark buffer as freed and set j_next_transaction to the new
> transaction when it already belongs to the committing transaction in
> jbd2_journal_forget(), so that commit code knows it should clear dirty
> bits when it is done with the buffer.
> 
> This problem can be reproduced by xfstests generic/455 easily with
> seeds (3246 3247 3248 3249).
> 
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> Cc: stable@vger.kernel.org

The patch looks good to me. You can add:

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

Just one comment below to make the comment more readable:

> @@ -1609,14 +1609,21 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh)
>  		/* However, if the buffer is still owned by a prior
>  		 * (committing) transaction, we can't drop it yet... */
>  		JBUFFER_TRACE(jh, "belongs to older transaction");
> -		/* ... but we CAN drop it from the new transaction if we
> -		 * have also modified it since the original commit. */
> +		/* ... but we CAN drop it from the new transaction through
> +		 * marking the buffer as freed and set j_next_transaction to
> +		 * the new transaction, so that not only the commit code
> +		 * knows it should clear dirty bits when it is done with the
> +		 * buffer, but also we can avoid this buffer be checkpointed
> +		 * without writing out before the new transaction complete. */

.... but also the buffer can be checkpointed only after the new transaction
commits.

									Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR