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

On Fri, Aug 07, 2026 at 03:05:09PM +0800, Zhang Yi wrote:
> 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 <[email protected]>
> >>
> >> 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.

Yes that's correct, we will keep merging into the existing extents so
there will be no new allocation (data or metadata) needed.

Regards,
ojaswin

> 
> 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);
> >>>  }
> >>>  
> >>>  /*
> >>
>