Hello Jan,
on 6/25/2024 1:01 AM, Jan Kara wrote:
> Instead of computing the number of descriptor blocks a transaction can
> have each time we need it (which is currently when starting each
> transaction but will become more frequent later) precompute the number
> once during journal initialization together with maximum transaction
> size. We perform the precomputation whenever journal feature set is
> updated similarly as for computation of
> journal->j_revoke_records_per_block.
>
> CC: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> ---
> fs/jbd2/journal.c | 61 ++++++++++++++++++++++++++++++++-----------
> fs/jbd2/transaction.c | 24 +----------------
> include/linux/jbd2.h | 7 +++++
> 3 files changed, 54 insertions(+), 38 deletions(-)
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 1bb73750d307..ae5b544ed0cc 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1451,6 +1451,48 @@ static int journal_revoke_records_per_block(journal_t *journal)
> return space / record_size;
> }
>
> +static int jbd2_journal_get_max_txn_bufs(journal_t *journal)
> +{
> + return (journal->j_total_len - journal->j_fc_wbufsize) / 4;
> +}
> +
> +/*
> + * Base amount of descriptor blocks we reserve for each transaction.
> + */
> +static int jbd2_descriptor_blocks_per_trans(journal_t *journal)
> +{
> + int tag_space = journal->j_blocksize - sizeof(journal_header_t);
> + int tags_per_block;
> +
> + /* Subtract UUID */
> + tag_space -= 16;
> + if (jbd2_journal_has_csum_v2or3(journal))
> + tag_space -= sizeof(struct jbd2_journal_block_tail);
> + /* Commit code leaves a slack space of 16 bytes at the end of block */
> + tags_per_block = (tag_space - 16) / journal_tag_bytes(journal);
> + /*
> + * Revoke descriptors are accounted separately so we need to reserve
> + * space for commit block and normal transaction descriptor blocks.
> + */
> + return 1 + DIV_ROUND_UP(jbd2_journal_get_max_txn_bufs(journal),
> + tags_per_block);
> +}
The change looks good to me. I wonder if the original calculation of
number of JBD2_DESCRIPTOR_BLOCK blocks is correct.
In my opinion, it should be:
DIV_ROUND_UP(jbd2_journal_get_max_txn_bufs(journal), tags_per_block *+ 1*)
Assume max_txn_bufs is 6, tags_per_block is 1, then we have one tag block
after each JBD2_DESCRIPTOR_BLOCK block. Then we could get 3
JBD2_DESCRIPTOR_BLOCK block at most rather than 6.
Please let me konw if I miss something, this confused me for sometime...
Thanks.
> +
> +/*
> + * Initialize number of blocks each transaction reserves for its bookkeeping
> + * and maximum number of blocks a transaction can use. This needs to be called
> + * after the journal size and the fastcommit area size are initialized.
> + */
> +static void jbd2_journal_init_transaction_limits(journal_t *journal)
> +{
> + journal->j_revoke_records_per_block =
> + journal_revoke_records_per_block(journal);
> + journal->j_transaction_overhead_buffers =
> + jbd2_descriptor_blocks_per_trans(journal);
> + journal->j_max_transaction_buffers =
> + jbd2_journal_get_max_txn_bufs(journal);
> +}
> +
> /*
> * Load the on-disk journal superblock and read the key fields into the
> * journal_t.
> @@ -1492,8 +1534,8 @@ static int journal_load_superblock(journal_t *journal)
> if (jbd2_journal_has_csum_v2or3(journal))
> journal->j_csum_seed = jbd2_chksum(journal, ~0, sb->s_uuid,
> sizeof(sb->s_uuid));
> - journal->j_revoke_records_per_block =
> - journal_revoke_records_per_block(journal);
> + /* After journal features are set, we can compute transaction limits */
> + jbd2_journal_init_transaction_limits(journal);
>
> if (jbd2_has_feature_fast_commit(journal)) {
> journal->j_fc_last = be32_to_cpu(sb->s_maxlen);
> @@ -1698,11 +1740,6 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
> return journal;
> }
>
> -static int jbd2_journal_get_max_txn_bufs(journal_t *journal)
> -{
> - return (journal->j_total_len - journal->j_fc_wbufsize) / 4;
> -}
> -
> /*
> * Given a journal_t structure, initialise the various fields for
> * startup of a new journaling session. We use this both when creating
> @@ -1748,8 +1785,6 @@ static int journal_reset(journal_t *journal)
> journal->j_commit_sequence = journal->j_transaction_sequence - 1;
> journal->j_commit_request = journal->j_commit_sequence;
>
> - journal->j_max_transaction_buffers = jbd2_journal_get_max_txn_bufs(journal);
> -
> /*
> * Now that journal recovery is done, turn fast commits off here. This
> * way, if fast commit was enabled before the crash but if now FS has
> @@ -2290,8 +2325,6 @@ jbd2_journal_initialize_fast_commit(journal_t *journal)
> journal->j_fc_first = journal->j_last + 1;
> journal->j_fc_off = 0;
> journal->j_free = journal->j_last - journal->j_first;
> - journal->j_max_transaction_buffers =
> - jbd2_journal_get_max_txn_bufs(journal);
>
> return 0;
> }
> @@ -2379,8 +2412,7 @@ int jbd2_journal_set_features(journal_t *journal, unsigned long compat,
> sb->s_feature_ro_compat |= cpu_to_be32(ro);
> sb->s_feature_incompat |= cpu_to_be32(incompat);
> unlock_buffer(journal->j_sb_buffer);
> - journal->j_revoke_records_per_block =
> - journal_revoke_records_per_block(journal);
> + jbd2_journal_init_transaction_limits(journal);
>
> return 1;
> #undef COMPAT_FEATURE_ON
> @@ -2411,8 +2443,7 @@ void jbd2_journal_clear_features(journal_t *journal, unsigned long compat,
> sb->s_feature_compat &= ~cpu_to_be32(compat);
> sb->s_feature_ro_compat &= ~cpu_to_be32(ro);
> sb->s_feature_incompat &= ~cpu_to_be32(incompat);
> - journal->j_revoke_records_per_block =
> - journal_revoke_records_per_block(journal);
> + jbd2_journal_init_transaction_limits(journal);
> }
> EXPORT_SYMBOL(jbd2_journal_clear_features);
>
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index cb0b8d6fc0c6..a095f1a3114b 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -62,28 +62,6 @@ void jbd2_journal_free_transaction(transaction_t *transaction)
> kmem_cache_free(transaction_cache, transaction);
> }
>
> -/*
> - * Base amount of descriptor blocks we reserve for each transaction.
> - */
> -static int jbd2_descriptor_blocks_per_trans(journal_t *journal)
> -{
> - int tag_space = journal->j_blocksize - sizeof(journal_header_t);
> - int tags_per_block;
> -
> - /* Subtract UUID */
> - tag_space -= 16;
> - if (jbd2_journal_has_csum_v2or3(journal))
> - tag_space -= sizeof(struct jbd2_journal_block_tail);
> - /* Commit code leaves a slack space of 16 bytes at the end of block */
> - tags_per_block = (tag_space - 16) / journal_tag_bytes(journal);
> - /*
> - * Revoke descriptors are accounted separately so we need to reserve
> - * space for commit block and normal transaction descriptor blocks.
> - */
> - return 1 + DIV_ROUND_UP(journal->j_max_transaction_buffers,
> - tags_per_block);
> -}
> -
> /*
> * jbd2_get_transaction: obtain a new transaction_t object.
> *
> @@ -109,7 +87,7 @@ static void jbd2_get_transaction(journal_t *journal,
> transaction->t_expires = jiffies + journal->j_commit_interval;
> atomic_set(&transaction->t_updates, 0);
> atomic_set(&transaction->t_outstanding_credits,
> - jbd2_descriptor_blocks_per_trans(journal) +
> + journal->j_transaction_overhead_buffers +
> atomic_read(&journal->j_reserved_credits));
> atomic_set(&transaction->t_outstanding_revokes, 0);
> atomic_set(&transaction->t_handle_count, 0);
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index f91b930abe20..b900c642210c 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1085,6 +1085,13 @@ struct journal_s
> */
> int j_revoke_records_per_block;
>
> + /**
> + * @j_transaction_overhead:
> + *
> + * Number of blocks each transaction needs for its own bookkeeping
> + */
> + int j_transaction_overhead_buffers;
> +
> /**
> * @j_commit_interval:
> *
>
