[PATCH 1/2] ext4: Make mb_optimize_scan option work with set/unset mount cmd

[PATCH 1/2] ext4: Make mb_optimize_scan option work with set/unset mount cmd

[Ojaswin Mujoo]

After moving to the new mount API, mb_optimize_scan mount option
handling was not working as expected due to the parsed value always
being overwritten by default. Refactor and fix this to the expected
behavior described below:

*  mb_optimize_scan=1 - On
*  mb_optimize_scan=0 - Off
*  mb_optimize_scan not passed - On if no. of BGs > threshold else off
*  Remounts retain previous value unless we explicitly pass the option
   with a new value

Reported-by: Ritesh Harjani <riteshh@linux.ibm.com>
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/super.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c5021ca0a28a..cd0547fabd79 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2021,12 +2021,12 @@ static int ext4_set_test_dummy_encryption(struct super_block *sb, char *arg)
 #define EXT4_SPEC_s_commit_interval		(1 << 16)
 #define EXT4_SPEC_s_fc_debug_max_replay		(1 << 17)
 #define EXT4_SPEC_s_sb_block			(1 << 18)
+#define EXT4_SPEC_mb_optimize_scan		(1 << 19)
 
 struct ext4_fs_context {
 	char		*s_qf_names[EXT4_MAXQUOTAS];
 	char		*test_dummy_enc_arg;
 	int		s_jquota_fmt;	/* Format of quota to use */
-	int		mb_optimize_scan;
 #ifdef CONFIG_EXT4_DEBUG
 	int s_fc_debug_max_replay;
 #endif
@@ -2451,12 +2451,17 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
 			ctx_clear_mount_opt(ctx, m->mount_opt);
 		return 0;
 	case Opt_mb_optimize_scan:
-		if (result.int_32 != 0 && result.int_32 != 1) {
+		if (result.int_32 == 1) {
+			ctx_set_mount_opt2(ctx, EXT4_MOUNT2_MB_OPTIMIZE_SCAN);
+			ctx->spec |= EXT4_SPEC_mb_optimize_scan;
+		} else if (result.int_32 == 0) {
+			ctx_clear_mount_opt2(ctx, EXT4_MOUNT2_MB_OPTIMIZE_SCAN);
+			ctx->spec |= EXT4_SPEC_mb_optimize_scan;
+		} else {
 			ext4_msg(NULL, KERN_WARNING,
 				 "mb_optimize_scan should be set to 0 or 1.");
 			return -EINVAL;
 		}
-		ctx->mb_optimize_scan = result.int_32;
 		return 0;
 	}
 
@@ -4369,7 +4374,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 
 	/* Set defaults for the variables that will be set during parsing */
 	ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
-	ctx->mb_optimize_scan = DEFAULT_MB_OPTIMIZE_SCAN;
 
 	sbi->s_inode_readahead_blks = EXT4_DEF_INODE_READAHEAD_BLKS;
 	sbi->s_sectors_written_start =
@@ -5320,12 +5324,12 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	 * turned off by passing "mb_optimize_scan=0". This can also be
 	 * turned on forcefully by passing "mb_optimize_scan=1".
 	 */
-	if (ctx->mb_optimize_scan == 1)
-		set_opt2(sb, MB_OPTIMIZE_SCAN);
-	else if (ctx->mb_optimize_scan == 0)
-		clear_opt2(sb, MB_OPTIMIZE_SCAN);
-	else if (sbi->s_groups_count >= MB_DEFAULT_LINEAR_SCAN_THRESHOLD)
-		set_opt2(sb, MB_OPTIMIZE_SCAN);
+	if (!(ctx->spec & EXT4_SPEC_mb_optimize_scan)) {
+		if (sbi->s_groups_count >= MB_DEFAULT_LINEAR_SCAN_THRESHOLD)
+			set_opt2(sb, MB_OPTIMIZE_SCAN);
+		else
+			clear_opt2(sb, MB_OPTIMIZE_SCAN);
+	}
 
 	err = ext4_mb_init(sb);
 	if (err) {
-- 
2.27.0

[PATCH 2/2] ext4: Make mb_optimize_scan performance mount option work with extents

[Ojaswin Mujoo]

Currently mb_optimize_scan scan feature which improves filesystem
performance heavily (when FS is fragmented), seems to be not working
with files with extents (ext4 by default has files with extents).

This patch fixes that and makes mb_optimize_scan feature work
for files with extents.

Below are some performance numbers obtained when allocating a 10M and 100M
file with and w/o this patch on a filesytem with no 1M contiguous block.

<perf numbers>
===============
Workload: dd if=/dev/urandom of=test conv=fsync bs=1M count=10/100

Time taken
=====================================================
no.     Size   without-patch     with-patch    Diff(%)
1       10M      0m8.401s         0m5.623s     33.06%
2       100M     1m40.465s        1m14.737s    25.6%

<debug stats>
=============
w/o patch:
  mballoc:
    reqs: 17056
    success: 11407
    groups_scanned: 13643
    cr0_stats:
            hits: 37
            groups_considered: 9472
            useless_loops: 36
            bad_suggestions: 0
    cr1_stats:
            hits: 11418
            groups_considered: 908560
            useless_loops: 1894
            bad_suggestions: 0
    cr2_stats:
            hits: 1873
            groups_considered: 6913
            useless_loops: 21
    cr3_stats:
            hits: 21
            groups_considered: 5040
            useless_loops: 21
    extents_scanned: 417364
            goal_hits: 3707
            2^n_hits: 37
            breaks: 1873
            lost: 0
    buddies_generated: 239/240
    buddies_time_used: 651080
    preallocated: 705
    discarded: 478

with patch:
  mballoc:
    reqs: 12768
    success: 11305
    groups_scanned: 12768
    cr0_stats:
            hits: 1
            groups_considered: 18
            useless_loops: 0
            bad_suggestions: 0
    cr1_stats:
            hits: 5829
            groups_considered: 50626
            useless_loops: 0
            bad_suggestions: 0
    cr2_stats:
            hits: 6938
            groups_considered: 580363
            useless_loops: 0
    cr3_stats:
            hits: 0
            groups_considered: 0
            useless_loops: 0
    extents_scanned: 309059
            goal_hits: 0
            2^n_hits: 1
            breaks: 1463
            lost: 0
    buddies_generated: 239/240
    buddies_time_used: 791392
    preallocated: 673
    discarded: 446

Fixes: 196e402 (ext4: improve cr 0 / cr 1 group scanning)
Reported-by: Geetika Moolchandani <Geetika.Moolchandani1@ibm.com>
Reported-by: Nageswara R Sastry <rnsastry@linux.ibm.com>
Suggested-by: Ritesh Harjani <riteshh@linux.ibm.com>
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 67ac95c4cd9b..f9be6ab482a5 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1000,7 +1000,7 @@ static inline int should_optimize_scan(struct ext4_allocation_context *ac)
 		return 0;
 	if (ac->ac_criteria >= 2)
 		return 0;
-	if (ext4_test_inode_flag(ac->ac_inode, EXT4_INODE_EXTENTS))
+	if (!ext4_test_inode_flag(ac->ac_inode, EXT4_INODE_EXTENTS))
 		return 0;
 	return 1;
 }
-- 
2.27.0

Re: [PATCH 1/2] ext4: Make mb_optimize_scan option work with set/unset mount cmd

[Ritesh Harjani]

cc' Lukas too

On 22/03/08 03:22PM, Ojaswin Mujoo wrote:
> After moving to the new mount API, mb_optimize_scan mount option
> handling was not working as expected due to the parsed value always
> being overwritten by default. Refactor and fix this to the expected
> behavior described below:
>
> *  mb_optimize_scan=1 - On
> *  mb_optimize_scan=0 - Off
> *  mb_optimize_scan not passed - On if no. of BGs > threshold else off
> *  Remounts retain previous value unless we explicitly pass the option
>    with a new value

So with new mount API, once we call ctx_set/clear_mount_opt2 with
EXT4_MOUNT2_MB_OPTIMIZE_SCAN, ext4_apply_options() will take care of
setting/clearing it in sbi->s_mount_**

Then with that small nit mentioned below, the patch looks good to me.
Feel free to add after addressing it.

Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>


>
> Reported-by: Ritesh Harjani <riteshh@linux.ibm.com>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
>  fs/ext4/super.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c5021ca0a28a..cd0547fabd79 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2021,12 +2021,12 @@ static int ext4_set_test_dummy_encryption(struct super_block *sb, char *arg)
>  #define EXT4_SPEC_s_commit_interval		(1 << 16)
>  #define EXT4_SPEC_s_fc_debug_max_replay		(1 << 17)
>  #define EXT4_SPEC_s_sb_block			(1 << 18)
> +#define EXT4_SPEC_mb_optimize_scan		(1 << 19)
>
>  struct ext4_fs_context {
>  	char		*s_qf_names[EXT4_MAXQUOTAS];
>  	char		*test_dummy_enc_arg;
>  	int		s_jquota_fmt;	/* Format of quota to use */
> -	int		mb_optimize_scan;
>  #ifdef CONFIG_EXT4_DEBUG
>  	int s_fc_debug_max_replay;
>  #endif
> @@ -2451,12 +2451,17 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  			ctx_clear_mount_opt(ctx, m->mount_opt);
>  		return 0;
>  	case Opt_mb_optimize_scan:
> -		if (result.int_32 != 0 && result.int_32 != 1) {
> +		if (result.int_32 == 1) {
> +			ctx_set_mount_opt2(ctx, EXT4_MOUNT2_MB_OPTIMIZE_SCAN);
> +			ctx->spec |= EXT4_SPEC_mb_optimize_scan;
> +		} else if (result.int_32 == 0) {
> +			ctx_clear_mount_opt2(ctx, EXT4_MOUNT2_MB_OPTIMIZE_SCAN);
> +			ctx->spec |= EXT4_SPEC_mb_optimize_scan;
> +		} else {
>  			ext4_msg(NULL, KERN_WARNING,
>  				 "mb_optimize_scan should be set to 0 or 1.");
>  			return -EINVAL;
>  		}
> -		ctx->mb_optimize_scan = result.int_32;
>  		return 0;
>  	}
>
> @@ -4369,7 +4374,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>
>  	/* Set defaults for the variables that will be set during parsing */
>  	ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> -	ctx->mb_optimize_scan = DEFAULT_MB_OPTIMIZE_SCAN;

So if we are not using this DEFAULT_MB_OPTIMIZE_SCAN macro anywhere else, then
we should just kill it's definition too in the same patch.

>
>  	sbi->s_inode_readahead_blks = EXT4_DEF_INODE_READAHEAD_BLKS;
>  	sbi->s_sectors_written_start =
> @@ -5320,12 +5324,12 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	 * turned off by passing "mb_optimize_scan=0". This can also be
>  	 * turned on forcefully by passing "mb_optimize_scan=1".
>  	 */
> -	if (ctx->mb_optimize_scan == 1)
> -		set_opt2(sb, MB_OPTIMIZE_SCAN);
> -	else if (ctx->mb_optimize_scan == 0)
> -		clear_opt2(sb, MB_OPTIMIZE_SCAN);
> -	else if (sbi->s_groups_count >= MB_DEFAULT_LINEAR_SCAN_THRESHOLD)
> -		set_opt2(sb, MB_OPTIMIZE_SCAN);
> +	if (!(ctx->spec & EXT4_SPEC_mb_optimize_scan)) {
> +		if (sbi->s_groups_count >= MB_DEFAULT_LINEAR_SCAN_THRESHOLD)
> +			set_opt2(sb, MB_OPTIMIZE_SCAN);
> +		else
> +			clear_opt2(sb, MB_OPTIMIZE_SCAN);
> +	}
>
>  	err = ext4_mb_init(sb);
>  	if (err) {
> --
> 2.27.0
>

Re: [PATCH 1/2] ext4: Make mb_optimize_scan option work with set/unset mount cmd

[Theodore Ts'o]

On Tue, 8 Mar 2022 15:22:00 +0530, Ojaswin Mujoo wrote:
> After moving to the new mount API, mb_optimize_scan mount option
> handling was not working as expected due to the parsed value always
> being overwritten by default. Refactor and fix this to the expected
> behavior described below:
> 
> *  mb_optimize_scan=1 - On
> *  mb_optimize_scan=0 - Off
> *  mb_optimize_scan not passed - On if no. of BGs > threshold else off
> *  Remounts retain previous value unless we explicitly pass the option
>    with a new value
> 
> [...]

Applied, thanks!

[1/2] ext4: Make mb_optimize_scan option work with set/unset mount cmd
      commit: 27b38686a3bb601db48901dbc4e2fc5d77ffa2c1
[2/2] ext4: Make mb_optimize_scan performance mount option work with extents
      commit: 077d0c2c78df6f7260cdd015a991327efa44d8ad

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>