Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 12 Mar 2022 11:28:18 +0530
From:   Ritesh Harjani <riteshh@linux.ibm.com>
To:     Ojaswin Mujoo <ojaswin@linux.ibm.com>
Cc:     linux-ext4@vger.kernel.org,
        Harshad Shirwadkar <harshadshirwadkar@gmail.com>,
        "Theodore Ts'o" <tytso@mit.edu>, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, Lukas Czerner <lczerner@redhat.com>
Subject: Re: [PATCH 1/2] ext4: Make mb_optimize_scan option work with
 set/unset mount cmd
Message-ID: <20220312055818.s6wdut3riqsqssq7@riteshh-domain>
References: <c98970fe99f26718586d02e942f293300fb48ef3.1646732698.git.ojaswin@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c98970fe99f26718586d02e942f293300fb48ef3.1646732698.git.ojaswin@linux.ibm.com>
Precedence: bulk

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
>