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

Jan Kara
2025-05-19T20:24:27Z

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

Honza

> 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
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR