On Wed 17-06-20 11:10:31, Lukas Czerner wrote:
> Callers of __jbd2_journal_unfile_buffer() and
> __jbd2_journal_refile_buffer() assume that the b_transaction is set. In
> fact if it's not, we can end up with journal_head refcounting errors
> leading to crash much later that might be very hard to track down. Add
> asserts to make sure that is the case.
>
> We also make sure that b_next_transaction is NULL in
> __jbd2_journal_unfile_buffer() since the callers expect that as well and
> we should not get into that stage in this state anyway, leading to
> problems later on if we do.
>
> Tested with fstests.
>
> Signed-off-by: Lukas Czerner <lczerner@xxxxxxxxxx>
Thanks! The patch looks good to me. You can add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Honza
> ---
> fs/jbd2/transaction.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index e91aad3637a2..e65e0aca2826 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -2026,6 +2026,9 @@ static void __jbd2_journal_temp_unlink_buffer(struct journal_head *jh)
> */
> static void __jbd2_journal_unfile_buffer(struct journal_head *jh)
> {
> + J_ASSERT_JH(jh, jh->b_transaction != NULL);
> + J_ASSERT_JH(jh, jh->b_next_transaction == NULL);
> +
> __jbd2_journal_temp_unlink_buffer(jh);
> jh->b_transaction = NULL;
> }
> @@ -2572,6 +2575,13 @@ bool __jbd2_journal_refile_buffer(struct journal_head *jh)
>
> was_dirty = test_clear_buffer_jbddirty(bh);
> __jbd2_journal_temp_unlink_buffer(jh);
> +
> + /*
> + * b_transaction must be set, otherwise the new b_transaction won't
> + * be holding jh reference
> + */
> + J_ASSERT_JH(jh, jh->b_transaction != NULL);
> +
> /*
> * We set b_transaction here because b_next_transaction will inherit
> * our jh reference and thus __jbd2_journal_file_buffer() must not
> --
> 2.21.3
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
