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:   Wed, 14 Sep 2022 10:44:16 +0200
From:   Jan Kara <jack@suse.cz>
To:     "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
Cc:     Yuan Can <yuancan@huawei.com>, jack@suse.com,
        linux-ext4@vger.kernel.org
Subject: Re: [PATCH] fs/ext2: remove unused variable es
Message-ID: <20220914084416.xp7y5tcqtrotjad6@quack3>
References: <20220913071141.94082-1-yuancan@huawei.com>
 <20220913145050.6si6rhpdtsem6vwl@riteshh-domain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220913145050.6si6rhpdtsem6vwl@riteshh-domain>
Precedence: bulk

On Tue 13-09-22 20:20:50, Ritesh Harjani (IBM) wrote:
> 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)

I don't have a strong opinion, but yeah, what you suggest makes sense.

								Honza

> 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
> > 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR