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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <17f3006d-4601-9f34-0ab0-fec6e9f55ffa@huaweicloud.com>
Date: Tue, 25 Jun 2024 20:02:54 +0800
From: Kemeng Shi <shikemeng@...weicloud.com>
To: Jan Kara <jack@...e.cz>
Cc: Ted Tso <tytso@....edu>, linux-ext4@...r.kernel.org,
 Alexander Coffin <alex.coffin@...icrobots.com>, stable@...r.kernel.org
Subject: Re: [PATCH v2 2/4] jbd2: Precompute number of transaction descriptor
 blocks



on 6/25/2024 7:07 PM, Jan Kara wrote:
> 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@...r.kernel.org
>>> Signed-off-by: Jan Kara <jack@...e.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...
Right, I agree with this. Thanks for explanation.
> 
> 								Honza
>