[PATCH] ext4: optimize the implementation of ext4_mb_good_group()

[PATCH] ext4: optimize the implementation of ext4_mb_good_group()

[brookxu]

It might be better to adjust the code in two places:
1. Determine whether grp is currupt or not should be placed first.
2. (cr<=2 && free <ac->ac_g_ex.fe_len)should may belong to the crx
   strategy, and it may be more appropriate to put it in the
   subsequent switch statement block. For cr1, cr2, the conditions
   in switch potentially realize the above judgment. For cr0, we
   should add (free <ac->ac_g_ex.fe_len) judgment, and then delete
   (free / fragments) >= ac->ac_g_ex.fe_len), because cr0 returns
   true by default.

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 fs/ext4/mballoc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 28a139f..4304113 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2119,13 +2119,11 @@ static bool ext4_mb_good_group(struct ext4_allocation_context *ac,
 
 	BUG_ON(cr < 0 || cr >= 4);
 
-	free = grp->bb_free;
-	if (free == 0)
-		return false;
-	if (cr <= 2 && free < ac->ac_g_ex.fe_len)
+	if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
 		return false;
 
-	if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
+	free = grp->bb_free;
+	if (free == 0)
 		return false;
 
 	fragments = grp->bb_fragments;
@@ -2142,8 +2140,10 @@ static bool ext4_mb_good_group(struct ext4_allocation_context *ac,
 		    ((group % flex_size) == 0))
 			return false;
 
-		if ((ac->ac_2order > ac->ac_sb->s_blocksize_bits+1) ||
-		    (free / fragments) >= ac->ac_g_ex.fe_len)
+		if (free < ac->ac_g_ex.fe_len)
+			return false;
+
+		if (ac->ac_2order > ac->ac_sb->s_blocksize_bits+1)
 			return true;
 
 		if (grp->bb_largest_free_order < ac->ac_2order)
-- 
1.8.3.1

Re: [PATCH] ext4: optimize the implementation of ext4_mb_good_group()

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

On Aug 7, 2020, at 8:01 AM, brookxu <brookxu.cn@gmail.com> wrote:
> 
> It might be better to adjust the code in two places:
> 1. Determine whether grp is currupt or not should be placed first.
> 2. (cr<=2 && free <ac->ac_g_ex.fe_len)should may belong to the crx
>   strategy, and it may be more appropriate to put it in the
>   subsequent switch statement block. For cr1, cr2, the conditions
>   in switch potentially realize the above judgment. For cr0, we
>   should add (free <ac->ac_g_ex.fe_len) judgment, and then delete
>   (free / fragments) >= ac->ac_g_ex.fe_len), because cr0 returns
>   true by default.
> 

> Signed-off-by: Chunguang Xu <brookxu@tencent.com>

This looks correct.  Not quite as simple as moving a few lines around,
but I think the logic is equivalent.

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> fs/ext4/mballoc.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 28a139f..4304113 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2119,13 +2119,11 @@ static bool ext4_mb_good_group(struct ext4_allocation_context *ac,
> 
> 	BUG_ON(cr < 0 || cr >= 4);

This could also potentially be removed and keep only "BUG()" in the
default: case, though it would be good to print the value of "cr".

> 
> -	free = grp->bb_free;
> -	if (free == 0)
> -		return false;
> -	if (cr <= 2 && free < ac->ac_g_ex.fe_len)
> +	if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
> 		return false;
> 
> -	if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
> +	free = grp->bb_free;
> +	if (free == 0)
> 		return false;
> 
> 	fragments = grp->bb_fragments;
> @@ -2142,8 +2140,10 @@ static bool ext4_mb_good_group(struct ext4_allocation_context *ac,
> 		    ((group % flex_size) == 0))
> 			return false;
> 
> -		if ((ac->ac_2order > ac->ac_sb->s_blocksize_bits+1) ||
> -		    (free / fragments) >= ac->ac_g_ex.fe_len)
> +		if (free < ac->ac_g_ex.fe_len)
> +			return false;
> +
> +		if (ac->ac_2order > ac->ac_sb->s_blocksize_bits+1)
> 			return true;
> 
> 		if (grp->bb_largest_free_order < ac->ac_2order)
> --
> 1.8.3.1
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH] ext4: optimize the implementation of ext4_mb_good_group()

[Ritesh Harjani]

On 8/7/20 7:31 PM, brookxu wrote:
> It might be better to adjust the code in two places:
> 1. Determine whether grp is currupt or not should be placed first.
> 2. (cr<=2 && free <ac->ac_g_ex.fe_len)should may belong to the crx
>     strategy, and it may be more appropriate to put it in the
>     subsequent switch statement block. For cr1, cr2, the conditions
>     in switch potentially realize the above judgment. For cr0, we
>     should add (free <ac->ac_g_ex.fe_len) judgment, and then delete
>     (free / fragments) >= ac->ac_g_ex.fe_len), because cr0 returns
>     true by default.
> 
> Signed-off-by: Chunguang Xu <brookxu@tencent.com>


Nice cleanup. This makes it less confusing :)

Logic looks fine to me.
Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>


> ---
>   fs/ext4/mballoc.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 28a139f..4304113 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2119,13 +2119,11 @@ static bool ext4_mb_good_group(struct ext4_allocation_context *ac,
> 
>   	BUG_ON(cr < 0 || cr >= 4);
> 
> -	free = grp->bb_free;
> -	if (free == 0)
> -		return false;
> -	if (cr <= 2 && free < ac->ac_g_ex.fe_len)
> +	if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
>   		return false;
> 
> -	if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
> +	free = grp->bb_free;
> +	if (free == 0)
>   		return false;
> 
>   	fragments = grp->bb_fragments;
> @@ -2142,8 +2140,10 @@ static bool ext4_mb_good_group(struct ext4_allocation_context *ac,
>   		    ((group % flex_size) == 0))
>   			return false;
> 
> -		if ((ac->ac_2order > ac->ac_sb->s_blocksize_bits+1) ||
> -		    (free / fragments) >= ac->ac_g_ex.fe_len)
> +		if (free < ac->ac_g_ex.fe_len)
> +			return false;
> +
> +		if (ac->ac_2order > ac->ac_sb->s_blocksize_bits+1)
>   			return true;
> 
>   		if (grp->bb_largest_free_order < ac->ac_2order)
> 

Re: [PATCH] ext4: optimize the implementation of ext4_mb_good_group()

[Theodore Y. Ts'o]

On Fri, Aug 07, 2020 at 10:01:39PM +0800, brookxu wrote:
> It might be better to adjust the code in two places:
> 1. Determine whether grp is currupt or not should be placed first.
> 2. (cr<=2 && free <ac->ac_g_ex.fe_len)should may belong to the crx
>    strategy, and it may be more appropriate to put it in the
>    subsequent switch statement block. For cr1, cr2, the conditions
>    in switch potentially realize the above judgment. For cr0, we
>    should add (free <ac->ac_g_ex.fe_len) judgment, and then delete
>    (free / fragments) >= ac->ac_g_ex.fe_len), because cr0 returns
>    true by default.
> 
> Signed-off-by: Chunguang Xu <brookxu@tencent.com>

Thanks, applied.

					- Ted