* [PATCH 1/2] ext4: fix race when setting the bitmap corrupted flag again
@ 2018-12-18 12:00 zhangyi (F)
2018-12-18 12:00 ` [PATCH 2/2] ext4: clean up group state test macros with predicate functions zhangyi (F)
2018-12-18 14:25 ` [PATCH 1/2] ext4: fix race when setting the bitmap corrupted flag again Wang Shilong
0 siblings, 2 replies; 9+ messages in thread
From: zhangyi (F) @ 2018-12-18 12:00 UTC (permalink / raw)
To: linux-ext4; +Cc: tytso, adilger.kernel, wshilong, yi.zhang, miaoxie
Commit 9af0b3d12577 "ext4: fix race when setting the bitmap corrupted
flag" want to fix race between setting inode/block bitmap corrupted
flag and reducing free group inodes/clusters counter to prevent
multiple frees. But ext4_test_and_set_bit() will invoke
__test_and_set_bit() which is non-atomic, so the race is still there.
Fix this by invoke test_and_set_bit() instead.
Fixes: 9af0b3d12577 ("ext4: fix race when setting the bitmap corrupted flag")
Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
fs/ext4/ext4.h | 6 ++++++
fs/ext4/super.c | 6 ++----
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 3f89d0a..755ba14 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2888,6 +2888,12 @@ struct ext4_group_info {
(test_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, &((grp)->bb_state)))
#define EXT4_MB_GRP_IBITMAP_CORRUPT(grp) \
(test_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, &((grp)->bb_state)))
+#define EXT4_MB_GRP_TEST_AND_SET_BBITMAP_CORRUPT(grp) \
+ (test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, \
+ &((grp)->bb_state)))
+#define EXT4_MB_GRP_TEST_AND_SET_IBITMAP_CORRUPT(grp) \
+ (test_and_set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, \
+ &((grp)->bb_state)))
#define EXT4_MB_GRP_WAS_TRIMMED(grp) \
(test_bit(EXT4_GROUP_INFO_WAS_TRIMMED_BIT, &((grp)->bb_state)))
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 53ff6c2..5b83765 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -798,16 +798,14 @@ void ext4_mark_group_bitmap_corrupted(struct super_block *sb,
int ret;
if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) {
- ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT,
- &grp->bb_state);
+ ret = EXT4_MB_GRP_TEST_AND_SET_BBITMAP_CORRUPT(grp);
if (!ret)
percpu_counter_sub(&sbi->s_freeclusters_counter,
grp->bb_free);
}
if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) {
- ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT,
- &grp->bb_state);
+ ret = EXT4_MB_GRP_TEST_AND_SET_IBITMAP_CORRUPT(grp);
if (!ret && gdp) {
int count;
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/2] ext4: clean up group state test macros with predicate functions 2018-12-18 12:00 [PATCH 1/2] ext4: fix race when setting the bitmap corrupted flag again zhangyi (F) @ 2018-12-18 12:00 ` zhangyi (F) 2018-12-18 14:40 ` Wang Shilong 2018-12-18 19:51 ` Andreas Dilger 2018-12-18 14:25 ` [PATCH 1/2] ext4: fix race when setting the bitmap corrupted flag again Wang Shilong 1 sibling, 2 replies; 9+ messages in thread From: zhangyi (F) @ 2018-12-18 12:00 UTC (permalink / raw) To: linux-ext4; +Cc: tytso, adilger.kernel, wshilong, yi.zhang, miaoxie Create separate predicate functions to test/set/clear/test_and_set bb_state flags in ext4_group_info like features testing, and then replace all old macros and the places where we use EXT4_GROUP_INFO_XXX_BIT directly. Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> --- fs/ext4/balloc.c | 6 +++--- fs/ext4/ext4.h | 45 ++++++++++++++++++++++++++------------------- fs/ext4/ialloc.c | 8 ++++---- fs/ext4/mballoc.c | 35 +++++++++++++++++------------------ fs/ext4/super.c | 4 ++-- 5 files changed, 52 insertions(+), 46 deletions(-) diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c index e5d6ee6..082387a 100644 --- a/fs/ext4/balloc.c +++ b/fs/ext4/balloc.c @@ -364,7 +364,7 @@ static int ext4_validate_block_bitmap(struct super_block *sb, if (buffer_verified(bh)) return 0; - if (EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) + if (ext4_mb_grp_bbitmap_corrupt(grp)) return -EFSCORRUPTED; ext4_lock_group(sb, block_group); @@ -699,7 +699,7 @@ ext4_fsblk_t ext4_count_free_clusters(struct super_block *sb) grp = NULL; if (EXT4_SB(sb)->s_group_info) grp = ext4_get_group_info(sb, i); - if (!grp || !EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) + if (!grp || !ext4_mb_grp_bbitmap_corrupt(grp)) desc_count += ext4_free_group_clusters(sb, gdp); brelse(bitmap_bh); bitmap_bh = ext4_read_block_bitmap(sb, i); @@ -729,7 +729,7 @@ ext4_fsblk_t ext4_count_free_clusters(struct super_block *sb) grp = NULL; if (EXT4_SB(sb)->s_group_info) grp = ext4_get_group_info(sb, i); - if (!grp || !EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) + if (!grp || !ext4_mb_grp_bbitmap_corrupt(grp)) desc_count += ext4_free_group_clusters(sb, gdp); } diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 755ba14..e460b05 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -2882,25 +2882,32 @@ struct ext4_group_info { #define EXT4_GROUP_INFO_IBITMAP_CORRUPT \ (1 << EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT) -#define EXT4_MB_GRP_NEED_INIT(grp) \ - (test_bit(EXT4_GROUP_INFO_NEED_INIT_BIT, &((grp)->bb_state))) -#define EXT4_MB_GRP_BBITMAP_CORRUPT(grp) \ - (test_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, &((grp)->bb_state))) -#define EXT4_MB_GRP_IBITMAP_CORRUPT(grp) \ - (test_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, &((grp)->bb_state))) -#define EXT4_MB_GRP_TEST_AND_SET_BBITMAP_CORRUPT(grp) \ - (test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, \ - &((grp)->bb_state))) -#define EXT4_MB_GRP_TEST_AND_SET_IBITMAP_CORRUPT(grp) \ - (test_and_set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, \ - &((grp)->bb_state))) - -#define EXT4_MB_GRP_WAS_TRIMMED(grp) \ - (test_bit(EXT4_GROUP_INFO_WAS_TRIMMED_BIT, &((grp)->bb_state))) -#define EXT4_MB_GRP_SET_TRIMMED(grp) \ - (set_bit(EXT4_GROUP_INFO_WAS_TRIMMED_BIT, &((grp)->bb_state))) -#define EXT4_MB_GRP_CLEAR_TRIMMED(grp) \ - (clear_bit(EXT4_GROUP_INFO_WAS_TRIMMED_BIT, &((grp)->bb_state))) + +#define EXT4_MB_GROUP_STATE_FUNCS(name, statename) \ +static inline int ext4_mb_grp_##name(struct ext4_group_info *grp) \ +{ \ + return test_bit(EXT4_GROUP_INFO_##statename##_BIT, \ + &(grp->bb_state)); \ +} \ +static inline void ext4_mb_grp_set_##name(struct ext4_group_info *grp) \ +{ \ + set_bit(EXT4_GROUP_INFO_##statename##_BIT, &(grp->bb_state)); \ +} \ +static inline void ext4_mb_grp_clear_##name(struct ext4_group_info *grp)\ +{ \ + clear_bit(EXT4_GROUP_INFO_##statename##_BIT, &(grp->bb_state)); \ +} \ +static inline int ext4_mb_grp_test_and_set_##name(struct ext4_group_info *grp) \ +{ \ + return test_and_set_bit(EXT4_GROUP_INFO_##statename##_BIT, \ + &(grp->bb_state)); \ +} + +EXT4_MB_GROUP_STATE_FUNCS(need_init, NEED_INIT) +EXT4_MB_GROUP_STATE_FUNCS(trimmed, WAS_TRIMMED) +EXT4_MB_GROUP_STATE_FUNCS(bbitmap_corrupt, BBITMAP_CORRUPT) +EXT4_MB_GROUP_STATE_FUNCS(ibitmap_corrupt, IBITMAP_CORRUPT) + #define EXT4_MAX_CONTENTION 8 #define EXT4_CONTENTION_THRESHOLD 2 diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c index 014f6a6..ca86368 100644 --- a/fs/ext4/ialloc.c +++ b/fs/ext4/ialloc.c @@ -86,7 +86,7 @@ static int ext4_validate_inode_bitmap(struct super_block *sb, if (buffer_verified(bh)) return 0; - if (EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) + if (ext4_mb_grp_ibitmap_corrupt(grp)) return -EFSCORRUPTED; ext4_lock_group(sb, block_group); @@ -293,7 +293,7 @@ void ext4_free_inode(handle_t *handle, struct inode *inode) bitmap_bh = NULL; goto error_return; } - if (unlikely(EXT4_MB_GRP_IBITMAP_CORRUPT(grp))) { + if (unlikely(ext4_mb_grp_ibitmap_corrupt(grp))) { fatal = -EFSCORRUPTED; goto error_return; } @@ -898,13 +898,13 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir, grp = ext4_get_group_info(sb, group); /* Skip groups with already-known suspicious inode tables */ - if (EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) + if (ext4_mb_grp_ibitmap_corrupt(grp)) goto next_group; brelse(inode_bitmap_bh); inode_bitmap_bh = ext4_read_inode_bitmap(sb, group); /* Skip groups with suspicious inode tables */ - if (EXT4_MB_GRP_IBITMAP_CORRUPT(grp) || + if (ext4_mb_grp_ibitmap_corrupt(grp) || IS_ERR(inode_bitmap_bh)) { inode_bitmap_bh = NULL; goto next_group; diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index e224808..4151c76 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -622,7 +622,7 @@ static int __mb_check_buddy(struct ext4_buddy *e4b, char *file, MB_CHECK_ASSERT(mb_test_bit(k, buddy2)); } } - MB_CHECK_ASSERT(!EXT4_MB_GRP_NEED_INIT(e4b->bd_info)); + MB_CHECK_ASSERT(!ext4_mb_grp_need_init(e4b->bd_info)); MB_CHECK_ASSERT(e4b->bd_info->bb_fragments == fragments); grp = ext4_get_group_info(sb, e4b->bd_group); @@ -755,7 +755,7 @@ void ext4_mb_generate_buddy(struct super_block *sb, } mb_set_largest_free_order(sb, grp); - clear_bit(EXT4_GROUP_INFO_NEED_INIT_BIT, &(grp->bb_state)); + ext4_mb_grp_clear_need_init(grp); period = get_cycles() - period; spin_lock(&sbi->s_bal_lock); @@ -857,7 +857,7 @@ static int ext4_mb_init_cache(struct page *page, char *incore, gfp_t gfp) * we must skip all initialized uptodate buddies on the page, * which may be currently in use by an allocating task. */ - if (PageUptodate(page) && !EXT4_MB_GRP_NEED_INIT(grinfo)) { + if (PageUptodate(page) && !ext4_mb_grp_need_init(grinfo)) { bh[i] = NULL; continue; } @@ -1050,7 +1050,7 @@ int ext4_mb_init_group(struct super_block *sb, ext4_group_t group, gfp_t gfp) * page accessed. */ ret = ext4_mb_get_buddy_page_lock(sb, group, &e4b, gfp); - if (ret || !EXT4_MB_GRP_NEED_INIT(this_grp)) { + if (ret || !ext4_mb_grp_need_init(this_grp)) { /* * somebody initialized the group * return without doing anything @@ -1122,7 +1122,7 @@ ext4_mb_load_buddy_gfp(struct super_block *sb, ext4_group_t group, e4b->bd_buddy_page = NULL; e4b->bd_bitmap_page = NULL; - if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) { + if (unlikely(ext4_mb_grp_need_init(grp))) { /* * we need full data about the group * to make a good selection @@ -1424,7 +1424,7 @@ static void mb_free_blocks(struct inode *inode, struct ext4_buddy *e4b, BUG_ON(last >= (sb->s_blocksize << 3)); assert_spin_locked(ext4_group_lock_ptr(sb, e4b->bd_group)); /* Don't bother if the block group is corrupt. */ - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(e4b->bd_info))) + if (unlikely(ext4_mb_grp_bbitmap_corrupt(e4b->bd_info))) return; mb_check_buddy(e4b); @@ -1833,7 +1833,7 @@ int ext4_mb_find_by_goal(struct ext4_allocation_context *ac, if (err) return err; - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(e4b->bd_info))) { + if (unlikely(ext4_mb_grp_bbitmap_corrupt(e4b->bd_info))) { ext4_mb_unload_buddy(e4b); return 0; } @@ -2046,11 +2046,11 @@ static int ext4_mb_good_group(struct ext4_allocation_context *ac, if (cr <= 2 && free < ac->ac_g_ex.fe_len) return 0; - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp))) + if (unlikely(ext4_mb_grp_bbitmap_corrupt(grp))) return 0; /* We only do this if the grp has never been initialized */ - if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) { + if (unlikely(ext4_mb_grp_need_init(grp))) { int ret = ext4_mb_init_group(ac->ac_sb, group, GFP_NOFS); if (ret) return ret; @@ -2304,7 +2304,7 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v) grinfo = ext4_get_group_info(sb, group); /* Load the group info in memory only if not already loaded. */ - if (unlikely(EXT4_MB_GRP_NEED_INIT(grinfo))) { + if (unlikely(ext4_mb_grp_need_init(grinfo))) { err = ext4_mb_load_buddy(sb, group, &e4b); if (err) { seq_printf(seq, "#%-5u: I/O error\n", group); @@ -2418,8 +2418,7 @@ int ext4_mb_add_groupinfo(struct super_block *sb, ext4_group_t group, ext4_msg(sb, KERN_ERR, "can't allocate buddy mem"); goto exit_group_info; } - set_bit(EXT4_GROUP_INFO_NEED_INIT_BIT, - &(meta_group_info[i]->bb_state)); + ext4_mb_grp_set_need_init(meta_group_info[i]); /* * initialize bb_free to be able to skip @@ -2807,7 +2806,7 @@ static void ext4_free_data_in_buddy(struct super_block *sb, * is supported and the free blocks will be trimmed online. */ if (!test_opt(sb, DISCARD)) - EXT4_MB_GRP_CLEAR_TRIMMED(db); + ext4_mb_grp_clear_trimmed(db); if (!db->bb_free_root.rb_node) { /* No more items in the per group rb tree @@ -4790,7 +4789,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode, overflow = 0; ext4_get_group_no_and_offset(sb, block, &block_group, &bit); - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT( + if (unlikely(ext4_mb_grp_bbitmap_corrupt( ext4_get_group_info(sb, block_group)))) return; @@ -4896,7 +4895,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode, " with %d", block_group, bit, count, err); } else - EXT4_MB_GRP_CLEAR_TRIMMED(e4b.bd_info); + ext4_mb_grp_clear_trimmed(e4b.bd_info); ext4_lock_group(sb, block_group); mb_clear_bits(bitmap_bh->b_data, bit, count_clusters); @@ -5169,7 +5168,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group, bitmap = e4b.bd_bitmap; ext4_lock_group(sb, group); - if (EXT4_MB_GRP_WAS_TRIMMED(e4b.bd_info) && + if (ext4_mb_grp_trimmed(e4b.bd_info) && minblocks >= atomic_read(&EXT4_SB(sb)->s_last_trim_minblks)) goto out; @@ -5210,7 +5209,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group, if (!ret) { ret = count; - EXT4_MB_GRP_SET_TRIMMED(e4b.bd_info); + ext4_mb_grp_set_trimmed(e4b.bd_info); } out: ext4_unlock_group(sb, group); @@ -5273,7 +5272,7 @@ int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range) for (group = first_group; group <= last_group; group++) { grp = ext4_get_group_info(sb, group); /* We only do this if the grp has never been initialized */ - if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) { + if (unlikely(ext4_mb_grp_need_init(grp))) { ret = ext4_mb_init_group(sb, group, GFP_NOFS); if (ret) break; diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 5b83765..d08bcd7 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -798,14 +798,14 @@ void ext4_mark_group_bitmap_corrupted(struct super_block *sb, int ret; if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) { - ret = EXT4_MB_GRP_TEST_AND_SET_BBITMAP_CORRUPT(grp); + ret = ext4_mb_grp_test_and_set_bbitmap_corrupt(grp); if (!ret) percpu_counter_sub(&sbi->s_freeclusters_counter, grp->bb_free); } if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) { - ret = EXT4_MB_GRP_TEST_AND_SET_IBITMAP_CORRUPT(grp); + ret = ext4_mb_grp_test_and_set_ibitmap_corrupt(grp); if (!ret && gdp) { int count; -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext4: clean up group state test macros with predicate functions 2018-12-18 12:00 ` [PATCH 2/2] ext4: clean up group state test macros with predicate functions zhangyi (F) @ 2018-12-18 14:40 ` Wang Shilong 2018-12-19 3:48 ` zhangyi (F) 2018-12-18 19:51 ` Andreas Dilger 1 sibling, 1 reply; 9+ messages in thread From: Wang Shilong @ 2018-12-18 14:40 UTC (permalink / raw) To: zhangyi (F), linux-ext4@vger.kernel.org Cc: tytso@mit.edu, adilger.kernel@dilger.ca, miaoxie@huawei.com Hi, 在 2018/12/18 下午7:57，“zhangyi (F)”<yi.zhang@huawei.com> 写入: Create separate predicate functions to test/set/clear/test_and_set bb_state flags in ext4_group_info like features testing, and then replace all old macros and the places where we use EXT4_GROUP_INFO_XXX_BIT directly. Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> --- fs/ext4/balloc.c | 6 +++--- fs/ext4/ext4.h | 45 ++++++++++++++++++++++++++------------------- fs/ext4/ialloc.c | 8 ++++---- fs/ext4/mballoc.c | 35 +++++++++++++++++------------------ fs/ext4/super.c | 4 ++-- 5 files changed, 52 insertions(+), 46 deletions(-) Patch Looks fine, but what is obvious benefits of this patch? except Converting function name to low-case letter, and patch introduce more 6 lines than before... Thanks, Shilong diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c index e5d6ee6..082387a 100644 --- a/fs/ext4/balloc.c +++ b/fs/ext4/balloc.c @@ -364,7 +364,7 @@ static int ext4_validate_block_bitmap(struct super_block *sb, if (buffer_verified(bh)) return 0; - if (EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) + if (ext4_mb_grp_bbitmap_corrupt(grp)) return -EFSCORRUPTED; ext4_lock_group(sb, block_group); @@ -699,7 +699,7 @@ ext4_fsblk_t ext4_count_free_clusters(struct super_block *sb) grp = NULL; if (EXT4_SB(sb)->s_group_info) grp = ext4_get_group_info(sb, i); - if (!grp || !EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) + if (!grp || !ext4_mb_grp_bbitmap_corrupt(grp)) desc_count += ext4_free_group_clusters(sb, gdp); brelse(bitmap_bh); bitmap_bh = ext4_read_block_bitmap(sb, i); @@ -729,7 +729,7 @@ ext4_fsblk_t ext4_count_free_clusters(struct super_block *sb) grp = NULL; if (EXT4_SB(sb)->s_group_info) grp = ext4_get_group_info(sb, i); - if (!grp || !EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) + if (!grp || !ext4_mb_grp_bbitmap_corrupt(grp)) desc_count += ext4_free_group_clusters(sb, gdp); } diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 755ba14..e460b05 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -2882,25 +2882,32 @@ struct ext4_group_info { #define EXT4_GROUP_INFO_IBITMAP_CORRUPT \ (1 << EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT) -#define EXT4_MB_GRP_NEED_INIT(grp) \ - (test_bit(EXT4_GROUP_INFO_NEED_INIT_BIT, &((grp)->bb_state))) -#define EXT4_MB_GRP_BBITMAP_CORRUPT(grp) \ - (test_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, &((grp)->bb_state))) -#define EXT4_MB_GRP_IBITMAP_CORRUPT(grp) \ - (test_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, &((grp)->bb_state))) -#define EXT4_MB_GRP_TEST_AND_SET_BBITMAP_CORRUPT(grp) \ - (test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, \ - &((grp)->bb_state))) -#define EXT4_MB_GRP_TEST_AND_SET_IBITMAP_CORRUPT(grp) \ - (test_and_set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, \ - &((grp)->bb_state))) - -#define EXT4_MB_GRP_WAS_TRIMMED(grp) \ - (test_bit(EXT4_GROUP_INFO_WAS_TRIMMED_BIT, &((grp)->bb_state))) -#define EXT4_MB_GRP_SET_TRIMMED(grp) \ - (set_bit(EXT4_GROUP_INFO_WAS_TRIMMED_BIT, &((grp)->bb_state))) -#define EXT4_MB_GRP_CLEAR_TRIMMED(grp) \ - (clear_bit(EXT4_GROUP_INFO_WAS_TRIMMED_BIT, &((grp)->bb_state))) + +#define EXT4_MB_GROUP_STATE_FUNCS(name, statename) \ +static inline int ext4_mb_grp_##name(struct ext4_group_info *grp) \ +{ \ + return test_bit(EXT4_GROUP_INFO_##statename##_BIT, \ + &(grp->bb_state)); \ +} \ +static inline void ext4_mb_grp_set_##name(struct ext4_group_info *grp) \ +{ \ + set_bit(EXT4_GROUP_INFO_##statename##_BIT, &(grp->bb_state)); \ +} \ +static inline void ext4_mb_grp_clear_##name(struct ext4_group_info *grp)\ +{ \ + clear_bit(EXT4_GROUP_INFO_##statename##_BIT, &(grp->bb_state)); \ +} \ +static inline int ext4_mb_grp_test_and_set_##name(struct ext4_group_info *grp) \ +{ \ + return test_and_set_bit(EXT4_GROUP_INFO_##statename##_BIT, \ + &(grp->bb_state)); \ +} + +EXT4_MB_GROUP_STATE_FUNCS(need_init, NEED_INIT) +EXT4_MB_GROUP_STATE_FUNCS(trimmed, WAS_TRIMMED) +EXT4_MB_GROUP_STATE_FUNCS(bbitmap_corrupt, BBITMAP_CORRUPT) +EXT4_MB_GROUP_STATE_FUNCS(ibitmap_corrupt, IBITMAP_CORRUPT) + #define EXT4_MAX_CONTENTION 8 #define EXT4_CONTENTION_THRESHOLD 2 diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c index 014f6a6..ca86368 100644 --- a/fs/ext4/ialloc.c +++ b/fs/ext4/ialloc.c @@ -86,7 +86,7 @@ static int ext4_validate_inode_bitmap(struct super_block *sb, if (buffer_verified(bh)) return 0; - if (EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) + if (ext4_mb_grp_ibitmap_corrupt(grp)) return -EFSCORRUPTED; ext4_lock_group(sb, block_group); @@ -293,7 +293,7 @@ void ext4_free_inode(handle_t *handle, struct inode *inode) bitmap_bh = NULL; goto error_return; } - if (unlikely(EXT4_MB_GRP_IBITMAP_CORRUPT(grp))) { + if (unlikely(ext4_mb_grp_ibitmap_corrupt(grp))) { fatal = -EFSCORRUPTED; goto error_return; } @@ -898,13 +898,13 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir, grp = ext4_get_group_info(sb, group); /* Skip groups with already-known suspicious inode tables */ - if (EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) + if (ext4_mb_grp_ibitmap_corrupt(grp)) goto next_group; brelse(inode_bitmap_bh); inode_bitmap_bh = ext4_read_inode_bitmap(sb, group); /* Skip groups with suspicious inode tables */ - if (EXT4_MB_GRP_IBITMAP_CORRUPT(grp) || + if (ext4_mb_grp_ibitmap_corrupt(grp) || IS_ERR(inode_bitmap_bh)) { inode_bitmap_bh = NULL; goto next_group; diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index e224808..4151c76 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -622,7 +622,7 @@ static int __mb_check_buddy(struct ext4_buddy *e4b, char *file, MB_CHECK_ASSERT(mb_test_bit(k, buddy2)); } } - MB_CHECK_ASSERT(!EXT4_MB_GRP_NEED_INIT(e4b->bd_info)); + MB_CHECK_ASSERT(!ext4_mb_grp_need_init(e4b->bd_info)); MB_CHECK_ASSERT(e4b->bd_info->bb_fragments == fragments); grp = ext4_get_group_info(sb, e4b->bd_group); @@ -755,7 +755,7 @@ void ext4_mb_generate_buddy(struct super_block *sb, } mb_set_largest_free_order(sb, grp); - clear_bit(EXT4_GROUP_INFO_NEED_INIT_BIT, &(grp->bb_state)); + ext4_mb_grp_clear_need_init(grp); period = get_cycles() - period; spin_lock(&sbi->s_bal_lock); @@ -857,7 +857,7 @@ static int ext4_mb_init_cache(struct page *page, char *incore, gfp_t gfp) * we must skip all initialized uptodate buddies on the page, * which may be currently in use by an allocating task. */ - if (PageUptodate(page) && !EXT4_MB_GRP_NEED_INIT(grinfo)) { + if (PageUptodate(page) && !ext4_mb_grp_need_init(grinfo)) { bh[i] = NULL; continue; } @@ -1050,7 +1050,7 @@ int ext4_mb_init_group(struct super_block *sb, ext4_group_t group, gfp_t gfp) * page accessed. */ ret = ext4_mb_get_buddy_page_lock(sb, group, &e4b, gfp); - if (ret || !EXT4_MB_GRP_NEED_INIT(this_grp)) { + if (ret || !ext4_mb_grp_need_init(this_grp)) { /* * somebody initialized the group * return without doing anything @@ -1122,7 +1122,7 @@ ext4_mb_load_buddy_gfp(struct super_block *sb, ext4_group_t group, e4b->bd_buddy_page = NULL; e4b->bd_bitmap_page = NULL; - if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) { + if (unlikely(ext4_mb_grp_need_init(grp))) { /* * we need full data about the group * to make a good selection @@ -1424,7 +1424,7 @@ static void mb_free_blocks(struct inode *inode, struct ext4_buddy *e4b, BUG_ON(last >= (sb->s_blocksize << 3)); assert_spin_locked(ext4_group_lock_ptr(sb, e4b->bd_group)); /* Don't bother if the block group is corrupt. */ - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(e4b->bd_info))) + if (unlikely(ext4_mb_grp_bbitmap_corrupt(e4b->bd_info))) return; mb_check_buddy(e4b); @@ -1833,7 +1833,7 @@ int ext4_mb_find_by_goal(struct ext4_allocation_context *ac, if (err) return err; - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(e4b->bd_info))) { + if (unlikely(ext4_mb_grp_bbitmap_corrupt(e4b->bd_info))) { ext4_mb_unload_buddy(e4b); return 0; } @@ -2046,11 +2046,11 @@ static int ext4_mb_good_group(struct ext4_allocation_context *ac, if (cr <= 2 && free < ac->ac_g_ex.fe_len) return 0; - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp))) + if (unlikely(ext4_mb_grp_bbitmap_corrupt(grp))) return 0; /* We only do this if the grp has never been initialized */ - if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) { + if (unlikely(ext4_mb_grp_need_init(grp))) { int ret = ext4_mb_init_group(ac->ac_sb, group, GFP_NOFS); if (ret) return ret; @@ -2304,7 +2304,7 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v) grinfo = ext4_get_group_info(sb, group); /* Load the group info in memory only if not already loaded. */ - if (unlikely(EXT4_MB_GRP_NEED_INIT(grinfo))) { + if (unlikely(ext4_mb_grp_need_init(grinfo))) { err = ext4_mb_load_buddy(sb, group, &e4b); if (err) { seq_printf(seq, "#%-5u: I/O error\n", group); @@ -2418,8 +2418,7 @@ int ext4_mb_add_groupinfo(struct super_block *sb, ext4_group_t group, ext4_msg(sb, KERN_ERR, "can't allocate buddy mem"); goto exit_group_info; } - set_bit(EXT4_GROUP_INFO_NEED_INIT_BIT, - &(meta_group_info[i]->bb_state)); + ext4_mb_grp_set_need_init(meta_group_info[i]); /* * initialize bb_free to be able to skip @@ -2807,7 +2806,7 @@ static void ext4_free_data_in_buddy(struct super_block *sb, * is supported and the free blocks will be trimmed online. */ if (!test_opt(sb, DISCARD)) - EXT4_MB_GRP_CLEAR_TRIMMED(db); + ext4_mb_grp_clear_trimmed(db); if (!db->bb_free_root.rb_node) { /* No more items in the per group rb tree @@ -4790,7 +4789,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode, overflow = 0; ext4_get_group_no_and_offset(sb, block, &block_group, &bit); - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT( + if (unlikely(ext4_mb_grp_bbitmap_corrupt( ext4_get_group_info(sb, block_group)))) return; @@ -4896,7 +4895,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode, " with %d", block_group, bit, count, err); } else - EXT4_MB_GRP_CLEAR_TRIMMED(e4b.bd_info); + ext4_mb_grp_clear_trimmed(e4b.bd_info); ext4_lock_group(sb, block_group); mb_clear_bits(bitmap_bh->b_data, bit, count_clusters); @@ -5169,7 +5168,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group, bitmap = e4b.bd_bitmap; ext4_lock_group(sb, group); - if (EXT4_MB_GRP_WAS_TRIMMED(e4b.bd_info) && + if (ext4_mb_grp_trimmed(e4b.bd_info) && minblocks >= atomic_read(&EXT4_SB(sb)->s_last_trim_minblks)) goto out; @@ -5210,7 +5209,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group, if (!ret) { ret = count; - EXT4_MB_GRP_SET_TRIMMED(e4b.bd_info); + ext4_mb_grp_set_trimmed(e4b.bd_info); } out: ext4_unlock_group(sb, group); @@ -5273,7 +5272,7 @@ int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range) for (group = first_group; group <= last_group; group++) { grp = ext4_get_group_info(sb, group); /* We only do this if the grp has never been initialized */ - if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) { + if (unlikely(ext4_mb_grp_need_init(grp))) { ret = ext4_mb_init_group(sb, group, GFP_NOFS); if (ret) break; diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 5b83765..d08bcd7 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -798,14 +798,14 @@ void ext4_mark_group_bitmap_corrupted(struct super_block *sb, int ret; if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) { - ret = EXT4_MB_GRP_TEST_AND_SET_BBITMAP_CORRUPT(grp); + ret = ext4_mb_grp_test_and_set_bbitmap_corrupt(grp); if (!ret) percpu_counter_sub(&sbi->s_freeclusters_counter, grp->bb_free); } if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) { - ret = EXT4_MB_GRP_TEST_AND_SET_IBITMAP_CORRUPT(grp); + ret = ext4_mb_grp_test_and_set_ibitmap_corrupt(grp); if (!ret && gdp) { int count; -- 2.7.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext4: clean up group state test macros with predicate functions 2018-12-18 14:40 ` Wang Shilong @ 2018-12-19 3:48 ` zhangyi (F) 0 siblings, 0 replies; 9+ messages in thread From: zhangyi (F) @ 2018-12-19 3:48 UTC (permalink / raw) To: Wang Shilong, linux-ext4@vger.kernel.org Cc: tytso@mit.edu, adilger.kernel@dilger.ca, miaoxie@huawei.com On 2018/12/18 22:40, Wang Shilong Wrote: > Hi, > > 在 2018/12/18 下午7:57，“zhangyi (F)”<yi.zhang@huawei.com> 写入: > > Create separate predicate functions to test/set/clear/test_and_set > bb_state flags in ext4_group_info like features testing, and then > replace all old macros and the places where we use > EXT4_GROUP_INFO_XXX_BIT directly. > > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> > --- > fs/ext4/balloc.c | 6 +++--- > fs/ext4/ext4.h | 45 ++++++++++++++++++++++++++------------------- > fs/ext4/ialloc.c | 8 ++++---- > fs/ext4/mballoc.c | 35 +++++++++++++++++------------------ > fs/ext4/super.c | 4 ++-- > 5 files changed, 52 insertions(+), 46 deletions(-) > > Patch Looks fine, but what is obvious benefits of this patch? except > Converting function name to low-case letter, and patch introduce more 6 lines than before... > This patch is not just converting function name to low-case letter, it use macro function instead. I checked all places where testing/setting/clearing the bb_state flags, some of them use EXT4_MB_GRP_XXX() macros but others invoke [clear|set|test]_bit() with EXT4_GROUP_INFO_XXX_BIT directly, It's better to unify them for easy access. At the same time, the ext4 features and the ext4_inode_info bit flags were also changed by macro functions, so unify the coding style as well. Thanks, Yi. > Thanks, > Shilong > > diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c > index e5d6ee6..082387a 100644 > --- a/fs/ext4/balloc.c > +++ b/fs/ext4/balloc.c > @@ -364,7 +364,7 @@ static int ext4_validate_block_bitmap(struct super_block *sb, > > if (buffer_verified(bh)) > return 0; > - if (EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) > + if (ext4_mb_grp_bbitmap_corrupt(grp)) > return -EFSCORRUPTED; > > ext4_lock_group(sb, block_group); > @@ -699,7 +699,7 @@ ext4_fsblk_t ext4_count_free_clusters(struct super_block *sb) > grp = NULL; > if (EXT4_SB(sb)->s_group_info) > grp = ext4_get_group_info(sb, i); > - if (!grp || !EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) > + if (!grp || !ext4_mb_grp_bbitmap_corrupt(grp)) > desc_count += ext4_free_group_clusters(sb, gdp); > brelse(bitmap_bh); > bitmap_bh = ext4_read_block_bitmap(sb, i); > @@ -729,7 +729,7 @@ ext4_fsblk_t ext4_count_free_clusters(struct super_block *sb) > grp = NULL; > if (EXT4_SB(sb)->s_group_info) > grp = ext4_get_group_info(sb, i); > - if (!grp || !EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) > + if (!grp || !ext4_mb_grp_bbitmap_corrupt(grp)) > desc_count += ext4_free_group_clusters(sb, gdp); > } > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 755ba14..e460b05 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -2882,25 +2882,32 @@ struct ext4_group_info { > #define EXT4_GROUP_INFO_IBITMAP_CORRUPT \ > (1 << EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT) > > -#define EXT4_MB_GRP_NEED_INIT(grp) \ > - (test_bit(EXT4_GROUP_INFO_NEED_INIT_BIT, &((grp)->bb_state))) > -#define EXT4_MB_GRP_BBITMAP_CORRUPT(grp) \ > - (test_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, &((grp)->bb_state))) > -#define EXT4_MB_GRP_IBITMAP_CORRUPT(grp) \ > - (test_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, &((grp)->bb_state))) > -#define EXT4_MB_GRP_TEST_AND_SET_BBITMAP_CORRUPT(grp) \ > - (test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, \ > - &((grp)->bb_state))) > -#define EXT4_MB_GRP_TEST_AND_SET_IBITMAP_CORRUPT(grp) \ > - (test_and_set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, \ > - &((grp)->bb_state))) > - > -#define EXT4_MB_GRP_WAS_TRIMMED(grp) \ > - (test_bit(EXT4_GROUP_INFO_WAS_TRIMMED_BIT, &((grp)->bb_state))) > -#define EXT4_MB_GRP_SET_TRIMMED(grp) \ > - (set_bit(EXT4_GROUP_INFO_WAS_TRIMMED_BIT, &((grp)->bb_state))) > -#define EXT4_MB_GRP_CLEAR_TRIMMED(grp) \ > - (clear_bit(EXT4_GROUP_INFO_WAS_TRIMMED_BIT, &((grp)->bb_state))) > + > +#define EXT4_MB_GROUP_STATE_FUNCS(name, statename) \ > +static inline int ext4_mb_grp_##name(struct ext4_group_info *grp) \ > +{ \ > + return test_bit(EXT4_GROUP_INFO_##statename##_BIT, \ > + &(grp->bb_state)); \ > +} \ > +static inline void ext4_mb_grp_set_##name(struct ext4_group_info *grp) \ > +{ \ > + set_bit(EXT4_GROUP_INFO_##statename##_BIT, &(grp->bb_state)); \ > +} \ > +static inline void ext4_mb_grp_clear_##name(struct ext4_group_info *grp)\ > +{ \ > + clear_bit(EXT4_GROUP_INFO_##statename##_BIT, &(grp->bb_state)); \ > +} \ > +static inline int ext4_mb_grp_test_and_set_##name(struct ext4_group_info *grp) \ > +{ \ > + return test_and_set_bit(EXT4_GROUP_INFO_##statename##_BIT, \ > + &(grp->bb_state)); \ > +} > + > +EXT4_MB_GROUP_STATE_FUNCS(need_init, NEED_INIT) > +EXT4_MB_GROUP_STATE_FUNCS(trimmed, WAS_TRIMMED) > +EXT4_MB_GROUP_STATE_FUNCS(bbitmap_corrupt, BBITMAP_CORRUPT) > +EXT4_MB_GROUP_STATE_FUNCS(ibitmap_corrupt, IBITMAP_CORRUPT) > + > > #define EXT4_MAX_CONTENTION 8 > #define EXT4_CONTENTION_THRESHOLD 2 > diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c > index 014f6a6..ca86368 100644 > --- a/fs/ext4/ialloc.c > +++ b/fs/ext4/ialloc.c > @@ -86,7 +86,7 @@ static int ext4_validate_inode_bitmap(struct super_block *sb, > > if (buffer_verified(bh)) > return 0; > - if (EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) > + if (ext4_mb_grp_ibitmap_corrupt(grp)) > return -EFSCORRUPTED; > > ext4_lock_group(sb, block_group); > @@ -293,7 +293,7 @@ void ext4_free_inode(handle_t *handle, struct inode *inode) > bitmap_bh = NULL; > goto error_return; > } > - if (unlikely(EXT4_MB_GRP_IBITMAP_CORRUPT(grp))) { > + if (unlikely(ext4_mb_grp_ibitmap_corrupt(grp))) { > fatal = -EFSCORRUPTED; > goto error_return; > } > @@ -898,13 +898,13 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir, > > grp = ext4_get_group_info(sb, group); > /* Skip groups with already-known suspicious inode tables */ > - if (EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) > + if (ext4_mb_grp_ibitmap_corrupt(grp)) > goto next_group; > > brelse(inode_bitmap_bh); > inode_bitmap_bh = ext4_read_inode_bitmap(sb, group); > /* Skip groups with suspicious inode tables */ > - if (EXT4_MB_GRP_IBITMAP_CORRUPT(grp) || > + if (ext4_mb_grp_ibitmap_corrupt(grp) || > IS_ERR(inode_bitmap_bh)) { > inode_bitmap_bh = NULL; > goto next_group; > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > index e224808..4151c76 100644 > --- a/fs/ext4/mballoc.c > +++ b/fs/ext4/mballoc.c > @@ -622,7 +622,7 @@ static int __mb_check_buddy(struct ext4_buddy *e4b, char *file, > MB_CHECK_ASSERT(mb_test_bit(k, buddy2)); > } > } > - MB_CHECK_ASSERT(!EXT4_MB_GRP_NEED_INIT(e4b->bd_info)); > + MB_CHECK_ASSERT(!ext4_mb_grp_need_init(e4b->bd_info)); > MB_CHECK_ASSERT(e4b->bd_info->bb_fragments == fragments); > > grp = ext4_get_group_info(sb, e4b->bd_group); > @@ -755,7 +755,7 @@ void ext4_mb_generate_buddy(struct super_block *sb, > } > mb_set_largest_free_order(sb, grp); > > - clear_bit(EXT4_GROUP_INFO_NEED_INIT_BIT, &(grp->bb_state)); > + ext4_mb_grp_clear_need_init(grp); > > period = get_cycles() - period; > spin_lock(&sbi->s_bal_lock); > @@ -857,7 +857,7 @@ static int ext4_mb_init_cache(struct page *page, char *incore, gfp_t gfp) > * we must skip all initialized uptodate buddies on the page, > * which may be currently in use by an allocating task. > */ > - if (PageUptodate(page) && !EXT4_MB_GRP_NEED_INIT(grinfo)) { > + if (PageUptodate(page) && !ext4_mb_grp_need_init(grinfo)) { > bh[i] = NULL; > continue; > } > @@ -1050,7 +1050,7 @@ int ext4_mb_init_group(struct super_block *sb, ext4_group_t group, gfp_t gfp) > * page accessed. > */ > ret = ext4_mb_get_buddy_page_lock(sb, group, &e4b, gfp); > - if (ret || !EXT4_MB_GRP_NEED_INIT(this_grp)) { > + if (ret || !ext4_mb_grp_need_init(this_grp)) { > /* > * somebody initialized the group > * return without doing anything > @@ -1122,7 +1122,7 @@ ext4_mb_load_buddy_gfp(struct super_block *sb, ext4_group_t group, > e4b->bd_buddy_page = NULL; > e4b->bd_bitmap_page = NULL; > > - if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) { > + if (unlikely(ext4_mb_grp_need_init(grp))) { > /* > * we need full data about the group > * to make a good selection > @@ -1424,7 +1424,7 @@ static void mb_free_blocks(struct inode *inode, struct ext4_buddy *e4b, > BUG_ON(last >= (sb->s_blocksize << 3)); > assert_spin_locked(ext4_group_lock_ptr(sb, e4b->bd_group)); > /* Don't bother if the block group is corrupt. */ > - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(e4b->bd_info))) > + if (unlikely(ext4_mb_grp_bbitmap_corrupt(e4b->bd_info))) > return; > > mb_check_buddy(e4b); > @@ -1833,7 +1833,7 @@ int ext4_mb_find_by_goal(struct ext4_allocation_context *ac, > if (err) > return err; > > - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(e4b->bd_info))) { > + if (unlikely(ext4_mb_grp_bbitmap_corrupt(e4b->bd_info))) { > ext4_mb_unload_buddy(e4b); > return 0; > } > @@ -2046,11 +2046,11 @@ static int ext4_mb_good_group(struct ext4_allocation_context *ac, > if (cr <= 2 && free < ac->ac_g_ex.fe_len) > return 0; > > - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp))) > + if (unlikely(ext4_mb_grp_bbitmap_corrupt(grp))) > return 0; > > /* We only do this if the grp has never been initialized */ > - if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) { > + if (unlikely(ext4_mb_grp_need_init(grp))) { > int ret = ext4_mb_init_group(ac->ac_sb, group, GFP_NOFS); > if (ret) > return ret; > @@ -2304,7 +2304,7 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v) > > grinfo = ext4_get_group_info(sb, group); > /* Load the group info in memory only if not already loaded. */ > - if (unlikely(EXT4_MB_GRP_NEED_INIT(grinfo))) { > + if (unlikely(ext4_mb_grp_need_init(grinfo))) { > err = ext4_mb_load_buddy(sb, group, &e4b); > if (err) { > seq_printf(seq, "#%-5u: I/O error\n", group); > @@ -2418,8 +2418,7 @@ int ext4_mb_add_groupinfo(struct super_block *sb, ext4_group_t group, > ext4_msg(sb, KERN_ERR, "can't allocate buddy mem"); > goto exit_group_info; > } > - set_bit(EXT4_GROUP_INFO_NEED_INIT_BIT, > - &(meta_group_info[i]->bb_state)); > + ext4_mb_grp_set_need_init(meta_group_info[i]); > > /* > * initialize bb_free to be able to skip > @@ -2807,7 +2806,7 @@ static void ext4_free_data_in_buddy(struct super_block *sb, > * is supported and the free blocks will be trimmed online. > */ > if (!test_opt(sb, DISCARD)) > - EXT4_MB_GRP_CLEAR_TRIMMED(db); > + ext4_mb_grp_clear_trimmed(db); > > if (!db->bb_free_root.rb_node) { > /* No more items in the per group rb tree > @@ -4790,7 +4789,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode, > overflow = 0; > ext4_get_group_no_and_offset(sb, block, &block_group, &bit); > > - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT( > + if (unlikely(ext4_mb_grp_bbitmap_corrupt( > ext4_get_group_info(sb, block_group)))) > return; > > @@ -4896,7 +4895,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode, > " with %d", block_group, bit, count, > err); > } else > - EXT4_MB_GRP_CLEAR_TRIMMED(e4b.bd_info); > + ext4_mb_grp_clear_trimmed(e4b.bd_info); > > ext4_lock_group(sb, block_group); > mb_clear_bits(bitmap_bh->b_data, bit, count_clusters); > @@ -5169,7 +5168,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group, > bitmap = e4b.bd_bitmap; > > ext4_lock_group(sb, group); > - if (EXT4_MB_GRP_WAS_TRIMMED(e4b.bd_info) && > + if (ext4_mb_grp_trimmed(e4b.bd_info) && > minblocks >= atomic_read(&EXT4_SB(sb)->s_last_trim_minblks)) > goto out; > > @@ -5210,7 +5209,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group, > > if (!ret) { > ret = count; > - EXT4_MB_GRP_SET_TRIMMED(e4b.bd_info); > + ext4_mb_grp_set_trimmed(e4b.bd_info); > } > out: > ext4_unlock_group(sb, group); > @@ -5273,7 +5272,7 @@ int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range) > for (group = first_group; group <= last_group; group++) { > grp = ext4_get_group_info(sb, group); > /* We only do this if the grp has never been initialized */ > - if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) { > + if (unlikely(ext4_mb_grp_need_init(grp))) { > ret = ext4_mb_init_group(sb, group, GFP_NOFS); > if (ret) > break; > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 5b83765..d08bcd7 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -798,14 +798,14 @@ void ext4_mark_group_bitmap_corrupted(struct super_block *sb, > int ret; > > if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) { > - ret = EXT4_MB_GRP_TEST_AND_SET_BBITMAP_CORRUPT(grp); > + ret = ext4_mb_grp_test_and_set_bbitmap_corrupt(grp); > if (!ret) > percpu_counter_sub(&sbi->s_freeclusters_counter, > grp->bb_free); > } > > if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) { > - ret = EXT4_MB_GRP_TEST_AND_SET_IBITMAP_CORRUPT(grp); > + ret = ext4_mb_grp_test_and_set_ibitmap_corrupt(grp); > if (!ret && gdp) { > int count; > > -- > 2.7.4 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext4: clean up group state test macros with predicate functions 2018-12-18 12:00 ` [PATCH 2/2] ext4: clean up group state test macros with predicate functions zhangyi (F) 2018-12-18 14:40 ` Wang Shilong @ 2018-12-18 19:51 ` Andreas Dilger 2018-12-19 4:47 ` zhangyi (F) 1 sibling, 1 reply; 9+ messages in thread From: Andreas Dilger @ 2018-12-18 19:51 UTC (permalink / raw) To: zhangyi (F) Cc: Ext4 Developers List, Theodore Ts'o, Wang Shilong, Miao Xie [-- Attachment #1: Type: text/plain, Size: 11042 bytes --] On Dec 18, 2018, at 5:00 AM, zhangyi (F) <yi.zhang@huawei.com> wrote: > > Create separate predicate functions to test/set/clear/test_and_set > bb_state flags in ext4_group_info like features testing, and then > replace all old macros and the places where we use > EXT4_GROUP_INFO_XXX_BIT directly. > > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> > --- > +#define EXT4_MB_GROUP_STATE_FUNCS(name, statename) \ > +static inline int ext4_mb_grp_##name(struct ext4_group_info *grp) \ > +{ \ > + return test_bit(EXT4_GROUP_INFO_##statename##_BIT, \ > + &(grp->bb_state)); \ > +} \ > +static inline void ext4_mb_grp_set_##name(struct ext4_group_info *grp) \ > +{ \ > + set_bit(EXT4_GROUP_INFO_##statename##_BIT, &(grp->bb_state)); \ > +} \ > +static inline void ext4_mb_grp_clear_##name(struct ext4_group_info *grp)\ > +{ \ > + clear_bit(EXT4_GROUP_INFO_##statename##_BIT, &(grp->bb_state)); \ > +} \ > +static inline int ext4_mb_grp_test_and_set_##name(struct ext4_group_info *grp) \ > +{ \ > + return test_and_set_bit(EXT4_GROUP_INFO_##statename##_BIT, \ > + &(grp->bb_state)); \ > +} > + > +EXT4_MB_GROUP_STATE_FUNCS(need_init, NEED_INIT) > +EXT4_MB_GROUP_STATE_FUNCS(trimmed, WAS_TRIMMED) > +EXT4_MB_GROUP_STATE_FUNCS(bbitmap_corrupt, BBITMAP_CORRUPT) > +EXT4_MB_GROUP_STATE_FUNCS(ibitmap_corrupt, IBITMAP_CORRUPT) One problem with macros like this that internally expand to multiple functions is that there is now nowhere in this code where, for example, the declaration of ext4_mb_grp_test_and_set_bbitmap_corrupt() can be found. That makes it hard to understand the code, because tags for this function name will not work, and even a grep through the entire code for this string will not show the function implementation, only users. One would have to search for only the "ext4_mb_grp_test_and_set" part, or "ext4_mb_grp_clear" to find the above macros. If such macros-that-generate-functions are being used, my preference is that at least a comment block is added that spells out the full function names, so that at least a grep will find them, like: /* * These macros implement the following functions: * - ext4_mb_grp_need_init(), ext4_mb_grp_test_and_set_need_init(), * ext4_mb_grp_set_need_init(), ext4_mb_grp_clear_need_init() * - ... * - ... */ Yes, this is a bit cumbersome the rare times a new function is added, but it really makes the code easier to understand in the future, without forcing a cut-and-paste of the body of each function. I don't know how many times I've had to search for commonly-used functions like buffer_uptodate() or buffer_dirty() in the code without being able to find them easily. Cheers, Andreas > diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c > index 014f6a6..ca86368 100644 > --- a/fs/ext4/ialloc.c > +++ b/fs/ext4/ialloc.c > @@ -86,7 +86,7 @@ static int ext4_validate_inode_bitmap(struct super_block *sb, > > if (buffer_verified(bh)) > return 0; > - if (EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) > + if (ext4_mb_grp_ibitmap_corrupt(grp)) > return -EFSCORRUPTED; > > ext4_lock_group(sb, block_group); > @@ -293,7 +293,7 @@ void ext4_free_inode(handle_t *handle, struct inode *inode) > bitmap_bh = NULL; > goto error_return; > } > - if (unlikely(EXT4_MB_GRP_IBITMAP_CORRUPT(grp))) { > + if (unlikely(ext4_mb_grp_ibitmap_corrupt(grp))) { > fatal = -EFSCORRUPTED; > goto error_return; > } > @@ -898,13 +898,13 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir, > > grp = ext4_get_group_info(sb, group); > /* Skip groups with already-known suspicious inode tables */ > - if (EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) > + if (ext4_mb_grp_ibitmap_corrupt(grp)) > goto next_group; > > brelse(inode_bitmap_bh); > inode_bitmap_bh = ext4_read_inode_bitmap(sb, group); > /* Skip groups with suspicious inode tables */ > - if (EXT4_MB_GRP_IBITMAP_CORRUPT(grp) || > + if (ext4_mb_grp_ibitmap_corrupt(grp) || > IS_ERR(inode_bitmap_bh)) { > inode_bitmap_bh = NULL; > goto next_group; > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > index e224808..4151c76 100644 > --- a/fs/ext4/mballoc.c > +++ b/fs/ext4/mballoc.c > @@ -622,7 +622,7 @@ static int __mb_check_buddy(struct ext4_buddy *e4b, char *file, > MB_CHECK_ASSERT(mb_test_bit(k, buddy2)); > } > } > - MB_CHECK_ASSERT(!EXT4_MB_GRP_NEED_INIT(e4b->bd_info)); > + MB_CHECK_ASSERT(!ext4_mb_grp_need_init(e4b->bd_info)); > MB_CHECK_ASSERT(e4b->bd_info->bb_fragments == fragments); > > grp = ext4_get_group_info(sb, e4b->bd_group); > @@ -755,7 +755,7 @@ void ext4_mb_generate_buddy(struct super_block *sb, > } > mb_set_largest_free_order(sb, grp); > > - clear_bit(EXT4_GROUP_INFO_NEED_INIT_BIT, &(grp->bb_state)); > + ext4_mb_grp_clear_need_init(grp); > > period = get_cycles() - period; > spin_lock(&sbi->s_bal_lock); > @@ -857,7 +857,7 @@ static int ext4_mb_init_cache(struct page *page, char *incore, gfp_t gfp) > * we must skip all initialized uptodate buddies on the page, > * which may be currently in use by an allocating task. > */ > - if (PageUptodate(page) && !EXT4_MB_GRP_NEED_INIT(grinfo)) { > + if (PageUptodate(page) && !ext4_mb_grp_need_init(grinfo)) { > bh[i] = NULL; > continue; > } > @@ -1050,7 +1050,7 @@ int ext4_mb_init_group(struct super_block *sb, ext4_group_t group, gfp_t gfp) > * page accessed. > */ > ret = ext4_mb_get_buddy_page_lock(sb, group, &e4b, gfp); > - if (ret || !EXT4_MB_GRP_NEED_INIT(this_grp)) { > + if (ret || !ext4_mb_grp_need_init(this_grp)) { > /* > * somebody initialized the group > * return without doing anything > @@ -1122,7 +1122,7 @@ ext4_mb_load_buddy_gfp(struct super_block *sb, ext4_group_t group, > e4b->bd_buddy_page = NULL; > e4b->bd_bitmap_page = NULL; > > - if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) { > + if (unlikely(ext4_mb_grp_need_init(grp))) { > /* > * we need full data about the group > * to make a good selection > @@ -1424,7 +1424,7 @@ static void mb_free_blocks(struct inode *inode, struct ext4_buddy *e4b, > BUG_ON(last >= (sb->s_blocksize << 3)); > assert_spin_locked(ext4_group_lock_ptr(sb, e4b->bd_group)); > /* Don't bother if the block group is corrupt. */ > - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(e4b->bd_info))) > + if (unlikely(ext4_mb_grp_bbitmap_corrupt(e4b->bd_info))) > return; > > mb_check_buddy(e4b); > @@ -1833,7 +1833,7 @@ int ext4_mb_find_by_goal(struct ext4_allocation_context *ac, > if (err) > return err; > > - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(e4b->bd_info))) { > + if (unlikely(ext4_mb_grp_bbitmap_corrupt(e4b->bd_info))) { > ext4_mb_unload_buddy(e4b); > return 0; > } > @@ -2046,11 +2046,11 @@ static int ext4_mb_good_group(struct ext4_allocation_context *ac, > if (cr <= 2 && free < ac->ac_g_ex.fe_len) > return 0; > > - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp))) > + if (unlikely(ext4_mb_grp_bbitmap_corrupt(grp))) > return 0; > > /* We only do this if the grp has never been initialized */ > - if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) { > + if (unlikely(ext4_mb_grp_need_init(grp))) { > int ret = ext4_mb_init_group(ac->ac_sb, group, GFP_NOFS); > if (ret) > return ret; > @@ -2304,7 +2304,7 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v) > > grinfo = ext4_get_group_info(sb, group); > /* Load the group info in memory only if not already loaded. */ > - if (unlikely(EXT4_MB_GRP_NEED_INIT(grinfo))) { > + if (unlikely(ext4_mb_grp_need_init(grinfo))) { > err = ext4_mb_load_buddy(sb, group, &e4b); > if (err) { > seq_printf(seq, "#%-5u: I/O error\n", group); > @@ -2418,8 +2418,7 @@ int ext4_mb_add_groupinfo(struct super_block *sb, ext4_group_t group, > ext4_msg(sb, KERN_ERR, "can't allocate buddy mem"); > goto exit_group_info; > } > - set_bit(EXT4_GROUP_INFO_NEED_INIT_BIT, > - &(meta_group_info[i]->bb_state)); > + ext4_mb_grp_set_need_init(meta_group_info[i]); > > /* > * initialize bb_free to be able to skip > @@ -2807,7 +2806,7 @@ static void ext4_free_data_in_buddy(struct super_block *sb, > * is supported and the free blocks will be trimmed online. > */ > if (!test_opt(sb, DISCARD)) > - EXT4_MB_GRP_CLEAR_TRIMMED(db); > + ext4_mb_grp_clear_trimmed(db); > > if (!db->bb_free_root.rb_node) { > /* No more items in the per group rb tree > @@ -4790,7 +4789,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode, > overflow = 0; > ext4_get_group_no_and_offset(sb, block, &block_group, &bit); > > - if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT( > + if (unlikely(ext4_mb_grp_bbitmap_corrupt( > ext4_get_group_info(sb, block_group)))) > return; > > @@ -4896,7 +4895,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode, > " with %d", block_group, bit, count, > err); > } else > - EXT4_MB_GRP_CLEAR_TRIMMED(e4b.bd_info); > + ext4_mb_grp_clear_trimmed(e4b.bd_info); > > ext4_lock_group(sb, block_group); > mb_clear_bits(bitmap_bh->b_data, bit, count_clusters); > @@ -5169,7 +5168,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group, > bitmap = e4b.bd_bitmap; > > ext4_lock_group(sb, group); > - if (EXT4_MB_GRP_WAS_TRIMMED(e4b.bd_info) && > + if (ext4_mb_grp_trimmed(e4b.bd_info) && > minblocks >= atomic_read(&EXT4_SB(sb)->s_last_trim_minblks)) > goto out; > > @@ -5210,7 +5209,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group, > > if (!ret) { > ret = count; > - EXT4_MB_GRP_SET_TRIMMED(e4b.bd_info); > + ext4_mb_grp_set_trimmed(e4b.bd_info); > } > out: > ext4_unlock_group(sb, group); > @@ -5273,7 +5272,7 @@ int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range) > for (group = first_group; group <= last_group; group++) { > grp = ext4_get_group_info(sb, group); > /* We only do this if the grp has never been initialized */ > - if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) { > + if (unlikely(ext4_mb_grp_need_init(grp))) { > ret = ext4_mb_init_group(sb, group, GFP_NOFS); > if (ret) > break; > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 5b83765..d08bcd7 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -798,14 +798,14 @@ void ext4_mark_group_bitmap_corrupted(struct super_block *sb, > int ret; > > if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) { > - ret = EXT4_MB_GRP_TEST_AND_SET_BBITMAP_CORRUPT(grp); > + ret = ext4_mb_grp_test_and_set_bbitmap_corrupt(grp); > if (!ret) > percpu_counter_sub(&sbi->s_freeclusters_counter, > grp->bb_free); > } > > if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) { > - ret = EXT4_MB_GRP_TEST_AND_SET_IBITMAP_CORRUPT(grp); > + ret = ext4_mb_grp_test_and_set_ibitmap_corrupt(grp); > if (!ret && gdp) { > int count; > > -- > 2.7.4 > Cheers, Andreas [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 873 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext4: clean up group state test macros with predicate functions 2018-12-18 19:51 ` Andreas Dilger @ 2018-12-19 4:47 ` zhangyi (F) 2018-12-19 4:55 ` Andreas Dilger 0 siblings, 1 reply; 9+ messages in thread From: zhangyi (F) @ 2018-12-19 4:47 UTC (permalink / raw) To: Andreas Dilger Cc: Ext4 Developers List, Theodore Ts'o, Wang Shilong, Miao Xie On 2018/12/19 3:51, Andreas Dilger Wrote: > On Dec 18, 2018, at 5:00 AM, zhangyi (F) <yi.zhang@huawei.com> wrote: >> >> Create separate predicate functions to test/set/clear/test_and_set >> bb_state flags in ext4_group_info like features testing, and then >> replace all old macros and the places where we use >> EXT4_GROUP_INFO_XXX_BIT directly. >> >> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> >> --- >> +#define EXT4_MB_GROUP_STATE_FUNCS(name, statename) \ >> +static inline int ext4_mb_grp_##name(struct ext4_group_info *grp) \ >> +{ \ >> + return test_bit(EXT4_GROUP_INFO_##statename##_BIT, \ >> + &(grp->bb_state)); \ >> +} \ >> +static inline void ext4_mb_grp_set_##name(struct ext4_group_info *grp) \ >> +{ \ >> + set_bit(EXT4_GROUP_INFO_##statename##_BIT, &(grp->bb_state)); \ >> +} \ >> +static inline void ext4_mb_grp_clear_##name(struct ext4_group_info *grp)\ >> +{ \ >> + clear_bit(EXT4_GROUP_INFO_##statename##_BIT, &(grp->bb_state)); \ >> +} \ >> +static inline int ext4_mb_grp_test_and_set_##name(struct ext4_group_info *grp) \ >> +{ \ >> + return test_and_set_bit(EXT4_GROUP_INFO_##statename##_BIT, \ >> + &(grp->bb_state)); \ >> +} >> + >> +EXT4_MB_GROUP_STATE_FUNCS(need_init, NEED_INIT) >> +EXT4_MB_GROUP_STATE_FUNCS(trimmed, WAS_TRIMMED) >> +EXT4_MB_GROUP_STATE_FUNCS(bbitmap_corrupt, BBITMAP_CORRUPT) >> +EXT4_MB_GROUP_STATE_FUNCS(ibitmap_corrupt, IBITMAP_CORRUPT) > > One problem with macros like this that internally expand to multiple > functions is that there is now nowhere in this code where, for example, > the declaration of ext4_mb_grp_test_and_set_bbitmap_corrupt() can be > found. That makes it hard to understand the code, because tags for this > function name will not work, and even a grep through the entire code for > this string will not show the function implementation, only users. One > would have to search for only the "ext4_mb_grp_test_and_set" part, or > "ext4_mb_grp_clear" to find the above macros. > > If such macros-that-generate-functions are being used, my preference is > that at least a comment block is added that spells out the full function > names, so that at least a grep will find them, like: > > /* > * These macros implement the following functions: > * - ext4_mb_grp_need_init(), ext4_mb_grp_test_and_set_need_init(), > * ext4_mb_grp_set_need_init(), ext4_mb_grp_clear_need_init() > * - ... > * - ... > */ > > Yes, this is a bit cumbersome the rare times a new function is added, but > it really makes the code easier to understand in the future, without forcing > a cut-and-paste of the body of each function. I don't know how many times > I've had to search for commonly-used functions like buffer_uptodate() or > buffer_dirty() in the code without being able to find them easily. > Thanks for your comments. Indeed, I also had the same hard time as you said. I am not so sure why we have been using these maco functions for ext4 features and ext4_inode_info bit flags. But I think it's still worth to unify them. I will add the comment block as your suggested and post the second version, BTW, I read the commit 3f61c0cc706 "ext4: add prototypes for macro-generated functions" you posted, it's also a good choice. Thanks, Yi. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext4: clean up group state test macros with predicate functions 2018-12-19 4:47 ` zhangyi (F) @ 2018-12-19 4:55 ` Andreas Dilger 0 siblings, 0 replies; 9+ messages in thread From: Andreas Dilger @ 2018-12-19 4:55 UTC (permalink / raw) To: zhangyi (F) Cc: Ext4 Developers List, Theodore Ts'o, Wang Shilong, Miao Xie [-- Attachment #1: Type: text/plain, Size: 2854 bytes --] On Dec 18, 2018, at 9:47 PM, zhangyi (F) <yi.zhang@huawei.com> wrote: > > On 2018/12/19 3:51, Andreas Dilger Wrote: >> On Dec 18, 2018, at 5:00 AM, zhangyi (F) <yi.zhang@huawei.com> wrote: >>> >>> Create separate predicate functions to test/set/clear/test_and_set >>> bb_state flags in ext4_group_info like features testing, and then >>> replace all old macros and the places where we use >>> EXT4_GROUP_INFO_XXX_BIT directly. >>> >>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> >>> --- >>> >>> +EXT4_MB_GROUP_STATE_FUNCS(need_init, NEED_INIT) >>> +EXT4_MB_GROUP_STATE_FUNCS(trimmed, WAS_TRIMMED) >>> +EXT4_MB_GROUP_STATE_FUNCS(bbitmap_corrupt, BBITMAP_CORRUPT) >>> +EXT4_MB_GROUP_STATE_FUNCS(ibitmap_corrupt, IBITMAP_CORRUPT) >> >> One problem with macros like this that internally expand to multiple >> functions is that there is now nowhere in this code where, for example, >> the declaration of ext4_mb_grp_test_and_set_bbitmap_corrupt() can be >> found. That makes it hard to understand the code, because tags for this >> function name will not work, and even a grep through the entire code for >> this string will not show the function implementation, only users. One >> would have to search for only the "ext4_mb_grp_test_and_set" part, or >> "ext4_mb_grp_clear" to find the above macros. >> >> If such macros-that-generate-functions are being used, my preference is >> that at least a comment block is added that spells out the full function >> names, so that at least a grep will find them, like: >> >> /* >> * These macros implement the following functions: >> * - ext4_mb_grp_need_init(), ext4_mb_grp_test_and_set_need_init(), >> * ext4_mb_grp_set_need_init(), ext4_mb_grp_clear_need_init() >> * - ... >> * - ... >> */ >> >> Yes, this is a bit cumbersome the rare times a new function is added, but >> it really makes the code easier to understand in the future, without forcing >> a cut-and-paste of the body of each function. I don't know how many times >> I've had to search for commonly-used functions like buffer_uptodate() or >> buffer_dirty() in the code without being able to find them easily. >> > > Thanks for your comments. Indeed, I also had the same hard time as you said. > I am not so sure why we have been using these maco functions for ext4 features > and ext4_inode_info bit flags. But I think it's still worth to unify them. > > I will add the comment block as your suggested and post the second version, > BTW, I read the commit 3f61c0cc706 "ext4: add prototypes for macro-generated > functions" you posted, it's also a good choice. Indeed, adding static function prototypes is even better than putting the function names in a comment, since tags/ctags/etags will find them for you. I'd forgotten about that patch. Cheers, Andreas [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 873 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] ext4: fix race when setting the bitmap corrupted flag again 2018-12-18 12:00 [PATCH 1/2] ext4: fix race when setting the bitmap corrupted flag again zhangyi (F) 2018-12-18 12:00 ` [PATCH 2/2] ext4: clean up group state test macros with predicate functions zhangyi (F) @ 2018-12-18 14:25 ` Wang Shilong 2018-12-19 3:35 ` zhangyi (F) 1 sibling, 1 reply; 9+ messages in thread From: Wang Shilong @ 2018-12-18 14:25 UTC (permalink / raw) To: zhangyi (F), linux-ext4@vger.kernel.org Cc: tytso@mit.edu, adilger.kernel@dilger.ca, miaoxie@huawei.com Hi, 在 2018/12/18 下午7:57，“zhangyi (F)”<yi.zhang@huawei.com> 写入: Commit 9af0b3d12577 "ext4: fix race when setting the bitmap corrupted flag" want to fix race between setting inode/block bitmap corrupted flag and reducing free group inodes/clusters counter to prevent multiple frees. But ext4_test_and_set_bit() will invoke __test_and_set_bit() which is non-atomic, so the race is still there. Fix this by invoke test_and_set_bit() instead. Fixes: 9af0b3d12577 ("ext4: fix race when setting the bitmap corrupted flag") Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> Thanks for fixing this!, I was not aware of __test_and_set_bit() is not non-atomic operation before. Reviewed-by: Wang Shilong <wshilong@ddn.com> Btw for a stable process question, commit 9af0b3d12577 had CC tag to Stable kernel, should we add it again here or with 'Fixes' tag, it will be included automatically? Thanks, Shilong --- fs/ext4/ext4.h | 6 ++++++ fs/ext4/super.c | 6 ++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 3f89d0a..755ba14 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -2888,6 +2888,12 @@ struct ext4_group_info { (test_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, &((grp)->bb_state))) #define EXT4_MB_GRP_IBITMAP_CORRUPT(grp) \ (test_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, &((grp)->bb_state))) +#define EXT4_MB_GRP_TEST_AND_SET_BBITMAP_CORRUPT(grp) \ + (test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, \ + &((grp)->bb_state))) +#define EXT4_MB_GRP_TEST_AND_SET_IBITMAP_CORRUPT(grp) \ + (test_and_set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, \ + &((grp)->bb_state))) #define EXT4_MB_GRP_WAS_TRIMMED(grp) \ (test_bit(EXT4_GROUP_INFO_WAS_TRIMMED_BIT, &((grp)->bb_state))) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 53ff6c2..5b83765 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -798,16 +798,14 @@ void ext4_mark_group_bitmap_corrupted(struct super_block *sb, int ret; if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) { - ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, - &grp->bb_state); + ret = EXT4_MB_GRP_TEST_AND_SET_BBITMAP_CORRUPT(grp); if (!ret) percpu_counter_sub(&sbi->s_freeclusters_counter, grp->bb_free); } if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) { - ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, - &grp->bb_state); + ret = EXT4_MB_GRP_TEST_AND_SET_IBITMAP_CORRUPT(grp); if (!ret && gdp) { int count; -- 2.7.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] ext4: fix race when setting the bitmap corrupted flag again 2018-12-18 14:25 ` [PATCH 1/2] ext4: fix race when setting the bitmap corrupted flag again Wang Shilong @ 2018-12-19 3:35 ` zhangyi (F) 0 siblings, 0 replies; 9+ messages in thread From: zhangyi (F) @ 2018-12-19 3:35 UTC (permalink / raw) To: Wang Shilong, linux-ext4@vger.kernel.org Cc: tytso@mit.edu, adilger.kernel@dilger.ca, miaoxie@huawei.com On 2018/12/18 22:25, Wang Shilong Wrote: > Hi, > > 在 2018/12/18 下午7:57，“zhangyi (F)”<yi.zhang@huawei.com> 写入: > > Commit 9af0b3d12577 "ext4: fix race when setting the bitmap corrupted > flag" want to fix race between setting inode/block bitmap corrupted > flag and reducing free group inodes/clusters counter to prevent > multiple frees. But ext4_test_and_set_bit() will invoke > __test_and_set_bit() which is non-atomic, so the race is still there. > Fix this by invoke test_and_set_bit() instead. > > Fixes: 9af0b3d12577 ("ext4: fix race when setting the bitmap corrupted flag") > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> > > Thanks for fixing this!, I was not aware of __test_and_set_bit() is not non-atomic operation before. > > Reviewed-by: Wang Shilong <wshilong@ddn.com> > > Btw for a stable process question, commit 9af0b3d12577 had CC tag to Stable kernel, should we > add it again here or with 'Fixes' tag, it will be included automatically? > Thank you for reminding me of the CC stable tag, I missed this tag and I'm not sure it will be added automatically or not. Anyway, I will add this tag in the next iteration. Thanks, Yi. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-12-19 4:55 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-12-18 12:00 [PATCH 1/2] ext4: fix race when setting the bitmap corrupted flag again zhangyi (F) 2018-12-18 12:00 ` [PATCH 2/2] ext4: clean up group state test macros with predicate functions zhangyi (F) 2018-12-18 14:40 ` Wang Shilong 2018-12-19 3:48 ` zhangyi (F) 2018-12-18 19:51 ` Andreas Dilger 2018-12-19 4:47 ` zhangyi (F) 2018-12-19 4:55 ` Andreas Dilger 2018-12-18 14:25 ` [PATCH 1/2] ext4: fix race when setting the bitmap corrupted flag again Wang Shilong 2018-12-19 3:35 ` zhangyi (F)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).