Re: [PATCH 1/3] ext4: Teach ext4_meta_trans_blocks() about number of allocated extents

[Zhang Yi <yi.zhang@huaweicloud.com>]

On 8/7/2026 1:59 PM, Ojaswin Mujoo wrote:
> On Fri, Aug 07, 2026 at 10:59:17AM +0800, Zhang Yi wrote:
>> On 8/5/2026 11:35 PM, Jan Kara wrote:
>>> So far ext4_meta_trans_blocks() expects that each extent counted in
>>> @pextents will be allocated in the transaction we estimate credits for.
>>> This is correct for the use in ext4_chunk_trans_blocks() and
>>> ext4_chunk_trans_extent() however the use in atomic write path
>>> (ext4_convert_unwritten_extents_atomic() and ext4_iomap_alloc() for
>>> IOMAP_ATOMIC) unnecessarily overestimates the number of necessary
>>> credits as neither of them allocates any data. Add argument to
>>> ext4_meta_trans_blocks() for number of extents that are going to be
>>> allocated in the transaction.
>>>
>>> Signed-off-by: Jan Kara <jack@suse.cz>
>>
>> Hi Jan,
>>
>> I have some questions below.
>>
>>> ---
>>>  fs/ext4/ext4.h    |  2 +-
>>>  fs/ext4/extents.c |  2 +-
>>>  fs/ext4/inode.c   | 32 +++++++++++++++++---------------
>>>  3 files changed, 19 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>>> index b37c136ea3ab..6e0cc9b845ae 100644
>>> --- a/fs/ext4/ext4.h
>>> +++ b/fs/ext4/ext4.h
>>> @@ -3184,7 +3184,7 @@ extern int ext4_normal_submit_inode_data_buffers(struct jbd2_inode *jinode);
>>>  extern int ext4_chunk_trans_blocks(struct inode *, int nrblocks);
>>>  extern int ext4_chunk_trans_extent(struct inode *inode, int nrblocks);
>>>  extern int ext4_meta_trans_blocks(struct inode *inode, int lblocks,
>>> -				  int pextents);
>>> +				  int pextents, int alloc_extents);
>>>  extern int ext4_block_zero_eof(struct inode *inode, loff_t from, loff_t end);
>>>  extern int ext4_zero_partial_blocks(struct inode *inode, loff_t lstart,
>>>  				    loff_t length, bool *did_zero);
>>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>>> index 91c97af64b31..44ab246a3176 100644
>>> --- a/fs/ext4/extents.c
>>> +++ b/fs/ext4/extents.c
>>> @@ -4976,7 +4976,7 @@ int ext4_convert_unwritten_extents_atomic(handle_t *handle, struct inode *inode,
>>>  		 * it can tell if the extent in the cache is a split extent.
>>>  		 * But for now let's assume pextents as 2 always.
>>>  		 */
>>> -		credits = ext4_meta_trans_blocks(inode, max_blocks, 2);
>>> +		credits = ext4_meta_trans_blocks(inode, max_blocks, 2, 0);
>>>  	}
>>>  
>>>  	if (credits) {
>>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>>> index ce99807c5f5b..f324a54f1dae 100644
>>> --- a/fs/ext4/inode.c
>>> +++ b/fs/ext4/inode.c
>>> @@ -3693,9 +3693,11 @@ static int ext4_iomap_alloc(struct inode *inode, struct ext4_map_blocks *map,
>>>  		if (ret < 0)
>>>  			return ret;
>>>  		if (map->m_len < orig_mlen) {
>>> +			int bpg = EXT4_BLOCKS_PER_GROUP(inode->i_sb);
>>> +
>>>  			map->m_len = orig_mlen;
>>> -			dio_credits = ext4_meta_trans_blocks(inode, orig_mlen,
>>> -							     map->m_len);
>>> +			dio_credits = ext4_meta_trans_blocks(inode, map->m_len,
>>> +							     map->m_len, 0);
>>
>> This pertains to the mixed map case, where holes may exist in between.
>> My understanding is that we cannot assume all blocks are already
>> allocated. Is this right?
> 
> Hi Yi,
> 
> With atomic writes, we can only allow multi block writes when bigalloc
> is set. So the common cases are that the whole bigalloc cluster is
> present or whole is missing.
> 
> If we do see map->m_len < orig_mlen that means the bigalloc cluster
> range we queried has some allocation. This can only happen if someone
> punched through the cluster because punch-hole can still happen at block
> granularity.
> 
> Regardless, if we do see a mixed map here, it means the cluster is
> allocated already, just a few blocks are punched in it, hence we won't
> need to do any allocation.
> 
> Regards,
> ojaswin

Hi, ojaswin,

Thank you for the clarification!

My initial concern wasn't about the new cluster allocation, but rather
the possibility of a new extent being allocated in this cluster. Which
means the hole-covered portion might require a new extent.

But after looking at ext4_map_blocks_atomic_write_slow(), I see that it
guarantees both the old and new blocks in this cluster are written type,
so the new range ought to merge into the existing extent. Furthermore,
ext4_ext_insert_extent() does not perform merging across leaf extent
blocks, so the extent count remains unchanged — no new extent
allocation should happen. But this point is indeed subtle, as you
suggested, adding a comment would be helpful.

Thanks,
Yi.

> 
>>
>>>  		} else {
>>>  			dio_credits = ext4_chunk_trans_blocks(inode,
>>>  							      map->m_len);
>>> @@ -6307,17 +6309,17 @@ static int ext4_index_trans_blocks(struct inode *inode, int lblocks,
>>>  }
>>>  
>>>  /*
>>> - * Account for index blocks, block groups bitmaps and block group
>>> - * descriptor blocks if modify datablocks and index blocks
>>> - * worse case, the indexs blocks spread over different block groups
>>> - *
>>> - * If datablocks are discontiguous, they are possible to spread over
>>> - * different block groups too. If they are contiguous, with flexbg,
>>> - * they could still across block group boundary.
>>> - *
>>> - * Also account for superblock, inode, quota and xattr blocks
>>> + * Calculate number of credits needed in a transaction to:
>>> + *   * Allocate data blocks from @alloc_extents different groups - note that
>>> + *     with flexbg a single physical extent can span multiple groups but
>>> + *     single mballoc request only returns extent within one group.
>>> + *   * Allocate metatadata (extent tree blocks, indirect blocks) to store
>>> + *     pointers to @pextents data extents having @lblocks in total.
>>> + *   * Modify extent tree / indirect block tree, inode, superblock, quota
>>> + *     tracking, xattr blocks
>>>   */
>>> -int ext4_meta_trans_blocks(struct inode *inode, int lblocks, int pextents)
>>> +int ext4_meta_trans_blocks(struct inode *inode, int lblocks, int pextents,
>>> +			   int alloc_extents)
>>
>> From what I can see, alloc_extents currently can only be 0 or equal to
>> pextents — 0 means a pure conversion, and equal to pextents means a new
>> allocation. pextents is supposed to cover the total count of
>> alloc_extents. Should we add some sanity checks to guard against invalid
>> inputs, like alloc_extents > pextents?
>>
>> Thanks,
>> Yi.
>>
>>>  {
>>>  	ext4_group_t groups, ngroups = ext4_get_groups_count(inode->i_sb);
>>>  	int gdpblocks;
>>> @@ -6334,7 +6336,7 @@ int ext4_meta_trans_blocks(struct inode *inode, int lblocks, int pextents)
>>>  	 * Now let's see how many group bitmaps and group descriptors need
>>>  	 * to account
>>>  	 */
>>> -	groups = idxblocks + pextents;
>>> +	groups = idxblocks + alloc_extents;
>>>  	gdpblocks = groups;
>>>  	if (groups > ngroups)
>>>  		groups = ngroups;
>>> @@ -6360,7 +6362,7 @@ int ext4_chunk_trans_extent(struct inode *inode, int nrblocks)
>>>  {
>>>  	int ret;
>>>  
>>> -	ret = ext4_meta_trans_blocks(inode, nrblocks, 1);
>>> +	ret = ext4_meta_trans_blocks(inode, nrblocks, 1, 1);
>>>  	/* Account for data blocks for journalled mode */
>>>  	if (ext4_should_journal_data(inode))
>>>  		ret += nrblocks;
>>> @@ -6378,7 +6380,7 @@ int ext4_chunk_trans_extent(struct inode *inode, int nrblocks)
>>>   */
>>>  int ext4_chunk_trans_blocks(struct inode *inode, int nrblocks)
>>>  {
>>> -	return ext4_meta_trans_blocks(inode, nrblocks, 1);
>>> +	return ext4_meta_trans_blocks(inode, nrblocks, 1, 1);
>>>  }
>>>  
>>>  /*
>>