linux-ext4 - Re: [PATCH v2 5/8] ext4: correct the journal credits calculations of allocating 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: <25d93183-35ac-4b58-9bd2-2c9179735601@huaweicloud.com>
Date: Tue, 20 May 2025 20:53:26 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: Jan Kara <jack@...e.cz>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, willy@...radead.org, tytso@....edu,
 adilger.kernel@...ger.ca, yi.zhang@...wei.com, libaokun1@...wei.com,
 yukuai3@...wei.com, yangerkun@...wei.com
Subject: Re: [PATCH v2 5/8] ext4: correct the journal credits calculations of
 allocating blocks

On 2025/5/20 4:24, Jan Kara wrote:
> On Mon 12-05-25 14:33:16, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@...wei.com>
>>
>> The journal credits calculation in ext4_ext_index_trans_blocks() is
>> currently inadequate. It only multiplies the depth of the extents tree
>> and doesn't account for the blocks that may be required for adding the
>> leaf extents themselves.
>>
>> After enabling large folios, we can easily run out of handle credits,
>> triggering a warning in jbd2_journal_dirty_metadata() on filesystems
>> with a 1KB block size. This occurs because we may need more extents when
>> iterating through each large folio in
>> ext4_do_writepages()->mpage_map_and_submit_extent(). Therefore, we
>> should modify ext4_ext_index_trans_blocks() to include a count of the
>> leaf extents in the worst case as well.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>
> 
> One comment below
> 
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index c616a16a9f36..e759941bd262 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -2405,9 +2405,10 @@ int ext4_ext_index_trans_blocks(struct inode *inode, int extents)
>>  	depth = ext_depth(inode);
>>  
>>  	if (extents <= 1)
>> -		index = depth * 2;
>> +		index = depth * 2 + extents;
>>  	else
>> -		index = depth * 3;
>> +		index = depth * 3 +
>> +			DIV_ROUND_UP(extents, ext4_ext_space_block(inode, 0));
>>  
>>  	return index;
>>  }
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index ffbf444b56d4..3e962a760d71 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -5792,18 +5792,16 @@ static int ext4_meta_trans_blocks(struct inode *inode, int lblocks,
>>  	int ret;
>>  
>>  	/*
>> -	 * How many index blocks need to touch to map @lblocks logical blocks
>> -	 * to @pextents physical extents?
>> +	 * How many index and lead blocks need to touch to map @lblocks
>> +	 * logical blocks to @pextents physical extents?
>>  	 */
>>  	idxblocks = ext4_index_trans_blocks(inode, lblocks, pextents);
>>  
>> -	ret = idxblocks;
>> -
>>  	/*
>>  	 * Now let's see how many group bitmaps and group descriptors need
>>  	 * to account
>>  	 */
>> -	groups = idxblocks + pextents;
>> +	groups = idxblocks;
> 
> I don't think you can drop 'pextents' from this computation... Yes, you now
> account possible number of modified extent tree leaf blocks in
> ext4_index_trans_blocks() but additionally, each extent separately may be
> allocated from a different group and thus need to update different bitmap
> and group descriptor block. That is separate from the computation you do in
> ext4_index_trans_blocks() AFAICT...
> 

Yes, that's right! Sorry for my mistake. I will fix this.

Thanks,
Yi.

> 
>>  	gdpblocks = groups;
>>  	if (groups > ngroups)
>>  		groups = ngroups;
>> @@ -5811,7 +5809,7 @@ static int ext4_meta_trans_blocks(struct inode *inode, int lblocks,
>>  		gdpblocks = EXT4_SB(inode->i_sb)->s_gdb_count;
>>  
>>  	/* bitmaps and block group descriptor blocks */
>> -	ret += groups + gdpblocks;
>> +	ret = idxblocks + groups + gdpblocks;
>>  
>>  	/* Blocks for super block, inode, quota and xattr blocks */
>>  	ret += EXT4_META_TRANS_BLOCKS(inode->i_sb);
>> -- 
>> 2.46.1
>>