[PATCH 0/8] Some random cleanups to mballoc

[PATCH 0/8] Some random cleanups to mballoc

[Kemeng Shi]

This series contains some random cleanups to mballoc. No function change
is intended except patch 8 may fix a potential memleak if non-used
preallocation spaces of inode could be greater than UNIT_MAX.
More details can be found in respective patches. Thanks!

Kemeng Shi (8):
  ext4: remove unused return value of __mb_check_buddy
  ext4: remove unused parameter group in ext4_mb_choose_next_group_*()
  ext4: remove unneeded return value of ext4_mb_release_context
  ext4: remove unused ext4_allocation_context::ac_groups_considered
  ext4: remove unused return value of ext4_mb_release
  ext4: remove unused return value of ext4_mb_release_inode_pa
  ext4: remove unused return value of ext4_mb_release_group_pa
  ext4: remove unnecessary parameter "needed" in
    ext4_discard_preallocations

 fs/ext4/ext4.h        |  4 ++--
 fs/ext4/extents.c     | 10 +++++-----
 fs/ext4/file.c        |  2 +-
 fs/ext4/indirect.c    |  2 +-
 fs/ext4/inode.c       |  6 +++---
 fs/ext4/ioctl.c       |  2 +-
 fs/ext4/mballoc.c     | 46 ++++++++++++++++---------------------------
 fs/ext4/mballoc.h     |  1 -
 fs/ext4/move_extent.c |  4 ++--
 fs/ext4/super.c       |  2 +-
 10 files changed, 33 insertions(+), 46 deletions(-)

-- 
2.30.0

[PATCH 1/8] ext4: remove unused return value of __mb_check_buddy

[Kemeng Shi]

Remove unused return value of __mb_check_buddy.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/mballoc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 454d56126..9f9b8dd06 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -677,7 +677,7 @@ do {									\
 	}								\
 } while (0)
 
-static int __mb_check_buddy(struct ext4_buddy *e4b, char *file,
+static void __mb_check_buddy(struct ext4_buddy *e4b, char *file,
 				const char *function, int line)
 {
 	struct super_block *sb = e4b->bd_sb;
@@ -696,7 +696,7 @@ static int __mb_check_buddy(struct ext4_buddy *e4b, char *file,
 	void *buddy2;
 
 	if (e4b->bd_info->bb_check_counter++ % 10)
-		return 0;
+		return;
 
 	while (order > 1) {
 		buddy = mb_find_buddy(e4b, order, &max);
@@ -758,7 +758,7 @@ static int __mb_check_buddy(struct ext4_buddy *e4b, char *file,
 
 	grp = ext4_get_group_info(sb, e4b->bd_group);
 	if (!grp)
-		return NULL;
+		return;
 	list_for_each(cur, &grp->bb_prealloc_list) {
 		ext4_group_t groupnr;
 		struct ext4_prealloc_space *pa;
@@ -768,7 +768,6 @@ static int __mb_check_buddy(struct ext4_buddy *e4b, char *file,
 		for (i = 0; i < pa->pa_len; i++)
 			MB_CHECK_ASSERT(mb_test_bit(k + i, buddy));
 	}
-	return 0;
 }
 #undef MB_CHECK_ASSERT
 #define mb_check_buddy(e4b) __mb_check_buddy(e4b,	\
-- 
2.30.0

[PATCH 2/8] ext4: remove unused parameter group in ext4_mb_choose_next_group_*()

[Kemeng Shi]

Remove unused parameter group in ext4_mb_choose_next_group_*().

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/mballoc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 9f9b8dd06..765b62729 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -870,7 +870,7 @@ mb_update_avg_fragment_size(struct super_block *sb, struct ext4_group_info *grp)
  * cr level needs an update.
  */
 static void ext4_mb_choose_next_group_p2_aligned(struct ext4_allocation_context *ac,
-			enum criteria *new_cr, ext4_group_t *group, ext4_group_t ngroups)
+			enum criteria *new_cr, ext4_group_t *group)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
 	struct ext4_group_info *iter;
@@ -944,7 +944,7 @@ ext4_mb_find_good_group_avg_frag_lists(struct ext4_allocation_context *ac, int o
  * order. Updates *new_cr if cr level needs an update.
  */
 static void ext4_mb_choose_next_group_goal_fast(struct ext4_allocation_context *ac,
-		enum criteria *new_cr, ext4_group_t *group, ext4_group_t ngroups)
+		enum criteria *new_cr, ext4_group_t *group)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
 	struct ext4_group_info *grp = NULL;
@@ -989,7 +989,7 @@ static void ext4_mb_choose_next_group_goal_fast(struct ext4_allocation_context *
  * much and fall to CR_GOAL_LEN_SLOW in that case.
  */
 static void ext4_mb_choose_next_group_best_avail(struct ext4_allocation_context *ac,
-		enum criteria *new_cr, ext4_group_t *group, ext4_group_t ngroups)
+		enum criteria *new_cr, ext4_group_t *group)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
 	struct ext4_group_info *grp = NULL;
@@ -1124,11 +1124,11 @@ static void ext4_mb_choose_next_group(struct ext4_allocation_context *ac,
 	}
 
 	if (*new_cr == CR_POWER2_ALIGNED) {
-		ext4_mb_choose_next_group_p2_aligned(ac, new_cr, group, ngroups);
+		ext4_mb_choose_next_group_p2_aligned(ac, new_cr, group);
 	} else if (*new_cr == CR_GOAL_LEN_FAST) {
-		ext4_mb_choose_next_group_goal_fast(ac, new_cr, group, ngroups);
+		ext4_mb_choose_next_group_goal_fast(ac, new_cr, group);
 	} else if (*new_cr == CR_BEST_AVAIL_LEN) {
-		ext4_mb_choose_next_group_best_avail(ac, new_cr, group, ngroups);
+		ext4_mb_choose_next_group_best_avail(ac, new_cr, group);
 	} else {
 		/*
 		 * TODO: For CR=2, we can arrange groups in an rb tree sorted by
-- 
2.30.0

[PATCH 3/8] ext4: remove unneeded return value of ext4_mb_release_context

[Kemeng Shi]

Function ext4_mb_release_context always return 0 and the return value is
never used. Just remove unneeded return value of ext4_mb_release_context.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/mballoc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 765b62729..f79e87ad3 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -5939,7 +5939,7 @@ static void ext4_mb_add_n_trim(struct ext4_allocation_context *ac)
 /*
  * release all resource we used in allocation
  */
-static int ext4_mb_release_context(struct ext4_allocation_context *ac)
+static void ext4_mb_release_context(struct ext4_allocation_context *ac)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
 	struct ext4_prealloc_space *pa = ac->ac_pa;
@@ -5976,7 +5976,6 @@ static int ext4_mb_release_context(struct ext4_allocation_context *ac)
 	if (ac->ac_flags & EXT4_MB_HINT_GROUP_ALLOC)
 		mutex_unlock(&ac->ac_lg->lg_mutex);
 	ext4_mb_collect_stats(ac);
-	return 0;
 }
 
 static int ext4_mb_discard_preallocations(struct super_block *sb, int needed)
-- 
2.30.0

[PATCH 4/8] ext4: remove unused ext4_allocation_context::ac_groups_considered

[Kemeng Shi]

Remove unused ext4_allocation_context::ac_groups_considered

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/mballoc.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/ext4/mballoc.h b/fs/ext4/mballoc.h
index d7aeb5da7..56938532b 100644
--- a/fs/ext4/mballoc.h
+++ b/fs/ext4/mballoc.h
@@ -192,7 +192,6 @@ struct ext4_allocation_context {
 	 */
 	ext4_grpblk_t	ac_orig_goal_len;
 
-	__u32 ac_groups_considered;
 	__u32 ac_flags;		/* allocation hints */
 	__u16 ac_groups_scanned;
 	__u16 ac_groups_linear_remaining;
-- 
2.30.0

[PATCH 5/8] ext4: remove unused return value of ext4_mb_release

[Kemeng Shi]

Remove unused return value of ext4_mb_release.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/ext4.h    | 2 +-
 fs/ext4/mballoc.c | 4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 8ccebe0d4..fb35cae16 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2902,7 +2902,7 @@ extern const struct seq_operations ext4_mb_seq_groups_ops;
 extern const struct seq_operations ext4_mb_seq_structs_summary_ops;
 extern int ext4_seq_mb_stats_show(struct seq_file *seq, void *offset);
 extern int ext4_mb_init(struct super_block *);
-extern int ext4_mb_release(struct super_block *);
+extern void ext4_mb_release(struct super_block *);
 extern ext4_fsblk_t ext4_mb_new_blocks(handle_t *,
 				struct ext4_allocation_request *, int *);
 extern void ext4_discard_preallocations(struct inode *, unsigned int);
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index f79e87ad3..8d6246db3 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3725,7 +3725,7 @@ static int ext4_mb_cleanup_pa(struct ext4_group_info *grp)
 	return count;
 }
 
-int ext4_mb_release(struct super_block *sb)
+void ext4_mb_release(struct super_block *sb)
 {
 	ext4_group_t ngroups = ext4_get_groups_count(sb);
 	ext4_group_t i;
@@ -3801,8 +3801,6 @@ int ext4_mb_release(struct super_block *sb)
 	}
 
 	free_percpu(sbi->s_locality_groups);
-
-	return 0;
 }
 
 static inline int ext4_issue_discard(struct super_block *sb,
-- 
2.30.0

[PATCH 6/8] ext4: remove unused return value of ext4_mb_release_inode_pa

[Kemeng Shi]

Remove unused return value of ext4_mb_release_inode_pa

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/mballoc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 8d6246db3..1ed6d1d5a 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -5278,7 +5278,7 @@ static void ext4_mb_new_preallocation(struct ext4_allocation_context *ac)
  * the caller MUST hold group/inode locks.
  * TODO: optimize the case when there are no in-core structures yet
  */
-static noinline_for_stack int
+static noinline_for_stack void
 ext4_mb_release_inode_pa(struct ext4_buddy *e4b, struct buffer_head *bitmap_bh,
 			struct ext4_prealloc_space *pa)
 {
@@ -5328,8 +5328,6 @@ ext4_mb_release_inode_pa(struct ext4_buddy *e4b, struct buffer_head *bitmap_bh,
 		 */
 	}
 	atomic_add(free, &sbi->s_mb_discarded);
-
-	return 0;
 }
 
 static noinline_for_stack int
-- 
2.30.0

[PATCH 7/8] ext4: remove unused return value of ext4_mb_release_group_pa

[Kemeng Shi]

Remove unused return value of ext4_mb_release_group_pa.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/mballoc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 1ed6d1d5a..e6561a09d 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -5330,7 +5330,7 @@ ext4_mb_release_inode_pa(struct ext4_buddy *e4b, struct buffer_head *bitmap_bh,
 	atomic_add(free, &sbi->s_mb_discarded);
 }
 
-static noinline_for_stack int
+static noinline_for_stack void
 ext4_mb_release_group_pa(struct ext4_buddy *e4b,
 				struct ext4_prealloc_space *pa)
 {
@@ -5344,13 +5344,11 @@ ext4_mb_release_group_pa(struct ext4_buddy *e4b,
 	if (unlikely(group != e4b->bd_group && pa->pa_len != 0)) {
 		ext4_warning(sb, "bad group: expected %u, group %u, pa_start %llu",
 			     e4b->bd_group, group, pa->pa_pstart);
-		return 0;
+		return;
 	}
 	mb_free_blocks(pa->pa_inode, e4b, bit, pa->pa_len);
 	atomic_add(pa->pa_len, &EXT4_SB(sb)->s_mb_discarded);
 	trace_ext4_mballoc_discard(sb, NULL, group, bit, pa->pa_len);
-
-	return 0;
 }
 
 /*
-- 
2.30.0

[PATCH 8/8] ext4: remove unnecessary parameter "needed" in ext4_discard_preallocations

[Kemeng Shi]

The "needed" controls the number of ext4_prealloc_space to discard in
ext4_discard_preallocations. Function ext4_discard_preallocations is
supposed to discard all non-used preallocated blocks when "needed"
is 0 and now ext4_discard_preallocations is always called with "needed"
= 0. Remove unnecessary parameter "needed" and remove all non-used
preallocated spaces in ext4_discard_preallocations to simplify the
code.

Note: If count of non-used preallocated spaces could be more than
UINT_MAX, there was a memory leak as some non-used preallocated
spaces are left unused and this commit will fix it. Otherwise,
there is no behavior change.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/ext4.h        |  2 +-
 fs/ext4/extents.c     | 10 +++++-----
 fs/ext4/file.c        |  2 +-
 fs/ext4/indirect.c    |  2 +-
 fs/ext4/inode.c       |  6 +++---
 fs/ext4/ioctl.c       |  2 +-
 fs/ext4/mballoc.c     | 10 +++-------
 fs/ext4/move_extent.c |  4 ++--
 fs/ext4/super.c       |  2 +-
 9 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index fb35cae16..2fd444034 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2905,7 +2905,7 @@ extern int ext4_mb_init(struct super_block *);
 extern void ext4_mb_release(struct super_block *);
 extern ext4_fsblk_t ext4_mb_new_blocks(handle_t *,
 				struct ext4_allocation_request *, int *);
-extern void ext4_discard_preallocations(struct inode *, unsigned int);
+extern void ext4_discard_preallocations(struct inode *);
 extern int __init ext4_init_mballoc(void);
 extern void ext4_exit_mballoc(void);
 extern ext4_group_t ext4_mb_prefetch(struct super_block *sb,
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 880f383df..fd350889a 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -100,7 +100,7 @@ static int ext4_ext_trunc_restart_fn(struct inode *inode, int *dropped)
 	 * i_rwsem. So we can safely drop the i_data_sem here.
 	 */
 	BUG_ON(EXT4_JOURNAL(inode) == NULL);
-	ext4_discard_preallocations(inode, 0);
+	ext4_discard_preallocations(inode);
 	up_write(&EXT4_I(inode)->i_data_sem);
 	*dropped = 1;
 	return 0;
@@ -4313,7 +4313,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 			 * not a good idea to call discard here directly,
 			 * but otherwise we'd need to call it every free().
 			 */
-			ext4_discard_preallocations(inode, 0);
+			ext4_discard_preallocations(inode);
 			if (flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE)
 				fb_flags = EXT4_FREE_BLOCKS_NO_QUOT_UPDATE;
 			ext4_free_blocks(handle, inode, NULL, newblock,
@@ -5354,7 +5354,7 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len)
 	ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_FALLOC_RANGE, handle);
 
 	down_write(&EXT4_I(inode)->i_data_sem);
-	ext4_discard_preallocations(inode, 0);
+	ext4_discard_preallocations(inode);
 	ext4_es_remove_extent(inode, punch_start, EXT_MAX_BLOCKS - punch_start);
 
 	ret = ext4_ext_remove_space(inode, punch_start, punch_stop - 1);
@@ -5362,7 +5362,7 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len)
 		up_write(&EXT4_I(inode)->i_data_sem);
 		goto out_stop;
 	}
-	ext4_discard_preallocations(inode, 0);
+	ext4_discard_preallocations(inode);
 
 	ret = ext4_ext_shift_extents(inode, handle, punch_stop,
 				     punch_stop - punch_start, SHIFT_LEFT);
@@ -5494,7 +5494,7 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
 		goto out_stop;
 
 	down_write(&EXT4_I(inode)->i_data_sem);
-	ext4_discard_preallocations(inode, 0);
+	ext4_discard_preallocations(inode);
 
 	path = ext4_find_extent(inode, offset_lblk, NULL, 0);
 	if (IS_ERR(path)) {
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 0166bb9ca..89cb28da8 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -174,7 +174,7 @@ static int ext4_release_file(struct inode *inode, struct file *filp)
 			(atomic_read(&inode->i_writecount) == 1) &&
 			!EXT4_I(inode)->i_reserved_data_blocks) {
 		down_write(&EXT4_I(inode)->i_data_sem);
-		ext4_discard_preallocations(inode, 0);
+		ext4_discard_preallocations(inode);
 		up_write(&EXT4_I(inode)->i_data_sem);
 	}
 	if (is_dx(inode) && filp->private_data)
diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index a9f371611..d8ca7f64f 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -714,7 +714,7 @@ static int ext4_ind_trunc_restart_fn(handle_t *handle, struct inode *inode,
 	 * i_rwsem. So we can safely drop the i_data_sem here.
 	 */
 	BUG_ON(EXT4_JOURNAL(inode) == NULL);
-	ext4_discard_preallocations(inode, 0);
+	ext4_discard_preallocations(inode);
 	up_write(&EXT4_I(inode)->i_data_sem);
 	*dropped = 1;
 	return 0;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index d77323204..53ed4a0ad 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -371,7 +371,7 @@ void ext4_da_update_reserve_space(struct inode *inode,
 	 */
 	if ((ei->i_reserved_data_blocks == 0) &&
 	    !inode_is_open_for_write(inode))
-		ext4_discard_preallocations(inode, 0);
+		ext4_discard_preallocations(inode);
 }
 
 static int __check_block_validity(struct inode *inode, const char *func,
@@ -4014,7 +4014,7 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
 	if (stop_block > first_block) {
 
 		down_write(&EXT4_I(inode)->i_data_sem);
-		ext4_discard_preallocations(inode, 0);
+		ext4_discard_preallocations(inode);
 
 		ext4_es_remove_extent(inode, first_block,
 				      stop_block - first_block);
@@ -4167,7 +4167,7 @@ int ext4_truncate(struct inode *inode)
 
 	down_write(&EXT4_I(inode)->i_data_sem);
 
-	ext4_discard_preallocations(inode, 0);
+	ext4_discard_preallocations(inode);
 
 	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
 		err = ext4_ext_truncate(handle, inode);
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 0bfe2ce58..2e2bcd22b 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -458,7 +458,7 @@ static long swap_inode_boot_loader(struct super_block *sb,
 	ext4_reset_inode_seed(inode);
 	ext4_reset_inode_seed(inode_bl);
 
-	ext4_discard_preallocations(inode, 0);
+	ext4_discard_preallocations(inode);
 
 	err = ext4_mark_inode_dirty(handle, inode);
 	if (err < 0) {
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index e6561a09d..0e6beb3b4 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -5469,7 +5469,7 @@ ext4_mb_discard_group_preallocations(struct super_block *sb,
  *
  * FIXME!! Make sure it is valid at all the call sites
  */
-void ext4_discard_preallocations(struct inode *inode, unsigned int needed)
+void ext4_discard_preallocations(struct inode *inode)
 {
 	struct ext4_inode_info *ei = EXT4_I(inode);
 	struct super_block *sb = inode->i_sb;
@@ -5491,15 +5491,12 @@ void ext4_discard_preallocations(struct inode *inode, unsigned int needed)
 	mb_debug(sb, "discard preallocation for inode %lu\n",
 		 inode->i_ino);
 	trace_ext4_discard_preallocations(inode,
-			atomic_read(&ei->i_prealloc_active), needed);
-
-	if (needed == 0)
-		needed = UINT_MAX;
+			atomic_read(&ei->i_prealloc_active), 0);
 
 repeat:
 	/* first, collect all pa's in the inode */
 	write_lock(&ei->i_prealloc_lock);
-	for (iter = rb_first(&ei->i_prealloc_node); iter && needed;
+	for (iter = rb_first(&ei->i_prealloc_node); iter;
 	     iter = rb_next(iter)) {
 		pa = rb_entry(iter, struct ext4_prealloc_space,
 			      pa_node.inode_node);
@@ -5523,7 +5520,6 @@ void ext4_discard_preallocations(struct inode *inode, unsigned int needed)
 			spin_unlock(&pa->pa_lock);
 			rb_erase(&pa->pa_node.inode_node, &ei->i_prealloc_node);
 			list_add(&pa->u.pa_tmp_list, &list);
-			needed--;
 			continue;
 		}
 
diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index 18a9e7c47..0abfc104a 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -689,8 +689,8 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
 
 out:
 	if (*moved_len) {
-		ext4_discard_preallocations(orig_inode, 0);
-		ext4_discard_preallocations(donor_inode, 0);
+		ext4_discard_preallocations(orig_inode);
+		ext4_discard_preallocations(donor_inode);
 	}
 
 	ext4_free_ext_path(path);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index d062383ea..c0cfc3e46 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1518,7 +1518,7 @@ void ext4_clear_inode(struct inode *inode)
 	ext4_fc_del(inode);
 	invalidate_inode_buffers(inode);
 	clear_inode(inode);
-	ext4_discard_preallocations(inode, 0);
+	ext4_discard_preallocations(inode);
 	ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
 	dquot_drop(inode);
 	if (EXT4_I(inode)->jinode) {
-- 
2.30.0

Re: [PATCH 1/8] ext4: remove unused return value of __mb_check_buddy

[Jan Kara]

On Sun 26-11-23 00:11:36, Kemeng Shi wrote:
> Remove unused return value of __mb_check_buddy.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/mballoc.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 454d56126..9f9b8dd06 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -677,7 +677,7 @@ do {									\
>  	}								\
>  } while (0)
>  
> -static int __mb_check_buddy(struct ext4_buddy *e4b, char *file,
> +static void __mb_check_buddy(struct ext4_buddy *e4b, char *file,
>  				const char *function, int line)
>  {
>  	struct super_block *sb = e4b->bd_sb;
> @@ -696,7 +696,7 @@ static int __mb_check_buddy(struct ext4_buddy *e4b, char *file,
>  	void *buddy2;
>  
>  	if (e4b->bd_info->bb_check_counter++ % 10)
> -		return 0;
> +		return;
>  
>  	while (order > 1) {
>  		buddy = mb_find_buddy(e4b, order, &max);
> @@ -758,7 +758,7 @@ static int __mb_check_buddy(struct ext4_buddy *e4b, char *file,
>  
>  	grp = ext4_get_group_info(sb, e4b->bd_group);
>  	if (!grp)
> -		return NULL;
> +		return;
>  	list_for_each(cur, &grp->bb_prealloc_list) {
>  		ext4_group_t groupnr;
>  		struct ext4_prealloc_space *pa;
> @@ -768,7 +768,6 @@ static int __mb_check_buddy(struct ext4_buddy *e4b, char *file,
>  		for (i = 0; i < pa->pa_len; i++)
>  			MB_CHECK_ASSERT(mb_test_bit(k + i, buddy));
>  	}
> -	return 0;
>  }
>  #undef MB_CHECK_ASSERT
>  #define mb_check_buddy(e4b) __mb_check_buddy(e4b,	\
> -- 
> 2.30.0
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/8] ext4: remove unused parameter group in ext4_mb_choose_next_group_*()

[Jan Kara]

On Sun 26-11-23 00:11:37, Kemeng Shi wrote:
> Remove unused parameter group in ext4_mb_choose_next_group_*().
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Makes sense. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/mballoc.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 9f9b8dd06..765b62729 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -870,7 +870,7 @@ mb_update_avg_fragment_size(struct super_block *sb, struct ext4_group_info *grp)
>   * cr level needs an update.
>   */
>  static void ext4_mb_choose_next_group_p2_aligned(struct ext4_allocation_context *ac,
> -			enum criteria *new_cr, ext4_group_t *group, ext4_group_t ngroups)
> +			enum criteria *new_cr, ext4_group_t *group)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
>  	struct ext4_group_info *iter;
> @@ -944,7 +944,7 @@ ext4_mb_find_good_group_avg_frag_lists(struct ext4_allocation_context *ac, int o
>   * order. Updates *new_cr if cr level needs an update.
>   */
>  static void ext4_mb_choose_next_group_goal_fast(struct ext4_allocation_context *ac,
> -		enum criteria *new_cr, ext4_group_t *group, ext4_group_t ngroups)
> +		enum criteria *new_cr, ext4_group_t *group)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
>  	struct ext4_group_info *grp = NULL;
> @@ -989,7 +989,7 @@ static void ext4_mb_choose_next_group_goal_fast(struct ext4_allocation_context *
>   * much and fall to CR_GOAL_LEN_SLOW in that case.
>   */
>  static void ext4_mb_choose_next_group_best_avail(struct ext4_allocation_context *ac,
> -		enum criteria *new_cr, ext4_group_t *group, ext4_group_t ngroups)
> +		enum criteria *new_cr, ext4_group_t *group)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
>  	struct ext4_group_info *grp = NULL;
> @@ -1124,11 +1124,11 @@ static void ext4_mb_choose_next_group(struct ext4_allocation_context *ac,
>  	}
>  
>  	if (*new_cr == CR_POWER2_ALIGNED) {
> -		ext4_mb_choose_next_group_p2_aligned(ac, new_cr, group, ngroups);
> +		ext4_mb_choose_next_group_p2_aligned(ac, new_cr, group);
>  	} else if (*new_cr == CR_GOAL_LEN_FAST) {
> -		ext4_mb_choose_next_group_goal_fast(ac, new_cr, group, ngroups);
> +		ext4_mb_choose_next_group_goal_fast(ac, new_cr, group);
>  	} else if (*new_cr == CR_BEST_AVAIL_LEN) {
> -		ext4_mb_choose_next_group_best_avail(ac, new_cr, group, ngroups);
> +		ext4_mb_choose_next_group_best_avail(ac, new_cr, group);
>  	} else {
>  		/*
>  		 * TODO: For CR=2, we can arrange groups in an rb tree sorted by
> -- 
> 2.30.0
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/8] ext4: remove unneeded return value of ext4_mb_release_context

[Jan Kara]

On Sun 26-11-23 00:11:38, Kemeng Shi wrote:
> Function ext4_mb_release_context always return 0 and the return value is
> never used. Just remove unneeded return value of ext4_mb_release_context.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/mballoc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 765b62729..f79e87ad3 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -5939,7 +5939,7 @@ static void ext4_mb_add_n_trim(struct ext4_allocation_context *ac)
>  /*
>   * release all resource we used in allocation
>   */
> -static int ext4_mb_release_context(struct ext4_allocation_context *ac)
> +static void ext4_mb_release_context(struct ext4_allocation_context *ac)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
>  	struct ext4_prealloc_space *pa = ac->ac_pa;
> @@ -5976,7 +5976,6 @@ static int ext4_mb_release_context(struct ext4_allocation_context *ac)
>  	if (ac->ac_flags & EXT4_MB_HINT_GROUP_ALLOC)
>  		mutex_unlock(&ac->ac_lg->lg_mutex);
>  	ext4_mb_collect_stats(ac);
> -	return 0;
>  }
>  
>  static int ext4_mb_discard_preallocations(struct super_block *sb, int needed)
> -- 
> 2.30.0
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/8] ext4: remove unused ext4_allocation_context::ac_groups_considered

[Jan Kara]

On Sun 26-11-23 00:11:39, Kemeng Shi wrote:
> Remove unused ext4_allocation_context::ac_groups_considered
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Hum, indeed. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/mballoc.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/ext4/mballoc.h b/fs/ext4/mballoc.h
> index d7aeb5da7..56938532b 100644
> --- a/fs/ext4/mballoc.h
> +++ b/fs/ext4/mballoc.h
> @@ -192,7 +192,6 @@ struct ext4_allocation_context {
>  	 */
>  	ext4_grpblk_t	ac_orig_goal_len;
>  
> -	__u32 ac_groups_considered;
>  	__u32 ac_flags;		/* allocation hints */
>  	__u16 ac_groups_scanned;
>  	__u16 ac_groups_linear_remaining;
> -- 
> 2.30.0
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 5/8] ext4: remove unused return value of ext4_mb_release

[Jan Kara]

On Sun 26-11-23 00:11:40, Kemeng Shi wrote:
> Remove unused return value of ext4_mb_release.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/ext4.h    | 2 +-
>  fs/ext4/mballoc.c | 4 +---
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 8ccebe0d4..fb35cae16 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2902,7 +2902,7 @@ extern const struct seq_operations ext4_mb_seq_groups_ops;
>  extern const struct seq_operations ext4_mb_seq_structs_summary_ops;
>  extern int ext4_seq_mb_stats_show(struct seq_file *seq, void *offset);
>  extern int ext4_mb_init(struct super_block *);
> -extern int ext4_mb_release(struct super_block *);
> +extern void ext4_mb_release(struct super_block *);
>  extern ext4_fsblk_t ext4_mb_new_blocks(handle_t *,
>  				struct ext4_allocation_request *, int *);
>  extern void ext4_discard_preallocations(struct inode *, unsigned int);
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index f79e87ad3..8d6246db3 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3725,7 +3725,7 @@ static int ext4_mb_cleanup_pa(struct ext4_group_info *grp)
>  	return count;
>  }
>  
> -int ext4_mb_release(struct super_block *sb)
> +void ext4_mb_release(struct super_block *sb)
>  {
>  	ext4_group_t ngroups = ext4_get_groups_count(sb);
>  	ext4_group_t i;
> @@ -3801,8 +3801,6 @@ int ext4_mb_release(struct super_block *sb)
>  	}
>  
>  	free_percpu(sbi->s_locality_groups);
> -
> -	return 0;
>  }
>  
>  static inline int ext4_issue_discard(struct super_block *sb,
> -- 
> 2.30.0
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 6/8] ext4: remove unused return value of ext4_mb_release_inode_pa

[Jan Kara]

On Sun 26-11-23 00:11:41, Kemeng Shi wrote:
> Remove unused return value of ext4_mb_release_inode_pa
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/mballoc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 8d6246db3..1ed6d1d5a 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -5278,7 +5278,7 @@ static void ext4_mb_new_preallocation(struct ext4_allocation_context *ac)
>   * the caller MUST hold group/inode locks.
>   * TODO: optimize the case when there are no in-core structures yet
>   */
> -static noinline_for_stack int
> +static noinline_for_stack void
>  ext4_mb_release_inode_pa(struct ext4_buddy *e4b, struct buffer_head *bitmap_bh,
>  			struct ext4_prealloc_space *pa)
>  {
> @@ -5328,8 +5328,6 @@ ext4_mb_release_inode_pa(struct ext4_buddy *e4b, struct buffer_head *bitmap_bh,
>  		 */
>  	}
>  	atomic_add(free, &sbi->s_mb_discarded);
> -
> -	return 0;
>  }
>  
>  static noinline_for_stack int
> -- 
> 2.30.0
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 7/8] ext4: remove unused return value of ext4_mb_release_group_pa

[Jan Kara]

On Sun 26-11-23 00:11:42, Kemeng Shi wrote:
> Remove unused return value of ext4_mb_release_group_pa.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/mballoc.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 1ed6d1d5a..e6561a09d 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -5330,7 +5330,7 @@ ext4_mb_release_inode_pa(struct ext4_buddy *e4b, struct buffer_head *bitmap_bh,
>  	atomic_add(free, &sbi->s_mb_discarded);
>  }
>  
> -static noinline_for_stack int
> +static noinline_for_stack void
>  ext4_mb_release_group_pa(struct ext4_buddy *e4b,
>  				struct ext4_prealloc_space *pa)
>  {
> @@ -5344,13 +5344,11 @@ ext4_mb_release_group_pa(struct ext4_buddy *e4b,
>  	if (unlikely(group != e4b->bd_group && pa->pa_len != 0)) {
>  		ext4_warning(sb, "bad group: expected %u, group %u, pa_start %llu",
>  			     e4b->bd_group, group, pa->pa_pstart);
> -		return 0;
> +		return;
>  	}
>  	mb_free_blocks(pa->pa_inode, e4b, bit, pa->pa_len);
>  	atomic_add(pa->pa_len, &EXT4_SB(sb)->s_mb_discarded);
>  	trace_ext4_mballoc_discard(sb, NULL, group, bit, pa->pa_len);
> -
> -	return 0;
>  }
>  
>  /*
> -- 
> 2.30.0
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 8/8] ext4: remove unnecessary parameter "needed" in ext4_discard_preallocations

[Jan Kara]

On Sun 26-11-23 00:11:43, Kemeng Shi wrote:
> The "needed" controls the number of ext4_prealloc_space to discard in
> ext4_discard_preallocations. Function ext4_discard_preallocations is
> supposed to discard all non-used preallocated blocks when "needed"
> is 0 and now ext4_discard_preallocations is always called with "needed"
> = 0. Remove unnecessary parameter "needed" and remove all non-used
> preallocated spaces in ext4_discard_preallocations to simplify the
> code.
> 
> Note: If count of non-used preallocated spaces could be more than
> UINT_MAX, there was a memory leak as some non-used preallocated
> spaces are left unused and this commit will fix it. Otherwise,
> there is no behavior change.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks good, just one nit:

> @@ -5491,15 +5491,12 @@ void ext4_discard_preallocations(struct inode *inode, unsigned int needed)
>  	mb_debug(sb, "discard preallocation for inode %lu\n",
>  		 inode->i_ino);
>  	trace_ext4_discard_preallocations(inode,
> -			atomic_read(&ei->i_prealloc_active), needed);
> -
> -	if (needed == 0)
> -		needed = UINT_MAX;
> +			atomic_read(&ei->i_prealloc_active), 0);

There's no point in always returning 0 in the tracepoint so I'd just update
it to not print the 'needed' value as well.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 8/8] ext4: remove unnecessary parameter "needed" in ext4_discard_preallocations

[Kemeng Shi]

on 1/2/2024 9:10 PM, Jan Kara wrote:
> On Sun 26-11-23 00:11:43, Kemeng Shi wrote:
>> The "needed" controls the number of ext4_prealloc_space to discard in
>> ext4_discard_preallocations. Function ext4_discard_preallocations is
>> supposed to discard all non-used preallocated blocks when "needed"
>> is 0 and now ext4_discard_preallocations is always called with "needed"
>> = 0. Remove unnecessary parameter "needed" and remove all non-used
>> preallocated spaces in ext4_discard_preallocations to simplify the
>> code.
>>
>> Note: If count of non-used preallocated spaces could be more than
>> UINT_MAX, there was a memory leak as some non-used preallocated
>> spaces are left unused and this commit will fix it. Otherwise,
>> there is no behavior change.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> 
> Looks good, just one nit:
> 
>> @@ -5491,15 +5491,12 @@ void ext4_discard_preallocations(struct inode *inode, unsigned int needed)
>>  	mb_debug(sb, "discard preallocation for inode %lu\n",
>>  		 inode->i_ino);
>>  	trace_ext4_discard_preallocations(inode,
>> -			atomic_read(&ei->i_prealloc_active), needed);
>> -
>> -	if (needed == 0)
>> -		needed = UINT_MAX;
>> +			atomic_read(&ei->i_prealloc_active), 0);
> 
> There's no point in always returning 0 in the tracepoint so I'd just update
> it to not print the 'needed' value as well.
Thanks for review! Sure, 'needed' looks meaningless to the tracepoint. I
will remove it in next version.
> 
> 								Honza
>