Re: [PATCH v2 2/4] jbd2: Precompute number of transaction descriptor blocks

[Jan Kara <jack@suse.cz>]

On Tue 25-06-24 17:31:15, Kemeng Shi wrote:
> 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@vger.kernel.org
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/jbd2/journal.c     | 61 ++++++++++++++++++++++++++++++++-----------
> >  fs/jbd2/transaction.c | 24 +----------------
> >  include/linux/jbd2.h  |  7 +++++
> >  3 files changed, 54 insertions(+), 38 deletions(-)
> > 
> > +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...

So you are correct that the expression is overestimating the number of
descriptor blocks required, essentially because we don't need descriptor
blocks for descriptor blocks. But given tags_per_block is at least over 60,
in common configurations over 250, this imprecision is not really
substantial and I prefer a simple to argue about upper estimate...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR