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

[Zhang Yi <yi.zhang@huawei.com>]

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.
> 
Hello Jan!

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().

Thanks,
Yi.

> Fix the problem by properly accounting for transaction space reserved
> for descriptor blocks when verifying requested transaction handle size.
> 
> CC: stable@vger.kernel.org
> Fixes: 9f356e5a4f12 ("jbd2: Account descriptor blocks into t_outstanding_credits")
> Reported-by: Alexander Coffin <alex.coffin@maticrobots.com>
> Link: https://lore.kernel.org/all/CA+hUFcuGs04JHZ_WzA1zGN57+ehL2qmHOt5a7RMpo+rv6Vyxtw@mail.gmail.com
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/jbd2/transaction.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index a095f1a3114b..66513c18ca29 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -191,6 +191,13 @@ static void sub_reserved_credits(journal_t *journal, int blocks)
>  	wake_up(&journal->j_wait_reserved);
>  }
>  
> +/* Maximum number of blocks for user transaction payload */
> +static int jbd2_max_user_trans_buffers(journal_t *journal)
> +{
> +	return journal->j_max_transaction_buffers -
> +				journal->j_transaction_overhead_buffers;
> +}
> +
>  /*
>   * Wait until we can add credits for handle to the running transaction.  Called
>   * with j_state_lock held for reading. Returns 0 if handle joined the running
> @@ -240,12 +247,12 @@ __must_hold(&journal->j_state_lock)
>  		 * big to fit this handle? Wait until reserved credits are freed.
>  		 */
>  		if (atomic_read(&journal->j_reserved_credits) + total >
> -		    journal->j_max_transaction_buffers) {
> +		    jbd2_max_user_trans_buffers(journal)) {
>  			read_unlock(&journal->j_state_lock);
>  			jbd2_might_wait_for_commit(journal);
>  			wait_event(journal->j_wait_reserved,
>  				   atomic_read(&journal->j_reserved_credits) + total <=
> -				   journal->j_max_transaction_buffers);
> +				   jbd2_max_user_trans_buffers(journal));
>  			__acquire(&journal->j_state_lock); /* fake out sparse */
>  			return 1;
>  		}
> @@ -285,14 +292,14 @@ __must_hold(&journal->j_state_lock)
>  
>  	needed = atomic_add_return(rsv_blocks, &journal->j_reserved_credits);
>  	/* We allow at most half of a transaction to be reserved */
> -	if (needed > journal->j_max_transaction_buffers / 2) {
> +	if (needed > jbd2_max_user_trans_buffers(journal) / 2) {
>  		sub_reserved_credits(journal, rsv_blocks);
>  		atomic_sub(total, &t->t_outstanding_credits);
>  		read_unlock(&journal->j_state_lock);
>  		jbd2_might_wait_for_commit(journal);
>  		wait_event(journal->j_wait_reserved,
>  			 atomic_read(&journal->j_reserved_credits) + rsv_blocks
> -			 <= journal->j_max_transaction_buffers / 2);
> +			 <= jbd2_max_user_trans_buffers(journal) / 2);
>  		__acquire(&journal->j_state_lock); /* fake out sparse */
>  		return 1;
>  	}
> @@ -322,12 +329,12 @@ static int start_this_handle(journal_t *journal, handle_t *handle,
>  	 * size and limit the number of total credits to not exceed maximum
>  	 * transaction size per operation.
>  	 */
> -	if ((rsv_blocks > journal->j_max_transaction_buffers / 2) ||
> -	    (rsv_blocks + blocks > journal->j_max_transaction_buffers)) {
> +	if (rsv_blocks > jbd2_max_user_trans_buffers(journal) / 2 ||
> +	    rsv_blocks + blocks > jbd2_max_user_trans_buffers(journal)) {
>  		printk(KERN_ERR "JBD2: %s wants too many credits "
>  		       "credits:%d rsv_credits:%d max:%d\n",
>  		       current->comm, blocks, rsv_blocks,
> -		       journal->j_max_transaction_buffers);
> +		       jbd2_max_user_trans_buffers(journal));
>  		WARN_ON(1);
>  		return -ENOSPC;
>  	}
>