在 2023/6/1 17:41, Jan Kara 写道:
Hi, Jan
On Wed 31-05-23 19:50:59, Zhang Yi wrote:
From: Zhihao Cheng <chengzhihao1@xxxxxxxxxx>
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 checking buffer dirty firstly and then checking buffer
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@xxxxxxxxxx>
Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
OK, the analysis is correct but I'm afraid the fix won't be that easy. The
reordering of tests you did below doesn't really help because CPU or the
compiler are free to order the loads (and stores) in whatever way they
wish. You'd have to use memory barriers when reading and modifying bh flags
(although the modification side is implicitely handled by the bitlock
code) to make this work reliably. But that is IMHO too subtle for this
code.
Do you mean there might be a sequence like following:
jbd2_log_do_checkpoint
if (buffer_dirty(bh))
else if (buffer_locked(bh))
else
__jbd2_journal_remove_checkpoint(jh)
CPU re-arranges the order of getting buffer state.
reg_1 = buffer_locked(bh) // false
lock_buffer(bh)
clear_buffer(bh)
reg_2 = buffer_dirty(bh) // false
Then, jbd2_log_do_checkpoint() could become:
if (reg_2)
else if (reg_1)
else
__jbd2_journal_remove_checkpoint(jh) // enter !
Am I understanding right?
What we should be doing to avoid these races is to lock the bh. So
something like:
if (jh->b_transaction != NULL) {
do stuff
}
if (!trylock_buffer(bh)) {
buffer_locked() branch
}
... Now we have the buffer locked and can safely check for dirtyness
And we need to do a similar treatment for journal_clean_one_cp_list() and
journal_shrink_one_cp_list().
BTW, I think we could merge journal_clean_one_cp_list() and
journal_shrink_one_cp_list() into a single common function. I think we can
drop the nr_to_scan argument and just always cleanup the whole checkpoint
list and return the number of freed buffers. That way we have one less
function to deal with checkpoint list cleaning.
Thinking about it some more maybe we can have a function like:
int jbd2_try_remove_checkpoint(struct journal_head *jh)
{
struct buffer_head *bh = jh2bh(jh);
if (!trylock_buffer(bh) || buffer_dirty(bh))
return -EBUSY;
/*
* Buffer is clean and the IO has finished (we hold the buffer lock) so
* the checkpoint is done. We can safely remove the buffer from this
* transaction.
*/
unlock_buffer(bh);
return __jbd2_journal_remove_checkpoint(jh);
}
and that can be used with a bit of care in the checkpointing functions as
well as in jbd2_journal_forget(), __journal_try_to_free_buffer(),
journal_unmap_buffer().
Honza
