On Thu 27-05-21 21:56:35, Zhang Yi wrote:
> Although we merged c044f3d8360 ("jbd2: abort journal if free a async
> write error metadata buffer"), there is a race between
> jbd2_journal_try_to_free_buffers() and jbd2_journal_destroy(), so the
> jbd2_log_do_checkpoint() may still fail to detect the buffer write
> io error flag which may lead to filesystem inconsistency.
>
> jbd2_journal_try_to_free_buffers() ext4_put_super()
> jbd2_journal_destroy()
> __jbd2_journal_remove_checkpoint()
> detect buffer write error jbd2_log_do_checkpoint()
> jbd2_cleanup_journal_tail()
> <--- lead to inconsistency
> jbd2_journal_abort()
>
> Fix this issue by introducing a new atomic flag which only have one
> JBD2_CHECKPOINT_IO_ERROR bit now, and set it in
> __jbd2_journal_remove_checkpoint() when freeing a checkpoint buffer
> which has write_io_error flag. Then jbd2_journal_destroy() will detect
> this mark and abort the journal to prevent updating log tail.
>
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
Looks good. Just one spelling fix below. Feel free to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Honza
> @@ -575,6 +576,17 @@ int __jbd2_journal_remove_checkpoint(struct journal_head *jh)
> journal = transaction->t_journal;
>
> JBUFFER_TRACE(jh, "removing from transaction");
> +
> + /*
> + * If we have failed to write the buffer out to disk, the filesystem
> + * may become inconsistent. We cannot abort the journal here since
> + * we hold j_list_lock and we have to careful about races with
^^^ to be careful ...
> + * jbd2_journal_destroy(). So mark the writeback IO error in the
> + * journal here and we abort the journal later from a better context.
> + */
> + if (buffer_write_io_error(bh))
> + set_bit(JBD2_CHECKPOINT_IO_ERROR, &journal->j_atomic_flags);
> +
> __buffer_unlink(jh);
> jh->b_cp_transaction = NULL;
> jbd2_journal_put_journal_head(jh);
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
