Re: [PATCH v2 3/4] jbd2: Avoid infinite transaction commit loop

[Jan Kara <jack@suse.cz>]

On Wed 26-06-24 15:38:42, Zhang Yi wrote:
> On 2024/6/25 1:01, Jan Kara wrote:
> > Commit 9f356e5a4f12 ("jbd2: Account descriptor blocks into
> > t_outstanding_credits") started to account descriptor blocks into
> > transactions outstanding credits. However it didn't appropriately
> > decrease the maximum amount of credits available to userspace. Thus if
> > the filesystem requests a transaction smaller than
> > j_max_transaction_buffers but large enough that when descriptor blocks
> > are added the size exceeds j_max_transaction_buffers, we confuse
> > add_transaction_credits() into thinking previous handles have grown the
> > transaction too much and enter infinite journal commit loop in
> > start_this_handle() -> add_transaction_credits() trying to create
> > transaction with enough credits available.
> 
> I understand that the incorrect max transaction limit in
> start_this_handle() could lead to infinite loop in
> start_this_handle()-> add_transaction_credits() with large enough
> userspace credits (from j_max_transaction_buffers - overheads to
> j_max_transaction_buffers), but I don't get how could it lead to ran
> out of space in the journal commit traction? IIUC, below codes in
> add_transaction_credits() could make sure that we have enough space
> when committing traction:
> 
> static int add_transaction_credits()
> {
> ...
> 	if (jbd2_log_space_left(journal) < journal->j_max_transaction_buffers) {
> 		...
> 		return 1;
> 		...
> 	}
> ...
> }
> 
> I can't open and download the image Alexander gave, so I can't get to
> the bottom of this issue, please let me know what happened with
> jbd2_journal_next_log_block().

Sure. So what was exactly happening is a loop like this:

start_this_handle()
  blocks = 252 (handle->h_total_credits)
  - starts a new transaction
    - t_outstanding_credits set to 6 to account for the commit block and
      descriptor blocks
  add_transaction_credits(journal, 252, 0)
     needed = atomic_add_return(252, &t->t_outstanding_credits);
     if (needed > journal->j_max_transaction_buffers) {
	/* Yes, this is exceeded due to descriptor blocks being in
	 * t_outstanding_credits */
        ...
        wait_transaction_locked(journal);
	  - this commits an empty transaction - contains only the commit
	    block
        return 1
  goto repeat

So we commit single block transactions in a loop until we exhaust all the
journal space. The condition in add_transaction_credits() whether there's
enough space in the journal is never reached so we don't ever push the
journal tail to make space in the journal.
    
								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR