2021-03-15 17:40:13

by harshad shirwadkar

[permalink] [raw]
Subject: [PATCH v4 2/6] ext4: add ability to return parsed options from parse_options

Before this patch, the function parse_options() was returning
journal_devnum and journal_ioprio variables to the caller. This patch
generalizes that interface to allow parse_options to return any parsed
options to return back to the caller. In this patch series, it gets
used to capture the value of "mb_optimize_scan=%u" mount option.

Signed-off-by: Harshad Shirwadkar <[email protected]>
---
fs/ext4/super.c | 50 ++++++++++++++++++++++++++++---------------------
1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 071d131fadd8..0491e7a6b04c 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2089,9 +2089,14 @@ static int ext4_set_test_dummy_encryption(struct super_block *sb,
return 1;
}

+struct ext4_parsed_options {
+ unsigned long journal_devnum;
+ unsigned int journal_ioprio;
+};
+
static int handle_mount_opt(struct super_block *sb, char *opt, int token,
- substring_t *args, unsigned long *journal_devnum,
- unsigned int *journal_ioprio, int is_remount)
+ substring_t *args, struct ext4_parsed_options *parsed_opts,
+ int is_remount)
{
struct ext4_sb_info *sbi = EXT4_SB(sb);
const struct mount_opts *m;
@@ -2248,7 +2253,7 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
"Cannot specify journal on remount");
return -1;
}
- *journal_devnum = arg;
+ parsed_opts->journal_devnum = arg;
} else if (token == Opt_journal_path) {
char *journal_path;
struct inode *journal_inode;
@@ -2284,7 +2289,7 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
return -1;
}

- *journal_devnum = new_encode_dev(journal_inode->i_rdev);
+ parsed_opts->journal_devnum = new_encode_dev(journal_inode->i_rdev);
path_put(&path);
kfree(journal_path);
} else if (token == Opt_journal_ioprio) {
@@ -2293,7 +2298,7 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
" (must be 0-7)");
return -1;
}
- *journal_ioprio =
+ parsed_opts->journal_ioprio =
IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, arg);
} else if (token == Opt_test_dummy_encryption) {
return ext4_set_test_dummy_encryption(sb, opt, &args[0],
@@ -2410,8 +2415,7 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
}

static int parse_options(char *options, struct super_block *sb,
- unsigned long *journal_devnum,
- unsigned int *journal_ioprio,
+ struct ext4_parsed_options *ret_opts,
int is_remount)
{
struct ext4_sb_info __maybe_unused *sbi = EXT4_SB(sb);
@@ -2431,8 +2435,8 @@ static int parse_options(char *options, struct super_block *sb,
*/
args[0].to = args[0].from = NULL;
token = match_token(p, tokens, args);
- if (handle_mount_opt(sb, p, token, args, journal_devnum,
- journal_ioprio, is_remount) < 0)
+ if (handle_mount_opt(sb, p, token, args, ret_opts,
+ is_remount) < 0)
return 0;
}
#ifdef CONFIG_QUOTA
@@ -4014,7 +4018,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
ext4_fsblk_t sb_block = get_sb_block(&data);
ext4_fsblk_t logical_sb_block;
unsigned long offset = 0;
- unsigned long journal_devnum = 0;
unsigned long def_mount_opts;
struct inode *root;
const char *descr;
@@ -4025,8 +4028,12 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
int needs_recovery, has_huge_files;
__u64 blocks_count;
int err = 0;
- unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
ext4_group_t first_not_zeroed;
+ struct ext4_parsed_options parsed_opts;
+
+ /* Set defaults for the variables that will be set during parsing */
+ parsed_opts.journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
+ parsed_opts.journal_devnum = 0;

if ((data && !orig_data) || !sbi)
goto out_free_base;
@@ -4272,8 +4279,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
GFP_KERNEL);
if (!s_mount_opts)
goto failed_mount;
- if (!parse_options(s_mount_opts, sb, &journal_devnum,
- &journal_ioprio, 0)) {
+ if (!parse_options(s_mount_opts, sb, &parsed_opts, 0)) {
ext4_msg(sb, KERN_WARNING,
"failed to parse options in superblock: %s",
s_mount_opts);
@@ -4281,8 +4287,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
kfree(s_mount_opts);
}
sbi->s_def_mount_opt = sbi->s_mount_opt;
- if (!parse_options((char *) data, sb, &journal_devnum,
- &journal_ioprio, 0))
+ if (!parse_options((char *) data, sb, &parsed_opts, 0))
goto failed_mount;

#ifdef CONFIG_UNICODE
@@ -4773,7 +4778,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
* root first: it may be modified in the journal!
*/
if (!test_opt(sb, NOLOAD) && ext4_has_feature_journal(sb)) {
- err = ext4_load_journal(sb, es, journal_devnum);
+ err = ext4_load_journal(sb, es, parsed_opts.journal_devnum);
if (err)
goto failed_mount3a;
} else if (test_opt(sb, NOLOAD) && !sb_rdonly(sb) &&
@@ -4873,7 +4878,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
goto failed_mount_wq;
}

- set_task_ioprio(sbi->s_journal->j_task, journal_ioprio);
+ set_task_ioprio(sbi->s_journal->j_task, parsed_opts.journal_ioprio);

sbi->s_journal->j_submit_inode_data_buffers =
ext4_journal_submit_inode_data_buffers;
@@ -5808,13 +5813,15 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
struct ext4_mount_options old_opts;
int enable_quota = 0;
ext4_group_t g;
- unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
int err = 0;
#ifdef CONFIG_QUOTA
int i, j;
char *to_free[EXT4_MAXQUOTAS];
#endif
char *orig_data = kstrdup(data, GFP_KERNEL);
+ struct ext4_parsed_options parsed_opts;
+
+ parsed_opts.journal_ioprio = DEFAULT_JOURNAL_IOPRIO;

if (data && !orig_data)
return -ENOMEM;
@@ -5845,7 +5852,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
old_opts.s_qf_names[i] = NULL;
#endif
if (sbi->s_journal && sbi->s_journal->j_task->io_context)
- journal_ioprio = sbi->s_journal->j_task->io_context->ioprio;
+ parsed_opts.journal_ioprio =
+ sbi->s_journal->j_task->io_context->ioprio;

/*
* Some options can be enabled by ext4 and/or by VFS mount flag
@@ -5855,7 +5863,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
vfs_flags = SB_LAZYTIME | SB_I_VERSION;
sb->s_flags = (sb->s_flags & ~vfs_flags) | (*flags & vfs_flags);

- if (!parse_options(data, sb, NULL, &journal_ioprio, 1)) {
+ if (!parse_options(data, sb, &parsed_opts, 1)) {
err = -EINVAL;
goto restore_opts;
}
@@ -5905,7 +5913,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)

if (sbi->s_journal) {
ext4_init_journal_params(sb, sbi->s_journal);
- set_task_ioprio(sbi->s_journal->j_task, journal_ioprio);
+ set_task_ioprio(sbi->s_journal->j_task, parsed_opts.journal_ioprio);
}

/* Flush outstanding errors before changing fs state */
--
2.31.0.rc2.261.g7f71774620-goog


2021-03-15 21:10:53

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v4 2/6] ext4: add ability to return parsed options from parse_options

On Mon, Mar 15, 2021 at 10:37:12AM -0700, Harshad Shirwadkar wrote:
> Before this patch, the function parse_options() was returning
> journal_devnum and journal_ioprio variables to the caller. This patch
> generalizes that interface to allow parse_options to return any parsed
> options to return back to the caller. In this patch series, it gets
> used to capture the value of "mb_optimize_scan=%u" mount option.

Instead of adding more code to the legacy option parsing code can
someone convert ext4 to the new mount API first?

2021-03-17 11:13:01

by Artem Blagodarenko

[permalink] [raw]
Subject: Re: [PATCH v4 2/6] ext4: add ability to return parsed options from parse_options

Hello Harshad,

Thank you for a new patchiest.

One comment bellow.

> On 15 Mar 2021, at 20:37, Harshad Shirwadkar <[email protected]> wrote:
>
> Before this patch, the function parse_options() was returning
> journal_devnum and journal_ioprio variables to the caller. This patch
> generalizes that interface to allow parse_options to return any parsed
> options to return back to the caller. In this patch series, it gets
> used to capture the value of "mb_optimize_scan=%u" mount option.
>
> Signed-off-by: Harshad Shirwadkar <[email protected]>
> ---
> fs/ext4/super.c | 50 ++++++++++++++++++++++++++++---------------------
> 1 file changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 071d131fadd8..0491e7a6b04c 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2089,9 +2089,14 @@ static int ext4_set_test_dummy_encryption(struct super_block *sb,
> return 1;
> }
>
> +struct ext4_parsed_options {
> + unsigned long journal_devnum;
> + unsigned int journal_ioprio;
> +};
> +
> static int handle_mount_opt(struct super_block *sb, char *opt, int token,
> - substring_t *args, unsigned long *journal_devnum,
> - unsigned int *journal_ioprio, int is_remount)
> + substring_t *args, struct ext4_parsed_options *parsed_opts,
> + int is_remount)
> {
> struct ext4_sb_info *sbi = EXT4_SB(sb);
> const struct mount_opts *m;
> @@ -2248,7 +2253,7 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
> "Cannot specify journal on remount");
> return -1;
> }
> - *journal_devnum = arg;
> + parsed_opts->journal_devnum = arg;
> } else if (token == Opt_journal_path) {
> char *journal_path;
> struct inode *journal_inode;
> @@ -2284,7 +2289,7 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
> return -1;
> }
>
> - *journal_devnum = new_encode_dev(journal_inode->i_rdev);
> + parsed_opts->journal_devnum = new_encode_dev(journal_inode->i_rdev);
> path_put(&path);
> kfree(journal_path);
> } else if (token == Opt_journal_ioprio) {
> @@ -2293,7 +2298,7 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
> " (must be 0-7)");
> return -1;
> }
> - *journal_ioprio =
> + parsed_opts->journal_ioprio =
> IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, arg);
> } else if (token == Opt_test_dummy_encryption) {
> return ext4_set_test_dummy_encryption(sb, opt, &args[0],
> @@ -2410,8 +2415,7 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
> }
>
> static int parse_options(char *options, struct super_block *sb,
> - unsigned long *journal_devnum,
> - unsigned int *journal_ioprio,
> + struct ext4_parsed_options *ret_opts,
> int is_remount)
> {
> struct ext4_sb_info __maybe_unused *sbi = EXT4_SB(sb);
> @@ -2431,8 +2435,8 @@ static int parse_options(char *options, struct super_block *sb,
> */
> args[0].to = args[0].from = NULL;
> token = match_token(p, tokens, args);
> - if (handle_mount_opt(sb, p, token, args, journal_devnum,
> - journal_ioprio, is_remount) < 0)
> + if (handle_mount_opt(sb, p, token, args, ret_opts,
> + is_remount) < 0)
> return 0;
> }
> #ifdef CONFIG_QUOTA
> @@ -4014,7 +4018,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> ext4_fsblk_t sb_block = get_sb_block(&data);
> ext4_fsblk_t logical_sb_block;
> unsigned long offset = 0;
> - unsigned long journal_devnum = 0;
> unsigned long def_mount_opts;
> struct inode *root;
> const char *descr;
> @@ -4025,8 +4028,12 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> int needs_recovery, has_huge_files;
> __u64 blocks_count;
> int err = 0;
> - unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> ext4_group_t first_not_zeroed;
> + struct ext4_parsed_options parsed_opts;
> +
> + /* Set defaults for the variables that will be set during parsing */
> + parsed_opts.journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> + parsed_opts.journal_devnum = 0;
>
> if ((data && !orig_data) || !sbi)
> goto out_free_base;
> @@ -4272,8 +4279,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> GFP_KERNEL);
> if (!s_mount_opts)
> goto failed_mount;
> - if (!parse_options(s_mount_opts, sb, &journal_devnum,
> - &journal_ioprio, 0)) {
> + if (!parse_options(s_mount_opts, sb, &parsed_opts, 0)) {
> ext4_msg(sb, KERN_WARNING,
> "failed to parse options in superblock: %s",
> s_mount_opts);
> @@ -4281,8 +4287,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> kfree(s_mount_opts);
> }
> sbi->s_def_mount_opt = sbi->s_mount_opt;
> - if (!parse_options((char *) data, sb, &journal_devnum,
> - &journal_ioprio, 0))
> + if (!parse_options((char *) data, sb, &parsed_opts, 0))
> goto failed_mount;
>
> #ifdef CONFIG_UNICODE
> @@ -4773,7 +4778,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> * root first: it may be modified in the journal!
> */
> if (!test_opt(sb, NOLOAD) && ext4_has_feature_journal(sb)) {
> - err = ext4_load_journal(sb, es, journal_devnum);
> + err = ext4_load_journal(sb, es, parsed_opts.journal_devnum);
> if (err)
> goto failed_mount3a;
> } else if (test_opt(sb, NOLOAD) && !sb_rdonly(sb) &&
> @@ -4873,7 +4878,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> goto failed_mount_wq;
> }
>
> - set_task_ioprio(sbi->s_journal->j_task, journal_ioprio);
> + set_task_ioprio(sbi->s_journal->j_task, parsed_opts.journal_ioprio);
>
> sbi->s_journal->j_submit_inode_data_buffers =
> ext4_journal_submit_inode_data_buffers;
> @@ -5808,13 +5813,15 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> struct ext4_mount_options old_opts;
> int enable_quota = 0;
> ext4_group_t g;
> - unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> int err = 0;
> #ifdef CONFIG_QUOTA
> int i, j;
> char *to_free[EXT4_MAXQUOTAS];
> #endif
> char *orig_data = kstrdup(data, GFP_KERNEL);
> + struct ext4_parsed_options parsed_opts;
> +
> + parsed_opts.journal_ioprio = DEFAULT_JOURNAL_IOPRIO;

Why don’t you set "parsed_opts.journal_devnum = 0;" here too?

>
> if (data && !orig_data)
> return -ENOMEM;
> @@ -5845,7 +5852,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> old_opts.s_qf_names[i] = NULL;
> #endif
> if (sbi->s_journal && sbi->s_journal->j_task->io_context)
> - journal_ioprio = sbi->s_journal->j_task->io_context->ioprio;
> + parsed_opts.journal_ioprio =
> + sbi->s_journal->j_task->io_context->ioprio;
>
> /*
> * Some options can be enabled by ext4 and/or by VFS mount flag
> @@ -5855,7 +5863,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> vfs_flags = SB_LAZYTIME | SB_I_VERSION;
> sb->s_flags = (sb->s_flags & ~vfs_flags) | (*flags & vfs_flags);
>
> - if (!parse_options(data, sb, NULL, &journal_ioprio, 1)) {
> + if (!parse_options(data, sb, &parsed_opts, 1)) {
> err = -EINVAL;
> goto restore_opts;
> }
> @@ -5905,7 +5913,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>
> if (sbi->s_journal) {
> ext4_init_journal_params(sb, sbi->s_journal);
> - set_task_ioprio(sbi->s_journal->j_task, journal_ioprio);
> + set_task_ioprio(sbi->s_journal->j_task, parsed_opts.journal_ioprio);
> }
>
> /* Flush outstanding errors before changing fs state */
> --
> 2.31.0.rc2.261.g7f71774620-goog

Best regards,
Artem Blagodarenko