* [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).