| Message ID | 40c85b86dd324a11c962843d8ef242780a84b25f.1643642105.git.riteshh@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ext4: fast_commit fixes and some minor cleanups | expand |
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? > @@ -6194,11 +6188,7 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb, > goto error_return; > } > > - if (in_range(ext4_block_bitmap(sb, desc), block, count) || > - in_range(ext4_inode_bitmap(sb, desc), block, count) || > - in_range(block, ext4_inode_table(sb, desc), sbi->s_itb_per_group) || > - in_range(block + count - 1, ext4_inode_table(sb, desc), > - sbi->s_itb_per_group)) { > + if (!ext4_group_block_valid(sb, block_group, block, count)) { > ext4_error(sb, "Adding blocks in system zones - " > "Block = %llu, count = %lu", > block, count); And here I'd rather refactor ext4_inode_block_valid() a bit to provide a more generic helper not requiring an inode and use it here... Honza
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? -riteshh > > > @@ -6194,11 +6188,7 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb, > > goto error_return; > > } > > > > - if (in_range(ext4_block_bitmap(sb, desc), block, count) || > > - in_range(ext4_inode_bitmap(sb, desc), block, count) || > > - in_range(block, ext4_inode_table(sb, desc), sbi->s_itb_per_group) || > > - in_range(block + count - 1, ext4_inode_table(sb, desc), > > - sbi->s_itb_per_group)) { > > + if (!ext4_group_block_valid(sb, block_group, block, count)) { > > ext4_error(sb, "Adding blocks in system zones - " > > "Block = %llu, count = %lu", > > block, count); > > And here I'd rather refactor ext4_inode_block_valid() a bit to provide a > more generic helper not requiring an inode and use it here... > > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
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
On 22/02/04 12:49PM, Jan Kara wrote: > 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... Agreed. Thanks for putting out your points. I am making these suggested changes to add stricter checking via ext4_inode_block_valid() and will be sending out v1 soon. -ritesh
diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c index 4666b55b736e..01d822c664df 100644 --- a/fs/ext4/block_validity.c +++ b/fs/ext4/block_validity.c @@ -361,3 +361,34 @@ int ext4_check_blockref(const char *function, unsigned int line, return 0; } +/* + * ext4_group_block_valid - This checks if any of FS metadata blocks of a + * given group (@bg) lies in the given range [block, block + count - 1] + * or not. + * + * Return - + * - false if it does + * - else true + */ +bool ext4_group_block_valid(struct super_block *sb, ext4_group_t bg, + ext4_fsblk_t block, unsigned int count) +{ + struct ext4_group_desc *gdp; + bool ret = true; + + gdp = ext4_get_group_desc(sb, bg, NULL); + if (!gdp) { + ret = false; + goto out; + } + + 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), + EXT4_SB(sb)->s_itb_per_group) || + in_range(block + count - 1, ext4_inode_table(sb, gdp), + EXT4_SB(sb)->s_itb_per_group)) + ret = false; +out: + return ret; +} diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 18cd5b3b4815..fc7aa4b3e415 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -3706,6 +3706,9 @@ extern int ext4_inode_block_valid(struct inode *inode, unsigned int count); extern int ext4_check_blockref(const char *, unsigned int, struct inode *, __le32 *, unsigned int); +extern bool ext4_group_block_valid(struct super_block *sb, ext4_group_t bg, + ext4_fsblk_t block, unsigned int count); + /* extents.c */ struct ext4_ext_path; 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 */ @@ -6078,7 +6072,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode, NULL); if (err && err != -EOPNOTSUPP) ext4_msg(sb, KERN_WARNING, "discard request in" - " group:%d block:%d count:%lu failed" + " group:%u block:%d count:%lu failed" " with %d", block_group, bit, count, err); } else @@ -6194,11 +6188,7 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb, goto error_return; } - if (in_range(ext4_block_bitmap(sb, desc), block, count) || - in_range(ext4_inode_bitmap(sb, desc), block, count) || - in_range(block, ext4_inode_table(sb, desc), sbi->s_itb_per_group) || - in_range(block + count - 1, ext4_inode_table(sb, desc), - sbi->s_itb_per_group)) { + if (!ext4_group_block_valid(sb, block_group, block, count)) { ext4_error(sb, "Adding blocks in system zones - " "Block = %llu, count = %lu", block, count);
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> --- fs/ext4/block_validity.c | 31 +++++++++++++++++++++++++++++++ fs/ext4/ext4.h | 3 +++ fs/ext4/mballoc.c | 16 +++------------- 3 files changed, 37 insertions(+), 13 deletions(-)