Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 13 Sep 2022 20:20:50 +0530
From:   "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
To:     Yuan Can <yuancan@huawei.com>
Cc:     jack@suse.com, linux-ext4@vger.kernel.org
Subject: Re: [PATCH] fs/ext2: remove unused variable es
Message-ID: <20220913145050.6si6rhpdtsem6vwl@riteshh-domain>
References: <20220913071141.94082-1-yuancan@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220913071141.94082-1-yuancan@huawei.com>
Precedence: bulk

On 22/09/13 07:11AM, Yuan Can wrote:
> The variable es is never used, remove it.
> No functional change.
> 
> Signed-off-by: Yuan Can <yuancan@huawei.com>
> ---
>  fs/ext2/ialloc.c | 2 --
>  1 file changed, 2 deletions(-)

Hi Yuan,

Thanks for the patch - 

However while reviewing this, I also looked at ext2_count_free_blocks(). 
And then I felt maybe the right thing to do is to print more info when
EXT2FS_DEBUG is enabled which would be to dump both stored counters in the debug message 
i.e. (from ext2_super_block -> s_free_**_count, and from ext2_sb_info -> s_free**_counter)

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index c17ccc19b938..87c57ddcd2ed 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -1475,8 +1475,10 @@ unsigned long ext2_count_free_blocks (struct super_block * sb)
                bitmap_count += x;
                brelse(bitmap_bh);
        }
-       printk("ext2_count_free_blocks: stored = %lu, computed = %lu, %lu\n",
-               (long)le32_to_cpu(es->s_free_blocks_count),
+       printk("ext2_count_free_blocks: stored = %lu, %lu, computed = %lu, %lu\n",
+               (unsigned long) le32_to_cpu(es->s_free_blocks_count),
+               (unsigned long)
+               percpu_counter_read(&EXT2_SB(sb)->s_freeblocks_counter),
                desc_count, bitmap_count);
        return bitmap_count;
 #else
diff --git a/fs/ext2/ialloc.c b/fs/ext2/ialloc.c
index 998dd2ac8008..436d5c4d61c0 100644
--- a/fs/ext2/ialloc.c
+++ b/fs/ext2/ialloc.c
@@ -643,7 +643,8 @@ unsigned long ext2_count_free_inodes (struct super_block * sb)
                bitmap_count += x;
        }
        brelse(bitmap_bh);
-       printk("ext2_count_free_inodes: stored = %lu, computed = %lu, %lu\n",
+       printk("ext2_count_free_inodes: stored = %lu, %lu, computed = %lu, %lu\n",
+               (unsigned long) le32_to_cpu(es->s_free_inodes_count),
                (unsigned long)
                percpu_counter_read(&EXT2_SB(sb)->s_freeinodes_counter),
                desc_count, bitmap_count);

@Jan, 
Please do let me know your thoughts on this. This doesn't changes the functionality, 
since the return value remains the same. But it dumps both stored counter values
in debug output, which is what I think the intended behaviour of the print
should be in the first place. 

If this looks correct to you - I can send an official patch fixing this.

-ritesh

> 
> diff --git a/fs/ext2/ialloc.c b/fs/ext2/ialloc.c
> index 998dd2ac8008..951b80a7f7d2 100644
> --- a/fs/ext2/ialloc.c
> +++ b/fs/ext2/ialloc.c
> @@ -620,11 +620,9 @@ unsigned long ext2_count_free_inodes (struct super_block * sb)
>  	int i;	
>  
>  #ifdef EXT2FS_DEBUG
> -	struct ext2_super_block *es;
>  	unsigned long bitmap_count = 0;
>  	struct buffer_head *bitmap_bh = NULL;
>  
> -	es = EXT2_SB(sb)->s_es;
>  	for (i = 0; i < EXT2_SB(sb)->s_groups_count; i++) {
>  		unsigned x;
>  
> -- 
> 2.17.1
>