From: Zhihao Cheng <[email protected]>
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 <[email protected]>
Signed-off-by: Zhang Yi <[email protected]>
---
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
On Tue 06-06-23 14:14:45, Zhang Yi wrote:
> From: Zhihao Cheng <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Zhang Yi <[email protected]>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR