2021-02-01 00:11:01

by Daeho Jeong

Subject: [PATCH] f2fs: fix checkpoint mount option wrong combination

From: Daeho Jeong <[email protected]>

As checkpoint=merge comes in, mount option setting related to
checkpoint had been mixed up. Fixed it.

Signed-off-by: Daeho Jeong <[email protected]>
---
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);
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.
*/
--
2.30.0.365.g02bc693789-goog

2021-02-01 12:44:20

by Chao Yu

Subject: Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination

On 2021/2/1 8:06, Daeho Jeong wrote:
> From: Daeho Jeong <[email protected]>
>
> As checkpoint=merge comes in, mount option setting related to
> checkpoint had been mixed up. Fixed it.
>
> Signed-off-by: Daeho Jeong <[email protected]>
> ---
> 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.
> */
>

2021-02-01 13:15:51

by Daeho Jeong

Subject: Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination

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 <[email protected]>님이 작성:
>
> On 2021/2/1 8:06, Daeho Jeong wrote:
> > From: Daeho Jeong <[email protected]>
> >
> > As checkpoint=merge comes in, mount option setting related to
> > checkpoint had been mixed up. Fixed it.
> >
> > Signed-off-by: Daeho Jeong <[email protected]>
> > ---
> > 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.
> > */
> >

2021-02-01 20:14:33

by Jaegeuk Kim

Subject: Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination

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 <[email protected]>님이 작성:
> >
> > On 2021/2/1 8:06, Daeho Jeong wrote:
> > > From: Daeho Jeong <[email protected]>
> > >
> > > As checkpoint=merge comes in, mount option setting related to
> > > checkpoint had been mixed up. Fixed it.
> > >
> > > Signed-off-by: Daeho Jeong <[email protected]>
> > > ---
> > > 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2021-02-01 23:35:33

by Daeho Jeong

Subject: Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination

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 <[email protected]>님이 작성:
>
> 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 <[email protected]>님이 작성:
> > >
> > > On 2021/2/1 8:06, Daeho Jeong wrote:
> > > > From: Daeho Jeong <[email protected]>
> > > >
> > > > As checkpoint=merge comes in, mount option setting related to
> > > > checkpoint had been mixed up. Fixed it.
> > > >
> > > > Signed-off-by: Daeho Jeong <[email protected]>
> > > > ---
> > > > 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
> > [email protected]
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2021-02-02 00:46:39

by Daeho Jeong

Subject: Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination

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 <[email protected]>님이 작성:
>
> 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 <[email protected]>님이 작성:
> >
> > 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 <[email protected]>님이 작성:
> > > >
> > > > On 2021/2/1 8:06, Daeho Jeong wrote:
> > > > > From: Daeho Jeong <[email protected]>
> > > > >
> > > > > As checkpoint=merge comes in, mount option setting related to
> > > > > checkpoint had been mixed up. Fixed it.
> > > > >
> > > > > Signed-off-by: Daeho Jeong <[email protected]>
> > > > > ---
> > > > > 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
> > > [email protected]
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

2021-02-02 01:33:11

by Chao Yu

Subject: Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination

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 <[email protected]>님이 작성:
>>
>> On 2021/2/1 8:06, Daeho Jeong wrote:
>>> From: Daeho Jeong <[email protected]>
>>>
>>> As checkpoint=merge comes in, mount option setting related to
>>> checkpoint had been mixed up. Fixed it.
>>>
>>> Signed-off-by: Daeho Jeong <[email protected]>
>>> ---
>>> 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>

2021-02-02 01:35:59

by Chao Yu

Subject: Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination

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 <[email protected]>님이 작성:
>>
>> 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 <[email protected]>님이 작성:
>>>
>>> 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 <[email protected]>님이 작성:
>>>>>
>>>>> On 2021/2/1 8:06, Daeho Jeong wrote:
>>>>>> From: Daeho Jeong <[email protected]>
>>>>>>
>>>>>> As checkpoint=merge comes in, mount option setting related to
>>>>>> checkpoint had been mixed up. Fixed it.
>>>>>>
>>>>>> Signed-off-by: Daeho Jeong <[email protected]>
>>>>>> ---
>>>>>> 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
>>>> [email protected]
>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>