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@xxxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx The patch looks good to me. You can add: Reviewed-by: Jan Kara <jack@xxxxxxx> 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@xxxxxxxx> SUSE Labs, CR
