All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] f2fs: fix checkpoint mount option wrong combination
@ 2021-02-01  0:06 ` Daeho Jeong
  0 siblings, 0 replies; 16+ messages in thread
From: Daeho Jeong @ 2021-02-01  0:06 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. 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);
 			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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination
@ 2021-02-01  0:06 ` Daeho Jeong
  0 siblings, 0 replies; 16+ messages in thread
From: Daeho Jeong @ 2021-02-01  0:06 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. 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);
 			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



_______________________________________________
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] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination
  2021-02-01  0:06 ` [f2fs-dev] " Daeho Jeong
@ 2021-02-01 12:40   ` Chao Yu
  -1 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2021-02-01 12:40 UTC (permalink / raw)
  To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Daeho Jeong

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.
>   	 */
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: fix checkpoint mount option wrong combination
@ 2021-02-01 12:40   ` Chao Yu
  0 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2021-02-01 12:40 UTC (permalink / raw)
  To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Daeho Jeong

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 12:40   ` Chao Yu
@ 2021-02-01 13:11     ` Daeho Jeong
  -1 siblings, 0 replies; 16+ messages in thread
From: Daeho Jeong @ 2021-02-01 13:11 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong

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.
> >        */
> >

^ 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
  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-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

* 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

end of thread, other threads:[~2021-02-02  1:33 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-01  0:06 [PATCH] f2fs: fix checkpoint mount option wrong combination Daeho Jeong
2021-02-01  0:06 ` [f2fs-dev] " Daeho Jeong
2021-02-01 12:40 ` Chao Yu
2021-02-01 12:40   ` Chao Yu
2021-02-01 13:11   ` Daeho Jeong
2021-02-01 13:11     ` Daeho Jeong
2021-02-01 20:11     ` Jaegeuk Kim
2021-02-01 20:11       ` Jaegeuk Kim
2021-02-01 23:33       ` Daeho Jeong
2021-02-01 23:33         ` Daeho Jeong
2021-02-02  0:41         ` Daeho Jeong
2021-02-02  0:41           ` Daeho Jeong
2021-02-02  1:32           ` Chao Yu
2021-02-02  1:32             ` Chao Yu
2021-02-02  1:28     ` Chao Yu
2021-02-02  1:28       ` 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.