* [PATCH v3] f2fs: rename checkpoint=merge mount option to checkpoint_merge
@ 2021-02-02 9:23 ` Daeho Jeong
0 siblings, 0 replies; 12+ messages in thread
From: Daeho Jeong @ 2021-02-02 9:23 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"
v3: removed "nocheckpoint_merge" option
---
Documentation/filesystems/f2fs.rst | 6 +++---
fs/f2fs/super.c | 21 +++++++++------------
2 files changed, 12 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..b60dcef7f9d0 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -215,7 +215,7 @@ 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_compress_algorithm, "compress_algorithm=%s"},
{Opt_compress_log_size, "compress_log_size=%u"},
{Opt_compress_extension, "compress_extension=%s"},
@@ -1142,12 +1142,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 +1776,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 +1821,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 +2061,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 +2070,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 +3827,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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [f2fs-dev] [PATCH v3] f2fs: rename checkpoint=merge mount option to checkpoint_merge
@ 2021-02-02 9:23 ` Daeho Jeong
0 siblings, 0 replies; 12+ messages in thread
From: Daeho Jeong @ 2021-02-02 9:23 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"
v3: removed "nocheckpoint_merge" option
---
Documentation/filesystems/f2fs.rst | 6 +++---
fs/f2fs/super.c | 21 +++++++++------------
2 files changed, 12 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..b60dcef7f9d0 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -215,7 +215,7 @@ 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_compress_algorithm, "compress_algorithm=%s"},
{Opt_compress_log_size, "compress_log_size=%u"},
{Opt_compress_extension, "compress_extension=%s"},
@@ -1142,12 +1142,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 +1776,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 +1821,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 +2061,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 +2070,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 +3827,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] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: rename checkpoint=merge mount option to checkpoint_merge
2021-02-02 9:23 ` [f2fs-dev] " Daeho Jeong
@ 2021-02-02 9:28 ` Chao Yu
-1 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2021-02-02 9:28 UTC (permalink / raw)
To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Daeho Jeong
On 2021/2/2 17:23, 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"
> v3: removed "nocheckpoint_merge" option
> ---
> Documentation/filesystems/f2fs.rst | 6 +++---
> fs/f2fs/super.c | 21 +++++++++------------
> 2 files changed, 12 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..b60dcef7f9d0 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -215,7 +215,7 @@ 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_compress_algorithm, "compress_algorithm=%s"},
> {Opt_compress_log_size, "compress_log_size=%u"},
> {Opt_compress_extension, "compress_extension=%s"},
> @@ -1142,12 +1142,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 +1776,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 +1821,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);
It's not needed here?
Thanks,
> if (f2fs_sb_has_blkzoned(sbi))
> F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS;
> else
> @@ -2066,9 +2061,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 +2070,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 +3827,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,
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: rename checkpoint=merge mount option to checkpoint_merge
@ 2021-02-02 9:28 ` Chao Yu
0 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2021-02-02 9:28 UTC (permalink / raw)
To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Daeho Jeong
On 2021/2/2 17:23, 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"
> v3: removed "nocheckpoint_merge" option
> ---
> Documentation/filesystems/f2fs.rst | 6 +++---
> fs/f2fs/super.c | 21 +++++++++------------
> 2 files changed, 12 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..b60dcef7f9d0 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -215,7 +215,7 @@ 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_compress_algorithm, "compress_algorithm=%s"},
> {Opt_compress_log_size, "compress_log_size=%u"},
> {Opt_compress_extension, "compress_extension=%s"},
> @@ -1142,12 +1142,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 +1776,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 +1821,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);
It's not needed here?
Thanks,
> if (f2fs_sb_has_blkzoned(sbi))
> F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS;
> else
> @@ -2066,9 +2061,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 +2070,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 +3827,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] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: rename checkpoint=merge mount option to checkpoint_merge
2021-02-02 9:28 ` Chao Yu
@ 2021-02-02 9:44 ` Daeho Jeong
-1 siblings, 0 replies; 12+ messages in thread
From: Daeho Jeong @ 2021-02-02 9:44 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong
When we remount it without the "checkpoint_merge" option, shouldn't we
need to clear "MERGE_CHECKPOINT" again?
This is actually what I intended, but I was wrong. Actually, I found this.
When we remount the filesystem, the previous mount option is passed
through the "data" argument in the below.
f2fs_remount(struct super_block *sb, int *flags, char *data)
If we don't provide the "nocheckpoint_merge" option, how can we turn
off the "checkpoint_merge" option which is turned on in the previous
mount?
2021년 2월 2일 (화) 오후 6:28, Chao Yu <yuchao0@huawei.com>님이 작성:
>
> On 2021/2/2 17:23, 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"
> > v3: removed "nocheckpoint_merge" option
> > ---
> > Documentation/filesystems/f2fs.rst | 6 +++---
> > fs/f2fs/super.c | 21 +++++++++------------
> > 2 files changed, 12 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..b60dcef7f9d0 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -215,7 +215,7 @@ 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_compress_algorithm, "compress_algorithm=%s"},
> > {Opt_compress_log_size, "compress_log_size=%u"},
> > {Opt_compress_extension, "compress_extension=%s"},
> > @@ -1142,12 +1142,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 +1776,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 +1821,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);
>
> It's not needed here?
>
> Thanks,
>
> > if (f2fs_sb_has_blkzoned(sbi))
> > F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS;
> > else
> > @@ -2066,9 +2061,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 +2070,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 +3827,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,
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: rename checkpoint=merge mount option to checkpoint_merge
@ 2021-02-02 9:44 ` Daeho Jeong
0 siblings, 0 replies; 12+ messages in thread
From: Daeho Jeong @ 2021-02-02 9:44 UTC (permalink / raw)
To: Chao Yu; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel
When we remount it without the "checkpoint_merge" option, shouldn't we
need to clear "MERGE_CHECKPOINT" again?
This is actually what I intended, but I was wrong. Actually, I found this.
When we remount the filesystem, the previous mount option is passed
through the "data" argument in the below.
f2fs_remount(struct super_block *sb, int *flags, char *data)
If we don't provide the "nocheckpoint_merge" option, how can we turn
off the "checkpoint_merge" option which is turned on in the previous
mount?
2021년 2월 2일 (화) 오후 6:28, Chao Yu <yuchao0@huawei.com>님이 작성:
>
> On 2021/2/2 17:23, 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"
> > v3: removed "nocheckpoint_merge" option
> > ---
> > Documentation/filesystems/f2fs.rst | 6 +++---
> > fs/f2fs/super.c | 21 +++++++++------------
> > 2 files changed, 12 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..b60dcef7f9d0 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -215,7 +215,7 @@ 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_compress_algorithm, "compress_algorithm=%s"},
> > {Opt_compress_log_size, "compress_log_size=%u"},
> > {Opt_compress_extension, "compress_extension=%s"},
> > @@ -1142,12 +1142,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 +1776,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 +1821,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);
>
> It's not needed here?
>
> Thanks,
>
> > if (f2fs_sb_has_blkzoned(sbi))
> > F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS;
> > else
> > @@ -2066,9 +2061,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 +2070,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 +3827,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] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: rename checkpoint=merge mount option to checkpoint_merge
2021-02-02 9:44 ` Daeho Jeong
@ 2021-02-02 9:54 ` Chao Yu
-1 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2021-02-02 9:54 UTC (permalink / raw)
To: Daeho Jeong; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong
On 2021/2/2 17:44, Daeho Jeong wrote:
> When we remount it without the "checkpoint_merge" option, shouldn't we
> need to clear "MERGE_CHECKPOINT" again?
> This is actually what I intended, but I was wrong. Actually, I found this.
>
> When we remount the filesystem, the previous mount option is passed
> through the "data" argument in the below.
> f2fs_remount(struct super_block *sb, int *flags, char *data)
>
> If we don't provide the "nocheckpoint_merge" option, how can we turn
> off the "checkpoint_merge" option which is turned on in the previous
> mount?
We can use "mount -o remount /dev/xxx /mnt" to disable checkpoint_merge,
since that command won't pass old mount options to remount?
Quoted from man mount:
mount -o remount,rw /dev/foo /dir
After this call all old mount options are replaced and arbitrary stuff from fstab (or mtab) is ignored, except the loop= option which is internally generated and maintained by the
mount command.
mount -o remount,rw /dir
After this call mount reads fstab and merges these options with the options from the command line (-o). If no mountpoint found in fstab than remount with unspecified source is allowed.
Thanks,
>
> 2021년 2월 2일 (화) 오후 6:28, Chao Yu <yuchao0@huawei.com>님이 작성:
>>
>> On 2021/2/2 17:23, 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"
>>> v3: removed "nocheckpoint_merge" option
>>> ---
>>> Documentation/filesystems/f2fs.rst | 6 +++---
>>> fs/f2fs/super.c | 21 +++++++++------------
>>> 2 files changed, 12 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..b60dcef7f9d0 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -215,7 +215,7 @@ 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_compress_algorithm, "compress_algorithm=%s"},
>>> {Opt_compress_log_size, "compress_log_size=%u"},
>>> {Opt_compress_extension, "compress_extension=%s"},
>>> @@ -1142,12 +1142,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 +1776,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 +1821,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);
>>
>> It's not needed here?
>>
>> Thanks,
>>
>>> if (f2fs_sb_has_blkzoned(sbi))
>>> F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS;
>>> else
>>> @@ -2066,9 +2061,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 +2070,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 +3827,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,
>>>
> .
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: rename checkpoint=merge mount option to checkpoint_merge
@ 2021-02-02 9:54 ` Chao Yu
0 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2021-02-02 9:54 UTC (permalink / raw)
To: Daeho Jeong; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel
On 2021/2/2 17:44, Daeho Jeong wrote:
> When we remount it without the "checkpoint_merge" option, shouldn't we
> need to clear "MERGE_CHECKPOINT" again?
> This is actually what I intended, but I was wrong. Actually, I found this.
>
> When we remount the filesystem, the previous mount option is passed
> through the "data" argument in the below.
> f2fs_remount(struct super_block *sb, int *flags, char *data)
>
> If we don't provide the "nocheckpoint_merge" option, how can we turn
> off the "checkpoint_merge" option which is turned on in the previous
> mount?
We can use "mount -o remount /dev/xxx /mnt" to disable checkpoint_merge,
since that command won't pass old mount options to remount?
Quoted from man mount:
mount -o remount,rw /dev/foo /dir
After this call all old mount options are replaced and arbitrary stuff from fstab (or mtab) is ignored, except the loop= option which is internally generated and maintained by the
mount command.
mount -o remount,rw /dir
After this call mount reads fstab and merges these options with the options from the command line (-o). If no mountpoint found in fstab than remount with unspecified source is allowed.
Thanks,
>
> 2021년 2월 2일 (화) 오후 6:28, Chao Yu <yuchao0@huawei.com>님이 작성:
>>
>> On 2021/2/2 17:23, 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"
>>> v3: removed "nocheckpoint_merge" option
>>> ---
>>> Documentation/filesystems/f2fs.rst | 6 +++---
>>> fs/f2fs/super.c | 21 +++++++++------------
>>> 2 files changed, 12 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..b60dcef7f9d0 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -215,7 +215,7 @@ 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_compress_algorithm, "compress_algorithm=%s"},
>>> {Opt_compress_log_size, "compress_log_size=%u"},
>>> {Opt_compress_extension, "compress_extension=%s"},
>>> @@ -1142,12 +1142,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 +1776,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 +1821,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);
>>
>> It's not needed here?
>>
>> Thanks,
>>
>>> if (f2fs_sb_has_blkzoned(sbi))
>>> F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS;
>>> else
>>> @@ -2066,9 +2061,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 +2070,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 +3827,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] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: rename checkpoint=merge mount option to checkpoint_merge
2021-02-02 9:54 ` Chao Yu
@ 2021-02-02 11:29 ` Daeho Jeong
-1 siblings, 0 replies; 12+ messages in thread
From: Daeho Jeong @ 2021-02-02 11:29 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong
Thanks for the explanation.
I am going to remove the line clearing "MERGE_CHECKPOINT".
But, when we go with the below remount command, I think the
"nocheckpoint_merge" option will work well to disable only just
"checkpoint_merge" from the previous option.
"mount -o remount,nocheckpoint_merge /dir"
It would be more convenient to users. What do you think?
2021년 2월 2일 (화) 오후 6:55, Chao Yu <yuchao0@huawei.com>님이 작성:
>
> On 2021/2/2 17:44, Daeho Jeong wrote:
> > When we remount it without the "checkpoint_merge" option, shouldn't we
> > need to clear "MERGE_CHECKPOINT" again?
> > This is actually what I intended, but I was wrong. Actually, I found this.
> >
> > When we remount the filesystem, the previous mount option is passed
> > through the "data" argument in the below.
> > f2fs_remount(struct super_block *sb, int *flags, char *data)
> >
> > If we don't provide the "nocheckpoint_merge" option, how can we turn
> > off the "checkpoint_merge" option which is turned on in the previous
> > mount?
>
> We can use "mount -o remount /dev/xxx /mnt" to disable checkpoint_merge,
> since that command won't pass old mount options to remount?
>
> Quoted from man mount:
>
> mount -o remount,rw /dev/foo /dir
>
> After this call all old mount options are replaced and arbitrary stuff from fstab (or mtab) is ignored, except the loop= option which is internally generated and maintained by the
> mount command.
>
> mount -o remount,rw /dir
>
> After this call mount reads fstab and merges these options with the options from the command line (-o). If no mountpoint found in fstab than remount with unspecified source is allowed.
>
> Thanks,
>
> >
> > 2021년 2월 2일 (화) 오후 6:28, Chao Yu <yuchao0@huawei.com>님이 작성:
> >>
> >> On 2021/2/2 17:23, 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"
> >>> v3: removed "nocheckpoint_merge" option
> >>> ---
> >>> Documentation/filesystems/f2fs.rst | 6 +++---
> >>> fs/f2fs/super.c | 21 +++++++++------------
> >>> 2 files changed, 12 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..b60dcef7f9d0 100644
> >>> --- a/fs/f2fs/super.c
> >>> +++ b/fs/f2fs/super.c
> >>> @@ -215,7 +215,7 @@ 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_compress_algorithm, "compress_algorithm=%s"},
> >>> {Opt_compress_log_size, "compress_log_size=%u"},
> >>> {Opt_compress_extension, "compress_extension=%s"},
> >>> @@ -1142,12 +1142,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 +1776,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 +1821,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);
> >>
> >> It's not needed here?
> >>
> >> Thanks,
> >>
> >>> if (f2fs_sb_has_blkzoned(sbi))
> >>> F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS;
> >>> else
> >>> @@ -2066,9 +2061,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 +2070,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 +3827,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,
> >>>
> > .
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: rename checkpoint=merge mount option to checkpoint_merge
@ 2021-02-02 11:29 ` Daeho Jeong
0 siblings, 0 replies; 12+ messages in thread
From: Daeho Jeong @ 2021-02-02 11:29 UTC (permalink / raw)
To: Chao Yu; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel
Thanks for the explanation.
I am going to remove the line clearing "MERGE_CHECKPOINT".
But, when we go with the below remount command, I think the
"nocheckpoint_merge" option will work well to disable only just
"checkpoint_merge" from the previous option.
"mount -o remount,nocheckpoint_merge /dir"
It would be more convenient to users. What do you think?
2021년 2월 2일 (화) 오후 6:55, Chao Yu <yuchao0@huawei.com>님이 작성:
>
> On 2021/2/2 17:44, Daeho Jeong wrote:
> > When we remount it without the "checkpoint_merge" option, shouldn't we
> > need to clear "MERGE_CHECKPOINT" again?
> > This is actually what I intended, but I was wrong. Actually, I found this.
> >
> > When we remount the filesystem, the previous mount option is passed
> > through the "data" argument in the below.
> > f2fs_remount(struct super_block *sb, int *flags, char *data)
> >
> > If we don't provide the "nocheckpoint_merge" option, how can we turn
> > off the "checkpoint_merge" option which is turned on in the previous
> > mount?
>
> We can use "mount -o remount /dev/xxx /mnt" to disable checkpoint_merge,
> since that command won't pass old mount options to remount?
>
> Quoted from man mount:
>
> mount -o remount,rw /dev/foo /dir
>
> After this call all old mount options are replaced and arbitrary stuff from fstab (or mtab) is ignored, except the loop= option which is internally generated and maintained by the
> mount command.
>
> mount -o remount,rw /dir
>
> After this call mount reads fstab and merges these options with the options from the command line (-o). If no mountpoint found in fstab than remount with unspecified source is allowed.
>
> Thanks,
>
> >
> > 2021년 2월 2일 (화) 오후 6:28, Chao Yu <yuchao0@huawei.com>님이 작성:
> >>
> >> On 2021/2/2 17:23, 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"
> >>> v3: removed "nocheckpoint_merge" option
> >>> ---
> >>> Documentation/filesystems/f2fs.rst | 6 +++---
> >>> fs/f2fs/super.c | 21 +++++++++------------
> >>> 2 files changed, 12 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..b60dcef7f9d0 100644
> >>> --- a/fs/f2fs/super.c
> >>> +++ b/fs/f2fs/super.c
> >>> @@ -215,7 +215,7 @@ 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_compress_algorithm, "compress_algorithm=%s"},
> >>> {Opt_compress_log_size, "compress_log_size=%u"},
> >>> {Opt_compress_extension, "compress_extension=%s"},
> >>> @@ -1142,12 +1142,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 +1776,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 +1821,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);
> >>
> >> It's not needed here?
> >>
> >> Thanks,
> >>
> >>> if (f2fs_sb_has_blkzoned(sbi))
> >>> F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS;
> >>> else
> >>> @@ -2066,9 +2061,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 +2070,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 +3827,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] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: rename checkpoint=merge mount option to checkpoint_merge
2021-02-02 11:29 ` Daeho Jeong
@ 2021-02-02 12:09 ` Chao Yu
-1 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2021-02-02 12:09 UTC (permalink / raw)
To: Daeho Jeong, Chao Yu
Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel
On 2021/2/2 19:29, Daeho Jeong wrote:
> Thanks for the explanation.
>
> I am going to remove the line clearing "MERGE_CHECKPOINT".
> But, when we go with the below remount command, I think the
> "nocheckpoint_merge" option will work well to disable only just
> "checkpoint_merge" from the previous option.
> "mount -o remount,nocheckpoint_merge /dir"
>
> It would be more convenient to users. What do you think?
It's fine to me, please go ahead. :)
Thanks,
>
> 2021년 2월 2일 (화) 오후 6:55, Chao Yu <yuchao0@huawei.com>님이 작성:
>>
>> On 2021/2/2 17:44, Daeho Jeong wrote:
>>> When we remount it without the "checkpoint_merge" option, shouldn't we
>>> need to clear "MERGE_CHECKPOINT" again?
>>> This is actually what I intended, but I was wrong. Actually, I found this.
>>>
>>> When we remount the filesystem, the previous mount option is passed
>>> through the "data" argument in the below.
>>> f2fs_remount(struct super_block *sb, int *flags, char *data)
>>>
>>> If we don't provide the "nocheckpoint_merge" option, how can we turn
>>> off the "checkpoint_merge" option which is turned on in the previous
>>> mount?
>>
>> We can use "mount -o remount /dev/xxx /mnt" to disable checkpoint_merge,
>> since that command won't pass old mount options to remount?
>>
>> Quoted from man mount:
>>
>> mount -o remount,rw /dev/foo /dir
>>
>> After this call all old mount options are replaced and arbitrary stuff from fstab (or mtab) is ignored, except the loop= option which is internally generated and maintained by the
>> mount command.
>>
>> mount -o remount,rw /dir
>>
>> After this call mount reads fstab and merges these options with the options from the command line (-o). If no mountpoint found in fstab than remount with unspecified source is allowed.
>>
>> Thanks,
>>
>>>
>>> 2021년 2월 2일 (화) 오후 6:28, Chao Yu <yuchao0@huawei.com>님이 작성:
>>>>
>>>> On 2021/2/2 17:23, 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"
>>>>> v3: removed "nocheckpoint_merge" option
>>>>> ---
>>>>> Documentation/filesystems/f2fs.rst | 6 +++---
>>>>> fs/f2fs/super.c | 21 +++++++++------------
>>>>> 2 files changed, 12 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..b60dcef7f9d0 100644
>>>>> --- a/fs/f2fs/super.c
>>>>> +++ b/fs/f2fs/super.c
>>>>> @@ -215,7 +215,7 @@ 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_compress_algorithm, "compress_algorithm=%s"},
>>>>> {Opt_compress_log_size, "compress_log_size=%u"},
>>>>> {Opt_compress_extension, "compress_extension=%s"},
>>>>> @@ -1142,12 +1142,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 +1776,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 +1821,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);
>>>>
>>>> It's not needed here?
>>>>
>>>> Thanks,
>>>>
>>>>> if (f2fs_sb_has_blkzoned(sbi))
>>>>> F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS;
>>>>> else
>>>>> @@ -2066,9 +2061,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 +2070,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 +3827,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] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: rename checkpoint=merge mount option to checkpoint_merge
@ 2021-02-02 12:09 ` Chao Yu
0 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2021-02-02 12:09 UTC (permalink / raw)
To: Daeho Jeong, Chao Yu
Cc: linux-f2fs-devel, kernel-team, Daeho Jeong, linux-kernel
On 2021/2/2 19:29, Daeho Jeong wrote:
> Thanks for the explanation.
>
> I am going to remove the line clearing "MERGE_CHECKPOINT".
> But, when we go with the below remount command, I think the
> "nocheckpoint_merge" option will work well to disable only just
> "checkpoint_merge" from the previous option.
> "mount -o remount,nocheckpoint_merge /dir"
>
> It would be more convenient to users. What do you think?
It's fine to me, please go ahead. :)
Thanks,
>
> 2021년 2월 2일 (화) 오후 6:55, Chao Yu <yuchao0@huawei.com>님이 작성:
>>
>> On 2021/2/2 17:44, Daeho Jeong wrote:
>>> When we remount it without the "checkpoint_merge" option, shouldn't we
>>> need to clear "MERGE_CHECKPOINT" again?
>>> This is actually what I intended, but I was wrong. Actually, I found this.
>>>
>>> When we remount the filesystem, the previous mount option is passed
>>> through the "data" argument in the below.
>>> f2fs_remount(struct super_block *sb, int *flags, char *data)
>>>
>>> If we don't provide the "nocheckpoint_merge" option, how can we turn
>>> off the "checkpoint_merge" option which is turned on in the previous
>>> mount?
>>
>> We can use "mount -o remount /dev/xxx /mnt" to disable checkpoint_merge,
>> since that command won't pass old mount options to remount?
>>
>> Quoted from man mount:
>>
>> mount -o remount,rw /dev/foo /dir
>>
>> After this call all old mount options are replaced and arbitrary stuff from fstab (or mtab) is ignored, except the loop= option which is internally generated and maintained by the
>> mount command.
>>
>> mount -o remount,rw /dir
>>
>> After this call mount reads fstab and merges these options with the options from the command line (-o). If no mountpoint found in fstab than remount with unspecified source is allowed.
>>
>> Thanks,
>>
>>>
>>> 2021년 2월 2일 (화) 오후 6:28, Chao Yu <yuchao0@huawei.com>님이 작성:
>>>>
>>>> On 2021/2/2 17:23, 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"
>>>>> v3: removed "nocheckpoint_merge" option
>>>>> ---
>>>>> Documentation/filesystems/f2fs.rst | 6 +++---
>>>>> fs/f2fs/super.c | 21 +++++++++------------
>>>>> 2 files changed, 12 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..b60dcef7f9d0 100644
>>>>> --- a/fs/f2fs/super.c
>>>>> +++ b/fs/f2fs/super.c
>>>>> @@ -215,7 +215,7 @@ 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_compress_algorithm, "compress_algorithm=%s"},
>>>>> {Opt_compress_log_size, "compress_log_size=%u"},
>>>>> {Opt_compress_extension, "compress_extension=%s"},
>>>>> @@ -1142,12 +1142,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 +1776,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 +1821,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);
>>>>
>>>> It's not needed here?
>>>>
>>>> Thanks,
>>>>
>>>>> if (f2fs_sb_has_blkzoned(sbi))
>>>>> F2FS_OPTION(sbi).fs_mode = FS_MODE_LFS;
>>>>> else
>>>>> @@ -2066,9 +2061,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 +2070,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 +3827,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
>
_______________________________________________
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] 12+ messages in thread
end of thread, other threads:[~2021-02-02 12:11 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-02 9:23 [PATCH v3] f2fs: rename checkpoint=merge mount option to checkpoint_merge Daeho Jeong
2021-02-02 9:23 ` [f2fs-dev] " Daeho Jeong
2021-02-02 9:28 ` Chao Yu
2021-02-02 9:28 ` Chao Yu
2021-02-02 9:44 ` Daeho Jeong
2021-02-02 9:44 ` Daeho Jeong
2021-02-02 9:54 ` Chao Yu
2021-02-02 9:54 ` Chao Yu
2021-02-02 11:29 ` Daeho Jeong
2021-02-02 11:29 ` Daeho Jeong
2021-02-02 12:09 ` Chao Yu
2021-02-02 12:09 ` Chao Yu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.