Re: [PATCH v2 5/8] ext4: correct the journal credits calculations of allocating blocks

Zhang Yi
2025-05-20T12:53:29Z

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@huawei.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@huawei.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
>>