On Tue 06-06-23 14:14:45, 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 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@xxxxxxxxxx> > Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx> Looks good. Feel free to add: Reviewed-by: Jan Kara <jack@xxxxxxx> 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@xxxxxxxx> SUSE Labs, CR
