[PATCH 1/2] f2fs: revisit the f2fs_gc flow

[PATCH 1/2] f2fs: revisit the f2fs_gc flow

[Jaegeuk Kim]

I'd like to revisit the f2fs_gc flow and rewrite as follows.

1. In practical, the nGC parameter of f2fs_gc is meaningless. So, let's
  remove it.
2. Background GC marks victim blocks as dirty one at a time.
3. Foreground GC should do cleaning job until acquiring enough free
  sections. Afterwards, it needs to do checkpoint.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/f2fs.h    |  2 +-
 fs/f2fs/gc.c      | 60 +++++++++++++++++++------------------------------------
 fs/f2fs/segment.c |  2 +-
 3 files changed, 23 insertions(+), 41 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 2807132..285e43d 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -986,7 +986,7 @@ int do_write_data_page(struct page *);
 int start_gc_thread(struct f2fs_sb_info *);
 void stop_gc_thread(struct f2fs_sb_info *);
 block_t start_bidx_of_node(unsigned int);
-int f2fs_gc(struct f2fs_sb_info *, int);
+int f2fs_gc(struct f2fs_sb_info *);
 void build_gc_manager(struct f2fs_sb_info *);
 int create_gc_caches(void);
 void destroy_gc_caches(void);
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index b0ec721..b4dd90c 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -78,7 +78,7 @@ static int gc_thread_func(void *data)
 
 		sbi->bg_gc++;
 
-		if (f2fs_gc(sbi, 1) == GC_NONE)
+		if (f2fs_gc(sbi) == GC_NONE)
 			wait_ms = GC_THREAD_NOGC_SLEEP_TIME;
 		else if (wait_ms == GC_THREAD_NOGC_SLEEP_TIME)
 			wait_ms = GC_THREAD_MAX_SLEEP_TIME;
@@ -651,62 +651,44 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi, unsigned int segno,
 	return ret;
 }
 
-int f2fs_gc(struct f2fs_sb_info *sbi, int nGC)
+int f2fs_gc(struct f2fs_sb_info *sbi)
 {
-	unsigned int segno;
-	int old_free_secs, cur_free_secs;
-	int gc_status, nfree;
 	struct list_head ilist;
+	unsigned int segno, i;
 	int gc_type = BG_GC;
+	int gc_status = GC_NONE;
 
 	INIT_LIST_HEAD(&ilist);
 gc_more:
-	nfree = 0;
-	gc_status = GC_NONE;
+	if (!(sbi->sb->s_flags & MS_ACTIVE))
+		goto stop;
 
 	if (has_not_enough_free_secs(sbi))
-		old_free_secs = reserved_sections(sbi);
-	else
-		old_free_secs = free_sections(sbi);
-
-	while (sbi->sb->s_flags & MS_ACTIVE) {
-		int i;
-		if (has_not_enough_free_secs(sbi))
-			gc_type = FG_GC;
+		gc_type = FG_GC;
 
-		cur_free_secs = free_sections(sbi) + nfree;
+	if (!__get_victim(sbi, &segno, gc_type, NO_CHECK_TYPE))
+		goto stop;
 
-		/* We got free space successfully. */
-		if (nGC < cur_free_secs - old_free_secs)
-			break;
-
-		if (!__get_victim(sbi, &segno, gc_type, NO_CHECK_TYPE))
+	for (i = 0; i < sbi->segs_per_sec; i++) {
+		/*
+		 * do_garbage_collect will give us three gc_status:
+		 * GC_ERROR, GC_DONE, and GC_BLOCKED.
+		 * If GC is finished uncleanly, we have to return
+		 * the victim to dirty segment list.
+		 */
+		gc_status = do_garbage_collect(sbi, segno + i, &ilist, gc_type);
+		if (gc_status != GC_DONE)
 			break;
-
-		for (i = 0; i < sbi->segs_per_sec; i++) {
-			/*
-			 * do_garbage_collect will give us three gc_status:
-			 * GC_ERROR, GC_DONE, and GC_BLOCKED.
-			 * If GC is finished uncleanly, we have to return
-			 * the victim to dirty segment list.
-			 */
-			gc_status = do_garbage_collect(sbi, segno + i,
-					&ilist, gc_type);
-			if (gc_status != GC_DONE)
-				goto stop;
-			nfree++;
-		}
 	}
-stop:
-	if (has_not_enough_free_secs(sbi) || gc_status == GC_BLOCKED) {
+	if (has_not_enough_free_secs(sbi)) {
 		write_checkpoint(sbi, (gc_status == GC_BLOCKED), false);
-		if (nfree)
+		if (has_not_enough_free_secs(sbi))
 			goto gc_more;
 	}
+stop:
 	mutex_unlock(&sbi->gc_mutex);
 
 	put_gc_inode(&ilist);
-	BUG_ON(!list_empty(&ilist));
 	return gc_status;
 }
 
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index de62409..4b00990 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -31,7 +31,7 @@ void f2fs_balance_fs(struct f2fs_sb_info *sbi)
 	 */
 	if (has_not_enough_free_secs(sbi)) {
 		mutex_lock(&sbi->gc_mutex);
-		f2fs_gc(sbi, 1);
+		f2fs_gc(sbi);
 	}
 }
 
-- 
1.8.0.1.250.gb7973fb

[PATCH 2/2] f2fs: add f2fs_balance_fs in several interfaces

[Jaegeuk Kim]

The f2fs_balance_fs() is to check the number of free sections and decide whether
it needs to conduct cleaning or not. If there are not enough free sections, the
cleaning job should be started.

In order to control an amount of free sections even under high utilization, f2fs
should call f2fs_balance_fs at all the VFS interfaces that are able to produce
dirty pages.
This patch adds the function calls in the missing interfaces as follows.

1. f2fs_setxattr()
The f2fs_setxattr() produces dirty node pages so that we should call
f2fs_balance_fs() either likewise doing in other VFS interfaces such as
f2fs_lookup(), f2fs_mkdir(), and so on.

2. f2fs_sync_file()
We should guarantee serving free sections for syncing metadata during fsync.
Previously, there is no space check before triggering checkpoint and
sync_node_pages.
Therefore, if a bunch of fsync calls are triggered under 100% of FS utilization,
f2fs is able to be faced with no free sections, resulting in BUG_ON().

3. f2fs_sync_fs()
Before calling write_checkpoint(), we should guarantee that there are minimum
free sections.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/file.c  | 3 +++
 fs/f2fs/super.c | 2 ++
 fs/f2fs/xattr.c | 2 ++
 3 files changed, 7 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 88593c5..7354c2d 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -137,6 +137,9 @@ int f2fs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	if (ret)
 		return ret;
 
+	/* guarantee free sections for fsync */
+	f2fs_balance_fs(sbi);
+
 	mutex_lock(&inode->i_mutex);
 
 	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index afa7ef0..0f2b2eb 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -137,6 +137,8 @@ int f2fs_sync_fs(struct super_block *sb, int sync)
 
 	if (sync)
 		write_checkpoint(sbi, false, false);
+	else
+		f2fs_balance_fs(sbi);
 
 	return 0;
 }
diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index 940136a..8038c04 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -318,6 +318,8 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
 	if (name_len > 255 || value_len > MAX_VALUE_LEN)
 		return -ERANGE;
 
+	f2fs_balance_fs(sbi);
+
 	mutex_lock_op(sbi, NODE_NEW);
 	if (!fi->i_xattr_nid) {
 		/* Allocate new attribute block */
-- 
1.8.0.1.250.gb7973fb

Re: [PATCH 2/2] f2fs: add f2fs_balance_fs in several interfaces

[Namjae Jeon]

2013/1/10, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> The f2fs_balance_fs() is to check the number of free sections and decide
> whether
> it needs to conduct cleaning or not. If there are not enough free sections,
> the
> cleaning job should be started.
>
> In order to control an amount of free sections even under high utilization,
> f2fs
> should call f2fs_balance_fs at all the VFS interfaces that are able to
> produce
> dirty pages.
> This patch adds the function calls in the missing interfaces as follows.
>
> 1. f2fs_setxattr()
> The f2fs_setxattr() produces dirty node pages so that we should call
> f2fs_balance_fs() either likewise doing in other VFS interfaces such as
> f2fs_lookup(), f2fs_mkdir(), and so on.
>
> 2. f2fs_sync_file()
> We should guarantee serving free sections for syncing metadata during
> fsync.
> Previously, there is no space check before triggering checkpoint and
> sync_node_pages.
> Therefore, if a bunch of fsync calls are triggered under 100% of FS
> utilization,
> f2fs is able to be faced with no free sections, resulting in BUG_ON().
>
> 3. f2fs_sync_fs()
> Before calling write_checkpoint(), we should guarantee that there are
> minimum
> free sections.
Hi Jaegeuk.
There is no need to add f2fs_balance_fs in f2fs_fallocate also ?

Thanks.
>
> Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
> ---
>  fs/f2fs/file.c  | 3 +++
>  fs/f2fs/super.c | 2 ++
>  fs/f2fs/xattr.c | 2 ++
>  3 files changed, 7 insertions(+)
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 88593c5..7354c2d 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -137,6 +137,9 @@ int f2fs_sync_file(struct file *file, loff_t start,
> loff_t end, int datasync)
>  	if (ret)
>  		return ret;
>
> +	/* guarantee free sections for fsync */
> +	f2fs_balance_fs(sbi);
> +
>  	mutex_lock(&inode->i_mutex);
>
>  	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index afa7ef0..0f2b2eb 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -137,6 +137,8 @@ int f2fs_sync_fs(struct super_block *sb, int sync)
>
>  	if (sync)
>  		write_checkpoint(sbi, false, false);
> +	else
> +		f2fs_balance_fs(sbi);
>
>  	return 0;
>  }
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index 940136a..8038c04 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -318,6 +318,8 @@ int f2fs_setxattr(struct inode *inode, int name_index,
> const char *name,
>  	if (name_len > 255 || value_len > MAX_VALUE_LEN)
>  		return -ERANGE;
>
> +	f2fs_balance_fs(sbi);
> +
>  	mutex_lock_op(sbi, NODE_NEW);
>  	if (!fi->i_xattr_nid) {
>  		/* Allocate new attribute block */
> --
> 1.8.0.1.250.gb7973fb
>
> --
> 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/2] f2fs: add f2fs_balance_fs in several interfaces

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 146 bytes --]


> Hi Jaegeuk.
> There is no need to add f2fs_balance_fs in f2fs_fallocate also ?

Noop. It exists. :)
Thanks.

-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] f2fs: add f2fs_balance_fs in several interfaces

[Namjae Jeon]

2013/1/11, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
>
>> Hi Jaegeuk.
>> There is no need to add f2fs_balance_fs in f2fs_fallocate also ?
>
> Noop. It exists. :)
Ah,, Does it need to move start of function ?

Thanks.
> Thanks.
>
> --
> Jaegeuk Kim
> Samsung
>

Re: [PATCH 2/2] f2fs: add f2fs_balance_fs in several interfaces

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

2013-01-11 (금), 10:26 +0900, Namjae Jeon:
> 2013/1/11, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> >
> >> Hi Jaegeuk.
> >> There is no need to add f2fs_balance_fs in f2fs_fallocate also ?
> >
> > Noop. It exists. :)
> Ah,, Does it need to move start of function ?

Ah, I'll check it.
Thanks,

> 
> Thanks.
> > Thanks.
> >
> > --
> > Jaegeuk Kim
> > Samsung
> >
> --
> 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

-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] f2fs: move f2fs_balance_fs to punch_hole

[Jaegeuk Kim]

The f2fs_fallocate() has two operations: punch_hole and expand_size.

Only in the case of punch_hole, dirty node pages can be produced, so let's
trigger f2fs_balance_fs() in this case only.
Furthermore, let's trigger it at every data truncation routine.

Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/file.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 7354c2d..819de7f 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -410,6 +410,8 @@ int truncate_hole(struct inode *inode, pgoff_t pg_start, pgoff_t pg_end)
 		struct dnode_of_data dn;
 		struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
 
+		f2fs_balance_fs(sbi);
+
 		mutex_lock_op(sbi, DATA_TRUNC);
 		set_new_dnode(&dn, inode, NULL, NULL, 0);
 		err = get_dnode_of_data(&dn, index, RDONLY_NODE);
@@ -537,7 +539,6 @@ static long f2fs_fallocate(struct file *file, int mode,
 				loff_t offset, loff_t len)
 {
 	struct inode *inode = file->f_path.dentry->d_inode;
-	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
 	long ret;
 
 	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
@@ -552,8 +553,6 @@ static long f2fs_fallocate(struct file *file, int mode,
 		inode->i_mtime = inode->i_ctime = CURRENT_TIME;
 		mark_inode_dirty(inode);
 	}
-
-	f2fs_balance_fs(sbi);
 	return ret;
 }
 
-- 
1.8.0.1.250.gb7973fb

Re: [PATCH 2/2 v2] f2fs: add f2fs_balance_fs in several interfaces

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 3317 bytes --]

Change log from v1:
 o add the f2fs_write_inode case

From 7d82db83165dbac8c3f6d47b73c84f38e3996e30 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk.kim@samsung.com>
Date: Fri, 11 Jan 2013 13:10:49 +0900
Subject: [PATCH] f2fs: add f2fs_balance_fs in several interfaces

The f2fs_balance_fs() is to check the number of free sections and decide
whether
it needs to conduct cleaning or not. If there are not enough free
sections, the
cleaning job should be started.

In order to control an amount of free sections even under high
utilization, f2fs
should call f2fs_balance_fs at all the VFS interfaces that are able to
produce
dirty pages.
This patch adds the function calls in the missing interfaces as follows.

1. f2fs_setxattr()
The f2fs_setxattr() produces dirty node pages so that we should call
f2fs_balance_fs() either likewise doing in other VFS interfaces such as
f2fs_lookup(), f2fs_mkdir(), and so on.

2. f2fs_sync_file()
We should guarantee serving free sections for syncing metadata during
fsync.
Previously, there is no space check before triggering checkpoint and
sync_node_pages.
Therefore, if a bunch of fsync calls are triggered under 100% of FS
utilization,
f2fs is able to be faced with no free sections, resulting in BUG_ON().

3. f2fs_sync_fs()
Before calling write_checkpoint(), we should guarantee that there are
minimum
free sections.

4. f2fs_write_inode()
f2fs_write_inode() is also able to produce dirty node pages.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/file.c  | 3 +++
 fs/f2fs/inode.c | 3 +++
 fs/f2fs/super.c | 2 ++
 fs/f2fs/xattr.c | 2 ++
 4 files changed, 10 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 88593c5..7354c2d 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -137,6 +137,9 @@ int f2fs_sync_file(struct file *file, loff_t start,
loff_t end, int datasync)
 	if (ret)
 		return ret;
 
+	/* guarantee free sections for fsync */
+	f2fs_balance_fs(sbi);
+
 	mutex_lock(&inode->i_mutex);
 
 	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index bf20b4d..7942417 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -217,6 +217,9 @@ int f2fs_write_inode(struct inode *inode, struct
writeback_control *wbc)
 			inode->i_ino == F2FS_META_INO(sbi))
 		return 0;
 
+	if (wbc)
+		f2fs_balance_fs(sbi);
+
 	node_page = get_node_page(sbi, inode->i_ino);
 	if (IS_ERR(node_page))
 		return PTR_ERR(node_page);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index afa7ef0..0f2b2eb 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -137,6 +137,8 @@ int f2fs_sync_fs(struct super_block *sb, int sync)
 
 	if (sync)
 		write_checkpoint(sbi, false, false);
+	else
+		f2fs_balance_fs(sbi);
 
 	return 0;
 }
diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index 940136a..8038c04 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -318,6 +318,8 @@ int f2fs_setxattr(struct inode *inode, int
name_index, const char *name,
 	if (name_len > 255 || value_len > MAX_VALUE_LEN)
 		return -ERANGE;
 
+	f2fs_balance_fs(sbi);
+
 	mutex_lock_op(sbi, NODE_NEW);
 	if (!fi->i_xattr_nid) {
 		/* Allocate new attribute block */
-- 
1.8.0.1.250.gb7973fb


-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]