Message ID | 20230826155028.4019470-5-shikemeng@huaweicloud.com |
---|---|
State | Superseded |
Headers | show |
Series | cleanups and unit test for mballoc | expand |
Kemeng Shi <shikemeng@huaweicloud.com> writes: > call ext4_mb_mark_context in ext4_mb_mark_diskspace_used to: > 1. remove repeat code to normally update bitmap and group descriptor > on disk. > 2. call ext4_mb_mark_context instead of only setting bits in block bitmap > to fix the bitmap. Function ext4_mb_mark_context will also update > checksum of bitmap and other counter along with the bit change to keep > the cosistent with bit change or block bitmap will be marked corrupted as > checksum of bitmap is in inconsistent state. > > Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> > Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> > --- > fs/ext4/mballoc.c | 86 +++++++++++------------------------------------ > 1 file changed, 20 insertions(+), 66 deletions(-) I was wondering whether checking for !ext4_inode_block_valid() can also be part of ext4_mb_mark_context() by passing EXT4_MB_METABLOCKS_VALID_CHECK flag. But as for this patch. It looks good to me. Please feel free to add - Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> -ritesh > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > index b066ee018cdb..e650eac22237 100644 > --- a/fs/ext4/mballoc.c > +++ b/fs/ext4/mballoc.c > @@ -4084,46 +4084,28 @@ static noinline_for_stack int > ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac, > handle_t *handle, unsigned int reserv_clstrs) > { > - struct buffer_head *bitmap_bh = NULL; > + struct ext4_mark_context mc; > struct ext4_group_desc *gdp; > - struct buffer_head *gdp_bh; > struct ext4_sb_info *sbi; > struct super_block *sb; > ext4_fsblk_t block; > int err, len; > + int flags = 0; > > BUG_ON(ac->ac_status != AC_STATUS_FOUND); > BUG_ON(ac->ac_b_ex.fe_len <= 0); > > sb = ac->ac_sb; > sbi = EXT4_SB(sb); > + ext4_mb_prepare_mark_context(&mc, handle, sb, 1); > > - bitmap_bh = ext4_read_block_bitmap(sb, ac->ac_b_ex.fe_group); > - if (IS_ERR(bitmap_bh)) { > - return PTR_ERR(bitmap_bh); > - } > - > - BUFFER_TRACE(bitmap_bh, "getting write access"); > - err = ext4_journal_get_write_access(handle, sb, bitmap_bh, > - EXT4_JTR_NONE); > - if (err) > - goto out_err; > - > - err = -EIO; > - gdp = ext4_get_group_desc(sb, ac->ac_b_ex.fe_group, &gdp_bh); > + gdp = ext4_get_group_desc(sb, ac->ac_b_ex.fe_group, NULL); > if (!gdp) > - goto out_err; > - > + return -EIO; > ext4_debug("using block group %u(%d)\n", ac->ac_b_ex.fe_group, > ext4_free_group_clusters(sb, gdp)); > > - BUFFER_TRACE(gdp_bh, "get_write_access"); > - err = ext4_journal_get_write_access(handle, sb, gdp_bh, EXT4_JTR_NONE); > - if (err) > - goto out_err; > - > block = ext4_grp_offs_to_block(sb, &ac->ac_b_ex); > - > len = EXT4_C2B(sbi, ac->ac_b_ex.fe_len); > if (!ext4_inode_block_valid(ac->ac_inode, block, len)) { > ext4_error(sb, "Allocating blocks %llu-%llu which overlap " > @@ -4132,41 +4114,28 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac, > * Fix the bitmap and return EFSCORRUPTED > * We leak some of the blocks here. > */ > - ext4_lock_group(sb, ac->ac_b_ex.fe_group); > - mb_set_bits(bitmap_bh->b_data, ac->ac_b_ex.fe_start, > - ac->ac_b_ex.fe_len); > - ext4_unlock_group(sb, ac->ac_b_ex.fe_group); > - err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh); > + err = ext4_mb_mark_context(&mc, ac->ac_b_ex.fe_group, > + ac->ac_b_ex.fe_start, > + ac->ac_b_ex.fe_len, > + 0); > if (!err) > err = -EFSCORRUPTED; > - goto out_err; > + return err; > } > > - ext4_lock_group(sb, ac->ac_b_ex.fe_group); > #ifdef AGGRESSIVE_CHECK > - { > - int i; > - for (i = 0; i < ac->ac_b_ex.fe_len; i++) { > - BUG_ON(mb_test_bit(ac->ac_b_ex.fe_start + i, > - bitmap_bh->b_data)); > - } > - } > + flags |= EXT4_MB_BITMAP_MARKED_CHECK; > #endif > - mb_set_bits(bitmap_bh->b_data, ac->ac_b_ex.fe_start, > - ac->ac_b_ex.fe_len); > - if (ext4_has_group_desc_csum(sb) && > - (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT))) { > - gdp->bg_flags &= cpu_to_le16(~EXT4_BG_BLOCK_UNINIT); > - ext4_free_group_clusters_set(sb, gdp, > - ext4_free_clusters_after_init(sb, > - ac->ac_b_ex.fe_group, gdp)); > - } > - len = ext4_free_group_clusters(sb, gdp) - ac->ac_b_ex.fe_len; > - ext4_free_group_clusters_set(sb, gdp, len); > - ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh); > - ext4_group_desc_csum_set(sb, ac->ac_b_ex.fe_group, gdp); > + err = ext4_mb_mark_context(&mc, ac->ac_b_ex.fe_group, > + ac->ac_b_ex.fe_start, ac->ac_b_ex.fe_len, > + flags); > + > + if (err && mc.changed == 0) > + return err; > > - ext4_unlock_group(sb, ac->ac_b_ex.fe_group); > +#ifdef AGGRESSIVE_CHECK > + BUG_ON(mc.changed != ac->ac_b_ex.fe_len); > +#endif > percpu_counter_sub(&sbi->s_freeclusters_counter, ac->ac_b_ex.fe_len); > /* > * Now reduce the dirty block count also. Should not go negative > @@ -4176,21 +4145,6 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac, > percpu_counter_sub(&sbi->s_dirtyclusters_counter, > reserv_clstrs); > > - if (sbi->s_log_groups_per_flex) { > - ext4_group_t flex_group = ext4_flex_group(sbi, > - ac->ac_b_ex.fe_group); > - atomic64_sub(ac->ac_b_ex.fe_len, > - &sbi_array_rcu_deref(sbi, s_flex_groups, > - flex_group)->free_clusters); > - } > - > - err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh); > - if (err) > - goto out_err; > - err = ext4_handle_dirty_metadata(handle, NULL, gdp_bh); > - > -out_err: > - brelse(bitmap_bh); > return err; > } > > -- > 2.30.0
on 9/1/2023 11:51 AM, Ritesh Harjani wrote: > Kemeng Shi <shikemeng@huaweicloud.com> writes: > >> call ext4_mb_mark_context in ext4_mb_mark_diskspace_used to: >> 1. remove repeat code to normally update bitmap and group descriptor >> on disk. >> 2. call ext4_mb_mark_context instead of only setting bits in block bitmap >> to fix the bitmap. Function ext4_mb_mark_context will also update >> checksum of bitmap and other counter along with the bit change to keep >> the cosistent with bit change or block bitmap will be marked corrupted as >> checksum of bitmap is in inconsistent state. >> >> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> >> Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> >> --- >> fs/ext4/mballoc.c | 86 +++++++++++------------------------------------ >> 1 file changed, 20 insertions(+), 66 deletions(-) > > I was wondering whether checking for !ext4_inode_block_valid() can also > be part of ext4_mb_mark_context() by passing EXT4_MB_METABLOCKS_VALID_CHECK > flag. Looks great to me. Thanks for the suggestion, I will do this in next version. > > But as for this patch. It looks good to me. Please feel free to add - > > Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > > -ritesh > >> >> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c >> index b066ee018cdb..e650eac22237 100644 >> --- a/fs/ext4/mballoc.c >> +++ b/fs/ext4/mballoc.c >> @@ -4084,46 +4084,28 @@ static noinline_for_stack int >> ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac, >> handle_t *handle, unsigned int reserv_clstrs) >> { >> - struct buffer_head *bitmap_bh = NULL; >> + struct ext4_mark_context mc; >> struct ext4_group_desc *gdp; >> - struct buffer_head *gdp_bh; >> struct ext4_sb_info *sbi; >> struct super_block *sb; >> ext4_fsblk_t block; >> int err, len; >> + int flags = 0; >> >> BUG_ON(ac->ac_status != AC_STATUS_FOUND); >> BUG_ON(ac->ac_b_ex.fe_len <= 0); >> >> sb = ac->ac_sb; >> sbi = EXT4_SB(sb); >> + ext4_mb_prepare_mark_context(&mc, handle, sb, 1); >> >> - bitmap_bh = ext4_read_block_bitmap(sb, ac->ac_b_ex.fe_group); >> - if (IS_ERR(bitmap_bh)) { >> - return PTR_ERR(bitmap_bh); >> - } >> - >> - BUFFER_TRACE(bitmap_bh, "getting write access"); >> - err = ext4_journal_get_write_access(handle, sb, bitmap_bh, >> - EXT4_JTR_NONE); >> - if (err) >> - goto out_err; >> - >> - err = -EIO; >> - gdp = ext4_get_group_desc(sb, ac->ac_b_ex.fe_group, &gdp_bh); >> + gdp = ext4_get_group_desc(sb, ac->ac_b_ex.fe_group, NULL); >> if (!gdp) >> - goto out_err; >> - >> + return -EIO; >> ext4_debug("using block group %u(%d)\n", ac->ac_b_ex.fe_group, >> ext4_free_group_clusters(sb, gdp)); >> >> - BUFFER_TRACE(gdp_bh, "get_write_access"); >> - err = ext4_journal_get_write_access(handle, sb, gdp_bh, EXT4_JTR_NONE); >> - if (err) >> - goto out_err; >> - >> block = ext4_grp_offs_to_block(sb, &ac->ac_b_ex); >> - >> len = EXT4_C2B(sbi, ac->ac_b_ex.fe_len); >> if (!ext4_inode_block_valid(ac->ac_inode, block, len)) { >> ext4_error(sb, "Allocating blocks %llu-%llu which overlap " >> @@ -4132,41 +4114,28 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac, >> * Fix the bitmap and return EFSCORRUPTED >> * We leak some of the blocks here. >> */ >> - ext4_lock_group(sb, ac->ac_b_ex.fe_group); >> - mb_set_bits(bitmap_bh->b_data, ac->ac_b_ex.fe_start, >> - ac->ac_b_ex.fe_len); >> - ext4_unlock_group(sb, ac->ac_b_ex.fe_group); >> - err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh); >> + err = ext4_mb_mark_context(&mc, ac->ac_b_ex.fe_group, >> + ac->ac_b_ex.fe_start, >> + ac->ac_b_ex.fe_len, >> + 0); >> if (!err) >> err = -EFSCORRUPTED; >> - goto out_err; >> + return err; >> } >> >> - ext4_lock_group(sb, ac->ac_b_ex.fe_group); >> #ifdef AGGRESSIVE_CHECK >> - { >> - int i; >> - for (i = 0; i < ac->ac_b_ex.fe_len; i++) { >> - BUG_ON(mb_test_bit(ac->ac_b_ex.fe_start + i, >> - bitmap_bh->b_data)); >> - } >> - } >> + flags |= EXT4_MB_BITMAP_MARKED_CHECK; >> #endif >> - mb_set_bits(bitmap_bh->b_data, ac->ac_b_ex.fe_start, >> - ac->ac_b_ex.fe_len); >> - if (ext4_has_group_desc_csum(sb) && >> - (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT))) { >> - gdp->bg_flags &= cpu_to_le16(~EXT4_BG_BLOCK_UNINIT); >> - ext4_free_group_clusters_set(sb, gdp, >> - ext4_free_clusters_after_init(sb, >> - ac->ac_b_ex.fe_group, gdp)); >> - } >> - len = ext4_free_group_clusters(sb, gdp) - ac->ac_b_ex.fe_len; >> - ext4_free_group_clusters_set(sb, gdp, len); >> - ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh); >> - ext4_group_desc_csum_set(sb, ac->ac_b_ex.fe_group, gdp); >> + err = ext4_mb_mark_context(&mc, ac->ac_b_ex.fe_group, >> + ac->ac_b_ex.fe_start, ac->ac_b_ex.fe_len, >> + flags); >> + >> + if (err && mc.changed == 0) >> + return err; >> >> - ext4_unlock_group(sb, ac->ac_b_ex.fe_group); >> +#ifdef AGGRESSIVE_CHECK >> + BUG_ON(mc.changed != ac->ac_b_ex.fe_len); >> +#endif >> percpu_counter_sub(&sbi->s_freeclusters_counter, ac->ac_b_ex.fe_len); >> /* >> * Now reduce the dirty block count also. Should not go negative >> @@ -4176,21 +4145,6 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac, >> percpu_counter_sub(&sbi->s_dirtyclusters_counter, >> reserv_clstrs); >> >> - if (sbi->s_log_groups_per_flex) { >> - ext4_group_t flex_group = ext4_flex_group(sbi, >> - ac->ac_b_ex.fe_group); >> - atomic64_sub(ac->ac_b_ex.fe_len, >> - &sbi_array_rcu_deref(sbi, s_flex_groups, >> - flex_group)->free_clusters); >> - } >> - >> - err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh); >> - if (err) >> - goto out_err; >> - err = ext4_handle_dirty_metadata(handle, NULL, gdp_bh); >> - >> -out_err: >> - brelse(bitmap_bh); >> return err; >> } >> >> -- >> 2.30.0 >
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index b066ee018cdb..e650eac22237 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -4084,46 +4084,28 @@ static noinline_for_stack int ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac, handle_t *handle, unsigned int reserv_clstrs) { - struct buffer_head *bitmap_bh = NULL; + struct ext4_mark_context mc; struct ext4_group_desc *gdp; - struct buffer_head *gdp_bh; struct ext4_sb_info *sbi; struct super_block *sb; ext4_fsblk_t block; int err, len; + int flags = 0; BUG_ON(ac->ac_status != AC_STATUS_FOUND); BUG_ON(ac->ac_b_ex.fe_len <= 0); sb = ac->ac_sb; sbi = EXT4_SB(sb); + ext4_mb_prepare_mark_context(&mc, handle, sb, 1); - bitmap_bh = ext4_read_block_bitmap(sb, ac->ac_b_ex.fe_group); - if (IS_ERR(bitmap_bh)) { - return PTR_ERR(bitmap_bh); - } - - BUFFER_TRACE(bitmap_bh, "getting write access"); - err = ext4_journal_get_write_access(handle, sb, bitmap_bh, - EXT4_JTR_NONE); - if (err) - goto out_err; - - err = -EIO; - gdp = ext4_get_group_desc(sb, ac->ac_b_ex.fe_group, &gdp_bh); + gdp = ext4_get_group_desc(sb, ac->ac_b_ex.fe_group, NULL); if (!gdp) - goto out_err; - + return -EIO; ext4_debug("using block group %u(%d)\n", ac->ac_b_ex.fe_group, ext4_free_group_clusters(sb, gdp)); - BUFFER_TRACE(gdp_bh, "get_write_access"); - err = ext4_journal_get_write_access(handle, sb, gdp_bh, EXT4_JTR_NONE); - if (err) - goto out_err; - block = ext4_grp_offs_to_block(sb, &ac->ac_b_ex); - len = EXT4_C2B(sbi, ac->ac_b_ex.fe_len); if (!ext4_inode_block_valid(ac->ac_inode, block, len)) { ext4_error(sb, "Allocating blocks %llu-%llu which overlap " @@ -4132,41 +4114,28 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac, * Fix the bitmap and return EFSCORRUPTED * We leak some of the blocks here. */ - ext4_lock_group(sb, ac->ac_b_ex.fe_group); - mb_set_bits(bitmap_bh->b_data, ac->ac_b_ex.fe_start, - ac->ac_b_ex.fe_len); - ext4_unlock_group(sb, ac->ac_b_ex.fe_group); - err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh); + err = ext4_mb_mark_context(&mc, ac->ac_b_ex.fe_group, + ac->ac_b_ex.fe_start, + ac->ac_b_ex.fe_len, + 0); if (!err) err = -EFSCORRUPTED; - goto out_err; + return err; } - ext4_lock_group(sb, ac->ac_b_ex.fe_group); #ifdef AGGRESSIVE_CHECK - { - int i; - for (i = 0; i < ac->ac_b_ex.fe_len; i++) { - BUG_ON(mb_test_bit(ac->ac_b_ex.fe_start + i, - bitmap_bh->b_data)); - } - } + flags |= EXT4_MB_BITMAP_MARKED_CHECK; #endif - mb_set_bits(bitmap_bh->b_data, ac->ac_b_ex.fe_start, - ac->ac_b_ex.fe_len); - if (ext4_has_group_desc_csum(sb) && - (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT))) { - gdp->bg_flags &= cpu_to_le16(~EXT4_BG_BLOCK_UNINIT); - ext4_free_group_clusters_set(sb, gdp, - ext4_free_clusters_after_init(sb, - ac->ac_b_ex.fe_group, gdp)); - } - len = ext4_free_group_clusters(sb, gdp) - ac->ac_b_ex.fe_len; - ext4_free_group_clusters_set(sb, gdp, len); - ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh); - ext4_group_desc_csum_set(sb, ac->ac_b_ex.fe_group, gdp); + err = ext4_mb_mark_context(&mc, ac->ac_b_ex.fe_group, + ac->ac_b_ex.fe_start, ac->ac_b_ex.fe_len, + flags); + + if (err && mc.changed == 0) + return err; - ext4_unlock_group(sb, ac->ac_b_ex.fe_group); +#ifdef AGGRESSIVE_CHECK + BUG_ON(mc.changed != ac->ac_b_ex.fe_len); +#endif percpu_counter_sub(&sbi->s_freeclusters_counter, ac->ac_b_ex.fe_len); /* * Now reduce the dirty block count also. Should not go negative @@ -4176,21 +4145,6 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac, percpu_counter_sub(&sbi->s_dirtyclusters_counter, reserv_clstrs); - if (sbi->s_log_groups_per_flex) { - ext4_group_t flex_group = ext4_flex_group(sbi, - ac->ac_b_ex.fe_group); - atomic64_sub(ac->ac_b_ex.fe_len, - &sbi_array_rcu_deref(sbi, s_flex_groups, - flex_group)->free_clusters); - } - - err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh); - if (err) - goto out_err; - err = ext4_handle_dirty_metadata(handle, NULL, gdp_bh); - -out_err: - brelse(bitmap_bh); return err; }