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:   Fri, 4 Feb 2022 12:49:30 +0100
From:   Jan Kara <jack@suse.cz>
To:     Ritesh Harjani <riteshh@linux.ibm.com>
Cc:     Jan Kara <jack@suse.cz>, linux-ext4@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, Theodore Ts'o <tytso@mit.edu>,
        Harshad Shirwadkar <harshadshirwadkar@gmail.com>
Subject: Re: [RFC 2/6] ext4: Implement ext4_group_block_valid() as common
 function
Message-ID: <20220204114930.7n7z2zqhtkzmco3p@quack3.lan>
References: <cover.1643642105.git.riteshh@linux.ibm.com>
 <40c85b86dd324a11c962843d8ef242780a84b25f.1643642105.git.riteshh@linux.ibm.com>
 <20220201113453.exaikdfsc3vubqel@quack3.lan>
 <20220204100844.ty23mdc5mfjbgiwj@riteshh-domain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220204100844.ty23mdc5mfjbgiwj@riteshh-domain>
Precedence: bulk

On Fri 04-02-22 15:38:44, Ritesh Harjani wrote:
> On 22/02/01 12:34PM, Jan Kara wrote:
> > On Mon 31-01-22 20:46:51, Ritesh Harjani wrote:
> > > This patch implements ext4_group_block_valid() check functionality,
> > > and refactors all the callers to use this common function instead.
> > >
> > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > ...
> >
> > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > > index 8d23108cf9d7..60d32d3d8dc4 100644
> > > --- a/fs/ext4/mballoc.c
> > > +++ b/fs/ext4/mballoc.c
> > > @@ -6001,13 +6001,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
> > >  		goto error_return;
> > >  	}
> > >
> > > -	if (in_range(ext4_block_bitmap(sb, gdp), block, count) ||
> > > -	    in_range(ext4_inode_bitmap(sb, gdp), block, count) ||
> > > -	    in_range(block, ext4_inode_table(sb, gdp),
> > > -		     sbi->s_itb_per_group) ||
> > > -	    in_range(block + count - 1, ext4_inode_table(sb, gdp),
> > > -		     sbi->s_itb_per_group)) {
> > > -
> > > +	if (!ext4_group_block_valid(sb, block_group, block, count)) {
> > >  		ext4_error(sb, "Freeing blocks in system zone - "
> > >  			   "Block = %llu, count = %lu", block, count);
> > >  		/* err = 0. ext4_std_error should be a no op */
> >
> > When doing this, why not rather directly use ext4_inode_block_valid() here?
> 
> This is because while freeing these blocks we have their's corresponding block
> group too. So there is little point in checking FS Metadata of all block groups
> v/s FS Metadata of just this block group, no?
> 
> Also, I am not sure if we changing this to check against system-zone's blocks
> (which has FS Metadata blocks from all block groups), can add any additional
> penalty?

I agree the check will be somewhat more costly (rbtree lookup). OTOH with
more complex fs structure (like flexbg which is default for quite some
time), this is by far not checking the only metadata blocks, that can
overlap the freed range. Also this is not checking for freeing journal
blocks. So I'd either got for no check (if we really want performance) or
full check (if we care more about detecting fs errors early). Because these
half-baked checks do not bring much value these days...

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