* Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination
@ 2021-02-01 13:11 ` Daeho Jeong
0 siblings, 0 replies; 16+ messages in thread
From: Daeho Jeong @ 2021-02-01 13:11 UTC (permalink / raw)
To: Chao Yu; +Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel
Actually, I think we need to select one among them, disable, enable
and merge. I realized my previous understanding about that was wrong.
In that case of "checkpoint=merge,checkpoint=enable", the last option
will override the ones before that.
This is how the other mount options like fsync_mode, whint_mode and etc.
So, the answer will be "checkpoint=enable". What do you think?
2021년 2월 1일 (월) 오후 9:40, Chao Yu <chao@kernel.org>님이 작성:
>
> On 2021/2/1 8:06, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> >
> > As checkpoint=merge comes in, mount option setting related to
> > checkpoint had been mixed up. Fixed it.
> >
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > ---
> > fs/f2fs/super.c | 11 +++++------
> > 1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 56696f6cfa86..8231c888c772 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -930,20 +930,25 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
> > return -EINVAL;
> > F2FS_OPTION(sbi).unusable_cap_perc = arg;
> > set_opt(sbi, DISABLE_CHECKPOINT);
> > + clear_opt(sbi, MERGE_CHECKPOINT);
> > break;
> > case Opt_checkpoint_disable_cap:
> > if (args->from && match_int(args, &arg))
> > return -EINVAL;
> > F2FS_OPTION(sbi).unusable_cap = arg;
> > set_opt(sbi, DISABLE_CHECKPOINT);
> > + clear_opt(sbi, MERGE_CHECKPOINT);
> > break;
> > case Opt_checkpoint_disable:
> > set_opt(sbi, DISABLE_CHECKPOINT);
> > + clear_opt(sbi, MERGE_CHECKPOINT);
> > break;
> > case Opt_checkpoint_enable:
> > clear_opt(sbi, DISABLE_CHECKPOINT);
> > + clear_opt(sbi, MERGE_CHECKPOINT);
>
> What if: -o checkpoint=merge,checkpoint=enable
>
> Can you please explain the rule of merge/disable/enable combination and their
> result? e.g.
> checkpoint=merge,checkpoint=enable
> checkpoint=enable,checkpoint=merge
> checkpoint=merge,checkpoint=disable
> checkpoint=disable,checkpoint=merge
>
> If the rule/result is clear, it should be documented.
>
> Thanks,
>
>
> > break;
> > case Opt_checkpoint_merge:
> > + clear_opt(sbi, DISABLE_CHECKPOINT);
> > set_opt(sbi, MERGE_CHECKPOINT);
> > break;
> > #ifdef CONFIG_F2FS_FS_COMPRESSION
> > @@ -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.
> > */
> >
_______________________________________________
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] 16+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination
2021-02-01 13:11 ` Daeho Jeong
@ 2021-02-01 20:11 ` Jaegeuk Kim
-1 siblings, 0 replies; 16+ messages in thread
From: Jaegeuk Kim @ 2021-02-01 20:11 UTC (permalink / raw)
To: Daeho Jeong
Cc: Chao Yu, Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel
On 02/01, Daeho Jeong wrote:
> Actually, I think we need to select one among them, disable, enable
> and merge. I realized my previous understanding about that was wrong.
> In that case of "checkpoint=merge,checkpoint=enable", the last option
> will override the ones before that.
> This is how the other mount options like fsync_mode, whint_mode and etc.
> So, the answer will be "checkpoint=enable". What do you think?
We need to clarify a bit more. :)
mount checkpoint=disable,checkpoint=merge
remount checkpoint=enable,checkpoint=merge
Then, is it going to enable checkpoint with a thread?
>
>
>
> 2021년 2월 1일 (월) 오후 9:40, Chao Yu <chao@kernel.org>님이 작성:
> >
> > On 2021/2/1 8:06, Daeho Jeong wrote:
> > > From: Daeho Jeong <daehojeong@google.com>
> > >
> > > As checkpoint=merge comes in, mount option setting related to
> > > checkpoint had been mixed up. Fixed it.
> > >
> > > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > > ---
> > > fs/f2fs/super.c | 11 +++++------
> > > 1 file changed, 5 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > index 56696f6cfa86..8231c888c772 100644
> > > --- a/fs/f2fs/super.c
> > > +++ b/fs/f2fs/super.c
> > > @@ -930,20 +930,25 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
> > > return -EINVAL;
> > > F2FS_OPTION(sbi).unusable_cap_perc = arg;
> > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > break;
> > > case Opt_checkpoint_disable_cap:
> > > if (args->from && match_int(args, &arg))
> > > return -EINVAL;
> > > F2FS_OPTION(sbi).unusable_cap = arg;
> > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > break;
> > > case Opt_checkpoint_disable:
> > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > break;
> > > case Opt_checkpoint_enable:
> > > clear_opt(sbi, DISABLE_CHECKPOINT);
> > > + clear_opt(sbi, MERGE_CHECKPOINT);
> >
> > What if: -o checkpoint=merge,checkpoint=enable
> >
> > Can you please explain the rule of merge/disable/enable combination and their
> > result? e.g.
> > checkpoint=merge,checkpoint=enable
> > checkpoint=enable,checkpoint=merge
> > checkpoint=merge,checkpoint=disable
> > checkpoint=disable,checkpoint=merge
> >
> > If the rule/result is clear, it should be documented.
> >
> > Thanks,
> >
> >
> > > break;
> > > case Opt_checkpoint_merge:
> > > + clear_opt(sbi, DISABLE_CHECKPOINT);
> > > set_opt(sbi, MERGE_CHECKPOINT);
> > > break;
> > > #ifdef CONFIG_F2FS_FS_COMPRESSION
> > > @@ -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.
> > > */
> > >
>
>
> _______________________________________________
> 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] 16+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination
@ 2021-02-01 20:11 ` Jaegeuk Kim
0 siblings, 0 replies; 16+ messages in thread
From: Jaegeuk Kim @ 2021-02-01 20:11 UTC (permalink / raw)
To: Daeho Jeong; +Cc: kernel-team, Daeho Jeong, linux-f2fs-devel, linux-kernel
On 02/01, Daeho Jeong wrote:
> Actually, I think we need to select one among them, disable, enable
> and merge. I realized my previous understanding about that was wrong.
> In that case of "checkpoint=merge,checkpoint=enable", the last option
> will override the ones before that.
> This is how the other mount options like fsync_mode, whint_mode and etc.
> So, the answer will be "checkpoint=enable". What do you think?
We need to clarify a bit more. :)
mount checkpoint=disable,checkpoint=merge
remount checkpoint=enable,checkpoint=merge
Then, is it going to enable checkpoint with a thread?
>
>
>
> 2021년 2월 1일 (월) 오후 9:40, Chao Yu <chao@kernel.org>님이 작성:
> >
> > On 2021/2/1 8:06, Daeho Jeong wrote:
> > > From: Daeho Jeong <daehojeong@google.com>
> > >
> > > As checkpoint=merge comes in, mount option setting related to
> > > checkpoint had been mixed up. Fixed it.
> > >
> > > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > > ---
> > > fs/f2fs/super.c | 11 +++++------
> > > 1 file changed, 5 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > index 56696f6cfa86..8231c888c772 100644
> > > --- a/fs/f2fs/super.c
> > > +++ b/fs/f2fs/super.c
> > > @@ -930,20 +930,25 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
> > > return -EINVAL;
> > > F2FS_OPTION(sbi).unusable_cap_perc = arg;
> > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > break;
> > > case Opt_checkpoint_disable_cap:
> > > if (args->from && match_int(args, &arg))
> > > return -EINVAL;
> > > F2FS_OPTION(sbi).unusable_cap = arg;
> > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > break;
> > > case Opt_checkpoint_disable:
> > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > break;
> > > case Opt_checkpoint_enable:
> > > clear_opt(sbi, DISABLE_CHECKPOINT);
> > > + clear_opt(sbi, MERGE_CHECKPOINT);
> >
> > What if: -o checkpoint=merge,checkpoint=enable
> >
> > Can you please explain the rule of merge/disable/enable combination and their
> > result? e.g.
> > checkpoint=merge,checkpoint=enable
> > checkpoint=enable,checkpoint=merge
> > checkpoint=merge,checkpoint=disable
> > checkpoint=disable,checkpoint=merge
> >
> > If the rule/result is clear, it should be documented.
> >
> > Thanks,
> >
> >
> > > break;
> > > case Opt_checkpoint_merge:
> > > + clear_opt(sbi, DISABLE_CHECKPOINT);
> > > set_opt(sbi, MERGE_CHECKPOINT);
> > > break;
> > > #ifdef CONFIG_F2FS_FS_COMPRESSION
> > > @@ -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.
> > > */
> > >
>
>
> _______________________________________________
> 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] 16+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination
2021-02-01 20:11 ` Jaegeuk Kim
@ 2021-02-01 23:33 ` Daeho Jeong
-1 siblings, 0 replies; 16+ messages in thread
From: Daeho Jeong @ 2021-02-01 23:33 UTC (permalink / raw)
To: Jaegeuk Kim
Cc: Chao Yu, Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel
The rightmost one is the final option. And checkpoint=merge means
checkpoint is enabled with a checkpoint thread.
mount checkpoint=disable,checkpoint=merge => checkpoint=merge
remount checkpoint=enable,checkpoint=merge => checkpoint=merge
remount checkpoint=merge,checkpoint=disable => checkpoint=disable
remount checkpoint=merge,checkpoint=enable => checkpoint=enable
Like
mount fsync_mode=posix, fsync_mode=strict, fsync_mode=nobarrier =>
fsync_mode=nobarrier
2021년 2월 2일 (화) 오전 5:11, Jaegeuk Kim <jaegeuk@kernel.org>님이 작성:
>
> On 02/01, Daeho Jeong wrote:
> > Actually, I think we need to select one among them, disable, enable
> > and merge. I realized my previous understanding about that was wrong.
> > In that case of "checkpoint=merge,checkpoint=enable", the last option
> > will override the ones before that.
> > This is how the other mount options like fsync_mode, whint_mode and etc.
> > So, the answer will be "checkpoint=enable". What do you think?
>
> We need to clarify a bit more. :)
>
> mount checkpoint=disable,checkpoint=merge
> remount checkpoint=enable,checkpoint=merge
>
> Then, is it going to enable checkpoint with a thread?
>
> >
> >
> >
> > 2021년 2월 1일 (월) 오후 9:40, Chao Yu <chao@kernel.org>님이 작성:
> > >
> > > On 2021/2/1 8:06, Daeho Jeong wrote:
> > > > From: Daeho Jeong <daehojeong@google.com>
> > > >
> > > > As checkpoint=merge comes in, mount option setting related to
> > > > checkpoint had been mixed up. Fixed it.
> > > >
> > > > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > > > ---
> > > > fs/f2fs/super.c | 11 +++++------
> > > > 1 file changed, 5 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > > index 56696f6cfa86..8231c888c772 100644
> > > > --- a/fs/f2fs/super.c
> > > > +++ b/fs/f2fs/super.c
> > > > @@ -930,20 +930,25 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
> > > > return -EINVAL;
> > > > F2FS_OPTION(sbi).unusable_cap_perc = arg;
> > > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > > break;
> > > > case Opt_checkpoint_disable_cap:
> > > > if (args->from && match_int(args, &arg))
> > > > return -EINVAL;
> > > > F2FS_OPTION(sbi).unusable_cap = arg;
> > > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > > break;
> > > > case Opt_checkpoint_disable:
> > > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > > break;
> > > > case Opt_checkpoint_enable:
> > > > clear_opt(sbi, DISABLE_CHECKPOINT);
> > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > >
> > > What if: -o checkpoint=merge,checkpoint=enable
> > >
> > > Can you please explain the rule of merge/disable/enable combination and their
> > > result? e.g.
> > > checkpoint=merge,checkpoint=enable
> > > checkpoint=enable,checkpoint=merge
> > > checkpoint=merge,checkpoint=disable
> > > checkpoint=disable,checkpoint=merge
> > >
> > > If the rule/result is clear, it should be documented.
> > >
> > > Thanks,
> > >
> > >
> > > > break;
> > > > case Opt_checkpoint_merge:
> > > > + clear_opt(sbi, DISABLE_CHECKPOINT);
> > > > set_opt(sbi, MERGE_CHECKPOINT);
> > > > break;
> > > > #ifdef CONFIG_F2FS_FS_COMPRESSION
> > > > @@ -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.
> > > > */
> > > >
> >
> >
> > _______________________________________________
> > 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] 16+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination
@ 2021-02-01 23:33 ` Daeho Jeong
0 siblings, 0 replies; 16+ messages in thread
From: Daeho Jeong @ 2021-02-01 23:33 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: kernel-team, Daeho Jeong, linux-f2fs-devel, linux-kernel
The rightmost one is the final option. And checkpoint=merge means
checkpoint is enabled with a checkpoint thread.
mount checkpoint=disable,checkpoint=merge => checkpoint=merge
remount checkpoint=enable,checkpoint=merge => checkpoint=merge
remount checkpoint=merge,checkpoint=disable => checkpoint=disable
remount checkpoint=merge,checkpoint=enable => checkpoint=enable
Like
mount fsync_mode=posix, fsync_mode=strict, fsync_mode=nobarrier =>
fsync_mode=nobarrier
2021년 2월 2일 (화) 오전 5:11, Jaegeuk Kim <jaegeuk@kernel.org>님이 작성:
>
> On 02/01, Daeho Jeong wrote:
> > Actually, I think we need to select one among them, disable, enable
> > and merge. I realized my previous understanding about that was wrong.
> > In that case of "checkpoint=merge,checkpoint=enable", the last option
> > will override the ones before that.
> > This is how the other mount options like fsync_mode, whint_mode and etc.
> > So, the answer will be "checkpoint=enable". What do you think?
>
> We need to clarify a bit more. :)
>
> mount checkpoint=disable,checkpoint=merge
> remount checkpoint=enable,checkpoint=merge
>
> Then, is it going to enable checkpoint with a thread?
>
> >
> >
> >
> > 2021년 2월 1일 (월) 오후 9:40, Chao Yu <chao@kernel.org>님이 작성:
> > >
> > > On 2021/2/1 8:06, Daeho Jeong wrote:
> > > > From: Daeho Jeong <daehojeong@google.com>
> > > >
> > > > As checkpoint=merge comes in, mount option setting related to
> > > > checkpoint had been mixed up. Fixed it.
> > > >
> > > > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > > > ---
> > > > fs/f2fs/super.c | 11 +++++------
> > > > 1 file changed, 5 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > > index 56696f6cfa86..8231c888c772 100644
> > > > --- a/fs/f2fs/super.c
> > > > +++ b/fs/f2fs/super.c
> > > > @@ -930,20 +930,25 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
> > > > return -EINVAL;
> > > > F2FS_OPTION(sbi).unusable_cap_perc = arg;
> > > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > > break;
> > > > case Opt_checkpoint_disable_cap:
> > > > if (args->from && match_int(args, &arg))
> > > > return -EINVAL;
> > > > F2FS_OPTION(sbi).unusable_cap = arg;
> > > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > > break;
> > > > case Opt_checkpoint_disable:
> > > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > > break;
> > > > case Opt_checkpoint_enable:
> > > > clear_opt(sbi, DISABLE_CHECKPOINT);
> > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > >
> > > What if: -o checkpoint=merge,checkpoint=enable
> > >
> > > Can you please explain the rule of merge/disable/enable combination and their
> > > result? e.g.
> > > checkpoint=merge,checkpoint=enable
> > > checkpoint=enable,checkpoint=merge
> > > checkpoint=merge,checkpoint=disable
> > > checkpoint=disable,checkpoint=merge
> > >
> > > If the rule/result is clear, it should be documented.
> > >
> > > Thanks,
> > >
> > >
> > > > break;
> > > > case Opt_checkpoint_merge:
> > > > + clear_opt(sbi, DISABLE_CHECKPOINT);
> > > > set_opt(sbi, MERGE_CHECKPOINT);
> > > > break;
> > > > #ifdef CONFIG_F2FS_FS_COMPRESSION
> > > > @@ -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.
> > > > */
> > > >
> >
> >
> > _______________________________________________
> > 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] 16+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination
2021-02-01 23:33 ` Daeho Jeong
@ 2021-02-02 0:41 ` Daeho Jeong
-1 siblings, 0 replies; 16+ messages in thread
From: Daeho Jeong @ 2021-02-02 0:41 UTC (permalink / raw)
To: Jaegeuk Kim
Cc: Chao Yu, Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel
For less confusion, I am going to separate the "merge" option from
"checkpoint=".
I am adding another "ckpt_merge" option. :)
2021년 2월 2일 (화) 오전 8:33, Daeho Jeong <daeho43@gmail.com>님이 작성:
>
> The rightmost one is the final option. And checkpoint=merge means
> checkpoint is enabled with a checkpoint thread.
>
> mount checkpoint=disable,checkpoint=merge => checkpoint=merge
> remount checkpoint=enable,checkpoint=merge => checkpoint=merge
> remount checkpoint=merge,checkpoint=disable => checkpoint=disable
> remount checkpoint=merge,checkpoint=enable => checkpoint=enable
>
> Like
>
> mount fsync_mode=posix, fsync_mode=strict, fsync_mode=nobarrier =>
> fsync_mode=nobarrier
>
> 2021년 2월 2일 (화) 오전 5:11, Jaegeuk Kim <jaegeuk@kernel.org>님이 작성:
> >
> > On 02/01, Daeho Jeong wrote:
> > > Actually, I think we need to select one among them, disable, enable
> > > and merge. I realized my previous understanding about that was wrong.
> > > In that case of "checkpoint=merge,checkpoint=enable", the last option
> > > will override the ones before that.
> > > This is how the other mount options like fsync_mode, whint_mode and etc.
> > > So, the answer will be "checkpoint=enable". What do you think?
> >
> > We need to clarify a bit more. :)
> >
> > mount checkpoint=disable,checkpoint=merge
> > remount checkpoint=enable,checkpoint=merge
> >
> > Then, is it going to enable checkpoint with a thread?
> >
> > >
> > >
> > >
> > > 2021년 2월 1일 (월) 오후 9:40, Chao Yu <chao@kernel.org>님이 작성:
> > > >
> > > > On 2021/2/1 8:06, Daeho Jeong wrote:
> > > > > From: Daeho Jeong <daehojeong@google.com>
> > > > >
> > > > > As checkpoint=merge comes in, mount option setting related to
> > > > > checkpoint had been mixed up. Fixed it.
> > > > >
> > > > > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > > > > ---
> > > > > fs/f2fs/super.c | 11 +++++------
> > > > > 1 file changed, 5 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > > > index 56696f6cfa86..8231c888c772 100644
> > > > > --- a/fs/f2fs/super.c
> > > > > +++ b/fs/f2fs/super.c
> > > > > @@ -930,20 +930,25 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
> > > > > return -EINVAL;
> > > > > F2FS_OPTION(sbi).unusable_cap_perc = arg;
> > > > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > > > break;
> > > > > case Opt_checkpoint_disable_cap:
> > > > > if (args->from && match_int(args, &arg))
> > > > > return -EINVAL;
> > > > > F2FS_OPTION(sbi).unusable_cap = arg;
> > > > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > > > break;
> > > > > case Opt_checkpoint_disable:
> > > > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > > > break;
> > > > > case Opt_checkpoint_enable:
> > > > > clear_opt(sbi, DISABLE_CHECKPOINT);
> > > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > >
> > > > What if: -o checkpoint=merge,checkpoint=enable
> > > >
> > > > Can you please explain the rule of merge/disable/enable combination and their
> > > > result? e.g.
> > > > checkpoint=merge,checkpoint=enable
> > > > checkpoint=enable,checkpoint=merge
> > > > checkpoint=merge,checkpoint=disable
> > > > checkpoint=disable,checkpoint=merge
> > > >
> > > > If the rule/result is clear, it should be documented.
> > > >
> > > > Thanks,
> > > >
> > > >
> > > > > break;
> > > > > case Opt_checkpoint_merge:
> > > > > + clear_opt(sbi, DISABLE_CHECKPOINT);
> > > > > set_opt(sbi, MERGE_CHECKPOINT);
> > > > > break;
> > > > > #ifdef CONFIG_F2FS_FS_COMPRESSION
> > > > > @@ -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.
> > > > > */
> > > > >
> > >
> > >
> > > _______________________________________________
> > > 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] 16+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination
@ 2021-02-02 0:41 ` Daeho Jeong
0 siblings, 0 replies; 16+ messages in thread
From: Daeho Jeong @ 2021-02-02 0:41 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: kernel-team, Daeho Jeong, linux-f2fs-devel, linux-kernel
For less confusion, I am going to separate the "merge" option from
"checkpoint=".
I am adding another "ckpt_merge" option. :)
2021년 2월 2일 (화) 오전 8:33, Daeho Jeong <daeho43@gmail.com>님이 작성:
>
> The rightmost one is the final option. And checkpoint=merge means
> checkpoint is enabled with a checkpoint thread.
>
> mount checkpoint=disable,checkpoint=merge => checkpoint=merge
> remount checkpoint=enable,checkpoint=merge => checkpoint=merge
> remount checkpoint=merge,checkpoint=disable => checkpoint=disable
> remount checkpoint=merge,checkpoint=enable => checkpoint=enable
>
> Like
>
> mount fsync_mode=posix, fsync_mode=strict, fsync_mode=nobarrier =>
> fsync_mode=nobarrier
>
> 2021년 2월 2일 (화) 오전 5:11, Jaegeuk Kim <jaegeuk@kernel.org>님이 작성:
> >
> > On 02/01, Daeho Jeong wrote:
> > > Actually, I think we need to select one among them, disable, enable
> > > and merge. I realized my previous understanding about that was wrong.
> > > In that case of "checkpoint=merge,checkpoint=enable", the last option
> > > will override the ones before that.
> > > This is how the other mount options like fsync_mode, whint_mode and etc.
> > > So, the answer will be "checkpoint=enable". What do you think?
> >
> > We need to clarify a bit more. :)
> >
> > mount checkpoint=disable,checkpoint=merge
> > remount checkpoint=enable,checkpoint=merge
> >
> > Then, is it going to enable checkpoint with a thread?
> >
> > >
> > >
> > >
> > > 2021년 2월 1일 (월) 오후 9:40, Chao Yu <chao@kernel.org>님이 작성:
> > > >
> > > > On 2021/2/1 8:06, Daeho Jeong wrote:
> > > > > From: Daeho Jeong <daehojeong@google.com>
> > > > >
> > > > > As checkpoint=merge comes in, mount option setting related to
> > > > > checkpoint had been mixed up. Fixed it.
> > > > >
> > > > > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > > > > ---
> > > > > fs/f2fs/super.c | 11 +++++------
> > > > > 1 file changed, 5 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > > > index 56696f6cfa86..8231c888c772 100644
> > > > > --- a/fs/f2fs/super.c
> > > > > +++ b/fs/f2fs/super.c
> > > > > @@ -930,20 +930,25 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
> > > > > return -EINVAL;
> > > > > F2FS_OPTION(sbi).unusable_cap_perc = arg;
> > > > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > > > break;
> > > > > case Opt_checkpoint_disable_cap:
> > > > > if (args->from && match_int(args, &arg))
> > > > > return -EINVAL;
> > > > > F2FS_OPTION(sbi).unusable_cap = arg;
> > > > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > > > break;
> > > > > case Opt_checkpoint_disable:
> > > > > set_opt(sbi, DISABLE_CHECKPOINT);
> > > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > > > break;
> > > > > case Opt_checkpoint_enable:
> > > > > clear_opt(sbi, DISABLE_CHECKPOINT);
> > > > > + clear_opt(sbi, MERGE_CHECKPOINT);
> > > >
> > > > What if: -o checkpoint=merge,checkpoint=enable
> > > >
> > > > Can you please explain the rule of merge/disable/enable combination and their
> > > > result? e.g.
> > > > checkpoint=merge,checkpoint=enable
> > > > checkpoint=enable,checkpoint=merge
> > > > checkpoint=merge,checkpoint=disable
> > > > checkpoint=disable,checkpoint=merge
> > > >
> > > > If the rule/result is clear, it should be documented.
> > > >
> > > > Thanks,
> > > >
> > > >
> > > > > break;
> > > > > case Opt_checkpoint_merge:
> > > > > + clear_opt(sbi, DISABLE_CHECKPOINT);
> > > > > set_opt(sbi, MERGE_CHECKPOINT);
> > > > > break;
> > > > > #ifdef CONFIG_F2FS_FS_COMPRESSION
> > > > > @@ -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.
> > > > > */
> > > > >
> > >
> > >
> > > _______________________________________________
> > > 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] 16+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination
2021-02-02 0:41 ` Daeho Jeong
@ 2021-02-02 1:32 ` Chao Yu
-1 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2021-02-02 1:32 UTC (permalink / raw)
To: Daeho Jeong, Jaegeuk Kim
Cc: kernel-team, Daeho Jeong, linux-f2fs-devel, linux-kernel
On 2021/2/2 8:41, Daeho Jeong wrote:
> For less confusion, I am going to separate the "merge" option from
Agreed.
> "checkpoint=".
> I am adding another "ckpt_merge" option. :)
Not sure, maybe "checkpoint_merge" will be better?
"ckpt_merge" looks more like a term only developer knew.
Thanks,
>
> 2021년 2월 2일 (화) 오전 8:33, Daeho Jeong <daeho43@gmail.com>님이 작성:
>>
>> The rightmost one is the final option. And checkpoint=merge means
>> checkpoint is enabled with a checkpoint thread.
>>
>> mount checkpoint=disable,checkpoint=merge => checkpoint=merge
>> remount checkpoint=enable,checkpoint=merge => checkpoint=merge
>> remount checkpoint=merge,checkpoint=disable => checkpoint=disable
>> remount checkpoint=merge,checkpoint=enable => checkpoint=enable
>>
>> Like
>>
>> mount fsync_mode=posix, fsync_mode=strict, fsync_mode=nobarrier =>
>> fsync_mode=nobarrier
>>
>> 2021년 2월 2일 (화) 오전 5:11, Jaegeuk Kim <jaegeuk@kernel.org>님이 작성:
>>>
>>> On 02/01, Daeho Jeong wrote:
>>>> Actually, I think we need to select one among them, disable, enable
>>>> and merge. I realized my previous understanding about that was wrong.
>>>> In that case of "checkpoint=merge,checkpoint=enable", the last option
>>>> will override the ones before that.
>>>> This is how the other mount options like fsync_mode, whint_mode and etc.
>>>> So, the answer will be "checkpoint=enable". What do you think?
>>>
>>> We need to clarify a bit more. :)
>>>
>>> mount checkpoint=disable,checkpoint=merge
>>> remount checkpoint=enable,checkpoint=merge
>>>
>>> Then, is it going to enable checkpoint with a thread?
>>>
>>>>
>>>>
>>>>
>>>> 2021년 2월 1일 (월) 오후 9:40, Chao Yu <chao@kernel.org>님이 작성:
>>>>>
>>>>> On 2021/2/1 8:06, Daeho Jeong wrote:
>>>>>> From: Daeho Jeong <daehojeong@google.com>
>>>>>>
>>>>>> As checkpoint=merge comes in, mount option setting related to
>>>>>> checkpoint had been mixed up. Fixed it.
>>>>>>
>>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>>>>> ---
>>>>>> fs/f2fs/super.c | 11 +++++------
>>>>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>>>> index 56696f6cfa86..8231c888c772 100644
>>>>>> --- a/fs/f2fs/super.c
>>>>>> +++ b/fs/f2fs/super.c
>>>>>> @@ -930,20 +930,25 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
>>>>>> return -EINVAL;
>>>>>> F2FS_OPTION(sbi).unusable_cap_perc = arg;
>>>>>> set_opt(sbi, DISABLE_CHECKPOINT);
>>>>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>>>>> break;
>>>>>> case Opt_checkpoint_disable_cap:
>>>>>> if (args->from && match_int(args, &arg))
>>>>>> return -EINVAL;
>>>>>> F2FS_OPTION(sbi).unusable_cap = arg;
>>>>>> set_opt(sbi, DISABLE_CHECKPOINT);
>>>>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>>>>> break;
>>>>>> case Opt_checkpoint_disable:
>>>>>> set_opt(sbi, DISABLE_CHECKPOINT);
>>>>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>>>>> break;
>>>>>> case Opt_checkpoint_enable:
>>>>>> clear_opt(sbi, DISABLE_CHECKPOINT);
>>>>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>>>>
>>>>> What if: -o checkpoint=merge,checkpoint=enable
>>>>>
>>>>> Can you please explain the rule of merge/disable/enable combination and their
>>>>> result? e.g.
>>>>> checkpoint=merge,checkpoint=enable
>>>>> checkpoint=enable,checkpoint=merge
>>>>> checkpoint=merge,checkpoint=disable
>>>>> checkpoint=disable,checkpoint=merge
>>>>>
>>>>> If the rule/result is clear, it should be documented.
>>>>>
>>>>> Thanks,
>>>>>
>>>>>
>>>>>> break;
>>>>>> case Opt_checkpoint_merge:
>>>>>> + clear_opt(sbi, DISABLE_CHECKPOINT);
>>>>>> set_opt(sbi, MERGE_CHECKPOINT);
>>>>>> break;
>>>>>> #ifdef CONFIG_F2FS_FS_COMPRESSION
>>>>>> @@ -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.
>>>>>> */
>>>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> 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] 16+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination
@ 2021-02-02 1:32 ` Chao Yu
0 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2021-02-02 1:32 UTC (permalink / raw)
To: Daeho Jeong, Jaegeuk Kim
Cc: linux-kernel, kernel-team, Daeho Jeong, linux-f2fs-devel
On 2021/2/2 8:41, Daeho Jeong wrote:
> For less confusion, I am going to separate the "merge" option from
Agreed.
> "checkpoint=".
> I am adding another "ckpt_merge" option. :)
Not sure, maybe "checkpoint_merge" will be better?
"ckpt_merge" looks more like a term only developer knew.
Thanks,
>
> 2021년 2월 2일 (화) 오전 8:33, Daeho Jeong <daeho43@gmail.com>님이 작성:
>>
>> The rightmost one is the final option. And checkpoint=merge means
>> checkpoint is enabled with a checkpoint thread.
>>
>> mount checkpoint=disable,checkpoint=merge => checkpoint=merge
>> remount checkpoint=enable,checkpoint=merge => checkpoint=merge
>> remount checkpoint=merge,checkpoint=disable => checkpoint=disable
>> remount checkpoint=merge,checkpoint=enable => checkpoint=enable
>>
>> Like
>>
>> mount fsync_mode=posix, fsync_mode=strict, fsync_mode=nobarrier =>
>> fsync_mode=nobarrier
>>
>> 2021년 2월 2일 (화) 오전 5:11, Jaegeuk Kim <jaegeuk@kernel.org>님이 작성:
>>>
>>> On 02/01, Daeho Jeong wrote:
>>>> Actually, I think we need to select one among them, disable, enable
>>>> and merge. I realized my previous understanding about that was wrong.
>>>> In that case of "checkpoint=merge,checkpoint=enable", the last option
>>>> will override the ones before that.
>>>> This is how the other mount options like fsync_mode, whint_mode and etc.
>>>> So, the answer will be "checkpoint=enable". What do you think?
>>>
>>> We need to clarify a bit more. :)
>>>
>>> mount checkpoint=disable,checkpoint=merge
>>> remount checkpoint=enable,checkpoint=merge
>>>
>>> Then, is it going to enable checkpoint with a thread?
>>>
>>>>
>>>>
>>>>
>>>> 2021년 2월 1일 (월) 오후 9:40, Chao Yu <chao@kernel.org>님이 작성:
>>>>>
>>>>> On 2021/2/1 8:06, Daeho Jeong wrote:
>>>>>> From: Daeho Jeong <daehojeong@google.com>
>>>>>>
>>>>>> As checkpoint=merge comes in, mount option setting related to
>>>>>> checkpoint had been mixed up. Fixed it.
>>>>>>
>>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>>>>> ---
>>>>>> fs/f2fs/super.c | 11 +++++------
>>>>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>>>> index 56696f6cfa86..8231c888c772 100644
>>>>>> --- a/fs/f2fs/super.c
>>>>>> +++ b/fs/f2fs/super.c
>>>>>> @@ -930,20 +930,25 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
>>>>>> return -EINVAL;
>>>>>> F2FS_OPTION(sbi).unusable_cap_perc = arg;
>>>>>> set_opt(sbi, DISABLE_CHECKPOINT);
>>>>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>>>>> break;
>>>>>> case Opt_checkpoint_disable_cap:
>>>>>> if (args->from && match_int(args, &arg))
>>>>>> return -EINVAL;
>>>>>> F2FS_OPTION(sbi).unusable_cap = arg;
>>>>>> set_opt(sbi, DISABLE_CHECKPOINT);
>>>>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>>>>> break;
>>>>>> case Opt_checkpoint_disable:
>>>>>> set_opt(sbi, DISABLE_CHECKPOINT);
>>>>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>>>>> break;
>>>>>> case Opt_checkpoint_enable:
>>>>>> clear_opt(sbi, DISABLE_CHECKPOINT);
>>>>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>>>>
>>>>> What if: -o checkpoint=merge,checkpoint=enable
>>>>>
>>>>> Can you please explain the rule of merge/disable/enable combination and their
>>>>> result? e.g.
>>>>> checkpoint=merge,checkpoint=enable
>>>>> checkpoint=enable,checkpoint=merge
>>>>> checkpoint=merge,checkpoint=disable
>>>>> checkpoint=disable,checkpoint=merge
>>>>>
>>>>> If the rule/result is clear, it should be documented.
>>>>>
>>>>> Thanks,
>>>>>
>>>>>
>>>>>> break;
>>>>>> case Opt_checkpoint_merge:
>>>>>> + clear_opt(sbi, DISABLE_CHECKPOINT);
>>>>>> set_opt(sbi, MERGE_CHECKPOINT);
>>>>>> break;
>>>>>> #ifdef CONFIG_F2FS_FS_COMPRESSION
>>>>>> @@ -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.
>>>>>> */
>>>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> 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
>
_______________________________________________
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] 16+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination
2021-02-01 13:11 ` Daeho Jeong
@ 2021-02-02 1:28 ` Chao Yu
-1 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2021-02-02 1:28 UTC (permalink / raw)
To: Daeho Jeong, Chao Yu
Cc: Daeho Jeong, kernel-team, linux-kernel, linux-f2fs-devel
On 2021/2/1 21:11, Daeho Jeong wrote:
> Actually, I think we need to select one among them, disable, enable
> and merge. I realized my previous understanding about that was wrong.
Actually,
1. chekcpoint=enable/disable decide whether we will allow checkpoint
2. checkpoint=merge or not decide how we issue checkpoint
They are different, we should not only select only one of them, the
combination of them is allowed.
Thanks,
> In that case of "checkpoint=merge,checkpoint=enable", the last option
> will override the ones before that.
> This is how the other mount options like fsync_mode, whint_mode and etc.
> So, the answer will be "checkpoint=enable". What do you think?
>
>
>
> 2021년 2월 1일 (월) 오후 9:40, Chao Yu <chao@kernel.org>님이 작성:
>>
>> On 2021/2/1 8:06, Daeho Jeong wrote:
>>> From: Daeho Jeong <daehojeong@google.com>
>>>
>>> As checkpoint=merge comes in, mount option setting related to
>>> checkpoint had been mixed up. Fixed it.
>>>
>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>> ---
>>> fs/f2fs/super.c | 11 +++++------
>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index 56696f6cfa86..8231c888c772 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -930,20 +930,25 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
>>> return -EINVAL;
>>> F2FS_OPTION(sbi).unusable_cap_perc = arg;
>>> set_opt(sbi, DISABLE_CHECKPOINT);
>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>> break;
>>> case Opt_checkpoint_disable_cap:
>>> if (args->from && match_int(args, &arg))
>>> return -EINVAL;
>>> F2FS_OPTION(sbi).unusable_cap = arg;
>>> set_opt(sbi, DISABLE_CHECKPOINT);
>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>> break;
>>> case Opt_checkpoint_disable:
>>> set_opt(sbi, DISABLE_CHECKPOINT);
>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>> break;
>>> case Opt_checkpoint_enable:
>>> clear_opt(sbi, DISABLE_CHECKPOINT);
>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>
>> What if: -o checkpoint=merge,checkpoint=enable
>>
>> Can you please explain the rule of merge/disable/enable combination and their
>> result? e.g.
>> checkpoint=merge,checkpoint=enable
>> checkpoint=enable,checkpoint=merge
>> checkpoint=merge,checkpoint=disable
>> checkpoint=disable,checkpoint=merge
>>
>> If the rule/result is clear, it should be documented.
>>
>> Thanks,
>>
>>
>>> break;
>>> case Opt_checkpoint_merge:
>>> + clear_opt(sbi, DISABLE_CHECKPOINT);
>>> set_opt(sbi, MERGE_CHECKPOINT);
>>> break;
>>> #ifdef CONFIG_F2FS_FS_COMPRESSION
>>> @@ -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.
>>> */
>>>
>
>
> _______________________________________________
> 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] 16+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination
@ 2021-02-02 1:28 ` Chao Yu
0 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2021-02-02 1:28 UTC (permalink / raw)
To: Daeho Jeong, Chao Yu
Cc: linux-f2fs-devel, kernel-team, Daeho Jeong, linux-kernel
On 2021/2/1 21:11, Daeho Jeong wrote:
> Actually, I think we need to select one among them, disable, enable
> and merge. I realized my previous understanding about that was wrong.
Actually,
1. chekcpoint=enable/disable decide whether we will allow checkpoint
2. checkpoint=merge or not decide how we issue checkpoint
They are different, we should not only select only one of them, the
combination of them is allowed.
Thanks,
> In that case of "checkpoint=merge,checkpoint=enable", the last option
> will override the ones before that.
> This is how the other mount options like fsync_mode, whint_mode and etc.
> So, the answer will be "checkpoint=enable". What do you think?
>
>
>
> 2021년 2월 1일 (월) 오후 9:40, Chao Yu <chao@kernel.org>님이 작성:
>>
>> On 2021/2/1 8:06, Daeho Jeong wrote:
>>> From: Daeho Jeong <daehojeong@google.com>
>>>
>>> As checkpoint=merge comes in, mount option setting related to
>>> checkpoint had been mixed up. Fixed it.
>>>
>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>> ---
>>> fs/f2fs/super.c | 11 +++++------
>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index 56696f6cfa86..8231c888c772 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -930,20 +930,25 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
>>> return -EINVAL;
>>> F2FS_OPTION(sbi).unusable_cap_perc = arg;
>>> set_opt(sbi, DISABLE_CHECKPOINT);
>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>> break;
>>> case Opt_checkpoint_disable_cap:
>>> if (args->from && match_int(args, &arg))
>>> return -EINVAL;
>>> F2FS_OPTION(sbi).unusable_cap = arg;
>>> set_opt(sbi, DISABLE_CHECKPOINT);
>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>> break;
>>> case Opt_checkpoint_disable:
>>> set_opt(sbi, DISABLE_CHECKPOINT);
>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>> break;
>>> case Opt_checkpoint_enable:
>>> clear_opt(sbi, DISABLE_CHECKPOINT);
>>> + clear_opt(sbi, MERGE_CHECKPOINT);
>>
>> What if: -o checkpoint=merge,checkpoint=enable
>>
>> Can you please explain the rule of merge/disable/enable combination and their
>> result? e.g.
>> checkpoint=merge,checkpoint=enable
>> checkpoint=enable,checkpoint=merge
>> checkpoint=merge,checkpoint=disable
>> checkpoint=disable,checkpoint=merge
>>
>> If the rule/result is clear, it should be documented.
>>
>> Thanks,
>>
>>
>>> break;
>>> case Opt_checkpoint_merge:
>>> + clear_opt(sbi, DISABLE_CHECKPOINT);
>>> set_opt(sbi, MERGE_CHECKPOINT);
>>> break;
>>> #ifdef CONFIG_F2FS_FS_COMPRESSION
>>> @@ -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.
>>> */
>>>
>
>
> _______________________________________________
> 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] 16+ messages in thread