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@xxxxxxxxxxxxxxx
> Fixes: 9f356e5a4f12 ("jbd2: Account descriptor blocks into t_outstanding_credits")
> Reported-by: Alexander Coffin <alex.coffin@xxxxxxxxxxxxxxx>
> Link: https://lore.kernel.org/all/CA+hUFcuGs04JHZ_WzA1zGN57+ehL2qmHOt5a7RMpo+rv6Vyxtw@xxxxxxxxxxxxxx
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> ---
> 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;
> }
>
