Skip to contentSkip to conversation

f2fs: avoid multiple node page writes due to inline_data

PATCH In review Jaegeuk Kim 2 participants 8 messages Jan 23, 2016 – Jan 26, 2016

Subsystems (2)
VFS F2FS file system
lore
Share & tools
lore

Jaegeuk Kim

author maintainer
#0 lore raw

PATCH 1/2 f2fs: avoid multiple node page writes due to inline_data

The sceanrio is:
1. create fully node blocks
2. flush node blocks
3. write inline_data for all the node blocks again
4. flush node blocks redundantly

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
fs/f2fs/data.c +11 -3
index 8d0d9ec..011456e 100644
@@ -1622,14 +1622,22 @@ static int f2fs_write_end(struct file *file,
 
 	trace_f2fs_write_end(inode, pos, len, copied);
 
-	set_page_dirty(page);
-
 	if (pos + copied > i_size_read(inode)) {
 		i_size_write(inode, pos + copied);
 		mark_inode_dirty(inode);
-		update_inode_page(inode);
 	}
 
+	if (f2fs_has_inline_data(inode) &&
+			is_inode_flag_set(F2FS_I(inode), FI_DATA_EXIST)) {
+		int err = f2fs_write_inline_data(inode, page);
+		if (err)
+			set_page_dirty(page);
+	} else {
+		set_page_dirty(page);
+	}
+
+	f2fs_write_inode(inode, NULL);
+
 	f2fs_put_page(page, 1);
 	f2fs_update_time(F2FS_I_SB(inode), REQ_TIME);
 	return copied;
Signature · 1 line
2.6.3
author maintainer
#1 lore raw

PATCH 2/2 f2fs: fix to overcome inline_data floods

The scenario is:
1. create lots of node blocks
2. sync
3. write lots of inline_data
-> got panic due to no free space

In that case, we should flush node blocks when writing inline_data in #3, and trigger gc as well.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/gc.c | 7 +++++++
 1 file changed, 7 insertions(+)
fs/f2fs/gc.c +7 -0
index 0204433..8686231 100644
@@ -838,8 +838,15 @@ gc_more:
 
 	if (gc_type == BG_GC && has_not_enough_free_secs(sbi, sec_freed)) {
 		gc_type = FG_GC;
+		/*
+		 * If there is no victim and no prefree segment but still not
+		 * enough free sections, we should flush dent/node blocks and do
+		 * garbage collections.
+		 */
 		if (__get_victim(sbi, &segno, gc_type) || prefree_segments(sbi))
 			write_checkpoint(sbi, &cpc);
+		else if (has_not_enough_free_secs(sbi, 0))
+			write_checkpoint(sbi, &cpc);
 	}
 
 	if (segno == NULL_SEGNO && !__get_victim(sbi, &segno, gc_type))
Signature · 1 line
2.6.3
#2 lore raw

RE: [f2fs-dev] [PATCH 2/2] f2fs: fix to overcome inline_data floods

Hi Jaegeuk,

Quoted 28 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 24, 2016 4:16 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 2/2] f2fs: fix to overcome inline_data floods

The scenario is:
1. create lots of node blocks
2. sync
3. write lots of inline_data
-> got panic due to no free space

In that case, we should flush node blocks when writing inline_data in #3, and trigger gc as well.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/gc.c | 7 +++++++
 1 file changed, 7 insertions(+)
fs/f2fs/gc.c +0 -0
index 0204433..8686231 100644
@@ -838,8 +838,15 @@ gc_more:

fs/f2fs/gc.c gc_more: · line 848

Quoted 10 lines
fs/f2fs/gc.c +6 -0
 	if (gc_type == BG_GC && has_not_enough_free_secs(sbi, sec_freed)) {
 		gc_type = FG_GC;
+		/*
+		 * If there is no victim and no prefree segment but still not
+		 * enough free sections, we should flush dent/node blocks and do
+		 * garbage collections.
+		 */
 		if (__get_victim(sbi, &segno, gc_type) || prefree_segments(sbi))
 			write_checkpoint(sbi, &cpc);
+		else if (has_not_enough_free_secs(sbi, 0))

I think this condition make checkpoint been triggered more frequently, could we trigger cp when dent/node block exceed some threshold?

Thanks,

Quoted 18 lines
fs/f2fs/gc.c +1 -1
+			write_checkpoint(sbi, &cpc);
 	}
 
 	if (segno == NULL_SEGNO && !__get_victim(sbi, &segno, gc_type))
--

2.6.3

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

author maintainer
#3 lore raw

Re: [f2fs-dev] [PATCH 2/2] f2fs: fix to overcome inline_data floods

Hi Chao,

Quoted 30 lines · Chao Yu

Hi Jaegeuk,

Quoted 28 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 24, 2016 4:16 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 2/2] f2fs: fix to overcome inline_data floods

The scenario is:
1. create lots of node blocks
2. sync
3. write lots of inline_data
-> got panic due to no free space

In that case, we should flush node blocks when writing inline_data in #3, and trigger gc as well.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/gc.c | 7 +++++++
 1 file changed, 7 insertions(+)
fs/f2fs/gc.c +0 -0
index 0204433..8686231 100644
@@ -838,8 +838,15 @@ gc_more:
Quoted 13 lines

fs/f2fs/gc.c gc_more: · line 848

Quoted 10 lines
fs/f2fs/gc.c +6 -0
 	if (gc_type == BG_GC && has_not_enough_free_secs(sbi, sec_freed)) {
 		gc_type = FG_GC;
+		/*
+		 * If there is no victim and no prefree segment but still not
+		 * enough free sections, we should flush dent/node blocks and do
+		 * garbage collections.
+		 */
 		if (__get_victim(sbi, &segno, gc_type) || prefree_segments(sbi))
 			write_checkpoint(sbi, &cpc);
+		else if (has_not_enough_free_secs(sbi, 0))

I think this condition make checkpoint been triggered more frequently, could we trigger cp when dent/node block exceed some threshold?

This only happens when there is no victim, no prefree, and no enough free secs, which is very corner case.
In addition, IMO, this condition is actually to resolve the inline_data issue.

Thanks,

Quoted 21 lines

Thanks,

Quoted 18 lines
fs/f2fs/gc.c +1 -1
+			write_checkpoint(sbi, &cpc);
 	}
 
 	if (segno == NULL_SEGNO && !__get_victim(sbi, &segno, gc_type))
--

2.6.3

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

#4 lore raw
In reply to Jaegeuk Kim

Hi Jaegeuk,

Quoted 28 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 24, 2016 4:16 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 1/2] f2fs: avoid multiple node page writes due to inline_data

The sceanrio is:
1. create fully node blocks
2. flush node blocks
3. write inline_data for all the node blocks again
4. flush node blocks redundantly

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
fs/f2fs/data.c +0 -1
index 8d0d9ec..011456e 100644
@@ -1622,14 +1622,22 @@ static int f2fs_write_end(struct file *file,
 
 	trace_f2fs_write_end(inode, pos, len, copied);
 
-	set_page_dirty(page);

fs/f2fs/data.c static int f2fs_write_end(struct file *file, · line 1632

Quoted 10 lines
fs/f2fs/data.c +3 -2
-
 	if (pos + copied > i_size_read(inode)) {
 		i_size_write(inode, pos + copied);
 		mark_inode_dirty(inode);
-		update_inode_page(inode);
 	}
 
+	if (f2fs_has_inline_data(inode) &&
+			is_inode_flag_set(F2FS_I(inode), FI_DATA_EXIST)) {
+		int err = f2fs_write_inline_data(inode, page);

Oh, I'm sure this can fix that issue, but IMO:
a) this implementation has side-effect, it triggers inline data copying between data page and node page whenever user write inline datas, so if user updates inline data frequently, write-through approach would cause memory copy overhead.
b) inline storm should be a rare case, as we didn't get any report about problem for long time until Dave's, and write_end is a hot path, I think it's better to be cautious to change our inline data cache policy for fixing a rare issue in hot path.

What about delaying the merge operation? like:
1) as I proposed before, merging inline page into inode page when detecting free_sections <= (node_secs + 2 * dent_secs + inline_secs).
2) merge inline page into inode page before writeback inode page in sync_node_pages.

Thanks,

Quoted 25 lines
fs/f2fs/data.c +8 -1
+		if (err)
+			set_page_dirty(page);
+	} else {
+		set_page_dirty(page);
+	}
+
+	f2fs_write_inode(inode, NULL);
+
 	f2fs_put_page(page, 1);
 	f2fs_update_time(F2FS_I_SB(inode), REQ_TIME);
 	return copied;
--

2.6.3

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

author maintainer
#5 lore raw

Hi Chao,

Quoted 37 lines · Chao Yu

Hi Jaegeuk,

Quoted 35 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 24, 2016 4:16 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 1/2] f2fs: avoid multiple node page writes due to inline_data

The sceanrio is:
1. create fully node blocks
2. flush node blocks
3. write inline_data for all the node blocks again
4. flush node blocks redundantly

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
fs/f2fs/data.c +0 -3
index 8d0d9ec..011456e 100644
@@ -1622,14 +1622,22 @@ static int f2fs_write_end(struct file *file,
 
 	trace_f2fs_write_end(inode, pos, len, copied);
 
-	set_page_dirty(page);
-
 	if (pos + copied > i_size_read(inode)) {
 		i_size_write(inode, pos + copied);
 		mark_inode_dirty(inode);
-		update_inode_page(inode);
 	}
Quoted 9 lines

fs/f2fs/data.c static int f2fs_write_end(struct file *file, · line 1632

Quoted 3 lines
fs/f2fs/data.c +3 -0
+	if (f2fs_has_inline_data(inode) &&
+			is_inode_flag_set(F2FS_I(inode), FI_DATA_EXIST)) {
+		int err = f2fs_write_inline_data(inode, page);

Oh, I'm sure this can fix that issue, but IMO:
a) this implementation has side-effect, it triggers inline data copying between data page and node page whenever user write inline datas, so if user updates inline data frequently, write-through approach would cause memory copy overhead.

Agreed.

Quoted 10 lines

b) inline storm should be a rare case, as we didn't get any report about problem for long time until Dave's, and write_end is a hot path, I think it's better to be cautious to change our inline data cache policy for fixing a rare issue in hot path.

What about delaying the merge operation? like:
1) as I proposed before, merging inline page into inode page when detecting free_sections <= (node_secs + 2 * dent_secs + inline_secs).
2) merge inline page into inode page before writeback inode page in sync_node_pages.

Okay, I'm thinking more general way where we can get rid of every inlien_data write when we flush node pages.

I've been testing this patch.

Quoted 1 line

From ebddf607c64da691fef08cf68a8ecadafd5d896b Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
Date: Mon, 25 Jan 2016 05:57:05 -0800
Subject: [PATCH] f2fs: avoid multiple node page writes due to inline_data

The sceanrio is:
1. create fully node blocks
2. flush node blocks
3. write inline_data for all the node blocks again
4. flush node blocks redundantly

So, this patch tries to flush inline_data when flushing node blocks.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c   |  1 +
 fs/f2fs/inline.c |  2 ++
 fs/f2fs/node.c   | 35 +++++++++++++++++++++++++++++++++++
 fs/f2fs/node.h   | 15 +++++++++++++++
 4 files changed, 53 insertions(+)
fs/f2fs/data.c +1 -0
index 6925c10..9043ecf 100644
@@ -1464,6 +1464,7 @@ restart:
 		if (pos + len <= MAX_INLINE_DATA) {
 			read_inline_data(page, ipage);
 			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
+			set_inline_node(ipage);
 			sync_inode_page(&dn);
 		} else {
 			err = f2fs_convert_inline_page(&dn, page);
fs/f2fs/inline.c +2 -0
index 8df13e5..fc4d298 100644
@@ -159,6 +159,7 @@ no_update:
 
 	/* clear inline data and flag after data writeback */
 	truncate_inline_inode(dn->inode_page, 0);
+	clear_inline_node(dn->inode_page);
 clear_out:
 	stat_dec_inline_inode(dn->inode);
 	f2fs_clear_inline_inode(dn->inode);

@@ -233,6 +234,7 @@ int f2fs_write_inline_data(struct inode *inode, struct page *page)
 	set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
 
 	sync_inode_page(&dn);
+	clear_inline_node(dn.inode_page);
 	f2fs_put_dnode(&dn);
 	return 0;
 }
fs/f2fs/node.c +35 -0
index 23b800d..1c5023e 100644
@@ -1154,6 +1154,33 @@ void sync_inode_page(struct dnode_of_data *dn)
 	dn->node_changed = ret ? true: false;
 }
 
+static void flush_inline_data(struct f2fs_sb_info *sbi, nid_t ino)
+{
+	struct inode *inode;
+	struct page *page;
+
+	inode = ilookup(sbi->sb, ino);
+	if (!inode)
+		return;
+
+	page = find_lock_page(inode->i_mapping, 0);
+	if (!page)
+		goto iput_out;
+
+	if (!PageDirty(page))
+		goto put_page_out;
+
+	if (!clear_page_dirty_for_io(page))
+		goto put_page_out;
+
+	if (!f2fs_write_inline_data(inode, page))
+		inode_dec_dirty_pages(inode);
+put_page_out:
+	f2fs_put_page(page, 1);
+iput_out:
+	iput(inode);
+}
+
 int sync_node_pages(struct f2fs_sb_info *sbi, nid_t ino,
 					struct writeback_control *wbc)
 {

@@ -1221,6 +1248,14 @@ continue_unlock:
 				goto continue_unlock;
 			}
 
+			/* flush inline_data */
+			if (!ino && is_inline_node(page)) {
+				clear_inline_node(page);
+				unlock_page(page);
+				flush_inline_data(sbi, ino_of_node(page));
+				continue;
+			}
+
 			if (!clear_page_dirty_for_io(page))
 				goto continue_unlock;
fs/f2fs/node.h +15 -0
index 23bd992..1f4f9d4 100644
@@ -379,6 +379,21 @@ static inline int is_node(struct page *page, int type)
 #define is_fsync_dnode(page)	is_node(page, FSYNC_BIT_SHIFT)
 #define is_dent_dnode(page)	is_node(page, DENT_BIT_SHIFT)
 
+static inline int is_inline_node(struct page *page)
+{
+	return PageChecked(page);
+}
+
+static inline void set_inline_node(struct page *page)
+{
+	SetPageChecked(page);
+}
+
+static inline void clear_inline_node(struct page *page)
+{
+	ClearPageChecked(page);
+}
+
 static inline void set_cold_node(struct inode *inode, struct page *page)
 {
 	struct f2fs_node *rn = F2FS_NODE(page);
Signature · 1 line
2.6.3
#6 lore raw

Hi Jaegeuk,

Quoted 65 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Tuesday, January 26, 2016 3:18 AM
To: Chao Yu
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH 1/2] f2fs: avoid multiple node page writes due to inline_data

Hi Chao,

Quoted 37 lines · Chao Yu

Hi Jaegeuk,

Quoted 35 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 24, 2016 4:16 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 1/2] f2fs: avoid multiple node page writes due to inline_data

The sceanrio is:
1. create fully node blocks
2. flush node blocks
3. write inline_data for all the node blocks again
4. flush node blocks redundantly

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
fs/f2fs/data.c +0 -3
index 8d0d9ec..011456e 100644
@@ -1622,14 +1622,22 @@ static int f2fs_write_end(struct file *file,
 
 	trace_f2fs_write_end(inode, pos, len, copied);
 
-	set_page_dirty(page);
-
 	if (pos + copied > i_size_read(inode)) {
 		i_size_write(inode, pos + copied);
 		mark_inode_dirty(inode);
-		update_inode_page(inode);
 	}
Quoted 9 lines

fs/f2fs/data.c static int f2fs_write_end(struct file *file, · line 1632

Quoted 3 lines
fs/f2fs/data.c +3 -0
+	if (f2fs_has_inline_data(inode) &&
+			is_inode_flag_set(F2FS_I(inode), FI_DATA_EXIST)) {
+		int err = f2fs_write_inline_data(inode, page);

Oh, I'm sure this can fix that issue, but IMO:
a) this implementation has side-effect, it triggers inline data copying between data page and node page whenever user write inline datas, so if user updates inline data frequently, write-through approach would cause memory copy overhead.

Agreed.

Quoted 5 lines

b) inline storm should be a rare case, as we didn't get any report about problem for long time until Dave's, and write_end is a hot path, I think it's better to be cautious to change our inline data cache policy for fixing a rare issue in hot path.

Quoted 8 lines
Quoted 5 lines

What about delaying the merge operation? like:
1) as I proposed before, merging inline page into inode page when detecting free_sections <= (node_secs + 2 * dent_secs + inline_secs).
2) merge inline page into inode page before writeback inode page in sync_node_pages.

Okay, I'm thinking more general way where we can get rid of every inlien_data write when we flush node pages.

I encountered deadlock issue, could you have a look at it?

======================================================
[ INFO: possible circular locking dependency detected ]
4.5.0-rc1 #45 Tainted: G O
-------------------------------------------------------
fstrim/15301 is trying to acquire lock:
(sb_internal#2){++++..}, at: [<ffffffff81216fca>] __sb_start_write+0xda/0xf0

but task is already holding lock:
(&sbi->cp_rwsem){++++..}, at: [<ffffffffa07d06d2>] block_operations+0x82/0x130 [f2fs]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (&sbi->cp_rwsem){++++..}:

        [<ffffffff810bf827>] lock_acquire+0xb7/0x130
        [<ffffffff817de829>] down_read+0x39/0x50
        [<ffffffffa07c27af>] f2fs_evict_inode+0x26f/0x370 [f2fs]
        [<ffffffff812326cd>] evict+0xdd/0x1d0
        [<ffffffff8123323f>] iput+0x19f/0x250
        [<ffffffff81224d9d>] do_unlinkat+0x20d/0x310
        [<ffffffff81224ee2>] SyS_unlinkat+0x22/0x40
        [<ffffffff817e0957>] entry_SYSCALL_64_fastpath+0x12/0x6f

-> #0 (sb_internal#2){++++..}:

        [<ffffffff810bf32b>] __lock_acquire+0x132b/0x1770
        [<ffffffff810bf827>] lock_acquire+0xb7/0x130
        [<ffffffff810b8fac>] percpu_down_read+0x3c/0x80
        [<ffffffff81216fca>] __sb_start_write+0xda/0xf0
        [<ffffffffa07c2761>] f2fs_evict_inode+0x221/0x370 [f2fs]
        [<ffffffff812326cd>] evict+0xdd/0x1d0
        [<ffffffff8123323f>] iput+0x19f/0x250
        [<ffffffffa07dd4d3>] sync_node_pages+0x703/0x900 [f2fs]
        [<ffffffffa07d075a>] block_operations+0x10a/0x130 [f2fs]
        [<ffffffffa07d13e4>] write_checkpoint+0xc4/0xb80 [f2fs]
        [<ffffffffa07e0af2>] f2fs_trim_fs+0x122/0x1d0 [f2fs]
        [<ffffffffa07c07da>] f2fs_ioctl+0x7fa/0x9d0 [f2fs]
        [<ffffffff81228448>] vfs_ioctl+0x18/0x40
        [<ffffffff81228b96>] do_vfs_ioctl+0x96/0x680
        [<ffffffff81229212>] SyS_ioctl+0x92/0xa0
        [<ffffffff817e0957>] entry_SYSCALL_64_fastpath+0x12/0x6f

other info that might help us debug this:

  Possible unsafe locking scenario:
        CPU0                    CPU1
        ----                    ----
   lock(&sbi->cp_rwsem);
                                lock(sb_internal#2);
                                lock(&sbi->cp_rwsem);

lock(sb_internal#2);

*** DEADLOCK ***

Thanks,

Quoted 138 lines

I've been testing this patch.

From ebddf607c64da691fef08cf68a8ecadafd5d896b Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Mon, 25 Jan 2016 05:57:05 -0800
Subject: [PATCH] f2fs: avoid multiple node page writes due to inline_data

The sceanrio is:
1. create fully node blocks
2. flush node blocks
3. write inline_data for all the node blocks again
4. flush node blocks redundantly

So, this patch tries to flush inline_data when flushing node blocks.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c   |  1 +
 fs/f2fs/inline.c |  2 ++
 fs/f2fs/node.c   | 35 +++++++++++++++++++++++++++++++++++
 fs/f2fs/node.h   | 15 +++++++++++++++
 4 files changed, 53 insertions(+)
fs/f2fs/data.c +1 -0
index 6925c10..9043ecf 100644
@@ -1464,6 +1464,7 @@ restart:
 		if (pos + len <= MAX_INLINE_DATA) {
 			read_inline_data(page, ipage);
 			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
+			set_inline_node(ipage);
 			sync_inode_page(&dn);
 		} else {
 			err = f2fs_convert_inline_page(&dn, page);
fs/f2fs/inline.c +2 -0
index 8df13e5..fc4d298 100644
@@ -159,6 +159,7 @@ no_update:
 
 	/* clear inline data and flag after data writeback */
 	truncate_inline_inode(dn->inode_page, 0);
+	clear_inline_node(dn->inode_page);
 clear_out:
 	stat_dec_inline_inode(dn->inode);
 	f2fs_clear_inline_inode(dn->inode);

@@ -233,6 +234,7 @@ int f2fs_write_inline_data(struct inode *inode, struct page *page)
 	set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
 
 	sync_inode_page(&dn);
+	clear_inline_node(dn.inode_page);
 	f2fs_put_dnode(&dn);
 	return 0;
 }
fs/f2fs/node.c +35 -0
index 23b800d..1c5023e 100644
@@ -1154,6 +1154,33 @@ void sync_inode_page(struct dnode_of_data *dn)
 	dn->node_changed = ret ? true: false;
 }
 
+static void flush_inline_data(struct f2fs_sb_info *sbi, nid_t ino)
+{
+	struct inode *inode;
+	struct page *page;
+
+	inode = ilookup(sbi->sb, ino);
+	if (!inode)
+		return;
+
+	page = find_lock_page(inode->i_mapping, 0);
+	if (!page)
+		goto iput_out;
+
+	if (!PageDirty(page))
+		goto put_page_out;
+
+	if (!clear_page_dirty_for_io(page))
+		goto put_page_out;
+
+	if (!f2fs_write_inline_data(inode, page))
+		inode_dec_dirty_pages(inode);
+put_page_out:
+	f2fs_put_page(page, 1);
+iput_out:
+	iput(inode);
+}
+
 int sync_node_pages(struct f2fs_sb_info *sbi, nid_t ino,
 					struct writeback_control *wbc)
 {

@@ -1221,6 +1248,14 @@ continue_unlock:
 				goto continue_unlock;
 			}
 
+			/* flush inline_data */
+			if (!ino && is_inline_node(page)) {
+				clear_inline_node(page);
+				unlock_page(page);
+				flush_inline_data(sbi, ino_of_node(page));
+				continue;
+			}
+
 			if (!clear_page_dirty_for_io(page))
 				goto continue_unlock;
fs/f2fs/node.h +15 -1
index 23bd992..1f4f9d4 100644
@@ -379,6 +379,21 @@ static inline int is_node(struct page *page, int type)
 #define is_fsync_dnode(page)	is_node(page, FSYNC_BIT_SHIFT)
 #define is_dent_dnode(page)	is_node(page, DENT_BIT_SHIFT)
 
+static inline int is_inline_node(struct page *page)
+{
+	return PageChecked(page);
+}
+
+static inline void set_inline_node(struct page *page)
+{
+	SetPageChecked(page);
+}
+
+static inline void clear_inline_node(struct page *page)
+{
+	ClearPageChecked(page);
+}
+
 static inline void set_cold_node(struct inode *inode, struct page *page)
 {
 	struct f2fs_node *rn = F2FS_NODE(page);
--

2.6.3

author maintainer
#7 lore raw

Hi Chao,

Quoted 73 lines · Chao Yu

Hi Jaegeuk,

Quoted 71 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Tuesday, January 26, 2016 3:18 AM
To: Chao Yu
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH 1/2] f2fs: avoid multiple node page writes due to inline_data

Hi Chao,

Quoted 37 lines · Chao Yu

Hi Jaegeuk,

Quoted 35 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 24, 2016 4:16 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 1/2] f2fs: avoid multiple node page writes due to inline_data

The sceanrio is:
1. create fully node blocks
2. flush node blocks
3. write inline_data for all the node blocks again
4. flush node blocks redundantly

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
fs/f2fs/data.c +0 -3
index 8d0d9ec..011456e 100644
@@ -1622,14 +1622,22 @@ static int f2fs_write_end(struct file *file,
 
 	trace_f2fs_write_end(inode, pos, len, copied);
 
-	set_page_dirty(page);
-
 	if (pos + copied > i_size_read(inode)) {
 		i_size_write(inode, pos + copied);
 		mark_inode_dirty(inode);
-		update_inode_page(inode);
 	}
Quoted 9 lines

fs/f2fs/data.c static int f2fs_write_end(struct file *file, · line 1632

Quoted 3 lines
fs/f2fs/data.c +3 -0
+	if (f2fs_has_inline_data(inode) &&
+			is_inode_flag_set(F2FS_I(inode), FI_DATA_EXIST)) {
+		int err = f2fs_write_inline_data(inode, page);

Oh, I'm sure this can fix that issue, but IMO:
a) this implementation has side-effect, it triggers inline data copying between data page and node page whenever user write inline datas, so if user updates inline data frequently, write-through approach would cause memory copy overhead.

Agreed.

Quoted 10 lines

b) inline storm should be a rare case, as we didn't get any report about problem for long time until Dave's, and write_end is a hot path, I think it's better to be cautious to change our inline data cache policy for fixing a rare issue in hot path.

What about delaying the merge operation? like:
1) as I proposed before, merging inline page into inode page when detecting free_sections <= (node_secs + 2 * dent_secs + inline_secs).
2) merge inline page into inode page before writeback inode page in sync_node_pages.

Quoted 4 lines
Quoted 2 lines

Okay, I'm thinking more general way where we can get rid of every inlien_data write when we flush node pages.

I encountered deadlock issue, could you have a look at it?

Yeah, I've been stablizing this for a while.
Please check f2fs.git/dev-test.

Thanks,

Quoted 198 lines

======================================================
[ INFO: possible circular locking dependency detected ]
4.5.0-rc1 #45 Tainted: G O
-------------------------------------------------------
fstrim/15301 is trying to acquire lock:
(sb_internal#2){++++..}, at: [<ffffffff81216fca>] __sb_start_write+0xda/0xf0

but task is already holding lock:
(&sbi->cp_rwsem){++++..}, at: [<ffffffffa07d06d2>] block_operations+0x82/0x130 [f2fs]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (&sbi->cp_rwsem){++++..}:

        [<ffffffff810bf827>] lock_acquire+0xb7/0x130
        [<ffffffff817de829>] down_read+0x39/0x50
        [<ffffffffa07c27af>] f2fs_evict_inode+0x26f/0x370 [f2fs]
        [<ffffffff812326cd>] evict+0xdd/0x1d0
        [<ffffffff8123323f>] iput+0x19f/0x250
        [<ffffffff81224d9d>] do_unlinkat+0x20d/0x310
        [<ffffffff81224ee2>] SyS_unlinkat+0x22/0x40
        [<ffffffff817e0957>] entry_SYSCALL_64_fastpath+0x12/0x6f

-> #0 (sb_internal#2){++++..}:

        [<ffffffff810bf32b>] __lock_acquire+0x132b/0x1770
        [<ffffffff810bf827>] lock_acquire+0xb7/0x130
        [<ffffffff810b8fac>] percpu_down_read+0x3c/0x80
        [<ffffffff81216fca>] __sb_start_write+0xda/0xf0
        [<ffffffffa07c2761>] f2fs_evict_inode+0x221/0x370 [f2fs]
        [<ffffffff812326cd>] evict+0xdd/0x1d0
        [<ffffffff8123323f>] iput+0x19f/0x250
        [<ffffffffa07dd4d3>] sync_node_pages+0x703/0x900 [f2fs]
        [<ffffffffa07d075a>] block_operations+0x10a/0x130 [f2fs]
        [<ffffffffa07d13e4>] write_checkpoint+0xc4/0xb80 [f2fs]
        [<ffffffffa07e0af2>] f2fs_trim_fs+0x122/0x1d0 [f2fs]
        [<ffffffffa07c07da>] f2fs_ioctl+0x7fa/0x9d0 [f2fs]
        [<ffffffff81228448>] vfs_ioctl+0x18/0x40
        [<ffffffff81228b96>] do_vfs_ioctl+0x96/0x680
        [<ffffffff81229212>] SyS_ioctl+0x92/0xa0
        [<ffffffff817e0957>] entry_SYSCALL_64_fastpath+0x12/0x6f

other info that might help us debug this:

  Possible unsafe locking scenario:
        CPU0                    CPU1
        ----                    ----
   lock(&sbi->cp_rwsem);
                                lock(sb_internal#2);
                                lock(&sbi->cp_rwsem);

lock(sb_internal#2);

*** DEADLOCK ***

Thanks,

Quoted 138 lines

I've been testing this patch.

From ebddf607c64da691fef08cf68a8ecadafd5d896b Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Mon, 25 Jan 2016 05:57:05 -0800
Subject: [PATCH] f2fs: avoid multiple node page writes due to inline_data

The sceanrio is:
1. create fully node blocks
2. flush node blocks
3. write inline_data for all the node blocks again
4. flush node blocks redundantly

So, this patch tries to flush inline_data when flushing node blocks.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c   |  1 +
 fs/f2fs/inline.c |  2 ++
 fs/f2fs/node.c   | 35 +++++++++++++++++++++++++++++++++++
 fs/f2fs/node.h   | 15 +++++++++++++++
 4 files changed, 53 insertions(+)
fs/f2fs/data.c +1 -0
index 6925c10..9043ecf 100644
@@ -1464,6 +1464,7 @@ restart:
 		if (pos + len <= MAX_INLINE_DATA) {
 			read_inline_data(page, ipage);
 			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
+			set_inline_node(ipage);
 			sync_inode_page(&dn);
 		} else {
 			err = f2fs_convert_inline_page(&dn, page);
fs/f2fs/inline.c +2 -0
index 8df13e5..fc4d298 100644
@@ -159,6 +159,7 @@ no_update:
 
 	/* clear inline data and flag after data writeback */
 	truncate_inline_inode(dn->inode_page, 0);
+	clear_inline_node(dn->inode_page);
 clear_out:
 	stat_dec_inline_inode(dn->inode);
 	f2fs_clear_inline_inode(dn->inode);

@@ -233,6 +234,7 @@ int f2fs_write_inline_data(struct inode *inode, struct page *page)
 	set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
 
 	sync_inode_page(&dn);
+	clear_inline_node(dn.inode_page);
 	f2fs_put_dnode(&dn);
 	return 0;
 }
fs/f2fs/node.c +35 -0
index 23b800d..1c5023e 100644
@@ -1154,6 +1154,33 @@ void sync_inode_page(struct dnode_of_data *dn)
 	dn->node_changed = ret ? true: false;
 }
 
+static void flush_inline_data(struct f2fs_sb_info *sbi, nid_t ino)
+{
+	struct inode *inode;
+	struct page *page;
+
+	inode = ilookup(sbi->sb, ino);
+	if (!inode)
+		return;
+
+	page = find_lock_page(inode->i_mapping, 0);
+	if (!page)
+		goto iput_out;
+
+	if (!PageDirty(page))
+		goto put_page_out;
+
+	if (!clear_page_dirty_for_io(page))
+		goto put_page_out;
+
+	if (!f2fs_write_inline_data(inode, page))
+		inode_dec_dirty_pages(inode);
+put_page_out:
+	f2fs_put_page(page, 1);
+iput_out:
+	iput(inode);
+}
+
 int sync_node_pages(struct f2fs_sb_info *sbi, nid_t ino,
 					struct writeback_control *wbc)
 {

@@ -1221,6 +1248,14 @@ continue_unlock:
 				goto continue_unlock;
 			}
 
+			/* flush inline_data */
+			if (!ino && is_inline_node(page)) {
+				clear_inline_node(page);
+				unlock_page(page);
+				flush_inline_data(sbi, ino_of_node(page));
+				continue;
+			}
+
 			if (!clear_page_dirty_for_io(page))
 				goto continue_unlock;
fs/f2fs/node.h +15 -1
index 23bd992..1f4f9d4 100644
@@ -379,6 +379,21 @@ static inline int is_node(struct page *page, int type)
 #define is_fsync_dnode(page)	is_node(page, FSYNC_BIT_SHIFT)
 #define is_dent_dnode(page)	is_node(page, DENT_BIT_SHIFT)
 
+static inline int is_inline_node(struct page *page)
+{
+	return PageChecked(page);
+}
+
+static inline void set_inline_node(struct page *page)
+{
+	SetPageChecked(page);
+}
+
+static inline void clear_inline_node(struct page *page)
+{
+	ClearPageChecked(page);
+}
+
 static inline void set_cold_node(struct inode *inode, struct page *page)
 {
 	struct f2fs_node *rn = F2FS_NODE(page);
--

2.6.3