Re: [RFC] Bug in journal_commit_transaction?

[Jan Kara <jack@suse.cz>]

  Hello,

  sorry, I was probably to terse in my previous mail. I'll try to
describe more details now.

> >   I think I have found a possible cause of assertion failure in
> > journal_commit_buffer() on line 793 (b_next_transaction == NULL) (which
> 
> you mean journal_commit_transaction().
  Yes.

> > was reported about two times as I remember). And I think the problem is
> > just a few lines below in cond_resched_lock(&journal->j_list_lock);
> >   The problem is following: we are processing t_forget list of a
> > committing transaction. On this forget list are among other buffers also
> > bitmaps and buffers that are freed by this transaction. Now if bitmap
> > buffer is processed first, we switch to new bitmap (hence the buffer
> > freed by this transaction is again available for allocation). If we
> > reschedule on that cond_resched_lock() and in the meantime someone
> > allocates the buffer, we later fail with that assertion failure.
> 
> I assume you're referring to the cond_resched_lock() at line 799.
> 
> I don't really follow your description here (it seems to be missing bits),
> but that loop drops that lock in other places - why wouldn't those places
> also be vulnerable?
  Yes, I meant that cond_resched_lock(). But you are right that the
problem is not in cond_resched_lock() itself. It is in the pure fact
that we can process bitmap buffer in t_forget list before all the
buffers transaction has freed. Explanation is below.
  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. But unlocking j_list_lock when
processing t_forget list in journal_commit_transaction() can actually
break that assumption in some cases:
  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. 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.

								Honza

-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs