Date:   Thu, 23 Feb 2023 07:04:35 +0300
From:   Dan Carpenter <error27@gmail.com>
To:     Kemeng Shi <shikemeng@huaweicloud.com>
Cc:     oe-kbuild@lists.linux.dev, tytso@mit.edu, adilger.kernel@dilger.ca,
        lkp@intel.com, oe-kbuild-all@lists.linux.dev,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 7/7] ext4: improve inode table blocks counting in
 ext4_num_overhead_clusters
Message-ID: <Y/bl05WBDNT8FPoi@kadam>
References: <202302222219.u328sqfs-lkp@intel.com>
 <1802222f-9c10-9ea3-93fb-6fce086e201a@huaweicloud.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1802222f-9c10-9ea3-93fb-6fce086e201a@huaweicloud.com>
Precedence: bulk

On Thu, Feb 23, 2023 at 09:31:54AM +0800, Kemeng Shi wrote:
> 
> 
> on 2/22/2023 11:13 PM, Dan Carpenter wrote:
> > Hi Kemeng,
> > 
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Kemeng-Shi/ext4-properly-handle-error-of-ext4_init_block_bitmap-in-ext4_read_block_bitmap_nowait/20230221-115830
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
> > patch link:    https://lore.kernel.org/r/20230221115919.1918161-8-shikemeng%40huaweicloud.com
> > patch subject: [PATCH 7/7] ext4: improve inode table blocks counting in ext4_num_overhead_clusters
> > config: riscv-randconfig-m031-20230219 (https://download.01.org/0day-ci/archive/20230222/202302222219.u328sqfs-lkp@intel.com/config)
> > compiler: riscv32-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 <error27@gmail.com>
> > | Link: https://lore.kernel.org/r/202302222219.u328sqfs-lkp@intel.com/
> > 
> > New smatch warnings:
> > fs/ext4/balloc.c:153 ext4_num_overhead_clusters() error: uninitialized symbol 'block_cluster'.
> > 
> > vim +/block_cluster +153 fs/ext4/balloc.c
> [...]
> > d5b8f31007a937 Theodore Ts'o     2011-09-09  128  	/*
> > 2b59a2fd93873a Kemeng Shi        2023-02-21  129  	 * For the allocation bitmaps, we first need to check to see
> > 2b59a2fd93873a Kemeng Shi        2023-02-21  130  	 * if the block is in the block group.  If it is, then check
> > 2b59a2fd93873a Kemeng Shi        2023-02-21  131  	 * to see if the cluster is already accounted for in the clusters
> > 2b59a2fd93873a Kemeng Shi        2023-02-21  132  	 * used for the base metadata cluster and inode tables cluster.
> > d5b8f31007a937 Theodore Ts'o     2011-09-09  133  	 * Normally all of these blocks are contiguous, so the special
> > d5b8f31007a937 Theodore Ts'o     2011-09-09  134  	 * case handling shouldn't be necessary except for *very*
> > d5b8f31007a937 Theodore Ts'o     2011-09-09  135  	 * unusual file system layouts.
> > d5b8f31007a937 Theodore Ts'o     2011-09-09  136  	 */
> > d5b8f31007a937 Theodore Ts'o     2011-09-09  137  	if (ext4_block_in_group(sb, ext4_block_bitmap(sb, gdp), block_group)) {
> > b0dd6b70f0fda1 Theodore Ts'o     2012-06-07  138  		block_cluster = EXT4_B2C(sbi,
> > b0dd6b70f0fda1 Theodore Ts'o     2012-06-07  139  					 ext4_block_bitmap(sb, gdp) - start);
> > 2b59a2fd93873a Kemeng Shi        2023-02-21  140  		if (block_cluster >= base_clusters &&
> > 2b59a2fd93873a Kemeng Shi        2023-02-21  141  		    (block_cluster < itbl_cluster_start ||
> > 2b59a2fd93873a Kemeng Shi        2023-02-21  142  		    block_cluster > itbl_cluster_end))
> > d5b8f31007a937 Theodore Ts'o     2011-09-09  143  			num_clusters++;
> > d5b8f31007a937 Theodore Ts'o     2011-09-09  144  	}
> > d5b8f31007a937 Theodore Ts'o     2011-09-09  145  
> > d5b8f31007a937 Theodore Ts'o     2011-09-09  146  	if (ext4_block_in_group(sb, ext4_inode_bitmap(sb, gdp), block_group)) {
> > 
> > These two conditions are exactly the same so I don't see why they can't
> > be combined into one condition.  I have read the comment, but I guess I
> > don't understand ext4 well enough to really understand it.
> These two conditions check two kinds of bitmap block: *block* bitmap block
> and *inode* bitmap block.

Ah right.  When I was reviewing this code, I copy and pasted the if
conditions so they were exactly lined up with each other and I still
didn't see the block vs inode difference until you pointed it out.  :P

Thanks!

regards,
dan carpenter