ext4: Fix journal_ioprio mount option handling

PATCH Applied Ojaswin Mujoo 3 participants 5 messages Apr 18, 2022 – May 14, 2022

Subsystems (2)

VFS ext4 file system

lore

Share & tools

lore

Ojaswin Mujoo

author reviewer

2022-04-18 08:36 UTC #0 lore raw

PATCH 1/1 ext4: Fix journal_ioprio mount option handling

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 <[email protected]>

---
 fs/ext4/super.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

fs/ext4/super.c +10 -5

index c5a9ffbf7f4f..bfd767c51203 100644
@@ -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);

Signature · 1 line

2.27.0

Ojaswin Mujoo

author reviewer

2022-05-09 06:36 UTC #1 lore raw

Hello,

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

Thank you in advance,
Ojaswin

Quoted 65 lines · 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 <[email protected]>
---
 fs/ext4/super.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
fs/ext4/super.c +10 -5
index c5a9ffbf7f4f..bfd767c51203 100644
@@ -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);
 
Signature · 1 line
2.27.0

Ritesh Harjani

reviewer Reviewed-by Tested-by on v1 1/1

2022-05-11 07:49 UTC #2 lore raw

In reply to Ojaswin Mujoo

Quoted 9 lines · 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

					^^^ __ext4_fill_super

Quoted 1 line

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()).

Quoted 7 lines

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

Quoted 2 lines

Signed-off-by Ojaswin Mujoo <[email protected]>

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 <[email protected]>

Tested-by Ritesh Harjani <[email protected]>

-ritesh

Quoted 47 lines

---
 fs/ext4/super.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

fs/ext4/super.c +10 -6

index c5a9ffbf7f4f..bfd767c51203 100644
@@ -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

Ojaswin Mujoo

author reviewer

2022-05-11 10:37 UTC #3 lore raw

Hi Ritesh,

Thanks for taking the time to review this.

Quoted 11 lines · Ritesh Harjani

Quoted 9 lines · 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
					^^^ __ext4_fill_super

Oops, will fix this in v2.

Quoted 7 lines

Quoted 1 line
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.

Quoted 10 lines

Quoted 9 lines
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.

Quoted 3 lines

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.

Quoted 31 lines

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

Quoted 8 lines

           mount  3530 [000]  2575.964048:    probe:__ext4_remount_L49: (ffffffff81479da2) journal_ioprio=16384 spec=0
Quoted 2 lines
Signed-off-by Ojaswin Mujoo <[email protected]>
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.

Quoted 5 lines

With that feel free to add below -
Reviewed-by Ritesh Harjani <[email protected]>
Tested-by Ritesh Harjani <[email protected]>

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

Quoted 51 lines

-ritesh

Quoted 47 lines

---
 fs/ext4/super.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

fs/ext4/super.c +10 -6

index c5a9ffbf7f4f..bfd767c51203 100644
@@ -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

Theodore Ts'o

maintainer Applied

2022-05-14 02:57 UTC #4 lore raw

In reply to Ojaswin Mujoo

Quoted 8 lines · 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.
[...]

Applied, thanks!

[1/1] ext4: Fix journal_ioprio mount option handling

      commit: 8d7b9890b5d18a593b7c7e10d23513340919b837

Best regards,

Signature · 1 line

Theodore Ts'o <[email protected]>