| Message ID | 1548419456-4331-2-git-send-email-yi.zhang@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: fix a data corruption problem | expand |
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
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index f07f006..0c0cbda 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -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. */ - if (jh->b_next_transaction) { - J_ASSERT(jh->b_next_transaction == transaction); + set_buffer_freed(bh); + + if (!jh->b_next_transaction) { spin_lock(&journal->j_list_lock); - jh->b_next_transaction = NULL; + jh->b_next_transaction = transaction; spin_unlock(&journal->j_list_lock); + } else { + J_ASSERT(jh->b_next_transaction == transaction); /* * only drop a reference if this transaction modified
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 --- fs/jbd2/transaction.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)