diff mbox series

[2/2] ext4: clean up group state test macros with predicate functions

Message ID 1545134401-104523-2-git-send-email-yi.zhang@huawei.com
State New
Headers show
Series [1/2] ext4: fix race when setting the bitmap corrupted flag again | expand

Commit Message

Zhang Yi Dec. 18, 2018, noon UTC 
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(-)

Comments

Wang Shilong Dec. 18, 2018, 2:40 p.m. UTC | #1 
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
Andreas Dilger Dec. 18, 2018, 7:51 p.m. UTC | #2 
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
Zhang Yi Dec. 19, 2018, 3:48 a.m. UTC | #3 
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
>     
>     
>
Zhang Yi Dec. 19, 2018, 4:47 a.m. UTC | #4 
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.
Andreas Dilger Dec. 19, 2018, 4:55 a.m. UTC | #5 
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
diff mbox series

Patch

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;