[PATCH 1/5] f2fs: fix not to drop mount options when retrying fill_super

[PATCH 1/5] f2fs: fix not to drop mount options when retrying fill_super

[Jaegeuk Kim]

If wrong mount option was requested, f2fs tries to fill_super again.
But, during the next trial, f2fs has no valid mount options, since
parse_options deleted all the separators in the original string.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/super.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 5706c17..fbc7f5a 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -950,6 +950,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	struct inode *root;
 	long err = -EINVAL;
 	bool retry = true;
+	char *options = NULL;
 	int i;
 
 try_onemore:
@@ -981,9 +982,15 @@ try_onemore:
 	set_opt(sbi, POSIX_ACL);
 #endif
 	/* parse mount options */
-	err = parse_options(sb, (char *)data);
-	if (err)
+	options = kstrdup((const char *)data, GFP_KERNEL);
+	if (data && !options) {
+		err = -ENOMEM;
 		goto free_sb_buf;
+	}
+
+	err = parse_options(sb, options);
+	if (err)
+		goto free_options;
 
 	sb->s_maxbytes = max_file_size(le32_to_cpu(raw_super->log_blocksize));
 	sb->s_max_links = F2FS_LINK_MAX;
@@ -1027,7 +1034,7 @@ try_onemore:
 	if (IS_ERR(sbi->meta_inode)) {
 		f2fs_msg(sb, KERN_ERR, "Failed to read F2FS meta data inode");
 		err = PTR_ERR(sbi->meta_inode);
-		goto free_sb_buf;
+		goto free_options;
 	}
 
 	err = get_valid_checkpoint(sbi);
@@ -1176,6 +1183,8 @@ free_cp:
 free_meta_inode:
 	make_bad_inode(sbi->meta_inode);
 	iput(sbi->meta_inode);
+free_options:
+	kfree(options);
 free_sb_buf:
 	brelse(raw_super_buf);
 free_sbi:
-- 
2.1.1

[PATCH 2/5] f2fs: support norecovery mount option

[Jaegeuk Kim]

This patch adds a mount option, norecovery, which is mostly same as
disable_roll_forward. The only difference is that norecovery should be activated
with read-only mount option.

This can be used when user wants to check whether f2fs is mountable or not
without any recovery process. (e.g., xfstests/200)

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/super.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index fbc7f5a..0ca1fb2 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -42,6 +42,7 @@ static struct kset *f2fs_kset;
 enum {
 	Opt_gc_background,
 	Opt_disable_roll_forward,
+	Opt_norecovery,
 	Opt_discard,
 	Opt_noheap,
 	Opt_user_xattr,
@@ -62,6 +63,7 @@ enum {
 static match_table_t f2fs_tokens = {
 	{Opt_gc_background, "background_gc=%s"},
 	{Opt_disable_roll_forward, "disable_roll_forward"},
+	{Opt_norecovery, "norecovery"},
 	{Opt_discard, "discard"},
 	{Opt_noheap, "no_heap"},
 	{Opt_user_xattr, "user_xattr"},
@@ -287,6 +289,12 @@ static int parse_options(struct super_block *sb, char *options)
 		case Opt_disable_roll_forward:
 			set_opt(sbi, DISABLE_ROLL_FORWARD);
 			break;
+		case Opt_norecovery:
+			/* this option mounts f2fs with ro */
+			set_opt(sbi, DISABLE_ROLL_FORWARD);
+			if (!f2fs_readonly(sb))
+				return -EINVAL;
+			break;
 		case Opt_discard:
 			set_opt(sbi, DISCARD);
 			break;
-- 
2.1.1

RE: [PATCH 2/5] f2fs: support norecovery mount option

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: linux-fsdevel-owner@vger.kernel.org [mailto:linux-fsdevel-owner@vger.kernel.org] On
> Behalf Of Jaegeuk Kim
> Sent: Wednesday, January 28, 2015 7:32 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [PATCH 2/5] f2fs: support norecovery mount option
> 
> This patch adds a mount option, norecovery, which is mostly same as
> disable_roll_forward. The only difference is that norecovery should be activated
> with read-only mount option.
> 
> This can be used when user wants to check whether f2fs is mountable or not
> without any recovery process. (e.g., xfstests/200)
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/super.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index fbc7f5a..0ca1fb2 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -42,6 +42,7 @@ static struct kset *f2fs_kset;
>  enum {
>  	Opt_gc_background,
>  	Opt_disable_roll_forward,
> +	Opt_norecovery,
>  	Opt_discard,
>  	Opt_noheap,
>  	Opt_user_xattr,
> @@ -62,6 +63,7 @@ enum {
>  static match_table_t f2fs_tokens = {
>  	{Opt_gc_background, "background_gc=%s"},
>  	{Opt_disable_roll_forward, "disable_roll_forward"},
> +	{Opt_norecovery, "norecovery"},

IMO, it's better to show 'norecovery' in f2fs_show_options instead of showing
'disable_roll_forward', so user will know the details of mount option as we show.

>  	{Opt_discard, "discard"},
>  	{Opt_noheap, "no_heap"},
>  	{Opt_user_xattr, "user_xattr"},
> @@ -287,6 +289,12 @@ static int parse_options(struct super_block *sb, char *options)
>  		case Opt_disable_roll_forward:
>  			set_opt(sbi, DISABLE_ROLL_FORWARD);
>  			break;
> +		case Opt_norecovery:
> +			/* this option mounts f2fs with ro */
> +			set_opt(sbi, DISABLE_ROLL_FORWARD);

set_opt(sbi, NORECOVERY); and check this option before recover_fsync_data()?

It's better to add description for this "norecovery" option in Documentation.

Thanks,

> +			if (!f2fs_readonly(sb))
> +				return -EINVAL;
> +			break;
>  		case Opt_discard:
>  			set_opt(sbi, DISCARD);
>  			break;
> --
> 2.1.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] f2fs: support norecovery mount option

[Jaegeuk Kim]

Hi Chao,

On Thu, Jan 29, 2015 at 07:52:56PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: linux-fsdevel-owner@vger.kernel.org [mailto:linux-fsdevel-owner@vger.kernel.org] On
> > Behalf Of Jaegeuk Kim
> > Sent: Wednesday, January 28, 2015 7:32 AM
> > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > linux-f2fs-devel@lists.sourceforge.net
> > Cc: Jaegeuk Kim
> > Subject: [PATCH 2/5] f2fs: support norecovery mount option
> > 
> > This patch adds a mount option, norecovery, which is mostly same as
> > disable_roll_forward. The only difference is that norecovery should be activated
> > with read-only mount option.
> > 
> > This can be used when user wants to check whether f2fs is mountable or not
> > without any recovery process. (e.g., xfstests/200)
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/super.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index fbc7f5a..0ca1fb2 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -42,6 +42,7 @@ static struct kset *f2fs_kset;
> >  enum {
> >  	Opt_gc_background,
> >  	Opt_disable_roll_forward,
> > +	Opt_norecovery,
> >  	Opt_discard,
> >  	Opt_noheap,
> >  	Opt_user_xattr,
> > @@ -62,6 +63,7 @@ enum {
> >  static match_table_t f2fs_tokens = {
> >  	{Opt_gc_background, "background_gc=%s"},
> >  	{Opt_disable_roll_forward, "disable_roll_forward"},
> > +	{Opt_norecovery, "norecovery"},
> 
> IMO, it's better to show 'norecovery' in f2fs_show_options instead of showing
> 'disable_roll_forward', so user will know the details of mount option as we show.

Hmm, I did think like that at the first time.
But, as you guess, this option is totally same as ro,disable_roll_forward.
So, I just wanted to avoid user's confusing for the behaviors of this option.

> 
> >  	{Opt_discard, "discard"},
> >  	{Opt_noheap, "no_heap"},
> >  	{Opt_user_xattr, "user_xattr"},
> > @@ -287,6 +289,12 @@ static int parse_options(struct super_block *sb, char *options)
> >  		case Opt_disable_roll_forward:
> >  			set_opt(sbi, DISABLE_ROLL_FORWARD);
> >  			break;
> > +		case Opt_norecovery:
> > +			/* this option mounts f2fs with ro */
> > +			set_opt(sbi, DISABLE_ROLL_FORWARD);
> 
> set_opt(sbi, NORECOVERY); and check this option before recover_fsync_data()?
> 
> It's better to add description for this "norecovery" option in Documentation.

I'll do that.

Thanks,

> 
> Thanks,
> 
> > +			if (!f2fs_readonly(sb))
> > +				return -EINVAL;
> > +			break;
> >  		case Opt_discard:
> >  			set_opt(sbi, DISCARD);
> >  			break;
> > --
> > 2.1.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [f2fs-dev] [PATCH 2/5 v2] f2fs: support norecovery mount option

[Jaegeuk Kim]

Change log from v1:
 o add description for the new mount option in Documentation/filesystems/f2fs.txt

>From 079dbb14c7d91d90863c9be4d9337b8ec086db7e Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Fri, 23 Jan 2015 18:33:46 -0800
Subject: [PATCH] f2fs: support norecovery mount option

This patch adds a mount option, norecovery, which is mostly same as
disable_roll_forward. The only difference is that norecovery should be activated
with read-only mount option.

This can be used when user wants to check whether f2fs is mountable or not
without any recovery process. (e.g., xfstests/200)

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 Documentation/filesystems/f2fs.txt | 2 ++
 fs/f2fs/super.c                    | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/filesystems/f2fs.txt b/Documentation/filesystems/f2fs.txt
index e0950c4..6758aa3 100644
--- a/Documentation/filesystems/f2fs.txt
+++ b/Documentation/filesystems/f2fs.txt
@@ -106,6 +106,8 @@ background_gc=%s       Turn on/off cleaning operations, namely garbage
                        Default value for this option is on. So garbage
                        collection is on by default.
 disable_roll_forward   Disable the roll-forward recovery routine
+norecovery             Disable the roll-forward recovery routine, mounted read-
+                       only (i.e., -o ro,disable_roll_forward)
 discard                Issue discard/TRIM commands when a segment is cleaned.
 no_heap                Disable heap-style segment allocation which finds free
                        segments for data from the beginning of main area, while
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 47d9b04..a60fa1a 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -42,6 +42,7 @@ static struct kset *f2fs_kset;
 enum {
 	Opt_gc_background,
 	Opt_disable_roll_forward,
+	Opt_norecovery,
 	Opt_discard,
 	Opt_noheap,
 	Opt_user_xattr,
@@ -62,6 +63,7 @@ enum {
 static match_table_t f2fs_tokens = {
 	{Opt_gc_background, "background_gc=%s"},
 	{Opt_disable_roll_forward, "disable_roll_forward"},
+	{Opt_norecovery, "norecovery"},
 	{Opt_discard, "discard"},
 	{Opt_noheap, "no_heap"},
 	{Opt_user_xattr, "user_xattr"},
@@ -287,6 +289,12 @@ static int parse_options(struct super_block *sb, char *options)
 		case Opt_disable_roll_forward:
 			set_opt(sbi, DISABLE_ROLL_FORWARD);
 			break;
+		case Opt_norecovery:
+			/* this option mounts f2fs with ro */
+			set_opt(sbi, DISABLE_ROLL_FORWARD);
+			if (!f2fs_readonly(sb))
+				return -EINVAL;
+			break;
 		case Opt_discard:
 			set_opt(sbi, DISCARD);
 			break;
-- 
2.1.1

RE: [f2fs-dev] [PATCH 2/5 v2] f2fs: support norecovery mount option

[Chao Yu]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Friday, January 30, 2015 2:31 AM
> To: Chao Yu
> Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 2/5 v2] f2fs: support norecovery mount option
> 
> Change log from v1:
>  o add description for the new mount option in Documentation/filesystems/f2fs.txt
> 
> From 079dbb14c7d91d90863c9be4d9337b8ec086db7e Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <jaegeuk@kernel.org>
> Date: Fri, 23 Jan 2015 18:33:46 -0800
> Subject: [PATCH] f2fs: support norecovery mount option
> 
> This patch adds a mount option, norecovery, which is mostly same as
> disable_roll_forward. The only difference is that norecovery should be activated
> with read-only mount option.
> 
> This can be used when user wants to check whether f2fs is mountable or not
> without any recovery process. (e.g., xfstests/200)
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <chao2.yu@samsung.com>

RE: [PATCH 2/5] f2fs: support norecovery mount option

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Friday, January 30, 2015 2:28 AM
> To: Chao Yu
> Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [PATCH 2/5] f2fs: support norecovery mount option
> 
> Hi Chao,
> 
> On Thu, Jan 29, 2015 at 07:52:56PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > > -----Original Message-----
> > > From: linux-fsdevel-owner@vger.kernel.org [mailto:linux-fsdevel-owner@vger.kernel.org] On
> > > Behalf Of Jaegeuk Kim
> > > Sent: Wednesday, January 28, 2015 7:32 AM
> > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > > linux-f2fs-devel@lists.sourceforge.net
> > > Cc: Jaegeuk Kim
> > > Subject: [PATCH 2/5] f2fs: support norecovery mount option
> > >
> > > This patch adds a mount option, norecovery, which is mostly same as
> > > disable_roll_forward. The only difference is that norecovery should be activated
> > > with read-only mount option.
> > >
> > > This can be used when user wants to check whether f2fs is mountable or not
> > > without any recovery process. (e.g., xfstests/200)
> > >
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > ---
> > >  fs/f2fs/super.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > > index fbc7f5a..0ca1fb2 100644
> > > --- a/fs/f2fs/super.c
> > > +++ b/fs/f2fs/super.c
> > > @@ -42,6 +42,7 @@ static struct kset *f2fs_kset;
> > >  enum {
> > >  	Opt_gc_background,
> > >  	Opt_disable_roll_forward,
> > > +	Opt_norecovery,
> > >  	Opt_discard,
> > >  	Opt_noheap,
> > >  	Opt_user_xattr,
> > > @@ -62,6 +63,7 @@ enum {
> > >  static match_table_t f2fs_tokens = {
> > >  	{Opt_gc_background, "background_gc=%s"},
> > >  	{Opt_disable_roll_forward, "disable_roll_forward"},
> > > +	{Opt_norecovery, "norecovery"},
> >
> > IMO, it's better to show 'norecovery' in f2fs_show_options instead of showing
> > 'disable_roll_forward', so user will know the details of mount option as we show.
> 
> Hmm, I did think like that at the first time.
> But, as you guess, this option is totally same as ro,disable_roll_forward.
> So, I just wanted to avoid user's confusing for the behaviors of this option.

All right, I just worry about that if users could not find the key word 'norecovery' in
/etc/mtab as they think it should be there because they mount with it before. Not sure
this will happen.

> 
> >
> > >  	{Opt_discard, "discard"},
> > >  	{Opt_noheap, "no_heap"},
> > >  	{Opt_user_xattr, "user_xattr"},
> > > @@ -287,6 +289,12 @@ static int parse_options(struct super_block *sb, char *options)
> > >  		case Opt_disable_roll_forward:
> > >  			set_opt(sbi, DISABLE_ROLL_FORWARD);
> > >  			break;
> > > +		case Opt_norecovery:
> > > +			/* this option mounts f2fs with ro */
> > > +			set_opt(sbi, DISABLE_ROLL_FORWARD);
> >
> > set_opt(sbi, NORECOVERY); and check this option before recover_fsync_data()?
> >
> > It's better to add description for this "norecovery" option in Documentation.
> 
> I'll do that.

It will really help our users.
Thank you. :)

Regards,

> 
> Thanks,
> 
> >
> > Thanks,
> >
> > > +			if (!f2fs_readonly(sb))
> > > +				return -EINVAL;
> > > +			break;
> > >  		case Opt_discard:
> > >  			set_opt(sbi, DISCARD);
> > >  			break;
> > > --
> > > 2.1.1
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] f2fs: avoid write_checkpoint if f2fs is mounted readonly

Do not change any partition when f2fs is changed to readonly mode.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 79f8281..1ee6162 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1067,6 +1067,8 @@ void write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 		goto out;
 	if (unlikely(f2fs_cp_error(sbi)))
 		goto out;
+	if (f2fs_readonly(sbi->sb))
+		goto out;
 	if (block_operations(sbi))
 		goto out;
 
-- 
2.1.1

[PATCH 3/5] f2fs: avoid write_checkpoint if f2fs is mounted readonly

[Jaegeuk Kim]

Do not change any partition when f2fs is changed to readonly mode.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 79f8281..1ee6162 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1067,6 +1067,8 @@ void write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 		goto out;
 	if (unlikely(f2fs_cp_error(sbi)))
 		goto out;
+	if (f2fs_readonly(sbi->sb))
+		goto out;
 	if (block_operations(sbi))
 		goto out;
 
-- 
2.1.1


------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/

RE: [PATCH 3/5] f2fs: avoid write_checkpoint if f2fs is mounted readonly

[Chao Yu]

> -----Original Message-----
> From: linux-fsdevel-owner@vger.kernel.org [mailto:linux-fsdevel-owner@vger.kernel.org] On
> Behalf Of Jaegeuk Kim
> Sent: Wednesday, January 28, 2015 7:32 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [PATCH 3/5] f2fs: avoid write_checkpoint if f2fs is mounted readonly
> 
> Do not change any partition when f2fs is changed to readonly mode.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <chao2.yu@samsung.com>

[PATCH 4/5] f2fs: should fail mount when trying to recover data on read-only dev

[Jaegeuk Kim]

If device is read-only, we should not proceed data recovery.
But, if the previous checkpoint was done by normal clean shutdown, it's safe to
proceed the recovery, since there will be no data to be recovered.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/super.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 0ca1fb2..7039969 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1149,6 +1149,15 @@ try_onemore:
 
 	/* recover fsynced data */
 	if (!test_opt(sbi, DISABLE_ROLL_FORWARD)) {
+		/*
+		 * mount should be failed, when device has readonly mode, and
+		 * previous checkpoint was not done by clean system shutdown.
+		 */
+		if (bdev_read_only(sb->s_bdev) &&
+				!is_set_ckpt_flags(sbi->ckpt, CP_UMOUNT_FLAG)) {
+			err = -EROFS;
+			goto free_kobj;
+		}
 		err = recover_fsync_data(sbi);
 		if (err) {
 			f2fs_msg(sb, KERN_ERR,
-- 
2.1.1

[PATCH 4/5] f2fs: should fail mount when trying to recover data on read-only dev

[Jaegeuk Kim]

If device is read-only, we should not proceed data recovery.
But, if the previous checkpoint was done by normal clean shutdown, it's safe to
proceed the recovery, since there will be no data to be recovered.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/super.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 0ca1fb2..7039969 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1149,6 +1149,15 @@ try_onemore:
 
 	/* recover fsynced data */
 	if (!test_opt(sbi, DISABLE_ROLL_FORWARD)) {
+		/*
+		 * mount should be failed, when device has readonly mode, and
+		 * previous checkpoint was not done by clean system shutdown.
+		 */
+		if (bdev_read_only(sb->s_bdev) &&
+				!is_set_ckpt_flags(sbi->ckpt, CP_UMOUNT_FLAG)) {
+			err = -EROFS;
+			goto free_kobj;
+		}
 		err = recover_fsync_data(sbi);
 		if (err) {
 			f2fs_msg(sb, KERN_ERR,
-- 
2.1.1


------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/

RE: [PATCH 4/5] f2fs: should fail mount when trying to recover data on read-only dev

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: linux-fsdevel-owner@vger.kernel.org [mailto:linux-fsdevel-owner@vger.kernel.org] On
> Behalf Of Jaegeuk Kim
> Sent: Wednesday, January 28, 2015 7:32 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [PATCH 4/5] f2fs: should fail mount when trying to recover data on read-only dev
> 
> If device is read-only, we should not proceed data recovery.
> But, if the previous checkpoint was done by normal clean shutdown, it's safe to
> proceed the recovery, since there will be no data to be recovered.

Now, with 'fastboot' option, f2fs will write cp with CP_UMOUNT_FLAG, but we count not
guarantee there is no abnormally power cut after data fsynced.

Thanks,

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/super.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 0ca1fb2..7039969 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1149,6 +1149,15 @@ try_onemore:
> 
>  	/* recover fsynced data */
>  	if (!test_opt(sbi, DISABLE_ROLL_FORWARD)) {
> +		/*
> +		 * mount should be failed, when device has readonly mode, and
> +		 * previous checkpoint was not done by clean system shutdown.
> +		 */
> +		if (bdev_read_only(sb->s_bdev) &&
> +				!is_set_ckpt_flags(sbi->ckpt, CP_UMOUNT_FLAG)) {
> +			err = -EROFS;
> +			goto free_kobj;
> +		}
>  		err = recover_fsync_data(sbi);
>  		if (err) {
>  			f2fs_msg(sb, KERN_ERR,
> --
> 2.1.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] f2fs: should fail mount when trying to recover data on read-only dev

[Jaegeuk Kim]

Hi Chao,

This is another patch to fix that.

>From d241924043778d0fe01e9020d5771cc42cf246e6 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Thu, 29 Jan 2015 11:45:33 -0800
Subject: [PATCH] f2fs: split UMOUNT and FASTBOOT flags

This patch adds FASTBOOT flag into checkpoint as follows.

 - CP_UMOUNT_FLAG is set when system is umounted.
 - CP_FASTBOOT_FLAG is set when intermediate checkpoint having node summaries
   was done.

So, if you get CP_UMOUNT_FLAG from checkpoint, the system was umounted cleanly.
Instead, if there was sudden-power-off, you can get CP_FASTBOOT_FLAG or nothing.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c        | 19 +++++++++++++------
 fs/f2fs/f2fs.h              | 23 +++++++++++++++++++++++
 fs/f2fs/gc.c                |  3 +--
 fs/f2fs/segment.c           | 11 +++++------
 fs/f2fs/super.c             |  5 ++---
 include/linux/f2fs_fs.h     |  1 +
 include/trace/events/f2fs.h |  1 +
 7 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 22165fb..f7cdcad 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -956,17 +956,24 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	ckpt->cp_pack_start_sum = cpu_to_le32(1 + cp_payload_blks +
 			orphan_blocks);
 
-	if (cpc->reason == CP_UMOUNT) {
-		set_ckpt_flags(ckpt, CP_UMOUNT_FLAG);
+	if (__remain_node_summaries(cpc->reason))
 		ckpt->cp_pack_total_block_count = cpu_to_le32(F2FS_CP_PACKS+
 				cp_payload_blks + data_sum_blocks +
 				orphan_blocks + NR_CURSEG_NODE_TYPE);
-	} else {
-		clear_ckpt_flags(ckpt, CP_UMOUNT_FLAG);
+	else
 		ckpt->cp_pack_total_block_count = cpu_to_le32(F2FS_CP_PACKS +
 				cp_payload_blks + data_sum_blocks +
 				orphan_blocks);
-	}
+
+	if (cpc->reason == CP_UMOUNT)
+		set_ckpt_flags(ckpt, CP_UMOUNT_FLAG);
+	else
+		clear_ckpt_flags(ckpt, CP_UMOUNT_FLAG);
+
+	if (cpc->reason == CP_FASTBOOT)
+		set_ckpt_flags(ckpt, CP_FASTBOOT_FLAG);
+	else
+		clear_ckpt_flags(ckpt, CP_FASTBOOT_FLAG);
 
 	if (orphan_num)
 		set_ckpt_flags(ckpt, CP_ORPHAN_PRESENT_FLAG);
@@ -1010,7 +1017,7 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	write_data_summaries(sbi, start_blk);
 	start_blk += data_sum_blocks;
-	if (cpc->reason == CP_UMOUNT) {
+	if (__remain_node_summaries(cpc->reason)) {
 		write_node_summaries(sbi, start_blk);
 		start_blk += NR_CURSEG_NODE_TYPE;
 	}
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index b629408..0a2d1ff 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -100,6 +100,7 @@ enum {
 
 enum {
 	CP_UMOUNT,
+	CP_FASTBOOT,
 	CP_SYNC,
 	CP_DISCARD,
 };
@@ -788,6 +789,28 @@ static inline void f2fs_unlock_all(struct f2fs_sb_info *sbi)
 	up_write(&sbi->cp_rwsem);
 }
 
+static inline int __get_cp_reason(struct f2fs_sb_info *sbi)
+{
+	int reason = CP_SYNC;
+
+	if (test_opt(sbi, FASTBOOT))
+		reason = CP_FASTBOOT;
+	if (is_sbi_flag_set(sbi, SBI_IS_CLOSE))
+		reason = CP_UMOUNT;
+	return reason;
+}
+
+static inline bool __remain_node_summaries(int reason)
+{
+	return (reason == CP_UMOUNT || reason == CP_FASTBOOT);
+}
+
+static inline bool __exist_node_summaries(struct f2fs_sb_info *sbi)
+{
+	return (is_set_ckpt_flags(F2FS_CKPT(sbi), CP_UMOUNT_FLAG) ||
+			is_set_ckpt_flags(F2FS_CKPT(sbi), CP_FASTBOOT_FLAG));
+}
+
 /*
  * Check whether the given nid is within node id range.
  */
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index ba89e27..76adbc3 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -698,8 +698,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi)
 		.iroot = RADIX_TREE_INIT(GFP_NOFS),
 	};
 
-	cpc.reason = test_opt(sbi, FASTBOOT) ? CP_UMOUNT : CP_SYNC;
-
+	cpc.reason = __get_cp_reason(sbi);
 gc_more:
 	if (unlikely(!(sbi->sb->s_flags & MS_ACTIVE)))
 		goto stop;
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 31c4e57..5ea57ec 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1401,7 +1401,7 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type)
 		segno = le32_to_cpu(ckpt->cur_data_segno[type]);
 		blk_off = le16_to_cpu(ckpt->cur_data_blkoff[type -
 							CURSEG_HOT_DATA]);
-		if (is_set_ckpt_flags(ckpt, CP_UMOUNT_FLAG))
+		if (__exist_node_summaries(sbi))
 			blk_addr = sum_blk_addr(sbi, NR_CURSEG_TYPE, type);
 		else
 			blk_addr = sum_blk_addr(sbi, NR_CURSEG_DATA_TYPE, type);
@@ -1410,7 +1410,7 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type)
 							CURSEG_HOT_NODE]);
 		blk_off = le16_to_cpu(ckpt->cur_node_blkoff[type -
 							CURSEG_HOT_NODE]);
-		if (is_set_ckpt_flags(ckpt, CP_UMOUNT_FLAG))
+		if (__exist_node_summaries(sbi))
 			blk_addr = sum_blk_addr(sbi, NR_CURSEG_NODE_TYPE,
 							type - CURSEG_HOT_NODE);
 		else
@@ -1421,7 +1421,7 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type)
 	sum = (struct f2fs_summary_block *)page_address(new);
 
 	if (IS_NODESEG(type)) {
-		if (is_set_ckpt_flags(ckpt, CP_UMOUNT_FLAG)) {
+		if (__exist_node_summaries(sbi)) {
 			struct f2fs_summary *ns = &sum->entries[0];
 			int i;
 			for (i = 0; i < sbi->blocks_per_seg; i++, ns++) {
@@ -1470,7 +1470,7 @@ static int restore_curseg_summaries(struct f2fs_sb_info *sbi)
 		type = CURSEG_HOT_NODE;
 	}
 
-	if (is_set_ckpt_flags(F2FS_CKPT(sbi), CP_UMOUNT_FLAG))
+	if (__exist_node_summaries(sbi))
 		ra_meta_pages(sbi, sum_blk_addr(sbi, NR_CURSEG_TYPE, type),
 					NR_CURSEG_TYPE - type, META_CP);
 
@@ -1567,8 +1567,7 @@ void write_data_summaries(struct f2fs_sb_info *sbi, block_t start_blk)
 
 void write_node_summaries(struct f2fs_sb_info *sbi, block_t start_blk)
 {
-	if (is_set_ckpt_flags(F2FS_CKPT(sbi), CP_UMOUNT_FLAG))
-		write_normal_summaries(sbi, start_blk, CURSEG_HOT_NODE);
+	write_normal_summaries(sbi, start_blk, CURSEG_HOT_NODE);
 }
 
 int lookup_journal_in_cursum(struct f2fs_summary_block *sum, int type,
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index a60fa1a..03d1b9c 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -500,9 +500,8 @@ int f2fs_sync_fs(struct super_block *sb, int sync)
 	if (sync) {
 		struct cp_control cpc;
 
-		cpc.reason = (test_opt(sbi, FASTBOOT) ||
-					is_sbi_flag_set(sbi, SBI_IS_CLOSE)) ?
-						CP_UMOUNT : CP_SYNC;
+		cpc.reason = __get_cp_reason(sbi);
+
 		mutex_lock(&sbi->gc_mutex);
 		write_checkpoint(sbi, &cpc);
 		mutex_unlock(&sbi->gc_mutex);
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index 295acfa..cfe771b 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -87,6 +87,7 @@ struct f2fs_super_block {
 /*
  * For checkpoint
  */
+#define CP_FASTBOOT_FLAG	0x00000020
 #define CP_FSCK_FLAG		0x00000010
 #define CP_ERROR_FLAG		0x00000008
 #define CP_COMPACT_SUM_FLAG	0x00000004
diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
index 5e1c029..6962982 100644
--- a/include/trace/events/f2fs.h
+++ b/include/trace/events/f2fs.h
@@ -72,6 +72,7 @@
 #define show_cpreason(type)						\
 	__print_symbolic(type,						\
 		{ CP_UMOUNT,	"Umount" },				\
+		{ CP_FASTBOOT,	"Fastboot" },				\
 		{ CP_SYNC,	"Sync" },				\
 		{ CP_DISCARD,	"Discard" })
 
-- 
2.1.1

RE: [PATCH 4/5] f2fs: should fail mount when trying to recover data on read-only dev

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Friday, January 30, 2015 5:40 AM
> To: Chao Yu
> Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [PATCH 4/5] f2fs: should fail mount when trying to recover data on read-only dev
> 
> Hi Chao,
> 
> This is another patch to fix that.
> 
> From d241924043778d0fe01e9020d5771cc42cf246e6 Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <jaegeuk@kernel.org>
> Date: Thu, 29 Jan 2015 11:45:33 -0800
> Subject: [PATCH] f2fs: split UMOUNT and FASTBOOT flags
> 
> This patch adds FASTBOOT flag into checkpoint as follows.
> 
>  - CP_UMOUNT_FLAG is set when system is umounted.
>  - CP_FASTBOOT_FLAG is set when intermediate checkpoint having node summaries
>    was done.
> 
> So, if you get CP_UMOUNT_FLAG from checkpoint, the system was umounted cleanly.
> Instead, if there was sudden-power-off, you can get CP_FASTBOOT_FLAG or nothing.

Nice work, it looks good to me, and this can fix that issue. :)

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <chao2.yu@samsung.com>

RE: [PATCH 4/5] f2fs: should fail mount when trying to recover data on read-only dev

[Chao Yu]

> -----Original Message-----
> From: linux-fsdevel-owner@vger.kernel.org [mailto:linux-fsdevel-owner@vger.kernel.org] On
> Behalf Of Jaegeuk Kim
> Sent: Wednesday, January 28, 2015 7:32 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [PATCH 4/5] f2fs: should fail mount when trying to recover data on read-only dev
> 
> If device is read-only, we should not proceed data recovery.
> But, if the previous checkpoint was done by normal clean shutdown, it's safe to
> proceed the recovery, since there will be no data to be recovered.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <chao2.yu@samsung.com>

[PATCH 5/5] f2fs: introduce a batched trim

[Jaegeuk Kim]

This patch introduces a batched trimming feature, which submits split discard
commands.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h    |  1 +
 fs/f2fs/segment.c | 15 ++++++++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index c0b83d6..ec4d16b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -104,6 +104,7 @@ enum {
 	CP_DISCARD,
 };
 
+#define BATCHED_TRIM_SEGMENTS	10
 struct cp_control {
 	int reason;
 	__u64 trim_start;
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 31c4e57..6c9c784 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1066,14 +1066,19 @@ int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range *range)
 	end_segno = (end >= MAX_BLKADDR(sbi)) ? MAIN_SEGS(sbi) - 1 :
 						GET_SEGNO(sbi, end);
 	cpc.reason = CP_DISCARD;
-	cpc.trim_start = start_segno;
-	cpc.trim_end = end_segno;
 	cpc.trim_minlen = range->minlen >> sbi->log_blocksize;
 
 	/* do checkpoint to issue discard commands safely */
-	mutex_lock(&sbi->gc_mutex);
-	write_checkpoint(sbi, &cpc);
-	mutex_unlock(&sbi->gc_mutex);
+	for (; start_segno <= end_segno;
+				start_segno += BATCHED_TRIM_SEGMENTS + 1) {
+		cpc.trim_start = start_segno;
+		cpc.trim_end = min_t(unsigned int,
+				start_segno + BATCHED_TRIM_SEGMENTS, end_segno);
+
+		mutex_lock(&sbi->gc_mutex);
+		write_checkpoint(sbi, &cpc);
+		mutex_unlock(&sbi->gc_mutex);
+	}
 out:
 	range->len = cpc.trimmed << sbi->log_blocksize;
 	return 0;
-- 
2.1.1

[PATCH 5/5] f2fs: introduce a batched trim

[Jaegeuk Kim]

This patch introduces a batched trimming feature, which submits split discard
commands.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h    |  1 +
 fs/f2fs/segment.c | 15 ++++++++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index c0b83d6..ec4d16b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -104,6 +104,7 @@ enum {
 	CP_DISCARD,
 };
 
+#define BATCHED_TRIM_SEGMENTS	10
 struct cp_control {
 	int reason;
 	__u64 trim_start;
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 31c4e57..6c9c784 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1066,14 +1066,19 @@ int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range *range)
 	end_segno = (end >= MAX_BLKADDR(sbi)) ? MAIN_SEGS(sbi) - 1 :
 						GET_SEGNO(sbi, end);
 	cpc.reason = CP_DISCARD;
-	cpc.trim_start = start_segno;
-	cpc.trim_end = end_segno;
 	cpc.trim_minlen = range->minlen >> sbi->log_blocksize;
 
 	/* do checkpoint to issue discard commands safely */
-	mutex_lock(&sbi->gc_mutex);
-	write_checkpoint(sbi, &cpc);
-	mutex_unlock(&sbi->gc_mutex);
+	for (; start_segno <= end_segno;
+				start_segno += BATCHED_TRIM_SEGMENTS + 1) {
+		cpc.trim_start = start_segno;
+		cpc.trim_end = min_t(unsigned int,
+				start_segno + BATCHED_TRIM_SEGMENTS, end_segno);
+
+		mutex_lock(&sbi->gc_mutex);
+		write_checkpoint(sbi, &cpc);
+		mutex_unlock(&sbi->gc_mutex);
+	}
 out:
 	range->len = cpc.trimmed << sbi->log_blocksize;
 	return 0;
-- 
2.1.1


------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/

RE: [PATCH 5/5] f2fs: introduce a batched trim

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: linux-fsdevel-owner@vger.kernel.org [mailto:linux-fsdevel-owner@vger.kernel.org] On
> Behalf Of Jaegeuk Kim
> Sent: Wednesday, January 28, 2015 7:32 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [PATCH 5/5] f2fs: introduce a batched trim
> 
> This patch introduces a batched trimming feature, which submits split discard
> commands.

I didn't get it, why we should split discard commands. :(

Does smaller discarding for flash shows better performance or effect or safety?
Can you please explain more about this patch?

Thanks,

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/f2fs.h    |  1 +
>  fs/f2fs/segment.c | 15 ++++++++++-----
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index c0b83d6..ec4d16b 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -104,6 +104,7 @@ enum {
>  	CP_DISCARD,
>  };
> 
> +#define BATCHED_TRIM_SEGMENTS	10
>  struct cp_control {
>  	int reason;
>  	__u64 trim_start;
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 31c4e57..6c9c784 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1066,14 +1066,19 @@ int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range *range)
>  	end_segno = (end >= MAX_BLKADDR(sbi)) ? MAIN_SEGS(sbi) - 1 :
>  						GET_SEGNO(sbi, end);
>  	cpc.reason = CP_DISCARD;
> -	cpc.trim_start = start_segno;
> -	cpc.trim_end = end_segno;
>  	cpc.trim_minlen = range->minlen >> sbi->log_blocksize;
> 
>  	/* do checkpoint to issue discard commands safely */
> -	mutex_lock(&sbi->gc_mutex);
> -	write_checkpoint(sbi, &cpc);
> -	mutex_unlock(&sbi->gc_mutex);
> +	for (; start_segno <= end_segno;
> +				start_segno += BATCHED_TRIM_SEGMENTS + 1) {
> +		cpc.trim_start = start_segno;
> +		cpc.trim_end = min_t(unsigned int,
> +				start_segno + BATCHED_TRIM_SEGMENTS, end_segno);
> +
> +		mutex_lock(&sbi->gc_mutex);
> +		write_checkpoint(sbi, &cpc);
> +		mutex_unlock(&sbi->gc_mutex);
> +	}
>  out:
>  	range->len = cpc.trimmed << sbi->log_blocksize;
>  	return 0;
> --
> 2.1.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] f2fs: introduce a batched trim

[Jaegeuk Kim]

Hi Chao,

On Thu, Jan 29, 2015 at 08:38:30PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: linux-fsdevel-owner@vger.kernel.org [mailto:linux-fsdevel-owner@vger.kernel.org] On
> > Behalf Of Jaegeuk Kim
> > Sent: Wednesday, January 28, 2015 7:32 AM
> > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > linux-f2fs-devel@lists.sourceforge.net
> > Cc: Jaegeuk Kim
> > Subject: [PATCH 5/5] f2fs: introduce a batched trim
> > 
> > This patch introduces a batched trimming feature, which submits split discard
> > commands.
> 
> I didn't get it, why we should split discard commands. :(
> 
> Does smaller discarding for flash shows better performance or effect or safety?
> Can you please explain more about this patch?

This is to avoid long latency due to huge trim commands.
If fstrim was triggered ranging from 0 to the end of device, we should lock
all the checkpoint-related mutexes, resulting in very long latency.

Thanks,

> 
> Thanks,
> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/f2fs.h    |  1 +
> >  fs/f2fs/segment.c | 15 ++++++++++-----
> >  2 files changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index c0b83d6..ec4d16b 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -104,6 +104,7 @@ enum {
> >  	CP_DISCARD,
> >  };
> > 
> > +#define BATCHED_TRIM_SEGMENTS	10
> >  struct cp_control {
> >  	int reason;
> >  	__u64 trim_start;
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 31c4e57..6c9c784 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -1066,14 +1066,19 @@ int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range *range)
> >  	end_segno = (end >= MAX_BLKADDR(sbi)) ? MAIN_SEGS(sbi) - 1 :
> >  						GET_SEGNO(sbi, end);
> >  	cpc.reason = CP_DISCARD;
> > -	cpc.trim_start = start_segno;
> > -	cpc.trim_end = end_segno;
> >  	cpc.trim_minlen = range->minlen >> sbi->log_blocksize;
> > 
> >  	/* do checkpoint to issue discard commands safely */
> > -	mutex_lock(&sbi->gc_mutex);
> > -	write_checkpoint(sbi, &cpc);
> > -	mutex_unlock(&sbi->gc_mutex);
> > +	for (; start_segno <= end_segno;
> > +				start_segno += BATCHED_TRIM_SEGMENTS + 1) {
> > +		cpc.trim_start = start_segno;
> > +		cpc.trim_end = min_t(unsigned int,
> > +				start_segno + BATCHED_TRIM_SEGMENTS, end_segno);
> > +
> > +		mutex_lock(&sbi->gc_mutex);
> > +		write_checkpoint(sbi, &cpc);
> > +		mutex_unlock(&sbi->gc_mutex);
> > +	}
> >  out:
> >  	range->len = cpc.trimmed << sbi->log_blocksize;
> >  	return 0;
> > --
> > 2.1.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 5/5] f2fs: introduce a batched trim

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Friday, January 30, 2015 5:41 AM
> To: Chao Yu
> Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [PATCH 5/5] f2fs: introduce a batched trim
> 
> Hi Chao,
> 
> On Thu, Jan 29, 2015 at 08:38:30PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > > -----Original Message-----
> > > From: linux-fsdevel-owner@vger.kernel.org [mailto:linux-fsdevel-owner@vger.kernel.org] On
> > > Behalf Of Jaegeuk Kim
> > > Sent: Wednesday, January 28, 2015 7:32 AM
> > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > > linux-f2fs-devel@lists.sourceforge.net
> > > Cc: Jaegeuk Kim
> > > Subject: [PATCH 5/5] f2fs: introduce a batched trim
> > >
> > > This patch introduces a batched trimming feature, which submits split discard
> > > commands.
> >
> > I didn't get it, why we should split discard commands. :(
> >
> > Does smaller discarding for flash shows better performance or effect or safety?
> > Can you please explain more about this patch?
> 
> This is to avoid long latency due to huge trim commands.
> If fstrim was triggered ranging from 0 to the end of device, we should lock
> all the checkpoint-related mutexes, resulting in very long latency.

Ah.. thanks for your explanation, how about adding above description into commit
log?

> 
> Thanks,
> 
> >
> > Thanks,
> >
> > >
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > ---
> > >  fs/f2fs/f2fs.h    |  1 +
> > >  fs/f2fs/segment.c | 15 ++++++++++-----
> > >  2 files changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index c0b83d6..ec4d16b 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -104,6 +104,7 @@ enum {
> > >  	CP_DISCARD,
> > >  };
> > >
> > > +#define BATCHED_TRIM_SEGMENTS	10

Our macro here shows one batched trim discard 10 segments, but actually we
discard 11 segments each time in f2fs_trim_fs, how about making them consistent?

One other point is that, I guess gc in FTL will be triggered each time after
discard command was issued, but if our basic size of one batched trim is not
multiple of gc unit size in FTL, efficiency of gc in FTL will be lower.

Now that f2fs has nature advantage that we can align section size to gc unit
size in FTL. Why not trying to op flash with section size as much as possible?

So how about use this:

#define BATCHED_TRIM_SECTIONS_SHIFT	4

our trim size: section size << BATCHED_TRIM_SECTIONS_SHIFT

Thanks,

> > >  struct cp_control {
> > >  	int reason;
> > >  	__u64 trim_start;
> > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > index 31c4e57..6c9c784 100644
> > > --- a/fs/f2fs/segment.c
> > > +++ b/fs/f2fs/segment.c
> > > @@ -1066,14 +1066,19 @@ int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range
> *range)
> > >  	end_segno = (end >= MAX_BLKADDR(sbi)) ? MAIN_SEGS(sbi) - 1 :
> > >  						GET_SEGNO(sbi, end);
> > >  	cpc.reason = CP_DISCARD;
> > > -	cpc.trim_start = start_segno;
> > > -	cpc.trim_end = end_segno;
> > >  	cpc.trim_minlen = range->minlen >> sbi->log_blocksize;
> > >
> > >  	/* do checkpoint to issue discard commands safely */
> > > -	mutex_lock(&sbi->gc_mutex);
> > > -	write_checkpoint(sbi, &cpc);
> > > -	mutex_unlock(&sbi->gc_mutex);
> > > +	for (; start_segno <= end_segno;
> > > +				start_segno += BATCHED_TRIM_SEGMENTS + 1) {
> > > +		cpc.trim_start = start_segno;
> > > +		cpc.trim_end = min_t(unsigned int,
> > > +				start_segno + BATCHED_TRIM_SEGMENTS, end_segno);
> > > +
> > > +		mutex_lock(&sbi->gc_mutex);
> > > +		write_checkpoint(sbi, &cpc);
> > > +		mutex_unlock(&sbi->gc_mutex);
> > > +	}
> > >  out:
> > >  	range->len = cpc.trimmed << sbi->log_blocksize;
> > >  	return 0;
> > > --
> > > 2.1.1
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5 v2] f2fs: introduce a batched trim

[Jaegeuk Kim]

Change long from v1:
 o add description
 o change the # of batched segments suggested by Chao
 o make consistent for # of batched segments

This patch introduces a batched trimming feature, which submits split discard
commands.

This patch introduces a batched trimming feature, which submits split discard
commands.

This is to avoid long latency due to huge trim commands.
If fstrim was triggered ranging from 0 to the end of device, we should lock
all the checkpoint-related mutexes, resulting in very long latency.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h    |  2 ++
 fs/f2fs/segment.c | 16 +++++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 8231a59..ec5e66f 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -105,6 +105,8 @@ enum {
 	CP_DISCARD,
 };
 
+#define BATCHED_TRIM_SEGMENTS(sbi)	(((sbi)->segs_per_sec) << 5)
+
 struct cp_control {
 	int reason;
 	__u64 trim_start;
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 5ea57ec..b85bb97 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1066,14 +1066,20 @@ int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range *range)
 	end_segno = (end >= MAX_BLKADDR(sbi)) ? MAIN_SEGS(sbi) - 1 :
 						GET_SEGNO(sbi, end);
 	cpc.reason = CP_DISCARD;
-	cpc.trim_start = start_segno;
-	cpc.trim_end = end_segno;
 	cpc.trim_minlen = range->minlen >> sbi->log_blocksize;
 
 	/* do checkpoint to issue discard commands safely */
-	mutex_lock(&sbi->gc_mutex);
-	write_checkpoint(sbi, &cpc);
-	mutex_unlock(&sbi->gc_mutex);
+	for (; start_segno <= end_segno;
+				start_segno += BATCHED_TRIM_SEGMENTS(sbi)) {
+		cpc.trim_start = start_segno;
+		cpc.trim_end = min_t(unsigned int,
+				start_segno + BATCHED_TRIM_SEGMENTS (sbi) - 1,
+				end_segno);
+
+		mutex_lock(&sbi->gc_mutex);
+		write_checkpoint(sbi, &cpc);
+		mutex_unlock(&sbi->gc_mutex);
+	}
 out:
 	range->len = cpc.trimmed << sbi->log_blocksize;
 	return 0;
-- 
2.1.1

Re: [f2fs-dev] [PATCH 5/5 v2] f2fs: introduce a batched trim

[Changman Lee]

Hi Jaegeuk,

IMHO, it looks better user could decide the size of trim considering latency of trim.
Otherwise, additional checkpoints user doesn't want will occur.

Regards,
Changman

On Mon, Feb 02, 2015 at 03:29:25PM -0800, Jaegeuk Kim wrote:
> Change long from v1:
>  o add description
>  o change the # of batched segments suggested by Chao
>  o make consistent for # of batched segments
> 
> This patch introduces a batched trimming feature, which submits split discard
> commands.
> 
> This patch introduces a batched trimming feature, which submits split discard
> commands.
> 
> This is to avoid long latency due to huge trim commands.
> If fstrim was triggered ranging from 0 to the end of device, we should lock
> all the checkpoint-related mutexes, resulting in very long latency.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/f2fs.h    |  2 ++
>  fs/f2fs/segment.c | 16 +++++++++++-----
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 8231a59..ec5e66f 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -105,6 +105,8 @@ enum {
>  	CP_DISCARD,
>  };
>  
> +#define BATCHED_TRIM_SEGMENTS(sbi)	(((sbi)->segs_per_sec) << 5)
> +
>  struct cp_control {
>  	int reason;
>  	__u64 trim_start;
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 5ea57ec..b85bb97 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1066,14 +1066,20 @@ int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range *range)
>  	end_segno = (end >= MAX_BLKADDR(sbi)) ? MAIN_SEGS(sbi) - 1 :
>  						GET_SEGNO(sbi, end);
>  	cpc.reason = CP_DISCARD;
> -	cpc.trim_start = start_segno;
> -	cpc.trim_end = end_segno;
>  	cpc.trim_minlen = range->minlen >> sbi->log_blocksize;
>  
>  	/* do checkpoint to issue discard commands safely */
> -	mutex_lock(&sbi->gc_mutex);
> -	write_checkpoint(sbi, &cpc);
> -	mutex_unlock(&sbi->gc_mutex);
> +	for (; start_segno <= end_segno;
> +				start_segno += BATCHED_TRIM_SEGMENTS(sbi)) {
> +		cpc.trim_start = start_segno;
> +		cpc.trim_end = min_t(unsigned int,
> +				start_segno + BATCHED_TRIM_SEGMENTS (sbi) - 1,
> +				end_segno);
> +
> +		mutex_lock(&sbi->gc_mutex);
> +		write_checkpoint(sbi, &cpc);
> +		mutex_unlock(&sbi->gc_mutex);
> +	}
>  out:
>  	range->len = cpc.trimmed << sbi->log_blocksize;
>  	return 0;
> -- 
> 2.1.1
> 
> 
> ------------------------------------------------------------------------------
> Dive into the World of Parallel Programming. The Go Parallel Website,
> sponsored by Intel and developed in partnership with Slashdot Media, is your
> hub for all things parallel software development, from weekly thought
> leadership blogs to news, videos, case studies, tutorials and more. Take a
> look and join the conversation now. http://goparallel.sourceforge.net/
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 5/5 v3] f2fs: introduce a batched trim

[Jaegeuk Kim]

Hi Changman,

Good idea!

Change log from v2:
 o add sysfs to change the # of sections for trimming.
 
Change log from v1:
 o add description
 o change the # of batched segments suggested by Chao
 o make consistent for # of batched segments

This patch introduces a batched trimming feature, which submits split discard
commands.

This is to avoid long latency due to huge trim commands.
If fstrim was triggered ranging from 0 to the end of device, we should lock
all the checkpoint-related mutexes, resulting in very long latency.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 Documentation/ABI/testing/sysfs-fs-f2fs |  6 ++++++
 Documentation/filesystems/f2fs.txt      |  4 ++++
 fs/f2fs/f2fs.h                          |  7 +++++++
 fs/f2fs/segment.c                       | 18 +++++++++++++-----
 fs/f2fs/super.c                         |  2 ++
 5 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
index 6f9157f..2c4cc42 100644
--- a/Documentation/ABI/testing/sysfs-fs-f2fs
+++ b/Documentation/ABI/testing/sysfs-fs-f2fs
@@ -74,3 +74,9 @@ Date:		March 2014
 Contact:	"Jaegeuk Kim" <jaegeuk.kim@samsung.com>
 Description:
 		 Controls the memory footprint used by f2fs.
+
+What:		/sys/fs/f2fs/<disk>/trim_sections
+Date:		February 2015
+Contact:	"Jaegeuk Kim" <jaegeuk@kernel.org>
+Description:
+		 Controls the trimming rate in batch mode.
diff --git a/Documentation/filesystems/f2fs.txt b/Documentation/filesystems/f2fs.txt
index 6758aa3..dac11d7 100644
--- a/Documentation/filesystems/f2fs.txt
+++ b/Documentation/filesystems/f2fs.txt
@@ -199,6 +199,10 @@ Files in /sys/fs/f2fs/<devname>
 			      checkpoint is triggered, and issued during the
 			      checkpoint. By default, it is disabled with 0.
 
+ trim_sections                This parameter controls the number of sections
+                              to be trimmed out in batch mode when FITRIM
+                              conducts. 32 sections is set by default.
+
  ipu_policy                   This parameter controls the policy of in-place
                               updates in f2fs. There are five policies:
                                0x01: F2FS_IPU_FORCE, 0x02: F2FS_IPU_SSR,
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 964c240..6f57da1 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -105,6 +105,10 @@ enum {
 	CP_DISCARD,
 };
 
+#define DEF_BATCHED_TRIM_SECTIONS	32
+#define BATCHED_TRIM_SEGMENTS(sbi)	\
+		(SM_I(sbi)->trim_sections * (sbi)->segs_per_sec)
+
 struct cp_control {
 	int reason;
 	__u64 trim_start;
@@ -448,6 +452,9 @@ struct f2fs_sm_info {
 	int nr_discards;			/* # of discards in the list */
 	int max_discards;			/* max. discards to be issued */
 
+	/* for batched trimming */
+	int trim_sections;			/* # of sections to trim */
+
 	struct list_head sit_entry_set;	/* sit entry set list */
 
 	unsigned int ipu_policy;	/* in-place-update policy */
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 5ea57ec..c542f63 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1066,14 +1066,20 @@ int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range *range)
 	end_segno = (end >= MAX_BLKADDR(sbi)) ? MAIN_SEGS(sbi) - 1 :
 						GET_SEGNO(sbi, end);
 	cpc.reason = CP_DISCARD;
-	cpc.trim_start = start_segno;
-	cpc.trim_end = end_segno;
 	cpc.trim_minlen = range->minlen >> sbi->log_blocksize;
 
 	/* do checkpoint to issue discard commands safely */
-	mutex_lock(&sbi->gc_mutex);
-	write_checkpoint(sbi, &cpc);
-	mutex_unlock(&sbi->gc_mutex);
+	for (; start_segno <= end_segno;
+				start_segno += BATCHED_TRIM_SEGMENTS(sbi)) {
+		cpc.trim_start = start_segno;
+		cpc.trim_end = min_t(unsigned int,
+				start_segno + BATCHED_TRIM_SEGMENTS (sbi) - 1,
+				end_segno);
+
+		mutex_lock(&sbi->gc_mutex);
+		write_checkpoint(sbi, &cpc);
+		mutex_unlock(&sbi->gc_mutex);
+	}
 out:
 	range->len = cpc.trimmed << sbi->log_blocksize;
 	return 0;
@@ -2127,6 +2133,8 @@ int build_segment_manager(struct f2fs_sb_info *sbi)
 	sm_info->nr_discards = 0;
 	sm_info->max_discards = 0;
 
+	sm_info->trim_sections = DEF_BATCHED_TRIM_SECTIONS;
+
 	INIT_LIST_HEAD(&sm_info->sit_entry_set);
 
 	if (test_opt(sbi, FLUSH_MERGE) && !f2fs_readonly(sbi->sb)) {
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 1e92c2e..f2fe666 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -195,6 +195,7 @@ F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_no_gc_sleep_time, no_gc_sleep_time);
 F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_idle, gc_idle);
 F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, reclaim_segments, rec_prefree_segments);
 F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, max_small_discards, max_discards);
+F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, batched_trim_sections, trim_sections);
 F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, ipu_policy, ipu_policy);
 F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, min_ipu_util, min_ipu_util);
 F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, min_fsync_blocks, min_fsync_blocks);
@@ -210,6 +211,7 @@ static struct attribute *f2fs_attrs[] = {
 	ATTR_LIST(gc_idle),
 	ATTR_LIST(reclaim_segments),
 	ATTR_LIST(max_small_discards),
+	ATTR_LIST(batched_trim_sections),
 	ATTR_LIST(ipu_policy),
 	ATTR_LIST(min_ipu_util),
 	ATTR_LIST(min_fsync_blocks),
-- 
2.1.1

RE: [f2fs-dev] [PATCH 5/5 v3] f2fs: introduce a batched trim

[Chao Yu]

Hi Jaegeuk, Changman,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Wednesday, February 04, 2015 4:11 AM
> To: Changman Lee
> Cc: Chao Yu; linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 5/5 v3] f2fs: introduce a batched trim
> 
> Hi Changman,
> 
> Good idea!
> 
> Change log from v2:
>  o add sysfs to change the # of sections for trimming.

Good idea!

> 
> Change log from v1:
>  o add description
>  o change the # of batched segments suggested by Chao
>  o make consistent for # of batched segments
> 
> This patch introduces a batched trimming feature, which submits split discard
> commands.
> 
> This is to avoid long latency due to huge trim commands.
> If fstrim was triggered ranging from 0 to the end of device, we should lock
> all the checkpoint-related mutexes, resulting in very long latency.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  Documentation/ABI/testing/sysfs-fs-f2fs |  6 ++++++
>  Documentation/filesystems/f2fs.txt      |  4 ++++
>  fs/f2fs/f2fs.h                          |  7 +++++++
>  fs/f2fs/segment.c                       | 18 +++++++++++++-----
>  fs/f2fs/super.c                         |  2 ++
>  5 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs
> b/Documentation/ABI/testing/sysfs-fs-f2fs
> index 6f9157f..2c4cc42 100644
> --- a/Documentation/ABI/testing/sysfs-fs-f2fs
> +++ b/Documentation/ABI/testing/sysfs-fs-f2fs
> @@ -74,3 +74,9 @@ Date:		March 2014
>  Contact:	"Jaegeuk Kim" <jaegeuk.kim@samsung.com>
>  Description:
>  		 Controls the memory footprint used by f2fs.
> +
> +What:		/sys/fs/f2fs/<disk>/trim_sections
> +Date:		February 2015
> +Contact:	"Jaegeuk Kim" <jaegeuk@kernel.org>
> +Description:
> +		 Controls the trimming rate in batch mode.
> diff --git a/Documentation/filesystems/f2fs.txt b/Documentation/filesystems/f2fs.txt
> index 6758aa3..dac11d7 100644
> --- a/Documentation/filesystems/f2fs.txt
> +++ b/Documentation/filesystems/f2fs.txt
> @@ -199,6 +199,10 @@ Files in /sys/fs/f2fs/<devname>
>  			      checkpoint is triggered, and issued during the
>  			      checkpoint. By default, it is disabled with 0.
> 
> + trim_sections                This parameter controls the number of sections
> +                              to be trimmed out in batch mode when FITRIM
> +                              conducts. 32 sections is set by default.
> +
>   ipu_policy                   This parameter controls the policy of in-place
>                                updates in f2fs. There are five policies:
>                                 0x01: F2FS_IPU_FORCE, 0x02: F2FS_IPU_SSR,
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 964c240..6f57da1 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -105,6 +105,10 @@ enum {
>  	CP_DISCARD,
>  };
> 
> +#define DEF_BATCHED_TRIM_SECTIONS	32
> +#define BATCHED_TRIM_SEGMENTS(sbi)	\
> +		(SM_I(sbi)->trim_sections * (sbi)->segs_per_sec)
> +
>  struct cp_control {
>  	int reason;
>  	__u64 trim_start;
> @@ -448,6 +452,9 @@ struct f2fs_sm_info {
>  	int nr_discards;			/* # of discards in the list */
>  	int max_discards;			/* max. discards to be issued */
> 
> +	/* for batched trimming */
> +	int trim_sections;			/* # of sections to trim */
> +
>  	struct list_head sit_entry_set;	/* sit entry set list */
> 
>  	unsigned int ipu_policy;	/* in-place-update policy */
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 5ea57ec..c542f63 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1066,14 +1066,20 @@ int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range *range)
>  	end_segno = (end >= MAX_BLKADDR(sbi)) ? MAIN_SEGS(sbi) - 1 :
>  						GET_SEGNO(sbi, end);
>  	cpc.reason = CP_DISCARD;
> -	cpc.trim_start = start_segno;
> -	cpc.trim_end = end_segno;
>  	cpc.trim_minlen = range->minlen >> sbi->log_blocksize;
> 
>  	/* do checkpoint to issue discard commands safely */
> -	mutex_lock(&sbi->gc_mutex);
> -	write_checkpoint(sbi, &cpc);
> -	mutex_unlock(&sbi->gc_mutex);
> +	for (; start_segno <= end_segno;
> +				start_segno += BATCHED_TRIM_SEGMENTS(sbi)) {
> +		cpc.trim_start = start_segno;
> +		cpc.trim_end = min_t(unsigned int,
> +				start_segno + BATCHED_TRIM_SEGMENTS (sbi) - 1,

Actually what I mean is that in each trim we try to align start segment
to first segment of section as much as possible.

How about using:
		cpc.trim_end = min_t(unsigned int, rounddown(start_segno +
					BATCHED_TRIM_SEGMENTS(sbi),
					(sbi)->segs_per_sec) - 1, end_segno);
Thanks,

> +				end_segno);
> +
> +		mutex_lock(&sbi->gc_mutex);
> +		write_checkpoint(sbi, &cpc);
> +		mutex_unlock(&sbi->gc_mutex);
> +	}
>  out:
>  	range->len = cpc.trimmed << sbi->log_blocksize;
>  	return 0;
> @@ -2127,6 +2133,8 @@ int build_segment_manager(struct f2fs_sb_info *sbi)
>  	sm_info->nr_discards = 0;
>  	sm_info->max_discards = 0;
> 
> +	sm_info->trim_sections = DEF_BATCHED_TRIM_SECTIONS;
> +
>  	INIT_LIST_HEAD(&sm_info->sit_entry_set);
> 
>  	if (test_opt(sbi, FLUSH_MERGE) && !f2fs_readonly(sbi->sb)) {
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 1e92c2e..f2fe666 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -195,6 +195,7 @@ F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_no_gc_sleep_time,
> no_gc_sleep_time);
>  F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_idle, gc_idle);
>  F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, reclaim_segments, rec_prefree_segments);
>  F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, max_small_discards, max_discards);
> +F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, batched_trim_sections, trim_sections);
>  F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, ipu_policy, ipu_policy);
>  F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, min_ipu_util, min_ipu_util);
>  F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, min_fsync_blocks, min_fsync_blocks);
> @@ -210,6 +211,7 @@ static struct attribute *f2fs_attrs[] = {
>  	ATTR_LIST(gc_idle),
>  	ATTR_LIST(reclaim_segments),
>  	ATTR_LIST(max_small_discards),
> +	ATTR_LIST(batched_trim_sections),
>  	ATTR_LIST(ipu_policy),
>  	ATTR_LIST(min_ipu_util),
>  	ATTR_LIST(min_fsync_blocks),
> --
> 2.1.1

Re: [f2fs-dev] [PATCH 5/5 v4] f2fs: introduce a batched trim

[Jaegeuk Kim]

Hi Chao,

Something like this?

Change log from v3:
 o align to section size

This patch introduces a batched trimming feature, which submits split discard
commands.

This is to avoid long latency due to huge trim commands.
If fstrim was triggered ranging from 0 to the end of device, we should lock
all the checkpoint-related mutexes, resulting in very long latency.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 Documentation/ABI/testing/sysfs-fs-f2fs |  6 ++++++
 Documentation/filesystems/f2fs.txt      |  4 ++++
 fs/f2fs/f2fs.h                          |  7 +++++++
 fs/f2fs/segment.c                       | 17 ++++++++++++-----
 fs/f2fs/super.c                         |  2 ++
 5 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
index 6f9157f..2c4cc42 100644
--- a/Documentation/ABI/testing/sysfs-fs-f2fs
+++ b/Documentation/ABI/testing/sysfs-fs-f2fs
@@ -74,3 +74,9 @@ Date:		March 2014
 Contact:	"Jaegeuk Kim" <jaegeuk.kim@samsung.com>
 Description:
 		 Controls the memory footprint used by f2fs.
+
+What:		/sys/fs/f2fs/<disk>/trim_sections
+Date:		February 2015
+Contact:	"Jaegeuk Kim" <jaegeuk@kernel.org>
+Description:
+		 Controls the trimming rate in batch mode.
diff --git a/Documentation/filesystems/f2fs.txt b/Documentation/filesystems/f2fs.txt
index 6758aa3..dac11d7 100644
--- a/Documentation/filesystems/f2fs.txt
+++ b/Documentation/filesystems/f2fs.txt
@@ -199,6 +199,10 @@ Files in /sys/fs/f2fs/<devname>
 			      checkpoint is triggered, and issued during the
 			      checkpoint. By default, it is disabled with 0.
 
+ trim_sections                This parameter controls the number of sections
+                              to be trimmed out in batch mode when FITRIM
+                              conducts. 32 sections is set by default.
+
  ipu_policy                   This parameter controls the policy of in-place
                               updates in f2fs. There are five policies:
                                0x01: F2FS_IPU_FORCE, 0x02: F2FS_IPU_SSR,
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index db1ff55..d82a10a 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -105,6 +105,10 @@ enum {
 	CP_DISCARD,
 };
 
+#define DEF_BATCHED_TRIM_SECTIONS	32
+#define BATCHED_TRIM_SEGMENTS(sbi)	\
+		(SM_I(sbi)->trim_sections * (sbi)->segs_per_sec)
+
 struct cp_control {
 	int reason;
 	__u64 trim_start;
@@ -448,6 +452,9 @@ struct f2fs_sm_info {
 	int nr_discards;			/* # of discards in the list */
 	int max_discards;			/* max. discards to be issued */
 
+	/* for batched trimming */
+	int trim_sections;			/* # of sections to trim */
+
 	struct list_head sit_entry_set;	/* sit entry set list */
 
 	unsigned int ipu_policy;	/* in-place-update policy */
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 5ea57ec..9f278d1 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1066,14 +1066,19 @@ int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range *range)
 	end_segno = (end >= MAX_BLKADDR(sbi)) ? MAIN_SEGS(sbi) - 1 :
 						GET_SEGNO(sbi, end);
 	cpc.reason = CP_DISCARD;
-	cpc.trim_start = start_segno;
-	cpc.trim_end = end_segno;
 	cpc.trim_minlen = range->minlen >> sbi->log_blocksize;
 
 	/* do checkpoint to issue discard commands safely */
-	mutex_lock(&sbi->gc_mutex);
-	write_checkpoint(sbi, &cpc);
-	mutex_unlock(&sbi->gc_mutex);
+	for (; start_segno <= end_segno; start_segno = cpc.trim_end + 1) {
+		cpc.trim_start = start_segno;
+		cpc.trim_end = min_t(unsigned int, rounddown(start_segno +
+				BATCHED_TRIM_SEGMENTS(sbi),
+				sbi->segs_per_sec) - 1, end_segno);
+
+		mutex_lock(&sbi->gc_mutex);
+		write_checkpoint(sbi, &cpc);
+		mutex_unlock(&sbi->gc_mutex);
+	}
 out:
 	range->len = cpc.trimmed << sbi->log_blocksize;
 	return 0;
@@ -2127,6 +2132,8 @@ int build_segment_manager(struct f2fs_sb_info *sbi)
 	sm_info->nr_discards = 0;
 	sm_info->max_discards = 0;
 
+	sm_info->trim_sections = DEF_BATCHED_TRIM_SECTIONS;
+
 	INIT_LIST_HEAD(&sm_info->sit_entry_set);
 
 	if (test_opt(sbi, FLUSH_MERGE) && !f2fs_readonly(sbi->sb)) {
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 1e92c2e..f2fe666 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -195,6 +195,7 @@ F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_no_gc_sleep_time, no_gc_sleep_time);
 F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_idle, gc_idle);
 F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, reclaim_segments, rec_prefree_segments);
 F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, max_small_discards, max_discards);
+F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, batched_trim_sections, trim_sections);
 F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, ipu_policy, ipu_policy);
 F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, min_ipu_util, min_ipu_util);
 F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, min_fsync_blocks, min_fsync_blocks);
@@ -210,6 +211,7 @@ static struct attribute *f2fs_attrs[] = {
 	ATTR_LIST(gc_idle),
 	ATTR_LIST(reclaim_segments),
 	ATTR_LIST(max_small_discards),
+	ATTR_LIST(batched_trim_sections),
 	ATTR_LIST(ipu_policy),
 	ATTR_LIST(min_ipu_util),
 	ATTR_LIST(min_fsync_blocks),
-- 
2.1.1

RE: [f2fs-dev] [PATCH 5/5 v4] f2fs: introduce a batched trim

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Friday, February 06, 2015 2:19 PM
> To: Chao Yu
> Cc: 'Changman Lee'; linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 5/5 v4] f2fs: introduce a batched trim
> 
> Hi Chao,
> 
> Something like this?
> 
> Change log from v3:
>  o align to section size
> 
> This patch introduces a batched trimming feature, which submits split discard
> commands.
> 
> This is to avoid long latency due to huge trim commands.
> If fstrim was triggered ranging from 0 to the end of device, we should lock
> all the checkpoint-related mutexes, resulting in very long latency.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Looks good to me.

Reviewed-by: Chao Yu <chao2.yu@samsung.com>

Re: [f2fs-dev] [PATCH 5/5 v4] f2fs: introduce a batched trim

[Leon Romanovsky]

Hi Jaegeuk,

> +       /* for batched trimming */
> +       int trim_sections;                      /* # of sections to trim */
I would like to suggest to declare trim_sections variable as an
unsigned int and not int, since it can't be negative by definition.
What do you think about it?

Re: [f2fs-dev] [PATCH 5/5 v4] f2fs: introduce a batched trim

[Jaegeuk Kim]

On Sat, Feb 07, 2015 at 05:57:46PM +0200, Leon Romanovsky wrote:
> Hi Jaegeuk,
> 
> > +       /* for batched trimming */
> > +       int trim_sections;                      /* # of sections to trim */
> I would like to suggest to declare trim_sections variable as an
> unsigned int and not int, since it can't be negative by definition.
> What do you think about it?

Hi Leon,

No problem. :)

I'll change that and merge the patch.

Thanks,

RE: [PATCH 1/5] f2fs: fix not to drop mount options when retrying fill_super

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: linux-fsdevel-owner@vger.kernel.org [mailto:linux-fsdevel-owner@vger.kernel.org] On
> Behalf Of Jaegeuk Kim
> Sent: Wednesday, January 28, 2015 7:32 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [PATCH 1/5] f2fs: fix not to drop mount options when retrying fill_super
> 
> If wrong mount option was requested, f2fs tries to fill_super again.
> But, during the next trial, f2fs has no valid mount options, since
> parse_options deleted all the separators in the original string.

Nice catch!

But couldn't we encounter memory leak for options if we did not fail to fill super.

Thanks,

Re: [PATCH 1/5] f2fs: fix not to drop mount options when retrying fill_super

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: linux-fsdevel-owner@vger.kernel.org [mailto:linux-fsdevel-owner@vger.kernel.org] On
> Behalf Of Jaegeuk Kim
> Sent: Wednesday, January 28, 2015 7:32 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [PATCH 1/5] f2fs: fix not to drop mount options when retrying fill_super
> 
> If wrong mount option was requested, f2fs tries to fill_super again.
> But, during the next trial, f2fs has no valid mount options, since
> parse_options deleted all the separators in the original string.

Nice catch!

But couldn't we encounter memory leak for options if we did not fail to fill super.

Thanks,


------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/

Re: [PATCH 1/5 v2] f2fs: fix not to drop mount options when retrying fill_super

[Jaegeuk Kim]

Chang log from v1:
 o fix memory leak pointed by Chao

>From 82760814db6a01539ad15026b0469686110d92bc Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Fri, 23 Jan 2015 17:41:39 -0800
Subject: [PATCH] f2fs: fix not to drop mount options when retrying fill_super

If wrong mount option was requested, f2fs tries to fill_super again.
But, during the next trial, f2fs has no valid mount options, since
parse_options deleted all the separators in the original string.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/super.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 25f4291..47d9b04 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -951,6 +951,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	struct inode *root;
 	long err = -EINVAL;
 	bool retry = true;
+	char *options = NULL;
 	int i;
 
 try_onemore:
@@ -982,9 +983,15 @@ try_onemore:
 	set_opt(sbi, POSIX_ACL);
 #endif
 	/* parse mount options */
-	err = parse_options(sb, (char *)data);
-	if (err)
+	options = kstrdup((const char *)data, GFP_KERNEL);
+	if (data && !options) {
+		err = -ENOMEM;
 		goto free_sb_buf;
+	}
+
+	err = parse_options(sb, options);
+	if (err)
+		goto free_options;
 
 	sb->s_maxbytes = max_file_size(le32_to_cpu(raw_super->log_blocksize));
 	sb->s_max_links = F2FS_LINK_MAX;
@@ -1028,7 +1035,7 @@ try_onemore:
 	if (IS_ERR(sbi->meta_inode)) {
 		f2fs_msg(sb, KERN_ERR, "Failed to read F2FS meta data inode");
 		err = PTR_ERR(sbi->meta_inode);
-		goto free_sb_buf;
+		goto free_options;
 	}
 
 	err = get_valid_checkpoint(sbi);
@@ -1153,6 +1160,7 @@ try_onemore:
 		if (err)
 			goto free_kobj;
 	}
+	kfree(options);
 	return 0;
 
 free_kobj:
@@ -1177,6 +1185,8 @@ free_cp:
 free_meta_inode:
 	make_bad_inode(sbi->meta_inode);
 	iput(sbi->meta_inode);
+free_options:
+	kfree(options);
 free_sb_buf:
 	brelse(raw_super_buf);
 free_sbi:
-- 
2.1.1

RE: [PATCH 1/5 v2] f2fs: fix not to drop mount options when retrying fill_super

[Chao Yu]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Friday, January 30, 2015 2:21 AM
> To: Chao Yu
> Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [PATCH 1/5 v2] f2fs: fix not to drop mount options when retrying fill_super
> 
> Chang log from v1:
>  o fix memory leak pointed by Chao
> 
> From 82760814db6a01539ad15026b0469686110d92bc Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <jaegeuk@kernel.org>
> Date: Fri, 23 Jan 2015 17:41:39 -0800
> Subject: [PATCH] f2fs: fix not to drop mount options when retrying fill_super
> 
> If wrong mount option was requested, f2fs tries to fill_super again.
> But, during the next trial, f2fs has no valid mount options, since
> parse_options deleted all the separators in the original string.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <chao2.yu@samsung.com>