| Message ID | 20220418083545.45778-1-ojaswin@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ext4: Fix journal_ioprio mount option handling | expand |
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 >
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 >
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 > >
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,
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);
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(-)