[PATCH] ext4: Fix journal_ioprio mount option handling

[PATCH] ext4: Fix journal_ioprio mount option handling

[Ojaswin Mujoo]

In __ext4_super() we always overwrote the user specified journal_ioprio
value with a default value, expecting  parse_apply_sb_mount_options() to
later correctly set ctx->journal_ioprio to the user specified value.
However, if parse_apply_sb_mount_options() returned early because of
empty sbi->es_s->s_mount_opts, the correct journal_ioprio value was
never set.

This patch fixes __ext4_super() to only use the default value if the
user has not specified any value for journal_ioprio.

Similarly, the remount behavior was to either use journal_ioprio
value specified during initial mount, or use the default value
irrespective of the journal_ioprio value specified during remount.
This patch modifies this to first check if a new value for ioprio
has been passed during remount and apply it. Incase, no new value is
passed, use the value specified during initial mount.

Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/super.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c5a9ffbf7f4f..bfd767c51203 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4427,7 +4427,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	int silent = fc->sb_flags & SB_SILENT;
 
 	/* Set defaults for the variables that will be set during parsing */
-	ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
+	if (!(ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO))
+		ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
 
 	sbi->s_inode_readahead_blks = EXT4_DEF_INODE_READAHEAD_BLKS;
 	sbi->s_sectors_written_start =
@@ -6289,7 +6290,6 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
 	char *to_free[EXT4_MAXQUOTAS];
 #endif
 
-	ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
 
 	/* Store the original options */
 	old_sb_flags = sb->s_flags;
@@ -6315,9 +6315,14 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
 		} else
 			old_opts.s_qf_names[i] = NULL;
 #endif
-	if (sbi->s_journal && sbi->s_journal->j_task->io_context)
-		ctx->journal_ioprio =
-			sbi->s_journal->j_task->io_context->ioprio;
+	if (!(ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO)) {
+		if (sbi->s_journal && sbi->s_journal->j_task->io_context)
+			ctx->journal_ioprio =
+				sbi->s_journal->j_task->io_context->ioprio;
+		else
+			ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
+
+	}
 
 	ext4_apply_options(fc, sb);
 
-- 
2.27.0

Re: [PATCH] ext4: Fix journal_ioprio mount option handling

[Ojaswin Mujoo]

Hello,

Please let me know if there are any suggestions or reviews on this patch.

Thank you in advance,
Ojaswin

On Mon, Apr 18, 2022 at 02:05:45PM +0530, Ojaswin Mujoo wrote:
> In __ext4_super() we always overwrote the user specified journal_ioprio
> value with a default value, expecting  parse_apply_sb_mount_options() to
> later correctly set ctx->journal_ioprio to the user specified value.
> However, if parse_apply_sb_mount_options() returned early because of
> empty sbi->es_s->s_mount_opts, the correct journal_ioprio value was
> never set.
> 
> This patch fixes __ext4_super() to only use the default value if the
> user has not specified any value for journal_ioprio.
> 
> Similarly, the remount behavior was to either use journal_ioprio
> value specified during initial mount, or use the default value
> irrespective of the journal_ioprio value specified during remount.
> This patch modifies this to first check if a new value for ioprio
> has been passed during remount and apply it. Incase, no new value is
> passed, use the value specified during initial mount.
> 
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
>  fs/ext4/super.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c5a9ffbf7f4f..bfd767c51203 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4427,7 +4427,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	int silent = fc->sb_flags & SB_SILENT;
>  
>  	/* Set defaults for the variables that will be set during parsing */
> -	ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> +	if (!(ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO))
> +		ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
>  
>  	sbi->s_inode_readahead_blks = EXT4_DEF_INODE_READAHEAD_BLKS;
>  	sbi->s_sectors_written_start =
> @@ -6289,7 +6290,6 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
>  	char *to_free[EXT4_MAXQUOTAS];
>  #endif
>  
> -	ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
>  
>  	/* Store the original options */
>  	old_sb_flags = sb->s_flags;
> @@ -6315,9 +6315,14 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
>  		} else
>  			old_opts.s_qf_names[i] = NULL;
>  #endif
> -	if (sbi->s_journal && sbi->s_journal->j_task->io_context)
> -		ctx->journal_ioprio =
> -			sbi->s_journal->j_task->io_context->ioprio;
> +	if (!(ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO)) {
> +		if (sbi->s_journal && sbi->s_journal->j_task->io_context)
> +			ctx->journal_ioprio =
> +				sbi->s_journal->j_task->io_context->ioprio;
> +		else
> +			ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> +
> +	}
>  
>  	ext4_apply_options(fc, sb);
>  
> -- 
> 2.27.0
> 

Re: [PATCH] ext4: Fix journal_ioprio mount option handling

[Ritesh Harjani]

On 22/04/18 02:05PM, Ojaswin Mujoo wrote:
> In __ext4_super() we always overwrote the user specified journal_ioprio
> value with a default value, expecting  parse_apply_sb_mount_options() to
> later correctly set ctx->journal_ioprio to the user specified value.
> However, if parse_apply_sb_mount_options() returned early because of
> empty sbi->es_s->s_mount_opts, the correct journal_ioprio value was
> never set.

>
> This patch fixes __ext4_super() to only use the default value if the
					^^^ __ext4_fill_super
> user has not specified any value for journal_ioprio.

Also the problem is that ext4_parse_param() is called before
__ext4_fill_super(). Hence when we overwrite ctx->journal_ioprio to default
value in __ext4_fill_super(), that will end up ignoring the user passed
journal_ioprio value via mount opts (which was passed earlier in
ext4_parse_param()).


>
> Similarly, the remount behavior was to either use journal_ioprio
> value specified during initial mount, or use the default value
> irrespective of the journal_ioprio value specified during remount.
> This patch modifies this to first check if a new value for ioprio
> has been passed during remount and apply it. Incase, no new value is
> passed, use the value specified during initial mount.

Yup, here also ext4_parse_param() is called before __ext4_remount().
Hence we should check if the user has passed it's value in mount opts, if not,
only then we should use the task original ioprio.


I tested this patch and with the patch applied, the task ioprio can be correctly
set using "journal_ioprio" mount option.

"Mount test"
=============
qemu-> sudo perf record -e probe:* -aR mount -o journal_ioprio=1 /dev/loop2 /mnt
qemu-> ps -eaf |grep -E "jbd2|loop2"
root        3506       2  0 07:41 ?        00:00:00 [jbd2/loop2-8]
qemu-> sudo perf script
           mount  3504 [000]  2503.106871: probe:ext4_parse_param_L222: (ffffffff8147817f) journal_ioprio=16385 spec=32
           mount  3504 [000]  2503.106908: probe:__ext4_fill_super_L26: (ffffffff8147a650) journal_ioprio=16385 spec=32
qemu-> ionice -p 3506
best-effort: prio 1

"remount test"
=================
qemu-> sudo perf record -e probe:* -aR mount -o remount,journal_ioprio=0 /dev/loop2 /mnt
qemu-> sudo perf script
           mount  3519 [000]  2544.958850: probe:ext4_parse_param_L222: (ffffffff8147817f) journal_ioprio=16384 spec=32
           mount  3519 [000]  2544.958860:    probe:__ext4_remount_L49: (ffffffff81479da2) journal_ioprio=16384 spec=32
qemu-> ionice -p 3506
best-effort: prio 0

"remount with no mount options"
=================================
qemu-> sudo perf record -e probe:* -aR mount -o remount /dev/loop2 /mnt
qemu-> ionice -p 3506
best-effort: prio 0
qemu-> sudo perf script
           mount  3530 [000]  2575.964048:    probe:__ext4_remount_L49: (ffffffff81479da2) journal_ioprio=16384 spec=0


>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

We should add fixes tag too. Can you please confirm if that would be this patch?
"ext4: Completely separate options parsing and sb setup".

With that feel free to add below -

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


-ritesh

> ---
>  fs/ext4/super.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c5a9ffbf7f4f..bfd767c51203 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4427,7 +4427,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	int silent = fc->sb_flags & SB_SILENT;
>
>  	/* Set defaults for the variables that will be set during parsing */
> -	ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> +	if (!(ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO))
> +		ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
>
>  	sbi->s_inode_readahead_blks = EXT4_DEF_INODE_READAHEAD_BLKS;
>  	sbi->s_sectors_written_start =
> @@ -6289,7 +6290,6 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
>  	char *to_free[EXT4_MAXQUOTAS];
>  #endif
>
> -	ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
>
>  	/* Store the original options */
>  	old_sb_flags = sb->s_flags;
> @@ -6315,9 +6315,14 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
>  		} else
>  			old_opts.s_qf_names[i] = NULL;
>  #endif
> -	if (sbi->s_journal && sbi->s_journal->j_task->io_context)
> -		ctx->journal_ioprio =
> -			sbi->s_journal->j_task->io_context->ioprio;
> +	if (!(ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO)) {
> +		if (sbi->s_journal && sbi->s_journal->j_task->io_context)
> +			ctx->journal_ioprio =
> +				sbi->s_journal->j_task->io_context->ioprio;
> +		else
> +			ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> +
> +	}
>
>  	ext4_apply_options(fc, sb);
>
> --
> 2.27.0
>

Re: [PATCH] ext4: Fix journal_ioprio mount option handling

[Ojaswin Mujoo]

Hi Ritesh,

Thanks for taking the time to review this.

On Wed, May 11, 2022 at 01:18:53PM +0530, Ritesh Harjani wrote:
> On 22/04/18 02:05PM, Ojaswin Mujoo wrote:
> > In __ext4_super() we always overwrote the user specified journal_ioprio
> > value with a default value, expecting  parse_apply_sb_mount_options() to
> > later correctly set ctx->journal_ioprio to the user specified value.
> > However, if parse_apply_sb_mount_options() returned early because of
> > empty sbi->es_s->s_mount_opts, the correct journal_ioprio value was
> > never set.
> 
> >
> > This patch fixes __ext4_super() to only use the default value if the
> 					^^^ __ext4_fill_super
Oops, will fix this in v2.

> > user has not specified any value for journal_ioprio.
> 
> Also the problem is that ext4_parse_param() is called before
> __ext4_fill_super(). Hence when we overwrite ctx->journal_ioprio to default
> value in __ext4_fill_super(), that will end up ignoring the user passed
> journal_ioprio value via mount opts (which was passed earlier in
> ext4_parse_param()).
Right, I think my commit mesage is a bit incorrect in this regards. I
will fix this in v2.
> 
> 
> >
> > Similarly, the remount behavior was to either use journal_ioprio
> > value specified during initial mount, or use the default value
> > irrespective of the journal_ioprio value specified during remount.
> > This patch modifies this to first check if a new value for ioprio
> > has been passed during remount and apply it. Incase, no new value is
> > passed, use the value specified during initial mount.
> 
> Yup, here also ext4_parse_param() is called before __ext4_remount().
> Hence we should check if the user has passed it's value in mount opts, if not,
> only then we should use the task original ioprio.
Yes correct.
> 
> 
> I tested this patch and with the patch applied, the task ioprio can be correctly
> set using "journal_ioprio" mount option.
> 
> "Mount test"
> =============
> qemu-> sudo perf record -e probe:* -aR mount -o journal_ioprio=1 /dev/loop2 /mnt
> qemu-> ps -eaf |grep -E "jbd2|loop2"
> root        3506       2  0 07:41 ?        00:00:00 [jbd2/loop2-8]
> qemu-> sudo perf script
>            mount  3504 [000]  2503.106871: probe:ext4_parse_param_L222: (ffffffff8147817f) journal_ioprio=16385 spec=32
>            mount  3504 [000]  2503.106908: probe:__ext4_fill_super_L26: (ffffffff8147a650) journal_ioprio=16385 spec=32
> qemu-> ionice -p 3506
> best-effort: prio 1
> 
> "remount test"
> =================
> qemu-> sudo perf record -e probe:* -aR mount -o remount,journal_ioprio=0 /dev/loop2 /mnt
> qemu-> sudo perf script
>            mount  3519 [000]  2544.958850: probe:ext4_parse_param_L222: (ffffffff8147817f) journal_ioprio=16384 spec=32
>            mount  3519 [000]  2544.958860:    probe:__ext4_remount_L49: (ffffffff81479da2) journal_ioprio=16384 spec=32
> qemu-> ionice -p 3506
> best-effort: prio 0
> 
> "remount with no mount options"
> =================================
> qemu-> sudo perf record -e probe:* -aR mount -o remount /dev/loop2 /mnt
> qemu-> ionice -p 3506
> best-effort: prio 0
> qemu-> sudo perf script
>            mount  3530 [000]  2575.964048:    probe:__ext4_remount_L49: (ffffffff81479da2) journal_ioprio=16384 spec=0
> 
> 
> >
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> 
> We should add fixes tag too. Can you please confirm if that would be this patch?
> "ext4: Completely separate options parsing and sb setup".
Yes, it does seem like this issue was intorduced in this particular
commit. I'll add the Fixes tag.
> 
> With that feel free to add below -
> 
> Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>
> Tested-by: Ritesh Harjani <riteshh@linux.ibm.com>

Thanks again for the review. I'll wait for a bit to see if we have any
more reviews and then send a v2 with the suggested changes.

Regards,
Ojaswin
> 
> 
> -ritesh
> 
> > ---
> >  fs/ext4/super.c | 15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index c5a9ffbf7f4f..bfd767c51203 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -4427,7 +4427,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> >  	int silent = fc->sb_flags & SB_SILENT;
> >
> >  	/* Set defaults for the variables that will be set during parsing */
> > -	ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> > +	if (!(ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO))
> > +		ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> >
> >  	sbi->s_inode_readahead_blks = EXT4_DEF_INODE_READAHEAD_BLKS;
> >  	sbi->s_sectors_written_start =
> > @@ -6289,7 +6290,6 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
> >  	char *to_free[EXT4_MAXQUOTAS];
> >  #endif
> >
> > -	ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> >
> >  	/* Store the original options */
> >  	old_sb_flags = sb->s_flags;
> > @@ -6315,9 +6315,14 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
> >  		} else
> >  			old_opts.s_qf_names[i] = NULL;
> >  #endif
> > -	if (sbi->s_journal && sbi->s_journal->j_task->io_context)
> > -		ctx->journal_ioprio =
> > -			sbi->s_journal->j_task->io_context->ioprio;
> > +	if (!(ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO)) {
> > +		if (sbi->s_journal && sbi->s_journal->j_task->io_context)
> > +			ctx->journal_ioprio =
> > +				sbi->s_journal->j_task->io_context->ioprio;
> > +		else
> > +			ctx->journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> > +
> > +	}
> >
> >  	ext4_apply_options(fc, sb);
> >
> > --
> > 2.27.0
> >

Re: [PATCH] ext4: Fix journal_ioprio mount option handling

[Theodore Ts'o]

On Mon, 18 Apr 2022 14:05:45 +0530, Ojaswin Mujoo wrote:
> In __ext4_super() we always overwrote the user specified journal_ioprio
> value with a default value, expecting  parse_apply_sb_mount_options() to
> later correctly set ctx->journal_ioprio to the user specified value.
> However, if parse_apply_sb_mount_options() returned early because of
> empty sbi->es_s->s_mount_opts, the correct journal_ioprio value was
> never set.
> 
> [...]

Applied, thanks!

[1/1] ext4: Fix journal_ioprio mount option handling
      commit: 8d7b9890b5d18a593b7c7e10d23513340919b837

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