Re: [PATCH 5/5] ext4: Use buckets for cr 1 block scan instead of rbtree

[Jan Kara <jack@suse.cz>]

On Thu 08-09-22 14:00:17, Dan Carpenter wrote:
> Hi Jan,
> 
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Jan-Kara/ext4-Fix-performance-regression-with-mballoc/20220907-000945
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 53e99dcff61e1523ec1c3628b2d564ba15d32eb7
> config: m68k-randconfig-m041-20220906 (https://download.01.org/0day-ci/archive/20220907/202209071206.u1iHKVzB-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 12.1.0
> 
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> New smatch warnings:
> fs/ext4/mballoc.c:945 ext4_mb_choose_next_group_cr1() error: uninitialized symbol 'grp'.
> 
> vim +/grp +945 fs/ext4/mballoc.c
> 
> 196e402adf2e4c Harshad Shirwadkar 2021-04-01  909  static void ext4_mb_choose_next_group_cr1(struct ext4_allocation_context *ac,
> 196e402adf2e4c Harshad Shirwadkar 2021-04-01  910  		int *new_cr, ext4_group_t *group, ext4_group_t ngroups)
> 196e402adf2e4c Harshad Shirwadkar 2021-04-01  911  {
> 196e402adf2e4c Harshad Shirwadkar 2021-04-01  912  	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
> 31b571b608cf66 Jan Kara           2022-09-06  913  	struct ext4_group_info *grp, *iter;
> 31b571b608cf66 Jan Kara           2022-09-06  914  	int i;
> 196e402adf2e4c Harshad Shirwadkar 2021-04-01  915  
> 196e402adf2e4c Harshad Shirwadkar 2021-04-01  916  	if (unlikely(ac->ac_flags & EXT4_MB_CR1_OPTIMIZED)) {
> 196e402adf2e4c Harshad Shirwadkar 2021-04-01  917  		if (sbi->s_mb_stats)
> 196e402adf2e4c Harshad Shirwadkar 2021-04-01  918  			atomic_inc(&sbi->s_bal_cr1_bad_suggestions);
> 31b571b608cf66 Jan Kara           2022-09-06  919  	}
> 31b571b608cf66 Jan Kara           2022-09-06  920  
> 31b571b608cf66 Jan Kara           2022-09-06  921  	for (i = mb_avg_fragment_size_order(ac->ac_sb, ac->ac_g_ex.fe_len);
> 31b571b608cf66 Jan Kara           2022-09-06  922  	     i < MB_NUM_ORDERS(ac->ac_sb); i++) {
> 31b571b608cf66 Jan Kara           2022-09-06  923  		if (list_empty(&sbi->s_mb_avg_fragment_size[i]))
> 31b571b608cf66 Jan Kara           2022-09-06  924  			continue;
> 31b571b608cf66 Jan Kara           2022-09-06  925  		read_lock(&sbi->s_mb_avg_fragment_size_locks[i]);
> 31b571b608cf66 Jan Kara           2022-09-06  926  		if (list_empty(&sbi->s_mb_avg_fragment_size[i])) {
> 31b571b608cf66 Jan Kara           2022-09-06  927  			read_unlock(&sbi->s_mb_largest_free_orders_locks[i]);
> 31b571b608cf66 Jan Kara           2022-09-06  928  			continue;
> 
> Smatch worries that we can hit these two continues on every iteration.
> Why not just initialize "grp = NULL;" at the start of the function?

Yes, good point. It may even be possible in some rare racy cornercase. I'll
do what you suggest. Thanks!

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