| Message ID | 1527594317-9214-3-git-send-email-wshilong1991@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] ext4: fix race with setting free_inode/clusters_counter | expand |
On May 29, 2018, at 5:45 AM, Wang Shilong <wangshilong1991@gmail.com> wrote: > > From: Wang Shilong <wshilong@ddn.com> > > only call ext4_error() if new corrupted bit set, > this could save us repeated error messages and > unecessary super block write. > > Suggested-by: Andreas Dilger <adilger@dilger.ca> > Signed-off-by: Wang Shilong <wshilong@ddn.com> > --- > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 554bceb..9f88991 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -766,21 +766,24 @@ void __ext4_grp_locked_error(const char *function, > > -void __ext4_mark_group_bitmap_corrupted(struct super_block *sb, > - ext4_group_t group, > - unsigned int flags) > +int __ext4_mark_group_bitmap_corrupted(struct super_block *sb, > + ext4_group_t group, > + unsigned int flags) > { > struct ext4_sb_info *sbi = EXT4_SB(sb); > struct ext4_group_info *grp = ext4_get_group_info(sb, group); > struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL); > int ret; > + int set = 0; > > if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) { > ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, > &grp->bb_state); > - if (!ret) > + if (!ret) { > percpu_counter_sub(&sbi->s_freeclusters_counter, > grp->bb_free); > + set = 1; > + } > } > > if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) { > @@ -792,8 +795,11 @@ void __ext4_mark_group_bitmap_corrupted(struct super_block *sb, > count = ext4_free_inodes_count(sb, gdp); > percpu_counter_sub(&sbi->s_freeinodes_counter, > count); > + set = 1; > } > } > + > + return set; Instead of adding a separate "set" variable, why not initialize "ret = 0" and then return "!ret"? It might be better to rename "ret" to something more useful like "was_set" (which could be done in the original 1/5 patch). Cheers, Andreas
Andreas, > > On May 29, 2018, at 5:45 AM, Wang Shilong <wangshilong1991@gmail.com> wrote: >> >> From: Wang Shilong <wshilong@ddn.com> >> >> only call ext4_error() if new corrupted bit set, >> this could save us repeated error messages and >> unecessary super block write. >> >> Suggested-by: Andreas Dilger <adilger@dilger.ca> >> Signed-off-by: Wang Shilong <wshilong@ddn.com> >> --- >> >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >> index 554bceb..9f88991 100644 >> --- a/fs/ext4/super.c >> +++ b/fs/ext4/super.c >> @@ -766,21 +766,24 @@ void __ext4_grp_locked_error(const char *function, >> >> -void __ext4_mark_group_bitmap_corrupted(struct super_block *sb, >> - ext4_group_t group, >> - unsigned int flags) >> +int __ext4_mark_group_bitmap_corrupted(struct super_block *sb, >> + ext4_group_t group, >> + unsigned int flags) >> { >> struct ext4_sb_info *sbi = EXT4_SB(sb); >> struct ext4_group_info *grp = ext4_get_group_info(sb, group); >> struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL); >> int ret; >> + int set = 0; >> >> if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) { >> ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, >> &grp->bb_state); >> - if (!ret) >> + if (!ret) { >> percpu_counter_sub(&sbi->s_freeclusters_counter, >> grp->bb_free); >> + set = 1; >> + } >> } >> >> if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) { >> @@ -792,8 +795,11 @@ void __ext4_mark_group_bitmap_corrupted(struct super_block *sb, >> count = ext4_free_inodes_count(sb, gdp); >> percpu_counter_sub(&sbi->s_freeinodes_counter, >> count); >> + set = 1; >> } >> } >> + >> + return set; > > Instead of adding a separate "set" variable, why not initialize "ret = 0" > and then return "!ret"? It might be better to rename "ret" to something > more useful like "was_set" (which could be done in the original 1/5 patch). > This doesn’t work, because it is possible that we don’t set any bit for the flags passed in. Or we could change it to: If (flags & (EXT4_GROUP_INFO_IBITMAP_CORRUPT | EXT4_GROUP_INFO_BBITMAP_CORRUPT) && !ret) return 1; else return 0; Instead of something like this, introduced another bool value looks more readable? > Cheers, Andreas > > > > >
On May 29, 2018, at 5:45 AM, Wang Shilong <wangshilong1991@gmail.com> wrote: > > From: Wang Shilong <wshilong@ddn.com> > > only call ext4_error() if new corrupted bit set, > this could save us repeated error messages and > unecessary super block write. > > Suggested-by: Andreas Dilger <adilger@dilger.ca> > Signed-off-by: Wang Shilong <wshilong@ddn.com> Reviewed-by: Andreas Dilger <adilger@dilger.ca> > --- > fs/ext4/ext4.h | 25 ++++++++++++++++--------- > fs/ext4/super.c | 14 ++++++++++---- > 2 files changed, 26 insertions(+), 13 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index ed5b988..26f8c12 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -2530,9 +2530,9 @@ extern int ext4_alloc_flex_bg_array(struct super_block *sb, > ext4_group_t ngroup); > extern const char *ext4_decode_error(struct super_block *sb, int errno, > char nbuf[16]); > -extern void __ext4_mark_group_bitmap_corrupted(struct super_block *sb, > - ext4_group_t block_group, > - unsigned int flags); > +extern int __ext4_mark_group_bitmap_corrupted(struct super_block *sb, > + ext4_group_t block_group, > + unsigned int flags); > > extern __printf(4, 5) > void __ext4_error(struct super_block *, const char *, unsigned int, > @@ -2596,9 +2596,12 @@ void __ext4_grp_locked_error(const char *, unsigned int, > flags, fmt, ##__VA_ARGS__) > #define ext4_mark_group_bitmap_corrupted(sb, group, flags, fmt, ...) \ > do { \ > - __ext4_error(sb, __func__, __LINE__, \ > - fmt, ##__VA_ARGS__); \ > - __ext4_mark_group_bitmap_corrupted(sb, group, flags); \ > + int ret; \ > + ret = __ext4_mark_group_bitmap_corrupted(sb, group, \ > + flags); \ > + if (ret) \ > + __ext4_error(sb, __func__, __LINE__, \ > + fmt, ##__VA_ARGS__); \ > } while (0) > > > @@ -2648,9 +2651,13 @@ void __ext4_grp_locked_error(const char *, unsigned int, > } while (0) > #define ext4_mark_group_bitmap_corrupted(sb, group, flags, fmt, ...) \ > do { \ > - no_printk(fmt, ##__VA_ARGS__); \ > - __ext4_error(sb, "", 0, " "); \ > - __ext4_mark_group_bitmap_corrupted(sb, group, flags); \ > + int ret; \ > + ret = __ext4_mark_group_bitmap_corrupted(sb, group, \ > + flags); \ > + if (ret) { \ > + no_printk(fmt, ##__VA_ARGS__); \ > + __ext4_error(sb, "", 0, " "); \ > + } \ > } while (0) > > #endif > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 554bceb..9f88991 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -766,21 +766,24 @@ void __ext4_grp_locked_error(const char *function, unsigned int line, > return; > } > > -void __ext4_mark_group_bitmap_corrupted(struct super_block *sb, > - ext4_group_t group, > - unsigned int flags) > +int __ext4_mark_group_bitmap_corrupted(struct super_block *sb, > + ext4_group_t group, > + unsigned int flags) > { > struct ext4_sb_info *sbi = EXT4_SB(sb); > struct ext4_group_info *grp = ext4_get_group_info(sb, group); > struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL); > int ret; > + int set = 0; > > if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) { > ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, > &grp->bb_state); > - if (!ret) > + if (!ret) { > percpu_counter_sub(&sbi->s_freeclusters_counter, > grp->bb_free); > + set = 1; > + } > } > > if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) { > @@ -792,8 +795,11 @@ void __ext4_mark_group_bitmap_corrupted(struct super_block *sb, > count = ext4_free_inodes_count(sb, gdp); > percpu_counter_sub(&sbi->s_freeinodes_counter, > count); > + set = 1; > } > } > + > + return set; > } > > void ext4_update_dynamic_rev(struct super_block *sb) > -- > 1.8.3.1 > Cheers, Andreas
It would be better to describe this with something like: ext4: avoid calling ext4_error if the bitmap is already marked bad "optimized" can mean many things, and so it's better to much more precise about what you are changing (and again, why). - Ted
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index ed5b988..26f8c12 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -2530,9 +2530,9 @@ extern int ext4_alloc_flex_bg_array(struct super_block *sb, ext4_group_t ngroup); extern const char *ext4_decode_error(struct super_block *sb, int errno, char nbuf[16]); -extern void __ext4_mark_group_bitmap_corrupted(struct super_block *sb, - ext4_group_t block_group, - unsigned int flags); +extern int __ext4_mark_group_bitmap_corrupted(struct super_block *sb, + ext4_group_t block_group, + unsigned int flags); extern __printf(4, 5) void __ext4_error(struct super_block *, const char *, unsigned int, @@ -2596,9 +2596,12 @@ void __ext4_grp_locked_error(const char *, unsigned int, flags, fmt, ##__VA_ARGS__) #define ext4_mark_group_bitmap_corrupted(sb, group, flags, fmt, ...) \ do { \ - __ext4_error(sb, __func__, __LINE__, \ - fmt, ##__VA_ARGS__); \ - __ext4_mark_group_bitmap_corrupted(sb, group, flags); \ + int ret; \ + ret = __ext4_mark_group_bitmap_corrupted(sb, group, \ + flags); \ + if (ret) \ + __ext4_error(sb, __func__, __LINE__, \ + fmt, ##__VA_ARGS__); \ } while (0) @@ -2648,9 +2651,13 @@ void __ext4_grp_locked_error(const char *, unsigned int, } while (0) #define ext4_mark_group_bitmap_corrupted(sb, group, flags, fmt, ...) \ do { \ - no_printk(fmt, ##__VA_ARGS__); \ - __ext4_error(sb, "", 0, " "); \ - __ext4_mark_group_bitmap_corrupted(sb, group, flags); \ + int ret; \ + ret = __ext4_mark_group_bitmap_corrupted(sb, group, \ + flags); \ + if (ret) { \ + no_printk(fmt, ##__VA_ARGS__); \ + __ext4_error(sb, "", 0, " "); \ + } \ } while (0) #endif diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 554bceb..9f88991 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -766,21 +766,24 @@ void __ext4_grp_locked_error(const char *function, unsigned int line, return; } -void __ext4_mark_group_bitmap_corrupted(struct super_block *sb, - ext4_group_t group, - unsigned int flags) +int __ext4_mark_group_bitmap_corrupted(struct super_block *sb, + ext4_group_t group, + unsigned int flags) { struct ext4_sb_info *sbi = EXT4_SB(sb); struct ext4_group_info *grp = ext4_get_group_info(sb, group); struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL); int ret; + int set = 0; if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) { ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, &grp->bb_state); - if (!ret) + if (!ret) { percpu_counter_sub(&sbi->s_freeclusters_counter, grp->bb_free); + set = 1; + } } if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) { @@ -792,8 +795,11 @@ void __ext4_mark_group_bitmap_corrupted(struct super_block *sb, count = ext4_free_inodes_count(sb, gdp); percpu_counter_sub(&sbi->s_freeinodes_counter, count); + set = 1; } } + + return set; } void ext4_update_dynamic_rev(struct super_block *sb)