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 --- 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); } 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) { 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); } /* -- 2.51.0 Commit 95ad8ee45cdb ("ext4: correct the reserved credits for extent conversion") was correct to note that we need to reserve enough credits for all extents possibly underlying a large folio. However it was too eager to reduce the number of reserved credits. Extent conversion may not only need to touch several leaf extent blocks, it may also need to split extents - for example a single large unwritten extent may need to be split into many small written ones in case of sparse folio dirtying. This can thus result not only in extent leaf modifications but also in a need to allocate new extent tree nodes. As a result the reserved transaction credits were not sufficient in some corner cases. Use ext4_meta_trans_blocks() for correct upper bound credit estimate. Fixes: 95ad8ee45cdb ("ext4: correct the reserved credits for extent conversion") Signed-off-by: Jan Kara --- fs/ext4/inode.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index f324a54f1dae..ddce319e53c5 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -2852,10 +2852,10 @@ static int ext4_do_writepages(struct mpage_da_data *mpd) if (ext4_should_dioread_nolock(inode)) { int bpf = ext4_journal_blocks_per_folio(inode); /* - * We may need to convert up to one extent per block in - * the folio and we may dirty the inode. + * We may need to convert up to one extent per block in the + * folio. */ - rsv_blocks = 1 + ext4_ext_index_trans_blocks(inode, bpf); + rsv_blocks = ext4_meta_trans_blocks(inode, bpf, bpf, 0); } if (wbc->range_start == 0 && wbc->range_end == LLONG_MAX) -- 2.51.0 The estimate of the number of impacted extent tree index blocks could be one-too-low. If we modify say 2 extents, already two leaf index blocks could be impacted, not just one the current estimate counts with. Fix the estimate. Signed-off-by: Jan Kara --- fs/ext4/extents.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 44ab246a3176..713b2c098af5 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -2427,9 +2427,17 @@ int ext4_ext_index_trans_blocks(struct inode *inode, int extents) */ if (extents <= 1) index = (EXT4_MAX_EXTENT_DEPTH * 2) + extents; - else - index = (EXT4_MAX_EXTENT_DEPTH * 3) + - DIV_ROUND_UP(extents, ext4_ext_space_block(inode, 0)); + else { + int ext_max = ext4_ext_space_block(inode, 0); + + index = EXT4_MAX_EXTENT_DEPTH * 3; + /* + * Modified extents need not start at the beginning of the + * leaf. Already two extents may need two leaf block + * modifications... + */ + index += DIV_ROUND_UP(extents + ext_max - 1, ext_max); + } return index; } -- 2.51.0