Re: [PATCH] jbd2: validate transaction state before dropping from journal

[Jan Kara]

On Mon 13-04-26 11:08:24, Milos Nikic wrote:
> Currently, __jbd2_journal_drop_transaction() unlinks the transaction
> from the journal's checkpoint lists and only then proceeds to validate the
> transaction's internal state using a series of J_ASSERTs.
> 
> There is no need to 'mutate before validate'. If we are going to halt the
> system that makes manipulating corrupted pointers in memory irrelevant.
> 
> Move the state validation block above the pointer manipulation. This
> ensures the transaction is entirely valid before modifying the journal's
> internal lists, modernizing the function's logic and paving the way
> for future graceful degradation of these assertions.

Either you have a poetry gift or you should tell your AI agent to keep the
tone more to the point :). Anyway I think this is really just a pointless
churn as it doesn't really matter whether we crash the kernel one way or
another...

								Honza

> 
> Signed-off-by: Milos Nikic <nikic.milos@xxxxxxxxx>
> ---
>  fs/jbd2/checkpoint.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index 1508e2f54462..c82b6bedd27b 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -703,6 +703,15 @@ void __jbd2_journal_drop_transaction(journal_t *journal, transaction_t *transact
>  {
>  	assert_spin_locked(&journal->j_list_lock);
>  
> +	J_ASSERT(transaction->t_state == T_FINISHED);
> +	J_ASSERT(transaction->t_buffers == NULL);
> +	J_ASSERT(transaction->t_forget == NULL);
> +	J_ASSERT(transaction->t_shadow_list == NULL);
> +	J_ASSERT(transaction->t_checkpoint_list == NULL);
> +	J_ASSERT(atomic_read(&transaction->t_updates) == 0);
> +	J_ASSERT(journal->j_committing_transaction != transaction);
> +	J_ASSERT(journal->j_running_transaction != transaction);
> +
>  	journal->j_shrink_transaction = NULL;
>  	if (transaction->t_cpnext) {
>  		transaction->t_cpnext->t_cpprev = transaction->t_cpprev;
> @@ -714,15 +723,6 @@ void __jbd2_journal_drop_transaction(journal_t *journal, transaction_t *transact
>  			journal->j_checkpoint_transactions = NULL;
>  	}
>  
> -	J_ASSERT(transaction->t_state == T_FINISHED);
> -	J_ASSERT(transaction->t_buffers == NULL);
> -	J_ASSERT(transaction->t_forget == NULL);
> -	J_ASSERT(transaction->t_shadow_list == NULL);
> -	J_ASSERT(transaction->t_checkpoint_list == NULL);
> -	J_ASSERT(atomic_read(&transaction->t_updates) == 0);
> -	J_ASSERT(journal->j_committing_transaction != transaction);
> -	J_ASSERT(journal->j_running_transaction != transaction);
> -
>  	trace_jbd2_drop_transaction(journal, transaction);
>  
>  	jbd2_debug(1, "Dropping transaction %d, all done\n", transaction->t_tid);
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR