Re: [PATCH] jbd: fix assertion failure in journal_next_log_block

[Mingming Cao <cmm@us.ibm.com>]

On Tue, 2008-02-05 at 13:59 -0500, Josef Bacik wrote:
> On Tuesday 05 February 2008 12:27:31 pm Jan Kara wrote:
> >   Hello,
> >
> >   Sorry for replying a bit late but I'm currently falling behind in
> > maling-list reading...
> >
> > > The way jbd tries to determine if there is enough space left on the
> > > journal in order to start a new transaction is looking at the space left
> > > in the journal and the space needed for the committing transaction, which
> > > is 1/4 of the journal + the number of t_outstanding_credits for that
> > > transaction.  In this case its assumed that t_outstanding_credits
> > > accurately represents the number of credits
> >
> >   Yes.
> >
> > > waiting to be written to the journal, but this sometimes isn't the case. 
> > > The transaction has two counters for buffers, t_outstanding_credits which
> > > is used in conjunction with handles that are added to the transaction,
> > > and t_nr_buffers which is only incremented/decremented when buffers are
> > > added/removed from the transaction and are actually destined to be
> > > journaled.  Normally these two
> >
> >   t_nr_buffers actually represents number of buffers on BJ_Metadata list
> > and nobody uses it (except for the assertion in
> > __journal_temp_unlink_buffer()). t_outstanding_credits is supposed to be
> > *the* counter making sure we don't write more than we have credits for.
> >
> > > counters are the same, however there are cases where the committing
> > > transaction can have buffers moved to the next running transaction, for
> > > example any buffers on the committing transactions t_reserved list would
> > > be moved to the next (running) transaction, and if it had been dirtied in
> > > the process it would immediately make it onto the t_updates list, which
> > > would increment t_nr_buffers
> >
> >   You probably mean t_buffers list here...
> >
> > > but not t_outstanding_credits.  So you get into this situation where
> >
> >   But which moving and dirtying do you mean? The caller which dirties
> > the buffer must make sure that he has acquired enough credits for the
> > transaction where the buffer ends up... So if there were not enough
> > buffers in the running transaction where we refiled the buffer it is a
> > bug in the caller which dirties the buffer.
> >
> 
> You know now that you say that I feel like an idiot, you are right the only way 
> for something to actually end up on that list was if somebody dirtied it and if 
> they did it would have had to been accounted for at some point on the running 
> transaction.
> 
> > > t_nr_buffers (the actual number of buffers that are on the transaction)
> > > is greater than the number of buffers accounted for via
> > > t_outstanding_credits. This presents a problem since as we loop through
> > > writting buffers to the journal, we decrement t_outstanding_credits, and
> > > if t_nr_buffers is more than t_outstanding_credits then we end up with a
> > > negative number for
> > > t_outstanding_credits, which means we start saying we need less than 1/4
> > > of the journal for our committing transaction and allow more transactions
> > > than we can handle to start, and then bam we fail because
> > > journal_next_log_block doesn't have enough free blocks in order to handle
> > > the request.  This has been tested and fixes the issue (which could not
> > > be reproduced by me but several other people could get it to reproduce
> > > using postmark), and although I couldn't reproduce the assertion, I could
> > > very easily reproduce the situation where t_outstanding_credits was <
> > > than t_nr_buffers.
> >
> >   I suppose you see the assertion J_ASSERT(journal->j_free > 1); to
> > fail, right? I don't see how your patch could help avoid that assertion.
> > You've just removed accounting of t_outstanding_credits which has no
> > impact on the real number of free blocks in the journal stored in
> > j_free. Anyway, if you can reproduce t_outstanding_credits <
> > t_nr_buffers, then there's something fishy. Are you able to reproduce it
> > also with a current kernel?
> >   Thanks for looking into the problem :)
> >
> 
> Well my patch helped avoid the assertion because t_outstanding_credits was going 
> negative therefore we were letting transactions start when we shouldn't be, and 
> eventually we would end up with too much of the journal in use and we'd assert.  
> Course I can't reproduce where t_outstanding_credits < t_nr_buffers upstream 
> (again I feel like an idiot, should have tested that first).  Thanks for 
> looking at this Jan.
> 
> Mingming, would you mind pulling this patch out of the patch queue please since 
> its wrong?  Thanks much,
> 

Sure, done!

Mingming