* [f2fs-dev] [PATCH v2] f2fs: rename checkpoint=merge mount option to checkpoint_merge @ 2021-02-02 5:18 Daeho Jeong 2021-02-02 5:33 ` Jaegeuk Kim 2021-02-02 7:40 ` Chao Yu 0 siblings, 2 replies; 7+ messages in thread From: Daeho Jeong @ 2021-02-02 5:18 UTC (permalink / raw) To: linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Daeho Jeong From: Daeho Jeong <daehojeong@google.com> As checkpoint=merge comes in, mount option setting related to checkpoint had been mixed up and it became hard to understand. So, I separated this option from "checkpoint=" and made another mount option "checkpoint_merge" for this. Signed-off-by: Daeho Jeong <daehojeong@google.com> --- v2: renamed "checkpoint=merge" to "checkpoint_merge" --- Documentation/filesystems/f2fs.rst | 6 +++--- fs/f2fs/super.c | 26 ++++++++++++++------------ 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst index d0ead45dc706..475994ed8b15 100644 --- a/Documentation/filesystems/f2fs.rst +++ b/Documentation/filesystems/f2fs.rst @@ -247,9 +247,9 @@ checkpoint=%s[:%u[%]] Set to "disable" to turn off checkpointing. Set to "enabl hide up to all remaining free space. The actual space that would be unusable can be viewed at /sys/fs/f2fs/<disk>/unusable This space is reclaimed once checkpoint=enable. - Here is another option "merge", which creates a kernel daemon - and makes it to merge concurrent checkpoint requests as much - as possible to eliminate redundant checkpoint issues. Plus, +checkpoint_merge When checkpoint is enabled, this can be used to create a kernel + daemon and make it to merge concurrent checkpoint requests as + much as possible to eliminate redundant checkpoint issues. Plus, we can eliminate the sluggish issue caused by slow checkpoint operation when the checkpoint is done in a process context in a cgroup having low i/o budget and cpu shares. To make this diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index 56696f6cfa86..d8603e6c4916 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -145,6 +145,7 @@ enum { Opt_checkpoint_disable_cap_perc, Opt_checkpoint_enable, Opt_checkpoint_merge, + Opt_nocheckpoint_merge, Opt_compress_algorithm, Opt_compress_log_size, Opt_compress_extension, @@ -215,7 +216,8 @@ static match_table_t f2fs_tokens = { {Opt_checkpoint_disable_cap, "checkpoint=disable:%u"}, {Opt_checkpoint_disable_cap_perc, "checkpoint=disable:%u%%"}, {Opt_checkpoint_enable, "checkpoint=enable"}, - {Opt_checkpoint_merge, "checkpoint=merge"}, + {Opt_checkpoint_merge, "checkpoint_merge"}, + {Opt_nocheckpoint_merge, "nocheckpoint_merge"}, {Opt_compress_algorithm, "compress_algorithm=%s"}, {Opt_compress_log_size, "compress_log_size=%u"}, {Opt_compress_extension, "compress_extension=%s"}, @@ -946,6 +948,9 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) case Opt_checkpoint_merge: set_opt(sbi, MERGE_CHECKPOINT); break; + case Opt_nocheckpoint_merge: + clear_opt(sbi, MERGE_CHECKPOINT); + break; #ifdef CONFIG_F2FS_FS_COMPRESSION case Opt_compress_algorithm: if (!f2fs_sb_has_compression(sbi)) { @@ -1142,12 +1147,6 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) return -EINVAL; } - if (test_opt(sbi, DISABLE_CHECKPOINT) && - test_opt(sbi, MERGE_CHECKPOINT)) { - f2fs_err(sbi, "checkpoint=merge cannot be used with checkpoint=disable\n"); - return -EINVAL; - } - /* Not pass down write hints if the number of active logs is lesser * than NR_CURSEG_PERSIST_TYPE. */ @@ -1782,7 +1781,7 @@ static int f2fs_show_options(struct seq_file *seq, struct dentry *root) seq_printf(seq, ",checkpoint=disable:%u", F2FS_OPTION(sbi).unusable_cap); if (test_opt(sbi, MERGE_CHECKPOINT)) - seq_puts(seq, ",checkpoint=merge"); + seq_puts(seq, ",checkpoint_merge"); if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_POSIX) seq_printf(seq, ",fsync_mode=%s", "posix"); else if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) @@ -1827,6 +1826,7 @@ static void default_options(struct f2fs_sb_info *sbi) sbi->sb->s_flags |= SB_LAZYTIME; set_opt(sbi, FLUSH_MERGE); set_opt(sbi, DISCARD); + clear_opt(sbi, MERGE_CHECKPOINT); if (f2fs_sb_has_blkzoned(sbi)) F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS; else @@ -2066,9 +2066,8 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) } } - if (!test_opt(sbi, MERGE_CHECKPOINT)) { - f2fs_stop_ckpt_thread(sbi); - } else { + if (!test_opt(sbi, DISABLE_CHECKPOINT) && + test_opt(sbi, MERGE_CHECKPOINT)) { err = f2fs_start_ckpt_thread(sbi); if (err) { f2fs_err(sbi, @@ -2076,6 +2075,8 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) err); goto restore_gc; } + } else { + f2fs_stop_ckpt_thread(sbi); } /* @@ -3831,7 +3832,8 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) /* setup checkpoint request control and start checkpoint issue thread */ f2fs_init_ckpt_req_control(sbi); - if (test_opt(sbi, MERGE_CHECKPOINT)) { + if (!test_opt(sbi, DISABLE_CHECKPOINT) && + test_opt(sbi, MERGE_CHECKPOINT)) { err = f2fs_start_ckpt_thread(sbi); if (err) { f2fs_err(sbi, -- 2.30.0.365.g02bc693789-goog _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: rename checkpoint=merge mount option to checkpoint_merge 2021-02-02 5:18 [f2fs-dev] [PATCH v2] f2fs: rename checkpoint=merge mount option to checkpoint_merge Daeho Jeong @ 2021-02-02 5:33 ` Jaegeuk Kim 2021-02-02 7:40 ` Chao Yu 1 sibling, 0 replies; 7+ messages in thread From: Jaegeuk Kim @ 2021-02-02 5:33 UTC (permalink / raw) To: Daeho Jeong; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel On 02/02, Daeho Jeong wrote: > From: Daeho Jeong <daehojeong@google.com> > > As checkpoint=merge comes in, mount option setting related to checkpoint > had been mixed up and it became hard to understand. So, I separated > this option from "checkpoint=" and made another mount option > "checkpoint_merge" for this. Thanks, merged to the original patch. > > Signed-off-by: Daeho Jeong <daehojeong@google.com> > --- > v2: renamed "checkpoint=merge" to "checkpoint_merge" > --- > Documentation/filesystems/f2fs.rst | 6 +++--- > fs/f2fs/super.c | 26 ++++++++++++++------------ > 2 files changed, 17 insertions(+), 15 deletions(-) > > diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst > index d0ead45dc706..475994ed8b15 100644 > --- a/Documentation/filesystems/f2fs.rst > +++ b/Documentation/filesystems/f2fs.rst > @@ -247,9 +247,9 @@ checkpoint=%s[:%u[%]] Set to "disable" to turn off checkpointing. Set to "enabl > hide up to all remaining free space. The actual space that > would be unusable can be viewed at /sys/fs/f2fs/<disk>/unusable > This space is reclaimed once checkpoint=enable. > - Here is another option "merge", which creates a kernel daemon > - and makes it to merge concurrent checkpoint requests as much > - as possible to eliminate redundant checkpoint issues. Plus, > +checkpoint_merge When checkpoint is enabled, this can be used to create a kernel > + daemon and make it to merge concurrent checkpoint requests as > + much as possible to eliminate redundant checkpoint issues. Plus, > we can eliminate the sluggish issue caused by slow checkpoint > operation when the checkpoint is done in a process context in > a cgroup having low i/o budget and cpu shares. To make this > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 56696f6cfa86..d8603e6c4916 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -145,6 +145,7 @@ enum { > Opt_checkpoint_disable_cap_perc, > Opt_checkpoint_enable, > Opt_checkpoint_merge, > + Opt_nocheckpoint_merge, > Opt_compress_algorithm, > Opt_compress_log_size, > Opt_compress_extension, > @@ -215,7 +216,8 @@ static match_table_t f2fs_tokens = { > {Opt_checkpoint_disable_cap, "checkpoint=disable:%u"}, > {Opt_checkpoint_disable_cap_perc, "checkpoint=disable:%u%%"}, > {Opt_checkpoint_enable, "checkpoint=enable"}, > - {Opt_checkpoint_merge, "checkpoint=merge"}, > + {Opt_checkpoint_merge, "checkpoint_merge"}, > + {Opt_nocheckpoint_merge, "nocheckpoint_merge"}, > {Opt_compress_algorithm, "compress_algorithm=%s"}, > {Opt_compress_log_size, "compress_log_size=%u"}, > {Opt_compress_extension, "compress_extension=%s"}, > @@ -946,6 +948,9 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) > case Opt_checkpoint_merge: > set_opt(sbi, MERGE_CHECKPOINT); > break; > + case Opt_nocheckpoint_merge: > + clear_opt(sbi, MERGE_CHECKPOINT); > + break; > #ifdef CONFIG_F2FS_FS_COMPRESSION > case Opt_compress_algorithm: > if (!f2fs_sb_has_compression(sbi)) { > @@ -1142,12 +1147,6 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) > return -EINVAL; > } > > - if (test_opt(sbi, DISABLE_CHECKPOINT) && > - test_opt(sbi, MERGE_CHECKPOINT)) { > - f2fs_err(sbi, "checkpoint=merge cannot be used with checkpoint=disable\n"); > - return -EINVAL; > - } > - > /* Not pass down write hints if the number of active logs is lesser > * than NR_CURSEG_PERSIST_TYPE. > */ > @@ -1782,7 +1781,7 @@ static int f2fs_show_options(struct seq_file *seq, struct dentry *root) > seq_printf(seq, ",checkpoint=disable:%u", > F2FS_OPTION(sbi).unusable_cap); > if (test_opt(sbi, MERGE_CHECKPOINT)) > - seq_puts(seq, ",checkpoint=merge"); > + seq_puts(seq, ",checkpoint_merge"); > if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_POSIX) > seq_printf(seq, ",fsync_mode=%s", "posix"); > else if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) > @@ -1827,6 +1826,7 @@ static void default_options(struct f2fs_sb_info *sbi) > sbi->sb->s_flags |= SB_LAZYTIME; > set_opt(sbi, FLUSH_MERGE); > set_opt(sbi, DISCARD); > + clear_opt(sbi, MERGE_CHECKPOINT); > if (f2fs_sb_has_blkzoned(sbi)) > F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS; > else > @@ -2066,9 +2066,8 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) > } > } > > - if (!test_opt(sbi, MERGE_CHECKPOINT)) { > - f2fs_stop_ckpt_thread(sbi); > - } else { > + if (!test_opt(sbi, DISABLE_CHECKPOINT) && > + test_opt(sbi, MERGE_CHECKPOINT)) { > err = f2fs_start_ckpt_thread(sbi); > if (err) { > f2fs_err(sbi, > @@ -2076,6 +2075,8 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) > err); > goto restore_gc; > } > + } else { > + f2fs_stop_ckpt_thread(sbi); > } > > /* > @@ -3831,7 +3832,8 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > > /* setup checkpoint request control and start checkpoint issue thread */ > f2fs_init_ckpt_req_control(sbi); > - if (test_opt(sbi, MERGE_CHECKPOINT)) { > + if (!test_opt(sbi, DISABLE_CHECKPOINT) && > + test_opt(sbi, MERGE_CHECKPOINT)) { > err = f2fs_start_ckpt_thread(sbi); > if (err) { > f2fs_err(sbi, > -- > 2.30.0.365.g02bc693789-goog > > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: rename checkpoint=merge mount option to checkpoint_merge 2021-02-02 5:18 [f2fs-dev] [PATCH v2] f2fs: rename checkpoint=merge mount option to checkpoint_merge Daeho Jeong 2021-02-02 5:33 ` Jaegeuk Kim @ 2021-02-02 7:40 ` Chao Yu 2021-02-02 8:02 ` Daeho Jeong 1 sibling, 1 reply; 7+ messages in thread From: Chao Yu @ 2021-02-02 7:40 UTC (permalink / raw) To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Daeho Jeong On 2021/2/2 13:18, Daeho Jeong wrote: > From: Daeho Jeong <daehojeong@google.com> > > As checkpoint=merge comes in, mount option setting related to checkpoint > had been mixed up and it became hard to understand. So, I separated > this option from "checkpoint=" and made another mount option > "checkpoint_merge" for this. > > Signed-off-by: Daeho Jeong <daehojeong@google.com> > --- > v2: renamed "checkpoint=merge" to "checkpoint_merge" > --- > Documentation/filesystems/f2fs.rst | 6 +++--- > fs/f2fs/super.c | 26 ++++++++++++++------------ > 2 files changed, 17 insertions(+), 15 deletions(-) > > diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst > index d0ead45dc706..475994ed8b15 100644 > --- a/Documentation/filesystems/f2fs.rst > +++ b/Documentation/filesystems/f2fs.rst > @@ -247,9 +247,9 @@ checkpoint=%s[:%u[%]] Set to "disable" to turn off checkpointing. Set to "enabl > hide up to all remaining free space. The actual space that > would be unusable can be viewed at /sys/fs/f2fs/<disk>/unusable > This space is reclaimed once checkpoint=enable. > - Here is another option "merge", which creates a kernel daemon > - and makes it to merge concurrent checkpoint requests as much > - as possible to eliminate redundant checkpoint issues. Plus, > +checkpoint_merge When checkpoint is enabled, this can be used to create a kernel > + daemon and make it to merge concurrent checkpoint requests as > + much as possible to eliminate redundant checkpoint issues. Plus, > we can eliminate the sluggish issue caused by slow checkpoint > operation when the checkpoint is done in a process context in > a cgroup having low i/o budget and cpu shares. To make this > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 56696f6cfa86..d8603e6c4916 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -145,6 +145,7 @@ enum { > Opt_checkpoint_disable_cap_perc, > Opt_checkpoint_enable, > Opt_checkpoint_merge, > + Opt_nocheckpoint_merge, > Opt_compress_algorithm, > Opt_compress_log_size, > Opt_compress_extension, > @@ -215,7 +216,8 @@ static match_table_t f2fs_tokens = { > {Opt_checkpoint_disable_cap, "checkpoint=disable:%u"}, > {Opt_checkpoint_disable_cap_perc, "checkpoint=disable:%u%%"}, > {Opt_checkpoint_enable, "checkpoint=enable"}, > - {Opt_checkpoint_merge, "checkpoint=merge"}, > + {Opt_checkpoint_merge, "checkpoint_merge"}, > + {Opt_nocheckpoint_merge, "nocheckpoint_merge"}, > {Opt_compress_algorithm, "compress_algorithm=%s"}, > {Opt_compress_log_size, "compress_log_size=%u"}, > {Opt_compress_extension, "compress_extension=%s"}, > @@ -946,6 +948,9 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) > case Opt_checkpoint_merge: > set_opt(sbi, MERGE_CHECKPOINT); > break; > + case Opt_nocheckpoint_merge: > + clear_opt(sbi, MERGE_CHECKPOINT); > + break; > #ifdef CONFIG_F2FS_FS_COMPRESSION > case Opt_compress_algorithm: > if (!f2fs_sb_has_compression(sbi)) { > @@ -1142,12 +1147,6 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) > return -EINVAL; > } > > - if (test_opt(sbi, DISABLE_CHECKPOINT) && > - test_opt(sbi, MERGE_CHECKPOINT)) { > - f2fs_err(sbi, "checkpoint=merge cannot be used with checkpoint=disable\n"); > - return -EINVAL; > - } > - > /* Not pass down write hints if the number of active logs is lesser > * than NR_CURSEG_PERSIST_TYPE. > */ > @@ -1782,7 +1781,7 @@ static int f2fs_show_options(struct seq_file *seq, struct dentry *root) > seq_printf(seq, ",checkpoint=disable:%u", > F2FS_OPTION(sbi).unusable_cap); > if (test_opt(sbi, MERGE_CHECKPOINT)) > - seq_puts(seq, ",checkpoint=merge"); > + seq_puts(seq, ",checkpoint_merge"); Other noxxx options will be shown in show_options(), how about following them? > if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_POSIX) > seq_printf(seq, ",fsync_mode=%s", "posix"); > else if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) > @@ -1827,6 +1826,7 @@ static void default_options(struct f2fs_sb_info *sbi) > sbi->sb->s_flags |= SB_LAZYTIME; > set_opt(sbi, FLUSH_MERGE); > set_opt(sbi, DISCARD); > + clear_opt(sbi, MERGE_CHECKPOINT); Why should we clear checkpoint_merge option in default_options()? Thanks, > if (f2fs_sb_has_blkzoned(sbi)) > F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS; > else > @@ -2066,9 +2066,8 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) > } > } > > - if (!test_opt(sbi, MERGE_CHECKPOINT)) { > - f2fs_stop_ckpt_thread(sbi); > - } else { > + if (!test_opt(sbi, DISABLE_CHECKPOINT) && > + test_opt(sbi, MERGE_CHECKPOINT)) { > err = f2fs_start_ckpt_thread(sbi); > if (err) { > f2fs_err(sbi, > @@ -2076,6 +2075,8 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) > err); > goto restore_gc; > } > + } else { > + f2fs_stop_ckpt_thread(sbi); > } > > /* > @@ -3831,7 +3832,8 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > > /* setup checkpoint request control and start checkpoint issue thread */ > f2fs_init_ckpt_req_control(sbi); > - if (test_opt(sbi, MERGE_CHECKPOINT)) { > + if (!test_opt(sbi, DISABLE_CHECKPOINT) && > + test_opt(sbi, MERGE_CHECKPOINT)) { > err = f2fs_start_ckpt_thread(sbi); > if (err) { > f2fs_err(sbi, > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: rename checkpoint=merge mount option to checkpoint_merge 2021-02-02 7:40 ` Chao Yu @ 2021-02-02 8:02 ` Daeho Jeong 2021-02-02 8:30 ` Chao Yu 0 siblings, 1 reply; 7+ messages in thread From: Daeho Jeong @ 2021-02-02 8:02 UTC (permalink / raw) To: Chao Yu; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel I chose the same step with "flush_merge", because it doesn't have "noflush_merge". Do you think we need that for both, "noflush_merge" and "nocheckpoint_merge"? I thought we needed to give some time to make this be turned on by default. It might be a little radical. :) What do you think? 2021년 2월 2일 (화) 오후 4:40, Chao Yu <yuchao0@huawei.com>님이 작성: > > On 2021/2/2 13:18, Daeho Jeong wrote: > > From: Daeho Jeong <daehojeong@google.com> > > > > As checkpoint=merge comes in, mount option setting related to checkpoint > > had been mixed up and it became hard to understand. So, I separated > > this option from "checkpoint=" and made another mount option > > "checkpoint_merge" for this. > > > > Signed-off-by: Daeho Jeong <daehojeong@google.com> > > --- > > v2: renamed "checkpoint=merge" to "checkpoint_merge" > > --- > > Documentation/filesystems/f2fs.rst | 6 +++--- > > fs/f2fs/super.c | 26 ++++++++++++++------------ > > 2 files changed, 17 insertions(+), 15 deletions(-) > > > > diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst > > index d0ead45dc706..475994ed8b15 100644 > > --- a/Documentation/filesystems/f2fs.rst > > +++ b/Documentation/filesystems/f2fs.rst > > @@ -247,9 +247,9 @@ checkpoint=%s[:%u[%]] Set to "disable" to turn off checkpointing. Set to "enabl > > hide up to all remaining free space. The actual space that > > would be unusable can be viewed at /sys/fs/f2fs/<disk>/unusable > > This space is reclaimed once checkpoint=enable. > > - Here is another option "merge", which creates a kernel daemon > > - and makes it to merge concurrent checkpoint requests as much > > - as possible to eliminate redundant checkpoint issues. Plus, > > +checkpoint_merge When checkpoint is enabled, this can be used to create a kernel > > + daemon and make it to merge concurrent checkpoint requests as > > + much as possible to eliminate redundant checkpoint issues. Plus, > > we can eliminate the sluggish issue caused by slow checkpoint > > operation when the checkpoint is done in a process context in > > a cgroup having low i/o budget and cpu shares. To make this > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > > index 56696f6cfa86..d8603e6c4916 100644 > > --- a/fs/f2fs/super.c > > +++ b/fs/f2fs/super.c > > @@ -145,6 +145,7 @@ enum { > > Opt_checkpoint_disable_cap_perc, > > Opt_checkpoint_enable, > > Opt_checkpoint_merge, > > + Opt_nocheckpoint_merge, > > Opt_compress_algorithm, > > Opt_compress_log_size, > > Opt_compress_extension, > > @@ -215,7 +216,8 @@ static match_table_t f2fs_tokens = { > > {Opt_checkpoint_disable_cap, "checkpoint=disable:%u"}, > > {Opt_checkpoint_disable_cap_perc, "checkpoint=disable:%u%%"}, > > {Opt_checkpoint_enable, "checkpoint=enable"}, > > - {Opt_checkpoint_merge, "checkpoint=merge"}, > > + {Opt_checkpoint_merge, "checkpoint_merge"}, > > + {Opt_nocheckpoint_merge, "nocheckpoint_merge"}, > > {Opt_compress_algorithm, "compress_algorithm=%s"}, > > {Opt_compress_log_size, "compress_log_size=%u"}, > > {Opt_compress_extension, "compress_extension=%s"}, > > @@ -946,6 +948,9 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) > > case Opt_checkpoint_merge: > > set_opt(sbi, MERGE_CHECKPOINT); > > break; > > + case Opt_nocheckpoint_merge: > > + clear_opt(sbi, MERGE_CHECKPOINT); > > + break; > > #ifdef CONFIG_F2FS_FS_COMPRESSION > > case Opt_compress_algorithm: > > if (!f2fs_sb_has_compression(sbi)) { > > @@ -1142,12 +1147,6 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) > > return -EINVAL; > > } > > > > - if (test_opt(sbi, DISABLE_CHECKPOINT) && > > - test_opt(sbi, MERGE_CHECKPOINT)) { > > - f2fs_err(sbi, "checkpoint=merge cannot be used with checkpoint=disable\n"); > > - return -EINVAL; > > - } > > - > > /* Not pass down write hints if the number of active logs is lesser > > * than NR_CURSEG_PERSIST_TYPE. > > */ > > @@ -1782,7 +1781,7 @@ static int f2fs_show_options(struct seq_file *seq, struct dentry *root) > > seq_printf(seq, ",checkpoint=disable:%u", > > F2FS_OPTION(sbi).unusable_cap); > > if (test_opt(sbi, MERGE_CHECKPOINT)) > > - seq_puts(seq, ",checkpoint=merge"); > > + seq_puts(seq, ",checkpoint_merge"); > > Other noxxx options will be shown in show_options(), how about following them? > > > if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_POSIX) > > seq_printf(seq, ",fsync_mode=%s", "posix"); > > else if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) > > @@ -1827,6 +1826,7 @@ static void default_options(struct f2fs_sb_info *sbi) > > sbi->sb->s_flags |= SB_LAZYTIME; > > set_opt(sbi, FLUSH_MERGE); > > set_opt(sbi, DISCARD); > > + clear_opt(sbi, MERGE_CHECKPOINT); > > Why should we clear checkpoint_merge option in default_options()? > > Thanks, > > > if (f2fs_sb_has_blkzoned(sbi)) > > F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS; > > else > > @@ -2066,9 +2066,8 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) > > } > > } > > > > - if (!test_opt(sbi, MERGE_CHECKPOINT)) { > > - f2fs_stop_ckpt_thread(sbi); > > - } else { > > + if (!test_opt(sbi, DISABLE_CHECKPOINT) && > > + test_opt(sbi, MERGE_CHECKPOINT)) { > > err = f2fs_start_ckpt_thread(sbi); > > if (err) { > > f2fs_err(sbi, > > @@ -2076,6 +2075,8 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) > > err); > > goto restore_gc; > > } > > + } else { > > + f2fs_stop_ckpt_thread(sbi); > > } > > > > /* > > @@ -3831,7 +3832,8 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > > > > /* setup checkpoint request control and start checkpoint issue thread */ > > f2fs_init_ckpt_req_control(sbi); > > - if (test_opt(sbi, MERGE_CHECKPOINT)) { > > + if (!test_opt(sbi, DISABLE_CHECKPOINT) && > > + test_opt(sbi, MERGE_CHECKPOINT)) { > > err = f2fs_start_ckpt_thread(sbi); > > if (err) { > > f2fs_err(sbi, > > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: rename checkpoint=merge mount option to checkpoint_merge 2021-02-02 8:02 ` Daeho Jeong @ 2021-02-02 8:30 ` Chao Yu 2021-02-02 9:07 ` Daeho Jeong 0 siblings, 1 reply; 7+ messages in thread From: Chao Yu @ 2021-02-02 8:30 UTC (permalink / raw) To: Daeho Jeong; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel On 2021/2/2 16:02, Daeho Jeong wrote: > I chose the same step with "flush_merge", because it doesn't have > "noflush_merge". Oh, "noxxx" option was added only when we set the option by default in default_options(), when user want to disable the default option, it needs to use "noxxx" option, and then we will show this "noxxx" option string to user via show_options() to indicate that "noxxx" option is working now. Anyway I think we should fix to show "noflush_merge" option because we have set flush_merge by default. > Do you think we need that for both, "noflush_merge" and "nocheckpoint_merge"? For "nocheckpoint_merge", we can introduce this option only when we want to set "checkpoint_merge" by default. Here is the example from noinline_data: Commit 75342797988 ("f2fs: enable inline data by default") Thanks, > > I thought we needed to give some time to make this be turned on by > default. It might be a little radical. :) > > What do you think? > > 2021년 2월 2일 (화) 오후 4:40, Chao Yu <yuchao0@huawei.com>님이 작성: >> >> On 2021/2/2 13:18, Daeho Jeong wrote: >>> From: Daeho Jeong <daehojeong@google.com> >>> >>> As checkpoint=merge comes in, mount option setting related to checkpoint >>> had been mixed up and it became hard to understand. So, I separated >>> this option from "checkpoint=" and made another mount option >>> "checkpoint_merge" for this. >>> >>> Signed-off-by: Daeho Jeong <daehojeong@google.com> >>> --- >>> v2: renamed "checkpoint=merge" to "checkpoint_merge" >>> --- >>> Documentation/filesystems/f2fs.rst | 6 +++--- >>> fs/f2fs/super.c | 26 ++++++++++++++------------ >>> 2 files changed, 17 insertions(+), 15 deletions(-) >>> >>> diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst >>> index d0ead45dc706..475994ed8b15 100644 >>> --- a/Documentation/filesystems/f2fs.rst >>> +++ b/Documentation/filesystems/f2fs.rst >>> @@ -247,9 +247,9 @@ checkpoint=%s[:%u[%]] Set to "disable" to turn off checkpointing. Set to "enabl >>> hide up to all remaining free space. The actual space that >>> would be unusable can be viewed at /sys/fs/f2fs/<disk>/unusable >>> This space is reclaimed once checkpoint=enable. >>> - Here is another option "merge", which creates a kernel daemon >>> - and makes it to merge concurrent checkpoint requests as much >>> - as possible to eliminate redundant checkpoint issues. Plus, >>> +checkpoint_merge When checkpoint is enabled, this can be used to create a kernel >>> + daemon and make it to merge concurrent checkpoint requests as >>> + much as possible to eliminate redundant checkpoint issues. Plus, >>> we can eliminate the sluggish issue caused by slow checkpoint >>> operation when the checkpoint is done in a process context in >>> a cgroup having low i/o budget and cpu shares. To make this >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>> index 56696f6cfa86..d8603e6c4916 100644 >>> --- a/fs/f2fs/super.c >>> +++ b/fs/f2fs/super.c >>> @@ -145,6 +145,7 @@ enum { >>> Opt_checkpoint_disable_cap_perc, >>> Opt_checkpoint_enable, >>> Opt_checkpoint_merge, >>> + Opt_nocheckpoint_merge, >>> Opt_compress_algorithm, >>> Opt_compress_log_size, >>> Opt_compress_extension, >>> @@ -215,7 +216,8 @@ static match_table_t f2fs_tokens = { >>> {Opt_checkpoint_disable_cap, "checkpoint=disable:%u"}, >>> {Opt_checkpoint_disable_cap_perc, "checkpoint=disable:%u%%"}, >>> {Opt_checkpoint_enable, "checkpoint=enable"}, >>> - {Opt_checkpoint_merge, "checkpoint=merge"}, >>> + {Opt_checkpoint_merge, "checkpoint_merge"}, >>> + {Opt_nocheckpoint_merge, "nocheckpoint_merge"}, >>> {Opt_compress_algorithm, "compress_algorithm=%s"}, >>> {Opt_compress_log_size, "compress_log_size=%u"}, >>> {Opt_compress_extension, "compress_extension=%s"}, >>> @@ -946,6 +948,9 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) >>> case Opt_checkpoint_merge: >>> set_opt(sbi, MERGE_CHECKPOINT); >>> break; >>> + case Opt_nocheckpoint_merge: >>> + clear_opt(sbi, MERGE_CHECKPOINT); >>> + break; >>> #ifdef CONFIG_F2FS_FS_COMPRESSION >>> case Opt_compress_algorithm: >>> if (!f2fs_sb_has_compression(sbi)) { >>> @@ -1142,12 +1147,6 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) >>> return -EINVAL; >>> } >>> >>> - if (test_opt(sbi, DISABLE_CHECKPOINT) && >>> - test_opt(sbi, MERGE_CHECKPOINT)) { >>> - f2fs_err(sbi, "checkpoint=merge cannot be used with checkpoint=disable\n"); >>> - return -EINVAL; >>> - } >>> - >>> /* Not pass down write hints if the number of active logs is lesser >>> * than NR_CURSEG_PERSIST_TYPE. >>> */ >>> @@ -1782,7 +1781,7 @@ static int f2fs_show_options(struct seq_file *seq, struct dentry *root) >>> seq_printf(seq, ",checkpoint=disable:%u", >>> F2FS_OPTION(sbi).unusable_cap); >>> if (test_opt(sbi, MERGE_CHECKPOINT)) >>> - seq_puts(seq, ",checkpoint=merge"); >>> + seq_puts(seq, ",checkpoint_merge"); >> >> Other noxxx options will be shown in show_options(), how about following them? >> >>> if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_POSIX) >>> seq_printf(seq, ",fsync_mode=%s", "posix"); >>> else if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) >>> @@ -1827,6 +1826,7 @@ static void default_options(struct f2fs_sb_info *sbi) >>> sbi->sb->s_flags |= SB_LAZYTIME; >>> set_opt(sbi, FLUSH_MERGE); >>> set_opt(sbi, DISCARD); >>> + clear_opt(sbi, MERGE_CHECKPOINT); >> >> Why should we clear checkpoint_merge option in default_options()? >> >> Thanks, >> >>> if (f2fs_sb_has_blkzoned(sbi)) >>> F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS; >>> else >>> @@ -2066,9 +2066,8 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) >>> } >>> } >>> >>> - if (!test_opt(sbi, MERGE_CHECKPOINT)) { >>> - f2fs_stop_ckpt_thread(sbi); >>> - } else { >>> + if (!test_opt(sbi, DISABLE_CHECKPOINT) && >>> + test_opt(sbi, MERGE_CHECKPOINT)) { >>> err = f2fs_start_ckpt_thread(sbi); >>> if (err) { >>> f2fs_err(sbi, >>> @@ -2076,6 +2075,8 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) >>> err); >>> goto restore_gc; >>> } >>> + } else { >>> + f2fs_stop_ckpt_thread(sbi); >>> } >>> >>> /* >>> @@ -3831,7 +3832,8 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) >>> >>> /* setup checkpoint request control and start checkpoint issue thread */ >>> f2fs_init_ckpt_req_control(sbi); >>> - if (test_opt(sbi, MERGE_CHECKPOINT)) { >>> + if (!test_opt(sbi, DISABLE_CHECKPOINT) && >>> + test_opt(sbi, MERGE_CHECKPOINT)) { >>> err = f2fs_start_ckpt_thread(sbi); >>> if (err) { >>> f2fs_err(sbi, >>> > . > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: rename checkpoint=merge mount option to checkpoint_merge 2021-02-02 8:30 ` Chao Yu @ 2021-02-02 9:07 ` Daeho Jeong 2021-02-02 9:13 ` Chao Yu 0 siblings, 1 reply; 7+ messages in thread From: Daeho Jeong @ 2021-02-02 9:07 UTC (permalink / raw) To: Chao Yu; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel If I understand it correctly, the only thing I have to do now is remove "nocheckpoint_merge" now. Am I correct? :) 2021년 2월 2일 (화) 오후 5:30, Chao Yu <yuchao0@huawei.com>님이 작성: > > On 2021/2/2 16:02, Daeho Jeong wrote: > > I chose the same step with "flush_merge", because it doesn't have > > "noflush_merge". > > Oh, "noxxx" option was added only when we set the option by default in > default_options(), when user want to disable the default option, it > needs to use "noxxx" option, and then we will show this "noxxx" option > string to user via show_options() to indicate that "noxxx" option is > working now. > > Anyway I think we should fix to show "noflush_merge" option because we > have set flush_merge by default. > > > Do you think we need that for both, "noflush_merge" and "nocheckpoint_merge"? > > For "nocheckpoint_merge", we can introduce this option only when we want > to set "checkpoint_merge" by default. > > Here is the example from noinline_data: > > Commit 75342797988 ("f2fs: enable inline data by default") > > Thanks, > > > > > I thought we needed to give some time to make this be turned on by > > default. It might be a little radical. :) > > > > What do you think? > > > > 2021년 2월 2일 (화) 오후 4:40, Chao Yu <yuchao0@huawei.com>님이 작성: > >> > >> On 2021/2/2 13:18, Daeho Jeong wrote: > >>> From: Daeho Jeong <daehojeong@google.com> > >>> > >>> As checkpoint=merge comes in, mount option setting related to checkpoint > >>> had been mixed up and it became hard to understand. So, I separated > >>> this option from "checkpoint=" and made another mount option > >>> "checkpoint_merge" for this. > >>> > >>> Signed-off-by: Daeho Jeong <daehojeong@google.com> > >>> --- > >>> v2: renamed "checkpoint=merge" to "checkpoint_merge" > >>> --- > >>> Documentation/filesystems/f2fs.rst | 6 +++--- > >>> fs/f2fs/super.c | 26 ++++++++++++++------------ > >>> 2 files changed, 17 insertions(+), 15 deletions(-) > >>> > >>> diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst > >>> index d0ead45dc706..475994ed8b15 100644 > >>> --- a/Documentation/filesystems/f2fs.rst > >>> +++ b/Documentation/filesystems/f2fs.rst > >>> @@ -247,9 +247,9 @@ checkpoint=%s[:%u[%]] Set to "disable" to turn off checkpointing. Set to "enabl > >>> hide up to all remaining free space. The actual space that > >>> would be unusable can be viewed at /sys/fs/f2fs/<disk>/unusable > >>> This space is reclaimed once checkpoint=enable. > >>> - Here is another option "merge", which creates a kernel daemon > >>> - and makes it to merge concurrent checkpoint requests as much > >>> - as possible to eliminate redundant checkpoint issues. Plus, > >>> +checkpoint_merge When checkpoint is enabled, this can be used to create a kernel > >>> + daemon and make it to merge concurrent checkpoint requests as > >>> + much as possible to eliminate redundant checkpoint issues. Plus, > >>> we can eliminate the sluggish issue caused by slow checkpoint > >>> operation when the checkpoint is done in a process context in > >>> a cgroup having low i/o budget and cpu shares. To make this > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > >>> index 56696f6cfa86..d8603e6c4916 100644 > >>> --- a/fs/f2fs/super.c > >>> +++ b/fs/f2fs/super.c > >>> @@ -145,6 +145,7 @@ enum { > >>> Opt_checkpoint_disable_cap_perc, > >>> Opt_checkpoint_enable, > >>> Opt_checkpoint_merge, > >>> + Opt_nocheckpoint_merge, > >>> Opt_compress_algorithm, > >>> Opt_compress_log_size, > >>> Opt_compress_extension, > >>> @@ -215,7 +216,8 @@ static match_table_t f2fs_tokens = { > >>> {Opt_checkpoint_disable_cap, "checkpoint=disable:%u"}, > >>> {Opt_checkpoint_disable_cap_perc, "checkpoint=disable:%u%%"}, > >>> {Opt_checkpoint_enable, "checkpoint=enable"}, > >>> - {Opt_checkpoint_merge, "checkpoint=merge"}, > >>> + {Opt_checkpoint_merge, "checkpoint_merge"}, > >>> + {Opt_nocheckpoint_merge, "nocheckpoint_merge"}, > >>> {Opt_compress_algorithm, "compress_algorithm=%s"}, > >>> {Opt_compress_log_size, "compress_log_size=%u"}, > >>> {Opt_compress_extension, "compress_extension=%s"}, > >>> @@ -946,6 +948,9 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) > >>> case Opt_checkpoint_merge: > >>> set_opt(sbi, MERGE_CHECKPOINT); > >>> break; > >>> + case Opt_nocheckpoint_merge: > >>> + clear_opt(sbi, MERGE_CHECKPOINT); > >>> + break; > >>> #ifdef CONFIG_F2FS_FS_COMPRESSION > >>> case Opt_compress_algorithm: > >>> if (!f2fs_sb_has_compression(sbi)) { > >>> @@ -1142,12 +1147,6 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) > >>> return -EINVAL; > >>> } > >>> > >>> - if (test_opt(sbi, DISABLE_CHECKPOINT) && > >>> - test_opt(sbi, MERGE_CHECKPOINT)) { > >>> - f2fs_err(sbi, "checkpoint=merge cannot be used with checkpoint=disable\n"); > >>> - return -EINVAL; > >>> - } > >>> - > >>> /* Not pass down write hints if the number of active logs is lesser > >>> * than NR_CURSEG_PERSIST_TYPE. > >>> */ > >>> @@ -1782,7 +1781,7 @@ static int f2fs_show_options(struct seq_file *seq, struct dentry *root) > >>> seq_printf(seq, ",checkpoint=disable:%u", > >>> F2FS_OPTION(sbi).unusable_cap); > >>> if (test_opt(sbi, MERGE_CHECKPOINT)) > >>> - seq_puts(seq, ",checkpoint=merge"); > >>> + seq_puts(seq, ",checkpoint_merge"); > >> > >> Other noxxx options will be shown in show_options(), how about following them? > >> > >>> if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_POSIX) > >>> seq_printf(seq, ",fsync_mode=%s", "posix"); > >>> else if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) > >>> @@ -1827,6 +1826,7 @@ static void default_options(struct f2fs_sb_info *sbi) > >>> sbi->sb->s_flags |= SB_LAZYTIME; > >>> set_opt(sbi, FLUSH_MERGE); > >>> set_opt(sbi, DISCARD); > >>> + clear_opt(sbi, MERGE_CHECKPOINT); > >> > >> Why should we clear checkpoint_merge option in default_options()? > >> > >> Thanks, > >> > >>> if (f2fs_sb_has_blkzoned(sbi)) > >>> F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS; > >>> else > >>> @@ -2066,9 +2066,8 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) > >>> } > >>> } > >>> > >>> - if (!test_opt(sbi, MERGE_CHECKPOINT)) { > >>> - f2fs_stop_ckpt_thread(sbi); > >>> - } else { > >>> + if (!test_opt(sbi, DISABLE_CHECKPOINT) && > >>> + test_opt(sbi, MERGE_CHECKPOINT)) { > >>> err = f2fs_start_ckpt_thread(sbi); > >>> if (err) { > >>> f2fs_err(sbi, > >>> @@ -2076,6 +2075,8 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) > >>> err); > >>> goto restore_gc; > >>> } > >>> + } else { > >>> + f2fs_stop_ckpt_thread(sbi); > >>> } > >>> > >>> /* > >>> @@ -3831,7 +3832,8 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > >>> > >>> /* setup checkpoint request control and start checkpoint issue thread */ > >>> f2fs_init_ckpt_req_control(sbi); > >>> - if (test_opt(sbi, MERGE_CHECKPOINT)) { > >>> + if (!test_opt(sbi, DISABLE_CHECKPOINT) && > >>> + test_opt(sbi, MERGE_CHECKPOINT)) { > >>> err = f2fs_start_ckpt_thread(sbi); > >>> if (err) { > >>> f2fs_err(sbi, > >>> > > . > > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: rename checkpoint=merge mount option to checkpoint_merge 2021-02-02 9:07 ` Daeho Jeong @ 2021-02-02 9:13 ` Chao Yu 0 siblings, 0 replies; 7+ messages in thread From: Chao Yu @ 2021-02-02 9:13 UTC (permalink / raw) To: Daeho Jeong; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel On 2021/2/2 17:07, Daeho Jeong wrote: > If I understand it correctly, the only thing I have to do now is > remove "nocheckpoint_merge" now. > Am I correct? :) For this patch, Yup. :) Thanks, > > 2021년 2월 2일 (화) 오후 5:30, Chao Yu <yuchao0@huawei.com>님이 작성: >> >> On 2021/2/2 16:02, Daeho Jeong wrote: >>> I chose the same step with "flush_merge", because it doesn't have >>> "noflush_merge". >> >> Oh, "noxxx" option was added only when we set the option by default in >> default_options(), when user want to disable the default option, it >> needs to use "noxxx" option, and then we will show this "noxxx" option >> string to user via show_options() to indicate that "noxxx" option is >> working now. >> >> Anyway I think we should fix to show "noflush_merge" option because we >> have set flush_merge by default. >> >>> Do you think we need that for both, "noflush_merge" and "nocheckpoint_merge"? >> >> For "nocheckpoint_merge", we can introduce this option only when we want >> to set "checkpoint_merge" by default. >> >> Here is the example from noinline_data: >> >> Commit 75342797988 ("f2fs: enable inline data by default") >> >> Thanks, >> >>> >>> I thought we needed to give some time to make this be turned on by >>> default. It might be a little radical. :) >>> >>> What do you think? >>> >>> 2021년 2월 2일 (화) 오후 4:40, Chao Yu <yuchao0@huawei.com>님이 작성: >>>> >>>> On 2021/2/2 13:18, Daeho Jeong wrote: >>>>> From: Daeho Jeong <daehojeong@google.com> >>>>> >>>>> As checkpoint=merge comes in, mount option setting related to checkpoint >>>>> had been mixed up and it became hard to understand. So, I separated >>>>> this option from "checkpoint=" and made another mount option >>>>> "checkpoint_merge" for this. >>>>> >>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com> >>>>> --- >>>>> v2: renamed "checkpoint=merge" to "checkpoint_merge" >>>>> --- >>>>> Documentation/filesystems/f2fs.rst | 6 +++--- >>>>> fs/f2fs/super.c | 26 ++++++++++++++------------ >>>>> 2 files changed, 17 insertions(+), 15 deletions(-) >>>>> >>>>> diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst >>>>> index d0ead45dc706..475994ed8b15 100644 >>>>> --- a/Documentation/filesystems/f2fs.rst >>>>> +++ b/Documentation/filesystems/f2fs.rst >>>>> @@ -247,9 +247,9 @@ checkpoint=%s[:%u[%]] Set to "disable" to turn off checkpointing. Set to "enabl >>>>> hide up to all remaining free space. The actual space that >>>>> would be unusable can be viewed at /sys/fs/f2fs/<disk>/unusable >>>>> This space is reclaimed once checkpoint=enable. >>>>> - Here is another option "merge", which creates a kernel daemon >>>>> - and makes it to merge concurrent checkpoint requests as much >>>>> - as possible to eliminate redundant checkpoint issues. Plus, >>>>> +checkpoint_merge When checkpoint is enabled, this can be used to create a kernel >>>>> + daemon and make it to merge concurrent checkpoint requests as >>>>> + much as possible to eliminate redundant checkpoint issues. Plus, >>>>> we can eliminate the sluggish issue caused by slow checkpoint >>>>> operation when the checkpoint is done in a process context in >>>>> a cgroup having low i/o budget and cpu shares. To make this >>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>>>> index 56696f6cfa86..d8603e6c4916 100644 >>>>> --- a/fs/f2fs/super.c >>>>> +++ b/fs/f2fs/super.c >>>>> @@ -145,6 +145,7 @@ enum { >>>>> Opt_checkpoint_disable_cap_perc, >>>>> Opt_checkpoint_enable, >>>>> Opt_checkpoint_merge, >>>>> + Opt_nocheckpoint_merge, >>>>> Opt_compress_algorithm, >>>>> Opt_compress_log_size, >>>>> Opt_compress_extension, >>>>> @@ -215,7 +216,8 @@ static match_table_t f2fs_tokens = { >>>>> {Opt_checkpoint_disable_cap, "checkpoint=disable:%u"}, >>>>> {Opt_checkpoint_disable_cap_perc, "checkpoint=disable:%u%%"}, >>>>> {Opt_checkpoint_enable, "checkpoint=enable"}, >>>>> - {Opt_checkpoint_merge, "checkpoint=merge"}, >>>>> + {Opt_checkpoint_merge, "checkpoint_merge"}, >>>>> + {Opt_nocheckpoint_merge, "nocheckpoint_merge"}, >>>>> {Opt_compress_algorithm, "compress_algorithm=%s"}, >>>>> {Opt_compress_log_size, "compress_log_size=%u"}, >>>>> {Opt_compress_extension, "compress_extension=%s"}, >>>>> @@ -946,6 +948,9 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) >>>>> case Opt_checkpoint_merge: >>>>> set_opt(sbi, MERGE_CHECKPOINT); >>>>> break; >>>>> + case Opt_nocheckpoint_merge: >>>>> + clear_opt(sbi, MERGE_CHECKPOINT); >>>>> + break; >>>>> #ifdef CONFIG_F2FS_FS_COMPRESSION >>>>> case Opt_compress_algorithm: >>>>> if (!f2fs_sb_has_compression(sbi)) { >>>>> @@ -1142,12 +1147,6 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount) >>>>> return -EINVAL; >>>>> } >>>>> >>>>> - if (test_opt(sbi, DISABLE_CHECKPOINT) && >>>>> - test_opt(sbi, MERGE_CHECKPOINT)) { >>>>> - f2fs_err(sbi, "checkpoint=merge cannot be used with checkpoint=disable\n"); >>>>> - return -EINVAL; >>>>> - } >>>>> - >>>>> /* Not pass down write hints if the number of active logs is lesser >>>>> * than NR_CURSEG_PERSIST_TYPE. >>>>> */ >>>>> @@ -1782,7 +1781,7 @@ static int f2fs_show_options(struct seq_file *seq, struct dentry *root) >>>>> seq_printf(seq, ",checkpoint=disable:%u", >>>>> F2FS_OPTION(sbi).unusable_cap); >>>>> if (test_opt(sbi, MERGE_CHECKPOINT)) >>>>> - seq_puts(seq, ",checkpoint=merge"); >>>>> + seq_puts(seq, ",checkpoint_merge"); >>>> >>>> Other noxxx options will be shown in show_options(), how about following them? >>>> >>>>> if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_POSIX) >>>>> seq_printf(seq, ",fsync_mode=%s", "posix"); >>>>> else if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT) >>>>> @@ -1827,6 +1826,7 @@ static void default_options(struct f2fs_sb_info *sbi) >>>>> sbi->sb->s_flags |= SB_LAZYTIME; >>>>> set_opt(sbi, FLUSH_MERGE); >>>>> set_opt(sbi, DISCARD); >>>>> + clear_opt(sbi, MERGE_CHECKPOINT); >>>> >>>> Why should we clear checkpoint_merge option in default_options()? >>>> >>>> Thanks, >>>> >>>>> if (f2fs_sb_has_blkzoned(sbi)) >>>>> F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS; >>>>> else >>>>> @@ -2066,9 +2066,8 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) >>>>> } >>>>> } >>>>> >>>>> - if (!test_opt(sbi, MERGE_CHECKPOINT)) { >>>>> - f2fs_stop_ckpt_thread(sbi); >>>>> - } else { >>>>> + if (!test_opt(sbi, DISABLE_CHECKPOINT) && >>>>> + test_opt(sbi, MERGE_CHECKPOINT)) { >>>>> err = f2fs_start_ckpt_thread(sbi); >>>>> if (err) { >>>>> f2fs_err(sbi, >>>>> @@ -2076,6 +2075,8 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data) >>>>> err); >>>>> goto restore_gc; >>>>> } >>>>> + } else { >>>>> + f2fs_stop_ckpt_thread(sbi); >>>>> } >>>>> >>>>> /* >>>>> @@ -3831,7 +3832,8 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) >>>>> >>>>> /* setup checkpoint request control and start checkpoint issue thread */ >>>>> f2fs_init_ckpt_req_control(sbi); >>>>> - if (test_opt(sbi, MERGE_CHECKPOINT)) { >>>>> + if (!test_opt(sbi, DISABLE_CHECKPOINT) && >>>>> + test_opt(sbi, MERGE_CHECKPOINT)) { >>>>> err = f2fs_start_ckpt_thread(sbi); >>>>> if (err) { >>>>> f2fs_err(sbi, >>>>> >>> . >>> > . > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-02-02 9:14 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-02 5:18 [f2fs-dev] [PATCH v2] f2fs: rename checkpoint=merge mount option to checkpoint_merge Daeho Jeong 2021-02-02 5:33 ` Jaegeuk Kim 2021-02-02 7:40 ` Chao Yu 2021-02-02 8:02 ` Daeho Jeong 2021-02-02 8:30 ` Chao Yu 2021-02-02 9:07 ` Daeho Jeong 2021-02-02 9:13 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).