Re: [RFC] Bug in journal_commit_transaction?

["Stephen C. Tweedie" <sct@redhat.com>]

Hi,

On Sat, 2006-04-15 at 23:06 +0200, Jan Kara wrote:

>   When we are freeing some blocks we do journal_get_undo_access() on the
> bitmap buffer. That copies current state of the bitmap into
> b_committed_data. When we allocate any new blocks we check that the
> block is free in both b_committed_data (old state) and in b_data (new
> state). That makes sure that we do not reallocate any freed block before
> transaction that frees it is committed. 

Right; it's a critical data integrity guarantee over crashes.

> But unlocking j_list_lock when
> processing t_forget list in journal_commit_transaction() can actually
> break that assumption in some cases:

Yes and no --- the main goal of the b_committed_data copy is still
preserved in this case, because by this point we've written the commit
block so it's actually OK to reallocate the buffers freed in that
transaction now.  As far as recovery is concerned, we *have* committed
by this point.  It's just that we haven't finished the in-memory cleanup
of buffers belonging to the committed transaction.

So we never break the primary assumption that the b_committed_data copy
is there to protect.  But we might well confuse other parts of the
internal error-checking logic.

>   If we commit the transaction we write the bitmap buffer into the log
> and then file it into BJ_Forget list. We also write the freed data
> buffer (e.g. zeroed indirect block) and file it into BJ_Forget list.
> Then we start processing BJ_Forget list. First we find the bitmap
> buffer. In line 748 we free the b_committed_data stored by
> journal_get_undo_access(). From now on the allocation code in
> ext3_test_allocatable() will no longer find b_committed_data and hence
> it will assume the block is freed. *BUT* we still have the freed buffer
> in t_forget list. 

Right.......

> If it happens that we reallocate the block before
> we process the buffer in t_forget list, it will have b_next_transaction
> set and we fail at the assertion failure in line 793.
>   I hope it is clearer now.

Much clearer, thanks: it wasn't clear in your first description that you
were talking about an interaction between *two* different buffers on the
forget list.  This sounds like a perfectly plausible explanation.

--Stephen